]> Cypherpunks repositories - gostls13.git/commitdiff
crypto/x509: don't reject certs with critical policy extensions.
authorAdam Langley <agl@golang.org>
Mon, 23 Feb 2015 22:27:50 +0000 (14:27 -0800)
committerAdam Langley <agl@golang.org>
Tue, 24 Feb 2015 19:36:52 +0000 (19:36 +0000)
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 <adg@golang.org>
src/crypto/x509/x509.go
src/crypto/x509/x509_test.go

index be61fd5607f67aaa76d3551ad90df7372c89eddc..e75120e9f31882131970cf8cee89516fffe690be 100644 (file)
@@ -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{}
                }
        }
index 45d49ce3e37a15a423ee5e151964a7a2e09f0195..011a84c07a2ecac563b05b53c96bec641c929502 100644 (file)
@@ -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-----