fix(s3gateway): enforce signature freshness and single key decoding (#25490)

- Reject signed requests older or newer than 15 minutes (x-amz-date
  for v4, Date for v2), so intercepted valid requests can no longer
  be replayed indefinitely
- Decode the object key exactly once: r.URL.Path is already decoded
  by net/http and is the form the signature covers, decoding again
  let a signature for one key be replayed against another
  (e.g. %252e%252e%252f resolving to ../)
- Add unit tests for both behaviors

Co-authored-by: Qiu Jian <qiujian@yunionyun.com>
Co-authored-by: Claude <noreply@anthropic.com>

pkg/mcclient/auth/aksk.go

@@ -16,6 +16,7 @@ package auth
 
 import (
 	"net/http"
+	"time"
 
 	"yunion.io/x/pkg/errors"
 	"yunion.io/x/pkg/util/cache"
@@ -24,6 +25,41 @@ import (
 	"yunion.io/x/onecloud/pkg/mcclient"
 )
 
+// maxSignatureSkew is the maximal clock skew accepted for a signed request,
+// the same window AWS uses: requests signed earlier can not be replayed
+// indefinitely.
+const maxSignatureSkew = 15 * time.Minute
+
+// checkRequestFreshness rejects replayed signatures: a signed request is only
+// accepted within 15 minutes of its signing time.
+func checkRequestFreshness(req http.Request) error {
+	dateStr := req.Header.Get("x-amz-date")
+	if len(dateStr) > 0 {
+		signTime, err := time.Parse("20060102T150405Z", dateStr)
+		if err != nil {
+			return errors.Wrap(err, "invalid x-amz-date")
+		}
+		return checkTimeSkew(signTime)
+	}
+	dateStr = req.Header.Get("Date")
+	if len(dateStr) > 0 {
+		signTime, err := http.ParseTime(dateStr)
+		if err != nil {
+			return errors.Wrap(err, "invalid Date header")
+		}
+		return checkTimeSkew(signTime)
+	}
+	return errors.Error("missing signing date")
+}
+
+func checkTimeSkew(signTime time.Time) error {
+	skew := time.Since(signTime)
+	if skew > maxSignatureSkew || skew < -maxSignatureSkew {
+		return errors.Errorf("request signature expired, signed at %s", signTime)
+	}
+	return nil
+}
+
 type sAccessKeyCache struct {
 	*cache.LRUCache
 }
@@ -64,6 +100,9 @@ func (c *sAccessKeyCache) Verify(cli *mcclient.Client, req http.Request, virtual
 	if err != nil {
 		return nil, errors.Wrap(err, "s3auth.DecodeAccessKeyRequestV2")
 	}
+	if err := checkRequestFreshness(req); err != nil {
+		return nil, errors.Wrap(err, "checkRequestFreshness")
+	}
 
 	token, found := c.getToken(aksk.GetAccessKey())
 	if found {

pkg/mcclient/auth/aksk_test.go

@@ -0,0 +1,78 @@
+// Copyright 2019 Yunion
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package auth
+
+import (
+	"net/http"
+	"testing"
+	"time"
+)
+
+func TestCheckRequestFreshness(t *testing.T) {
+	now := time.Now()
+
+	cases := []struct {
+		name    string
+		req     http.Request
+		wantErr bool
+	}{
+		{
+			name:    "v4 recent x-amz-date",
+			req:     http.Request{Header: http.Header{"X-Amz-Date": []string{now.UTC().Format("20060102T150405Z")}}},
+			wantErr: false,
+		},
+		{
+			name:    "v4 expired x-amz-date",
+			req:     http.Request{Header: http.Header{"X-Amz-Date": []string{now.Add(-time.Hour).UTC().Format("20060102T150405Z")}}},
+			wantErr: true,
+		},
+		{
+			name:    "v4 future x-amz-date",
+			req:     http.Request{Header: http.Header{"X-Amz-Date": []string{now.Add(time.Hour).UTC().Format("20060102T150405Z")}}},
+			wantErr: true,
+		},
+		{
+			name:    "v4 malformed x-amz-date",
+			req:     http.Request{Header: http.Header{"X-Amz-Date": []string{"not-a-date"}}},
+			wantErr: true,
+		},
+		{
+			name:    "v2 recent Date header",
+			req:     http.Request{Header: http.Header{"Date": []string{now.UTC().Format(http.TimeFormat)}}},
+			wantErr: false,
+		},
+		{
+			name:    "v2 expired Date header",
+			req:     http.Request{Header: http.Header{"Date": []string{now.Add(-time.Hour).UTC().Format(http.TimeFormat)}}},
+			wantErr: true,
+		},
+		{
+			name:    "missing date",
+			req:     http.Request{Header: http.Header{}},
+			wantErr: true,
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			err := checkRequestFreshness(c.req)
+			if c.wantErr && err == nil {
+				t.Fatalf("expected error, got nil")
+			}
+			if !c.wantErr && err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+		})
+	}
+}

pkg/s3gateway/handlers/handlers.go

@@ -17,7 +17,6 @@ package handlers
 import (
 	"context"
 	"net/http"
-	"net/url"
 	"strings"
 	"time"
 
@@ -115,11 +114,10 @@ func getObjectRequest(r *http.Request) (SObjectRequest, error) {
 	} else {
 		return o, errors.Error("invalid S3 request")
 	}
-	var err error
-	o.Key, err = url.PathUnescape(o.Key)
-	if err != nil {
-		return o, errors.Wrap(err, "url.PathUnescape")
-	}
+	// r.URL.Path is already percent-decoded once by net/http, and the
+	// signature covers exactly this form. Decoding again would let a valid
+	// signature for one key be replayed against a different key
+	// (e.g. %252e%252e%252f would resolve to ../)
 	return o, o.Validate()
 }
 

pkg/s3gateway/handlers/handlers_test.go

@@ -0,0 +1,55 @@
+// Copyright 2019 Yunion
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package handlers
+
+import (
+	"net/http"
+	"net/url"
+	"testing"
+)
+
+// the object key must be percent-decoded exactly once (the form the
+// signature covers); decoding twice would let a signature for one key be
+// replayed against another
+func TestGetObjectRequestSingleDecode(t *testing.T) {
+	cases := []struct {
+		name    string
+		urlPath string // URL.Path as net/http sets it: raw path decoded once
+		wantKey string
+	}{
+		// raw /bucket/a%20b -> Path /bucket/a b
+		{name: "simple space", urlPath: "/bucket/a b", wantKey: "a b"},
+		// raw /bucket/a%252Fb -> Path /bucket/a%2Fb, must stay literal
+		{name: "literal percent", urlPath: "/bucket/a%2Fb", wantKey: "a%2Fb"},
+		// raw /bucket/a%252e%252e%252fc -> Path /bucket/a%2e%2e%2fc,
+		// must not resolve to a/../c
+		{name: "no double dot resolution", urlPath: "/bucket/a%2e%2e%2fc", wantKey: "a%2e%2e%2fc"},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			req := &http.Request{
+				Host: "127.0.0.1",
+				URL:  &url.URL{Path: c.urlPath},
+			}
+			o, err := getObjectRequest(req)
+			if err != nil {
+				t.Fatalf("getObjectRequest: %v", err)
+			}
+			if o.Key != c.wantKey {
+				t.Fatalf("key = %q, want %q", o.Key, c.wantKey)
+			}
+		})
+	}
+}