From 7543a7a87884aca957590b20b0714078d51af87b Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Thu, 16 Jul 2026 18:31:17 +0100 Subject: [PATCH] s3: strip S3 Express session token on cross-host redirects GHSA-8mxv-9xhp-86h4 The AWS SDK signs S3 Express (directory bucket) requests with a session token in the x-amz-s3session-token header. Go's HTTP client treats it as an ordinary custom header and copies it when following a redirect to another host, and it was missing from the list of secret headers the redirect policy strips. Add it to the list. The redirect tests derived their inputs from the production header list, so a header accidentally dropped from that list would silently lose test coverage rather than fail. The test list is now a deliberately literal copy, kept in sync with the production list by a new test, so removing a header from either list is a test failure. There is also a new regression test verifying the Referer header that net/http generates automatically - which for a presigned request carries the signed query string - is not forwarded across hosts. See GHSA-8mxv-9xhp-86h4 --- backend/s3/s3.go | 5 ++-- backend/s3/s3_test.go | 69 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 70 insertions(+), 4 deletions(-) diff --git a/backend/s3/s3.go b/backend/s3/s3.go index 2b7747937..94de158b1 100644 --- a/backend/s3/s3.go +++ b/backend/s3/s3.go @@ -1362,8 +1362,9 @@ func getClient(ctx context.Context, opt *Options) *http.Client { // 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 + "X-Amz-Security-Token", // AWS STS session token + "X-Amz-S3session-Token", // S3 Express (directory bucket) 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", diff --git a/backend/s3/s3_test.go b/backend/s3/s3_test.go index f41d18ccd..9c6f83509 100644 --- a/backend/s3/s3_test.go +++ b/backend/s3/s3_test.go @@ -25,10 +25,36 @@ func SetupS3Test(t *testing.T) (context.Context, *Options, *http.Client) { return ctx, opt, client } +// s3SecretTestHeaderNames is a deliberately literal copy of +// s3RedirectSecretHeaders: deriving the test inputs from the production list +// would make the redirect tests unable to detect a header missing from it. +// TestRedirectSecretHeadersMatchTestList keeps the two lists in sync. +var s3SecretTestHeaderNames = []string{ + "X-Amz-Security-Token", + "X-Amz-S3session-Token", + "Authorization", + "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", +} + +// TestRedirectSecretHeadersMatchTestList fails when a header is added to +// s3RedirectSecretHeaders without a matching literal entry in +// s3SecretTestHeaderNames (or vice versa), so every stripped header stays +// covered by the redirect tests. +func TestRedirectSecretHeadersMatchTestList(t *testing.T) { + assert.ElementsMatch(t, s3RedirectSecretHeaders, s3SecretTestHeaderNames) +} + // 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 := make(map[string]string, len(s3SecretTestHeaderNames)) + for _, header := range s3SecretTestHeaderNames { headers[header] = "secret-" + header } return headers @@ -139,6 +165,45 @@ func TestClientKeepsSecretHeadersOnSameHostRedirect(t *testing.T) { assert.NoError(t, resp.Body.Close()) } +// TestClientRemovesGeneratedRefererOnCrossHostRedirect checks that the +// Referer header net/http generates automatically when following a redirect - +// which for a presigned request carries the signed query string - is not +// forwarded to a different host. The same-host hop first proves the client +// really does generate the Referer, so the cross-host assertion can't pass +// vacuously. +func TestClientRemovesGeneratedRefererOnCrossHostRedirect(t *testing.T) { + ctx, _, client := SetupS3Test(t) + + crossHostServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Empty(t, r.Header.Get("Referer"), "Referer should have been stripped on cross-host redirect") + w.WriteHeader(http.StatusOK) + })) + defer crossHostServer.Close() + + var presignedURL string + initialServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/bucket/object": + http.Redirect(w, r, "/middle", http.StatusTemporaryRedirect) + case "/middle": + assert.Equal(t, presignedURL, r.Header.Get("Referer"), "client should generate a Referer holding the presigned URL") + http.Redirect(w, r, crossHostServer.URL, http.StatusTemporaryRedirect) + default: + http.NotFound(w, r) + } + })) + defer initialServer.Close() + presignedURL = initialServer.URL + "/bucket/object?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Signature=secret-signature" + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, presignedURL, nil) + require.NoError(t, err) + + resp, err := client.Do(req) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) + 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)