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: Wildcard Cerficate Validation Weakness #4658

Closed
gopherbot opened this issue Jan 14, 2013 · 8 comments
Closed

crypto/x509: Wildcard Cerficate Validation Weakness #4658

gopherbot opened this issue Jan 14, 2013 · 8 comments
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 14, 2013

by richmoore44:

Taking a look at the code for certificate verification, the hostname matching function
appears to allow wildcards in IP addresses. I previously reported this issue against FF,
Chrome etc. back in 2010. The code that appears vulnerable can be seen at:
http://code.google.com/p/go/source/browse/src/pkg/crypto/x509/verify.go#237

http://www.westpoint.ltd.uk/advisories/wp-10-0001.txt
@agl
Copy link
Contributor

@agl agl commented Jan 14, 2013

Comment 2:

The Go code ignores IP address SANs, so the '*.2.3.4' form would have to appear in the
CN or as a DNSName SAN.
If the user sets the `hostname' of a TLS connection to the string form of an IP address,
then we will match it against such a wildcard. However, setting the hostname to an IP
address isn't supported.
So I think this bug boils down to "we should try parsing the hostname that the user
asked us to verify as an IP address and fail immediately if it looks like one". Do you
agree?
If, in the future, we support IP addresses like that, we would need to be careful to
match only against IP address SANs, but we don't currently support that at all.

Status changed to WaitingForReply.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jan 14, 2013

Comment 3 by richmoore44:

Yes, the fix that I applied for Qt is that if you're asked to connect to an IP address
(IPv4 or IPv6) then do not process wildcards. I believe the FF team took the same
approach.
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jan 25, 2013

Comment 4 by richmoore44:

I'm unsure why this still has the status waiting for reply. Is there more information
you need?
@agl
Copy link
Contributor

@agl agl commented Jan 25, 2013

Comment 5:

Indeed, sorry. It's not waiting-for-reply, it's just waiting-for-time. Changed to
"Accepted".

Status changed to Accepted.

@rsc
Copy link
Contributor

@rsc rsc commented Jan 30, 2013

Comment 6:

Labels changed: added priority-later, removed priority-triage.

@rsc
Copy link
Contributor

@rsc rsc commented Feb 4, 2013

Comment 7:

Status changed to Started.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Feb 12, 2013

Comment 8:

Note that https://golang.org/cl/7277051/ is effectively a Go 1 API regression,
since it breaks anybody using net/http/httptest's NewTLSServer and trying to test
against it with httptest.(*Server).URL.
Could we keep CL 7277051 but white-list certs using "127.0.0.1" or "[::1]" as subjects? 
That would keep Go 1 users' tests still working.  Is that a security problem for any
reason?
@agl
Copy link
Contributor

@agl agl commented Feb 15, 2013

Comment 9:

This issue was closed by revision 5b20a18.

Status changed to Fixed.

@rsc rsc added this to the Go1.1 milestone Apr 14, 2015
@rsc rsc removed the go1.1 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
Subject Alternative Names in X.509 certificates may include IP
addresses. This change adds support for marshaling, unmarshaling and
verifying this form of SAN.

It also causes IP addresses to only be checked against IP SANs,
rather than against hostnames as was previously the case. This
reflects RFC 6125.

Fixes golang#4658.

R=golang-dev, mikioh.mikioh, bradfitz
CC=golang-dev
https://golang.org/cl/7336046
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.