From f238640d732f36a557d3090ce7576c3b93e41a6b Mon Sep 17 00:00:00 2001
From: 1seal <security@1seal.org>
Date: Wed, 22 Apr 2026 19:01:53 +0300
Subject: [PATCH] fix: confine GCS downloads to destination root

---
 pkg/skaffold/gcs/client/native.go      |  33 +++++++-
 pkg/skaffold/gcs/client/native_test.go | 102 +++++++++++++++++++++++++
 2 files changed, 134 insertions(+), 1 deletion(-)

diff --git a/pkg/skaffold/gcs/client/native.go b/pkg/skaffold/gcs/client/native.go
index f5f5418e6f2..6f6f7afa10c 100644
--- a/pkg/skaffold/gcs/client/native.go
+++ b/pkg/skaffold/gcs/client/native.go
@@ -79,7 +79,10 @@ func (n *Native) DownloadRecursive(ctx context.Context, src, dst string) error {
 	}
 
 	for uri, localPath := range files {
-		fullPath := filepath.Join(dst, localPath)
+		fullPath, err := resolveDestinationPath(dst, localPath)
+		if err != nil {
+			return err
+		}
 		dir := filepath.Dir(fullPath)
 		if _, err := os.Stat(dir); os.IsNotExist(err) {
 			if err := os.MkdirAll(dir, os.ModePerm); err != nil {
@@ -95,6 +98,34 @@ func (n *Native) DownloadRecursive(ctx context.Context, src, dst string) error {
 	return nil
 }
 
+func resolveDestinationPath(dst, localPath string) (string, error) {
+	normalizedLocalPath := filepath.Clean(filepath.FromSlash(localPath))
+	if filepath.IsAbs(normalizedLocalPath) {
+		return "", fmt.Errorf("gcs object path %q escapes destination root %q", localPath, dst)
+	}
+
+	joinedPath := filepath.Clean(filepath.Join(dst, localPath))
+	dstRoot, err := filepath.Abs(dst)
+	if err != nil {
+		return "", fmt.Errorf("failed to resolve destination root: %w", err)
+	}
+
+	fullPath, err := filepath.Abs(joinedPath)
+	if err != nil {
+		return "", fmt.Errorf("failed to resolve destination path: %w", err)
+	}
+
+	relPath, err := filepath.Rel(dstRoot, fullPath)
+	if err != nil {
+		return "", fmt.Errorf("failed to compare destination path: %w", err)
+	}
+	if relPath == ".." || strings.HasPrefix(relPath, ".."+string(filepath.Separator)) {
+		return "", fmt.Errorf("gcs object path %q escapes destination root %q", localPath, dst)
+	}
+
+	return joinedPath, nil
+}
+
 // Uploads a single file to the given dst.
 func (n *Native) UploadFile(ctx context.Context, src, dst string) error {
 	f, err := os.Open(src)
diff --git a/pkg/skaffold/gcs/client/native_test.go b/pkg/skaffold/gcs/client/native_test.go
index 25a9a20f788..85178847203 100644
--- a/pkg/skaffold/gcs/client/native_test.go
+++ b/pkg/skaffold/gcs/client/native_test.go
@@ -39,6 +39,12 @@ type repoHandlerMock struct {
 	uploadedFile    string
 }
 
+type listedObjectBucketHandler struct {
+	listedObjects    []string
+	downloadedFiles  map[string]string
+	downloadAttempts int
+}
+
 func (r *repoHandlerMock) filterOnlyFiles(paths []string) ([]string, error) {
 	matches := []string{}
 	for _, m := range paths {
@@ -100,6 +106,25 @@ func (r *repoHandlerMock) UploadObject(ctx context.Context, objName string, cont
 
 func (r *repoHandlerMock) Close() {}
 
+func (l *listedObjectBucketHandler) ListObjects(ctx context.Context, q *storage.Query) ([]string, error) {
+	return l.listedObjects, nil
+}
+
+func (l *listedObjectBucketHandler) DownloadObject(ctx context.Context, localPath, uri string) error {
+	if l.downloadedFiles == nil {
+		l.downloadedFiles = map[string]string{}
+	}
+	l.downloadAttempts++
+	l.downloadedFiles[uri] = localPath
+	return nil
+}
+
+func (l *listedObjectBucketHandler) UploadObject(ctx context.Context, objName string, content *os.File) error {
+	return nil
+}
+
+func (l *listedObjectBucketHandler) Close() {}
+
 func TestDownloadRecursive(t *testing.T) {
 	tests := []struct {
 		name                    string
@@ -286,6 +311,83 @@ func TestDownloadRecursive(t *testing.T) {
 	}
 }
 
+func TestDownloadRecursiveRejectsEscapingPaths(t *testing.T) {
+	tests := []struct {
+		name        string
+		listedFiles []string
+		dst         string
+	}{
+		{
+			name:        "rejects traversal in object path",
+			listedFiles: []string{"prefix/../../.kube/config"},
+			dst:         "download",
+		},
+		{
+			name:        "rejects absolute object path",
+			listedFiles: []string{"/tmp/owned"},
+			dst:         "download",
+		},
+	}
+
+	for _, test := range tests {
+		testutil.Run(t, test.name, func(t *testutil.T) {
+			bucket := &listedObjectBucketHandler{
+				listedObjects: test.listedFiles,
+			}
+			t.Override(&GetBucketManager, func(ctx context.Context, bucketName string) (bucketHandler, error) {
+				return bucket, nil
+			})
+
+			n := Native{}
+			err := n.DownloadRecursive(context.TODO(), "gs://bucket/prefix", test.dst)
+			t.CheckErrorContains("escapes destination root", err)
+			t.CheckDeepEqual(0, bucket.downloadAttempts)
+		})
+	}
+}
+
+func TestResolveDestinationPath(t *testing.T) {
+	tests := []struct {
+		name        string
+		dst         string
+		localPath   string
+		wantErr     bool
+		expectedEnd string
+	}{
+		{
+			name:        "preserves nested path within root",
+			dst:         "download",
+			localPath:   "dir/manifest.yaml",
+			expectedEnd: filepath.Join("download", "dir", "manifest.yaml"),
+		},
+		{
+			name:      "rejects traversal segment",
+			dst:       "download",
+			localPath: "../manifest.yaml",
+			wantErr:   true,
+		},
+		{
+			name:      "rejects absolute path",
+			dst:       "download",
+			localPath: filepath.Join(string(filepath.Separator), "tmp", "manifest.yaml"),
+			wantErr:   true,
+		},
+	}
+
+	for _, test := range tests {
+		testutil.Run(t, test.name, func(t *testutil.T) {
+			fullPath, err := resolveDestinationPath(test.dst, test.localPath)
+			if test.wantErr {
+				t.CheckErrorContains("escapes destination root", err)
+				return
+			}
+
+			t.CheckNoError(err)
+			t.CheckDeepEqual(filepath.Clean(test.expectedEnd), fullPath)
+		})
+	}
+}
+
 func TestUploadFile(t *testing.T) {
 	tests := []struct {
 		name                string