From c2317db2f9bf9f097f0bc297004a8f581b944206 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Mon, 23 Feb 2015 14:27:50 -0800 Subject: [PATCH] crypto/x509: don't reject certs with critical policy extensions. There was a missing continue that caused certificates with critical certificate-policy extensions to be rejected. Additionally, that code structure in general was prone to exactly that bug so I changed it around to hopefully be more robust in the future. Fixes #9964. Change-Id: I58fc6ef3a84c1bd292a35b8b700f44ef312ec1c1 Reviewed-on: https://go-review.googlesource.com/5670 Reviewed-by: Andrew Gerrand --- src/crypto/x509/x509.go | 75 +++++++++++++++++------------------- src/crypto/x509/x509_test.go | 46 ++++++++++++++++++++++ 2 files changed, 81 insertions(+), 40 deletions(-) diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go index be61fd5607..e75120e9f3 100644 --- a/src/crypto/x509/x509.go +++ b/src/crypto/x509/x509.go @@ -891,47 +891,47 @@ func parseCertificate(in *certificate) (*Certificate, error) { for _, e := range in.TBSCertificate.Extensions { out.Extensions = append(out.Extensions, e) + failIfCritical := false if len(e.Id) == 4 && e.Id[0] == 2 && e.Id[1] == 5 && e.Id[2] == 29 { switch e.Id[3] { case 15: // RFC 5280, 4.2.1.3 var usageBits asn1.BitString - _, err := asn1.Unmarshal(e.Value, &usageBits) + if _, err := asn1.Unmarshal(e.Value, &usageBits); err != nil { + return nil, err + } - if err == nil { - var usage int - for i := 0; i < 9; i++ { - if usageBits.At(i) != 0 { - usage |= 1 << uint(i) - } + var usage int + for i := 0; i < 9; i++ { + if usageBits.At(i) != 0 { + usage |= 1 << uint(i) } - out.KeyUsage = KeyUsage(usage) - continue } + out.KeyUsage = KeyUsage(usage) + case 19: // RFC 5280, 4.2.1.9 var constraints basicConstraints - _, err := asn1.Unmarshal(e.Value, &constraints) - - if err == nil { - out.BasicConstraintsValid = true - out.IsCA = constraints.IsCA - out.MaxPathLen = constraints.MaxPathLen - out.MaxPathLenZero = out.MaxPathLen == 0 - continue + if _, err := asn1.Unmarshal(e.Value, &constraints); err != nil { + return nil, err } + + out.BasicConstraintsValid = true + out.IsCA = constraints.IsCA + out.MaxPathLen = constraints.MaxPathLen + out.MaxPathLenZero = out.MaxPathLen == 0 + case 17: out.DNSNames, out.EmailAddresses, out.IPAddresses, err = parseSANExtension(e.Value) if err != nil { return nil, err } - if len(out.DNSNames) > 0 || len(out.EmailAddresses) > 0 || len(out.IPAddresses) > 0 { - continue + if len(out.DNSNames) == 0 && len(out.EmailAddresses) == 0 && len(out.IPAddresses) == 0 { + // If we didn't parse anything then we do the critical check, below. + failIfCritical = true } - // If we didn't parse any of the names then we - // fall through to the critical check below. case 30: // RFC 5280, 4.2.1.10 @@ -950,8 +950,7 @@ func parseCertificate(in *certificate) (*Certificate, error) { // BaseDistance ::= INTEGER (0..MAX) var constraints nameConstraints - _, err := asn1.Unmarshal(e.Value, &constraints) - if err != nil { + if _, err := asn1.Unmarshal(e.Value, &constraints); err != nil { return nil, err } @@ -968,7 +967,6 @@ func parseCertificate(in *certificate) (*Certificate, error) { } out.PermittedDNSDomains = append(out.PermittedDNSDomains, subtree.Name) } - continue case 31: // RFC 5280, 4.2.1.14 @@ -985,15 +983,13 @@ func parseCertificate(in *certificate) (*Certificate, error) { // nameRelativeToCRLIssuer [1] RelativeDistinguishedName } var cdp []distributionPoint - _, err := asn1.Unmarshal(e.Value, &cdp) - if err != nil { + if _, err := asn1.Unmarshal(e.Value, &cdp); err != nil { return nil, err } for _, dp := range cdp { var n asn1.RawValue - _, err = asn1.Unmarshal(dp.DistributionPoint.FullName.Bytes, &n) - if err != nil { + if _, err = asn1.Unmarshal(dp.DistributionPoint.FullName.Bytes, &n); err != nil { return nil, err } @@ -1001,17 +997,14 @@ func parseCertificate(in *certificate) (*Certificate, error) { out.CRLDistributionPoints = append(out.CRLDistributionPoints, string(n.Bytes)) } } - continue case 35: // RFC 5280, 4.2.1.1 var a authKeyId - _, err = asn1.Unmarshal(e.Value, &a) - if err != nil { + if _, err = asn1.Unmarshal(e.Value, &a); err != nil { return nil, err } out.AuthorityKeyId = a.Id - continue case 37: // RFC 5280, 4.2.1.12. Extended Key Usage @@ -1023,8 +1016,7 @@ func parseCertificate(in *certificate) (*Certificate, error) { // KeyPurposeId ::= OBJECT IDENTIFIER var keyUsage []asn1.ObjectIdentifier - _, err = asn1.Unmarshal(e.Value, &keyUsage) - if err != nil { + if _, err = asn1.Unmarshal(e.Value, &keyUsage); err != nil { return nil, err } @@ -1036,17 +1028,13 @@ func parseCertificate(in *certificate) (*Certificate, error) { } } - continue - case 14: // RFC 5280, 4.2.1.2 var keyid []byte - _, err = asn1.Unmarshal(e.Value, &keyid) - if err != nil { + if _, err = asn1.Unmarshal(e.Value, &keyid); err != nil { return nil, err } out.SubjectKeyId = keyid - continue case 32: // RFC 5280 4.2.1.4: Certificate Policies @@ -1058,6 +1046,10 @@ func parseCertificate(in *certificate) (*Certificate, error) { for i, policy := range policies { out.PolicyIdentifiers[i] = policy.Policy } + + default: + // Unknown extensions cause an error if marked as critical. + failIfCritical = true } } else if e.Id.Equal(oidExtensionAuthorityInfoAccess) { // RFC 5280 4.2.2.1: Authority Information Access @@ -1077,9 +1069,12 @@ func parseCertificate(in *certificate) (*Certificate, error) { out.IssuingCertificateURL = append(out.IssuingCertificateURL, string(v.Location.Bytes)) } } + } else { + // Unknown extensions cause an error if marked as critical. + failIfCritical = true } - if e.Critical { + if e.Critical && failIfCritical { return out, UnhandledCriticalExtension{} } } diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go index 45d49ce3e3..011a84c07a 100644 --- a/src/crypto/x509/x509_test.go +++ b/src/crypto/x509/x509_test.go @@ -480,6 +480,52 @@ func TestCreateSelfSignedCertificate(t *testing.T) { } } +func TestUnknownCriticalExtension(t *testing.T) { + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("Failed to generate ECDSA key: %s", err) + } + + oids := []asn1.ObjectIdentifier{ + // This OID is in the PKIX arc, but unknown. + asn1.ObjectIdentifier{2, 5, 29, 999999}, + // This is a nonsense, unassigned OID. + asn1.ObjectIdentifier{1, 2, 3, 4}, + } + + for _, oid := range oids { + template := Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{ + CommonName: "foo", + }, + NotBefore: time.Unix(1000, 0), + NotAfter: time.Unix(100000, 0), + + ExtraExtensions: []pkix.Extension{ + { + Id: oid, + Critical: true, + Value: nil, + }, + }, + } + + derBytes, err := CreateCertificate(rand.Reader, &template, &template, &priv.PublicKey, priv) + if err != nil { + t.Fatalf("failed to create certificate: %s", err) + } + + _, err = ParseCertificate(derBytes) + if err == nil { + t.Fatalf("Certificate with critical extension was parsed without error.") + } + if _, ok := err.(UnhandledCriticalExtension); !ok { + t.Fatalf("Error was %#v, but wanted one of type UnhandledCriticalExtension", err) + } + } +} + // Self-signed certificate using ECDSA with SHA1 & secp256r1 var ecdsaSHA1CertPem = ` -----BEGIN CERTIFICATE----- -- 2.48.1