From 0cc40bce0933f86d020006f3104ec32fabe90f82 Mon Sep 17 00:00:00 2001 From: Terry Rankine Date: Fri, 18 Sep 2026 06:59:53 +0200 Subject: [PATCH] fix: refuse object keys that resolve outside the local destination S3 object keys are arbitrary strings and may contain ".." segments. cp and sync built the local destination from the raw key with path.Join, which resolves ".." and lands outside the chosen directory: downloading "s3://bucket/*" where an attacker wrote one object with key "data/../../../.bashrc" overwrites the caller's shell profile. Reported upstream as peak/s5cmd#872. Add URL.JoinInside, which joins and then checks the cleaned result is still under the destination, and use it at every local sink: batch downloads in prepareLocalDestination, the trailing-slash and directory cases in prepareDownloadDestination, and sync's generateDestinationURL. Offending objects are reported as errors and skipped; the rest of the run continues and the exit code is non-zero. Remote destinations are unchanged (keys are opaque there). --flatten and single-object copies use Base() and were never affected. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Et7MCFVnqJFF8NLkA1VJrP --- command/cp.go | 18 +++++++++++++--- command/sync.go | 23 ++++++++++++++------ e2e/cp_test.go | 46 +++++++++++++++++++++++++++++++++++++++ e2e/sync_test.go | 37 +++++++++++++++++++++++++++++++ storage/url/url.go | 26 ++++++++++++++++++++++ storage/url/url_test.go | 48 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 189 insertions(+), 9 deletions(-) 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) + } + }) + } +}