From 07a31bc3da1115775c6607fa400e2d147f6c17c3 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Wed, 26 Oct 2016 13:57:08 -0700 Subject: [PATCH] crypto/x509: don't accept a root that already appears in a chain. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Since a root certificate is self-signed, it's a valid child of itself. If a root certificate appeared both in the pool of intermediates and roots the verification code could find a chain which included it twice: first as an intermediate and then as a root. (Existing checks prevented the code from looping any more.) This change stops the exact same certificate from appearing twice in a chain. This simplifies the results in the face of the common configuration error of a TLS server returning a root certificate. (This should also stop two different versions of the “same” root appearing in a chain because the self-signature on one will not validate for the other.) Fixes #16800. Change-Id: I004853baa0eea27b44d47b9b34f96113a92ebac8 Reviewed-on: https://go-review.googlesource.com/32121 Run-TryBot: Adam Langley Reviewed-by: Brad Fitzpatrick TryBot-Result: Gobot Gobot --- src/crypto/x509/verify.go | 10 +++++++++- src/crypto/x509/verify_test.go | 8 -------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go index aa9e374165..6988ad7871 100644 --- a/src/crypto/x509/verify.go +++ b/src/crypto/x509/verify.go @@ -346,8 +346,16 @@ func appendToFreshChain(chain []*Certificate, cert *Certificate) []*Certificate func (c *Certificate) buildChains(cache map[int][][]*Certificate, currentChain []*Certificate, opts *VerifyOptions) (chains [][]*Certificate, err error) { possibleRoots, failedRoot, rootErr := opts.Roots.findVerifiedParents(c) +nextRoot: for _, rootNum := range possibleRoots { root := opts.Roots.certs[rootNum] + + for _, cert := range currentChain { + if cert.Equal(root) { + continue nextRoot + } + } + err = root.isValid(rootCertificate, currentChain, opts) if err != nil { continue @@ -360,7 +368,7 @@ nextIntermediate: for _, intermediateNum := range possibleIntermediates { intermediate := opts.Intermediates.certs[intermediateNum] for _, cert := range currentChain { - if cert == intermediate { + if cert.Equal(intermediate) { continue nextIntermediate } } diff --git a/src/crypto/x509/verify_test.go b/src/crypto/x509/verify_test.go index 346129219b..5a7481fea1 100644 --- a/src/crypto/x509/verify_test.go +++ b/src/crypto/x509/verify_test.go @@ -104,10 +104,6 @@ var verifyTests = []verifyTest{ expectedChains: [][]string{ {"Google", "Google Internet Authority", "GeoTrust"}, - // TODO(agl): this is ok, but it would be nice if the - // chain building didn't visit the same SPKI - // twice. - {"Google", "Google Internet Authority", "GeoTrust", "GeoTrust"}, }, // CAPI doesn't build the chain with the duplicated GeoTrust // entry so the results don't match. Thus we skip this test @@ -130,12 +126,8 @@ var verifyTests = []verifyTest{ roots: []string{startComRoot}, currentTime: 1302726541, - // Skip when using systemVerify, since Windows - // can only return a single chain to us (for now). - systemSkip: true, expectedChains: [][]string{ {"dnssec-exp", "StartCom Class 1", "StartCom Certification Authority"}, - {"dnssec-exp", "StartCom Class 1", "StartCom Certification Authority", "StartCom Certification Authority"}, }, }, { -- 2.48.1