From 2804606f9f8dc5078c38580bac363b47eb638620 Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Tue, 1 Jun 2021 07:59:59 +0200 Subject: [PATCH] remote: guard against symlinks in earlier path components This was only an issue when syncing files to directories where other users have write access to the parent directory. For example when copying files to /home/user/.ssh/authorized_keys the user could replace .ssh with a symlink which permitted overwriting authorized_keys anywhere on the system. Fix possible attacks by using *at syscalls and disallowing symlinks in all path components except for the last of synced paths. Syncing symlinks is obviously still permitted. --- cmd/safcm/fixperms.go | 2 +- remote/ainsl/ainsl.go | 34 ++-- remote/ainsl/ainsl_test.go | 6 +- remote/sync/files.go | 301 +++++++++++++++++++++------- remote/sync/files_compat.go | 26 +++ remote/sync/files_compat_openbsd.go | 26 +++ remote/sync/files_test.go | 103 +++++++++- remote/sync/files_windows.go | 2 +- 8 files changed, 408 insertions(+), 92 deletions(-) create mode 100644 remote/sync/files_compat.go create mode 100644 remote/sync/files_compat_openbsd.go diff --git a/cmd/safcm/fixperms.go b/cmd/safcm/fixperms.go index 36d618f..af5c658 100644 --- a/cmd/safcm/fixperms.go +++ b/cmd/safcm/fixperms.go @@ -101,7 +101,7 @@ func fixpermsWalkDirFunc(path string, d fs.DirEntry, err error) error { // chmodNoFollow works like os.Chmod but doesn't follow symlinks. func chmodNoFollow(path string, mode fs.FileMode) error { - x, err := sync.OpenFileNoFollow(path) + x, err := sync.OpenFileNoSymlinks(path) if err != nil { return err } diff --git a/remote/ainsl/ainsl.go b/remote/ainsl/ainsl.go index d716b2b..2a64d63 100644 --- a/remote/ainsl/ainsl.go +++ b/remote/ainsl/ainsl.go @@ -28,10 +28,11 @@ import ( "io" "io/fs" "os" - "path/filepath" "strings" "syscall" + "golang.org/x/sys/unix" + "ruderich.org/simon/safcm/remote/sync" ) @@ -74,14 +75,20 @@ func handle(path string, line string, create bool) ([]string, error) { line) } + parentFd, baseName, err := sync.OpenParentDirectoryNoSymlinks(path) + if err != nil { + return nil, err + } + defer unix.Close(parentFd) + var changes []string var uid, gid int var mode fs.FileMode - data, stat, err := readFileNoFollow(path) + data, stat, err := readFileAtNoFollow(parentFd, baseName) if err != nil { if !os.IsNotExist(err) { - return nil, err + return nil, fmt.Errorf("%q: %v", path, err) } if !create { return nil, fmt.Errorf( @@ -147,18 +154,17 @@ func handle(path string, line string, create bool) ([]string, error) { } // Write via temporary file and rename - dir := filepath.Dir(path) - base := filepath.Base(path) - tmpPath, err := sync.WriteTemp(dir, "."+base, data, uid, gid, mode) + tmpBase, err := sync.WriteTempAt(parentFd, "."+baseName, + data, uid, gid, mode) if err != nil { return nil, err } - err = os.Rename(tmpPath, path) + err = unix.Renameat(parentFd, tmpBase, parentFd, baseName) if err != nil { - os.Remove(tmpPath) + unix.Unlinkat(parentFd, tmpBase, 0) return nil, err } - err = sync.SyncPath(dir) + err = unix.Fsync(parentFd) if err != nil { return nil, err } @@ -166,8 +172,8 @@ func handle(path string, line string, create bool) ([]string, error) { return changes, nil } -func readFileNoFollow(path string) ([]byte, fs.FileInfo, error) { - fh, err := sync.OpenFileNoFollow(path) +func readFileAtNoFollow(dirfd int, base string) ([]byte, fs.FileInfo, error) { + fh, err := sync.OpenAtNoFollow(dirfd, base) if err != nil { return nil, nil, err } @@ -178,13 +184,13 @@ func readFileNoFollow(path string) ([]byte, fs.FileInfo, error) { return nil, nil, err } if stat.Mode().Type() != 0 /* regular file */ { - return nil, nil, fmt.Errorf("%q is not a regular file but %s", - path, stat.Mode().Type()) + return nil, nil, fmt.Errorf("not a regular file but %s", + stat.Mode().Type()) } x, err := io.ReadAll(fh) if err != nil { - return nil, nil, fmt.Errorf("%q: %v", path, err) + return nil, nil, err } return x, stat, nil diff --git a/remote/ainsl/ainsl_test.go b/remote/ainsl/ainsl_test.go index 3d391f0..ed52bb0 100644 --- a/remote/ainsl/ainsl_test.go +++ b/remote/ainsl/ainsl_test.go @@ -54,10 +54,10 @@ func TestHandle(t *testing.T) { } _, uid, _, gid := ft.CurrentUserAndGroup() - symlinkExists := "open file: too many levels of symbolic links" + symlinkExists := "\"file\": too many levels of symbolic links" if runtime.GOOS == "freebsd" { // EMLINK instead of ELOOP - symlinkExists = "open file: too many links" + symlinkExists = "\"file\": too many links" } tests := []struct { @@ -293,7 +293,7 @@ func TestHandle(t *testing.T) { }, }, nil, - fmt.Errorf("\"file\" is not a regular file but p---------"), + fmt.Errorf("\"file\": not a regular file but p---------"), }, } diff --git a/remote/sync/files.go b/remote/sync/files.go index e3fc3d9..edf81bd 100644 --- a/remote/sync/files.go +++ b/remote/sync/files.go @@ -28,18 +28,30 @@ import ( "net/http" "os" "os/user" + slashpath "path" "path/filepath" "sort" "strconv" "strings" - "syscall" "time" "github.com/ianbruene/go-difflib/difflib" + "golang.org/x/sys/unix" "ruderich.org/simon/safcm" ) +// NOTE: Don't use plain os.Open, os.OpenFile, os.Remove or any other function +// which uses absolute paths. These are vulnerable to symlink attacks. Always +// use *at syscalls. See below for details. + +// openReadonlyFlags are flags for open* syscalls to safely read a file or +// directory. +// +// O_NOFOLLOW prevents symlink attacks +// O_NONBLOCK is necessary to prevent blocking on FIFOs +const openReadonlyFlags = unix.O_RDONLY | unix.O_NOFOLLOW | unix.O_NONBLOCK + func (s *Sync) syncFiles() error { // To create random file names for symlinks rand.Seed(time.Now().UnixNano()) @@ -83,7 +95,14 @@ func (s *Sync) syncFile(file *safcm.File, changed *bool) error { // The implementation is careful not to follow any symlinks to prevent // possible race conditions which can be exploited and are especially // dangerous when running with elevated privileges (which will most - // likely be the case). + // likely be the case). This includes not using absolute paths in + // syscalls to prevent symlink attacks when a directory is writable by + // other users (e.g. when syncing a file to /home/user/dir/file the + // user could create dir as symlink to another directory and file + // would be written there). To prevent this *at syscalls are used and + // all symlinks in the path are rejected. This still permits the user + // to move dir during the sync but only to places which are writable + // by the user which cannot be prevented. err := s.fileResolveIds(file) if err != nil { @@ -110,26 +129,27 @@ func (s *Sync) syncFile(file *safcm.File, changed *bool) error { file.Path, file.OrigGroup, fmt.Sprintf(format, a...)) } - var oldStat fs.FileInfo + parentFd, baseName, err := OpenParentDirectoryNoSymlinks(file.Path) + if err != nil { + return err + } + defer unix.Close(parentFd) + + var oldStat unix.Stat_t reopen: - oldFh, err := OpenFileNoFollow(file.Path) + oldFh, err := OpenAtNoFollow(parentFd, baseName) if err != nil { - err := err.(*fs.PathError) - if err.Err == syscall.ELOOP || err.Err == syscall.EMLINK { - // Check if ELOOP was caused not by O_NOFOLLOW but by - // too many nested symlinks before the final path - // component. - x, err := os.Lstat(file.Path) + if err == unix.ELOOP || err == unix.EMLINK { + // ELOOP from symbolic link, this is fine + err := unix.Fstatat(parentFd, baseName, &oldStat, + unix.AT_SYMLINK_NOFOLLOW) if err != nil { return err } - if x.Mode().Type() != fs.ModeSymlink { - debugf("type changed from symlink to %s, retry", - x.Mode().Type()) + if oldStat.Mode&unix.S_IFMT != unix.S_IFLNK { + debugf("type changed from symlink, retrying") goto reopen } - // ELOOP from symbolic link, this is fine - oldStat = x } else if os.IsNotExist(err) { change.Created = true debugf("will create") @@ -139,18 +159,53 @@ reopen: } else { defer oldFh.Close() - x, err := oldFh.Stat() + err := unix.Fstat(int(oldFh.Fd()), &oldStat) if err != nil { return err } - oldStat = x } var oldData []byte var changeType, changePerm, changeUserOrGroup, changeData bool if !change.Created { + // Manually convert to FileMode; from src/os/stat_linux.go in + // Go's sources (stat_*.go for other UNIX systems are + // identical, except for stat_darwin.go which has an extra + // S_IFWHT) + mode := fs.FileMode(oldStat.Mode & 0777) + switch oldStat.Mode & unix.S_IFMT { + case unix.S_IFBLK: + mode |= fs.ModeDevice + case unix.S_IFCHR: + mode |= fs.ModeDevice | fs.ModeCharDevice + case unix.S_IFDIR: + mode |= fs.ModeDir + case unix.S_IFIFO: + mode |= fs.ModeNamedPipe + case unix.S_IFLNK: + mode |= fs.ModeSymlink + case unix.S_IFREG: + // nothing to do + case unix.S_IFSOCK: + mode |= fs.ModeSocket + // Guard against unknown file types (e.g. on darwin); not in + // stat_*.go + default: + return fmt.Errorf("unexpected file mode %v", + oldStat.Mode&unix.S_IFMT) + } + if oldStat.Mode&unix.S_ISGID != 0 { + mode |= fs.ModeSetgid + } + if oldStat.Mode&unix.S_ISUID != 0 { + mode |= fs.ModeSetuid + } + if oldStat.Mode&unix.S_ISVTX != 0 { + mode |= fs.ModeSticky + } + // Compare permissions - change.Old.Mode = oldStat.Mode() + change.Old.Mode = mode if change.Old.Mode.Type() == fs.ModeSymlink { // Some BSD systems permit changing permissions of // symlinks but ignore them on traversal. To keep it @@ -176,12 +231,8 @@ reopen: } // Compare user/group - x, ok := oldStat.Sys().(*syscall.Stat_t) - if !ok { - return fmt.Errorf("unsupported Stat().Sys()") - } - change.Old.Uid = int(x.Uid) - change.Old.Gid = int(x.Gid) + change.Old.Uid = int(oldStat.Uid) + change.Old.Gid = int(oldStat.Gid) if change.Old.Uid != file.Uid || change.Old.Gid != file.Gid { changeUserOrGroup = true debugf("uid/gid differs %d/%d -> %d/%d", @@ -207,12 +258,13 @@ reopen: } oldData = x case fs.ModeSymlink: - x, err := os.Readlink(file.Path) + buf := make([]byte, unix.PathMax) + n, err := unix.Readlinkat(parentFd, baseName, buf) if err != nil { return fmt.Errorf("reading old content: %v", err) } - oldData = []byte(x) + oldData = buf[:n] } if !changeType && file.Mode.Type() != fs.ModeDir { if !bytes.Equal(oldData, file.Data) { @@ -221,7 +273,6 @@ reopen: } } } - oldStat = nil // prevent accidental use // No changes if !change.Created && !changeType && @@ -263,25 +314,43 @@ reopen: // We cannot rename over directories and vice versa if changeType && (change.Old.Mode.IsDir() || file.Mode.IsDir()) { debugf("removing (due to type change)") - err := os.RemoveAll(file.Path) - if err != nil { - return err + // In the past os.RemoveAll() was used here. However, this is + // difficult to implement manually with *at syscalls. To keep + // it simple only permit removing files and empty directories + // here. This also has the bonus of preventing data loss when + // (accidentally) replacing a directory tree with a file. + const msg = "will not replace non-empty directory, " + + "please remove manually" + err := unix.Unlinkat(parentFd, baseName, 0) + if err != nil && !os.IsNotExist(err) { + err2 := unix.Unlinkat(parentFd, baseName, + AT_REMOVEDIR) + if err2 != nil && !os.IsNotExist(err2) { + // See src/os/file_unix.go in Go's sources + if err2 == unix.ENOTDIR { + return err + } else if err2 == unix.ENOTEMPTY { + return fmt.Errorf(msg) + } else { + return err2 + } + } } } // Directory: create new directory, also type change to directory if file.Mode.IsDir() && (change.Created || changeType) { debugf("creating directory") - err := os.Mkdir(file.Path, 0700) + err := unix.Mkdirat(parentFd, baseName, 0700) if err != nil { return err } // We must be careful not to chmod arbitrary files. If the // target directory is writable then it might have changed to - // a symlink at this point. There's no lchmod so open the - // directory. + // a symlink at this point. There's no lchmod and fchmodat is + // incomplete so open the directory. debugf("chmodding %s", file.Mode) - dh, err := OpenFileNoFollow(file.Path) + dh, err := OpenAtNoFollow(parentFd, baseName) if err != nil { return err } @@ -337,31 +406,29 @@ reopen: return nil } - dir := filepath.Dir(file.Path) + dir := slashpath.Dir(file.Path) // only used in debug messages // Create hidden file which should be ignored by most other tools and // thus not affect anything during creation - base := "." + filepath.Base(file.Path) + tmpBase := "." + baseName - var tmpPath string switch file.Mode.Type() { case 0: // regular file debugf("creating temporary file %q", - filepath.Join(dir, base+"*")) - tmpPath, err = WriteTemp(dir, base, file.Data, + slashpath.Join(dir, tmpBase+"*")) + x, err := WriteTempAt(parentFd, tmpBase, file.Data, file.Uid, file.Gid, file.Mode) if err != nil { return err } + tmpBase = x case fs.ModeSymlink: i := 0 retry: - // Similar to os.CreateTemp() but for symlinks which we cannot - // open as file - tmpPath = filepath.Join(dir, - base+strconv.Itoa(rand.Int())) - debugf("creating temporary symlink %q", tmpPath) - err := os.Symlink(string(file.Data), tmpPath) + x := tmpBase + strconv.Itoa(rand.Int()) + debugf("creating temporary symlink %q", + slashpath.Join(dir, x)) + err := unix.Symlinkat(string(file.Data), parentFd, x) if err != nil { if os.IsExist(err) && i < 10000 { i++ @@ -369,9 +436,12 @@ reopen: } return err } - err = os.Lchown(tmpPath, file.Uid, file.Gid) + tmpBase = x + + err = unix.Fchownat(parentFd, tmpBase, file.Uid, file.Gid, + unix.AT_SYMLINK_NOFOLLOW) if err != nil { - os.Remove(tmpPath) + unix.Unlinkat(parentFd, tmpBase, 0) return err } // Permissions are irrelevant for symlinks (on most systems) @@ -380,13 +450,13 @@ reopen: panic(fmt.Sprintf("invalid file type %s", file.Mode)) } - debugf("renaming %q", tmpPath) - err = os.Rename(tmpPath, file.Path) + debugf("renaming %q", slashpath.Join(dir, tmpBase)) + err = unix.Renameat(parentFd, tmpBase, parentFd, baseName) if err != nil { - os.Remove(tmpPath) + unix.Unlinkat(parentFd, tmpBase, 0) return err } - err = SyncPath(dir) + err = unix.Fsync(parentFd) if err != nil { return err } @@ -437,6 +507,72 @@ func (s *Sync) fileResolveIds(file *safcm.File) error { return nil } +// OpenParentDirectoryNoSymlinks opens the dirname of path without following +// any symlinks and returns a file descriptor to it and the basename of path. +// To prevent symlink attacks in earlier path components when these are +// writable by other users it starts at the root (or current directory for +// relative paths) and uses openat (with O_NOFOLLOW) for each path component. +// If a symlink is encountered it returns an error. However, it's impossible +// to guarantee that the returned descriptor refers to the same location as +// given in path because users with write access can rename path components. +// But this is not required to prevent the mentioned attacks. +func OpenParentDirectoryNoSymlinks(path string) (int, string, error) { + // Slash separated paths are used for the configuration + parts := strings.Split(path, "/") + + var dir string + if path == "/" { + // Root: use root itself as base name because root is the + // parent of itself + dir = "/" + parts = []string{"/"} + } else if parts[0] == "" { + // Absolute path + dir = "/" + parts = parts[1:] + } else if path == "." { + // Current directory: open parent directory and use current + // directory name as base name + wd, err := os.Getwd() + if err != nil { + return -1, "", fmt.Errorf( + "failed to get working directory: %w", err) + } + dir = ".." + parts = []string{filepath.Base(wd)} + } else if parts[0] != "." { + // Relative path: start at the current directory + dir = "." + } + + dirFd, err := unix.Openat(unix.AT_FDCWD, dir, openReadonlyFlags, 0) + if err != nil { + return -1, "", err + } + // Walk path one directory at a time to ensure there are no symlinks + // in the path. This prevents users with write access to change the + // path to point to arbitrary locations. O_NOFOLLOW when opening the + // path is not enough as only the last path component is checked. + for i, name := range parts[:len(parts)-1] { + fd, err := unix.Openat(dirFd, name, openReadonlyFlags, 0) + if err != nil { + unix.Close(dirFd) + if err == unix.ELOOP || err == unix.EMLINK { + x := filepath.Join(append([]string{dir}, + parts[:i+1]...)...) + return -1, "", fmt.Errorf( + "symlink not permitted in path: %q", + x) + } + return -1, "", err + } + unix.Close(dirFd) + dirFd = fd + } + + return dirFd, parts[len(parts)-1], nil +} + func resolveIdsToNames(uid, gid int) (string, string, error) { u, err := user.LookupId(strconv.Itoa(uid)) if err != nil { @@ -479,54 +615,85 @@ func diffData(oldData []byte, newData []byte) (string, error) { return result, nil } -func OpenFileNoFollow(path string) (*os.File, error) { - return os.OpenFile(path, - // O_NOFOLLOW prevents symlink attacks - // O_NONBLOCK is necessary to prevent blocking on FIFOs - os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) +func OpenFileNoSymlinks(path string) (*os.File, error) { + parentFd, baseName, err := OpenParentDirectoryNoSymlinks(path) + if err != nil { + return nil, err + } + defer unix.Close(parentFd) + return OpenAtNoFollow(parentFd, baseName) +} + +func OpenAtNoFollow(dirFd int, base string) (*os.File, error) { + fd, err := unix.Openat(dirFd, base, openReadonlyFlags, 0) + if err != nil { + return nil, err + } + return os.NewFile(uintptr(fd), ""), nil } -func WriteTemp(dir, base string, data []byte, uid, gid int, mode fs.FileMode) ( - string, error) { +func WriteTempAt(dirFd int, base string, data []byte, uid, gid int, + mode fs.FileMode) (string, error) { - fh, err := os.CreateTemp(dir, base) + fh, tmpBase, err := createTempAt(dirFd, base) if err != nil { return "", err } - tmpPath := fh.Name() _, err = fh.Write(data) if err != nil { fh.Close() - os.Remove(tmpPath) + unix.Unlinkat(dirFd, tmpBase, 0) return "", err } - // CreateTemp() creates the file with 0600 + // createTempAt() creates the file with 0600 err = fh.Chown(uid, gid) if err != nil { fh.Close() - os.Remove(tmpPath) + unix.Unlinkat(dirFd, tmpBase, 0) return "", err } err = fh.Chmod(mode) if err != nil { fh.Close() - os.Remove(tmpPath) + unix.Unlinkat(dirFd, tmpBase, 0) return "", err } err = fh.Sync() if err != nil { fh.Close() - os.Remove(tmpPath) + unix.Unlinkat(dirFd, tmpBase, 0) return "", err } err = fh.Close() if err != nil { - os.Remove(tmpPath) + unix.Unlinkat(dirFd, tmpBase, 0) return "", err } - return tmpPath, nil + return tmpBase, nil +} + +// createTempAt works similar to os.CreateTemp but uses unix.Openat to create +// the temporary file. +func createTempAt(dirFd int, base string) (*os.File, string, error) { + var tmpBase string + + i := 0 +retry: + tmpBase = base + strconv.Itoa(rand.Int()) + + fd, err := unix.Openat(dirFd, tmpBase, + unix.O_RDWR|unix.O_CREAT|unix.O_EXCL, 0600) + if err != nil { + if os.IsExist(err) && i < 10000 { + i++ + goto retry + } + return nil, "", err + } + + return os.NewFile(uintptr(fd), ""), tmpBase, nil } // SyncPath syncs path, which should be a directory. To guarantee durability diff --git a/remote/sync/files_compat.go b/remote/sync/files_compat.go new file mode 100644 index 0000000..b293148 --- /dev/null +++ b/remote/sync/files_compat.go @@ -0,0 +1,26 @@ +// MsgSyncReq: copy files to the remote host; compatibility + +// Copyright (C) 2021 Simon Ruderich +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +// +build !openbsd,!windows + +package sync + +import ( + "golang.org/x/sys/unix" +) + +const AT_REMOVEDIR = unix.AT_REMOVEDIR diff --git a/remote/sync/files_compat_openbsd.go b/remote/sync/files_compat_openbsd.go new file mode 100644 index 0000000..f684883 --- /dev/null +++ b/remote/sync/files_compat_openbsd.go @@ -0,0 +1,26 @@ +// MsgSyncReq: copy files to the remote host; OpenBSD compatibility + +// Copyright (C) 2021 Simon Ruderich +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +// +build openbsd + +package sync + +// Currently not defined in golang.org/x/sys/unix, see also +// https://github.com/golang/go/issues/46342 +// +// From /usr/include/fcntl.h +const AT_REMOVEDIR = 0x08 diff --git a/remote/sync/files_test.go b/remote/sync/files_test.go index 3971459..3d19e7f 100644 --- a/remote/sync/files_test.go +++ b/remote/sync/files_test.go @@ -963,7 +963,7 @@ func TestSyncFile(t *testing.T) { `4: files: "file" (group): will create`, `3: files: "file" (group): creating`, `4: files: "file" (group): creating temporary file ".file*"`, - `4: files: "file" (group): renaming "./.fileRND"`, + `4: files: "file" (group): renaming ".fileRND"`, }, nil, }, @@ -1116,7 +1116,7 @@ func TestSyncFile(t *testing.T) { `4: files: "file" (group): permission differs -rwxr-xr-x -> urwxr-xr-x`, `3: files: "file" (group): updating`, `4: files: "file" (group): creating temporary file ".file*"`, - `4: files: "file" (group): renaming "./.fileRND"`, + `4: files: "file" (group): renaming ".fileRND"`, }, nil, }, @@ -1174,7 +1174,7 @@ func TestSyncFile(t *testing.T) { `4: files: "file" (group): content differs`, `3: files: "file" (group): updating`, `4: files: "file" (group): creating temporary file ".file*"`, - `4: files: "file" (group): renaming "./.fileRND"`, + `4: files: "file" (group): renaming ".fileRND"`, }, nil, }, @@ -1734,7 +1734,7 @@ func TestSyncFile(t *testing.T) { `4: files: "path" (group): type differs L--------- -> ----------`, `3: files: "path" (group): updating`, `4: files: "path" (group): creating temporary file ".path*"`, - `4: files: "path" (group): renaming "./.pathRND"`, + `4: files: "path" (group): renaming ".pathRND"`, }, nil, }, @@ -1845,10 +1845,66 @@ func TestSyncFile(t *testing.T) { `3: files: "path" (group): updating`, `4: files: "path" (group): removing (due to type change)`, `4: files: "path" (group): creating temporary file ".path*"`, - `4: files: "path" (group): renaming "./.pathRND"`, + `4: files: "path" (group): renaming ".pathRND"`, }, nil, }, + { + "change: directory to file (non-empty)", + safcm.MsgSyncReq{}, + &safcm.File{ + Path: "path", + Mode: 0666, + Uid: -1, + Gid: -1, + OrigGroup: "group", + Data: []byte("content\n"), + }, + func() { + ft.CreateDirectory("path", 0777) + ft.CreateFile("path/file", "content\n", 0644) + }, + true, + []ft.File{ + root, + { + Path: "path", + Mode: fs.ModeDir | 0777, + }, + { + Path: "path/file", + Mode: 0644, + Data: []byte("content\n"), + }, + }, + safcm.MsgSyncResp{ + FileChanges: []safcm.FileChange{ + { + Path: "path", + Old: safcm.FileChangeInfo{ + Mode: fs.ModeDir | 0777, + User: user, + Uid: uid, + Group: group, + Gid: gid, + }, + New: safcm.FileChangeInfo{ + Mode: 0666, + User: user, + Uid: uid, + Group: group, + Gid: gid, + }, + }, + }, + }, + []string{ + `4: files: "path" (group): type differs d--------- -> ----------`, + `3: files: "path" (group): updating`, + `4: files: "path" (group): removing (due to type change)`, + }, + fmt.Errorf("will not replace non-empty directory, please remove manually"), + }, { "change: directory to symlink", @@ -1952,7 +2008,7 @@ func TestSyncFile(t *testing.T) { `4: files: "path" (group): type differs p--------- -> ----------`, `3: files: "path" (group): updating`, `4: files: "path" (group): creating temporary file ".path*"`, - `4: files: "path" (group): renaming "./.pathRND"`, + `4: files: "path" (group): renaming ".pathRND"`, }, nil, }, @@ -2116,6 +2172,41 @@ func TestSyncFile(t *testing.T) { nil, }, + // Symlink "attacks" + + { + "symlink in earlier path component", + safcm.MsgSyncReq{}, + &safcm.File{ + Path: "dir/file", + Mode: 0644, + Uid: -1, + Gid: -1, + OrigGroup: "group", + Data: []byte("content"), + }, + func() { + ft.CreateDirectory("tmp", 0755) + ft.CreateSymlink("dir", "tmp") + }, + false, + []ft.File{ + root, + { + Path: "dir", + Mode: fs.ModeSymlink | 0777, + Data: []byte("tmp"), + }, + { + Path: "tmp", + Mode: fs.ModeDir | 0755, + }, + }, + safcm.MsgSyncResp{}, + nil, + fmt.Errorf("symlink not permitted in path: \"dir\""), + }, + // Diffs { diff --git a/remote/sync/files_windows.go b/remote/sync/files_windows.go index 11252ef..b440ed6 100644 --- a/remote/sync/files_windows.go +++ b/remote/sync/files_windows.go @@ -29,6 +29,6 @@ func (s *Sync) syncFiles() error { return fmt.Errorf("not implemented on Windows") } -func OpenFileNoFollow(path string) (*os.File, error) { +func OpenFileNoSymlinks(path string) (*os.File, error) { return nil, fmt.Errorf("not implemented on Windows") } -- 2.45.2