code review 4657080: crypto/x509: prevent chain cycles in Verify

crypto/x509: prevent chain cycles in Verify

It's possible to include a self-signed root certificate as an
intermediate and push Verify into a loop.

I already had a test for this so I thought that it was ok, but it
turns out that the test was void because the Verisign root certificate
doesn't contain the "IsCA" flag and so it wasn't an acceptable
intermediate certificate for that reason.

[agl1, 2011-07-07 17:59:14]

Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-07-07 18:01:47]

LGTM

On Thu, Jul 7, 2011 at 10:59 AM, <agl@golang.org> wrote:

> Reviewers: bradfitz,
>
> Message:
> Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> crypto/x509: prevent chain cycles in Verify
>
> It's possible to include a self-signed root certificate as an
> intermediate and push Verify into a loop.
>
> I already had a test for this so I thought that it was ok, but it
> turns out that the test was void because the Verisign root certificate
> doesn't contain the "IsCA" flag and so it wasn't an acceptable
> intermediate certificate for that reason.
>
> Please review this at
http://codereview.appspot.com/**4657080/<http://codereview.appspot.com/4657080/>
>
> Affected files:
>  M src/pkg/crypto/x509/verify.go
>  M src/pkg/crypto/x509/verify_**test.go
>
>
> Index: src/pkg/crypto/x509/verify.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/x509/verify.**go
> +++ b/src/pkg/crypto/x509/verify.**go
> @@ -171,8 +171,14 @@
>                chains = append(chains, appendToFreshChain(**currentChain,
> root))
>        }
>
> +nextIntermediate:
>        for _, intermediateNum := range
opts.Intermediates.**findVerifiedParents(c)
> {
>                intermediate := opts.Intermediates.certs[**intermediateNum]
> +               for _, cert := range currentChain {
> +                       if cert == intermediate {
> +                               continue nextIntermediate
> +                       }
> +               }
>                err = intermediate.isValid(**intermediateCertificate, opts)
>                if err != nil {
>                        continue
> Index: src/pkg/crypto/x509/verify_**test.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/x509/verify_**test.go
> +++ b/src/pkg/crypto/x509/verify_**test.go
> @@ -72,16 +72,6 @@
>                },
>        },
>        {
> -               leaf:          googleLeaf,
> -               intermediates: []string{verisignRoot, thawteIntermediate},
> -               roots:         []string{verisignRoot},
> -               currentTime:   1302726541,
> -
> -               expectedChains: [][]string{
> -                       []string{"Google", "Thawte", "VeriSign"},
> -               },
> -       },
> -       {
>                leaf:          dnssecExpLeaf,
>                intermediates: []string{startComIntermediate}**,
>                roots:         []string{startComRoot},
> @@ -91,6 +81,17 @@
>                        []string{"dnssec-exp", "StartCom Class 1", "StartCom
> Certification Authority"},
>                },
>        },
> +       {
> +               leaf:          dnssecExpLeaf,
> +               intermediates: []string{startComIntermediate,
> startComRoot},
> +               roots:         []string{startComRoot},
> +               currentTime:   1302726541,
> +
> +               expectedChains: [][]string{
> +                       []string{"dnssec-exp", "StartCom Class 1",
> "StartCom Certification Authority"},
> +                       []string{"dnssec-exp", "StartCom Class 1",
> "StartCom Certification Authority", "StartCom Certification Authority"},
> +               },
> +       },
>  }
>
>  func expectHostnameError(t *testing.T, i int, err os.Error) (ok bool) {
>
>
>

[agl1, 2011-07-07 22:06:59]

*** Submitted as http://code.google.com/p/go/source/detail?r=8c5c270c9653 ***

crypto/x509: prevent chain cycles in Verify

It's possible to include a self-signed root certificate as an
intermediate and push Verify into a loop.

I already had a test for this so I thought that it was ok, but it
turns out that the test was void because the Verisign root certificate
doesn't contain the "IsCA" flag and so it wasn't an acceptable
intermediate certificate for that reason.

R=bradfitz
CC=golang-dev
http://codereview.appspot.com/4657080