Skip to content
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: An invalid certificate chain may be returned by "Certificate.Verify(opts VerifyOptions)" #8029

Closed
gopherbot opened this issue May 19, 2014 · 5 comments
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented May 19, 2014

by us@ulrich-simon.de:

If opts contains at least 2 intermediate certificates and for the first one the
validation process fails, the invalid chain is still added to the returned list.

The error variable will be overwritten in the verification of the 2nd intermediate
certificate.

Thus the function returns err == nil with 2 chains, even though only one is valid.

The missing error handling can be found in:

crypto/x509/verify.go:285-304

nextIntermediate:
        for _, intermediateNum := range possibleIntermediates {
                intermediate := opts.Intermediates.certs[intermediateNum]
                for _, cert := range currentChain {
                        if cert == intermediate {
                                continue nextIntermediate
                        }
                }
                err = intermediate.isValid(intermediateCertificate, currentChain, opts)
                if err != nil {
                        continue
                }
                var childChains [][]*Certificate
                childChains, ok := cache[intermediateNum]
                if !ok {
                        childChains, err = intermediate.buildChains(cache, appendToFreshChain(currentChain, intermediate), opts)
                        cache[intermediateNum] = childChains
                }
                chains = append(chains, childChains...)
        }

In line 300, the returned err in "childChains, err =
intermediate.buildChains(..)" is not handled and might be overwritten in the next
loop in line 293.

Proposed Solution: Insert @line 301: "if err != nil { continue }"

I hope my observation is right and it helps :-) .
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented May 19, 2014

Comment 1:

To agl for review.

Owner changed to @agl.

@rsc
Copy link
Contributor

@rsc rsc commented May 21, 2014

Comment 2:

Labels changed: added release-go1.3maybe.

Status changed to Accepted.

@agl
Copy link
Contributor

@agl agl commented May 22, 2014

Comment 3:

Cert verification is much more complex than I like and I don't exclude the possibility
of a bug in there, but do you have a case where this has gone wrong, or is it based on
inspection?
I'm trying to build a test case where an extra chain can be returned but no luck so far.
In the code, if there's no valid path from an intermediate to a root, then childChains
is intended to be empty. So the append should be a noop (note the "..." in the append
line).
My best guess for triggering a problem was this structure:
leaf -> issuer2 -> issuer2Parent
     -> issuer1 -> root*
Both issuer1 and issuer2 are possible issuers for leaf. issuer2 is listed first and the
chain builds up to another parent, but no root. issuer1 is then considered and does
chain to a trusted root. (Where a trusted certificate is indicated by a *.)
However, this works as expected.
@gopherbot
Copy link
Author

@gopherbot gopherbot commented May 22, 2014

Comment 4 by us@ulrich-simon.de:

This is on inspection and I didn't really recognize the "...".
This "noop append" will wor. The function is built so that there are no chains returned
when there is err != nil.
Even the cache seems to work right if I think it through. Things are very dependend on
each other.
Unfortunatelly there are no source comments. I guess this is the advanced stuff...
Thanks for verifying :) .
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented May 22, 2014

Comment 5:

Status changed to Retracted.

@rsc rsc added this to the Go1.3 milestone Apr 14, 2015
@rsc rsc removed the release-go1.3maybe label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.