From 0b826cd436711061c1da765e9835c186e94ae655 Mon Sep 17 00:00:00 2001 From: Samuel Giddins Date: Sun, 21 Jun 2026 00:08:14 -0500 Subject: [PATCH] [fix] X509::Certificate#tbs_bytes honors extensions= mutations tbs_bytes re-encoded the original cached TBSCertificate and ignored prior in-place mutation (e.g. cert.extensions=), diverging from MRI's i2d_re_X509_tbs which reflects the certificate's current state. This broke Certificate Transparency / SCT verification, which reconstructs a precertificate TBS by cloning the leaf, removing the ct_precert_scts extension, and signing-verifying over tbs_bytes. tbs_bytes now re-encodes the TBSCertificate from the current state when the cert has been mutated (preserving non-extension fields verbatim and taking the live extension set), and returns the cached TBS otherwise. Also parse extensions in DER order rather than critical-then-non-critical set order, matching MRI's X509#extensions and required for the rebuilt precert TBS to carry extensions in the order the log signed. Verified byte-identical to MRI (OpenSSL 3.5.1) for the unmutated and single-extension-removal cases. --- .../java/org/jruby/ext/openssl/X509Cert.java | 108 ++++++++++++++++-- src/test/ruby/x509/test_x509cert.rb | 68 +++++++++++ 2 files changed, 166 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/jruby/ext/openssl/X509Cert.java b/src/main/java/org/jruby/ext/openssl/X509Cert.java index c998fa67..9e84e354 100644 --- a/src/main/java/org/jruby/ext/openssl/X509Cert.java +++ b/src/main/java/org/jruby/ext/openssl/X509Cert.java @@ -53,15 +53,22 @@ import java.util.Map; import java.util.Set; +import org.bouncycastle.asn1.ASN1BitString; import org.bouncycastle.asn1.ASN1Encodable; import org.bouncycastle.asn1.ASN1EncodableVector; +import org.bouncycastle.asn1.ASN1Encoding; import org.bouncycastle.asn1.ASN1ObjectIdentifier; import org.bouncycastle.asn1.ASN1Sequence; +import org.bouncycastle.asn1.DERBitString; import org.bouncycastle.asn1.DLSequence; +import org.bouncycastle.asn1.x509.Extensions; +import org.bouncycastle.asn1.x509.ExtensionsGenerator; import org.bouncycastle.asn1.x509.GeneralName; import org.bouncycastle.asn1.x509.GeneralNames; import org.bouncycastle.asn1.x509.SubjectPublicKeyInfo; +import org.bouncycastle.asn1.x509.TBSCertificate; +import org.bouncycastle.asn1.x509.V3TBSCertificateGenerator; import org.bouncycastle.cert.X509CertificateHolder; import org.bouncycastle.cert.X509v3CertificateBuilder; import org.bouncycastle.operator.ContentSigner; @@ -239,22 +246,52 @@ private void initialize(final ThreadContext context, final X509Certificate cert) } // "hot" path e.g. sha256WithRSAEncryption this.sig_alg = RubyString.newString(runtime, sigAlgorithm); - final Set criticalExtOIDs = cert.getCriticalExtensionOIDs(); - if ( criticalExtOIDs != null ) { - for ( final String extOID : criticalExtOIDs ) { - addExtension(context, extOID, true); + if ( ! addExtensionsInDERorder(context, cert) ) { + // could not determine the DER order - fall back to the (unordered) + // critical / non-critical extension OID sets + final Set criticalExtOIDs = cert.getCriticalExtensionOIDs(); + if ( criticalExtOIDs != null ) { + for ( final String extOID : criticalExtOIDs ) { + addExtension(context, extOID, true); + } } - } - final Set nonCriticalExtOIDs = cert.getNonCriticalExtensionOIDs(); - if ( nonCriticalExtOIDs != null ) { - for ( final String extOID : nonCriticalExtOIDs ) { - addExtension(context, extOID, false); + final Set nonCriticalExtOIDs = cert.getNonCriticalExtensionOIDs(); + if ( nonCriticalExtOIDs != null ) { + for ( final String extOID : nonCriticalExtOIDs ) { + addExtension(context, extOID, false); + } } } changed = false; } + // Loads the certificate's extensions preserving their DER (on-the-wire) order. + // This matches MRI/OpenSSL (whose X509#extensions follows the encoded order) and + // is required to faithfully re-encode the TBSCertificate from the current set + // (see #tbs_bytes / CT precertificate reconstruction). Returns false if the order + // cannot be determined so the caller can fall back to the critical/non-critical sets. + private boolean addExtensionsInDERorder(final ThreadContext context, final X509Certificate cert) { + final Extensions exts; + try { + // getEncoded() round-trips a freshly parsed cert, so neither call fails in + // practice; the catch is needed because getEncoded() declares the checked + // CertificateEncodingException, plus defensive handling of a malformed + // structure (IllegalArgumentException) for unusual certs passed via wrap(). + exts = org.bouncycastle.asn1.x509.Certificate.getInstance(cert.getEncoded()) + .getTBSCertificate().getExtensions(); + } + catch (CertificateEncodingException|IllegalArgumentException e) { + debugStackTrace(context.runtime, e); + return false; + } + if ( exts == null ) return true; // no extensions present + for ( final ASN1ObjectIdentifier oid : exts.getExtensionOIDs() ) { + addExtension(context, oid.getId(), exts.getExtension(oid).isCritical()); + } + return true; + } + private void addExtension(final ThreadContext context, final String extOID, final boolean critical) { try { @@ -353,11 +390,62 @@ public IRubyObject tbs_bytes() { throw newCertificateError(getRuntime(), "no certificate"); } try { - return StringHelper.newString(getRuntime(), cert.getTBSCertificate()); + // When nothing has been mutated since the certificate was parsed or + // signed, the cached certificate's TBS is exactly the TBSCertificate + // slice of #to_der; otherwise re-encode from the current state. + final byte[] tbs = changed ? rebuildTBSCertificate() : cert.getTBSCertificate(); + return StringHelper.newString(getRuntime(), tbs); } catch (CertificateEncodingException ex) { throw newCertificateError(getRuntime(), ex); } + catch (IOException ex) { + throw newCertificateError(getRuntime(), ex); + } + } + + // Re-encodes the TBSCertificate from the certificate's current state, mirroring + // MRI's i2d_re_X509_tbs which always reflects the live cert -- notably after + // extensions= (relied on by Certificate Transparency precertificate + // reconstruction, which clones the leaf, drops the ct_precert_scts extension and + // signs/verifies over the resulting TBS). The extension set is taken from the + // mutable list the setters modify, in its current order; the remaining fields are + // preserved verbatim from the parsed certificate (consistent with #to_der, which + // likewise only reflects the field setters after a subsequent #sign). + private byte[] rebuildTBSCertificate() throws CertificateEncodingException, IOException { + final org.bouncycastle.asn1.x509.Certificate bcCert = + org.bouncycastle.asn1.x509.Certificate.getInstance(cert.getEncoded()); + final TBSCertificate origTBS = bcCert.getTBSCertificate(); + + final V3TBSCertificateGenerator gen = new V3TBSCertificateGenerator(); + gen.setSerialNumber(origTBS.getSerialNumber()); + gen.setSignature(origTBS.getSignature()); + gen.setIssuer(origTBS.getIssuer()); + gen.setStartDate(origTBS.getStartDate()); + gen.setEndDate(origTBS.getEndDate()); + gen.setSubject(origTBS.getSubject()); + gen.setSubjectPublicKeyInfo(origTBS.getSubjectPublicKeyInfo()); + final ASN1BitString issuerUID = origTBS.getIssuerUniqueId(); + if ( issuerUID != null ) { + gen.setIssuerUniqueID(new DERBitString(issuerUID.getBytes(), issuerUID.getPadBits())); + } + final ASN1BitString subjectUID = origTBS.getSubjectUniqueId(); + if ( subjectUID != null ) { + gen.setSubjectUniqueID(new DERBitString(subjectUID.getBytes(), subjectUID.getPadBits())); + } + + final ExtensionsGenerator extGen = new ExtensionsGenerator(); + for ( final X509Extension ext : uniqueExtensions() ) { + extGen.addExtension(ext.getRealObjectID(), ext.isRealCritical(), ext.getRealValueEncoded()); + } + // When the current set is empty the extensions field is omitted (RFC 5280 + // forbids an empty extensions SEQUENCE); MRI instead emits an empty field, so + // re-encoding a cert with *all* extensions removed differs by those few bytes. + if ( ! extGen.isEmpty() ) { + gen.setExtensions(extGen.generate()); + } + + return gen.generateTBSCertificate().getEncoded(ASN1Encoding.DER); } @Override diff --git a/src/test/ruby/x509/test_x509cert.rb b/src/test/ruby/x509/test_x509cert.rb index 8ef7ecbe..af7fadbf 100644 --- a/src/test/ruby/x509/test_x509cert.rb +++ b/src/test/ruby/x509/test_x509cert.rb @@ -944,4 +944,72 @@ def test_dup_unsigned_cert_deep_copies_names assert_equal "/CN=unsigned/O=mutated", duped.subject.to_s assert_equal "/CN=issuer/O=mutated", duped.issuer.to_s end + + # OpenSSL::X509::Certificate#tbs_bytes returns the DER of the TBSCertificate, + # re-encoded from the certificate's *current* state (mirrors MRI's + # ossl_x509_get_tbs_bytes / i2d_re_X509_tbs). It must reflect prior in-place + # mutation such as extensions= -- this is what Certificate Transparency / SCT + # verification relies on when reconstructing a precertificate TBS. + + def test_tbs_bytes_equals_to_der_tbs_slice + cert = OpenSSL::X509::Certificate.new(read_x509_pem('digicert.pem')) + # the TBSCertificate is the first element of the Certificate SEQUENCE + expected = OpenSSL::ASN1.decode(cert.to_der).value[0].to_der + assert_equal expected, cert.tbs_bytes + end + + def test_tbs_bytes_preserves_der_extension_order + cert = OpenSSL::X509::Certificate.new(read_x509_pem('digicert.pem')) + # extensions must be reported in the certificate's DER order (matches MRI) + assert_equal tbs_extension_oids(cert.tbs_bytes), extension_oids(cert) + end + + def test_tbs_bytes_reflects_extensions_removal + cert = OpenSSL::X509::Certificate.new(read_x509_pem('digicert.pem')) + assert cert.extensions.size > 1 + original = cert.tbs_bytes + + dup = cert.dup + removed = cert.extensions.first.oid + dup.extensions = dup.extensions.reject { |e| e.oid == removed } + + mutated = dup.tbs_bytes + refute_equal original, mutated, 'tbs_bytes must reflect extensions= mutation' + assert mutated.bytesize < original.bytesize, 'removing an extension must shrink tbs_bytes' + + # valid DER of a TBSCertificate (a SEQUENCE) whose extension set drops the + # removed OID while keeping the rest in their original DER order + assert_equal OpenSSL::ASN1::Sequence, OpenSSL::ASN1.decode(mutated).class + expected = extension_oids(cert).reject { |o| o == OpenSSL::ASN1::ObjectId.new(removed).oid } + assert_equal expected, tbs_extension_oids(mutated) + end + + def test_tbs_bytes_unchanged_after_no_op_extensions_assignment + cert = OpenSSL::X509::Certificate.new(read_x509_pem('digicert.pem')) + dup = cert.dup + dup.extensions = dup.extensions # reassign the same set + assert_equal cert.tbs_bytes, dup.tbs_bytes + end + + private + + def read_x509_pem(name) + File.read(File.expand_path(name, File.dirname(__FILE__))) + end + + # Certificate extension OIDs in dotted form (so they compare equal regardless + # of whether #oid yields a short name or a numeric OID). + def extension_oids(cert) + cert.extensions.map { |e| OpenSSL::ASN1::ObjectId.new(e.oid).oid } + end + + # Extract the extension OIDs (in order) from a DER-encoded TBSCertificate. + def tbs_extension_oids(tbs_der) + tbs = OpenSSL::ASN1.decode(tbs_der) + holder = tbs.value.find do |e| + e.respond_to?(:tag) && e.tag == 3 && e.tag_class == :CONTEXT_SPECIFIC + end + return [] unless holder + holder.value[0].value.map { |ext| ext.value[0].oid } + end end