crypto/x509: require perfect matches for invalid hostnames

When the input or SAN dNSNames are not valid hostnames, the specs don't
define what should happen, because this should ideally never happen, so
everything we do is undefined behavior. Browsers get to just return an
error, because browsers can assume that the resolving layer is DNS. We
can't, names can be resolved by anything implementing a Dial function,
and the crypto/x509 APIs can also be used directly without actual
networks in sight.

Trying to process invalid hostnames leads to issues like #27591 where
wildcards glob stuff they aren't expected to, because wildcards are only
defined on hostnames.

Try to rationalize the behavior like this: if both the VerifyHostname
input and the SAN dNSNames are a valid hostname, follow the specs;
otherwise, only accept perfect 1:1 case-insensitive matches (without
wildcards or trailing dot processing).

This should allow us to keep supporting weird names, with less
unexpected side-effects from undefined behavior. Also, it's a rule, even
if completely made up, so something we can reason about and code against.

The commonName field does allow any string, but no specs define how to
process it. Processing it differently from dNSNames would be confusing,
and allowing it to match invalid hostnames is incompatible with Name
Constraint processing (#24151).

This does encourage invalid dNSNames, regrettably, but we need some way
for the standard API to match weird names, and the alternative of
keeping CN alive sounds less appealing.

Fixes #27591

Change-Id: Id2d515f068a17ff796a32b30733abe44ad4f0339
Reviewed-on: https://go-review.googlesource.com/c/go/+/231378
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>

diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index 05936f2e358db061096460719a04c1a437459056..7427c5714f7aa12d288736115cb4b30cdcf52a97 100644 (file)
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -110,7 +110,7 @@ func (h HostnameError) Error() string {
        c := h.Certificate
 
        if !c.hasSANExtension() && !validHostname(c.Subject.CommonName) &&
-               matchHostnames(toLowerCaseASCII(c.Subject.CommonName), toLowerCaseASCII(h.Host)) {
+               matchHostnames(c.Subject.CommonName, h.Host) {
                // 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
        }
@@ -954,9 +954,16 @@ func (c *Certificate) commonNameAsHostname() bool {
        return !ignoreCN && !c.hasSANExtension() && validHostname(c.Subject.CommonName)
 }
 
+func matchExactly(hostA, hostB string) bool {
+       if hostA == "" || hostA == "." || hostB == "" || hostB == "." {
+               return false
+       }
+       return toLowerCaseASCII(hostA) == toLowerCaseASCII(hostB)
+}
+
 func matchHostnames(pattern, host string) bool {
-       host = strings.TrimSuffix(host, ".")
-       pattern = strings.TrimSuffix(pattern, ".")
+       pattern = toLowerCaseASCII(strings.TrimSuffix(pattern, "."))
+       host = toLowerCaseASCII(strings.TrimSuffix(host, "."))
 
        if len(pattern) == 0 || len(host) == 0 {
                return false
@@ -1018,8 +1025,8 @@ func toLowerCaseASCII(in string) string {
 //
 // IP addresses can be optionally enclosed in square brackets and are checked
 // against the IPAddresses field. Other names are checked case insensitively
-// against the DNSNames field, with support for only one wildcard as the whole
-// left-most label.
+// against the DNSNames field. If the names are valid hostnames, the certificate
+// fields can have a wildcard as the left-most label.
 //
 // If the Common Name field is a valid hostname, and the certificate doesn't
 // have any Subject Alternative Names, the name will also be checked against the
@@ -1042,15 +1049,26 @@ func (c *Certificate) VerifyHostname(h string) error {
                return HostnameError{c, candidateIP}
        }
 
-       lowered := toLowerCaseASCII(h)
-
+       names := c.DNSNames
        if c.commonNameAsHostname() {
-               if matchHostnames(toLowerCaseASCII(c.Subject.CommonName), lowered) {
-                       return nil
-               }
-       } else {
-               for _, match := range c.DNSNames {
-                       if matchHostnames(toLowerCaseASCII(match), lowered) {
+               names = []string{c.Subject.CommonName}
+       }
+
+       candidateName := toLowerCaseASCII(h) // Save allocations inside the loop.
+       validCandidateName := validHostname(candidateName)
+
+       for _, match := range names {
+               // Ideally, we'd only match valid hostnames according to RFC 6125 like
+               // browsers (more or less) do, but in practice Go is used in a wider
+               // array of contexts and can't even assume DNS resolution. Instead,
+               // always allow perfect matches, and only apply wildcard and trailing
+               // dot processing to valid hostnames.
+               if validCandidateName && validHostname(match) {
+                       if matchHostnames(match, candidateName) {
+                               return nil
+                       }
+               } else {
+                       if matchExactly(match, candidateName) {
                                return nil
                        }
                }

diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 05bade5a8f31ad55ca7073f4b9fe125a2776743a..7e431a6e9ee0d656b952e7b5598927c8efaa2dee 100644 (file)
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -357,10 +357,14 @@ var matchHostnamesTests = []matchHostnamesTest{
        {"*.example.com", "www.example.com", true},
        {"*.example.com", "www.example.com.", true},
        {"*.example.com", "xyz.www.example.com", false},
+       {"*.example.com", "https://www.example.com", false}, // Issue 27591
+       {"*.example..com", "www.example..com", false},
+       {"www.example..com", "www.example..com", true},
        {"*.*.example.com", "xyz.www.example.com", false},
        {"*.www.*.com", "xyz.www.example.com", false},
        {"*bar.example.com", "foobar.example.com", false},
        {"f*.example.com", "foobar.example.com", false},
+       {"www.example.com", "*.example.com", false},
        {"", ".", false},
        {".", "", false},
        {".", ".", false},
@@ -371,11 +375,14 @@ var matchHostnamesTests = []matchHostnamesTest{
        {"*.com.", "example.com", true},
        {"*.com", "example.com", true},
        {"*.com", "example.com.", true},
+       {"foo:bar", "foo:bar", true},
+       {"*.foo:bar", "xxx.foo:bar", true},
 }
 
 func TestMatchHostnames(t *testing.T) {
        for i, test := range matchHostnamesTests {
-               r := matchHostnames(test.pattern, test.host)
+               c := &Certificate{DNSNames: []string{test.pattern}}
+               r := c.VerifyHostname(test.host) == nil
                if r != test.ok {
                        t.Errorf("#%d mismatch got: %t want: %t when matching '%s' against '%s'", i, r, test.ok, test.host, test.pattern)
                }