diff --git a/backend/http/http.go b/backend/http/http.go index f3ec78105..05dae780d 100644 --- a/backend/http/http.go +++ b/backend/http/http.go @@ -526,40 +526,20 @@ func addHeaders(req *http.Request, opt *Options) { // rest.ErrHTTPSDowngrade and strips the configured headers when the // redirect chain has left the originally requested host at any point. func checkRedirect(opt *Options) func(req *http.Request, via []*http.Request) error { + headers := make([]string, 0, len(opt.Headers)/2) + for i := 0; i < len(opt.Headers); i += 2 { + headers = append(headers, opt.Headers[i]) + } + stripHeaders := rest.StripHeadersOnCrossHostRedirectFn(headers...) return func(req *http.Request, via []*http.Request) error { - if err := rest.RefuseHTTPSDowngradeRedirectFn(req, via); err != nil { - if errors.Is(err, rest.ErrHTTPSDowngrade) { - err = fmt.Errorf("%w (the configured headers would be sent to the plaintext target)", err) - } - return err + err := stripHeaders(req, via) + if errors.Is(err, rest.ErrHTTPSDowngrade) { + err = fmt.Errorf("%w (the configured headers would be sent to the plaintext target)", err) } - if redirectLeavesHost(req, via) { - for i := 0; i < len(opt.Headers); i += 2 { - req.Header.Del(opt.Headers[i]) - } - } - return nil + return err } } -// redirectLeavesHost reports whether any hop in the redirect chain -// via plus the pending request req is to a different host from the -// original request via[0]. -// -// net/http copies the headers afresh from the original request for -// every hop, so once the chain has visited another host the headers -// must be stripped from every subsequent hop, even one back to the -// original host, as the other host chose the URL. -func redirectLeavesHost(req *http.Request, via []*http.Request) bool { - origin := via[0].URL - for _, hop := range via { - if !rest.SameHost(hop.URL, origin) { - return true - } - } - return !rest.SameHost(req.URL, origin) -} - // Adds the configured headers to the request if any func (f *Fs) addHeaders(req *http.Request) { addHeaders(req, &f.opt) diff --git a/backend/s3/s3.go b/backend/s3/s3.go index 389ff7274..e4734c63a 100644 --- a/backend/s3/s3.go +++ b/backend/s3/s3.go @@ -1357,10 +1357,10 @@ func getClient(ctx context.Context, opt *Options) *http.Client { } // s3RedirectSecretHeaders are the request headers carrying origin-bound -// secrets that must not be forwarded when a redirect crosses a host or -// downgrades the scheme. Go strips Authorization on a hostname change but not -// on a scheme downgrade, and has no knowledge that the SSE-C headers hold raw -// encryption keys, so we strip them all ourselves. +// secrets that must not be forwarded when a redirect leaves the original +// host. Go strips Authorization on a hostname change but keeps it for a +// subdomain or a different port, and has no knowledge that the SSE-C +// headers hold raw encryption keys, so we strip them all ourselves. var s3RedirectSecretHeaders = []string{ "X-Amz-Security-Token", // AWS STS session token "X-Amz-S3session-Token", // S3 Express (directory bucket) session token @@ -1375,36 +1375,10 @@ var s3RedirectSecretHeaders = []string{ "Referer", // may be a presigned request } -func s3CheckRedirect(req *http.Request, via []*http.Request) error { - if len(via) >= 10 { - return errors.New("stopped after 10 redirects") - } - // Never follow a redirect that downgrades the transport from HTTPS to - // HTTP. An S3 endpoint has no legitimate reason to do this, and replaying - // the request over plaintext would expose whatever credential it carries. - if via[len(via)-1].URL.Scheme == "https" && req.URL.Scheme == "http" { - return fmt.Errorf("refusing to follow insecure redirect from HTTPS to HTTP: %s", req.URL.Redacted()) - } - if s3RedirectCrossesHost(req, via) { - for _, header := range s3RedirectSecretHeaders { - req.Header.Del(header) - } - } - return nil -} - -func s3RedirectCrossesHost(req *http.Request, via []*http.Request) bool { - if len(via) == 0 { - return false - } - scheme, host := via[0].URL.Scheme, via[0].URL.Host - for _, redirect := range via[1:] { - if redirect.URL.Host != host || redirect.URL.Scheme != scheme { - return true - } - } - return host != req.URL.Host || scheme != req.URL.Scheme -} +// s3CheckRedirect is the http.Client CheckRedirect policy for S3. It +// refuses HTTPS to HTTP downgrades and strips s3RedirectSecretHeaders +// once a redirect has left the original host. +var s3CheckRedirect = rest.StripHeadersOnCrossHostRedirectFn(s3RedirectSecretHeaders...) // Fixup the request if needed. // diff --git a/backend/s3/s3_test.go b/backend/s3/s3_test.go index 9c6f83509..37b905988 100644 --- a/backend/s3/s3_test.go +++ b/backend/s3/s3_test.go @@ -228,54 +228,6 @@ func TestS3CheckRedirectRejectsSchemeDowngrade(t *testing.T) { } } -func TestRedirectCrossesHost(t *testing.T) { - mustReq := func(method, url string) *http.Request { - req, err := http.NewRequest(method, url, nil) - require.NoError(t, err) - return req - } - for _, test := range []struct { - name string - via []string - req string - want bool - }{ - { - name: "SameHost", - via: []string{"https://bucket.example.com/"}, - req: "https://bucket.example.com/redirected", - want: false, - }, - { - name: "DifferentHost", - via: []string{"https://bucket.example.com/"}, - req: "https://evil.example.com/redirected", - want: true, - }, - { - name: "SchemeDowngradeSameHost", - via: []string{"https://bucket.example.com/"}, - req: "http://bucket.example.com/redirected", - want: true, - }, - { - name: "SchemeDowngradeMidChain", - via: []string{"https://bucket.example.com/", "http://bucket.example.com/middle"}, - req: "http://bucket.example.com/final", - want: true, - }, - } { - t.Run(test.name, func(t *testing.T) { - via := make([]*http.Request, len(test.via)) - for i, url := range test.via { - via[i] = mustReq(http.MethodGet, url) - } - got := s3RedirectCrossesHost(mustReq(http.MethodGet, test.req), via) - assert.Equal(t, test.want, got) - }) - } -} - func TestClientStopsAfterTenRedirects(t *testing.T) { _, _, client := SetupS3Test(t) diff --git a/lib/rest/rest.go b/lib/rest/rest.go index da7b2b534..ecb35047f 100644 --- a/lib/rest/rest.go +++ b/lib/rest/rest.go @@ -307,6 +307,60 @@ func RefuseHTTPSDowngradeRedirectFn(req *http.Request, via []*http.Request) erro return nil } +// StripHeadersOnCrossHostRedirectFn returns a CheckRedirect function +// which follows redirects like the default net/http client but refuses +// an HTTPS to HTTP downgrade with ErrHTTPSDowngrade and removes the +// named headers from the request once the redirect chain has left the +// host of the original request. +// +// Start a header with "*" to match it without canonicalising, as +// SetHeader and Opts.ExtraHeaders do. +// +// Go strips Authorization on a hostname change but keeps it for a +// subdomain or a different port, and has no idea which custom headers +// carry secrets, so a backend which sends credentials in headers to a +// host it does not control uses this to keep them at home. +func StripHeadersOnCrossHostRedirectFn(headers ...string) func(req *http.Request, via []*http.Request) error { + return func(req *http.Request, via []*http.Request) error { + err := RefuseHTTPSDowngradeRedirectFn(req, via) + if err != nil { + return err + } + if redirectLeavesHost(req, via) { + fs.Debugf(nil, "redirect to %s leaves %s: not sending %s", req.URL.Redacted(), via[0].URL.Redacted(), strings.Join(headers, ", ")) + for _, header := range headers { + if strings.HasPrefix(header, "*") { + delete(req.Header, header[1:]) + } else { + req.Header.Del(header) + } + } + } + return nil + } +} + +// redirectLeavesHost reports whether any hop in the redirect chain +// via plus the pending request req is to a different host from the +// original request via[0]. +// +// net/http copies the headers afresh from the original request for +// every hop, so once the chain has visited another host the headers +// must be stripped from every subsequent hop, even one back to the +// original host, as the other host chose the URL. +func redirectLeavesHost(req *http.Request, via []*http.Request) bool { + if len(via) == 0 { + return false + } + origin := via[0].URL + for _, hop := range via[1:] { + if !SameHost(hop.URL, origin) { + return true + } + } + return !SameHost(req.URL, origin) +} + // Do calls the internal http.Client.Do method func (api *Client) Do(req *http.Request) (*http.Response, error) { return api.c.Do(req) diff --git a/lib/rest/rest_test.go b/lib/rest/rest_test.go index f95016d81..cb689388b 100644 --- a/lib/rest/rest_test.go +++ b/lib/rest/rest_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "strings" "sync/atomic" "testing" @@ -230,3 +231,206 @@ func TestReadBodyLimit(t *testing.T) { assert.Less(t, written.Load(), int64(maxChunks*chunk), "client should stop reading before the server stops sending") }) } + +func TestRedirectLeavesHost(t *testing.T) { + for _, test := range []struct { + name string + via []string + req string + want bool + }{ + {"NoVia", nil, "https://example.com/a", false}, + {"SameHost", []string{"https://example.com/"}, "https://example.com/b", false}, + {"SameHostDifferentCase", []string{"https://example.com/"}, "https://EXAMPLE.com/b", false}, + {"SameHostDefaultPort", []string{"https://example.com/"}, "https://example.com:443/b", false}, + // An upgrade changes the default port so counts as a new host + {"SameHostUpgrade", []string{"http://example.com/"}, "https://example.com/b", true}, + {"DifferentHost", []string{"https://example.com/"}, "https://other.example.com/b", true}, + {"DifferentPort", []string{"https://example.com/"}, "https://example.com:8443/b", true}, + {"BackToOrigin", []string{"https://example.com/", "https://other.example.com/b"}, "https://example.com/c", true}, + } { + t.Run(test.name, func(t *testing.T) { + via := make([]*http.Request, len(test.via)) + for i, rawURL := range test.via { + via[i] = mkRedirectReq(t, rawURL, "GET") + } + assert.Equal(t, test.want, redirectLeavesHost(mkRedirectReq(t, test.req, "GET"), via)) + }) + } +} + +// stripTestHeaders are the headers StripHeadersOnCrossHostRedirectFn +// is asked to strip in the tests, with the value each is sent with. +// +// The one starting with "*" is sent without canonicalising. +var stripTestHeaders = map[string]string{ + "Authorization": "secret-token", + "X-Custom-Secret": "secret-custom", + "*x-raw-secret": "secret-raw", +} + +// stripTestRequest makes a GET request to rawURL carrying the headers +// to be stripped plus a harmless marker header. +func stripTestRequest(t *testing.T, rawURL string) *http.Request { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, rawURL, nil) + require.NoError(t, err) + for header, value := range stripTestHeaders { + if raw, ok := strings.CutPrefix(header, "*"); ok { + req.Header[raw] = []string{value} + } else { + req.Header.Set(header, value) + } + } + req.Header.Set("X-Marker", "marker") + return req +} + +// stripTestClient makes an http.Client using StripHeadersOnCrossHostRedirectFn +func stripTestClient() *http.Client { + headers := make([]string, 0, len(stripTestHeaders)) + for header := range stripTestHeaders { + headers = append(headers, header) + } + return &http.Client{CheckRedirect: StripHeadersOnCrossHostRedirectFn(headers...)} +} + +func TestStripHeadersOnCrossHostRedirectFn(t *testing.T) { + assertStripped := func(r *http.Request) { + for header := range stripTestHeaders { + assert.Empty(t, r.Header.Get(strings.TrimPrefix(header, "*")), "%s should have been stripped", header) + } + assert.Equal(t, "marker", r.Header.Get("X-Marker")) + } + assertKept := func(r *http.Request) { + for header, value := range stripTestHeaders { + assert.Equal(t, value, r.Header.Get(strings.TrimPrefix(header, "*")), "%s should have been kept", header) + } + assert.Equal(t, "marker", r.Header.Get("X-Marker")) + } + + t.Run("CrossHost", func(t *testing.T) { + // httptest servers listen on 127.0.0.1:port so two servers + // count as different hosts. + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assertStripped(r) + w.WriteHeader(http.StatusOK) + })) + defer target.Close() + redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, target.URL, http.StatusTemporaryRedirect) + })) + defer redirector.Close() + + resp, err := stripTestClient().Do(stripTestRequest(t, redirector.URL)) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.NoError(t, resp.Body.Close()) + }) + + t.Run("BackToOrigin", func(t *testing.T) { + // The origin redirects to another host which redirects back + // to the origin. The headers must stay stripped on the way + // back as the other host chose the URL. + var origin *httptest.Server + bouncer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assertStripped(r) + http.Redirect(w, r, origin.URL+"/final", http.StatusTemporaryRedirect) + })) + defer bouncer.Close() + origin = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/": + assertKept(r) + http.Redirect(w, r, bouncer.URL, http.StatusTemporaryRedirect) + case "/final": + assertStripped(r) + w.WriteHeader(http.StatusOK) + default: + http.NotFound(w, r) + } + })) + defer origin.Close() + + resp, err := stripTestClient().Do(stripTestRequest(t, origin.URL)) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.NoError(t, resp.Body.Close()) + }) + + t.Run("SameHost", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/": + http.Redirect(w, r, "/redirected", http.StatusTemporaryRedirect) + case "/redirected": + assertKept(r) + w.WriteHeader(http.StatusOK) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + resp, err := stripTestClient().Do(stripTestRequest(t, server.URL)) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.NoError(t, resp.Body.Close()) + }) + + t.Run("SameHostDifferentCase", func(t *testing.T) { + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/": + u, err := url.Parse(server.URL) + require.NoError(t, err) + http.Redirect(w, r, "http://LOCALHOST:"+u.Port()+"/redirected", http.StatusTemporaryRedirect) + case "/redirected": + assertKept(r) + w.WriteHeader(http.StatusOK) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + u, err := url.Parse(server.URL) + require.NoError(t, err) + resp, err := stripTestClient().Do(stripTestRequest(t, "http://localhost:"+u.Port()+"/")) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.NoError(t, resp.Body.Close()) + }) + + t.Run("RefusesDowngrade", func(t *testing.T) { + plain := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Fail(t, "plaintext server should not have been contacted") + })) + defer plain.Close() + tlsServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, plain.URL, http.StatusTemporaryRedirect) + })) + defer tlsServer.Close() + + client := stripTestClient() + client.Transport = tlsServer.Client().Transport + // On a CheckRedirect error the client returns the redirect + // response, with its body already closed, alongside the error. + resp, err := client.Do(stripTestRequest(t, tlsServer.URL)) //nolint:bodyclose // closed by the client + require.ErrorIs(t, err, ErrHTTPSDowngrade) + require.NotNil(t, resp) + assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + }) + + t.Run("StopsAfterTenRedirects", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, r.URL.String(), http.StatusTemporaryRedirect) + })) + defer server.Close() + + resp, err := stripTestClient().Do(stripTestRequest(t, server.URL)) //nolint:bodyclose // closed by the client + require.Error(t, err) + assert.Contains(t, err.Error(), "stopped after 10 redirects") + require.NotNil(t, resp) + }) +}