From 9328763d1b73db71e97c0332b19e3747abeb9191 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Wed, 15 Jul 2026 11:56:09 +0100 Subject: [PATCH] s3: fix redirect credential leaks, reject HTTPS->HTTP and strip secrets GHSA-8mxv-9xhp-86h4 The CheckRedirect policy stripped only the X-Amz-Security-Token header when a redirect crossed a host, leaving other origin-bound secrets exposed: - Go removes Authorization on a hostname change but not on a same-host scheme downgrade, so an IBM IAM bearer token was re-sent over plaintext HTTP. - The SSE-C and copy-source SSE-C customer-key headers (which Go has no reason to treat as sensitive) were forwarded across a redirect to another host. - On a cross-host redirect net/http copies the previous request URL into the Referer header; for a presigned request that URL carries the signature and session token in its query string, disclosing them to the new origin. This now refuses outright to follow any HTTPS->HTTP redirect - an S3 endpoint has no legitimate reason to downgrade the transport, and this closes the plaintext-exposure class regardless of which header or query string carries the secret. On a cross-host redirect also strip the known credential headers and the Referer. The IDrive e2 region-lookup call posts the access key ID to a fixed HTTPS endpoint but used a bare http.Client that would follow a redirect downgrading to plaintext HTTP. Apply the same CheckRedirect policy as the main S3 client so the request can't be replayed over an unencrypted connection. Fixes GHSA-8mxv-9xhp-86h4 --- backend/s3/s3.go | 32 ++++++++++++++++-- backend/s3/s3_test.go | 78 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 93 insertions(+), 17 deletions(-) diff --git a/backend/s3/s3.go b/backend/s3/s3.go index a2da0a0b5..2b7747937 100644 --- a/backend/s3/s3.go +++ b/backend/s3/s3.go @@ -1356,12 +1356,38 @@ 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. +var s3RedirectSecretHeaders = []string{ + "X-Amz-Security-Token", // AWS STS session token + "Authorization", // e.g. IBM IAM bearer token + "ibm-service-instance-id", + "X-Amz-Server-Side-Encryption-Customer-Algorithm", + "X-Amz-Server-Side-Encryption-Customer-Key", + "X-Amz-Server-Side-Encryption-Customer-Key-Md5", + "X-Amz-Copy-Source-Server-Side-Encryption-Customer-Algorithm", + "X-Amz-Copy-Source-Server-Side-Encryption-Customer-Key", + "X-Amz-Copy-Source-Server-Side-Encryption-Customer-Key-Md5", + "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) { - req.Header.Del("X-Amz-Security-Token") + for _, header := range s3RedirectSecretHeaders { + req.Header.Del(header) + } } return nil } @@ -1689,7 +1715,9 @@ func setEndpointValueForIDriveE2(m configmap.Mapper) (err error) { if !ok || value == "" { return } - client := &http.Client{Timeout: time.Second * 3} + // Reuse the S3 redirect policy so this bootstrap call, which posts the + // access key ID, won't be redirected from HTTPS to plaintext HTTP. + client := &http.Client{Timeout: time.Second * 3, CheckRedirect: s3CheckRedirect} // API to get user region endpoint against the Access Key details: https://www.idrive.com/e2/guides/get_region_endpoint resp, err := client.Post("https://api.idrivee2.com/api/service/get_region_end_point", "application/json", diff --git a/backend/s3/s3_test.go b/backend/s3/s3_test.go index a10416dbb..f41d18ccd 100644 --- a/backend/s3/s3_test.go +++ b/backend/s3/s3_test.go @@ -25,25 +25,38 @@ func SetupS3Test(t *testing.T) (context.Context, *Options, *http.Client) { return ctx, opt, client } -func TestClientRemovesSecurityTokenOnCrossHostRedirect(t *testing.T) { +// s3SecretTestHeaders assigns each header a distinct test value +func s3SecretTestHeaders() map[string]string { + headers := make(map[string]string, len(s3RedirectSecretHeaders)) + for _, header := range s3RedirectSecretHeaders { + headers[header] = "secret-" + header + } + return headers +} + +func TestClientRemovesSecretHeadersOnCrossHostRedirect(t *testing.T) { ctx, _, client := SetupS3Test(t) + secretHeaders := s3SecretTestHeaders() redirectServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Empty(t, r.Header.Get("X-Amz-Security-Token")) + for header := range secretHeaders { + assert.Empty(t, r.Header.Get(header), "%s should have been stripped", header) + } assert.Equal(t, "date", r.Header.Get("X-Amz-Date")) w.WriteHeader(http.StatusOK) })) defer redirectServer.Close() initialServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, "token", r.Header.Get("X-Amz-Security-Token")) http.Redirect(w, r, redirectServer.URL, http.StatusTemporaryRedirect) })) defer initialServer.Close() req, err := http.NewRequestWithContext(ctx, http.MethodGet, initialServer.URL, nil) require.NoError(t, err) - req.Header.Set("X-Amz-Security-Token", "token") + for header, value := range secretHeaders { + req.Header.Set(header, value) + } req.Header.Set("X-Amz-Date", "date") resp, err := client.Do(req) @@ -52,18 +65,24 @@ func TestClientRemovesSecurityTokenOnCrossHostRedirect(t *testing.T) { assert.NoError(t, resp.Body.Close()) } -func TestClientDoesNotRestoreSecurityTokenAfterCrossHostRedirect(t *testing.T) { +func TestClientDoesNotRestoreSecretHeadersAfterCrossHostRedirect(t *testing.T) { ctx, _, client := SetupS3Test(t) + secretHeaders := s3SecretTestHeaders() + + assertStripped := func(r *http.Request) { + for header := range secretHeaders { + assert.Empty(t, r.Header.Get(header), "%s should have been stripped", header) + } + assert.Equal(t, "date", r.Header.Get("X-Amz-Date")) + } redirectServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { case "/middle": - assert.Empty(t, r.Header.Get("X-Amz-Security-Token")) - assert.Equal(t, "date", r.Header.Get("X-Amz-Date")) + assertStripped(r) http.Redirect(w, r, "/final", http.StatusTemporaryRedirect) case "/final": - assert.Empty(t, r.Header.Get("X-Amz-Security-Token")) - assert.Equal(t, "date", r.Header.Get("X-Amz-Date")) + assertStripped(r) w.WriteHeader(http.StatusOK) default: http.NotFound(w, r) @@ -72,14 +91,15 @@ func TestClientDoesNotRestoreSecurityTokenAfterCrossHostRedirect(t *testing.T) { defer redirectServer.Close() initialServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, "token", r.Header.Get("X-Amz-Security-Token")) http.Redirect(w, r, redirectServer.URL+"/middle", http.StatusTemporaryRedirect) })) defer initialServer.Close() req, err := http.NewRequestWithContext(ctx, http.MethodGet, initialServer.URL, nil) require.NoError(t, err) - req.Header.Set("X-Amz-Security-Token", "token") + for header, value := range secretHeaders { + req.Header.Set(header, value) + } req.Header.Set("X-Amz-Date", "date") resp, err := client.Do(req) @@ -88,16 +108,18 @@ func TestClientDoesNotRestoreSecurityTokenAfterCrossHostRedirect(t *testing.T) { assert.NoError(t, resp.Body.Close()) } -func TestClientKeepsSecurityTokenOnSameHostRedirect(t *testing.T) { +func TestClientKeepsSecretHeadersOnSameHostRedirect(t *testing.T) { ctx, _, client := SetupS3Test(t) + secretHeaders := s3SecretTestHeaders() server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { case "/": - assert.Equal(t, "token", r.Header.Get("X-Amz-Security-Token")) http.Redirect(w, r, "/redirected", http.StatusTemporaryRedirect) case "/redirected": - assert.Equal(t, "token", r.Header.Get("X-Amz-Security-Token")) + for header, value := range secretHeaders { + assert.Equal(t, value, r.Header.Get(header), "%s should have been preserved", header) + } w.WriteHeader(http.StatusOK) default: http.NotFound(w, r) @@ -107,7 +129,9 @@ func TestClientKeepsSecurityTokenOnSameHostRedirect(t *testing.T) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, server.URL, nil) require.NoError(t, err) - req.Header.Set("X-Amz-Security-Token", "token") + for header, value := range secretHeaders { + req.Header.Set(header, value) + } resp, err := client.Do(req) require.NoError(t, err) @@ -115,6 +139,30 @@ func TestClientKeepsSecurityTokenOnSameHostRedirect(t *testing.T) { assert.NoError(t, resp.Body.Close()) } +func mustNewGet(t *testing.T, url string) *http.Request { + t.Helper() + req, err := http.NewRequest(http.MethodGet, url, nil) + require.NoError(t, err) + return req +} + +func TestS3CheckRedirectRejectsSchemeDowngrade(t *testing.T) { + for _, test := range []struct { + name string + via string + req string + }{ + {"SameHostDowngrade", "https://bucket.example.com/", "http://bucket.example.com/redirected"}, + {"CrossHostDowngrade", "https://bucket.example.com/", "http://evil.example.com/redirected"}, + } { + t.Run(test.name, func(t *testing.T) { + err := s3CheckRedirect(mustNewGet(t, test.req), []*http.Request{mustNewGet(t, test.via)}) + require.Error(t, err) + assert.Contains(t, err.Error(), "HTTPS to HTTP") + }) + } +} + func TestRedirectCrossesHost(t *testing.T) { mustReq := func(method, url string) *http.Request { req, err := http.NewRequest(method, url, nil)