diff --git a/backend/local/local.go b/backend/local/local.go index 91ec9cdce..50bfba7d0 100644 --- a/backend/local/local.go +++ b/backend/local/local.go @@ -818,7 +818,7 @@ func (f *Fs) PutStream(ctx context.Context, in io.Reader, src fs.ObjectInfo, opt // Mkdir creates the directory if it doesn't exist func (f *Fs) Mkdir(ctx context.Context, dir string) error { localPath := f.localPath(dir) - err := file.MkdirAll(localPath, 0777) + err := f.mkdirAll(localPath) if err != nil { return err } @@ -1435,10 +1435,84 @@ func (o *Object) Open(ctx context.Context, options ...fs.OpenOption) (in io.Read return in, nil } +// osRoot opens an *os.Root anchored at f.root, together with localPath +// expressed relative to it. The caller must Close the returned root. +// +// When translating symlinks (-l/--links) all writes go through an os.Root so a +// symlink planted by a malicious source can never be traversed to escape the +// destination (CWE-59, GHSA-cf44-9pgv-m4xc). +// +// f.root is the trusted destination the user chose, so it is created (and +// followed if it is itself a symlink) before being opened. +func (f *Fs) osRoot(localPath string) (root *os.Root, rel string, err error) { + rel, err = filepath.Rel(f.root, localPath) + if err != nil { + return nil, "", err + } + if err = file.MkdirAll(f.root, 0777); err != nil { + return nil, "", err + } + root, err = os.OpenRoot(f.root) + if err != nil { + return nil, "", err + } + return root, rel, nil +} + +// mkdirAll makes localPath and any missing parents. When translating +// symlinks it does so through os.Root so directory creation can't be +// redirected through a planted symlink out of the destination. +func (f *Fs) mkdirAll(localPath string) (err error) { + if !f.opt.TranslateSymlinks { + return file.MkdirAll(localPath, 0777) + } + root, rel, err := f.osRoot(localPath) + if err != nil { + return err + } + defer fs.CheckClose(root, &err) + if rel == "." { + return nil // the root itself, already created by linkRoot + } + return root.MkdirAll(rel, 0777) +} + +// openFile opens localPath for writing. When translating symlinks it goes +// through os.Root so a symlink planted at the path, or at any parent, is never +// followed out of the destination. The returned file is independent of the +// root, which is closed before returning. +func (f *Fs) openFile(localPath string, flags int, perm os.FileMode) (fi *os.File, err error) { + if !f.opt.TranslateSymlinks { + return file.OpenFile(localPath, flags, perm) + } + root, rel, err := f.osRoot(localPath) + if err != nil { + return nil, err + } + defer fs.CheckClose(root, &err) + return root.OpenFile(rel, flags, perm) +} + +// symlink creates a symlink with the given target at localPath, removing +// any existing file or symlink there first. It goes through os.Root, which +// creates the link verbatim (the target may point anywhere, preserving a +// faithful backup) but refuses to create it through a planted symlink, and +// won't remove a directory in the way. +func (f *Fs) symlink(target, localPath string) (err error) { + root, rel, err := f.osRoot(localPath) + if err != nil { + return err + } + defer fs.CheckClose(root, &err) + if err := root.Remove(rel); err != nil && !os.IsNotExist(err) { + return err + } + return root.Symlink(target, rel) +} + // mkdirAll makes all the directories needed to store the object func (o *Object) mkdirAll() error { - dir := filepath.Dir(o.path) - return file.MkdirAll(dir, 0777) + return o.fs.mkdirAll(filepath.Dir(o.path)) } type nopWriterCloser struct { @@ -1491,13 +1565,13 @@ func (o *Object) Update(ctx context.Context, in io.Reader, src fs.ObjectInfo, op // If it is a translated link, just read in the contents, and // then create a symlink if !o.translatedLink { - f, err := file.OpenFile(o.path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0666) + f, err := o.fs.openFile(o.path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0666) if err != nil { if runtime.GOOS == "windows" && os.IsPermission(err) { // If permission denied on Windows might be trying to update a // hidden file, in which case try opening without CREATE // See: https://stackoverflow.com/questions/13215716/ioerror-errno-13-permission-denied-when-trying-to-open-hidden-file-in-w-mod - f, err = file.OpenFile(o.path, os.O_WRONLY|os.O_TRUNC, 0666) + f, err = o.fs.openFile(o.path, os.O_WRONLY|os.O_TRUNC, 0666) if err != nil { return err } @@ -1534,15 +1608,10 @@ func (o *Object) Update(ctx context.Context, in io.Reader, src fs.ObjectInfo, op if o.translatedLink { if err == nil { - // Remove any current symlink or file, if one exists - if _, err := os.Lstat(o.path); err == nil { - if removeErr := os.Remove(o.path); removeErr != nil { - fs.Errorf(o, "Failed to remove previous file: %v", removeErr) - return removeErr - } - } - // Use the contents for the copied object to create a symlink - err = os.Symlink(symlinkData.String(), o.path) + // Use the contents of the copied object to create a symlink, + // without following or creating it through a planted symlink + // (CWE-59). Any existing file or symlink at the path is replaced. + err = o.fs.symlink(symlinkData.String(), o.path) } // only continue if symlink creation succeeded @@ -1606,7 +1675,7 @@ func (f *Fs) OpenWriterAt(ctx context.Context, remote string, size int64) (fs.Wr return nil, errors.New("can't open a symlink for random writing") } - out, err := file.OpenFile(o.path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0666) + out, err := f.openFile(o.path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0666) if err != nil { return nil, err } diff --git a/backend/local/local_internal_test.go b/backend/local/local_internal_test.go index f36cc1791..1b178904d 100644 --- a/backend/local/local_internal_test.go +++ b/backend/local/local_internal_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "runtime" "sort" + "sync" "testing" "time" @@ -204,6 +205,140 @@ func TestSymlinkError(t *testing.T) { assert.Equal(t, errLinksAndCopyLinks, err) } +// putLink writes target as a translated link object (name + ".rclonelink") on f. +func putLink(ctx context.Context, f fs.Fs, name, target string) error { + in := bytes.NewBufferString(target) + src := object.NewStaticObjectInfo(name+fs.LinkSuffix, fstest.Time("2001-02-03T04:05:10Z"), int64(len(target)), true, nil, nil) + _, err := f.Put(ctx, in, src) + return err +} + +// putFile writes content as a regular object on f. +func putFile(ctx context.Context, f fs.Fs, remote, content string) error { + in := bytes.NewBufferString(content) + src := object.NewStaticObjectInfo(remote, fstest.Time("2001-02-03T04:05:10Z"), int64(len(content)), true, nil, nil) + _, err := f.Put(ctx, in, src) + return err +} + +// linksMode puts f into "-l/--links" mode, as if --links or the backend +// links=true option were set. +func linksMode(f *Fs) { + f.opt.FollowSymlinks = false + f.opt.TranslateSymlinks = true + f.lstat = os.Lstat +} + +// TestSymlinkEscapeWriteThroughBlocked mirrors the GHSA-cf44-9pgv-m4xc PoC: a +// malicious --links source serves "pwn.rclonelink" whose body is a path outside +// the destination, plus a sibling "pwn/authkeys" that sorts after it and would +// be written through the planted symlink. rclone reproduces the symlink (a +// faithful backup of the source) but must refuse to write through it, so +// nothing lands outside the destination (CWE-59). +func TestSymlinkEscapeWriteThroughBlocked(t *testing.T) { + ctx := context.Background() + + // A directory outside the destination the attacker wants to write into + evil := t.TempDir() + evilFile := filepath.Join(evil, "authkeys") + + r := fstest.NewRun(t) + f := r.Flocal.(*Fs) + linksMode(f) + + // The symlink is reproduced faithfully, pointing outside the destination. + require.NoError(t, putLink(ctx, f, "pwn", evil)) + link := filepath.Join(f.root, "pwn") + fi, err := os.Lstat(link) + require.NoError(t, err) + require.True(t, fi.Mode()&os.ModeSymlink != 0, "symlink should be reproduced faithfully") + target, err := os.Readlink(link) + require.NoError(t, err) + require.Equal(t, evil, target) + + // But writing the sibling object through it must be refused. + err = putFile(ctx, f, "pwn/authkeys", "PWNED") + require.Error(t, err, "writing through a planted symlink should be refused") + + // Nothing escaped the destination. + _, err = os.Stat(evilFile) + require.True(t, os.IsNotExist(err), "a file escaped the destination into %q", evilFile) +} + +// TestSymlinkEscapeNestedBlocked covers the chained variant: an in-tree symlink +// "evil" -> "." (the destination root) is created, then "evil/pwn" -> outside +// is planted through it, then a write nested under that. Every component is +// re-validated against the root, so the write-through is refused and nothing +// escapes. +func TestSymlinkEscapeNestedBlocked(t *testing.T) { + ctx := context.Background() + + evil := t.TempDir() + evilFile := filepath.Join(evil, "authkeys") + + r := fstest.NewRun(t) + f := r.Flocal.(*Fs) + linksMode(f) + + require.NoError(t, putLink(ctx, f, "evil", ".")) + require.NoError(t, putLink(ctx, f, "evil/pwn", evil)) + + err := putFile(ctx, f, "evil/pwn/authkeys", "PWNED") + require.Error(t, err, "writing through a nested planted symlink should be refused") + + _, err = os.Stat(evilFile) + require.True(t, os.IsNotExist(err), "a file escaped the destination into %q", evilFile) +} + +// TestSymlinkEscapeConcurrent races symlink creation against the sibling write +// for many pairs at once, exercising the time-of-check/time-of-use window. +// os.Root resolves relative to a directory file descriptor, so whatever the +// interleaving nothing may escape the destination. +func TestSymlinkEscapeConcurrent(t *testing.T) { + ctx := context.Background() + + evil := t.TempDir() + + r := fstest.NewRun(t) + f := r.Flocal.(*Fs) + linksMode(f) + + const pairs = 50 + var wg sync.WaitGroup + for i := range pairs { + name := fmt.Sprintf("pwn%d", i) + wg.Add(2) + go func() { defer wg.Done(); _ = putLink(ctx, f, name, evil) }() + go func() { defer wg.Done(); _ = putFile(ctx, f, name+"/authkeys", "PWNED") }() + } + wg.Wait() + + entries, err := os.ReadDir(evil) + require.NoError(t, err) + require.Empty(t, entries, "files escaped the destination into %q", evil) +} + +// TestSymlinkInTreeWriteThroughWorks checks the fix doesn't break legitimate +// use: an in-tree symlink to a sibling directory can still be created and +// written through, since that write stays inside the destination. +func TestSymlinkInTreeWriteThroughWorks(t *testing.T) { + ctx := context.Background() + + r := fstest.NewRun(t) + f := r.Flocal.(*Fs) + linksMode(f) + + require.NoError(t, putFile(ctx, f, "sub/keep.txt", "hello")) + require.NoError(t, putLink(ctx, f, "link", "sub")) + + require.NoError(t, putFile(ctx, f, "link/file.txt", "world")) + + // The write landed in the real sibling directory, inside the destination. + got, err := os.ReadFile(filepath.Join(f.root, "sub", "file.txt")) + require.NoError(t, err) + require.Equal(t, "world", string(got)) +} + func TestHashWithTypeNone(t *testing.T) { ctx := context.Background() r := fstest.NewRun(t) diff --git a/docs/content/local.md b/docs/content/local.md index f5c621efa..5f480dd19 100644 --- a/docs/content/local.md +++ b/docs/content/local.md @@ -296,6 +296,31 @@ backends and the VFS. Note that this flag is incompatible with `-copy-links` / `-L`. +#### Symlink targets and the destination + +When rclone recreates a `.rclonelink` file as a symlink on local storage, +the symlink can point anywhere - including, with an absolute path or one +using `../` - to a location outside the directory you are copying into. This +is normal - rclone reproduces whatever target the link had, so backups +round-trip faithfully. + +What rclone will **not** do is *write through* such a link. If a remote you +are copying from contains both a symlink and a file or directory that would +be placed inside it - for example a `dir.rclonelink` pointing somewhere +outside the destination, alongside a `dir/file.txt` - rclone refuses to +follow the symlink when writing `dir/file.txt`. The offending file is +skipped with an error, the rest of the transfer continues, and the skipped +file is counted in the error summary printed at the end of the run. + +This protects you from a malicious or compromised remote using `-l` / +`--links` to plant a symlink and then write through it to somewhere outside +your destination. Ordinary symlink round-trips, and symlinks that stay +inside the destination, are unaffected. + +If you have intentionally pre-created a symlinked directory inside your +destination and want rclone to write into the directory it points at, do +not use `-l` / `--links` for that copy, or remove the symlink first. + ### Restricting filesystems with --one-file-system Normally rclone will recurse through filesystems as mounted.