From ac17bb6f13979f2ab9fcd45f0758b43ed72d0973 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Tue, 21 Mar 2023 13:45:18 -0700 Subject: [PATCH] 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 Auto-Submit: Roland Shoemaker Run-TryBot: Roland Shoemaker TryBot-Result: Gopher Robot --- src/crypto/x509/verify.go | 11 ++-- src/crypto/x509/verify_test.go | 92 ++++++++++++++++++++++++++++++---- 2 files changed, 86 insertions(+), 17 deletions(-) diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go index 8f9610f8e6..345d434453 100644 --- 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 988b17e15d..fdb20c0887 100644 --- 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")) -- 2.48.1