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/5670Reviewed-by: Andrew Gerrand <adg@golang.org>

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{}
 		}
 	}

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-----