Skip to content

Commit

Permalink
crypto/x509: treat certificate names with trailing dots as invalid
Browse files Browse the repository at this point in the history 
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 <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Katie Hockman <[email protected]>
  • Loading branch information
@FiloSottile
FiloSottile committed May 8, 2020
1 parent d65e1b2 commit 95c5ec6
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 31 deletions.
24 changes: 14 additions & 10 deletions src/crypto/x509/verify.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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.
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -1061,15 +1065,15 @@ 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
// 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 validCandidateName && validHostnamePattern(match) {
if matchHostnames(match, candidateName) {
return nil
}
Expand Down
39 changes: 22 additions & 17 deletions src/crypto/x509/verify_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/crypto/x509/x509_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down

0 comments on commit 95c5ec6

Please sign in to comment.