f-denkena/rclone
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

s3: fix redirect credential leaks, reject HTTPS->HTTP and strip secrets GHSA-8mxv-9xhp-86h4

Browse Source 
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
This commit is contained in:
Nick Craig-Wood
2026-07-31 13:21:59 +01:00
parent 59b513b0e7
commit 9328763d1b
2 changed files with 93 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/s3/s3.go
+30 -2
View File
@@ -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",
backend/s3/s3_test.go
+63 -15
View File
@@ -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)