diff --git a/command/cp.go b/command/cp.go index 7ed06bfee..94f5818b0 100644 --- a/command/cp.go +++ b/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 } } diff --git a/command/sync.go b/command/sync.go index 6dede5a66..033e03072 100644 --- a/command/sync.go +++ b/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,7 +543,7 @@ 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() @@ -540,13 +551,13 @@ func generateDestinationURL(srcurl, dsturl *url.URL, isBatch bool) *url.URL { 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. diff --git a/e2e/cp_test.go b/e2e/cp_test.go index a13b2ceda..78d2ae7d9 100644 --- a/e2e/cp_test.go +++ b/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() diff --git a/e2e/sync_test.go b/e2e/sync_test.go index d592e6d52..6ec4475d8 100644 --- a/e2e/sync_test.go +++ b/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() diff --git a/storage/url/url.go b/storage/url/url.go index a9489b7e2..d409e802a 100644 --- a/storage/url/url.go +++ b/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" { diff --git a/storage/url/url_test.go b/storage/url/url_test.go index ffe83b704..1a64671b4 100644 --- a/storage/url/url_test.go +++ b/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) + } + }) + } +}