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 <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Katie Hockman <[email protected]>

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
 			}
 		}

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)
 		}