crypto/x509: Verify returns incorrect chains for some situations

[ramoas]

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.

[ramoas]

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.

[gopherbot]

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