crypto/x509: don't accept a root that already appears in a chain.

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 <agl@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index aa9e37416501f635bb9d53e168817d9d7a152358..6988ad7871fe23b9dd09984390f1a234bdaa7570 100644 (file)
--- 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 346129219b46481e2bc98445b7f6c6d741c729c3..5a7481fea1ddb81cd8aae562226cd8da3e94d917 100644 (file)
--- 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"},
                },
        },
        {