crypto/x509: treat certificate names with trailing dots as invalid

Trailing dots are not allowed in certificate fields like CN and SANs
(while they are allowed and ignored as inputs to verification APIs).
Move to considering names with trailing dots in certificates as invalid
hostnames.

Following the rule of CL 231378, these invalid names lose wildcard
processing, but can still match if there is a 1:1 match, trailing dot
included, with the VerifyHostname input.

They also become ignored Common Name values regardless of the
GODEBUG=x509ignoreCN=X value, because we have to ignore invalid
hostnames in Common Name for #24151. The error message automatically
accounts for this, and doesn't suggest the environment variable. You
don't get to use a legacy deprecated field AND invalid hostnames.

(While at it, also consider wildcards in VerifyHostname inputs as
invalid hostnames, not that it should change any observed behavior.)

Change-Id: Iecdee8927df50c1d9daf904776b051de9f5e76ad
Reviewed-on: https://go-review.googlesource.com/c/go/+/231380
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 e8886c14c7e4844482aeb26eb12f07b77351532a..a9516fc375d074bb6fedd9d40463b493311912d0 100644 (file)
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -110,11 +110,11 @@ func (h HostnameError) Error() string {
        c := h.Certificate
 
        if !c.hasSANExtension() && matchHostnames(c.Subject.CommonName, h.Host) {
-               if !ignoreCN && !validHostname(c.Subject.CommonName) {
+               if !ignoreCN && !validHostnamePattern(c.Subject.CommonName) {
                        // 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 ignoreCN && validHostname(c.Subject.CommonName) {
+               if ignoreCN && validHostnamePattern(c.Subject.CommonName) {
                        // This would have validated if x509ignoreCN=0 were set.
                        return "x509: certificate relies on legacy Common Name field, " +
                                "use SANs or temporarily enable Common Name matching with GODEBUG=x509ignoreCN=0"
@@ -902,12 +902,16 @@ func (c *Certificate) buildChains(cache map[*Certificate][][]*Certificate, curre
        return
 }
 
+func validHostnamePattern(host string) bool { return validHostname(host, true) }
+func validHostnameInput(host string) bool   { return validHostname(host, false) }
+
 // validHostname reports whether host is a valid hostname that can be matched or
 // matched against according to RFC 6125 2.2, with some leniency to accommodate
 // legacy values.
-func validHostname(host string) bool {
-       host = strings.TrimSuffix(host, ".")
-
+func validHostname(host string, isPattern bool) bool {
+       if !isPattern {
+               host = strings.TrimSuffix(host, ".")
+       }
        if len(host) == 0 {
                return false
        }
@@ -917,7 +921,7 @@ func validHostname(host string) bool {
                        // Empty label.
                        return false
                }
-               if i == 0 && part == "*" {
+               if isPattern && i == 0 && part == "*" {
                        // Only allow full left-most wildcards, as those are the only ones
                        // we match, and matching literal '*' characters is probably never
                        // the expected behavior.
@@ -957,7 +961,7 @@ func validHostname(host string) bool {
 // constraints if there is no risk the CN would be matched as a hostname.
 // See NameConstraintsWithoutSANs and issue 24151.
 func (c *Certificate) commonNameAsHostname() bool {
-       return !ignoreCN && !c.hasSANExtension() && validHostname(c.Subject.CommonName)
+       return !ignoreCN && !c.hasSANExtension() && validHostnamePattern(c.Subject.CommonName)
 }
 
 func matchExactly(hostA, hostB string) bool {
@@ -968,7 +972,7 @@ func matchExactly(hostA, hostB string) bool {
 }
 
 func matchHostnames(pattern, host string) bool {
-       pattern = toLowerCaseASCII(strings.TrimSuffix(pattern, "."))
+       pattern = toLowerCaseASCII(pattern)
        host = toLowerCaseASCII(strings.TrimSuffix(host, "."))
 
        if len(pattern) == 0 || len(host) == 0 {
@@ -1061,7 +1065,7 @@ func (c *Certificate) VerifyHostname(h string) error {
        }
 
        candidateName := toLowerCaseASCII(h) // Save allocations inside the loop.
-       validCandidateName := validHostname(candidateName)
+       validCandidateName := validHostnameInput(candidateName)
 
        for _, match := range names {
                // Ideally, we'd only match valid hostnames according to RFC 6125 like
@@ -1069,7 +1073,7 @@ func (c *Certificate) VerifyHostname(h string) error {
                // 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 validCandidateName && validHostnamePattern(match) {
                        if matchHostnames(match, candidateName) {
                                return nil
                        }

diff --git a/src/crypto/x509/verify_test.go b/src/crypto/x509/verify_test.go
index 8a9036a3d0a66b5fd01eb0a63010846070e910ce..18271540c7513167293928ea8c680723488998a7 100644 (file)
--- a/src/crypto/x509/verify_test.go
+++ b/src/crypto/x509/verify_test.go
@@ -1987,26 +1987,31 @@ CCqGSM49BAMCA0gAMEUCIQClA3d4tdrDu9Eb5ZBpgyC+fU1xTZB0dKQHz6M5fPZA
 
 func TestValidHostname(t *testing.T) {
        tests := []struct {
-               host string
-               want bool
+               host                     string
+               validInput, validPattern bool
        }{
-               {"example.com", true},
-               {"eXample123-.com", true},
-               {"-eXample123-.com", false},
-               {"", false},
-               {".", false},
-               {"example..com", false},
-               {".example.com", false},
-               {"*.example.com", true},
-               {"*foo.example.com", false},
-               {"foo.*.example.com", false},
-               {"exa_mple.com", true},
-               {"foo,bar", false},
-               {"project-dev:us-central1:main", true},
+               {host: "example.com", validInput: true, validPattern: true},
+               {host: "eXample123-.com", validInput: true, validPattern: true},
+               {host: "-eXample123-.com"},
+               {host: ""},
+               {host: "."},
+               {host: "example..com"},
+               {host: ".example.com"},
+               {host: "example.com.", validInput: true},
+               {host: "*.example.com."},
+               {host: "*.example.com", validPattern: true},
+               {host: "*foo.example.com"},
+               {host: "foo.*.example.com"},
+               {host: "exa_mple.com", validInput: true, validPattern: true},
+               {host: "foo,bar"},
+               {host: "project-dev:us-central1:main", validInput: true, validPattern: true},
        }
        for _, tt := range tests {
-               if got := validHostname(tt.host); got != tt.want {
-                       t.Errorf("validHostname(%q) = %v, want %v", tt.host, got, tt.want)
+               if got := validHostnamePattern(tt.host); got != tt.validPattern {
+                       t.Errorf("validHostnamePattern(%q) = %v, want %v", tt.host, got, tt.validPattern)
+               }
+               if got := validHostnameInput(tt.host); got != tt.validInput {
+                       t.Errorf("validHostnameInput(%q) = %v, want %v", tt.host, got, tt.validInput)
                }
        }
 }

diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index f29e322bb4f036f4b03957aedbdc0146a91a371b..d69c8ba72ee474bc28b27fd88724a1f70c6623a1 100644 (file)
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -369,10 +369,10 @@ var matchHostnamesTests = []matchHostnamesTest{
        {".", "", false},
        {".", ".", false},
        {"example.com", "example.com.", true},
-       {"example.com.", "example.com", true},
-       {"example.com.", "example.com.", true},
-       {"*.com.", "example.com.", true},
-       {"*.com.", "example.com", true},
+       {"example.com.", "example.com", false},
+       {"example.com.", "example.com.", true}, // perfect matches allow trailing dots in patterns
+       {"*.com.", "example.com.", false},
+       {"*.com.", "example.com", false},
        {"*.com", "example.com", true},
        {"*.com", "example.com.", true},
        {"foo:bar", "foo:bar", true},