-
Notifications
You must be signed in to change notification settings - Fork 17.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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>
- Loading branch information
Showing
2 changed files
with
9 additions
and
9 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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] | ||
|
||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
bradfitz
Contributor
|
||
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 | ||
} | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
@agl Reiterating my comment from Gerrit CR: Is this loop and label needed? Unlike intermediates, there should only be one root. Thus, unless I'm just missing something, it should be sufficient to just compare root to the very last certificate of currentChain.