From 5464970c5dde193cfe935c20b01f14dbd043cf9d Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Sat, 23 May 2026 08:21:04 +0200 Subject: [PATCH] fix(versioner): ensure user read/write/execute on archived dirs (fixes #10532) (#10696) This makes sure the user running Syncthing, and hence Synchting itself, has read/write/execute on directories in .stversions. The other permission bits remain copied from the source directory, ensuring whatever group and other permissions were set remain in effect. Closes #10695. --------- Signed-off-by: Jakob Borg --- lib/versioner/simple_test.go | 55 +++++++++++++++++++ lib/versioner/util.go | 102 ++++++++++++++++++++++------------- 2 files changed, 121 insertions(+), 36 deletions(-) diff --git a/lib/versioner/simple_test.go b/lib/versioner/simple_test.go index d2d008706..6ad83be20 100644 --- a/lib/versioner/simple_test.go +++ b/lib/versioner/simple_test.go @@ -15,6 +15,7 @@ import ( "github.com/syncthing/syncthing/lib/build" "github.com/syncthing/syncthing/lib/config" + "github.com/syncthing/syncthing/lib/fs" ) func TestTaggedFilename(t *testing.T) { @@ -256,3 +257,57 @@ func TestArchiveFoldersCreationPermission(t *testing.T) { t.Errorf("földer2 permissions %v, want %v", folder2VersionsInfo.Mode(), folder2Perms) } } + +func TestDupDirTreeWritePermissions(t *testing.T) { + // The structure should be replicated, with user permission bits set along the way + + srcFs := fs.NewFilesystem(fs.FilesystemTypeFake, "TestDupDirTreeWritePermissions/srcFs") + dstFs := fs.NewFilesystem(fs.FilesystemTypeFake, "TestDupDirTreeWritePermissions/dstFs") + + // A source dir to duplicate + _ = srcFs.Mkdir("foo", 0o444) + _ = srcFs.Mkdir("foo/bar", 0o555) + _ = srcFs.Mkdir("foo/bar/baz", 0o000) + + // Duplication should succeed + if err := dupDirTree(srcFs, dstFs, "foo/bar/baz"); err != nil { + t.Fatal(err) + } + + // Permissions should be the same, but with read/write/execute bits for + // the user + if info, err := dstFs.Lstat("foo"); err != nil || info.Mode() != 0o744 { + t.Fatalf("foo: 0o%o", info.Mode()) + } + if info, err := dstFs.Lstat("foo/bar"); err != nil || info.Mode() != 0o755 { + t.Fatalf("foo/bar: 0o%o", info.Mode()) + } + if info, err := dstFs.Lstat("foo/bar/baz"); err != nil || info.Mode() != 0o700 { + t.Fatalf("foo/bar/baz: 0o%o", info.Mode()) + } +} + +func TestDupDirFastPath(t *testing.T) { + srcFs := fs.NewFilesystem(fs.FilesystemTypeFake, "TestDupDirFastPath/srcFs") + dstFs := fs.NewFilesystem(fs.FilesystemTypeFake, "TestDupDirFastPath/dstFs") + + // A source dir to duplicate + _ = srcFs.Mkdir("foo", 0o444) + _ = srcFs.Mkdir("foo/bar", 0o555) + _ = srcFs.Mkdir("foo/bar/baz", 0o000) + + // The destination exists, but with too few permission bits + _ = dstFs.MkdirAll("foo/bar/baz", 0o555) + + // Duplication should succeed + if err := dupDirTree(srcFs, dstFs, "foo/bar/baz"); err != nil { + t.Fatal(err) + } + + // Permissions for the destination should have been updated. (This + // differs from what would have been created by the duplication of the + // 0o000 dir in the src, because it already existed.) + if info, err := dstFs.Lstat("foo/bar/baz"); err != nil || info.Mode() != 0o755 { + t.Fatalf("foo/bar/baz: 0o%o", info.Mode()) + } +} diff --git a/lib/versioner/util.go b/lib/versioner/util.go index ebb358f1f..221de5dc5 100644 --- a/lib/versioner/util.go +++ b/lib/versioner/util.go @@ -192,48 +192,78 @@ func archiveFile(method fs.CopyRangeMethod, srcFs, dstFs fs.Filesystem, filePath return err } -func dupDirTree(srcFs, dstFs fs.Filesystem, folderPath string) error { - // Return early if the folder already exists. - _, err := dstFs.Stat(folderPath) - if err == nil || !fs.IsNotExist(err) { - return err +// dupDirTree ensures folderPath exists in dstFs, copying permissions mostly +// from srcFs. Permissions are altered to have the user read, write, and +// execute bits set so that Syncthing file operations are possible within +// the destination directory. +// +// We want to retain the source group and other bits so that we do not +// inadvertently open up a directory for users who shouldn't have access to +// it, but we do not consider it a security issue to open up the permissions +// for the current user. +// +// This is based on os.MkdirAll with our srcFs adjustments. +func dupDirTree(srcFs, dstFs fs.Filesystem, path string) error { + const ( + allPerms = 0o777 + minDirPerms = 0o700 + ) + + // Fast path: if we can tell whether path is a directory or file, stop with success or error. + if dir, err := dstFs.Lstat(path); err == nil { + if !dir.IsDir() { + return errors.New("destination exists but is not a directory") + } + if dir.Mode()&minDirPerms != minDirPerms { + // We want all the required permission bits set + _ = dstFs.Chmod(path, dir.Mode()&allPerms|minDirPerms) + } + return nil } - hadParent := true - for i := range folderPath { - if os.IsPathSeparator(folderPath[i]) { - // If the parent folder didn't exist, then this folder doesn't exist - // so we can skip the check - if hadParent { - _, err := dstFs.Stat(folderPath[:i]) - if err == nil { - continue - } - if !fs.IsNotExist(err) { - return err - } - } - hadParent = false - err := dupDirWithPerms(srcFs, dstFs, folderPath[:i]) - if err != nil { - return err - } + + // Slow path: make sure parent exists and then call Mkdir for path. + + // Extract the parent folder from path by first removing any trailing + // path separator and then scanning backward until finding a path + // separator or reaching the beginning of the string. + i := len(path) - 1 + for i >= 0 && os.IsPathSeparator(path[i]) { + i-- + } + for i >= 0 && !os.IsPathSeparator(path[i]) { + i-- + } + if i < 0 { + i = 0 + } + + // If there is a parent directory, and it is not the volume name, + // recurse to ensure parent directory exists. + if parent := path[:i]; len(parent) > len(filepath.VolumeName(path)) { + if err := dupDirTree(srcFs, dstFs, parent); err != nil { + return err } } - return dupDirWithPerms(srcFs, dstFs, folderPath) -} -func dupDirWithPerms(srcFs, dstFs fs.Filesystem, folderPath string) error { - srcStat, err := srcFs.Stat(folderPath) - if err != nil { + // Parent now exists; invoke Mkdir and use its result. + srcPerms := fs.FileMode(minDirPerms) + if srcDir, err := srcFs.Lstat(path); err == nil { + srcPerms = srcDir.Mode()&allPerms | minDirPerms + } + if err := dstFs.Mkdir(path, srcPerms); err != nil { + // Handle arguments like "foo/." by + // double-checking that directory doesn't exist. + dir, err1 := dstFs.Lstat(path) + if err1 == nil && dir.IsDir() { + return nil + } return err } - // If we call Mkdir with srcStat.Mode(), we won't get the expected perms because of umask - // So, we create the folder with 0700, and then change the perms to the srcStat.Mode() - err = dstFs.Mkdir(folderPath, 0o700) - if err != nil { - return err - } - return dstFs.Chmod(folderPath, srcStat.Mode()) + + // Extra chmod to ensure our permissions override umask + _ = dstFs.Chmod(path, srcPerms) + + return nil } func restoreFile(method fs.CopyRangeMethod, src, dst fs.Filesystem, filePath string, versionTime time.Time, tagger fileTagger) error {