fix: refuse object keys that resolve outside the local destination (#872)

command/cp.go

@@ -957,7 +957,11 @@ func prepareLocalDestination(
 	}
 
 	if isBatch && !flatten {
-		dsturl = dsturl.Join(objname)
+		joined, jerr := dsturl.JoinInside(objname)
+		if jerr != nil {
+			return nil, jerr
+		}
+		dsturl = joined
 		err := client.MkdirAll(dsturl.Dir())
 		if err != nil {
 			return nil, err
@@ -970,11 +974,19 @@ func prepareLocalDestination(
 			return nil, err
 		}
 		if strings.HasSuffix(dsturl.Absolute(), "/") {
-			dsturl = dsturl.Join(objname)
+			joined, err := dsturl.JoinInside(objname)
+			if err != nil {
+				return nil, err
+			}
+			dsturl = joined
 		}
 	} else {
 		if obj.Type.IsDir() {
-			dsturl = obj.URL.Join(objname)
+			joined, err := obj.URL.JoinInside(objname)
+			if err != nil {
+				return nil, err
+			}
+			dsturl = joined
 		}
 	}
 

command/sync.go

@@ -8,6 +8,7 @@ import (
 	"path/filepath"
 	"strings"
 	"sync"
+	"sync/atomic"
 
 	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/hashicorp/go-multierror"
@@ -232,9 +233,13 @@ func (s Sync) Run(c *cli.Context) error {
 	pipeReader, pipeWriter := io.Pipe() // create a reader, writer pipe to pass commands to run
 
 	// Create commands in background.
-	go s.planRun(c, onlySource, onlyDest, commonObjects, dsturl, strategy, pipeWriter, isBatch)
+	var rejected int64 // objects planRun refused to generate a command for
+	go s.planRun(c, onlySource, onlyDest, commonObjects, dsturl, strategy, pipeWriter, isBatch, &rejected)
 
 	err = NewRun(c, pipeReader).Run(ctx)
+	if n := atomic.LoadInt64(&rejected); n > 0 {
+		err = multierror.Append(err, fmt.Errorf("%d object(s) were not synced", n))
+	}
 	return multierror.Append(err, merrorWaiter).ErrorOrNil()
 }
 
@@ -444,6 +449,7 @@ func (s Sync) planRun(
 	strategy SyncStrategy,
 	w io.WriteCloser,
 	isBatch bool,
+	rejected *int64,
 ) {
 	defer w.Close()
 
@@ -463,7 +469,12 @@ func (s Sync) planRun(
 	go func() {
 		defer wg.Done()
 		for srcurl := range onlySource {
-			curDestURL := generateDestinationURL(srcurl, dsturl, isBatch)
+			curDestURL, err := generateDestinationURL(srcurl, dsturl, isBatch)
+			if err != nil {
+				printError(s.fullCommand, s.op, err)
+				atomic.AddInt64(rejected, 1)
+				continue
+			}
 			command, err := generateCommand(c, "cp", defaultFlags, srcurl, curDestURL)
 			if err != nil {
 				printDebug(s.op, err, srcurl, curDestURL)
@@ -532,21 +543,21 @@ func (s Sync) planRun(
 
 // generateDestinationURL generates destination url for given
 // source url if it would have been in destination.
-func generateDestinationURL(srcurl, dsturl *url.URL, isBatch bool) *url.URL {
+func generateDestinationURL(srcurl, dsturl *url.URL, isBatch bool) (*url.URL, error) {
 	objname := srcurl.Base()
 	if isBatch {
 		objname = srcurl.Relative()
 	}
 
 	if dsturl.IsRemote() {
 		if dsturl.IsPrefix() || dsturl.IsBucket() {
-			return dsturl.Join(objname)
+			return dsturl.Join(objname), nil
 		}
-		return dsturl.Clone()
+		return dsturl.Clone(), nil
 
 	}
 
-	return dsturl.Join(objname)
+	return dsturl.JoinInside(objname)
 }
 
 // shouldSkipObject checks is object should be skipped.

e2e/cp_test.go

@@ -309,6 +309,52 @@ func TestCopyMultipleFlatS3ObjectsToLocal(t *testing.T) {
 	}
 }
 
+// cp s3://bucket/* dir/  (object key contains "..")
+//
+// Object keys are arbitrary strings. A key such as "data/../../escape.txt"
+// must never resolve outside the destination directory.
+func TestCopyS3ObjectsToLocalWithPathTraversalKey(t *testing.T) {
+	t.Parallel()
+
+	s3client, s5cmd := setup(t)
+
+	bucket := s3BucketFromTestName(t)
+	createBucket(t, s3client, bucket)
+
+	const (
+		safeKey      = "data/ok.txt"
+		traversalKey = "data/../../escape.txt"
+	)
+	putFile(t, s3client, bucket, safeKey, "ok")
+	putFile(t, s3client, bucket, traversalKey, "pwned")
+	assert.Assert(t, ensureS3Object(s3client, bucket, traversalKey, "pwned"))
+
+	workdir := fs.NewDir(t, "somedir", fs.WithDir("dest"))
+	defer workdir.Remove()
+
+	cmd := s5cmd("cp", "s3://"+bucket+"/*", "dest/")
+	result := icmd.RunCmd(cmd, withWorkingDir(workdir))
+
+	result.Assert(t, icmd.Expected{ExitCode: 1})
+
+	assertLines(t, result.Stdout(), map[int]compareFunc{
+		0: equals(`cp s3://%v/data/ok.txt dest/data/ok.txt`, bucket),
+	})
+	assertLines(t, result.Stderr(), map[int]compareFunc{
+		0: contains(`escapes destination`),
+	})
+
+	// nothing written outside dest/, the safe object copied normally.
+	expected := fs.Expected(t,
+		fs.WithDir("dest",
+			fs.WithDir("data",
+				fs.WithFile("ok.txt", "ok"),
+			),
+		),
+	)
+	assert.Assert(t, fs.Equal(workdir.Path(), expected))
+}
+
 // cp --flatten s3://bucket/*.txt dir/
 func TestCopyMultipleFlatS3ObjectsToLocalWithPartialMatching(t *testing.T) {
 	t.Parallel()

e2e/sync_test.go

@@ -2439,6 +2439,43 @@ func TestSyncLocalDirectoryToS3WithExcludeFilter(t *testing.T) {
 	}
 }
 
+// sync s3://bucket/* dir/  (object key contains "..")
+func TestSyncS3ObjectsToLocalWithPathTraversalKey(t *testing.T) {
+	t.Parallel()
+
+	s3client, s5cmd := setup(t)
+
+	bucket := s3BucketFromTestName(t)
+	createBucket(t, s3client, bucket)
+
+	putFile(t, s3client, bucket, "data/ok.txt", "ok")
+	putFile(t, s3client, bucket, "data/../../escape.txt", "pwned")
+
+	workdir := fs.NewDir(t, "somedir", fs.WithDir("dest"))
+	defer workdir.Remove()
+
+	cmd := s5cmd("sync", "s3://"+bucket+"/*", "dest/")
+	result := icmd.RunCmd(cmd, withWorkingDir(workdir))
+
+	result.Assert(t, icmd.Expected{ExitCode: 1})
+
+	assertLines(t, result.Stdout(), map[int]compareFunc{
+		0: equals(`cp s3://%v/data/ok.txt dest/data/ok.txt`, bucket),
+	})
+	assertLines(t, result.Stderr(), map[int]compareFunc{
+		0: contains(`escapes destination`),
+	})
+
+	expected := fs.Expected(t,
+		fs.WithDir("dest",
+			fs.WithDir("data",
+				fs.WithFile("ok.txt", "ok"),
+			),
+		),
+	)
+	assert.Assert(t, fs.Equal(workdir.Path(), expected))
+}
+
 // sync --delete somedir s3://bucket/ (removes 10k objects)
 func TestIssue435(t *testing.T) {
 	t.Parallel()

storage/url/url.go

@@ -195,6 +195,32 @@ func (u *URL) Dir() string {
 	return basefn(u.Path)
 }
 
+// JoinInside is Join for destinations that must contain the result. Object
+// keys are arbitrary strings and may hold ".." components; joined onto a
+// local directory they would resolve outside it, so an attacker who can
+// write one object to a bucket could write anywhere the downloading user
+// can. Remote URLs keep keys verbatim and are joined as is.
+func (u *URL) JoinInside(s string) (*URL, error) {
+	clone := u.Join(s)
+	if clone.IsRemote() {
+		return clone, nil
+	}
+
+	base := path.Clean(u.Path)
+	target := clone.Path // already cleaned by path.Join
+
+	var escapes bool
+	if base == "." {
+		escapes = target == ".." || strings.HasPrefix(target, "../")
+	} else {
+		escapes = target != base && !strings.HasPrefix(target, strings.TrimSuffix(base, "/")+"/")
+	}
+	if escapes {
+		return nil, fmt.Errorf("object key %q escapes destination %q", s, u.Path)
+	}
+	return clone, nil
+}
+
 // Join joins string and returns new URL.
 func (u *URL) Join(s string) *URL {
 	if runtime.GOOS == "windows" {

storage/url/url_test.go

@@ -644,3 +644,51 @@ func TestToFromBytes(t *testing.T) {
 		})
 	}
 }
+
+func TestJoinInside(t *testing.T) {
+	t.Parallel()
+
+	testcases := []struct {
+		base    string
+		join    string
+		want    string
+		escapes bool
+	}{
+		{base: "dest", join: "a/b.txt", want: "dest/a/b.txt"},
+		{base: "dest/", join: "a/b.txt", want: "dest/a/b.txt"},
+		{base: "dest", join: "a/../b.txt", want: "dest/b.txt"},
+		{base: "dest", join: "../b.txt", escapes: true},
+		{base: "dest", join: "a/../../b.txt", escapes: true},
+		{base: "dest", join: "..", escapes: true},
+		{base: "dest", join: "../dest2/b.txt", escapes: true},
+		{base: ".", join: "a/b.txt", want: "a/b.txt"},
+		{base: ".", join: "../b.txt", escapes: true},
+		{base: "/", join: "../b.txt", want: "/b.txt"},
+		{base: "s3://bucket/prefix/", join: "../b.txt", want: "prefix/../b.txt"},
+	}
+
+	for _, tc := range testcases {
+		tc := tc
+		t.Run(tc.base+"+"+tc.join, func(t *testing.T) {
+			t.Parallel()
+
+			u, err := New(tc.base)
+			if err != nil {
+				t.Fatal(err)
+			}
+			got, err := u.JoinInside(tc.join)
+			if tc.escapes {
+				if err == nil {
+					t.Fatalf("expected an error, got %q", got.Path)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatal(err)
+			}
+			if got.Path != tc.want {
+				t.Errorf("got %q, want %q", got.Path, tc.want)
+			}
+		})
+	}
+}