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/tls: default tls.Config does not seem to verify the server certificate matches the hostname #7342

Closed
gopherbot opened this issue Feb 17, 2014 · 4 comments
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 17, 2014

by willchan@chromium.org:

I'm not sure if I'm reading this correctly, but AFAICT, the default tls.Config does not
verify the certificate matches the server hostname. It seems that the hostname
verification is enabled by setting tls.Config.ServerName. Did I get that right? It seems
non-obvious that tls.Config.ServerName would do this, as the comments say:
"""
// ServerName is included in the client's handshake to support virtual
// hosting.
"""

My expectation on reading that comment was that tls.Config.ServerName only controlled
SNI. But, AFAICT, this is also used for controlling certificate hostname verification:

http://golang.org/src/pkg/crypto/tls/handshake_client.go:
   126 if !c.config.InsecureSkipVerify {
   127 opts := x509.VerifyOptions{
   128 Roots:         c.config.RootCAs,
   129 CurrentTime:   c.config.time(),
   130 DNSName:       c.config.ServerName,
   131 Intermediates: x509.NewCertPool(),
   132 }

So, VerifyOptions{DNSName} comes from tls.Config.ServerName.

http://golang.org/src/pkg/crypto/x509/verify.go:
   228 if len(opts.DNSName) > 0 {
   229 err = c.VerifyHostname(opts.DNSName)
   230 if err != nil {
   231 return
   232 }
   233 }

AFAICT, this means that if tls.Config.ServerName is empty (which I think is the
default), then server host verification is disabled. Am I understanding the code
correctly?

Assuming that's correct, then my next question is that is this intended? It does indeed
make sense that tls.Config.ServerName must be empty by default. That said, it seems like
it'd be nice to encourage people to enable certificate hostname verification, perhaps by
erroring out when it's not enabled and tls.Config.InsecureSkipVerify isn't set. It seems
like an easy error to make (spoken from personal experience).
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 17, 2014

Comment 1:

Adam?

Owner changed to @agl.

Status changed to Accepted.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 17, 2014

Comment 2:

Labels changed: added release-go1.3maybe, repo-main.

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Feb 18, 2014

Comment 3:

The certificates will still be verified when no hostname is provided, but they won't be
checked against any hostname.
However, when using tls.Dial, the hostname will be taken from the server's address if
none is given in the tls.Config. So generally things will Just Work.
Only if tls.Client is used directly and no hostname is given can there be a problem.
Obviously, if a hostname isn't given anywhere then there's nothing that *can* be matched
against and the user of low-level interfaces like that has to take care of it.
Perhaps it should have been a fatal error, but I don't believe that we can break
backwards compatibility. There would also then be the question of how to support code
that does want to do the checking itself (i.e. because it wants to match iOS which I
still believe has non-standard wildcard support.)
So I think this is WaI. tls.Dial should do what you want without surprises.

Status changed to WorkingAsIntended.

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Feb 21, 2014

Comment 4:

This issue was closed by revision fca335e.

Status changed to Fixed.

@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
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
…iven.

crypto/tls has two functions for creating a client connection: Dial,
which most users are expected to use, and Client, which is the
lower-level API.

Dial does what you expect: it gives you a secure connection to the host
that you specify and the majority of users of crypto/tls appear to work
fine with it.

Client gives more control but needs more care. Specifically, if it
wasn't given a server name in the tls.Config then it didn't check that
the server's certificates match any hostname - because it doesn't have
one to check against. It was assumed that users of the low-level API
call VerifyHostname on the certificate themselves if they didn't supply
a hostname.

A review of the uses of Client both within Google and in a couple of
external libraries has shown that nearly all of them got this wrong.

Thus, this change enforces that either a ServerName or
InsecureSkipVerify is given. This does not affect tls.Dial.

See discussion at https://groups.google.com/d/msg/golang-nuts/4vnt7NdLvVU/b1SJ4u0ikb0J.

Fixes golang#7342.

LGTM=bradfitz
R=golang-codereviews, bradfitz
CC=golang-codereviews
https://golang.org/cl/67010043
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.