rclone: apply patch for CVE-2024-52522

This commit is contained in:
Leona Maroni 2024-11-16 22:03:14 +01:00
parent 035d434d48
commit e8fa322231
No known key found for this signature in database
GPG Key ID: D5B08ADFC75E3605
2 changed files with 445 additions and 0 deletions

View File

@ -0,0 +1,441 @@
From a314da376573a48317515c95d7b529b63c219c65 Mon Sep 17 00:00:00 2001
From: Leona Maroni <dev@leona.is>
Date: Thu, 14 Nov 2024 16:13:57 +0000
Subject: [PATCH] Patch for CVE-2024-52522
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
(cherry picked from commit 1e2b354456882ed22d8674b8bf37de55e0069514)
Original author: Nick Craig-Wood <nick@craig-wood.com>
---
backend/local/lchmod.go | 16 ++++++
backend/local/lchmod_unix.go | 41 +++++++++++++++
backend/local/lchtimes.go | 4 +-
backend/local/lchtimes_windows.go | 19 +++++++
backend/local/local_internal_test.go | 76 ++++++++++++++++++++++++----
backend/local/metadata.go | 33 ++++++++++--
backend/local/setbtime.go | 6 +++
backend/local/setbtime_windows.go | 37 ++++++++++++--
8 files changed, 211 insertions(+), 21 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 97f3b1be1..38ec73dfe 100644
--- a/backend/local/lchtimes.go
+++ b/backend/local/lchtimes.go
@@ -1,5 +1,5 @@
-//go:build windows || plan9 || js
-// +build windows plan9 js
+//go:build plan9 || js
+// +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 06e47bed2..fc3fcce41 100644
--- a/backend/local/local_internal_test.go
+++ b/backend/local/local_internal_test.go
@@ -251,22 +251,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":
@@ -289,6 +333,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",
@@ -303,18 +351,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)
@@ -324,13 +375,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 {
@@ -354,6 +404,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)
@@ -363,7 +417,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)
@@ -373,7 +431,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 097abedf1..a4995714e 100644
--- a/backend/local/metadata.go
+++ b/backend/local/metadata.go
@@ -2,6 +2,7 @@ package local
import (
"fmt"
+ "math"
"os"
"runtime"
"strconv"
@@ -104,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)
}
@@ -120,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)
}
@@ -128,9 +137,23 @@ func (o *Object) writeMetadataToFile(m fs.Metadata) (outErr error) {
}
mode, hasMode := o.parseMetadataInt(m, "mode", 8)
if hasMode {
- err = os.Chmod(o.path, os.FileMode(mode))
- if err != nil {
- outErr = fmt.Errorf("failed to change permissions: %w", err)
+ if mode >= 0 {
+ umode := uint(mode)
+ if umode <= math.MaxUint32 {
+ 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)
+ }
+ }
}
}
// FIXME not parsing rdev yet
diff --git a/backend/local/setbtime.go b/backend/local/setbtime.go
index 5d2c462ee..c85310916 100644
--- a/backend/local/setbtime.go
+++ b/backend/local/setbtime.go
@@ -14,3 +14,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 2a46f09eb..b04a2cfb5 100644
--- a/backend/local/setbtime_windows.go
+++ b/backend/local/setbtime_windows.go
@@ -10,15 +10,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
}
@@ -28,6 +33,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)
}
--
2.47.0

View File

@ -15,6 +15,10 @@ buildGoModule rec {
hash = "sha256-75RnAROICtRUDn95gSCNO0F6wes4CkJteNfUN38GQIY="; hash = "sha256-75RnAROICtRUDn95gSCNO0F6wes4CkJteNfUN38GQIY=";
}; };
patches = [
./CVE-2024-52522.patch
];
vendorHash = "sha256-zGBwgIuabLDqWbutvPHDbPRo5Dd9kNfmgToZXy7KVgI="; vendorHash = "sha256-zGBwgIuabLDqWbutvPHDbPRo5Dd9kNfmgToZXy7KVgI=";
subPackages = [ "." ]; subPackages = [ "." ];