fix: reject object keys and path components that escape their own tenant

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"

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
 }

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)
 

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,15 +77,18 @@ 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
 	}
 	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)
 	}
 

internal/services/s3/store_test.go

@@ -63,3 +63,37 @@ 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")
 }
+
+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) {
+	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")
+}

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