From 98a7b7f402de2b3cdd47559f373b675e01473138 Mon Sep 17 00:00:00 2001 From: niksis02 Date: Fri, 4 Jul 2025 00:32:30 +0400 Subject: [PATCH] feat: adds a middleware to validate bucket/object names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements a middleware that validates incoming bucket and object names before authentication. This helps prevent malicious attacks that attempt to access restricted or unreachable data in `POSIX`. Adds test cases to cover such attack scenarios, including false negatives where encoded paths are used to try accessing resources outside the intended bucket. Removes bucket validation from all other layers—including `controllers` and both `POSIX` and `ScoutFS` backends — by moving the logic entirely into the middleware layer. --- backend/posix/posix.go | 58 ------------- backend/scoutfs/scoutfs.go | 9 -- s3api/controllers/base.go | 8 -- s3api/controllers/base_test.go | 9 -- .../bucket-object-name-validator.go | 58 +++++++++++++ s3api/server.go | 3 + s3err/s3err.go | 6 ++ tests/integration/group-tests.go | 5 ++ tests/integration/tests.go | 83 ++++++++++++++++++- 9 files changed, 151 insertions(+), 88 deletions(-) create mode 100644 s3api/middlewares/bucket-object-name-validator.go diff --git a/backend/posix/posix.go b/backend/posix/posix.go index 76b1e87b..de4214e9 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -334,10 +334,6 @@ func (p *Posix) ListBuckets(_ context.Context, input s3response.ListBucketsInput } func (p *Posix) HeadBucket(_ context.Context, input *s3.HeadBucketInput) (*s3.HeadBucketOutput, error) { - if input.Bucket == nil { - return nil, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } - _, err := os.Lstat(*input.Bucket) if errors.Is(err, fs.ErrNotExist) { return nil, s3err.GetAPIError(s3err.ErrNoSuchBucket) @@ -350,10 +346,6 @@ func (p *Posix) HeadBucket(_ context.Context, input *s3.HeadBucketInput) (*s3.He } func (p *Posix) CreateBucket(ctx context.Context, input *s3.CreateBucketInput, acl []byte) error { - if input.Bucket == nil { - return s3err.GetAPIError(s3err.ErrInvalidBucketName) - } - acct, ok := ctx.Value("account").(auth.Account) if !ok { acct = auth.Account{} @@ -465,10 +457,6 @@ func (p *Posix) isBucketEmpty(bucket string) error { } func (p *Posix) DeleteBucket(_ context.Context, bucket string) error { - if bucket == "" { - return s3err.GetAPIError(s3err.ErrInvalidBucketName) - } - // Check if the bucket is empty err := p.isBucketEmpty(bucket) if err != nil { @@ -1189,9 +1177,6 @@ func (p *Posix) fileToObjVersions(bucket string) backend.GetVersionsFunc { } func (p *Posix) CreateMultipartUpload(ctx context.Context, mpu s3response.CreateMultipartUploadInput) (s3response.InitiateMultipartUploadResult, error) { - if mpu.Bucket == nil { - return s3response.InitiateMultipartUploadResult{}, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } if mpu.Key == nil { return s3response.InitiateMultipartUploadResult{}, s3err.GetAPIError(s3err.ErrNoSuchKey) } @@ -1380,9 +1365,6 @@ func (p *Posix) CompleteMultipartUpload(ctx context.Context, input *s3.CompleteM var res s3response.CompleteMultipartUploadResult - if input.Bucket == nil { - return res, "", s3err.GetAPIError(s3err.ErrInvalidBucketName) - } if input.Key == nil { return res, "", s3err.GetAPIError(s3err.ErrNoSuchKey) } @@ -1950,9 +1932,6 @@ func isValidMeta(val string) bool { } func (p *Posix) AbortMultipartUpload(_ context.Context, mpu *s3.AbortMultipartUploadInput) error { - if mpu.Bucket == nil { - return s3err.GetAPIError(s3err.ErrInvalidBucketName) - } if mpu.Key == nil { return s3err.GetAPIError(s3err.ErrNoSuchKey) } @@ -1992,10 +1971,6 @@ func (p *Posix) AbortMultipartUpload(_ context.Context, mpu *s3.AbortMultipartUp func (p *Posix) ListMultipartUploads(_ context.Context, mpu *s3.ListMultipartUploadsInput) (s3response.ListMultipartUploadsResult, error) { var lmu s3response.ListMultipartUploadsResult - if mpu.Bucket == nil { - return lmu, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } - bucket := *mpu.Bucket var delimiter string if mpu.Delimiter != nil { @@ -2143,9 +2118,6 @@ func (p *Posix) ListMultipartUploads(_ context.Context, mpu *s3.ListMultipartUpl func (p *Posix) ListParts(ctx context.Context, input *s3.ListPartsInput) (s3response.ListPartsResult, error) { var lpr s3response.ListPartsResult - if input.Bucket == nil { - return lpr, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } if input.Key == nil { return lpr, s3err.GetAPIError(s3err.ErrNoSuchKey) } @@ -2299,9 +2271,6 @@ func (p *Posix) UploadPart(ctx context.Context, input *s3.UploadPartInput) (*s3. acct = auth.Account{} } - if input.Bucket == nil { - return nil, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } if input.Key == nil { return nil, s3err.GetAPIError(s3err.ErrNoSuchKey) } @@ -2469,9 +2438,6 @@ func (p *Posix) UploadPartCopy(ctx context.Context, upi *s3.UploadPartCopyInput) acct = auth.Account{} } - if upi.Bucket == nil { - return s3response.CopyPartResult{}, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } if upi.Key == nil { return s3response.CopyPartResult{}, s3err.GetAPIError(s3err.ErrNoSuchKey) } @@ -2683,9 +2649,6 @@ func (p *Posix) PutObject(ctx context.Context, po s3response.PutObjectInput) (s3 acct = auth.Account{} } - if po.Bucket == nil { - return s3response.PutObjectOutput{}, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } if po.Key == nil { return s3response.PutObjectOutput{}, s3err.GetAPIError(s3err.ErrNoSuchKey) } @@ -3003,9 +2966,6 @@ func (p *Posix) PutObject(ctx context.Context, po s3response.PutObjectInput) (s3 } func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) (*s3.DeleteObjectOutput, error) { - if input.Bucket == nil { - return nil, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } if input.Key == nil { return nil, s3err.GetAPIError(s3err.ErrNoSuchKey) } @@ -3361,9 +3321,6 @@ func (p *Posix) DeleteObjects(ctx context.Context, input *s3.DeleteObjectsInput) } func (p *Posix) GetObject(_ context.Context, input *s3.GetObjectInput) (*s3.GetObjectOutput, error) { - if input.Bucket == nil { - return nil, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } if input.Key == nil { return nil, s3err.GetAPIError(s3err.ErrNoSuchKey) } @@ -3592,9 +3549,6 @@ func (p *Posix) GetObject(_ context.Context, input *s3.GetObjectInput) (*s3.GetO } func (p *Posix) HeadObject(ctx context.Context, input *s3.HeadObjectInput) (*s3.HeadObjectOutput, error) { - if input.Bucket == nil { - return nil, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } if input.Key == nil { return nil, s3err.GetAPIError(s3err.ErrNoSuchKey) } @@ -3862,9 +3816,6 @@ func (p *Posix) GetObjectAttributes(ctx context.Context, input *s3.GetObjectAttr } func (p *Posix) CopyObject(ctx context.Context, input s3response.CopyObjectInput) (s3response.CopyObjectOutput, error) { - if input.Bucket == nil { - return s3response.CopyObjectOutput{}, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } if input.Key == nil { return s3response.CopyObjectOutput{}, s3err.GetAPIError(s3err.ErrInvalidCopyDest) } @@ -4177,9 +4128,6 @@ func (p *Posix) CopyObject(ctx context.Context, input s3response.CopyObjectInput } func (p *Posix) ListObjects(ctx context.Context, input *s3.ListObjectsInput) (s3response.ListObjectsResult, error) { - if input.Bucket == nil { - return s3response.ListObjectsResult{}, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } bucket := *input.Bucket prefix := "" if input.Prefix != nil { @@ -4329,9 +4277,6 @@ func (p *Posix) fileToObj(bucket string, fetchOwner bool) backend.GetObjFunc { } func (p *Posix) ListObjectsV2(ctx context.Context, input *s3.ListObjectsV2Input) (s3response.ListObjectsV2Result, error) { - if input.Bucket == nil { - return s3response.ListObjectsV2Result{}, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } bucket := *input.Bucket prefix := "" if input.Prefix != nil { @@ -4408,9 +4353,6 @@ func (p *Posix) PutBucketAcl(_ context.Context, bucket string, data []byte) erro } func (p *Posix) GetBucketAcl(_ context.Context, input *s3.GetBucketAclInput) ([]byte, error) { - if input.Bucket == nil { - return nil, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } _, err := os.Stat(*input.Bucket) if errors.Is(err, fs.ErrNotExist) { return nil, s3err.GetAPIError(s3err.ErrNoSuchBucket) diff --git a/backend/scoutfs/scoutfs.go b/backend/scoutfs/scoutfs.go index 462cfc3f..ddffd06e 100644 --- a/backend/scoutfs/scoutfs.go +++ b/backend/scoutfs/scoutfs.go @@ -201,9 +201,6 @@ func (s *ScoutFS) CompleteMultipartUpload(ctx context.Context, input *s3.Complet var res s3response.CompleteMultipartUploadResult - if input.Bucket == nil { - return res, "", s3err.GetAPIError(s3err.ErrInvalidBucketName) - } if input.Key == nil { return res, "", s3err.GetAPIError(s3err.ErrNoSuchKey) } @@ -730,9 +727,6 @@ func (s *ScoutFS) GetObject(ctx context.Context, input *s3.GetObjectInput) (*s3. } func (s *ScoutFS) ListObjects(ctx context.Context, input *s3.ListObjectsInput) (s3response.ListObjectsResult, error) { - if input.Bucket == nil { - return s3response.ListObjectsResult{}, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } bucket := *input.Bucket prefix := "" if input.Prefix != nil { @@ -780,9 +774,6 @@ func (s *ScoutFS) ListObjects(ctx context.Context, input *s3.ListObjectsInput) ( } func (s *ScoutFS) ListObjectsV2(ctx context.Context, input *s3.ListObjectsV2Input) (s3response.ListObjectsV2Result, error) { - if input.Bucket == nil { - return s3response.ListObjectsV2Result{}, s3err.GetAPIError(s3err.ErrInvalidBucketName) - } bucket := *input.Bucket prefix := "" if input.Prefix != nil { diff --git a/s3api/controllers/base.go b/s3api/controllers/base.go index 0aa14030..ea829ff4 100644 --- a/s3api/controllers/base.go +++ b/s3api/controllers/base.go @@ -1720,14 +1720,6 @@ func (c S3ApiController) PutBucketActions(ctx *fiber.Ctx) error { }) } - if ok := utils.IsValidBucketName(bucket); !ok { - return SendResponse(ctx, s3err.GetAPIError(s3err.ErrInvalidBucketName), - &MetaOpts{ - Logger: c.logger, - MetricsMng: c.mm, - Action: metrics.ActionCreateBucket, - }) - } if ok := utils.IsValidOwnership(objectOwnership); !ok { return SendResponse(ctx, s3err.APIError{ Code: "InvalidArgument", diff --git a/s3api/controllers/base_test.go b/s3api/controllers/base_test.go index 74d1636c..f9d83d54 100644 --- a/s3api/controllers/base_test.go +++ b/s3api/controllers/base_test.go @@ -884,15 +884,6 @@ func TestS3ApiController_PutBucketActions(t *testing.T) { wantErr: false, statusCode: 400, }, - { - name: "Create-bucket-invalid-bucket-name", - app: app, - args: args{ - req: httptest.NewRequest(http.MethodPut, "/aa", nil), - }, - wantErr: false, - statusCode: 400, - }, { name: "Create-bucket-success", app: app, diff --git a/s3api/middlewares/bucket-object-name-validator.go b/s3api/middlewares/bucket-object-name-validator.go new file mode 100644 index 00000000..1abd8a05 --- /dev/null +++ b/s3api/middlewares/bucket-object-name-validator.go @@ -0,0 +1,58 @@ +// Copyright 2023 Versity Software +// This file is 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 middlewares + +import ( + "net/http" + + "github.com/gofiber/fiber/v2" + "github.com/versity/versitygw/metrics" + "github.com/versity/versitygw/s3api/utils" + "github.com/versity/versitygw/s3err" + "github.com/versity/versitygw/s3log" +) + +// BucketObjectNameValidator extracts and validates +// the bucket and object names from the request URI. +func BucketObjectNameValidator(l s3log.AuditLogger, mm *metrics.Manager) fiber.Handler { + return func(ctx *fiber.Ctx) error { + // skip the check for admin apis + if ctx.Method() == http.MethodPatch { + return ctx.Next() + } + + path := ctx.Path() + // skip the check if the operation isn't bucket/object scoped + // e.g ListBuckets + if path == "/" { + return ctx.Next() + } + + bucket, object := parsePath(path) + + // check if the provided bucket name is valid + if !utils.IsValidBucketName(bucket) { + return sendResponse(ctx, s3err.GetAPIError(s3err.ErrInvalidBucketName), l, mm) + } + + // check if the provided object name is valid + // skip for empty objects: e.g bucket operations: HeadBucket... + if object != "" && !utils.IsObjectNameValid(object) { + return sendResponse(ctx, s3err.GetAPIError(s3err.ErrBadRequest), l, mm) + } + + return ctx.Next() + } +} diff --git a/s3api/server.go b/s3api/server.go index c23b0650..4583e9da 100644 --- a/s3api/server.go +++ b/s3api/server.go @@ -91,6 +91,9 @@ func New( app.Use(middlewares.DebugLogger()) } + // initialize the bucket/object name validator + app.Use(middlewares.BucketObjectNameValidator(l, mm)) + // Public buckets access checker app.Use(middlewares.AuthorizePublicBucketAccess(be, l, mm)) diff --git a/s3err/s3err.go b/s3err/s3err.go index 0d8b41fc..59aca425 100644 --- a/s3err/s3err.go +++ b/s3err/s3err.go @@ -167,6 +167,7 @@ const ( ErrChecksumTypeWithAlgo ErrInvalidChecksumHeader ErrTrailerHeaderNotSupported + ErrBadRequest // Non-AWS errors ErrExistingObjectIsDirectory @@ -726,6 +727,11 @@ var errorCodeResponse = map[ErrorCode]APIError{ Description: "The value specified in the x-amz-trailer header is not supported", HTTPStatusCode: http.StatusBadRequest, }, + ErrBadRequest: { + Code: "400", + Description: "Bad Request", + HTTPStatusCode: http.StatusBadRequest, + }, // non aws errors ErrExistingObjectIsDirectory: { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index deec793d..6435e736 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -150,12 +150,15 @@ func TestPutObject(s *S3Conf) { PutObject_incorrect_checksums(s) PutObject_default_checksum(s) PutObject_checksums_success(s) + // azure applies some encoding mechanisms. + PutObject_false_negative_object_names(s) } PutObject_success(s) if !s.versioningEnabled { PutObject_racey_success(s) } PutObject_invalid_credentials(s) + PutObject_invalid_object_names(s) } func TestHeadObject(s *S3Conf) { @@ -969,6 +972,8 @@ func GetIntTests() IntTests { "PutObject_special_chars": PutObject_special_chars, "PutObject_tagging": PutObject_tagging, "PutObject_success": PutObject_success, + "PutObject_invalid_object_names": PutObject_invalid_object_names, + "PutObject_false_negative_object_names": PutObject_false_negative_object_names, "PutObject_racey_success": PutObject_racey_success, "HeadObject_non_existing_object": HeadObject_non_existing_object, "HeadObject_invalid_part_number": HeadObject_invalid_part_number, diff --git a/tests/integration/tests.go b/tests/integration/tests.go index a2351211..74345044 100644 --- a/tests/integration/tests.go +++ b/tests/integration/tests.go @@ -3415,6 +3415,81 @@ func PutObject_invalid_credentials(s *S3Conf) error { }) } +func PutObject_invalid_object_names(s *S3Conf) error { + testName := "PutObject_invalid_object_names" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + for _, obj := range []string{ + ".", + "..", + "./", + "/.", + "//", + "../", + "/..", + "/..", + "../.", + "../../../.", + "../../../etc/passwd", + "../../../../tmp/foo", + "for/../../bar/", + "a/a/a/../../../../../etc/passwd", + "/a/../../b/../../c/../../../etc/passwd", + } { + _, err := putObjects(s3client, []string{obj}, bucket) + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrBadRequest)); err != nil { + return err + } + } + + return nil + }) +} + +func PutObject_false_negative_object_names(s *S3Conf) error { + testName := "PutObject_false_negative_object_names" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + objs := []string{ + "%252e%252e%252fetc/passwd", // double encoding + "%2e%2e/%2e%2e/%2e%2e/.ssh/id_rsa", // double URL-encoded + "%u002e%u002e/%u002e%u002e/etc/passwd", // unicode escape + "..%2f..%2f..%2fsecret/file.txt", // URL-encoded + "..%c0%af..%c0%afetc/passwd", // UTF-8 overlong trick + ".../.../.../target.txt", + "..\\u2215..\\u2215etc/passwd", // Unicode division slash + "dir/%20../file.txt", // encoded space + "dir/%c0%ae%c0%ae/%c0%ae%c0%ae/etc/passwd", // overlong UTF-8 encoding + "logs/latest -> /etc/passwd", // symlink attacks + //TODO: add this test case in advanced routing + // "/etc/passwd" // absolute path injection + } + _, err := putObjects(s3client, objs, bucket) + if err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.ListObjects(ctx, &s3.ListObjectsInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if len(res.Contents) != len(objs) { + return fmt.Errorf("expected %v objects, instead got %v", len(objs), len(res.Contents)) + } + + for i, obj := range res.Contents { + if *obj.Key != objs[i] { + return fmt.Errorf("expected the %vth object name to be %s, instead got %s", i+1, objs[i], *obj.Key) + } + } + + return nil + }) +} + func HeadObject_non_existing_object(s *S3Conf) error { testName := "HeadObject_non_existing_object" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -9995,7 +10070,7 @@ func AbortMultipartUpload_non_existing_bucket(s *S3Conf) error { return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) _, err := s3client.AbortMultipartUpload(ctx, &s3.AbortMultipartUploadInput{ - Bucket: getPtr("incorrectBucket"), + Bucket: getPtr("incorrect-bucket"), Key: getPtr("my-obj"), UploadId: getPtr("uploadId"), }) @@ -12328,7 +12403,7 @@ func PutBucketPolicy_non_existing_bucket(s *S3Conf) error { ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) doc := genPolicyDoc("Allow", `"*"`, `"s3:*"`, fmt.Sprintf(`"arn:aws:s3:::%v"`, bucket)) _, err := s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{ - Bucket: getPtr("non_existing_bucket"), + Bucket: getPtr("non-existing-bucket"), Policy: &doc, }) cancel() @@ -13003,7 +13078,7 @@ func GetBucketPolicy_non_existing_bucket(s *S3Conf) error { return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) _, err := s3client.GetBucketPolicy(ctx, &s3.GetBucketPolicyInput{ - Bucket: getPtr("non_existing_bucket"), + Bucket: getPtr("non-existing-bucket"), }) cancel() @@ -13069,7 +13144,7 @@ func DeleteBucketPolicy_non_existing_bucket(s *S3Conf) error { return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) _, err := s3client.DeleteBucketPolicy(ctx, &s3.DeleteBucketPolicyInput{ - Bucket: getPtr("non_existing_bucket"), + Bucket: getPtr("non-existing-bucket"), }) cancel()