Commit 07a31bc3 authored by Adam Langley's avatar Adam Langley

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: default avatarBrad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
parent a047b6bf
...@@ -346,8 +346,16 @@ func appendToFreshChain(chain []*Certificate, cert *Certificate) []*Certificate ...@@ -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) { func (c *Certificate) buildChains(cache map[int][][]*Certificate, currentChain []*Certificate, opts *VerifyOptions) (chains [][]*Certificate, err error) {
possibleRoots, failedRoot, rootErr := opts.Roots.findVerifiedParents(c) possibleRoots, failedRoot, rootErr := opts.Roots.findVerifiedParents(c)
nextRoot:
for _, rootNum := range possibleRoots { for _, rootNum := range possibleRoots {
root := opts.Roots.certs[rootNum] root := opts.Roots.certs[rootNum]
for _, cert := range currentChain {
if cert.Equal(root) {
continue nextRoot
}
}
err = root.isValid(rootCertificate, currentChain, opts) err = root.isValid(rootCertificate, currentChain, opts)
if err != nil { if err != nil {
continue continue
...@@ -360,7 +368,7 @@ nextIntermediate: ...@@ -360,7 +368,7 @@ nextIntermediate:
for _, intermediateNum := range possibleIntermediates { for _, intermediateNum := range possibleIntermediates {
intermediate := opts.Intermediates.certs[intermediateNum] intermediate := opts.Intermediates.certs[intermediateNum]
for _, cert := range currentChain { for _, cert := range currentChain {
if cert == intermediate { if cert.Equal(intermediate) {
continue nextIntermediate continue nextIntermediate
} }
} }
......
...@@ -104,10 +104,6 @@ var verifyTests = []verifyTest{ ...@@ -104,10 +104,6 @@ var verifyTests = []verifyTest{
expectedChains: [][]string{ expectedChains: [][]string{
{"Google", "Google Internet Authority", "GeoTrust"}, {"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 // CAPI doesn't build the chain with the duplicated GeoTrust
// entry so the results don't match. Thus we skip this test // entry so the results don't match. Thus we skip this test
...@@ -130,12 +126,8 @@ var verifyTests = []verifyTest{ ...@@ -130,12 +126,8 @@ var verifyTests = []verifyTest{
roots: []string{startComRoot}, roots: []string{startComRoot},
currentTime: 1302726541, currentTime: 1302726541,
// Skip when using systemVerify, since Windows
// can only return a single chain to us (for now).
systemSkip: true,
expectedChains: [][]string{ expectedChains: [][]string{
{"dnssec-exp", "StartCom Class 1", "StartCom Certification Authority"}, {"dnssec-exp", "StartCom Class 1", "StartCom Certification Authority"},
{"dnssec-exp", "StartCom Class 1", "StartCom Certification Authority", "StartCom Certification Authority"},
}, },
}, },
{ {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment