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

storage/transfermanager/downloader.go

@@ -22,6 +22,7 @@ import (
 	"hash/crc32"
 	"io"
 	"io/fs"
+	"log"
 	"math"
 	"os"
 	"path/filepath"
@@ -181,6 +182,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 +193,27 @@ 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 {
+			log.Printf("transfermanager: skipping object with unsafe path after stripping prefix %q", objectWithoutPrefix)
+			// 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 +239,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 +344,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 +354,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 +842,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 +981,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
+}

storage/transfermanager/downloader_test.go

@@ -18,6 +18,8 @@ import (
 	"bytes"
 	"context"
 	"errors"
+	"os"
+	"path/filepath"
 	"strings"
 	"sync"
 	"testing"
@@ -232,6 +234,113 @@ 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
+		wantErr        bool
+	}{
+		{
+			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",
+			wantErr:        true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			origWd, _ := os.Getwd()
+			t.Cleanup(func() {
+				os.Chdir(origWd)
+			})
+			// induce filepath.Abs() error
+			if tc.wantErr {
+				dir, _ := os.MkdirTemp("", "")
+				os.Chdir(dir)
+				os.RemoveAll(dir)
+			}
+			isSub, err := isSubPath(tc.localDirectory, tc.filePath)
+
+			if (err != nil) != tc.wantErr {
+				t.Errorf("isSubPath() error = %v, wantErr %v", err, tc.wantErr)
+				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 {