From d4cc4b6c57ee6749d9c813f83982218d78626ad5 Mon Sep 17 00:00:00 2001 From: Gustavo Falco Date: Sun, 11 Dec 2022 02:39:20 +0000 Subject: [PATCH 1/3] [Backport] net/http, net/http/cookiejar: avoid subdomain matches on IPv6 zones Offering: Cloud Core Network CVE: CVE-2023-45289 Reference: https://go-review.googlesource.com/c/go/+/569340 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. Note: The upstream does not submit this change to go1.17 according to the rules of MinorReleases. Corego3.x are based on go1.17.8. Therefore, it need to submit the change to corego3.x. Edited-by: zhaoshengwei z00581105 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 Reviewed-by: Roland Shoemaker Reviewed-on: https://go-review.googlesource.com/c/go/+/569340 Reviewed-by: Damien Neil LUCI-TryBot-Result: Go LUCI Reviewed-by: Carlos Amedee Auto-Submit: Michael Knyszek Signed-off-by: Zhao Sheng Wei zhaoshengwei@huawei.com [Backport] net/http: keep sensitive headers on redirects to the same host Offering: Cloud Core Network Reference: https://go-review.googlesource.com/c/go/+/424935 Preserve sensitive headers on a redirect to a different port of the same host. Note: The upstream does not submit this change to go1.17 according to the rules of MinorReleases. Corego3.x are based on go1.17.8. Therefore, it need to submit the change to corego3.x. Edited-by: zhaoshengwei z00581105 Fixes #35104 Change-Id: I5ab57c414ce92a70e688ee684b9ff02fb062b3c6 GitHub-Last-Rev: 8d53e71e2243c141d70d27a503d0f7e6dee64c3c GitHub-Pull-Request: golang/go#54539 Reviewed-on: https://go-review.googlesource.com/c/go/+/424935 TryBot-Result: Gopher Robot Reviewed-by: Cherry Mui Reviewed-by: Damien Neil Run-TryBot: Damien Neil Signed-off-by: Zhao Sheng Wei zhaoshengwei@huawei.com Conflict:no Reference:https://go-review.googlesource.com/c/go/+/424935;https://go-review.googlesource.com/c/go/+/569340 --- src/net/http/client.go | 10 ++++++++-- src/net/http/client_test.go | 30 +++++++++++++++++++++++++----- src/net/http/cookiejar/jar.go | 7 +++++++ src/net/http/cookiejar/jar_test.go | 10 ++++++++++ src/net/http/transport.go | 10 +++++++--- 5 files changed, 57 insertions(+), 10 deletions(-) diff --git a/src/net/http/client.go b/src/net/http/client.go index 4d380c65db..ac14071aee 100644 --- a/src/net/http/client.go +++ b/src/net/http/client.go @@ -1000,8 +1000,8 @@ func shouldCopyHeaderOnRedirect(headerKey string, initial, dest *url.URL) bool { // directly, we don't know their scope, so we assume // it's for *.domain.com. - ihost := canonicalAddr(initial) - dhost := canonicalAddr(dest) + ihost := idnaASCIIFromURL(initial) + dhost := idnaASCIIFromURL(dest) return isDomainOrSubdomain(dhost, ihost) } // All other headers are copied: @@ -1016,6 +1016,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. diff --git a/src/net/http/client_test.go b/src/net/http/client_test.go index 01d605c351..6c8d8558bb 100644 --- a/src/net/http/client_test.go +++ b/src/net/http/client_test.go @@ -1465,6 +1465,9 @@ func TestClientRedirectResponseWithoutRequest(t *testing.T) { } // Issue 4800: copy (some) headers when Client follows a redirect. +// Issue 35104: Since both URLs have the same host (localhost) +// but different ports, sensitive headers like Cookie and Authorization +// are preserved. func TestClientCopyHeadersOnRedirect(t *testing.T) { const ( ua = "some-agent/1.2" @@ -1477,6 +1480,8 @@ func TestClientCopyHeadersOnRedirect(t *testing.T) { "X-Foo": []string{xfoo}, "Referer": []string{ts2URL}, "Accept-Encoding": []string{"gzip"}, + "Cookie": []string{"foo=bar"}, + "Authorization": []string{"secretpassword"}, } if !reflect.DeepEqual(r.Header, want) { t.Errorf("Request.Header = %#v; want %#v", r.Header, want) @@ -1497,9 +1502,11 @@ func TestClientCopyHeadersOnRedirect(t *testing.T) { c := ts1.Client() c.CheckRedirect = func(r *Request, via []*Request) error { want := Header{ - "User-Agent": []string{ua}, - "X-Foo": []string{xfoo}, - "Referer": []string{ts2URL}, + "User-Agent": []string{ua}, + "X-Foo": []string{xfoo}, + "Referer": []string{ts2URL}, + "Cookie": []string{"foo=bar"}, + "Authorization": []string{"secretpassword"}, } if !reflect.DeepEqual(r.Header, want) { t.Errorf("CheckRedirect Request.Header = %#v; want %#v", r.Header, want) @@ -1701,18 +1708,31 @@ func TestShouldCopyHeaderOnRedirect(t *testing.T) { {"cookie", "http://foo.com/", "http://bar.com/", false}, {"cookie2", "http://foo.com/", "http://bar.com/", false}, {"authorization", "http://foo.com/", "http://bar.com/", false}, + {"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}, {"www-authenticate", "http://foo.com/", "http://sub.foo.com/", true}, {"www-authenticate", "http://foo.com/", "http://notfoo.com/", false}, - {"www-authenticate", "http://foo.com/", "https://foo.com/", false}, + {"www-authenticate", "http://foo.com/", "https://foo.com/", true}, {"www-authenticate", "http://foo.com:80/", "http://foo.com/", true}, {"www-authenticate", "http://foo.com:80/", "http://sub.foo.com/", true}, {"www-authenticate", "http://foo.com:443/", "https://foo.com/", true}, {"www-authenticate", "http://foo.com:443/", "https://sub.foo.com/", true}, - {"www-authenticate", "http://foo.com:1234/", "http://foo.com/", false}, + {"www-authenticate", "http://foo.com:1234/", "http://foo.com/", true}, + + {"authorization", "http://foo.com/", "http://foo.com/", true}, + {"authorization", "http://foo.com/", "http://sub.foo.com/", true}, + {"authorization", "http://foo.com/", "http://notfoo.com/", false}, + {"authorization", "http://foo.com/", "https://foo.com/", true}, + {"authorization", "http://foo.com:80/", "http://foo.com/", true}, + {"authorization", "http://foo.com:80/", "http://sub.foo.com/", true}, + {"authorization", "http://foo.com:443/", "https://foo.com/", true}, + {"authorization", "http://foo.com:443/", "https://sub.foo.com/", true}, + {"authorization", "http://foo.com:1234/", "http://foo.com/", true}, } for i, tt := range tests { u0, err := url.Parse(tt.initialURL) diff --git a/src/net/http/cookiejar/jar.go b/src/net/http/cookiejar/jar.go index e6583da7fe..f2cf9c2d8d 100644 --- a/src/net/http/cookiejar/jar.go +++ b/src/net/http/cookiejar/jar.go @@ -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 } diff --git a/src/net/http/cookiejar/jar_test.go b/src/net/http/cookiejar/jar_test.go index 47fb1abdaa..fd8d40ed1b 100644 --- a/src/net/http/cookiejar/jar_test.go +++ b/src/net/http/cookiejar/jar_test.go @@ -251,6 +251,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, @@ -613,6 +614,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) { diff --git a/src/net/http/transport.go b/src/net/http/transport.go index 309194e8e5..6cf5e65276 100644 --- a/src/net/http/transport.go +++ b/src/net/http/transport.go @@ -2719,17 +2719,21 @@ var portMap = map[string]string{ "socks5": "1080", } -// canonicalAddr returns url.Host but always with a ":port" suffix -func canonicalAddr(url *url.URL) string { +func idnaASCIIFromURL(url *url.URL) string { addr := url.Hostname() if v, err := idnaASCII(addr); err == nil { addr = v } + return addr +} + +// canonicalAddr returns url.Host but always with a ":port" suffix. +func canonicalAddr(url *url.URL) string { port := url.Port() if port == "" { port = portMap[url.Scheme] } - return net.JoinHostPort(addr, port) + return net.JoinHostPort(idnaASCIIFromURL(url), port) } // bodyEOFSignal is used by the HTTP/1 transport when reading response -- 2.33.0