From c6bfc14474638d707ccb88e7f888688ef41ea28c Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Mon, 7 Sep 2026 07:17:16 +0900 Subject: [PATCH 1/4] test: add reproducer for cross-tenant S3 key escape A key like ../victim/secret.txt leaves its own bucket but stays under baseDir, so the containment check against baseDir alone lets it through. RED: the attacker overwrites and deletes another tenant's objects. --- internal/services/s3/store_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/internal/services/s3/store_test.go b/internal/services/s3/store_test.go index 72f7f5b6..ef09614e 100644 --- a/internal/services/s3/store_test.go +++ b/internal/services/s3/store_test.go @@ -63,3 +63,28 @@ func TestFileStore_PathTraversal(t *testing.T) { err = store.PutObject("000000000000", "bucket", "a/b/../../c/file.txt", []byte("ok")) require.NoError(t, err, "relative path within bounds should be allowed") } + +// A key that climbs out of its own bucket but stays under baseDir passes a +// containment check made against baseDir alone, so it needs its own guard. +func TestFileStore_KeyCannotEscapeItsBucket(t *testing.T) { + store := NewFileStore(t.TempDir()) + require.NoError(t, store.PutObject("000000000000", "victim", "secret.txt", []byte("secret"))) + + err := store.PutObject("000000000000", "attacker", "../victim/secret.txt", []byte("owned")) + assert.Error(t, err, "key escaping its own bucket should be rejected") + + data, err := store.GetObject("000000000000", "victim", "secret.txt") + require.NoError(t, err) + assert.Equal(t, []byte("secret"), data, "victim object should be untouched") +} + +func TestFileStore_KeyCannotEscapeItsAccount(t *testing.T) { + store := NewFileStore(t.TempDir()) + require.NoError(t, store.PutObject("000000000000", "victim", "secret.txt", []byte("secret"))) + + err := store.DeleteObject("111111111111", "attacker", "../../000000000000/victim/secret.txt") + assert.Error(t, err, "key escaping its own account should be rejected") + + _, err = store.GetObject("000000000000", "victim", "secret.txt") + require.NoError(t, err, "victim object should still exist") +} From 2a40ab9a9929680e2f0ccc6301ebe99e28b21458 Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Mon, 7 Sep 2026 07:21:10 +0900 Subject: [PATCH 2/4] fix: reject object keys and path components that escape their own tenant An S3 key like ../victim/secret.txt left its own bucket while staying under baseDir, so the containment check against baseDir alone allowed cross-bucket and cross-account read, overwrite and delete. Guard every user-controlled path component with filepath.IsLocal, which is also the barrier CodeQL recognises for go/path-injection. --- internal/services/lambda/store.go | 29 +++++++++++------------------ internal/services/s3/store.go | 19 ++++++++++--------- internal/shared/validate.go | 6 ++++-- 3 files changed, 25 insertions(+), 29 deletions(-) diff --git a/internal/services/lambda/store.go b/internal/services/lambda/store.go index 37205c0c..694e23cb 100644 --- a/internal/services/lambda/store.go +++ b/internal/services/lambda/store.go @@ -149,8 +149,9 @@ func (s *LambdaStore) Close() error { // separators or absolute markers. It intentionally does not reject ".." as a // substring (e.g. "user..1") since the containment check below handles traversal. func isSafePathComponent(v string) bool { - return v != "" && v != "." && v != ".." && !filepath.IsAbs(v) && - !strings.ContainsAny(v, "/\\") + // IsLocal covers "", "..", "../x" and absolute paths, and is the guard + // CodeQL recognises as a path-injection barrier. + return filepath.IsLocal(v) && v != "." && !strings.ContainsAny(v, "/\\") } // codePath returns the filesystem path for a function's code zip. @@ -331,23 +332,15 @@ func (s *LambdaStore) DeleteFunction(accountID, functionName string) error { return ErrFunctionNotFound } - // Remove the code directory (best-effort; ignore errors), but only if the - // resolved path stays within the configured base code directory. - codeDirAbs := filepath.Join(s.codeDir, accountID, functionName) - cleaned := filepath.Clean(codeDirAbs) - baseDirAbs, err := filepath.Abs(s.codeDir) - if err == nil { - absCleaned, err := filepath.Abs(cleaned) - if err == nil { - rel, err := filepath.Rel(baseDirAbs, absCleaned) - if err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) && !filepath.IsAbs(rel) { - _ = os.RemoveAll(absCleaned) - } else { - slog.Debug("skipped code directory removal: path outside base directory", - "path", absCleaned, "base", baseDirAbs) - } - } + // Remove the code directory (best-effort; ignore errors). Going through + // codePath applies the same component and containment checks CreateFunction + // used to write it, so the removal cannot reach another account's directory. + codeZip, err := s.codePath(accountID, functionName) + if err != nil { + slog.Debug("skipped code directory removal", "error", err) + return nil } + _ = os.RemoveAll(filepath.Dir(codeZip)) return nil } diff --git a/internal/services/s3/store.go b/internal/services/s3/store.go index f4a6e76b..9fa9ed45 100644 --- a/internal/services/s3/store.go +++ b/internal/services/s3/store.go @@ -30,7 +30,9 @@ func NewFileStore(baseDir string) *FileStore { // validPathComponent checks that a single path segment contains no path // separators, dot-only components, or empty values. func validPathComponent(part string) error { - if part == "" || part == "." || part == ".." { + // IsLocal covers "", "..", "../x" and absolute paths, and is the guard + // CodeQL recognises as a path-injection barrier. + if !filepath.IsLocal(part) || part == "." { return fmt.Errorf("invalid path component: %q", part) } if strings.ContainsAny(part, "/\\") || @@ -49,14 +51,10 @@ func (fs *FileStore) safePath(parts ...string) (string, error) { if part == "" { continue } - if filepath.IsAbs(part) { - return "", fmt.Errorf("absolute path segment not allowed: %s", part) - } - cleanPart := filepath.Clean(part) - if cleanPart == ".." || strings.HasPrefix(cleanPart, ".."+string(filepath.Separator)) { + if !filepath.IsLocal(part) { return "", fmt.Errorf("path traversal detected in segment: %s", part) } - cleanParts = append(cleanParts, cleanPart) + cleanParts = append(cleanParts, filepath.Clean(part)) } candidate := filepath.Join(append([]string{fs.baseDir}, cleanParts...)...) @@ -79,7 +77,7 @@ func (fs *FileStore) safePath(parts ...string) (string, error) { // objectPath returns the absolute filesystem path for the given object. // Unlike safePath, the key may contain '/' (e.g. "photos/a.jpg") which is -// valid for S3 object keys. Containment under baseDir is still enforced. +// valid for S3 object keys. Containment under the bucket is still enforced. func (fs *FileStore) objectPath(accountID, bucket, key string) (string, error) { if err := validPathComponent(accountID); err != nil { return "", err @@ -87,7 +85,10 @@ func (fs *FileStore) objectPath(accountID, bucket, key string) (string, error) { if err := validPathComponent(bucket); err != nil { return "", err } - if key == "" { + // The key gets IsLocal rather than validPathComponent because '/' is legal + // in it. Checking the joined path against baseDir alone is not enough: a key + // like "../victim/secret" leaves its own bucket while staying under baseDir. + if !filepath.IsLocal(key) { return "", fmt.Errorf("invalid path component: %q", key) } diff --git a/internal/shared/validate.go b/internal/shared/validate.go index ad91ba87..9f1e637c 100644 --- a/internal/shared/validate.go +++ b/internal/shared/validate.go @@ -4,7 +4,6 @@ package shared import ( "path/filepath" - "strings" ) // ValidateUploadID checks that id is a 32-character lowercase hex string, @@ -40,5 +39,8 @@ func IsWithinDir(child, parent string) bool { if err != nil { return false } - return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) && !filepath.IsAbs(rel) + // IsLocal rejects "..", "../x" and absolute results, and accepts "." for + // child == parent. It is also the containment check CodeQL recognises as a + // path-injection barrier, which filepath.Rel comparisons are not. + return filepath.IsLocal(rel) } From 673f7f8a6d5670ae3da525c9b3c01e66f9087023 Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Mon, 7 Sep 2026 07:22:56 +0900 Subject: [PATCH 3/4] test: cover the path guards in lambda codePath and s3 bucketDir --- internal/services/lambda/store_test.go | 47 ++++++++++++++++++++++++++ internal/services/s3/store_test.go | 9 +++++ 2 files changed, 56 insertions(+) diff --git a/internal/services/lambda/store_test.go b/internal/services/lambda/store_test.go index 84092f52..7c926b8c 100644 --- a/internal/services/lambda/store_test.go +++ b/internal/services/lambda/store_test.go @@ -118,6 +118,53 @@ func TestLambdaStore_DeleteFunction(t *testing.T) { assert.ErrorIs(t, err, ErrFunctionNotFound) } +func TestLambdaStore_CodePathRejectsTraversal(t *testing.T) { + store := newTestLambdaStore(t) + + tests := []struct { + name string + accountID string + functionName string + }{ + {"traversing account", "../..", "fn"}, + {"traversing function", "123456789012", "../other-account"}, + {"separator in function", "123456789012", "a/b"}, + {"absolute function", "123456789012", "/etc/passwd"}, + {"empty account", "", "fn"}, + {"dot function", "123456789012", "."}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := store.codePath(tt.accountID, tt.functionName) + assert.Error(t, err) + }) + } +} + +// DeleteFunction removes a directory, so it must refuse names codePath rejects +// rather than falling back to its own containment check. +func TestLambdaStore_DeleteFunctionRejectsTraversal(t *testing.T) { + store := newTestLambdaStore(t) + + victim := &FunctionInfo{ + FunctionName: "victim", + FunctionArn: "arn:aws:lambda:us-east-1:123456789012:function:victim", + Runtime: "python3.12", + Handler: "index.handler", + AccountID: "123456789012", + } + _, err := store.CreateFunction(victim, []byte("zip")) + require.NoError(t, err) + + err = store.DeleteFunction("999999999999", "../123456789012/victim") + assert.ErrorIs(t, err, ErrFunctionNotFound) + + code, err := store.GetFunctionCode("123456789012", "victim") + require.NoError(t, err, "victim code should survive") + assert.Equal(t, []byte("zip"), code) +} + func TestLambdaStore_UpdateFunctionCode(t *testing.T) { store := newTestLambdaStore(t) diff --git a/internal/services/s3/store_test.go b/internal/services/s3/store_test.go index ef09614e..c91e703d 100644 --- a/internal/services/s3/store_test.go +++ b/internal/services/s3/store_test.go @@ -64,6 +64,15 @@ func TestFileStore_PathTraversal(t *testing.T) { require.NoError(t, err, "relative path within bounds should be allowed") } +func TestFileStore_BucketDirRejectsTraversal(t *testing.T) { + store := NewFileStore(t.TempDir()) + + assert.Error(t, store.CreateBucketDir("000000000000", "..")) + assert.Error(t, store.CreateBucketDir("000000000000", "../escape")) + assert.Error(t, store.CreateBucketDir("000000000000", "/abs")) + assert.Error(t, store.DeleteBucketDir("..", "bucket")) +} + // A key that climbs out of its own bucket but stays under baseDir passes a // containment check made against baseDir alone, so it needs its own guard. func TestFileStore_KeyCannotEscapeItsBucket(t *testing.T) { From 8c3b936f21345e188b26d539ab213040c88f631f Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Mon, 7 Sep 2026 07:50:18 +0900 Subject: [PATCH 4/4] chore: add Security changelog fragment for the tenant escape fix --- changes/unreleased/Security-20260907-074946.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changes/unreleased/Security-20260907-074946.yaml diff --git a/changes/unreleased/Security-20260907-074946.yaml b/changes/unreleased/Security-20260907-074946.yaml new file mode 100644 index 00000000..540131b0 --- /dev/null +++ b/changes/unreleased/Security-20260907-074946.yaml @@ -0,0 +1,5 @@ +kind: Security +body: An S3 object key can no longer leave the bucket that owns it — a key such as `../victim/secret.txt` stayed under the store base directory and so passed the old containment check, letting one tenant read, overwrite and delete another tenant's objects — because every user-controlled path component is now guarded with `filepath.IsLocal` +time: 2026-09-07T07:49:46.951306+09:00 +custom: + Issue: "155"