[s3] validate indirect filer path inputs (#9931)

* s3: validate indirect filer path inputs

* s3: avoid query parsing on common request path

* filer: scope copy/move source against JWT AllowedPrefixes

maybeCheckJwtAuthorization only checked r.URL.Path, but copy and move read
their source from the cp.from / mv.from query params. A prefix-restricted
token could copy or move data out of a subtree it cannot otherwise reach.
Check every path the request touches, reusing pathHasComponentPrefix so
`..` in the source is collapsed before the prefix match.

* s3: confine iceberg CreateTable location to the catalog bucket

CreateTable derived the metadata bucket and path from the client-supplied
req.Location / req.Name and wrote there directly, so a caller scoped to one
table bucket could place metadata in another bucket (and path.Join collapsed
any `..`). Require the parsed bucket to equal the request's catalog bucket
and reject traversal segments in the table path.

* webdav: clean client path before subFolder confinement

wrappedFs concatenated subFolder + name before the underlying FileSystem
ran path.Clean, so `..` in the request path or COPY/MOVE Destination
resolved across the FilerRootPath confinement boundary. Clean the name as a
rooted path first so traversal segments collapse below subFolder. Only the
non-default -filer.path (non-empty subFolder) setup was affected.

* filer: enforce read-only rule on real write path with destination header

The x-seaweedfs-destination header overrides the path used for storage-rule
matching while the entry is written at r.URL.Path, letting a caller select a
writable rule for a read-only target. When the header is present, also check
the read-only/quota rule against the actual write path.
This commit is contained in:
Chris Lu authored and GitHub committed 2026-06-11 21:56:16 -07:00
1 parent 34f9b91d69
commit 0345658ea8
26 files changed
+508 -24

No files matched your search

+4 -2
View File
@@ -158,6 +158,8 @@ func TestCopySourceRejectsTraversal(t *testing.T) {
{"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},
{"versionId encoded slash rejected", "bucket-a/dir/key?versionId=v1%2F..%2Fsecret", true},
{"versionId backslash rejected", "bucket-a/dir/key?versionId=v1%5C..%5Csecret", true},
}
for _, tc := range tests {
@@ -166,9 +168,9 @@ func TestCopySourceRejectsTraversal(t *testing.T) {
if err != nil {
cpSrcPath = tc.rawCopySource
}
srcBucket, srcObject, _ := pathToBucketObjectAndVersion(tc.rawCopySource, cpSrcPath)
srcBucket, srcObject, srcVersionId := pathToBucketObjectAndVersion(tc.rawCopySource, cpSrcPath)
gotErr := ValidateCopySource(cpSrcPath, srcBucket, srcObject) != nil
gotErr := validateCopySource(cpSrcPath, srcBucket, srcObject, srcVersionId) != 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)
+12
View File
@@ -175,6 +175,18 @@ func (s *Server) handleCreateTable(w http.ResponseWriter, r *http.Request) {
return
}
// location/name are client-supplied; confine the metadata write to the
// authorized catalog bucket and reject traversal segments so path.Join in
// saveMetadataFile cannot escape into another bucket.
if metadataBucket != bucketName {
writeError(w, http.StatusBadRequest, "BadRequestException", "table location must be within bucket "+bucketName)
return
}
if !isValidTablePath(metadataPath) {
writeError(w, http.StatusBadRequest, "BadRequestException", "invalid table location path")
return
}
// Authoritative existence check: ask the catalog whether a table is registered
// at this name. If it is, short-circuit with the existing table (idempotent
// CreateTable). Any leftover objects at the target path from a previous
+15
View File
@@ -56,6 +56,21 @@ func validateRequestPath(next http.Handler) http.Handler {
})
}
// isValidTablePath reports whether a "/"-separated table path (the part below
// the bucket) is free of segments that path.Join would collapse to escape the
// bucket directory. Empty segments (from leading/duplicate slashes) are ignored.
func isValidTablePath(tablePath string) bool {
for _, segment := range strings.Split(tablePath, "/") {
if segment == "" {
continue
}
if !isValidNameSegment(segment) {
return false
}
}
return true
}
// isValidNameSegment rejects a single path-segment value (bucket prefix slot,
// table name, or one namespace part) that would be unsafe to embed in a filer
// path: `.`, `..`, embedded slash/backslash, or NUL.
@@ -8,6 +8,27 @@ import (
"github.com/gorilla/mux"
)
func TestIsValidTablePath(t *testing.T) {
tests := []struct {
tablePath string
want bool
}{
{"sales.orders", true},
{"ns/table", true},
{"ns//table", true},
{"", true},
{"../other", false},
{"ns/../../etc", false},
{"ns/./x", false},
{`ns\..\x`, false},
}
for _, tt := range tests {
if got := isValidTablePath(tt.tablePath); got != tt.want {
t.Errorf("isValidTablePath(%q) = %v, want %v", tt.tablePath, got, tt.want)
}
}
}
func TestValidateRequestPath_RejectsTraversal(t *testing.T) {
tests := []struct {
name string
+10
View File
@@ -212,6 +212,16 @@ func IsValidBucketName(bucket string) bool {
return !strings.ContainsAny(bucket, "/\\\x00")
}
// IsValidPathSegment reports whether value is safe to use as one filer path
// component. It intentionally does not enforce any application-specific
// format; callers can layer stricter checks on top.
func IsValidPathSegment(value string) bool {
if value == "" || value == "." || value == ".." {
return false
}
return !strings.ContainsAny(value, "/\\\x00")
}
// NormalizeObjectKey normalizes object keys by removing duplicate slashes and converting backslashes.
// This normalizes keys from various sources (URL path, form values, etc.) to a consistent format.
// It also converts Windows-style backslashes to forward slashes for cross-platform compatibility.
+23
View File
@@ -148,6 +148,29 @@ func TestIsValidBucketName(t *testing.T) {
}
}
func TestIsValidPathSegment(t *testing.T) {
tests := []struct {
input string
want bool
}{
{"opaque-id_123", true},
{"..hidden", true},
{"", false},
{".", false},
{"..", false},
{"dir/value", false},
{"dir\\value", false},
{"nul\x00value", false},
}
for _, tt := range tests {
t.Run(tt.input, func(t *testing.T) {
if got := IsValidPathSegment(tt.input); got != tt.want {
t.Errorf("IsValidPathSegment(%q) = %v, want %v", tt.input, got, tt.want)
}
})
}
}
func TestRemoveDuplicateSlashes(t *testing.T) {
tests := []struct {
name string
+11 -1
View File
@@ -242,8 +242,12 @@ func validateEncryptionContext(contextHeader string) error {
return nil
}
// ValidateCopySource validates the copy source path and permissions
// ValidateCopySource validates the copy source path.
func ValidateCopySource(copySource string, srcBucket, srcObject string) error {
return validateCopySource(copySource, srcBucket, srcObject, "")
}
func validateCopySource(copySource string, srcBucket, srcObject, srcVersionId string) error {
if copySource == "" {
return &CopyValidationError{
Code: s3err.ErrInvalidCopySource,
@@ -273,6 +277,12 @@ func ValidateCopySource(copySource string, srcBucket, srcObject string) error {
Message: "Copy source contains invalid path segments",
}
}
if !isValidVersionID(srcVersionId) {
return &CopyValidationError{
Code: s3err.ErrInvalidCopySource,
Message: "Copy source contains an invalid version ID",
}
}
return nil
}
@@ -0,0 +1,39 @@
package s3api
import (
"strings"
"testing"
)
func TestCheckUploadIDRequiresGeneratedFormat(t *testing.T) {
s3a := &S3ApiServer{}
const object = "dir/object"
hash := s3a.generateUploadID(object)
tests := []struct {
name string
uploadID string
wantErr bool
}{
{"legacy hash", hash, false},
{"current format", hash + "_" + strings.Repeat("a", 32), false},
{"wrong object hash", strings.Repeat("0", 40) + "_" + strings.Repeat("a", 32), true},
{"arbitrary suffix", hash + "_suffix", true},
{"uppercase suffix", hash + "_" + strings.Repeat("A", 32), true},
{"short suffix", hash + "_" + strings.Repeat("a", 31), true},
{"long suffix", hash + "_" + strings.Repeat("a", 33), true},
{"slash traversal", hash + "/../../../victim-bucket", true},
{"suffix traversal", hash + "_" + strings.Repeat("a", 32) + "/../../../victim-bucket", true},
{"backslash traversal", hash + `\..\..\victim-bucket`, true},
{"nul suffix", hash + "_abc\x00def", true},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
gotErr := s3a.checkUploadId(object, tt.uploadID) != nil
if gotErr != tt.wantErr {
t.Errorf("checkUploadId(%q) error = %v, want %v", tt.uploadID, gotErr, tt.wantErr)
}
})
}
}
+2 -2
View File
@@ -101,7 +101,7 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request
}
// Validate copy source and destination
if err := ValidateCopySource(cpSrcPath, srcBucket, srcObject); err != nil {
if err := validateCopySource(cpSrcPath, srcBucket, srcObject, srcVersionId); err != nil {
glog.V(2).Infof("CopyObjectHandler validation error: %v", err)
errCode := MapCopyValidationError(err)
s3err.WriteErrorResponse(w, r, errCode)
@@ -765,7 +765,7 @@ func (s3a *S3ApiServer) CopyObjectPartHandler(w http.ResponseWriter, r *http.Req
}
// Validate the copy source as CopyObject does.
if err := ValidateCopySource(cpSrcPath, srcBucket, srcObject); err != nil {
if err := validateCopySource(cpSrcPath, srcBucket, srcObject, srcVersionId); err != nil {
glog.V(2).Infof("CopyObjectPartHandler validation error: %v", err)
s3err.WriteErrorResponse(w, r, MapCopyValidationError(err))
return
@@ -34,6 +34,13 @@ func deleteErrorFromCode(code s3err.ErrorCode, key, versionId string) DeleteErro
}
}
func validateDeleteObjectIdentifier(object ObjectIdentifier) s3err.ErrorCode {
if !s3_constants.IsValidObjectKey(object.Key) || !isValidVersionID(object.VersionId) {
return s3err.ErrInvalidRequest
}
return s3err.ErrNone
}
// isMissingDeleteConditionTarget normalizes missing-target detection for conditional deletes.
// Prefer errors.Is(err, filer_pb.ErrNotFound) and errors.Is(err, ErrDeleteMarker); keep the
// string-based fallback only as a defensive bridge for filer paths that still return plain text.
@@ -50,6 +57,9 @@ func isMissingDeleteConditionTarget(err error) bool {
}
func (s3a *S3ApiServer) resolveDeleteConditionalEntry(bucket, object, versionId, versioningState string) (*filer_pb.Entry, error) {
if !isValidVersionID(versionId) {
return nil, errInvalidVersionID
}
normalizedObject := s3_constants.NormalizeObjectKey(object)
bucketDir := s3a.bucketDir(bucket)
@@ -173,6 +183,9 @@ func (s3a *S3ApiServer) deleteVersionedObject(r *http.Request, bucket, object, v
// when the entry's Attributes.TtlSec > 0 so the volume is guaranteed to
// drop the chunks on its own.
func (s3a *S3ApiServer) deleteUnversionedObjectWithClient(client filer_pb.SeaweedFilerClient, bucket, object string, metadataOnly bool) error {
if !s3_constants.IsValidBucketName(bucket) || !s3_constants.IsValidObjectKey(object) {
return errors.New("invalid bucket or object path")
}
target := util.NewFullPath(s3a.bucketDir(bucket), object)
dir, name := target.DirAndName()
return deleteObjectEntry(client, dir, name, !metadataOnly, false)
@@ -423,6 +436,10 @@ func (s3a *S3ApiServer) DeleteMultipleObjectsHandler(w http.ResponseWriter, r *h
if object.Key == "" {
continue
}
if validationCode := validateDeleteObjectIdentifier(object); validationCode != s3err.ErrNone {
deleteErrors = append(deleteErrors, deleteErrorFromCode(validationCode, object.Key, object.VersionId))
continue
}
if err := s3a.validateTableBucketObjectPath(bucket, object.Key); err != nil {
deleteErrors = append(deleteErrors, deleteErrorFromCode(s3err.ErrAccessDenied, object.Key, object.VersionId))
continue
@@ -1,13 +1,45 @@
package s3api
import (
"errors"
"testing"
"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
"github.com/seaweedfs/seaweedfs/weed/s3api/s3err"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestValidateDeleteObjectIdentifier(t *testing.T) {
tests := []struct {
name string
identifier ObjectIdentifier
want s3err.ErrorCode
}{
{"clean key", ObjectIdentifier{Key: "dir/key"}, s3err.ErrNone},
{"clean version", ObjectIdentifier{Key: "dir/key", VersionId: "opaque-version"}, s3err.ErrNone},
{"key traversal", ObjectIdentifier{Key: "../victim/key"}, s3err.ErrInvalidRequest},
{"encoded traversal already decoded", ObjectIdentifier{Key: "dir/../../victim/key"}, s3err.ErrInvalidRequest},
{"backslash traversal", ObjectIdentifier{Key: `..\victim\key`}, s3err.ErrInvalidRequest},
{"version traversal", ObjectIdentifier{Key: "key", VersionId: "v1/../../../victim"}, s3err.ErrInvalidRequest},
{"version backslash", ObjectIdentifier{Key: "key", VersionId: `v1\..\victim`}, s3err.ErrInvalidRequest},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.want, validateDeleteObjectIdentifier(tt.identifier))
})
}
}
func TestGetSpecificObjectVersionRejectsUnsafeVersionID(t *testing.T) {
s3a := &S3ApiServer{option: &S3ApiServerOption{BucketsPath: "/buckets"}}
_, err := s3a.getSpecificObjectVersion("bucket", "key", "v1/../../../victim")
require.Error(t, err)
assert.True(t, errors.Is(err, errInvalidVersionID))
}
func TestDeleteUnversionedObjectWithClient_MetadataOnlySkipsChunkDelete(t *testing.T) {
// metadataOnly=true must reach the filer as IsDeleteData=false so the
// volume server reclaims chunks via TTL instead of the filer enqueueing
@@ -50,6 +82,16 @@ func TestDeleteUnversionedObjectWithClient_FullPathFromBucketsRoot(t *testing.T)
assert.Equal(t, "c.txt", client.deleteReq.Name)
}
func TestDeleteUnversionedObjectWithClientRejectsTraversal(t *testing.T) {
s3a := &S3ApiServer{option: &S3ApiServerOption{BucketsPath: "/buckets"}}
client := &deleteObjectEntryTestClient{}
err := s3a.deleteUnversionedObjectWithClient(client, "source-bucket", "../victim-bucket/secret", false)
require.Error(t, err)
assert.Nil(t, client.deleteReq, "invalid path must be rejected before a filer delete RPC")
}
func TestDeleteUnversionedObjectWithClient_PropagatesEntryAttributesIrrelevant(t *testing.T) {
// The metadataOnly decision is the caller's responsibility (the
// lifecycle handler). This function is dumb plumbing — it must not
+20 -1
View File
@@ -521,13 +521,32 @@ func (s3a *S3ApiServer) checkUploadId(object string, id string) error {
hash := s3a.generateUploadID(object)
if !strings.HasPrefix(id, hash) {
// uploadID becomes a filer directory name. Accept the historical hash-only
// form and the exact current hash_UUID form, not arbitrary hash-prefixed paths.
valid := id == hash // legacy upload IDs generated before the UUID suffix was added
if len(id) == len(hash)+1+32 && strings.HasPrefix(id, hash+"_") {
valid = isLowerHex(id[len(hash)+1:])
}
if !valid {
glog.Errorf("object %s and uploadID %s are not matched", object, id)
return fmt.Errorf("object %s and uploadID %s are not matched", object, id)
}
return nil
}
func isLowerHex(value string) bool {
if value == "" {
return false
}
for i := 0; i < len(value); i++ {
c := value[i]
if (c < '0' || c > '9') && (c < 'a' || c > 'f') {
return false
}
}
return true
}
// Parse bucket url queries for ?uploads
func getBucketMultipartResources(values url.Values) (prefix, keyMarker, uploadIDMarker, delimiter string, maxUploads int, encodingType string) {
prefix = values.Get("prefix")
@@ -56,7 +56,12 @@ func (s3a *S3ApiServer) PostPolicyBucketHandler(w http.ResponseWriter, r *http.R
if fileName != "" && strings.Contains(formValues.Get("Key"), "${filename}") {
formValues.Set("Key", strings.Replace(formValues.Get("Key"), "${filename}", fileName, -1))
}
object := s3_constants.NormalizeObjectKey(formValues.Get("Key"))
rawObject := formValues.Get("Key")
if rawObject == "" || !s3_constants.IsValidObjectKey(rawObject) {
s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest)
return
}
object := s3_constants.NormalizeObjectKey(rawObject)
if err := s3a.validateTableBucketObjectPath(bucket, object); err != nil {
s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied)
return
@@ -555,6 +555,32 @@ func TestPostPolicyBucketHandlerKeyExtraction(t *testing.T) {
}
}
func TestPostPolicyBucketHandlerRejectsTraversalKey(t *testing.T) {
var buf bytes.Buffer
writer := multipart.NewWriter(&buf)
assert.NoError(t, writer.WriteField("key", "uploads/../../victim-bucket/secret"))
assert.NoError(t, writer.WriteField("Policy", ""))
part, err := writer.CreateFormFile("file", "payload.txt")
assert.NoError(t, err)
_, err = part.Write([]byte("secret"))
assert.NoError(t, err)
assert.NoError(t, writer.Close())
req := httptest.NewRequest(http.MethodPost, "/source-bucket", &buf)
req.Header.Set("Content-Type", writer.FormDataContentType())
req = mux.SetURLVars(req, map[string]string{"bucket": "source-bucket"})
rec := httptest.NewRecorder()
s3a := &S3ApiServer{
option: &S3ApiServerOption{BucketsPath: "/buckets"},
iam: &IdentityAccessManagement{},
}
s3a.PostPolicyBucketHandler(rec, req)
assert.Equal(t, http.StatusBadRequest, rec.Code)
assert.Contains(t, rec.Body.String(), "InvalidRequest")
}
// TestPostPolicyBucketHandler_PolicyViolationReturns403 drives the handler
// end-to-end with a signed multipart POST whose policy conditions cannot be
// satisfied by the form fields. It verifies the handler responds 403
+3
View File
@@ -371,6 +371,9 @@ type putFinalize struct {
// targets and a part write would otherwise bind a TTL clock starting
// before CompleteMultipartUpload.
func (s3a *S3ApiServer) putToFiler(r *http.Request, filePath string, dataReader io.Reader, bucket string, object string, partNumber int, lifecycleTTLSec int32, finalize *putFinalize, uniqueWritePath bool) (etag string, code s3err.ErrorCode, sseMetadata SSEResponseMetadata) {
if !s3_constants.IsValidBucketName(bucket) || (object != "" && !s3_constants.IsValidObjectKey(object)) {
return "", s3err.ErrInvalidRequest, SSEResponseMetadata{}
}
// NEW OPTIMIZATION: Write directly to volume servers, bypassing filer proxy
// This eliminates the filer proxy overhead for PUT operations
// Note: filePath is now passed directly instead of URL (no parsing needed)
@@ -112,6 +112,9 @@ func wormDeleteCondition(worm, bypass bool) *filer_pb.WriteCondition {
// object (serializing the pointer recompute); for object-lock buckets the
// condition gates the delete on the version's WORM guards evaluated on the owner.
func (s3a *S3ApiServer) routedDeleteSpecificVersion(owner pb.ServerAddress, bucket, object, versionId string, worm, bypass bool) s3err.ErrorCode {
if !isValidVersionID(versionId) {
return s3err.ErrInvalidRequest
}
versionFileName := s3a.getVersionFileName(versionId)
versionsPath := s3a.toFilerPath(bucket, object+s3_constants.VersionsFolder)
cond := wormDeleteCondition(worm, bypass)
+6
View File
@@ -1033,6 +1033,9 @@ func (s3a *S3ApiServer) calculateETagFromChunks(chunks []*filer_pb.FileChunk) st
// getSpecificObjectVersion retrieves a specific version of an object
func (s3a *S3ApiServer) getSpecificObjectVersion(bucket, object, versionId string) (*filer_pb.Entry, error) {
if !isValidVersionID(versionId) {
return nil, errInvalidVersionID
}
// Normalize object path to ensure consistency with toFilerPath behavior
normalizedObject := s3_constants.NormalizeObjectKey(object)
@@ -1068,6 +1071,9 @@ func (s3a *S3ApiServer) getSpecificObjectVersion(bucket, object, versionId strin
// pass true when the live entry's Attributes.TtlSec > 0 so the volume
// reclaims chunks on its own.
func (s3a *S3ApiServer) deleteSpecificObjectVersion(ctx context.Context, bucket, object, versionId string, metadataOnly bool) error {
if !isValidVersionID(versionId) {
return errInvalidVersionID
}
// Normalize object path to ensure consistency with toFilerPath behavior
normalizedObject := s3_constants.NormalizeObjectKey(object)
+49
View File
@@ -2,12 +2,52 @@ package s3api
import (
"net/http"
"net/url"
"strings"
"github.com/gorilla/mux"
"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
"github.com/seaweedfs/seaweedfs/weed/s3api/s3err"
)
func hasPathSegmentQuery(rawQuery string) bool {
if strings.Contains(rawQuery, "versionId") || strings.Contains(rawQuery, "uploadId") {
return true
}
if !strings.Contains(rawQuery, "%") {
return false
}
for rawQuery != "" {
field := rawQuery
if i := strings.IndexByte(rawQuery, '&'); i >= 0 {
field, rawQuery = rawQuery[:i], rawQuery[i+1:]
} else {
rawQuery = ""
}
if i := strings.IndexByte(field, '='); i >= 0 {
field = field[:i]
}
if !strings.Contains(field, "%") {
continue
}
key, err := url.QueryUnescape(field)
if err == nil && (key == "versionId" || key == "uploadId") {
return true
}
}
return false
}
func hasInvalidPathSegment(values []string) bool {
for _, value := range values {
if value != "" && !s3_constants.IsValidPathSegment(value) {
return true
}
}
return false
}
// validateRequestPath rejects requests whose captured {bucket}/{object} mux
// vars would normalize to a parent-directory traversal once joined into a
// filer path. The router runs with mux.NewRouter().SkipClean(true), so
@@ -33,6 +73,15 @@ func validateRequestPath(next http.Handler) http.Handler {
return
}
}
// versionId and uploadId are later used as filer entry names. Avoid
// parsing every request's query while still recognizing encoded names.
if hasPathSegmentQuery(r.URL.RawQuery) {
query := r.URL.Query()
if hasInvalidPathSegment(query["versionId"]) || hasInvalidPathSegment(query["uploadId"]) {
s3err.WriteErrorResponse(w, r, s3err.ErrInvalidRequest)
return
}
}
next.ServeHTTP(w, r)
})
}
+54
View File
@@ -85,3 +85,57 @@ func TestValidateRequestPath_RejectsEmptyCapturedVars(t *testing.T) {
})
}
}
func TestValidateRequestPath_RejectsUnsafePathQueryValues(t *testing.T) {
tests := []struct {
name string
rawQuery string
wantCode int
}{
{"clean version ID", "versionId=opaque-version", http.StatusOK},
{"clean upload ID", "uploadId=opaque_upload", http.StatusOK},
{"empty values", "versionId=&uploadId=", http.StatusOK},
{"version ID encoded slash", "versionId=v1%2F..%2Fsecret", http.StatusBadRequest},
{"version ID backslash", "versionId=v1%5C..%5Csecret", http.StatusBadRequest},
{"upload ID traversal", "uploadId=hash%2F..%2F..%2Fvictim", http.StatusBadRequest},
{"unsafe repeated value", "versionId=clean&versionId=bad%2Fvalue", http.StatusBadRequest},
{"encoded version ID name", "version%49d=v1%2F..%2Fsecret", http.StatusBadRequest},
{"encoded upload ID name", "upload%49d=hash%2F..%2Fvictim", http.StatusBadRequest},
{"unrelated query", "prefix=folder&delimiter=%2F", http.StatusOK},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
called := false
h := validateRequestPath(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
called = true
w.WriteHeader(http.StatusOK)
}))
req := mux.SetURLVars(
httptest.NewRequest(http.MethodGet, "/bucket-a/key?"+tt.rawQuery, nil),
map[string]string{"bucket": "bucket-a", "object": "key"},
)
rr := httptest.NewRecorder()
h.ServeHTTP(rr, req)
if rr.Code != tt.wantCode {
t.Fatalf("query %q: got status %d, want %d", tt.rawQuery, rr.Code, tt.wantCode)
}
if tt.wantCode != http.StatusOK && called {
t.Fatalf("query %q: inner handler reached despite rejection", tt.rawQuery)
}
})
}
}
func TestHasPathSegmentQuery_CommonPathDoesNotAllocate(t *testing.T) {
allocations := testing.AllocsPerRun(1000, func() {
if hasPathSegmentQuery("prefix=folder&delimiter=%2F") {
t.Fatal("unrelated query recognized as a path segment query")
}
})
if allocations != 0 {
t.Fatalf("common query path allocated %v times per run, want 0", allocations)
}
}
+8
View File
@@ -3,6 +3,7 @@ package s3api
import (
"crypto/rand"
"encoding/hex"
"errors"
"fmt"
"math"
"strconv"
@@ -24,6 +25,13 @@ const (
versionIdFormatThreshold = 0x4000000000000000
)
var errInvalidVersionID = errors.New("invalid version ID")
// Version IDs are opaque to clients but become filenames under .versions.
func isValidVersionID(versionId string) bool {
return versionId == "" || s3_constants.IsValidPathSegment(versionId)
}
// generateVersionId creates a unique version ID
// If useInvertedFormat is true, uses inverted timestamps so newer versions sort first
// If false, uses raw timestamps (old format) for backward compatibility
+21
View File
@@ -63,6 +63,27 @@ func TestVersionIdFormatDetection(t *testing.T) {
}
}
func TestIsValidVersionID(t *testing.T) {
tests := []struct {
versionID string
want bool
}{
{"", true},
{"null", true},
{"opaque-version_123", true},
{"v1/../../../victim", false},
{`v1\..\victim`, false},
{"bad\x00version", false},
}
for _, tt := range tests {
t.Run(tt.versionID, func(t *testing.T) {
if got := isValidVersionID(tt.versionID); got != tt.want {
t.Errorf("isValidVersionID(%q) = %v, want %v", tt.versionID, got, tt.want)
}
})
}
}
// TestGenerateVersionIdFormats tests that generateVersionId produces correct format based on parameter
func TestGenerateVersionIdFormats(t *testing.T) {
// Generate old format version ID
+32
View File
@@ -185,6 +185,38 @@ func TestFilerServer_maybeCheckJwtAuthorization_Scoped(t *testing.T) {
isWrite: false,
expectAuthorized: false,
},
{
name: "copy source within subtree allowed",
token: genToken([]string{"/tenant1"}, nil),
method: "POST",
path: "/tenant1/loot?cp.from=/tenant1/src",
isWrite: true,
expectAuthorized: true,
},
{
name: "copy source outside subtree denied",
token: genToken([]string{"/tenant1"}, nil),
method: "POST",
path: "/tenant1/loot?cp.from=/tenant2/secret",
isWrite: true,
expectAuthorized: false,
},
{
name: "copy source dot-dot escape denied",
token: genToken([]string{"/tenant1"}, nil),
method: "POST",
path: "/tenant1/loot?cp.from=/tenant1/../tenant2/secret",
isWrite: true,
expectAuthorized: false,
},
{
name: "move source outside subtree denied",
token: genToken([]string{"/tenant1"}, nil),
method: "POST",
path: "/tenant1/dst?mv.from=/tenant2/secret",
isWrite: true,
expectAuthorized: false,
},
}
for _, tt := range tests {
+33 -9
View File
@@ -253,17 +253,15 @@ func (fs *FilerServer) maybeCheckJwtAuthorization(r *http.Request, isWrite bool)
}
if len(claims.AllowedPrefixes) > 0 {
hasPrefix := false
for _, prefix := range claims.AllowedPrefixes {
if pathHasComponentPrefix(r.URL.Path, prefix) {
hasPrefix = true
break
// Copy and move name their source via a query parameter, not r.URL.Path.
// Scope every path the request reads or relocates, or a prefix-restricted
// token could reach data outside its allowed subtree.
for _, p := range jwtScopedRequestPaths(r) {
if !anyComponentPrefixMatches(claims.AllowedPrefixes, p) {
glog.V(1).Infof("jwt path not allowed from %s: %v", r.RemoteAddr, p)
return false
}
}
if !hasPrefix {
glog.V(1).Infof("jwt path not allowed from %s: %v", r.RemoteAddr, r.URL.Path)
return false
}
}
if len(claims.AllowedMethods) > 0 {
hasMethod := false
@@ -282,6 +280,32 @@ func (fs *FilerServer) maybeCheckJwtAuthorization(r *http.Request, isWrite bool)
return true
}
// jwtScopedRequestPaths returns every filer path a request touches that must be
// covered by the JWT AllowedPrefixes: the write target (r.URL.Path) plus any
// copy/move source named by the cp.from / mv.from query parameters.
func jwtScopedRequestPaths(r *http.Request) []string {
paths := []string{r.URL.Path}
if query := r.URL.Query(); query.Has("cp.from") || query.Has("mv.from") {
if from := query.Get("cp.from"); from != "" {
paths = append(paths, from)
}
if from := query.Get("mv.from"); from != "" {
paths = append(paths, from)
}
}
return paths
}
// anyComponentPrefixMatches reports whether p is within any of the prefixes.
func anyComponentPrefixMatches(prefixes []string, p string) bool {
for _, prefix := range prefixes {
if pathHasComponentPrefix(p, prefix) {
return true
}
}
return false
}
// pathHasComponentPrefix reports whether reqPath is contained within the
// directory subtree denoted by prefix, treating both as "/"-separated
// path components. Both inputs are normalised with path.Clean to neutralise
+12 -2
View File
@@ -81,8 +81,18 @@ func (fs *FilerServer) PostHandler(w http.ResponseWriter, r *http.Request, conte
ctx := r.Context()
destination := r.RequestURI
if finalDestination := r.Header.Get(s3_constants.SeaweedStorageDestinationHeader); finalDestination != "" {
destination = finalDestination
headerDestination := r.Header.Get(s3_constants.SeaweedStorageDestinationHeader)
if headerDestination != "" {
destination = headerDestination
}
// The destination header picks storage rules for a logical destination, but
// the entry is written at r.URL.Path. Enforce the read-only/quota rule on the
// actual write path too, so the header cannot route a write into a read-only
// location.
if headerDestination != "" && fs.filer.FilerConf.MatchStorageRule(r.URL.Path).ReadOnly {
writeJsonError(w, r, http.StatusInsufficientStorage, ErrReadOnly)
return
}
query := r.URL.Query()
+15 -6
View File
@@ -4,6 +4,7 @@ import (
"context"
"io/fs"
"os"
"path"
"strings"
"golang.org/x/net/webdav"
@@ -14,6 +15,14 @@ type wrappedFs struct {
webdav.FileSystem
}
// confine joins a client-supplied name under subFolder. The name is cleaned as
// a rooted path first so `..` segments cannot climb above subFolder; cleaning
// after the concat (as the underlying FileSystem does) would resolve them across
// the confinement boundary.
func (w wrappedFs) confine(name string) string {
return w.subFolder + path.Clean("/"+name)
}
// NewWrappedFs returns a webdav.FileSystem identical to fs, except it
// provides access to a sub-folder of fs that is denominated by subFolder.
// It transparently handles renaming paths and filenames so that the outer part of the wrapped filesystem
@@ -26,12 +35,12 @@ func NewWrappedFs(fs webdav.FileSystem, subFolder string) webdav.FileSystem {
}
func (w wrappedFs) Mkdir(ctx context.Context, name string, perm os.FileMode) error {
name = w.subFolder + name
name = w.confine(name)
return w.FileSystem.Mkdir(ctx, name, perm)
}
func (w wrappedFs) OpenFile(ctx context.Context, name string, flag int, perm os.FileMode) (webdav.File, error) {
name = w.subFolder + name
name = w.confine(name)
file, err := w.FileSystem.OpenFile(ctx, name, flag, perm)
file = wrappedFile{
File: file,
@@ -42,18 +51,18 @@ func (w wrappedFs) OpenFile(ctx context.Context, name string, flag int, perm os.
}
func (w wrappedFs) RemoveAll(ctx context.Context, name string) error {
name = w.subFolder + name
name = w.confine(name)
return w.FileSystem.RemoveAll(ctx, name)
}
func (w wrappedFs) Rename(ctx context.Context, oldName, newName string) error {
oldName = w.subFolder + oldName
newName = w.subFolder + newName
oldName = w.confine(oldName)
newName = w.confine(newName)
return w.FileSystem.Rename(ctx, oldName, newName)
}
func (w wrappedFs) Stat(ctx context.Context, name string) (os.FileInfo, error) {
name = w.subFolder + name
name = w.confine(name)
info, err := w.FileSystem.Stat(ctx, name)
info = wrappedFileInfo{
subFolder: &w.subFolder,
+24
View File
@@ -0,0 +1,24 @@
package weed_server
import "testing"
func TestWrappedFsConfine(t *testing.T) {
w := wrappedFs{subFolder: "/confined"}
tests := []struct {
name string
want string
}{
{"/a/b", "/confined/a/b"},
{"a/b", "/confined/a/b"},
{"/", "/confined/"},
{"/../etc/passwd", "/confined/etc/passwd"},
{"/a/../../etc", "/confined/etc"},
{"/a/./b", "/confined/a/b"},
{"/a//b", "/confined/a/b"},
}
for _, tt := range tests {
if got := w.confine(tt.name); got != tt.want {
t.Errorf("confine(%q) = %q, want %q (must stay under subFolder)", tt.name, got, tt.want)
}
}
}