X-Git-Url: https://ruderich.org/simon/gitweb/?p=safcm%2Fsafcm.git;a=blobdiff_plain;f=remote%2Fsync%2Ffiles.go;h=edf81bd2cc795a2630f4b775eca25786ad838c61;hp=e3fc3d981a8c17c91f78a21bade00e3f5e607619;hb=2804606;hpb=a950980d2e302547743fb567d0636b766f4e9704 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