[go] x509: Validate HostName with VerifyHostname function
Gobot Gobot (Gerrit)
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.
To view, visit change 131575. To unsubscribe, or for help writing mail filters, visit settings.
Miyachi Katsuya (Gerrit)
Miyachi Katsuya has uploaded this change for review.
x509: Validate HostName with VerifyHostname function
In VerifyHostname function, if there is a host name containing wildcards in DNSNames, the host name specified in the argument will be executed even if the host name is incorrect.
ex)
DNSNames: ["*. Foo.bar.baz"]
hostname: https: // quux.foo.bar.baz
Even if an incorrect host name is specified in the SNI of TLS, there is a possibility that it can be executed if the certificate is a wild card.
https://tools.ietf.org/html/rfc6066#section-3
> Currently, the only server names supported are DNS hostnames;
Change-Id: Ia24355d11ee8d84a38e6f3baedcf3d31faba3741
---
M src/crypto/x509/verify.go
M src/crypto/x509/x509_test.go
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index 4c2ff7b..fde64d3 100644
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -114,7 +114,9 @@
// This would have validated, if it weren't for the validHostname check on Common Name.
return "x509: Common Name is not a valid hostname: " + c.Subject.CommonName
}
-
+ if !validHostname(h.Host) {
+ return "x509: Server Name Indication is not a valid hostname: " + h.Host
+ }
var valid string
if ip := net.ParseIP(h.Host); ip != nil {
// Trying to validate an IP
@@ -134,7 +136,6 @@
valid = strings.Join(c.DNSNames, ", ")
}
}
-
if len(valid) == 0 {
return "x509: certificate is not valid for any names, but wanted to match " + h.Host
}
@@ -996,6 +997,9 @@
return HostnameError{c, candidateIP}
}
+ if !validHostname(h) {
+ return HostnameError{c, h}
+ }
lowered := toLowerCaseASCII(h)
if c.commonNameAsHostname() {
diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 388156e..df47b5d 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -352,6 +352,10 @@
t.Fatalf("VerifyHostname(quux.foo.bar.baz): %v", err)
}
+ err = c.VerifyHostname("https://quux.foo.bar.baz")
+ if err == nil {
+ t.Fatalf("VerifyHostname(https://quux.foo.bar.baz): should have failed, did not")
+ }
// But check that if we change it to be matching against an IP address,
// it is rejected.
c = &Certificate{
To view, visit change 131575. To unsubscribe, or for help writing mail filters, visit settings.
Miyachi Katsuya (Gerrit)
Miyachi Katsuya uploaded patch set #2 to this change.
x509: Validate HostName with VerifyHostname function
In VerifyHostname function, if there is a host name containing wildcards in DNSNames, the host name specified in the argument will be executed even if the host name is incorrect.
ex)
DNSNames: ｛"*. Foo.bar.baz"｝
hostname: https://quux.foo.bar.baz
Even if an incorrect host name is specified in the SNI of TLS, there is a possibility that it can be executed if the certificate is a wild card.
https://tools.ietf.org/html/rfc6066#section-3
> Currently, the only server names supported are DNS hostnames;
Change-Id: Ia24355d11ee8d84a38e6f3baedcf3d31faba3741
---
M src/crypto/x509/verify.go
M src/crypto/x509/x509_test.go
2 files changed, 10 insertions(+), 2 deletions(-)
To view, visit change 131575. To unsubscribe, or for help writing mail filters, visit settings.
Adam Shannon (Gerrit)
1 comment:
Patch Set #2, Line 18:
Change-Id: Ia24355d11ee8d84a38e6f3baedcf3d31faba3741Does this change have an issue attached?
To view, visit change 131575. To unsubscribe, or for help writing mail filters, visit settings.
Miyachi Katsuya (Gerrit)
Patch Set 2:
(1 comment)
Thank you for your reply.
I am sorry, I made a PR without building an issue.
Created here
https://github.com/golang/go/issues/27591
Filippo Valsorda (Gerrit)
As mentioned on the issue, we should only do the check when matching a wildcard.
3 comments:
"crypto/x509:", followed by lowercase
Patch Set #2, Line 18:
Change-Id: Ia24355d11ee8d84a38e6f3baedcf3d31faba3741Does this change have an issue attached?
Add "Fixes #27591"
File src/crypto/x509/x509_test.go:
"TestMatchIP" feels like the wrong place, use "matchHostnamesTests" above?
To view, visit change 131575. To unsubscribe, or for help writing mail filters, visit settings.
Miyachi Katsuya (Gerrit)
Miyachi Katsuya uploaded patch set #3 to this change.
crypto/x509: validate HostName with VerifyHostname function
In VerifyHostname function, if there is a host name containing wildcards in DNSNames, the host name specified in the argument will be executed even if the host name is incorrect.
ex)
DNSNames: ["*. Foo.bar.baz"]
hostname: https://quux.foo.bar.baz
Even if an incorrect host name is specified in the SNI of TLS, there is a possibility that it can be executed if the certificate is a wild card.
https://tools.ietf.org/html/rfc6066#section-3
> Currently, the only server names supported are DNS hostnames;
Fixes #27591
Change-Id: Ia24355d11ee8d84a38e6f3baedcf3d31faba3741
---
M src/crypto/x509/verify.go
M src/crypto/x509/x509_test.go
2 files changed, 4 insertions(+), 0 deletions(-)
To view, visit change 131575. To unsubscribe, or for help writing mail filters, visit settings.
Miyachi Katsuya (Gerrit)
Thank you for review!
Changed to be valid hostname only for wildcard cases in matchHostnames.
I have a question.
At the beginning, I checked at the beginning of matchHostnames whether the argument host is a valid hostname, but in this case the following test fails.
--- FAIL: TestGoVerify (0.41 s)
verify_test.go: 393: # 25: HostnameError did not contain "Common Name is not a valid hostname": x509: certificate is not valid for any names, but wanted to match foo, invalid
verify_test.go: 393: # 28: HostnameError did not contain "Common Name is not a valid hostname": x509: certificate is not valid for any names, but wanted to match foo, invalid
Probably because matchHostnames is used here, why are you checking CommonName and h.host here?
https://github.com/golang/go/blob/master/src/crypto/x509/verify.go#L112-L116
3 comments:
"crypto/x509:", followed by lowercase
Fixed
Patch Set #2, Line 18:
Fixes #27591Add "Fixes #27591"
Fixed
File src/crypto/x509/x509_test.go:
Patch Set #2, Line 358:
c = &Certificate{
"TestMatchIP" feels like the wrong place, use "matchHostnamesTests" above?
indeed. "matchHostnamesTests: is more appropriate, so fix it. Thank you
To view, visit change 131575. To unsubscribe, or for help writing mail filters, visit settings.
Miyachi Katsuya (Gerrit)
Miyachi Katsuya uploaded patch set #4 to this change.
crypto/x509: validate HostName with VerifyHostname function
In VerifyHostname function, if there is a host name containing wildcards in DNSNames, the host name specified in the argument will be executed even if the host name is incorrect.
ex)
DNSNames: ["*. Foo.bar.baz"]
hostname: https://quux.foo.bar.baz
Even if an incorrect host name is specified in the SNI of TLS, there is a possibility that it can be executed if the certificate is a wild card.
https://tools.ietf.org/html/rfc6066#section-3
> Currently, the only server names supported are DNS hostnames;
Fixes #27591
Change-Id: Ia24355d11ee8d84a38e6f3baedcf3d31faba3741
---
M src/crypto/x509/verify.go
M src/crypto/x509/x509_test.go
2 files changed, 5 insertions(+), 0 deletions(-)
To view, visit change 131575. To unsubscribe, or for help writing mail filters, visit settings.
Miyachi Katsuya (Gerrit)
Hi all
What is the status here?
I am saved if you can tell me if there is still a counterpart in my direction
2 comments:
Fixed
Done
Patch Set #2, Line 18:
Fixes #27591
Fixed
Done
To view, visit change 131575. To unsubscribe, or for help writing mail filters, visit settings.