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 <[email protected]>
Auto-Submit: Roland Shoemaker <[email protected]>
Run-TryBot: Roland Shoemaker <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: rolandshoemaker

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 {

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