Skip to content

Fix X509::Certificate#tbs_bytes to honor extensions= mutations #363

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
segiddins wants to merge 1 commit into from
+166 −10
Closed

Fix X509::Certificate#tbs_bytes to honor extensions= mutations #363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 98 additions & 10 deletions src/main/java/org/jruby/ext/openssl/X509Cert.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<String> 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<String> criticalExtOIDs = cert.getCriticalExtensionOIDs();
if ( criticalExtOIDs != null ) {
for ( final String extOID : criticalExtOIDs ) {
addExtension(context, extOID, true);
}
}
}

final Set<String> nonCriticalExtOIDs = cert.getNonCriticalExtensionOIDs();
if ( nonCriticalExtOIDs != null ) {
for ( final String extOID : nonCriticalExtOIDs ) {
addExtension(context, extOID, false);
final Set<String> 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 {
Expand Down Expand Up @@ -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
Expand Down
68 changes: 68 additions & 0 deletions src/test/ruby/x509/test_x509cert.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading