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()