s3: validate copy source path segments (#9929)

Reject copy sources whose bucket/object fail IsValidBucketName /
IsValidObjectKey, the helpers validateRequestPath already applies to the
request URL. The object is joined onto the bucket path and `.`/`..`
segments are collapsed by the filer, so without this the source need not
stay within the parsed bucket. Route UploadPartCopy through
ValidateCopySource too; it previously only checked for empty bucket/object.
This commit is contained in:
Chris Lu
2026-06-11 17:07:15 -07:00
committed by GitHub
parent 4f8af455bf
commit b44cf51fe9
3 changed files with 53 additions and 6 deletions
+40
View File
@@ -137,6 +137,46 @@ func TestCopySourceDecodingPlusSign(t *testing.T) {
}
}
// TestCopySourceRejectsTraversal verifies `.`/`..` segments in a copy source
// are rejected so the source stays within the parsed bucket.
func TestCopySourceRejectsTraversal(t *testing.T) {
tests := []struct {
name string
rawCopySource string
wantErr bool
}{
{"clean source passes", "bucket-a/dir/key", false},
{"leading slash source passes", "/bucket-a/dir/key", false},
{"source with versionId passes", "bucket-a/dir/key?versionId=v1", false},
{"dotdot escapes bucket", "bucket-a/../bucket-secret/flag", true},
{"leading slash dotdot escapes bucket", "/bucket-a/../bucket-secret/flag", true},
{"percent-encoded dotdot escapes bucket", "bucket-a/%2e%2e/bucket-secret/flag", true},
{"encoded slash dotdot escapes bucket", "bucket-a/..%2fbucket-secret/flag", true},
{"backslash dotdot escapes bucket", "bucket-a/..\\bucket-secret/flag", true},
{"nested dotdot escapes bucket", "bucket-a/dir/../../bucket-secret/flag", true},
{"bare dot segment rejected", "bucket-a/./key", true},
{"dotdot bucket rejected", "../bucket-secret/flag", true},
{"dotdot with versionId escapes bucket", "bucket-a/../bucket-secret/flag?versionId=v1", true},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
cpSrcPath, err := url.PathUnescape(tc.rawCopySource)
if err != nil {
cpSrcPath = tc.rawCopySource
}
srcBucket, srcObject, _ := pathToBucketObjectAndVersion(tc.rawCopySource, cpSrcPath)
gotErr := ValidateCopySource(cpSrcPath, srcBucket, srcObject) != nil
if gotErr != tc.wantErr {
t.Errorf("ValidateCopySource(%q) error = %v, want %v (parsed bucket=%q object=%q)",
tc.rawCopySource, gotErr, tc.wantErr, srcBucket, srcObject)
}
})
}
}
// TestCopySourceRoutingWithSpecialChars tests that mux variable extraction
// correctly handles special characters like ! (%21) in both the URL path
// and the X-Amz-Copy-Source header.
+9
View File
@@ -265,6 +265,15 @@ func ValidateCopySource(copySource string, srcBucket, srcObject string) error {
}
}
// `.`/`..` segments are collapsed by the filer's path join; reject them as
// IsValidObjectKey does for the request URL so the source stays in-bucket.
if !s3_constants.IsValidBucketName(srcBucket) || !s3_constants.IsValidObjectKey(srcObject) {
return &CopyValidationError{
Code: s3err.ErrInvalidCopySource,
Message: "Copy source contains invalid path segments",
}
}
return nil
}
+4 -6
View File
@@ -764,12 +764,10 @@ func (s3a *S3ApiServer) CopyObjectPartHandler(w http.ResponseWriter, r *http.Req
}
}
// If source object is empty or bucket is empty, reply back invalid copy source.
// Note: srcObject can be "/" for root-level objects, but empty string means parsing failed
if srcObject == "" || srcBucket == "" {
glog.Errorf("CopyObjectPart: Invalid copy source - srcBucket=%q, srcObject=%q (original header: %q)",
srcBucket, srcObject, r.Header.Get("X-Amz-Copy-Source"))
s3err.WriteErrorResponse(w, r, s3err.ErrInvalidCopySource)
// Validate the copy source as CopyObject does.
if err := ValidateCopySource(cpSrcPath, srcBucket, srcObject); err != nil {
glog.V(2).Infof("CopyObjectPartHandler validation error: %v", err)
s3err.WriteErrorResponse(w, r, MapCopyValidationError(err))
return
}