Skip to content

Commit

Permalink
net/http, net/http/cookiejar: avoid subdomain matches on IPv6 zones
Browse files Browse the repository at this point in the history
When deciding whether to forward cookies or sensitive headers
across a redirect, do not attempt to interpret an IPv6 address
as a domain name.

Avoids a case where a maliciously-crafted redirect to an
IPv6 address with a scoped addressing zone could be
misinterpreted as a within-domain redirect. For example,
we could interpret "::1%.www.example.com" as a subdomain
of "www.example.com".

Thanks to Juho Nurminen of Mattermost for reporting this issue.

Fixes CVE-2023-45289
Fixes #65065

Change-Id: I8f463f59f0e700c8a18733d2b264a8bcb3a19599
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/2131938
Reviewed-by: Tatiana Bradley <tatianabradley@google.com>
Reviewed-by: Roland Shoemaker <bracewell@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/569340
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
  • Loading branch information
neild authored and gopherbot committed Mar 5, 2024
1 parent afb1050 commit 821bf37
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/net/http/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -1008,6 +1008,12 @@ func isDomainOrSubdomain(sub, parent string) bool {
if sub == parent {
return true
}
// If sub contains a :, it's probably an IPv6 address (and is definitely not a hostname).
// Don't check the suffix in this case, to avoid matching the contents of a IPv6 zone.
// For example, "::1%.www.example.com" is not a subdomain of "www.example.com".
if strings.ContainsAny(sub, ":%") {
return false
}
// If sub is "foo.example.com" and parent is "example.com",
// that means sub must end in "."+parent.
// Do it without allocating.
Expand Down
1 change: 1 addition & 0 deletions src/net/http/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1717,6 +1717,7 @@ func TestShouldCopyHeaderOnRedirect(t *testing.T) {
{"authorization", "http://foo.com/", "https://foo.com/", true},
{"authorization", "http://foo.com:1234/", "http://foo.com:4321/", true},
{"www-authenticate", "http://foo.com/", "http://bar.com/", false},
{"authorization", "http://foo.com/", "http://[::1%25.foo.com]/", false},

// But subdomains should work:
{"www-authenticate", "http://foo.com/", "http://foo.com/", true},
Expand Down
7 changes: 7 additions & 0 deletions src/net/http/cookiejar/jar.go
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,13 @@ func jarKey(host string, psl PublicSuffixList) string {

// isIP reports whether host is an IP address.
func isIP(host string) bool {
if strings.ContainsAny(host, ":%") {
// Probable IPv6 address.
// Hostnames can't contain : or %, so this is definitely not a valid host.
// Treating it as an IP is the more conservative option, and avoids the risk
// of interpeting ::1%.www.example.com as a subtomain of www.example.com.
return true
}
return net.ParseIP(host) != nil
}

Expand Down
10 changes: 10 additions & 0 deletions src/net/http/cookiejar/jar_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ var isIPTests = map[string]bool{
"127.0.0.1": true,
"1.2.3.4": true,
"2001:4860:0:2001::68": true,
"::1%zone": true,
"example.com": false,
"1.1.1.300": false,
"www.foo.bar.net": false,
Expand Down Expand Up @@ -629,6 +630,15 @@ var basicsTests = [...]jarTest{
{"http://www.host.test:1234/", "a=1"},
},
},
{
"IPv6 zone is not treated as a host.",
"https://example.com/",
[]string{"a=1"},
"a=1",
[]query{
{"https://[::1%25.example.com]:80/", ""},
},
},
}

func TestBasics(t *testing.T) {
Expand Down

0 comments on commit 821bf37

Please sign in to comment.