crypto/x509: properly apply name constrains to roots and intermediates

Name constraints are checked during path building. When a new
certificate is considered for inclusion in a chain we check if it has
name constraints, and if it does, check that they apply to the certs
already in the chain, discarding it if the current chain violates any
of the constraints the candidate introduces.

This check was not acting as intended in two ways. The first was that
we only checked that the constraints on the candidate certificate
applied to the leaf certificate, and not the rest of the certiifcates in
the chain. This was the intended behavior pre-1.19, but in 1.19 we
intended for the constraints to be applied to the entire chain (although
obviously they were not).

The second was that we checked that the candidates constraints applied
to the candidate itself. This is not conformant with RFC 5280, which
says that during path building the constraint should only be applied to
the certificates which follow the certificate which introduces the
constraint (e.g. in the chain A -> B -> C, if certificate Bcontains a
name constraint, the constraint should only apply to certificate C).

The intended behavior introduced in 1.19 was mainly intended to reject
dubious chains which the WebPKI disallows, and are relatively rare, but
don't have significant security impact. Since the constraints were
properly applied to the leaf certificate, there should be no real impact
to the majority of users.

Fixes #59171

Change-Id: Ie6def55b8ab7f14d6ed2c09351f664e148a4160d
Reviewed-on: https://go-review.googlesource.com/c/go/+/478216
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Roland Shoemaker <roland@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>

diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index 8f9610f8e6d8efcf73e2e74dbccf6c48b204de21..345d434453c44b98b00be4899862c57d1c8973a1 100644 (file)
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -591,22 +591,19 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V
        }
        comparisonCount := 0
 
-       var leaf *Certificate
        if certType == intermediateCertificate || certType == rootCertificate {
                if len(currentChain) == 0 {
                        return errors.New("x509: internal error: empty chain when appending CA cert")
                }
-               leaf = currentChain[0]
        }
 
        if (certType == intermediateCertificate || certType == rootCertificate) &&
                c.hasNameConstraints() {
                toCheck := []*Certificate{}
-               if leaf.hasSANExtension() {
-                       toCheck = append(toCheck, leaf)
-               }
-               if c.hasSANExtension() {
-                       toCheck = append(toCheck, c)
+               for _, c := range currentChain {
+                       if c.hasSANExtension() {
+                               toCheck = append(toCheck, c)
+                       }
                }
                for _, sanCert := range toCheck {
                        err := forEachSAN(sanCert.getSANExtension(), func(tag int, data []byte) error {

diff --git a/src/crypto/x509/verify_test.go b/src/crypto/x509/verify_test.go
index 988b17e15d4ac7a0674fc0d2b78c12f67917f55b..fdb20c0887b95bde14b43a005ad9f2aa75e3c57e 100644 (file)
--- a/src/crypto/x509/verify_test.go
+++ b/src/crypto/x509/verify_test.go
@@ -1921,8 +1921,13 @@ type trustGraphEdge struct {
        MutateTemplate func(*Certificate)
 }
 
+type rootDescription struct {
+       Subject        string
+       MutateTemplate func(*Certificate)
+}
+
 type trustGraphDescription struct {
-       Roots []string
+       Roots []rootDescription
        Leaf  string
        Graph []trustGraphEdge
 }
@@ -1977,10 +1982,10 @@ func buildTrustGraph(t *testing.T, d trustGraphDescription) (*CertPool, *CertPoo
                if err != nil {
                        t.Fatalf("failed to generate test key: %s", err)
                }
-               root := genCertEdge(t, r, k, nil, rootCertificate, nil, nil)
+               root := genCertEdge(t, r.Subject, k, r.MutateTemplate, rootCertificate, nil, nil)
                roots = append(roots, root)
-               certs[r] = root
-               keys[r] = k
+               certs[r.Subject] = root
+               keys[r.Subject] = k
        }
 
        intermediates := []*Certificate{}
@@ -2072,7 +2077,7 @@ func TestPathBuilding(t *testing.T) {
                        //       +----+
                        name: "bad EKU",
                        graph: trustGraphDescription{
-                               Roots: []string{"root"},
+                               Roots: []rootDescription{{Subject: "root"}},
                                Leaf:  "leaf",
                                Graph: []trustGraphEdge{
                                        {
@@ -2148,7 +2153,7 @@ func TestPathBuilding(t *testing.T) {
                        //       +----+
                        name: "bad EKU",
                        graph: trustGraphDescription{
-                               Roots: []string{"root"},
+                               Roots: []rootDescription{{Subject: "root"}},
                                Leaf:  "leaf",
                                Graph: []trustGraphEdge{
                                        {
@@ -2230,7 +2235,7 @@ func TestPathBuilding(t *testing.T) {
                        //            +----+
                        name: "all paths",
                        graph: trustGraphDescription{
-                               Roots: []string{"root"},
+                               Roots: []rootDescription{{Subject: "root"}},
                                Leaf:  "leaf",
                                Graph: []trustGraphEdge{
                                        {
@@ -2294,7 +2299,7 @@ func TestPathBuilding(t *testing.T) {
                        //       +----+
                        name: "ignore cross-sig loops",
                        graph: trustGraphDescription{
-                               Roots: []string{"root"},
+                               Roots: []rootDescription{{Subject: "root"}},
                                Leaf:  "leaf",
                                Graph: []trustGraphEdge{
                                        {
@@ -2347,7 +2352,7 @@ func TestPathBuilding(t *testing.T) {
                        // the leaf has SANs.
                        name: "leaf with same subject, key, as parent but with SAN",
                        graph: trustGraphDescription{
-                               Roots: []string{"root"},
+                               Roots: []rootDescription{{Subject: "root"}},
                                Leaf:  "root",
                                Graph: []trustGraphEdge{
                                        {
@@ -2369,7 +2374,7 @@ func TestPathBuilding(t *testing.T) {
                        // through C should be ignored, because it has invalid EKU nesting.
                        name: "ignore invalid EKU path",
                        graph: trustGraphDescription{
-                               Roots: []string{"root"},
+                               Roots: []rootDescription{{Subject: "root"}},
                                Leaf:  "leaf",
                                Graph: []trustGraphEdge{
                                        {
@@ -2412,6 +2417,70 @@ func TestPathBuilding(t *testing.T) {
                                "CN=leaf -> CN=inter b -> CN=inter a -> CN=root",
                        },
                },
+               {
+                       // A name constraint on the root should apply to any names that appear
+                       // on the intermediate, meaning there is no valid chain.
+                       name: "contrained root, invalid intermediate",
+                       graph: trustGraphDescription{
+                               Roots: []rootDescription{
+                                       {
+                                               Subject: "root",
+                                               MutateTemplate: func(t *Certificate) {
+                                                       t.PermittedDNSDomains = []string{"example.com"}
+                                               },
+                                       },
+                               },
+                               Leaf: "leaf",
+                               Graph: []trustGraphEdge{
+                                       {
+                                               Issuer:  "root",
+                                               Subject: "inter",
+                                               Type:    intermediateCertificate,
+                                               MutateTemplate: func(t *Certificate) {
+                                                       t.DNSNames = []string{"beep.com"}
+                                               },
+                                       },
+                                       {
+                                               Issuer:  "inter",
+                                               Subject: "leaf",
+                                               Type:    leafCertificate,
+                                               MutateTemplate: func(t *Certificate) {
+                                                       t.DNSNames = []string{"www.example.com"}
+                                               },
+                                       },
+                               },
+                       },
+                       expectedErr: "x509: a root or intermediate certificate is not authorized to sign for this name: DNS name \"beep.com\" is not permitted by any constraint",
+               },
+               {
+                       // A name constraint on the intermediate does not apply to the intermediate
+                       // itself, so this is a valid chain.
+                       name: "contrained intermediate, non-matching SAN",
+                       graph: trustGraphDescription{
+                               Roots: []rootDescription{{Subject: "root"}},
+                               Leaf:  "leaf",
+                               Graph: []trustGraphEdge{
+                                       {
+                                               Issuer:  "root",
+                                               Subject: "inter",
+                                               Type:    intermediateCertificate,
+                                               MutateTemplate: func(t *Certificate) {
+                                                       t.DNSNames = []string{"beep.com"}
+                                                       t.PermittedDNSDomains = []string{"example.com"}
+                                               },
+                                       },
+                                       {
+                                               Issuer:  "inter",
+                                               Subject: "leaf",
+                                               Type:    leafCertificate,
+                                               MutateTemplate: func(t *Certificate) {
+                                                       t.DNSNames = []string{"www.example.com"}
+                                               },
+                                       },
+                               },
+                       },
+                       expectedChains: []string{"CN=leaf -> CN=inter -> CN=root"},
+               },
        }
 
        for _, tc := range tests {
@@ -2424,6 +2493,9 @@ func TestPathBuilding(t *testing.T) {
                        if err != nil && err.Error() != tc.expectedErr {
                                t.Fatalf("unexpected error: got %q, want %q", err, tc.expectedErr)
                        }
+                       if len(tc.expectedChains) == 0 {
+                               return
+                       }
                        gotChains := chainsToStrings(chains)
                        if !reflect.DeepEqual(gotChains, tc.expectedChains) {
                                t.Errorf("unexpected chains returned:\ngot:\n\t%s\nwant:\n\t%s", strings.Join(gotChains, "\n\t"), strings.Join(tc.expectedChains, "\n\t"))