[release-branch.go1.8] crypto/x509: reject intermediates with unknown critical extens...

In https://golang.org/cl/9390 I messed up and put the critical extension
test in the wrong function. Thus it only triggered for leaf certificates
and not for intermediates or roots.

In practice, this is not expected to have a security impact in the web
PKI.

[Merge conflicts resolved in verify_test.go]

Change-Id: I4f2464ef2fb71b5865389901f293062ba1327702
Reviewed-on: https://go-review.googlesource.com/69294
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-on: https://go-review.googlesource.com/70842
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>

diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index 29345a1755c8e1cde125946ae1cd3eb3299256e7..67f9ff530ef04c1117a6bfbc16fe9fb5da7c1a50 100644 (file)
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -191,6 +191,10 @@ func matchNameConstraint(domain, constraint string) bool {
 
 // isValid performs validity checks on the c.
 func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *VerifyOptions) error {
+       if len(c.UnhandledCriticalExtensions) > 0 {
+               return UnhandledCriticalExtension{}
+       }
+
        if len(currentChain) > 0 {
                child := currentChain[len(currentChain)-1]
                if !bytes.Equal(child.RawIssuer, c.RawSubject) {
@@ -279,10 +283,6 @@ func (c *Certificate) Verify(opts VerifyOptions) (chains [][]*Certificate, err e
                return c.systemVerify(&opts)
        }
 
-       if len(c.UnhandledCriticalExtensions) > 0 {
-               return nil, UnhandledCriticalExtension{}
-       }
-
        if opts.Roots == nil {
                opts.Roots = systemRootsPool()
                if opts.Roots == nil {

diff --git a/src/crypto/x509/verify_test.go b/src/crypto/x509/verify_test.go
index 15c40914444075b47c5857a0bb26b38ed68a48ea..63661348ee708d09c3be3a9d34dcb267afb849a4 100644 (file)
--- a/src/crypto/x509/verify_test.go
+++ b/src/crypto/x509/verify_test.go
@@ -263,6 +263,30 @@ var verifyTests = []verifyTest{
 
                errorCallback: expectSubjectIssuerMismatcthError,
        },
+       {
+               // Test that unknown critical extensions in a leaf cause a
+               // verify error.
+               leaf:          criticalExtLeafWithExt,
+               dnsName:       "example.com",
+               intermediates: []string{criticalExtIntermediate},
+               roots:         []string{criticalExtRoot},
+               currentTime:   1486684488,
+               systemSkip:    true,
+
+               errorCallback: expectUnhandledCriticalExtension,
+       },
+       {
+               // Test that unknown critical extensions in an intermediate
+               // cause a verify error.
+               leaf:          criticalExtLeaf,
+               dnsName:       "example.com",
+               intermediates: []string{criticalExtIntermediateWithExt},
+               roots:         []string{criticalExtRoot},
+               currentTime:   1486684488,
+               systemSkip:    true,
+
+               errorCallback: expectUnhandledCriticalExtension,
+       },
 }
 
 func expectHostnameError(t *testing.T, i int, err error) (ok bool) {
@@ -330,6 +354,14 @@ func expectSubjectIssuerMismatcthError(t *testing.T, i int, err error) (ok bool)
        return true
 }
 
+func expectUnhandledCriticalExtension(t *testing.T, i int, err error) (ok bool) {
+       if _, ok := err.(UnhandledCriticalExtension); !ok {
+               t.Errorf("#%d: error was not an UnhandledCriticalExtension: %s", i, err)
+               return false
+       }
+       return true
+}
+
 func certificateFromPEM(pemBytes string) (*Certificate, error) {
        block, _ := pem.Decode([]byte(pemBytes))
        if block == nil {
@@ -1379,3 +1411,67 @@ w67CoNRb81dy+4Q1lGpA8ORoLWh5fIq2t2eNGc4qB8vlTIKiESzAwu7u3sRfuWQi
 4R+gnfLd37FWflMHwztFbVTuNtPOljCX0LN7KcuoXYlr05RhQrmoN7fQHsrZMNLs
 8FVjHdKKu+uPstwd04Uy4BR/H2y1yerN9j/L6ZkMl98iiA==
 -----END CERTIFICATE-----`
+
+const criticalExtRoot = `-----BEGIN CERTIFICATE-----
+MIIBqzCCAVGgAwIBAgIJAJ+mI/85cXApMAoGCCqGSM49BAMCMB0xDDAKBgNVBAoT
+A09yZzENMAsGA1UEAxMEUm9vdDAeFw0xNTAxMDEwMDAwMDBaFw0yNTAxMDEwMDAw
+MDBaMB0xDDAKBgNVBAoTA09yZzENMAsGA1UEAxMEUm9vdDBZMBMGByqGSM49AgEG
+CCqGSM49AwEHA0IABJGp9joiG2QSQA+1FczEDAsWo84rFiP3GTL+n+ugcS6TyNib
+gzMsdbJgVi+a33y0SzLZxB+YvU3/4KTk8yKLC+2jejB4MA4GA1UdDwEB/wQEAwIC
+BDAdBgNVHSUEFjAUBggrBgEFBQcDAQYIKwYBBQUHAwIwDwYDVR0TAQH/BAUwAwEB
+/zAZBgNVHQ4EEgQQQDfXAftAL7gcflQEJ4xZATAbBgNVHSMEFDASgBBAN9cB+0Av
+uBx+VAQnjFkBMAoGCCqGSM49BAMCA0gAMEUCIFeSV00fABFceWR52K+CfIgOHotY
+FizzGiLB47hGwjMuAiEA8e0um2Kr8FPQ4wmFKaTRKHMaZizCGl3m+RG5QsE1KWo=
+-----END CERTIFICATE-----`
+
+const criticalExtIntermediate = `-----BEGIN CERTIFICATE-----
+MIIBszCCAVmgAwIBAgIJAL2kcGZKpzVqMAoGCCqGSM49BAMCMB0xDDAKBgNVBAoT
+A09yZzENMAsGA1UEAxMEUm9vdDAeFw0xNTAxMDEwMDAwMDBaFw0yNTAxMDEwMDAw
+MDBaMCUxDDAKBgNVBAoTA09yZzEVMBMGA1UEAxMMSW50ZXJtZWRpYXRlMFkwEwYH
+KoZIzj0CAQYIKoZIzj0DAQcDQgAESqVq92iPEq01cL4o99WiXDc5GZjpjNlzMS1n
+rk8oHcVDp4tQRRQG3F4A6dF1rn/L923ha3b0fhDLlAvXZB+7EKN6MHgwDgYDVR0P
+AQH/BAQDAgIEMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcDAjAPBgNVHRMB
+Af8EBTADAQH/MBkGA1UdDgQSBBCMGmiotXbbXVd7H40UsgajMBsGA1UdIwQUMBKA
+EEA31wH7QC+4HH5UBCeMWQEwCgYIKoZIzj0EAwIDSAAwRQIhAOhhNRb6KV7h3wbE
+cdap8bojzvUcPD78fbsQPCNw1jPxAiBOeAJhlTwpKn9KHpeJphYSzydj9NqcS26Y
+xXbdbm27KQ==
+-----END CERTIFICATE-----`
+
+const criticalExtLeafWithExt = `-----BEGIN CERTIFICATE-----
+MIIBxTCCAWugAwIBAgIJAJZAUtw5ccb1MAoGCCqGSM49BAMCMCUxDDAKBgNVBAoT
+A09yZzEVMBMGA1UEAxMMSW50ZXJtZWRpYXRlMB4XDTE1MDEwMTAwMDAwMFoXDTI1
+MDEwMTAwMDAwMFowJDEMMAoGA1UEChMDT3JnMRQwEgYDVQQDEwtleGFtcGxlLmNv
+bTBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABF3ABa2+B6gUyg6ayCaRQWYY/+No
+6PceLqEavZNUeVNuz7bS74Toy8I7R3bGMkMgbKpLSPlPTroAATvebTXoBaijgYQw
+gYEwDgYDVR0PAQH/BAQDAgWgMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcD
+AjAMBgNVHRMBAf8EAjAAMBkGA1UdDgQSBBBRNtBL2vq8nCV3qVp7ycxMMBsGA1Ud
+IwQUMBKAEIwaaKi1dttdV3sfjRSyBqMwCgYDUQMEAQH/BAAwCgYIKoZIzj0EAwID
+SAAwRQIgVjy8GBgZFiagexEuDLqtGjIRJQtBcf7lYgf6XFPH1h4CIQCT6nHhGo6E
+I+crEm4P5q72AnA/Iy0m24l7OvLuXObAmg==
+-----END CERTIFICATE-----`
+
+const criticalExtIntermediateWithExt = `-----BEGIN CERTIFICATE-----
+MIIB2TCCAX6gAwIBAgIIQD3NrSZtcUUwCgYIKoZIzj0EAwIwHTEMMAoGA1UEChMD
+T3JnMQ0wCwYDVQQDEwRSb290MB4XDTE1MDEwMTAwMDAwMFoXDTI1MDEwMTAwMDAw
+MFowPTEMMAoGA1UEChMDT3JnMS0wKwYDVQQDEyRJbnRlcm1lZGlhdGUgd2l0aCBD
+cml0aWNhbCBFeHRlbnNpb24wWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAQtnmzH
+mcRm10bdDBnJE7xQEJ25cLCL5okuEphRR0Zneo6+nQZikoh+UBbtt5GV3Dms7LeP
+oF5HOplYDCd8wi/wo4GHMIGEMA4GA1UdDwEB/wQEAwICBDAdBgNVHSUEFjAUBggr
+BgEFBQcDAQYIKwYBBQUHAwIwDwYDVR0TAQH/BAUwAwEB/zAZBgNVHQ4EEgQQKxdv
+UuQZ6sO3XvBsxgNZ3zAbBgNVHSMEFDASgBBAN9cB+0AvuBx+VAQnjFkBMAoGA1ED
+BAEB/wQAMAoGCCqGSM49BAMCA0kAMEYCIQCQzTPd6XKex+OAPsKT/1DsoMsg8vcG
+c2qZ4Q0apT/kvgIhAKu2TnNQMIUdcO0BYQIl+Uhxc78dc9h4lO+YJB47pHGx
+-----END CERTIFICATE-----`
+
+const criticalExtLeaf = `-----BEGIN CERTIFICATE-----
+MIIBzzCCAXWgAwIBAgIJANoWFIlhCI9MMAoGCCqGSM49BAMCMD0xDDAKBgNVBAoT
+A09yZzEtMCsGA1UEAxMkSW50ZXJtZWRpYXRlIHdpdGggQ3JpdGljYWwgRXh0ZW5z
+aW9uMB4XDTE1MDEwMTAwMDAwMFoXDTI1MDEwMTAwMDAwMFowJDEMMAoGA1UEChMD
+T3JnMRQwEgYDVQQDEwtleGFtcGxlLmNvbTBZMBMGByqGSM49AgEGCCqGSM49AwEH
+A0IABG1Lfh8A0Ho2UvZN5H0+ONil9c8jwtC0y0xIZftyQE+Fwr9XwqG3rV2g4M1h
+GnJa9lV9MPHg8+b85Hixm0ZSw7SjdzB1MA4GA1UdDwEB/wQEAwIFoDAdBgNVHSUE
+FjAUBggrBgEFBQcDAQYIKwYBBQUHAwIwDAYDVR0TAQH/BAIwADAZBgNVHQ4EEgQQ
+UNhY4JhezH9gQYqvDMWrWDAbBgNVHSMEFDASgBArF29S5Bnqw7de8GzGA1nfMAoG
+CCqGSM49BAMCA0gAMEUCIQClA3d4tdrDu9Eb5ZBpgyC+fU1xTZB0dKQHz6M5fPZA
+2AIgN96lM+CPGicwhN24uQI6flOsO3H0TJ5lNzBYLtnQtlc=
+-----END CERTIFICATE-----`

diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index b085dad90f06a5f78a21e91d0f4ea5f2ef102522..5e81e9ff5aeeb606a09f112cd55305857741b8f9 100644 (file)
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -518,74 +518,6 @@ 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.
-               {2, 5, 29, 999999},
-               // This is a nonsense, unassigned OID.
-               {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.Now().AddDate(1, 0, 0),
-
-                       BasicConstraintsValid: true,
-                       IsCA: true,
-
-                       KeyUsage:    KeyUsageCertSign,
-                       ExtKeyUsage: []ExtKeyUsage{ExtKeyUsageServerAuth},
-
-                       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)
-               }
-
-               cert, err := ParseCertificate(derBytes)
-               if err != nil {
-                       t.Fatalf("Certificate with unknown critical extension was not parsed: %s", err)
-               }
-
-               roots := NewCertPool()
-               roots.AddCert(cert)
-
-               // Setting Roots ensures that Verify won't delegate to the OS
-               // library and thus the correct error should always be
-               // returned.
-               _, err = cert.Verify(VerifyOptions{Roots: roots})
-               if err == nil {
-                       t.Fatal("Certificate with unknown critical extension was verified without error")
-               }
-               if _, ok := err.(UnhandledCriticalExtension); !ok {
-                       t.Fatalf("Error was %#v, but wanted one of type UnhandledCriticalExtension", err)
-               }
-
-               cert.UnhandledCriticalExtensions = nil
-               if _, err = cert.Verify(VerifyOptions{Roots: roots}); err != nil {
-                       t.Errorf("Certificate failed to verify after unhandled critical extensions were cleared: %s", err)
-               }
-       }
-}
-
 // Self-signed certificate using ECDSA with SHA1 & secp256r1
 var ecdsaSHA1CertPem = `
 -----BEGIN CERTIFICATE-----