From 6b605505047416bbbf513bba1540220a8897f3f6 Mon Sep 17 00:00:00 2001
From: Damien Neil <dneil@google.com>
Date: Fri, 22 Nov 2024 12:34:11 -0800
Subject: [PATCH] [release-branch.go1.24] net/http: persist header stripping across repeated redirects

When an HTTP redirect changes the host of a request, we drop
sensitive headers such as Authorization from the redirected request.
Fix a bug where a chain of redirects could result in sensitive
headers being sent to the wrong host:

  1. request to a.tld with Authorization header
  2. a.tld redirects to b.tld
  3. request to b.tld with no Authorization header
  4. b.tld redirects to b.tld
  3. request to b.tld with Authorization header restored

Thanks to Kyle Seely for reporting this issue.

For #70530
Fixes #71212
Fixes CVE-2024-45336

Change-Id: Ia58a2e10d33d6b0cc7220935e771450e5c34de72
Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/1641
Reviewed-by: Roland Shoemaker <bracewell@google.com>
Reviewed-by: Tatiana Bradley <tatianabradley@google.com>
Commit-Queue: Roland Shoemaker <bracewell@google.com>
(cherry picked from commit 2889169b87a61f1218a02994feb80fd3d8bfa87c)
Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/1766
Reviewed-on: https://go-review.googlesource.com/c/go/+/643100
Auto-Submit: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
---

diff --git a/src/net/http/client.go b/src/net/http/client.go
index fda7815..9231f63 100644
--- a/src/net/http/client.go
+++ b/src/net/http/client.go
@@ -610,8 +610,9 @@
 		reqBodyClosed = false // have we closed the current req.Body?
 
 		// Redirect behavior:
-		redirectMethod string
-		includeBody    bool
+		redirectMethod        string
+		includeBody           = true
+		stripSensitiveHeaders = false
 	)
 	uerr := func(err error) error {
 		// the body may have been closed already by c.send()
@@ -678,7 +679,12 @@
 			// in case the user set Referer on their first request.
 			// If they really want to override, they can do it in
 			// their CheckRedirect func.
-			copyHeaders(req)
+			if !stripSensitiveHeaders && reqs[0].URL.Host != req.URL.Host {
+				if !shouldCopyHeaderOnRedirect(reqs[0].URL, req.URL) {
+					stripSensitiveHeaders = true
+				}
+			}
+			copyHeaders(req, stripSensitiveHeaders)
 
 			// Add the Referer header from the most recent
 			// request URL to the new one, if it's not https->http:
@@ -746,7 +752,7 @@
 // makeHeadersCopier makes a function that copies headers from the
 // initial Request, ireq. For every redirect, this function must be called
 // so that it can copy headers into the upcoming Request.
-func (c *Client) makeHeadersCopier(ireq *Request) func(*Request) {
+func (c *Client) makeHeadersCopier(ireq *Request) func(req *Request, stripSensitiveHeaders bool) {
 	// The headers to copy are from the very initial request.
 	// We use a closured callback to keep a reference to these original headers.
 	var (
@@ -760,8 +766,7 @@
 		}
 	}
 
-	preq := ireq // The previous request
-	return func(req *Request) {
+	return func(req *Request, stripSensitiveHeaders bool) {
 		// If Jar is present and there was some initial cookies provided
 		// via the request header, then we may need to alter the initial
 		// cookies as we follow redirects since each redirect may end up
@@ -798,12 +803,15 @@
 		// Copy the initial request's Header values
 		// (at least the safe ones).
 		for k, vv := range ireqhdr {
-			if shouldCopyHeaderOnRedirect(k, preq.URL, req.URL) {
+			sensitive := false
+			switch CanonicalHeaderKey(k) {
+			case "Authorization", "Www-Authenticate", "Cookie", "Cookie2":
+				sensitive = true
+			}
+			if !(sensitive && stripSensitiveHeaders) {
 				req.Header[k] = vv
 			}
 		}
-
-		preq = req // Update previous Request with the current request
 	}
 }
 
@@ -979,28 +987,23 @@
 	return err
 }
 
-func shouldCopyHeaderOnRedirect(headerKey string, initial, dest *url.URL) bool {
-	switch CanonicalHeaderKey(headerKey) {
-	case "Authorization", "Www-Authenticate", "Cookie", "Cookie2":
-		// Permit sending auth/cookie headers from "foo.com"
-		// to "sub.foo.com".
+func shouldCopyHeaderOnRedirect(initial, dest *url.URL) bool {
+	// Permit sending auth/cookie headers from "foo.com"
+	// to "sub.foo.com".
 
-		// Note that we don't send all cookies to subdomains
-		// automatically. This function is only used for
-		// Cookies set explicitly on the initial outgoing
-		// client request. Cookies automatically added via the
-		// CookieJar mechanism continue to follow each
-		// cookie's scope as set by Set-Cookie. But for
-		// outgoing requests with the Cookie header set
-		// directly, we don't know their scope, so we assume
-		// it's for *.domain.com.
+	// Note that we don't send all cookies to subdomains
+	// automatically. This function is only used for
+	// Cookies set explicitly on the initial outgoing
+	// client request. Cookies automatically added via the
+	// CookieJar mechanism continue to follow each
+	// cookie's scope as set by Set-Cookie. But for
+	// outgoing requests with the Cookie header set
+	// directly, we don't know their scope, so we assume
+	// it's for *.domain.com.
 
-		ihost := idnaASCIIFromURL(initial)
-		dhost := idnaASCIIFromURL(dest)
-		return isDomainOrSubdomain(dhost, ihost)
-	}
-	// All other headers are copied:
-	return true
+	ihost := idnaASCIIFromURL(initial)
+	dhost := idnaASCIIFromURL(dest)
+	return isDomainOrSubdomain(dhost, ihost)
 }
 
 // isDomainOrSubdomain reports whether sub is a subdomain (or exact
diff --git a/src/net/http/client_test.go b/src/net/http/client_test.go
index 429b8f1..1ce9539 100644
--- a/src/net/http/client_test.go
+++ b/src/net/http/client_test.go
@@ -1536,6 +1536,55 @@
 	}
 }
 
+// Issue #70530: Once we strip a header on a redirect to a different host,
+// the header should stay stripped across any further redirects.
+func TestClientStripHeadersOnRepeatedRedirect(t *testing.T) {
+	run(t, testClientStripHeadersOnRepeatedRedirect)
+}
+func testClientStripHeadersOnRepeatedRedirect(t *testing.T, mode testMode) {
+	var proto string
+	ts := newClientServerTest(t, mode, HandlerFunc(func(w ResponseWriter, r *Request) {
+		if r.Host+r.URL.Path != "a.example.com/" {
+			if h := r.Header.Get("Authorization"); h != "" {
+				t.Errorf("on request to %v%v, Authorization=%q, want no header", r.Host, r.URL.Path, h)
+			}
+		}
+		// Follow a chain of redirects from a to b and back to a.
+		// The Authorization header is stripped on the first redirect to b,
+		// and stays stripped even if we're sent back to a.
+		switch r.Host + r.URL.Path {
+		case "a.example.com/":
+			Redirect(w, r, proto+"://b.example.com/", StatusFound)
+		case "b.example.com/":
+			Redirect(w, r, proto+"://b.example.com/redirect", StatusFound)
+		case "b.example.com/redirect":
+			Redirect(w, r, proto+"://a.example.com/redirect", StatusFound)
+		case "a.example.com/redirect":
+			w.Header().Set("X-Done", "true")
+		default:
+			t.Errorf("unexpected request to %v", r.URL)
+		}
+	})).ts
+	proto, _, _ = strings.Cut(ts.URL, ":")
+
+	c := ts.Client()
+	c.Transport.(*Transport).Dial = func(_ string, _ string) (net.Conn, error) {
+		return net.Dial("tcp", ts.Listener.Addr().String())
+	}
+
+	req, _ := NewRequest("GET", proto+"://a.example.com/", nil)
+	req.Header.Add("Cookie", "foo=bar")
+	req.Header.Add("Authorization", "secretpassword")
+	res, err := c.Do(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer res.Body.Close()
+	if res.Header.Get("X-Done") != "true" {
+		t.Fatalf("response missing expected header: X-Done=true")
+	}
+}
+
 // Issue 22233: copy host when Client follows a relative redirect.
 func TestClientCopyHostOnRedirect(t *testing.T) {
 	// Virtual hostname: should not receive any request.
@@ -1702,43 +1751,39 @@
 // Part of Issue 4800
 func TestShouldCopyHeaderOnRedirect(t *testing.T) {
 	tests := []struct {
-		header     string
 		initialURL string
 		destURL    string
 		want       bool
 	}{
-		{"User-Agent", "http://foo.com/", "http://bar.com/", true},
-		{"X-Foo", "http://foo.com/", "http://bar.com/", true},
-
 		// Sensitive headers:
-		{"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},
+		{"http://foo.com/", "http://bar.com/", false},
+		{"http://foo.com/", "http://bar.com/", false},
+		{"http://foo.com/", "http://bar.com/", false},
+		{"http://foo.com/", "https://foo.com/", true},
+		{"http://foo.com:1234/", "http://foo.com:4321/", true},
+		{"http://foo.com/", "http://bar.com/", false},
+		{"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/", 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/", true},
+		{"http://foo.com/", "http://foo.com/", true},
+		{"http://foo.com/", "http://sub.foo.com/", true},
+		{"http://foo.com/", "http://notfoo.com/", false},
+		{"http://foo.com/", "https://foo.com/", true},
+		{"http://foo.com:80/", "http://foo.com/", true},
+		{"http://foo.com:80/", "http://sub.foo.com/", true},
+		{"http://foo.com:443/", "https://foo.com/", true},
+		{"http://foo.com:443/", "https://sub.foo.com/", true},
+		{"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},
+		{"http://foo.com/", "http://foo.com/", true},
+		{"http://foo.com/", "http://sub.foo.com/", true},
+		{"http://foo.com/", "http://notfoo.com/", false},
+		{"http://foo.com/", "https://foo.com/", true},
+		{"http://foo.com:80/", "http://foo.com/", true},
+		{"http://foo.com:80/", "http://sub.foo.com/", true},
+		{"http://foo.com:443/", "https://foo.com/", true},
+		{"http://foo.com:443/", "https://sub.foo.com/", true},
+		{"http://foo.com:1234/", "http://foo.com/", true},
 	}
 	for i, tt := range tests {
 		u0, err := url.Parse(tt.initialURL)
@@ -1751,10 +1796,10 @@
 			t.Errorf("%d. dest URL %q parse error: %v", i, tt.destURL, err)
 			continue
 		}
-		got := Export_shouldCopyHeaderOnRedirect(tt.header, u0, u1)
+		got := Export_shouldCopyHeaderOnRedirect(u0, u1)
 		if got != tt.want {
-			t.Errorf("%d. shouldCopyHeaderOnRedirect(%q, %q => %q) = %v; want %v",
-				i, tt.header, tt.initialURL, tt.destURL, got, tt.want)
+			t.Errorf("%d. shouldCopyHeaderOnRedirect(%q => %q) = %v; want %v",
+				i, tt.initialURL, tt.destURL, got, tt.want)
 		}
 	}
 }
