From 01ccf204f42b4f68541b16843292439090a2dcf0 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Wed, 13 Nov 2024 11:55:28 +0000 Subject: [PATCH] local: fix permission and ownership on symlinks with --links and --metadata Before this change, if writing to a local backend with --metadata and --links, if the incoming metadata contained mode or ownership information then rclone would apply the mode/ownership to the destination of the link not the link itself. This fixes the problem by using the link safe sycall variants lchown/fchmodat when --links and --metadata is in use. Note that Linux does not support setting permissions on symlinks, so rclone emits a debug message in this case. This also fixes setting times on symlinks on Windows which wasn't implemented for atime, mtime and was incorrectly setting the target of the symlink for btime. See: https://github.com/rclone/rclone/security/advisories/GHSA-hrxh-9w67-g4cv --- backend/local/lchmod.go | 16 ++++++ backend/local/lchmod_unix.go | 41 +++++++++++++++ backend/local/lchtimes.go | 2 +- backend/local/lchtimes_windows.go | 19 +++++++ backend/local/local_internal_test.go | 76 ++++++++++++++++++++++++---- backend/local/metadata.go | 23 +++++++-- backend/local/setbtime.go | 6 +++ backend/local/setbtime_windows.go | 37 ++++++++++++-- 8 files changed, 202 insertions(+), 18 deletions(-) create mode 100644 backend/local/lchmod.go create mode 100644 backend/local/lchmod_unix.go create mode 100644 backend/local/lchtimes_windows.go diff --git a/backend/local/lchmod.go b/backend/local/lchmod.go new file mode 100644 index 000000000..823718dfe --- /dev/null +++ b/backend/local/lchmod.go @@ -0,0 +1,16 @@ +//go:build windows || plan9 || js || linux + +package local + +import "os" + +const haveLChmod = false + +// lChmod changes the mode of the named file to mode. If the file is a symbolic +// link, it changes the link, not the target. If there is an error, +// it will be of type *PathError. +func lChmod(name string, mode os.FileMode) error { + // Can't do this safely on this OS - chmoding a symlink always + // changes the destination. + return nil +} diff --git a/backend/local/lchmod_unix.go b/backend/local/lchmod_unix.go new file mode 100644 index 000000000..f1fdc4745 --- /dev/null +++ b/backend/local/lchmod_unix.go @@ -0,0 +1,41 @@ +//go:build !windows && !plan9 && !js && !linux + +package local + +import ( + "os" + "syscall" + + "golang.org/x/sys/unix" +) + +const haveLChmod = true + +// syscallMode returns the syscall-specific mode bits from Go's portable mode bits. +// +// Borrowed from the syscall source since it isn't public. +func syscallMode(i os.FileMode) (o uint32) { + o |= uint32(i.Perm()) + if i&os.ModeSetuid != 0 { + o |= syscall.S_ISUID + } + if i&os.ModeSetgid != 0 { + o |= syscall.S_ISGID + } + if i&os.ModeSticky != 0 { + o |= syscall.S_ISVTX + } + return o +} + +// lChmod changes the mode of the named file to mode. If the file is a symbolic +// link, it changes the link, not the target. If there is an error, +// it will be of type *PathError. +func lChmod(name string, mode os.FileMode) error { + // NB linux does not support AT_SYMLINK_NOFOLLOW as a parameter to fchmodat + // and returns ENOTSUP if you try, so we don't support this on linux + if e := unix.Fchmodat(unix.AT_FDCWD, name, syscallMode(mode), unix.AT_SYMLINK_NOFOLLOW); e != nil { + return &os.PathError{Op: "lChmod", Path: name, Err: e} + } + return nil +} diff --git a/backend/local/lchtimes.go b/backend/local/lchtimes.go index c8f03ef46..fcabdcc34 100644 --- a/backend/local/lchtimes.go +++ b/backend/local/lchtimes.go @@ -1,4 +1,4 @@ -//go:build windows || plan9 || js +//go:build plan9 || js package local diff --git a/backend/local/lchtimes_windows.go b/backend/local/lchtimes_windows.go new file mode 100644 index 000000000..a6dec9a12 --- /dev/null +++ b/backend/local/lchtimes_windows.go @@ -0,0 +1,19 @@ +//go:build windows + +package local + +import ( + "time" +) + +const haveLChtimes = true + +// lChtimes changes the access and modification times of the named +// link, similar to the Unix utime() or utimes() functions. +// +// The underlying filesystem may truncate or round the values to a +// less precise time unit. +// If there is an error, it will be of type *PathError. +func lChtimes(name string, atime time.Time, mtime time.Time) error { + return setTimes(name, atime, mtime, time.Time{}, true) +} diff --git a/backend/local/local_internal_test.go b/backend/local/local_internal_test.go index ea0fdc765..b3b9b8ba1 100644 --- a/backend/local/local_internal_test.go +++ b/backend/local/local_internal_test.go @@ -268,22 +268,66 @@ func TestMetadata(t *testing.T) { r := fstest.NewRun(t) const filePath = "metafile.txt" when := time.Now() - const dayLength = len("2001-01-01") - whenRFC := when.Format(time.RFC3339Nano) r.WriteFile(filePath, "metadata file contents", when) f := r.Flocal.(*Fs) + // Set fs into "-l" / "--links" mode + f.opt.TranslateSymlinks = true + + // Write a symlink to the file + symlinkPath := "metafile-link.txt" + osSymlinkPath := filepath.Join(f.root, symlinkPath) + symlinkPath += linkSuffix + require.NoError(t, os.Symlink(filePath, osSymlinkPath)) + symlinkModTime := fstest.Time("2002-02-03T04:05:10.123123123Z") + require.NoError(t, lChtimes(osSymlinkPath, symlinkModTime, symlinkModTime)) + // Get the object obj, err := f.NewObject(ctx, filePath) require.NoError(t, err) o := obj.(*Object) + // Get the symlink object + symlinkObj, err := f.NewObject(ctx, symlinkPath) + require.NoError(t, err) + symlinkO := symlinkObj.(*Object) + + // Record metadata for o + oMeta, err := o.Metadata(ctx) + require.NoError(t, err) + + // Test symlink first to check it doesn't mess up file + t.Run("Symlink", func(t *testing.T) { + testMetadata(t, r, symlinkO, symlinkModTime) + }) + + // Read it again + oMetaNew, err := o.Metadata(ctx) + require.NoError(t, err) + + // Check that operating on the symlink didn't change the file it was pointing to + // See: https://github.com/rclone/rclone/security/advisories/GHSA-hrxh-9w67-g4cv + assert.Equal(t, oMeta, oMetaNew, "metadata setting on symlink messed up file") + + // Now run the same tests on the file + t.Run("File", func(t *testing.T) { + testMetadata(t, r, o, when) + }) +} + +func testMetadata(t *testing.T, r *fstest.Run, o *Object, when time.Time) { + ctx := context.Background() + whenRFC := when.Format(time.RFC3339Nano) + const dayLength = len("2001-01-01") + + f := r.Flocal.(*Fs) features := f.Features() - var hasXID, hasAtime, hasBtime bool + var hasXID, hasAtime, hasBtime, canSetXattrOnLinks bool switch runtime.GOOS { case "darwin", "freebsd", "netbsd", "linux": hasXID, hasAtime, hasBtime = true, true, true + canSetXattrOnLinks = runtime.GOOS != "linux" case "openbsd", "solaris": hasXID, hasAtime = true, true case "windows": @@ -306,6 +350,10 @@ func TestMetadata(t *testing.T) { require.NoError(t, err) assert.Nil(t, m) + if !canSetXattrOnLinks && o.translatedLink { + t.Skip("Skip remainder of test as can't set xattr on symlinks on this OS") + } + inM := fs.Metadata{ "potato": "chips", "cabbage": "soup", @@ -320,18 +368,21 @@ func TestMetadata(t *testing.T) { }) checkTime := func(m fs.Metadata, key string, when time.Time) { + t.Helper() mt, ok := o.parseMetadataTime(m, key) assert.True(t, ok) dt := mt.Sub(when) precision := time.Second - assert.True(t, dt >= -precision && dt <= precision, fmt.Sprintf("%s: dt %v outside +/- precision %v", key, dt, precision)) + assert.True(t, dt >= -precision && dt <= precision, fmt.Sprintf("%s: dt %v outside +/- precision %v want %v got %v", key, dt, precision, mt, when)) } checkInt := func(m fs.Metadata, key string, base int) int { + t.Helper() value, ok := o.parseMetadataInt(m, key, base) assert.True(t, ok) return value } + t.Run("Read", func(t *testing.T) { m, err := o.Metadata(ctx) require.NoError(t, err) @@ -341,13 +392,12 @@ func TestMetadata(t *testing.T) { checkInt(m, "mode", 8) checkTime(m, "mtime", when) - assert.Equal(t, len(whenRFC), len(m["mtime"])) assert.Equal(t, whenRFC[:dayLength], m["mtime"][:dayLength]) - if hasAtime { + if hasAtime && !o.translatedLink { // symlinks generally don't record atime checkTime(m, "atime", when) } - if hasBtime { + if hasBtime && !o.translatedLink { // symlinks generally don't record btime checkTime(m, "btime", when) } if hasXID { @@ -371,6 +421,10 @@ func TestMetadata(t *testing.T) { "mode": "0767", "potato": "wedges", } + if !canSetXattrOnLinks && o.translatedLink { + // Don't change xattr if not supported on symlinks + delete(newM, "potato") + } err := o.writeMetadata(newM) require.NoError(t, err) @@ -380,7 +434,11 @@ func TestMetadata(t *testing.T) { mode := checkInt(m, "mode", 8) if runtime.GOOS != "windows" { - assert.Equal(t, 0767, mode&0777, fmt.Sprintf("mode wrong - expecting 0767 got 0%o", mode&0777)) + expectedMode := 0767 + if o.translatedLink && runtime.GOOS == "linux" { + expectedMode = 0777 // perms of symlinks always read as 0777 on linux + } + assert.Equal(t, expectedMode, mode&0777, fmt.Sprintf("mode wrong - expecting 0%o got 0%o", expectedMode, mode&0777)) } checkTime(m, "mtime", newMtime) @@ -390,7 +448,7 @@ func TestMetadata(t *testing.T) { if haveSetBTime { checkTime(m, "btime", newBtime) } - if xattrSupported { + if xattrSupported && (canSetXattrOnLinks || !o.translatedLink) { assert.Equal(t, "wedges", m["potato"]) } }) diff --git a/backend/local/metadata.go b/backend/local/metadata.go index 7ab69af30..75b195e64 100644 --- a/backend/local/metadata.go +++ b/backend/local/metadata.go @@ -105,7 +105,11 @@ func (o *Object) writeMetadataToFile(m fs.Metadata) (outErr error) { } if haveSetBTime { if btimeOK { - err = setBTime(o.path, btime) + if o.translatedLink { + err = lsetBTime(o.path, btime) + } else { + err = setBTime(o.path, btime) + } if err != nil { outErr = fmt.Errorf("failed to set birth (creation) time: %w", err) } @@ -121,7 +125,11 @@ func (o *Object) writeMetadataToFile(m fs.Metadata) (outErr error) { if runtime.GOOS == "windows" || runtime.GOOS == "plan9" { fs.Debugf(o, "Ignoring request to set ownership %o.%o on this OS", gid, uid) } else { - err = os.Chown(o.path, uid, gid) + if o.translatedLink { + err = os.Lchown(o.path, uid, gid) + } else { + err = os.Chown(o.path, uid, gid) + } if err != nil { outErr = fmt.Errorf("failed to change ownership: %w", err) } @@ -132,7 +140,16 @@ func (o *Object) writeMetadataToFile(m fs.Metadata) (outErr error) { if mode >= 0 { umode := uint(mode) if umode <= math.MaxUint32 { - err = os.Chmod(o.path, os.FileMode(umode)) + if o.translatedLink { + if haveLChmod { + err = lChmod(o.path, os.FileMode(umode)) + } else { + fs.Debugf(o, "Unable to set mode %v on a symlink on this OS", os.FileMode(umode)) + err = nil + } + } else { + err = os.Chmod(o.path, os.FileMode(umode)) + } if err != nil { outErr = fmt.Errorf("failed to change permissions: %w", err) } diff --git a/backend/local/setbtime.go b/backend/local/setbtime.go index 5c946348f..bb37b1735 100644 --- a/backend/local/setbtime.go +++ b/backend/local/setbtime.go @@ -13,3 +13,9 @@ func setBTime(name string, btime time.Time) error { // Does nothing return nil } + +// lsetBTime changes the birth time of the link passed in +func lsetBTime(name string, btime time.Time) error { + // Does nothing + return nil +} diff --git a/backend/local/setbtime_windows.go b/backend/local/setbtime_windows.go index 510a4da6a..8ae499826 100644 --- a/backend/local/setbtime_windows.go +++ b/backend/local/setbtime_windows.go @@ -9,15 +9,20 @@ import ( const haveSetBTime = true -// setBTime sets the birth time of the file passed in -func setBTime(name string, btime time.Time) (err error) { +// setTimes sets any of atime, mtime or btime +// if link is set it sets a link rather than the target +func setTimes(name string, atime, mtime, btime time.Time, link bool) (err error) { pathp, err := syscall.UTF16PtrFromString(name) if err != nil { return err } + fileFlag := uint32(syscall.FILE_FLAG_BACKUP_SEMANTICS) + if link { + fileFlag |= syscall.FILE_FLAG_OPEN_REPARSE_POINT + } h, err := syscall.CreateFile(pathp, syscall.FILE_WRITE_ATTRIBUTES, syscall.FILE_SHARE_WRITE, nil, - syscall.OPEN_EXISTING, syscall.FILE_FLAG_BACKUP_SEMANTICS, 0) + syscall.OPEN_EXISTING, fileFlag, 0) if err != nil { return err } @@ -27,6 +32,28 @@ func setBTime(name string, btime time.Time) (err error) { err = closeErr } }() - bFileTime := syscall.NsecToFiletime(btime.UnixNano()) - return syscall.SetFileTime(h, &bFileTime, nil, nil) + var patime, pmtime, pbtime *syscall.Filetime + if !atime.IsZero() { + t := syscall.NsecToFiletime(atime.UnixNano()) + patime = &t + } + if !mtime.IsZero() { + t := syscall.NsecToFiletime(mtime.UnixNano()) + pmtime = &t + } + if !btime.IsZero() { + t := syscall.NsecToFiletime(btime.UnixNano()) + pbtime = &t + } + return syscall.SetFileTime(h, pbtime, patime, pmtime) +} + +// setBTime sets the birth time of the file passed in +func setBTime(name string, btime time.Time) (err error) { + return setTimes(name, time.Time{}, time.Time{}, btime, false) +} + +// lsetBTime changes the birth time of the link passed in +func lsetBTime(name string, btime time.Time) error { + return setTimes(name, time.Time{}, time.Time{}, btime, true) }