fix(storage): skip download of file outside of target dir (#12945)

krishnamd-jkp · web-flow · commit 6259aeec393d · 2025-10-10T09:30:33.000+05:30
This PR introduces logic in transfermanager download directories flow
where the objects are skipped if the download path is outside of target
directory. The changes directly adds the results of such objects using
addResult method bypassing the addition of such objects to workers
threads
diff --git a/storage/transfermanager/downloader.go b/storage/transfermanager/downloader.go
@@ -181,6 +181,7 @@ func (d *Downloader) DownloadDirectory(ctx context.Context, input *DownloadDirec
 
 	outs := make(chan DownloadOutput, len(objectsToQueue))
 	inputs := make([]DownloadObjectInput, 0, len(objectsToQueue))
+	illegalPathObjects := make([]DownloadObjectInput, 0, len(objectsToQueue))
 
 	for _, object := range objectsToQueue {
 		objectWithoutPrefix := object
@@ -191,8 +192,26 @@ func (d *Downloader) DownloadDirectory(ctx context.Context, input *DownloadDirec
 		objDirectory := filepath.Join(input.LocalDirectory, filepath.Dir(objectWithoutPrefix))
 		filePath := filepath.Join(input.LocalDirectory, objectWithoutPrefix)
 
+		// Prevent directory traversal attacks.
+		isUnder, err := isSubPath(input.LocalDirectory, filePath)
+		if err != nil {
+			cleanFiles(inputs)
+			return fmt.Errorf("transfermanager: DownloadDirectory failed to verify path: %w", err)
+		}
+		if !isUnder {
+			// skipped files will later be added in the results
+			illegalPathObjects = append(illegalPathObjects, DownloadObjectInput{
+				Bucket:                 input.Bucket,
+				Object:                 object,
+				Callback:               input.OnObjectDownload,
+				ctx:                    ctx,
+				directoryObjectOutputs: outs,
+				directory:              true,
+			})
+			continue
+		}
 		// Make sure all directories in the object path exist.
-		err := os.MkdirAll(objDirectory, fs.ModeDir|fs.ModePerm)
+		err = os.MkdirAll(objDirectory, fs.ModeDir|fs.ModePerm)
 		if err != nil {
 			cleanFiles(inputs)
 			return fmt.Errorf("transfermanager: DownloadDirectory failed to make directory(%q): %w", objDirectory, err)
@@ -218,9 +237,21 @@ func (d *Downloader) DownloadDirectory(ctx context.Context, input *DownloadDirec
 
 	if d.config.asynchronous {
 		d.downloadsInProgress.Add(1)
-		go d.gatherObjectOutputs(input, outs, len(inputs))
+		allObjectsCount := len(inputs) + len(illegalPathObjects)
+		go d.gatherObjectOutputs(input, outs, allObjectsCount)
 	}
 	d.addNewInputs(inputs)
+
+	for _, file := range illegalPathObjects {
+		// the waitgroup is further decremented in addResult method
+		d.downloadsInProgress.Add(1)
+		d.addResult(&file, &DownloadOutput{
+			Bucket:  file.Bucket,
+			Object:  file.Object,
+			Err:     fmt.Errorf("skipping download of object with unsafe path %q", file.Object),
+			skipped: true,
+		})
+	}
 	return nil
 }
 
@@ -311,7 +342,7 @@ func (d *Downloader) addNewInputs(inputs []DownloadObjectInput) {
 func (d *Downloader) addResult(input *DownloadObjectInput, result *DownloadOutput) {
 	copiedResult := *result // make a copy so that callbacks do not affect the result
 
-	if input.directory {
+	if input.directory && !result.skipped {
 		f := input.Destination.(*os.File)
 		if err := f.Close(); err != nil && result.Err == nil {
 			result.Err = fmt.Errorf("closing file(%q): %w", f.Name(), err)
@@ -321,10 +352,9 @@ func (d *Downloader) addResult(input *DownloadObjectInput, result *DownloadOutpu
 		if result.Err != nil {
 			os.Remove(f.Name())
 		}
-
-		if d.config.asynchronous {
-			input.directoryObjectOutputs <- copiedResult
-		}
+	}
+	if input.directory && d.config.asynchronous {
+		input.directoryObjectOutputs <- copiedResult
 	}
 
 	if d.config.asynchronous || input.directory {
@@ -810,6 +840,7 @@ type DownloadOutput struct {
 	Err    error                      // error occurring during download
 	Attrs  *storage.ReaderObjectAttrs // attributes of downloaded object, if successful
 
+	skipped     bool
 	shard       int
 	shardLength int64
 	crc32c      uint32
@@ -948,3 +979,28 @@ func checksumObject(got, want uint32) error {
 	}
 	return nil
 }
+
+func isSubPath(localDirectory, filePath string) (bool, error) {
+	// Validate if paths can be converted to absolute paths.
+	absLocalDirectory, err := filepath.Abs(localDirectory)
+	if err != nil {
+		return false, fmt.Errorf("cannot convert local directory to absolute path: %w", err)
+	}
+	absFilePath, err := filepath.Abs(filePath)
+	if err != nil {
+		return false, fmt.Errorf("cannot convert file path to absolute path: %w", err)
+	}
+
+	// The relative path from the local directory to the file path.
+	// ex: if localDirectory is /tmp/foo and filePath is /tmp/foo/bar, rel will be "bar".
+	rel, err := filepath.Rel(absLocalDirectory, absFilePath)
+	if err != nil {
+		return false, err
+	}
+
+	// rel should not start with ".." to escape target directory
+	prevDir := ".." + string(filepath.Separator)
+	isUnder := !strings.HasPrefix(rel, prevDir) && rel != ".."
+
+	return isUnder, nil
+}
diff --git a/storage/transfermanager/downloader_test.go b/storage/transfermanager/downloader_test.go
@@ -18,6 +18,8 @@ import (
 	"bytes"
 	"context"
 	"errors"
+	"os"
+	"path/filepath"
 	"strings"
 	"sync"
 	"testing"
@@ -232,6 +234,117 @@ func TestNumShards(t *testing.T) {
 	}
 }
 
+func TestIsSubPath(t *testing.T) {
+	t.Parallel()
+	sep := string(filepath.Separator)
+
+	// Create a temporary directory to work with relative paths.
+	tempDir := t.TempDir()
+
+	testCases := []struct {
+		name           string
+		localDirectory string
+		filePath       string
+		wantIsSub      bool
+		wantErrMsg     string
+	}{
+		{
+			name:           "filePath is a child",
+			localDirectory: "/tmp/foo",
+			filePath:       "/tmp/foo/bar",
+			wantIsSub:      true,
+		},
+		{
+			name:           "filePath is a nested child",
+			localDirectory: "/tmp/foo",
+			filePath:       "/tmp/foo/bar/baz",
+			wantIsSub:      true,
+		},
+		{
+			name:           "filePath is the same as localDirectory",
+			localDirectory: "/tmp/foo",
+			filePath:       "/tmp/foo",
+			wantIsSub:      true,
+		},
+		{
+			name:           "filePath is a sibling",
+			localDirectory: "/tmp/foo",
+			filePath:       "/tmp/bar",
+			wantIsSub:      false,
+		},
+		{
+			name:           "filePath is a parent",
+			localDirectory: "/tmp/foo",
+			filePath:       "/tmp",
+			wantIsSub:      false,
+		},
+		{
+			name:           "directory traversal attempt",
+			localDirectory: "/tmp/foo",
+			filePath:       "/tmp/foo/../bar", // resolves to /tmp/bar
+			wantIsSub:      false,
+		},
+		{
+			name:           "deeper directory traversal attempt",
+			localDirectory: "/tmp/foo",
+			filePath:       "/tmp/foo/bar/../../baz", // resolves to /tmp/baz
+			wantIsSub:      false,
+		},
+		{
+			name:           "relative paths - valid",
+			localDirectory: tempDir,
+			filePath:       filepath.Join(tempDir, "bar"),
+			wantIsSub:      true,
+		},
+		{
+			name:           "relative paths - traversal",
+			localDirectory: tempDir,
+			filePath:       filepath.Join(tempDir, "..", "bar"),
+			wantIsSub:      false,
+		},
+		{
+			name:           "relative path is just ..",
+			localDirectory: "foo",
+			filePath:       "foo" + sep + ".." + sep + "..",
+			wantIsSub:      false,
+		},
+		{
+			name:           "IsSubPath returns error when base dir is changed",
+			localDirectory: "foo",
+			filePath:       "bar",
+			wantErrMsg:     "no such file or directory",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			origWd, _ := os.Getwd()
+			t.Cleanup(func() {
+				os.Chdir(origWd)
+			})
+			wantErr := (tc.wantErrMsg != "")
+			// induce filepath.Abs() error
+			if wantErr {
+				dir, _ := os.MkdirTemp("", "")
+				os.Chdir(dir)
+				os.RemoveAll(dir)
+			}
+			isSub, err := isSubPath(tc.localDirectory, tc.filePath)
+
+			if (err != nil) != wantErr {
+				t.Fatalf("isSubPath() error = %v, wantErr %v", err, tc.wantErrMsg)
+			}
+			if wantErr && !strings.Contains(err.Error(), tc.wantErrMsg) {
+				t.Errorf("isSubPath() error = %s, want err containing %s", err.Error(), tc.wantErrMsg)
+				return
+			}
+			if isSub != tc.wantIsSub {
+				t.Errorf("isSubPath() = %v, want %v", isSub, tc.wantIsSub)
+			}
+		})
+	}
+}
+
 func TestCalculateRange(t *testing.T) {
 	t.Parallel()
 	for _, test := range []struct {
diff --git a/storage/transfermanager/integration_test.go b/storage/transfermanager/integration_test.go
