From: Filippo Valsorda Date: Fri, 1 May 2020 00:20:56 +0000 (-0400) Subject: crypto/x509: require perfect matches for invalid hostnames X-Git-Tag: go1.15beta1~190 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=9d1e120c421440b450e87cc6ac8900658f9bf01a;p=gostls13.git 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 TryBot-Result: Gobot Gobot Reviewed-by: Katie Hockman --- diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go index 05936f2e35..7427c5714f 100644 --- 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 05bade5a8f..7e431a6e9e 100644 --- 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) }