]> ruderich.org/simon Gitweb - safcm/safcm.git/blobdiff - remote/sync/files.go
remote: guard against symlinks in earlier path components
[safcm/safcm.git] / remote / sync / files.go
index e3fc3d981a8c17c91f78a21bade00e3f5e607619..edf81bd2cc795a2630f4b775eca25786ad838c61 100644 (file)
@@ -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