fshttp: drop --header values over HTTP after HTTPS downgrade GHSA-rrwf-2qr6-p832 CVE-PENDING

The transport strips the headers set with --header once a redirect
chain leaves the originally requested host, but the host comparison
ignores the URL scheme. A server which redirects https://host:8443/
to http://host:8443/ keeps the same host and port, so the headers
were added to the plaintext request and could be read by anyone on
the network path.

The redirect chain walk now also treats a hop which downgrades the
original HTTPS request to HTTP as leaving the host, so the headers are
removed from that hop and every one after it.

The transport also logs at debug level when it removes the headers,
for either reason, so that a resulting 401 from the redirect target
can be diagnosed.

docs/content/docs.md

@@ -1465,6 +1465,12 @@ This flag is supported for all HTTP based backends even those not
 supported by `--header-upload` and `--header-download` so may be used
 as a workaround for those with care.
 
+The headers are only sent to the host of the original request. If the
+server redirects to another host (including a subdomain or a different
+port), or redirects an `https` request to `http`, the headers are not
+sent to it, or to any further hop in that redirect chain. Rclone logs
+this at debug level (`-vv`).
+
 ```console
 rclone ls remote:test --header "X-Rclone: Foo" --header "X-LetMeIn: Yes"
 ```

fs/fshttp/http.go

@@ -594,7 +594,12 @@ func newClientTrace(req *http.Request) *httptrace.ClientTrace {
 
 // redirectLeavesHost reports whether req is a redirect hop in a
 // chain which has visited a host other than that of the original
-// request at any point.
+// request, or downgraded an HTTPS request to plaintext HTTP, at any
+// point.
+//
+// The scheme is checked as well as the host and port since a
+// redirect from https://host:8443/ to http://host:8443/ is the same
+// host but would send the headers in plaintext.
 func redirectLeavesHost(req *http.Request) bool {
 	origin := req
 	for origin.Response != nil && origin.Response.Request != nil {
@@ -604,6 +609,9 @@ func redirectLeavesHost(req *http.Request) bool {
 		if !rest.SameHost(hop.URL, origin.URL) {
 			return true
 		}
+		if origin.URL.Scheme == "https" && hop.URL.Scheme == "http" {
+			return true
+		}
 	}
 	return false
 }
@@ -622,6 +630,7 @@ func (t *Transport) RoundTrip(req *http.Request) (resp *http.Response, err error
 	// Set user defined headers, unless redirected elsewhere
 	if redirectLeavesHost(req) {
 		for _, option := range t.headers {
+			fs.Debugf(nil, "redirect to %s leaves the original host or downgrades to HTTP: not sending --header %s", req.URL.Redacted(), option.Key)
 			req.Header.Del(option.Key)
 		}
 	} else {

fs/fshttp/http_test.go

@@ -10,9 +10,11 @@ import (
 	"encoding/pem"
 	"fmt"
 	"math/big"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -258,3 +260,79 @@ func TestRedirectStripsGlobalHeaders(t *testing.T) {
 		assert.Equal(t, test.want, got.Get("X-Potato"), test.path)
 	}
 }
+
+// TestRedirectLeavesHost checks the redirect chain walk against
+// synthetic chains, in particular that an HTTPS to HTTP downgrade
+// which keeps the host and port counts as leaving the host.
+func TestRedirectLeavesHost(t *testing.T) {
+	// chain builds the linked list of requests net/http records
+	// when following redirects, returning the last hop
+	chain := func(urls ...string) *http.Request {
+		var req *http.Request
+		for _, u := range urls {
+			next, err := http.NewRequest(http.MethodGet, u, nil)
+			require.NoError(t, err)
+			if req != nil {
+				next.Response = &http.Response{Request: req}
+			}
+			req = next
+		}
+		return req
+	}
+	for _, test := range []struct {
+		urls []string
+		want bool
+	}{
+		{[]string{"https://a.test/"}, false},
+		{[]string{"https://a.test/", "https://a.test/x"}, false},
+		{[]string{"https://a.test/", "https://A.test:443/x"}, false},
+		{[]string{"https://a.test/", "https://b.test/x"}, true},
+		{[]string{"https://a.test/", "https://b.test/x", "https://a.test/y"}, true},
+		{[]string{"https://a.test/", "http://a.test/x"}, true},
+		{[]string{"https://a.test:8443/", "http://a.test:8443/x"}, true},
+		{[]string{"https://a.test:8443/", "http://a.test:8443/x", "https://a.test:8443/y"}, true},
+		{[]string{"http://a.test:8080/", "https://a.test:8080/x"}, false},
+		{[]string{"http://a.test:8080/", "https://a.test:8080/x", "http://a.test:8080/y"}, false},
+	} {
+		assert.Equal(t, test.want, redirectLeavesHost(chain(test.urls...)), test.urls)
+	}
+}
+
+// TestRedirectStripsGlobalHeadersOnDowngrade checks the headers set
+// with --header are not sent over plaintext HTTP when an HTTPS
+// request is redirected to an http:// URL with the same host and
+// port.
+//
+// The https:// request is made to the plain server's host and port
+// with a TLS dialer which ignores the address and connects to the
+// TLS server instead, so both hops share a host and port as they
+// would in the wild.
+func TestRedirectStripsGlobalHeadersOnDowngrade(t *testing.T) {
+	var gotTLS, gotPlain http.Header
+	plain := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		gotPlain = r.Header.Clone()
+	}))
+	defer plain.Close()
+	secure := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		gotTLS = r.Header.Clone()
+		http.Redirect(w, r, plain.URL+"/final", http.StatusFound)
+	}))
+	defer secure.Close()
+	tlsConfig := secure.Client().Transport.(*http.Transport).TLSClientConfig
+
+	ctx, ci := fs.AddConfig(context.Background())
+	ci.Headers = []*fs.HTTPOption{{Key: "X-Potato", Value: "sausage"}}
+	client := NewClientCustom(ctx, func(tr *http.Transport) {
+		tr.DialTLSContext = func(ctx context.Context, network, _ string) (net.Conn, error) {
+			return (&tls.Dialer{Config: tlsConfig}).DialContext(ctx, network, secure.Listener.Addr().String())
+		}
+	})
+
+	resp, err := client.Get("https" + strings.TrimPrefix(plain.URL, "http") + "/start")
+	require.NoError(t, err)
+	require.NoError(t, resp.Body.Close())
+	require.NotNil(t, gotTLS)
+	require.NotNil(t, gotPlain)
+	assert.Equal(t, "sausage", gotTLS.Get("X-Potato"))
+	assert.Equal(t, "", gotPlain.Get("X-Potato"))
+}