crypto/x509: parsers accept trailing DER inside several X.509 structures

[jvdprng]

Description

Several X.509 parsing paths accept well-formed DER elements after consuming only the fields Go understands, without checking that the nested ASN.1 value was fully consumed. For certificate and CRL extensions, the expected behavior depends on extension criticality and RFC 5280's extensibility rules: some unknown trailing elements in non-critical extensions may be ignorable, while unknown elements in critical extensions require rejecting the containing certificate or CRL.

No higher-level policy split has been shown for the full family. The issue is parser strictness, critical-extension handling, and cross-parser disagreement on signed objects.

Affected subcases:

• BasicConstraints: extra fields inside the BasicConstraints SEQUENCE after the known optional fields are accepted.
• KeyUsage: an extra DER element after the required BIT STRING inside the extension value is accepted.
• Certificate structures: hidden fields inside parsed certificate sequences are accepted.
• CRLs: extra DER around a parsed CRL is accepted.
• Extension envelope: extra fields after extnValue are accepted.
• Private keys: extra DER after PKCS#8 or SEC1 keys is accepted.
• Certificate policies: extra fields inside policy entries or policy mappings are accepted.
• inhibitAnyPolicy: extra DER after the INTEGER is accepted.

Specification

RFC 5280 Appendix B is the key reference for extension trailing data. It describes X.500 extensibility rules for unknown elements inside extensions: for non-critical extensions, unknown elements at the end of SEQUENCEs or in SETs may be ignored in the cases Appendix B describes; if unknown elements appear in an extension marked critical, the containing certificate or CRL must be rejected.

RFC 5280 Section 4.1 defines the certificate structure; Section 4.2 defines certificate extensions; Section 4.2.1.3 defines KeyUsage; Section 4.2.1.4 defines CertificatePolicies; Section 4.2.1.5 defines PolicyMappings; Section 4.2.1.9 defines BasicConstraints; Section 4.2.1.14 defines inhibitAnyPolicy; and Section 5 defines the CRL profile.

RFC 5280 Section 4.2 also states that an extension's extnValue contains the DER encoding of the ASN.1 value corresponding to the extension type. Accepting a complete value followed by additional DER elements inside that extnValue conflicts with treating it as the DER encoding of exactly that extension value.

For private keys, PKCS#8 uses ASN.1 DER private-key structures, now described by RFC 5958 Section 2. SEC1 EC private keys are represented by the ASN.1 structure referenced by Go's ParseECPrivateKey documentation.

Code references

• go/src/crypto/x509/parser.go:428 through go/src/crypto/x509/parser.go:448 parses BasicConstraints without checking that the inner sequence is empty after known fields.
• go/src/crypto/x509/parser.go:413 through go/src/crypto/x509/parser.go:425 parses KeyUsage without checking for remaining DER elements after the BIT STRING.
• go/src/crypto/x509/parser.go:964 through go/src/crypto/x509/parser.go:1014 parses certificate wrapper and TBS fields.
• go/src/crypto/x509/parser.go:1188 through go/src/crypto/x509/parser.go:1373 parses current CRLs.
• go/src/crypto/x509/parser.go:557 through go/src/crypto/x509/parser.go:579 parses certificate policies by reading the OID and ignoring any trailing fields in each policy info sequence.
• go/src/crypto/x509/parser.go:892 through go/src/crypto/x509/parser.go:906 parses policy mappings without checking that each mapping sequence is exhausted.
• go/src/crypto/x509/parser.go:907 through go/src/crypto/x509/parser.go:912 parses inhibitAnyPolicy as an INTEGER without checking for trailing bytes.
• go/src/crypto/x509/pkcs8.go:39 through go/src/crypto/x509/pkcs8.go:49 uses asn1.Unmarshal without checking a non-empty rest.
• go/src/crypto/x509/sec1.go:33 through go/src/crypto/x509/sec1.go:37 routes SEC1 key parsing through parseECPrivateKey.

Test case

This should be a set of X.509 parser regression tests. Each test can mutate an otherwise valid DER object by adding an extra valid DER element at a location where the expected ASN.1 type does not allow additional fields.

For extension test cases, the expected result should distinguish critical from non-critical extensions. A certificate or CRL containing a critical extension with unknown trailing elements should be rejected. Non-critical extensions should be checked against the RFC 5280 Appendix B extensibility cases before treating ignored trailing elements as an error.

These are good candidates for a limbo-style X.509 corpus because they are cross-parser strictness cases.

[gabyhelp]

Related Issues

• crypto/x509: Parser support for extensions #71499 (closed)
• crypto/x509: reject non-profile minimum/maximum in GeneralSubtree name constraints #77826
• encoding/asn1: extra elements at the end of a sequence are completely ignored #35680 (closed)
• crypto/x509: ParsePKIXPublicKey ignores tail of ASN.1 encoding #10583 (closed)
• crypto/x509: reject misencoded empty EKU #79743
• crypto/x509: parseCertificate fails if Cert Policy extension is critical #9964 (closed)
• crypto/x509: ParseCertificate should error on Subject Information Access and Policy Constraints being makred as critical #70278
• crypto/x509: ParseCertificate and ParseCertificateRequest should return an error when there are duplicate x509 extensions #50988 (closed)
• crypto/x509: ParseCertificate should error on invalid tag for PolicyMappings #70074 (closed)
• crypto/x509: ParseCertificate accepts rfc822Name values with empty domains #79856

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[jvdprng]

Playground reproducer: https://go.dev/play/p/DRQ5PyX4zzh

[cherrymui]

cc @golang/security

[rolandshoemaker]

This is a long running question. Ignoring trailing data at the end of a SEQUENCE is the expected behavior for ASN.1 parsers, since you "can" add fields to a SEQUENCE, and the parser should be tolerant of that.

I think it would be fine to not accept that property for critical extensions, since explicitly you are supposed to understand and process all of a critical extension, but I think that is the only place 5280 really overrides the generic ASN.1 rules.