New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
crypto/x509: Verify returns incorrect chains for some situations #16800
Comments
One possible implementation path to address (A): inside x509.Verify, first filter the incoming opts.Intermediates to see if they actually contain any root certs (self-signed cert: RawSubject == RawIssuer). If so, move these masquerading root certs from opts.Intermediates to opts.Roots before calling x509.buildChains. x509.Verify already scans all opts.Intermediates to ensure they're already parsed (https://golang.org/src/crypto/x509/verify.go?h=opts.Intermediates#L223; BTW: Should opts.Roots be similarly checked when opts.Roots != nil?), so the check for masquerading root certs could be added to that existing loop. However, that loop occurs before opts.Roots are populated when opts.Roots == nil, so the actual move of any masquerading root certs should be deferred until just before the call to isValid (https://golang.org/src/crypto/x509/verify.go?#L252) to avoid disrupting that root cert population logic. That is, I suggest changing the beginning of x509.Verify to something like the following (NOTE: I have not tested on Windows, so I do not know if systemVerify properly handles this case. If not, additional adjustments will need to be made): func (c *Certificate) Verify(opts VerifyOptions) (chains [][]*Certificate, err error) {
// Platform-specific verification needs the ASN.1 contents so
// this makes the behaviour consistent across platforms.
if len(c.Raw) == 0 {
return nil, errNotParsed
}
var trueIntermediates *CertPool
var masqueradingRoots []*Certificate
if opts.Intermediates != nil {
trueIntermediates = NewCertPool()
for _, intermediate := range opts.Intermediates.certs {
if len(intermediate.Raw) == 0 {
return nil, errNotParsed
}
if len(intermediate.RawSubject) > 0 && bytes.Equal(intermediate.RawSubject, intermediate.RawIssuer) {
masqueradingRoots = append(masqueradingRoots, intermediate)
} else {
trueIntermediates.AddCert(intermediate)
}
}
}
// Use Windows's own verification and chain building.
if opts.Roots == nil && runtime.GOOS == "windows" {
return c.systemVerify(&opts)
}
if len(c.UnhandledCriticalExtensions) > 0 {
return nil, UnhandledCriticalExtension{}
}
if opts.Roots == nil {
opts.Roots = systemRootsPool()
if opts.Roots == nil {
return nil, SystemRootsError{systemRootsErr}
}
}
for _, root := range masqueradingRoots {
opts.Roots.AddCert(root)
}
//TODO: Test on Windows; may need to move this before call to c.systemVerify above
opts.Intermediates = trueIntermediates
err = c.isValid(leafCertificate, nil, &opts) This approach avoids changing x509.buildChains, so it's probably less risky, not to mention just easier to understand and actually enforces the long-held assumption that opts.Intermediates only contains intermediate certs. |
CL https://golang.org/cl/32121 mentions this issue. |
What version of Go are you using?
This was first observed with Go 1.5.3, and still exists as of Go 1.7. From cursory historical code inspection, this problem looks like it has existed well before Go 1.5.3.
What operating system and processor architecture are you using?
Linux AMD64
What did you do?
(A) Verifying TLS server cert from a trusted CA hierarchy: https://play.golang.org/p/6L_GA0ZkcD
(B) Verifying TLS server cert that is self-signed: https://play.golang.org/p/ycS8K__AwJ
What did you expect to see?
(A) I expect to see only one verified chain: [Leaf] -> [Intermediate] -> [Root]
(B) I expect to see only one verified chain: [Root]
What did you see instead?
(A) I see two verified chains:
(B) I see one verified chain that is not correct: [Root] -> [Root]
The cause for both (A) and (B) seems to stem from x509.buildChains.
(B) should be fully addressed by yesterday's fix (https://go-review.googlesource.com/#/c/27393/2/src/crypto/x509/verify.go) by @agl for Issue #16763.
The text was updated successfully, but these errors were encountered: