diff --git a/cmd/bucket-policy.go b/cmd/bucket-policy.go index 35c7a0f22..9657abf98 100644 --- a/cmd/bucket-policy.go +++ b/cmd/bucket-policy.go @@ -255,8 +255,8 @@ func getConditionValuesWithTags(r *http.Request, lc string, cred auth.Credential } cloneHeader := r.Header.Clone() - signatureAge := cloneHeader.Get("x-amz-signature-age") - cloneHeader.Del("x-amz-signature-age") + signatureAge := cloneHeader.Get(xhttp.AmzSignatureAge) + cloneHeader.Del(xhttp.AmzSignatureAge) // The presigned V4 verifier overwrites this internal scratch header after // validating the signature. Ignore a value supplied on every other request // type, where it would otherwise synthesize s3:signatureAge. diff --git a/cmd/erasure-multipart-upload-checksum_test.go b/cmd/erasure-multipart-upload-checksum_test.go index dd10a2026..96f6fe4fd 100644 --- a/cmd/erasure-multipart-upload-checksum_test.go +++ b/cmd/erasure-multipart-upload-checksum_test.go @@ -157,6 +157,13 @@ func copyPartWithoutChecksumHTTP(t *testing.T, apiRouter http.Handler, creds aut if sourceRange != "" { req.Header.Set(xhttp.AmzCopySourceRange, sourceRange) } + // Re-sign so the copy-source x-amz-* headers are covered by the signature, + // as real S3 clients send them; the verifier rejects unsigned x-amz-*. + if creds.AccessKey != "" && creds.SecretKey != "" { + if err := signRequestV4(req, creds.AccessKey, creds.SecretKey); err != nil { + t.Fatalf("failed to re-sign UploadPartCopy request: %v", err) + } + } rec := httptest.NewRecorder() apiRouter.ServeHTTP(rec, req) if rec.Code != http.StatusOK { diff --git a/cmd/object-copy-checksum_test.go b/cmd/object-copy-checksum_test.go index 571e781d5..831d8b548 100644 --- a/cmd/object-copy-checksum_test.go +++ b/cmd/object-copy-checksum_test.go @@ -62,6 +62,13 @@ func copyChecksumRequest(t *testing.T, apiRouter http.Handler, credentials auth. t.Fatalf("failed to build CopyObject request: %v", err) } req.Header.Set(xhttp.AmzCopySource, SlashSeparator+pathJoin(bucket, source)) + // Re-sign so x-amz-copy-source is covered by the signature, as real S3 + // clients send it; the verifier rejects unsigned x-amz-* headers. + if credentials.AccessKey != "" && credentials.SecretKey != "" { + if err := signRequestV4(req, credentials.AccessKey, credentials.SecretKey); err != nil { + t.Fatalf("failed to re-sign CopyObject request: %v", err) + } + } rec := httptest.NewRecorder() apiRouter.ServeHTTP(rec, req) return rec diff --git a/cmd/object-copy-federation_test.go b/cmd/object-copy-federation_test.go index 40fe9b4e1..5c11863a8 100644 --- a/cmd/object-copy-federation_test.go +++ b/cmd/object-copy-federation_test.go @@ -197,6 +197,13 @@ func federatedCopyRequest(t *testing.T, apiRouter http.Handler, credentials auth t.Fatalf("failed to build federated CopyObject request: %v", err) } req.Header.Set(xhttp.AmzCopySource, SlashSeparator+pathJoin(srcBucket, srcObject)) + // Re-sign so x-amz-copy-source is covered by the signature, as real S3 + // clients send it; the verifier rejects unsigned x-amz-* headers. + if credentials.AccessKey != "" && credentials.SecretKey != "" { + if err := signRequestV4(req, credentials.AccessKey, credentials.SecretKey); err != nil { + t.Fatalf("failed to re-sign federated CopyObject request: %v", err) + } + } rec := httptest.NewRecorder() apiRouter.ServeHTTP(rec, req) return rec diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index 65a839dea..c8fbee472 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -3658,9 +3658,6 @@ func (api objectAPIHandlers) PutObjectTaggingHandler(w http.ResponseWriter, r *h } tagsStr := tags.String() - // Set this such that authorization policies can be applied on the object tags. - r.Header.Set(xhttp.AmzObjectTagging, tagsStr) - logger.GetReqInfo(ctx).BucketName = bucket logger.GetReqInfo(ctx).ObjectName = object if s3Error := authenticateRequest(ctx, r, policy.PutObjectTaggingAction); s3Error != ErrNone { @@ -3668,6 +3665,12 @@ func (api objectAPIHandlers) PutObjectTaggingHandler(w http.ResponseWriter, r *h return } + // Set this such that authorization policies can be applied on the object + // tags. This is derived from the request body, so it must be injected only + // after signature verification: the SigV4 verifier now rejects unsigned + // x-amz-* request headers, and this synthesized header is never signed. + r.Header.Set(xhttp.AmzObjectTagging, tagsStr) + opts, err := getOpts(ctx, r, bucket, object) if err != nil { writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) diff --git a/cmd/object-handlers_test.go b/cmd/object-handlers_test.go index d3cc64fdf..ca1f236d0 100644 --- a/cmd/object-handlers_test.go +++ b/cmd/object-handlers_test.go @@ -1799,6 +1799,12 @@ func testAPICopyObjectPartHandlerSanity(obj ObjectLayer, instanceType, bucketNam req.Header.Set("X-Amz-Copy-Source", url.QueryEscape(pathJoin(bucketName, objectName))) req.Header.Set("X-Amz-Copy-Source-Range", fmt.Sprintf("bytes=%d-%d", a, b)) + // Re-sign so the copy-source x-amz-* headers are covered by the + // signature, as real clients do; the verifier rejects unsigned x-amz-*. + if err = signRequestV4(req, credentials.AccessKey, credentials.SecretKey); err != nil { + t.Fatalf("Test failed to re-sign HTTP request for copy object part: %v", err) + } + // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. // Call the ServeHTTP to execute the handler, `func (api objectAPIHandlers) CopyObjectHandler` handles the request. a = globalMinPartSize + 1 @@ -2200,6 +2206,15 @@ func testAPICopyObjectPartHandler(obj ObjectLayer, instanceType, bucketName stri } } + // Re-sign so the copy-source x-amz-* headers set above are covered by + // the signature, as real clients do; the verifier rejects unsigned + // x-amz-* headers. + if testCase.accessKey != "" && testCase.secretKey != "" { + if err = signRequestV4(req, testCase.accessKey, testCase.secretKey); err != nil { + t.Fatalf("Test %d: Failed to re-sign HTTP request for copy Object: %v", i+1, err) + } + } + // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. // Call the ServeHTTP to execute the handler, `func (api objectAPIHandlers) CopyObjectHandler` handles the request. apiRouter.ServeHTTP(rec, req) @@ -2626,6 +2641,16 @@ func testAPICopyObjectHandler(obj ObjectLayer, instanceType, bucketName string, if testCase.metadataGarbage { req.Header.Set("X-Amz-Metadata-Directive", "Unknown") } + // The x-amz-copy-source and related x-amz-* headers set above must be + // part of the SigV4 signature, exactly as real S3 clients send them. + // Re-sign now that they are present; the verifier rejects unsigned + // x-amz-* headers (an unsigned x-amz-copy-source could otherwise turn a + // PUT grant into a server-side copy). + if testCase.accessKey != "" && testCase.secretKey != "" { + if err = signRequestV4(req, testCase.accessKey, testCase.secretKey); err != nil { + t.Fatalf("Test %d: Failed to re-sign HTTP request for copy Object: %v", i, err) + } + } // Since `apiRouter` satisfies `http.Handler` it has a ServeHTTP to execute the logic of the handler. // Call the ServeHTTP to execute the handler, `func (api objectAPIHandlers) CopyObjectHandler` handles the request. apiRouter.ServeHTTP(rec, req) diff --git a/cmd/object-ssec-zero-byte_test.go b/cmd/object-ssec-zero-byte_test.go index 787c11893..f3273dbda 100644 --- a/cmd/object-ssec-zero-byte_test.go +++ b/cmd/object-ssec-zero-byte_test.go @@ -165,6 +165,11 @@ func testAPIZeroByteSSECAuthenticatesKey(obj ObjectLayer, instanceType, bucketNa t.Fatal(err) } req.Header.Set(xhttp.AmzCopySource, SlashSeparator+pathJoin(bucketName, object)) + // Re-sign so x-amz-copy-source is covered by the signature, as real S3 + // clients send it; the verifier rejects unsigned x-amz-* headers. + if err = signRequestV4(req, credentials.AccessKey, credentials.SecretKey); err != nil { + t.Fatalf("%s: failed to re-sign UploadPartCopy request: %v", instanceType, err) + } rec = httptest.NewRecorder() apiRouter.ServeHTTP(rec, req) if rec.Code != http.StatusForbidden { diff --git a/cmd/signature-v4-utils.go b/cmd/signature-v4-utils.go index 82b767ed3..88ed7e9cd 100644 --- a/cmd/signature-v4-utils.go +++ b/cmd/signature-v4-utils.go @@ -264,14 +264,49 @@ func signV4TrimAll(input string) string { return strings.Join(strings.Fields(input), " ") } -// checkMetaHeaders will check if the metadata from header/url is the same with the one from signed headers -func checkMetaHeaders(signedHeadersMap http.Header, r *http.Request) APIErrorCode { - // check values from http header - for k, val := range r.Header { - if stringsHasPrefixFold(k, "X-Amz-Meta-") { - if signedHeadersMap.Get(k) == val[0] { - continue - } +// checkUnsignedHeaders rejects any x-amz-* request header that is not covered by +// the SigV4 signed-headers list. AWS S3 requires every x-amz-* header to be +// signed and returns AccessDenied ("There were headers present in the request +// which were not signed") otherwise. Enforcing the same here prevents an +// unsigned x-amz-* header (for example x-amz-copy-source) from changing the +// semantics of an already-signed or presigned request: without this check a +// presigned PUT grant could be turned into a server-side copy that reads any +// object the signing key can reach. +// +// Only headers actually sent by the client are inspected. Server-synthesized +// x-amz-* headers (e.g. x-amz-tagging derived from a request body, or the +// post-verification x-amz-signature-age scratch header) are set after signature +// verification and therefore never reach this walk. +func checkUnsignedHeaders(signedHeadersMap http.Header, r *http.Request) APIErrorCode { + // check headers that arrived on the request + for k := range r.Header { + if !stringsHasPrefixFold(k, "X-Amz-") { + continue + } + // X-Amz-Content-Sha256 carries the payload hash, not an operation or + // authorization input, and is handled specially: for presigned requests + // it is read from the query string (getContentSha256Cksum) and any + // header copy is ignored, while for signed requests it is bound into the + // string-to-sign as the payload hash, so a tampered value fails + // signature verification regardless of the signed-headers list. Some + // clients send it as an unsigned header, so exempt it to preserve + // compatibility without weakening the operation-header protection. + if strings.EqualFold(k, xhttp.AmzContentSha256) { + continue + } + // X-Amz-Signature-Age is an internal scratch header written by the + // presigned verifier itself, after this check, purely so bucket-policy + // evaluation can expose s3:signatureAge. It is never sent or signed by a + // client, and exempting it keeps signature verification idempotent when + // the same request is verified more than once. + if strings.EqualFold(k, xhttp.AmzSignatureAge) { + continue + } + // The header must be a member of the signed-headers list. Testing + // membership (not value equality) is essential: an unsigned header whose + // first value is empty would otherwise compare equal to the empty string + // returned for an absent key and slip through. + if _, ok := signedHeadersMap[http.CanonicalHeaderKey(k)]; !ok { return ErrUnsignedHeaders } } diff --git a/cmd/signature-v4-utils_test.go b/cmd/signature-v4-utils_test.go index 74830fc9a..d957908b9 100644 --- a/cmd/signature-v4-utils_test.go +++ b/cmd/signature-v4-utils_test.go @@ -363,8 +363,8 @@ func TestGetContentSha256Cksum(t *testing.T) { } } -// Test TestCheckMetaHeaders tests the logic of checkMetaHeaders() function -func TestCheckMetaHeaders(t *testing.T) { +// Test TestCheckUnsignedHeaders tests the logic of checkUnsignedHeaders() function +func TestCheckUnsignedHeaders(t *testing.T) { signedHeadersMap := map[string][]string{ "X-Amz-Meta-Test": {"test"}, "X-Amz-Meta-Extension": {"png"}, @@ -384,7 +384,7 @@ func TestCheckMetaHeaders(t *testing.T) { inputHeader.Set("X-Amz-Meta-Extension", expectedMetaExtension) inputHeader.Set("X-Amz-Meta-Name", expectedMetaName) // calling the function being tested. - errCode := checkMetaHeaders(signedHeadersMap, r) + errCode := checkUnsignedHeaders(signedHeadersMap, r) if errCode != ErrNone { t.Fatalf("Expected the APIErrorCode to be %d, but got %d", ErrNone, errCode) } @@ -392,7 +392,7 @@ func TestCheckMetaHeaders(t *testing.T) { // Add new metadata in inputHeader inputHeader.Set("X-Amz-Meta-Clone", "fail") // calling the function being tested. - errCode = checkMetaHeaders(signedHeadersMap, r) + errCode = checkUnsignedHeaders(signedHeadersMap, r) if errCode != ErrUnsignedHeaders { t.Fatalf("Expected the APIErrorCode to be %d, but got %d", ErrUnsignedHeaders, errCode) } @@ -400,7 +400,7 @@ func TestCheckMetaHeaders(t *testing.T) { // Delete extra metadata from header to don't affect other test inputHeader.Del("X-Amz-Meta-Clone") // calling the function being tested. - errCode = checkMetaHeaders(signedHeadersMap, r) + errCode = checkUnsignedHeaders(signedHeadersMap, r) if errCode != ErrNone { t.Fatalf("Expected the APIErrorCode to be %d, but got %d", ErrNone, errCode) } @@ -413,8 +413,71 @@ func TestCheckMetaHeaders(t *testing.T) { r.ParseForm() // calling the function being tested. - errCode = checkMetaHeaders(signedHeadersMap, r) + errCode = checkUnsignedHeaders(signedHeadersMap, r) if errCode != ErrNone { t.Fatalf("Expected the APIErrorCode to be %d, but got %d", ErrNone, errCode) } + + // Regression for the unsigned x-amz-copy-source coverage gap: an x-amz-* + // header outside the signed-headers list (here x-amz-copy-source, which the + // router uses to select CopyObjectHandler) must be rejected. Previously only + // x-amz-meta-* headers were inspected, so this header slipped through and a + // presigned/authorized PUT could be turned into a server-side copy. + r, err = http.NewRequest(http.MethodPut, "http://play.min.io:9000", nil) + if err != nil { + t.Fatal("Unable to create http.Request :", err) + } + r.Header.Set("X-Amz-Copy-Source", "/src/secret.txt") + if errCode = checkUnsignedHeaders(signedHeadersMap, r); errCode != ErrUnsignedHeaders { + t.Fatalf("unsigned x-amz-copy-source: expected %d, got %d", ErrUnsignedHeaders, errCode) + } + + // When the same header is part of the signed-headers list with a matching + // value it is allowed through, exactly as for x-amz-meta-*. + signedWithCopy := http.Header{} + for k, v := range signedHeadersMap { + signedWithCopy[k] = v + } + signedWithCopy.Set("X-Amz-Copy-Source", "/src/secret.txt") + if errCode = checkUnsignedHeaders(signedWithCopy, r); errCode != ErrNone { + t.Fatalf("signed x-amz-copy-source: expected %d, got %d", ErrNone, errCode) + } + + // Membership, not value equality: an unsigned x-amz-* header whose first + // value is empty must still be rejected. A value-equality check would + // compare "" against the empty string returned for an absent signed header + // and wrongly let it through, so a multi-value header like + // {"", "/src/secret.txt"} could smuggle an unsigned copy-source. + r, err = http.NewRequest(http.MethodPut, "http://play.min.io:9000", nil) + if err != nil { + t.Fatal("Unable to create http.Request :", err) + } + r.Header["X-Amz-Copy-Source"] = []string{"", "/src/secret.txt"} + if errCode = checkUnsignedHeaders(signedHeadersMap, r); errCode != ErrUnsignedHeaders { + t.Fatalf("empty-first unsigned x-amz-copy-source: expected %d, got %d", ErrUnsignedHeaders, errCode) + } + + // X-Amz-Content-Sha256 is exempt: it carries the payload hash (handled from + // the query for presigned and bound into the string-to-sign for signed + // requests), so it is allowed even when it is not in the signed-headers map. + r, err = http.NewRequest(http.MethodPut, "http://play.min.io:9000", nil) + if err != nil { + t.Fatal("Unable to create http.Request :", err) + } + r.Header.Set(xhttp.AmzContentSha256, unsignedPayload) + if errCode = checkUnsignedHeaders(signedHeadersMap, r); errCode != ErrNone { + t.Fatalf("unsigned x-amz-content-sha256 must be exempt: expected %d, got %d", ErrNone, errCode) + } + + // X-Amz-Signature-Age is the presigned verifier's own scratch header, + // written after this check. Exempting it keeps verification idempotent when + // the same request object is verified more than once. + r, err = http.NewRequest(http.MethodPut, "http://play.min.io:9000", nil) + if err != nil { + t.Fatal("Unable to create http.Request :", err) + } + r.Header.Set(xhttp.AmzSignatureAge, "1234") + if errCode = checkUnsignedHeaders(signedHeadersMap, r); errCode != ErrNone { + t.Fatalf("internal x-amz-signature-age must be exempt: expected %d, got %d", ErrNone, errCode) + } } diff --git a/cmd/signature-v4.go b/cmd/signature-v4.go index ceb8b4b2f..f204cd33c 100644 --- a/cmd/signature-v4.go +++ b/cmd/signature-v4.go @@ -229,10 +229,11 @@ func doesPresignedSignatureMatch(hashedPayload string, r *http.Request, region s return errCode } - // Check if the metadata headers are equal with signedheaders - errMetaCode := checkMetaHeaders(extractedSignedHeaders, r) - if errMetaCode != ErrNone { - return errMetaCode + // Reject any x-amz-* header that the client did not sign. Without this an + // unsigned header (e.g. x-amz-copy-source) could alter the request that the + // presigned URL actually authorized. + if errUnsigned := checkUnsignedHeaders(extractedSignedHeaders, r); errUnsigned != ErrNone { + return errUnsigned } // If the host which signed the request is slightly ahead in time (by less than globalMaxSkewTime) the @@ -335,7 +336,7 @@ func doesPresignedSignatureMatch(hashedPayload string, r *http.Request, region s return ErrSignatureDoesNotMatch } - r.Header.Set("x-amz-signature-age", strconv.FormatInt(UTCNow().Sub(pSignValues.Date).Milliseconds(), 10)) + r.Header.Set(xhttp.AmzSignatureAge, strconv.FormatInt(UTCNow().Sub(pSignValues.Date).Milliseconds(), 10)) return ErrNone } @@ -363,6 +364,14 @@ func doesSignatureMatch(hashedPayload string, r *http.Request, region string, st return errCode } + // Reject any x-amz-* header that the client did not sign. The Authorization + // header path shares extractSignedHeaders with the presigned path but, prior + // to this, never inspected the headers that actually arrived, so an unsigned + // x-amz-copy-source could redirect a signed PUT into a server-side copy. + if errUnsigned := checkUnsignedHeaders(extractedSignedHeaders, r); errUnsigned != ErrNone { + return errUnsigned + } + cred, _, s3Err := checkKeyValid(r, signV4Values.Credential.accessKey) if s3Err != ErrNone { return s3Err diff --git a/cmd/signature-v4_test.go b/cmd/signature-v4_test.go index a0f5d8155..72129940d 100644 --- a/cmd/signature-v4_test.go +++ b/cmd/signature-v4_test.go @@ -25,6 +25,8 @@ import ( "os" "testing" "time" + + xhttp "github.com/minio/minio/internal/http" ) func niceError(code APIErrorCode) string { @@ -313,3 +315,42 @@ func TestDoesPresignedSignatureMatch(t *testing.T) { } } } + +// TestPresignedVerifyIdempotent guards against a regression where verifying the +// same presigned request twice began to fail. doesPresignedSignatureMatch +// writes an internal x-amz-signature-age header after validating the signature; +// the unsigned-header check must exempt that scratch header (and an unsigned +// x-amz-content-sha256 the client may carry) so a second verification of the +// same *http.Request still succeeds. +func TestPresignedVerifyIdempotent(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + obj, fsDir, err := prepareFS(ctx) + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(fsDir) + if err = newTestConfig(globalMinioDefaultRegion, obj); err != nil { + t.Fatal(err) + } + + req, err := newTestRequest(http.MethodGet, "http://127.0.0.1:9000/bucket/object", 0, nil) + if err != nil { + t.Fatal(err) + } + if err = preSignV4(req, globalActiveCred.AccessKey, globalActiveCred.SecretKey, int64(10*60)); err != nil { + t.Fatal(err) + } + if err = req.ParseForm(); err != nil { + t.Fatal(err) + } + + if got := reqSignatureV4Verify(req, globalSite.Region(), serviceS3); got != ErrNone { + t.Fatalf("first verification: expected ErrNone, got %s", niceError(got)) + } + if got := reqSignatureV4Verify(req, globalSite.Region(), serviceS3); got != ErrNone { + t.Fatalf("second verification of the same request: expected ErrNone, got %s (x-amz-signature-age=%q)", + niceError(got), req.Header.Get(xhttp.AmzSignatureAge)) + } +} diff --git a/internal/http/headers.go b/internal/http/headers.go index 9195e122e..93e55264f 100644 --- a/internal/http/headers.go +++ b/internal/http/headers.go @@ -132,6 +132,11 @@ const ( AmzMaxParts = "X-Amz-Max-Parts" AmzPartNumberMarker = "X-Amz-Part-Number-Marker" + // AmzSignatureAge is an internal scratch header the presigned verifier + // writes after validating the signature so that bucket-policy evaluation can + // expose s3:signatureAge. It is never sent or signed by a client. + AmzSignatureAge = "X-Amz-Signature-Age" + // Constants used for GetObjectAttributes and GetObjectVersionAttributes AmzObjectAttributes = "X-Amz-Object-Attributes"