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: Verify returns incorrect chains for some situations #16800

Closed
ramoas opened this issue Aug 19, 2016 · 2 comments
Closed

crypto/x509: Verify returns incorrect chains for some situations #16800

ramoas opened this issue Aug 19, 2016 · 2 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ramoas
Copy link

ramoas commented Aug 19, 2016

  1. 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.

  2. What operating system and processor architecture are you using?
    Linux AMD64

  3. What did you do?

    (A) Verifying TLS server cert from a trusted CA hierarchy: https://play.golang.org/p/6L_GA0ZkcD

    • This example simulates what happens when a Go TLS client talks to a TLS server that is sending the root cert as part of the server cert's chain. Even though sending the root is not a TLS best practice, such TLS servers are still quite common on the public Internet.

      (B) Verifying TLS server cert that is self-signed: https://play.golang.org/p/ycS8K__AwJ
    • This example simulates what is more likely to happen in an enterprise / private environment. Obviously, the Go TLS client must not be using InsecureSkipVerify. Either the TLS server's self-signed cert must be explicitly set as one of the roots in the options passed to x509.Verify or the system's roots must be modified to include the self-signed cert.
  4. 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]

  5. What did you see instead?

    (A) I see two verified chains:

    1. correct: [Leaf] -> [Intermediate] -> [Root]
    2. *not* correct: [Leaf] -> [Intermediate] -> [Root] -> [Root]


    (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.

@agl agl self-assigned this Aug 19, 2016
@bradfitz bradfitz added this to the Go1.8 milestone Aug 19, 2016
@ramoas
Copy link
Author

ramoas commented Aug 20, 2016

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.

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 5, 2016
@gopherbot
Copy link

CL https://golang.org/cl/32121 mentions this issue.

@golang golang locked and limited conversation to collaborators Oct 27, 2017
@rsc rsc unassigned agl Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants