From 8c70975283f9f4ce80f331a25c7475a36279e519 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Thu, 3 Apr 2025 07:55:52 -0700 Subject: [PATCH] make sure to validate signature unsigned trailer stream (#21103) This is a security incident fix, it would seem like since the implementation of unsigned payload trailer on PUTs, we do not validate the signature of the incoming request. The signature can be invalid and is totally being ignored, this in-turn allows any arbitrary secret to upload objects given the user has "WRITE" permissions on the bucket, since acces-key is a public information in general exposes these potential users with WRITE on the bucket to be used by any arbitrary client to make a fake request to MinIO the signature under Authorization: header is totally ignored. A test has been added to cover this scenario and fail appropriately. --- cmd/auth-handler.go | 30 ++--------------- cmd/object-handlers.go | 15 +++------ cmd/object-multipart-handlers.go | 2 +- cmd/server_test.go | 55 ++++++++++++++++++++++++++++++++ cmd/streaming-v4-unsigned.go | 7 +++- 5 files changed, 69 insertions(+), 40 deletions(-) diff --git a/cmd/auth-handler.go b/cmd/auth-handler.go index 53d285e4f..7b831d76b 100644 --- a/cmd/auth-handler.go +++ b/cmd/auth-handler.go @@ -96,7 +96,7 @@ func isRequestSignStreamingTrailerV4(r *http.Request) bool { // Verify if the request has AWS Streaming Signature Version '4', with unsigned content and trailer. func isRequestUnsignedTrailerV4(r *http.Request) bool { return r.Header.Get(xhttp.AmzContentSha256) == unsignedPayloadTrailer && - r.Method == http.MethodPut && strings.Contains(r.Header.Get(xhttp.ContentEncoding), streamingContentEncoding) + r.Method == http.MethodPut } // Authorization type. @@ -363,7 +363,7 @@ func authenticateRequest(ctx context.Context, r *http.Request, action policy.Act var cred auth.Credentials var owner bool switch getRequestAuthType(r) { - case authTypeUnknown, authTypeStreamingSigned: + case authTypeUnknown, authTypeStreamingSigned, authTypeStreamingSignedTrailer, authTypeStreamingUnsignedTrailer: return ErrSignatureVersionNotSupported case authTypePresignedV2, authTypeSignedV2: if s3Err = isReqAuthenticatedV2(r); s3Err != ErrNone { @@ -674,32 +674,6 @@ func setAuthMiddleware(h http.Handler) http.Handler { }) } -func validateSignature(atype authType, r *http.Request) (auth.Credentials, bool, APIErrorCode) { - var cred auth.Credentials - var owner bool - var s3Err APIErrorCode - switch atype { - case authTypeUnknown, authTypeStreamingSigned: - return cred, owner, ErrSignatureVersionNotSupported - case authTypeSignedV2, authTypePresignedV2: - if s3Err = isReqAuthenticatedV2(r); s3Err != ErrNone { - return cred, owner, s3Err - } - cred, owner, s3Err = getReqAccessKeyV2(r) - case authTypePresigned, authTypeSigned: - region := globalSite.Region() - if s3Err = isReqAuthenticated(GlobalContext, r, region, serviceS3); s3Err != ErrNone { - return cred, owner, s3Err - } - cred, owner, s3Err = getReqAccessKeyV4(r, region, serviceS3) - } - if s3Err != ErrNone { - return cred, owner, s3Err - } - - return cred, owner, ErrNone -} - func isPutRetentionAllowed(bucketName, objectName string, retDays int, retDate time.Time, retMode objectlock.RetMode, byPassSet bool, r *http.Request, cred auth.Credentials, owner bool) (s3Err APIErrorCode) { var retSet bool if cred.AccessKey == "" { diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index fc42d2feb..b9876d743 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -1850,7 +1850,7 @@ func (api objectAPIHandlers) PutObjectHandler(w http.ResponseWriter, r *http.Req } case authTypeStreamingUnsignedTrailer: // Initialize stream chunked reader with optional trailers. - rd, s3Err = newUnsignedV4ChunkedReader(r, true) + rd, s3Err = newUnsignedV4ChunkedReader(r, true, r.Header.Get(xhttp.Authorization) != "") if s3Err != ErrNone { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Err), r.URL) return @@ -2879,14 +2879,8 @@ func (api objectAPIHandlers) PutObjectRetentionHandler(w http.ResponseWriter, r } // Check permissions to perform this object retention operation - if s3Err := checkRequestAuthType(ctx, r, policy.PutObjectRetentionAction, bucket, object); s3Err != ErrNone { - writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Err), r.URL) - return - } - - cred, owner, s3Err := validateSignature(getRequestAuthType(r), r) - if s3Err != ErrNone { - writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Err), r.URL) + if s3Error := authenticateRequest(ctx, r, policy.PutObjectRetentionAction); s3Error != ErrNone { + writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL) return } @@ -2912,6 +2906,7 @@ func (api objectAPIHandlers) PutObjectRetentionHandler(w http.ResponseWriter, r writeErrorResponse(ctx, w, apiErr, r.URL) return } + reqInfo := logger.GetReqInfo(ctx) reqInfo.SetTags("retention", objRetention.String()) @@ -2925,7 +2920,7 @@ func (api objectAPIHandlers) PutObjectRetentionHandler(w http.ResponseWriter, r MTime: opts.MTime, VersionID: opts.VersionID, EvalMetadataFn: func(oi *ObjectInfo, gerr error) (dsc ReplicateDecision, err error) { - if err := enforceRetentionBypassForPut(ctx, r, *oi, objRetention, cred, owner); err != nil { + if err := enforceRetentionBypassForPut(ctx, r, *oi, objRetention, reqInfo.Cred, reqInfo.Owner); err != nil { return dsc, err } if objRetention.Mode.Valid() { diff --git a/cmd/object-multipart-handlers.go b/cmd/object-multipart-handlers.go index c36d0abb0..f074638c5 100644 --- a/cmd/object-multipart-handlers.go +++ b/cmd/object-multipart-handlers.go @@ -682,7 +682,7 @@ func (api objectAPIHandlers) PutObjectPartHandler(w http.ResponseWriter, r *http } case authTypeStreamingUnsignedTrailer: // Initialize stream signature verifier. - reader, s3Error = newUnsignedV4ChunkedReader(r, true) + reader, s3Error = newUnsignedV4ChunkedReader(r, true, r.Header.Get(xhttp.Authorization) != "") if s3Error != ErrNone { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL) return diff --git a/cmd/server_test.go b/cmd/server_test.go index 77be4038b..ec49d89bd 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -37,6 +37,7 @@ import ( "github.com/dustin/go-humanize" jwtgo "github.com/golang-jwt/jwt/v4" "github.com/minio/minio-go/v7/pkg/set" + "github.com/minio/minio-go/v7/pkg/signer" xhttp "github.com/minio/minio/internal/http" "github.com/minio/pkg/v3/policy" ) @@ -126,6 +127,7 @@ func runAllTests(suite *TestSuiteCommon, c *check) { suite.TestMetricsV3Handler(c) suite.TestBucketSQSNotificationWebHook(c) suite.TestBucketSQSNotificationAMQP(c) + suite.TestUnsignedCVE(c) suite.TearDownSuite(c) } @@ -354,6 +356,59 @@ func (s *TestSuiteCommon) TestObjectDir(c *check) { c.Assert(response.StatusCode, http.StatusNoContent) } +func (s *TestSuiteCommon) TestUnsignedCVE(c *check) { + c.Helper() + + // generate a random bucket Name. + bucketName := getRandomBucketName() + + // HTTP request to create the bucket. + request, err := newTestSignedRequest(http.MethodPut, getMakeBucketURL(s.endPoint, bucketName), + 0, nil, s.accessKey, s.secretKey, s.signer) + c.Assert(err, nil) + + // execute the request. + response, err := s.client.Do(request) + c.Assert(err, nil) + + // assert the http response status code. + c.Assert(response.StatusCode, http.StatusOK) + + req, err := http.NewRequest(http.MethodPut, getPutObjectURL(s.endPoint, bucketName, "test-cve-object.txt"), nil) + c.Assert(err, nil) + + req.Body = io.NopCloser(bytes.NewReader([]byte("foobar!\n"))) + req.Trailer = http.Header{} + req.Trailer.Set("x-amz-checksum-crc32", "rK0DXg==") + + now := UTCNow() + + req = signer.StreamingUnsignedV4(req, "", 8, now) + + maliciousHeaders := http.Header{ + "Authorization": []string{fmt.Sprintf("AWS4-HMAC-SHA256 Credential=%s/%s/us-east-1/s3/aws4_request, SignedHeaders=invalidheader, Signature=deadbeefdeadbeefdeadbeeddeadbeeddeadbeefdeadbeefdeadbeefdeadbeef", s.accessKey, now.Format(yyyymmdd))}, + "User-Agent": []string{"A malicious request"}, + "X-Amz-Decoded-Content-Length": []string{"8"}, + "Content-Encoding": []string{"aws-chunked"}, + "X-Amz-Trailer": []string{"x-amz-checksum-crc32"}, + "x-amz-content-sha256": []string{unsignedPayloadTrailer}, + } + + for k, v := range maliciousHeaders { + req.Header.Set(k, v[0]) + } + + // execute the request. + response, err = s.client.Do(req) + c.Assert(err, nil) + + // out, err = httputil.DumpResponse(response, true) + // fmt.Println("RESPONSE ===\n", string(out), err) + + // assert the http response status code. + c.Assert(response.StatusCode, http.StatusBadRequest) +} + func (s *TestSuiteCommon) TestBucketSQSNotificationAMQP(c *check) { // Sample bucket notification. bucketNotificationBuf := `s3:ObjectCreated:Putprefiximages/1arn:minio:sqs:us-east-1:444455556666:amqp` diff --git a/cmd/streaming-v4-unsigned.go b/cmd/streaming-v4-unsigned.go index e0acb95e4..a31668678 100644 --- a/cmd/streaming-v4-unsigned.go +++ b/cmd/streaming-v4-unsigned.go @@ -29,7 +29,12 @@ import ( // newUnsignedV4ChunkedReader returns a new s3UnsignedChunkedReader that translates the data read from r // out of HTTP "chunked" format before returning it. // The s3ChunkedReader returns io.EOF when the final 0-length chunk is read. -func newUnsignedV4ChunkedReader(req *http.Request, trailer bool) (io.ReadCloser, APIErrorCode) { +func newUnsignedV4ChunkedReader(req *http.Request, trailer bool, signature bool) (io.ReadCloser, APIErrorCode) { + if signature { + if errCode := doesSignatureMatch(unsignedPayloadTrailer, req, globalSite.Region(), serviceS3); errCode != ErrNone { + return nil, errCode + } + } if trailer { // Discard anything unsigned. req.Trailer = make(http.Header)