code review 7277051: crypto/x509: reject IP address in VerifyHostname

crypto/x509: reject IP address in VerifyHostname

New dependency: x509 depends on net for checking whether the
host name is in the form of a valid IP address. Since the primary
client of x509 is crypto/tls, and the latter already depends on net,
no big change.

Fixes issue 4658.

[rsc, 2013-02-04 05:08:39]

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

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2013-02-04 05:12:39]

LGTM

On Sun, Feb 3, 2013 at 9:08 PM, <rsc@golang.org> wrote:

> Reviewers: agl1,
>
> Message:
> Hello agl1 (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> crypto/x509: reject IP address in VerifyHostname
>
> New dependency: x509 depends on net for checking whether the
> host name is in the form of a valid IP address. Since the primary
> client of x509 is crypto/tls, and the latter already depends on net,
> no big change.
>
> Fixes issue 4658.
>
> Please review this at
https://codereview.appspot.**com/7277051/<https://codereview.appspot.com/7277...
>
> Affected files:
>   M src/pkg/crypto/x509/verify.go
>   M src/pkg/crypto/x509/x509_test.**go
>   M src/pkg/go/build/deps_test.go
>
>
> Index: src/pkg/crypto/x509/verify.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/x509/verify.**go
> +++ b/src/pkg/crypto/x509/verify.**go
> @@ -5,6 +5,7 @@
>  package x509
>
>  import (
> +       "net"
>         "runtime"
>         "strings"
>         "time"
> @@ -334,6 +335,13 @@
>  // VerifyHostname returns nil if c is a valid certificate for the named
> host.
>  // Otherwise it returns an error describing the mismatch.
>  func (c *Certificate) VerifyHostname(h string) error {
> +       ip := net.ParseIP(h)
> +       if ip != nil {
> +               // IP addresses must not be matched against DNS names.
> +               // See issue 4658.
> +               return HostnameError{c, h}
> +       }
> +
>         lowered := toLowerCaseASCII(h)
>
>         if len(c.DNSNames) > 0 {
> Index: src/pkg/crypto/x509/x509_test.**go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/x509/x509_**test.go
> +++ b/src/pkg/crypto/x509/x509_**test.go
> @@ -174,6 +174,33 @@
>         }
>  }
>
> +func TestMatchIP(t *testing.T) {
> +       // Check that pattern matching is working.
> +       c := &Certificate{
> +               DNSNames: []string{"*.foo.bar.baz"},
> +               Subject: pkix.Name{
> +                       CommonName: "*.foo.bar.baz",
> +               },
> +       }
> +       err := c.VerifyHostname("quux.foo.**bar.baz")
> +       if err != nil {
> +               t.Fatalf("VerifyHostname(quux.**foo.bar.baz): %v", err)
> +       }
> +
> +       // But check that if we change it to be matching against an IP
> address,
> +       // it is rejected.
> +       c = &Certificate{
> +               DNSNames: []string{"*.2.3.4"},
> +               Subject: pkix.Name{
> +                       CommonName: "*.2.3.4",
> +               },
> +       }
> +       err = c.VerifyHostname("1.2.3.4")
> +       if err == nil {
> +               t.Fatalf("VerifyHostname(1.2.**3.4) should have failed,
> did not")
> +       }
> +}
> +
>  func TestCertificateParse(t *testing.T) {
>         s, _ := hex.DecodeString(certBytes)
>         certs, err := ParseCertificates(s)
> Index: src/pkg/go/build/deps_test.go
> ==============================**==============================**=======
> --- a/src/pkg/go/build/deps_test.**go
> +++ b/src/pkg/go/build/deps_test.**go
> @@ -306,7 +306,7 @@
>         },
>         "crypto/x509": {
>                 "L4", "CRYPTO-MATH", "OS", "CGO",
> -               "crypto/x509/pkix", "encoding/pem", "encoding/hex",
> "syscall",
> +               "crypto/x509/pkix", "encoding/pem", "encoding/hex", "net",
> "syscall",
>         },
>         "crypto/x509/pkix": {"L4", "CRYPTO-MATH"},
>
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[rsc, 2013-02-04 05:18:39]

Too bad this breaks net/http's tests:

--- FAIL: TestClientWithCorrectTLSServerName (0.02 seconds)
client_test.go:625: expected successful TLS connection, got error: Get
https://127.0.0.1:65091: certificate is valid for 127.0.0.1, [::1], not
127.0.0.1
FAIL
FAIL net/http 4.095s

I assume the http test is depending on exactly the bug we're trying to kill?

[bradfitz, 2013-02-04 05:21:35]

The net/http/httptest certificate IIRC has a subject of "127.0.0.1".

I thought this bug was only about not matching *wildcards* against IP
addresses.

In this failing net/http test, no wildcard is involved.


On Sun, Feb 3, 2013 at 9:18 PM, Russ Cox <rsc@golang.org> wrote:

> Too bad this breaks net/http's tests:
>
> --- FAIL: TestClientWithCorrectTLSServerName (0.02 seconds)
> client_test.go:625: expected successful TLS connection, got error: Get
> https://127.0.0.1:65091: certificate is valid for 127.0.0.1, [::1], not
> 127.0.0.1
> FAIL
> FAIL net/http 4.095s
>
> I assume the http test is depending on exactly the bug we're trying to
> kill?
>
>

[agl1, 2013-02-04 20:17:44]

This change is probably the right thing to do - although perhaps we should add
IP SAN support in the same change.

Although, as Brad notes, the bug was only against matching IP addresses against
wildcards. i.e. 1.2.3.4 shouldn't match against *.2.3.4.

If you're thinking "WTF are IP SANs?" then I'm happy to take over this bug.

[bradfitz, 2013-02-04 20:46:25]

On Mon, Feb 4, 2013 at 12:17 PM, <agl@golang.org> wrote:

> This change is probably the right thing to do - although perhaps we
> should add IP SAN support in the same change.
>
> Although, as Brad notes, the bug was only against matching IP addresses
> against wildcards. i.e. 1.2.3.4 shouldn't match against *.2.3.4.
>
> If you're thinking "WTF are IP SANs?" then I'm happy to take over this
> bug.
>

As long as that doesn't involve a new iSCSI implementation in the standard
library.

[mikio, 2013-02-04 23:40:44]

I'm a bit confused. Are you guys talking about: 
Subject Alternative Names for SSL or
Storage Area Networks or
both?

[agl1, 2013-02-04 23:42:47]

On Mon, Feb 4, 2013 at 6:40 PM,  <mikioh.mikioh@gmail.com> wrote:
> I'm a bit confused. Are you guys talking about:
> Subject Alternative Names for SSL or

Subject Alternative Names only :)


Cheers

AGL

[rsc, 2013-02-05 01:36:07]

It's all yours if you have time.

Thanks.
Russ

[agl1, 2013-02-11 19:42:11]

On second thoughts, I like this change and think it should be submitted as is.

The reason that I hesitated is that some people actually have certificates for
IP addresses, as silly as that is. However, the CAs should be phasing that out
and there are plenty of other corners of X.509 that we omit for sanity's sake.
I'm happy with this being one of them.

[bradfitz, 2013-02-11 19:44:07]

It can't be submitted as-is, as it breaks the net/http tests, since
net/http/httptest relies on certs for IP addresses (127.0.0.1).

What do you propose?


On Mon, Feb 11, 2013 at 11:42 AM, <agl@golang.org> wrote:

> On second thoughts, I like this change and think it should be submitted
> as is.
>
> The reason that I hesitated is that some people actually have
> certificates for IP addresses, as silly as that is. However, the CAs
> should be phasing that out and there are plenty of other corners of
> X.509 that we omit for sanity's sake. I'm happy with this being one of
> them.
>
>
https://codereview.appspot.**com/7277051/<https://codereview.appspot.com/7277...
>

[agl1, 2013-02-11 20:29:56]

On Mon, Feb 11, 2013 at 2:44 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> It can't be submitted as-is, as it breaks the net/http tests, since
> net/http/httptest relies on certs for IP addresses (127.0.0.1).

Oh nuts, missed that.

Can we depend on "localhost" working in Go tests?


Cheers

AGL

[bradfitz, 2013-02-11 20:32:20]

On Mon, Feb 11, 2013 at 12:29 PM, Adam Langley <agl@golang.org> wrote:

> On Mon, Feb 11, 2013 at 2:44 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > It can't be submitted as-is, as it breaks the net/http tests, since
> > net/http/httptest relies on certs for IP addresses (127.0.0.1).
>
> Oh nuts, missed that.
>
> Can we depend on "localhost" working in Go tests?
>

No.  But we could fake the http.Transport.Dialer to special-case "DNS" for
a name.  But if we do that, I'd rather be explicit and make that magic name
and magic cert be for "go-test-suite.example.com", rather than "localhost",
to root out tests accidentally getting away with "localhost".

[bradfitz, 2013-02-12 01:49:14]

On Mon, Feb 11, 2013 at 12:32 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

>
>
> On Mon, Feb 11, 2013 at 12:29 PM, Adam Langley <agl@golang.org> wrote:
>
>> On Mon, Feb 11, 2013 at 2:44 PM, Brad Fitzpatrick <bradfitz@golang.org>
>> wrote:
>> > It can't be submitted as-is, as it breaks the net/http tests, since
>> > net/http/httptest relies on certs for IP addresses (127.0.0.1).
>>
>> Oh nuts, missed that.
>>
>> Can we depend on "localhost" working in Go tests?
>>
>
> No.  But we could fake the http.Transport.Dialer to special-case "DNS" for
> a name.  But if we do that, I'd rather be explicit and make that magic name
> and magic cert be for "go-test-suite.example.com", rather than
> "localhost", to root out tests accidentally getting away with "localhost".
>

I've created  https://codereview.appspot.com/7307098 for this approach, but
it doesn't yet work.  A couple tests fail with:

--- FAIL: TestClientWithCorrectTLSServerName (0.06 seconds)
client_test.go:607: expected successful TLS connection, got error: Get
https://go-local-test: x509: certificate signed by unknown authority
--- FAIL: TestNextProtoUpgrade (0.06 seconds)
npn_test.go:49: Get https://go-local-test: x509: certificate signed by
unknown authority
--- FAIL: TestTLSServer (0.05 seconds)
serve_test.go:695: Get https://go-local-test: x509: certificate signed by
unknown authority

Maybe I generated the cert wrong?  I want it to be self-signed.  I added
that cert to the RootCAs list as used by the Transport.

Any clues?

[richmoore44, 2013-02-12 17:26:21]

I'd suggest you take the approach of allowing IP addresses in DNS subject
alternative names, but disallowing wildcards if the user has asked to connect an
IP address rather than a domainname. This is the approach other people have
taken to fixing the issue. It is still common to see certificates with IP
addresses in the subjectAltNames when looking at certificates automatically
generated by firewalls etc.

[bradfitz, 2013-02-12 17:47:35]

That's what I thought "we" were going to do too, but agl changed his mind.
 I don't claim to understand the pros and cons to either approach.

On Tue, Feb 12, 2013 at 9:26 AM, <richmoore44@gmail.com> wrote:

> I'd suggest you take the approach of allowing IP addresses in DNS
> subject alternative names, but disallowing wildcards if the user has
> asked to connect an IP address rather than a domainname. This is the
> approach other people have taken to fixing the issue. It is still common
> to see certificates with IP addresses in the subjectAltNames when
> looking at certificates automatically generated by firewalls etc.
>
>
>
https://codereview.appspot.**com/7277051/<https://codereview.appspot.com/7277...
>

[agl1, 2013-02-12 19:04:01]

On Tue, Feb 12, 2013 at 12:47 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> That's what I thought "we" were going to do too, but agl changed his mind.
> I don't claim to understand the pros and cons to either approach.

I didn't know that the http tests would be a problem. In light of
that, supporting IP SANs seems like the best approach and I'll do
that.


Cheers

AGL

[mikio, 2013-02-12 23:56:29]

On 2013/02/12 19:04:01, agl1 wrote:

> In light of that, supporting IP SANs seems like the best approach and I'll do
that.

yay!

[bradfitz, 2013-02-24 19:04:29]

This can be closed now.

[rsc, 2013-02-25 22:01:27]

Discarded by hand.