remote: use better approach to upload remote helper

This change also prevents a race condition when the remote helper is run
concurrently. An attacker could use the small window when the old helper
gets removed to create a file which is then executed by the concurrent
run.

A few other approaches were considered. The main issue is the
uncertainty in the behavior of command line tools like `mv`. The system
calls have well defined behavior but it's not clear how exactly the
tools are implemented. The goal was to find an approach which is secure
even under these circumstances without having to validate them across
multiple operating systems.

diff --git a/README.adoc b/README.adoc
index 57e7125b484d1347fb64e1b67f5a66534a67acf1..aa17d986857fc26f690fbb5870551fea46966f57 100644 (file)
--- a/README.adoc
+++ b/README.adoc
@@ -138,7 +138,7 @@ future, others are due to the design of safcm.
   * Go support for architecture and operating system
   * Supported operating system:
     ** GNU/Linux with common commands (`uname`, `id`, `stat`, `sha512sum`,
-       `cat`, `mktemp`, `rm`, `ln`, `chmod`)
+       `cat`, `mkdir`, `mv`, `rm`, `chmod`, `chgrp`)
     ** FreeBSD (same commands, but uses `sha512`)
     ** OpenBSD (same commands, but uses `sha512`)
   * SSH server

diff --git a/cmd/safcm/main_sync_test.go b/cmd/safcm/main_sync_test.go
index ac1ace020d5c11521b7d9e0320ec856e56336163..1adb5e1ca1a9fe9e10c75e0d0d332a350dbc662e 100644 (file)
--- a/cmd/safcm/main_sync_test.go
+++ b/cmd/safcm/main_sync_test.go
@@ -347,7 +347,10 @@ executed 2 command(s):
        for _, tc := range tests {
                t.Run(tc.name, func(t *testing.T) {
                        if tc.remove {
-                               _ = os.Remove(remotePath)
+                               err := os.RemoveAll(remotePath)
+                               if err != nil {
+                                       t.Fatal(err)
+                               }
                        }
 
                        args := append([]string{"sync",
@@ -371,7 +374,10 @@ executed 2 command(s):
                })
        }
 
-       _ = os.Remove(remotePath)
+       err = os.RemoveAll(remotePath)
+       if err != nil {
+               t.Fatal(err)
+       }
 }
 
 // vi: set noet ts=4 sw=4 sts=4:

diff --git a/rpc/dial.go b/rpc/dial.go
index 3b6a956a2ca7746105fddbab52ad8781c5855383..49ce49eae3cf51faf7214c85bb1d42e7d8124764 100644 (file)
--- a/rpc/dial.go
+++ b/rpc/dial.go
@@ -102,7 +102,9 @@ func (c *Conn) dialSSH(stdin io.Writer, stdout_ io.Reader) error {
                return err
        }
 
-       path := fmt.Sprintf("/tmp/safcm-remote-%d", uid)
+       base := "/tmp"
+       dir := fmt.Sprintf("%s/safcm-remote-%d", base, uid)
+       path := dir + "/helper"
 
        c.debugf("DialSSH: probing remote at %q", path)
 
@@ -111,8 +113,8 @@ func (c *Conn) dialSSH(stdin io.Writer, stdout_ io.Reader) error {
        switch goos {
        case "linux":
                compat = `
-dir_stat='drwxrwxrwt 0 0'
-file_stat="-rwx------ $(id -u) $(id -g)"
+base_stat='drwxrwxrwt 0 0'
+dir_stat="drwx------ $(id -u) $(id -g)"
 compat_stat() {
        stat -c '%A %u %g' "$1"
 }
@@ -122,8 +124,8 @@ compat_sha512sum() {
 `
        case "freebsd", "openbsd":
                compat = `
-dir_stat='41777 0 0'
-file_stat="100700 $(id -u) $(id -g)"
+base_stat='41777 0 0'
+dir_stat="40700 $(id -u) $(id -g)"
 compat_stat() {
        stat -f '%p %u %g' "$1"
 }
@@ -145,25 +147,37 @@ compat_sha512sum() {
        // is guaranteed by the sticky bit. The code verifies the directory has
        // the proper permissions.
        //
-       // We cannot use `test -f && test -O` because this is open to TOCTOU
-       // attacks. `stat` gives use the full file state. If the file is owned by
-       // us and not a symlink then it's safe to use (assuming sticky directory
-       // or directory not writable by others).
-       //
-       // `test -e` is only used to prevent error messages if the file doesn't
-       // exist. It does not guard against any races.
+       // We need to guard against TOCTOU races as well as concurrent runs of
+       // this function. The simplest way is to securely create a directory which
+       // can only be written by the current user. Then a temporary file can be
+       // used for uploads which is atomically renamed over the current file.
        _, err = fmt.Fprintf(stdin, `
 %s
 f() {
+       base=%q
+       dir=%q
        x=%q
 
-       dir="$(dirname "$x")"
+       if ! test "$(compat_stat "$base")" = "$base_stat"; then
+               echo "unsafe permissions on $base, aborting" >&2
+               exit 1
+       fi
+
+       # Migrate from old file-based approach
+       rm -f "$dir" 2>/dev/null || true
+
+       if mkdir -m 0700 "$dir" 2>/dev/null; then
+               # Some BSD create files with group wheel in /tmp
+               chgrp "$(id -g)" "$dir"
+       fi
        if ! test "$(compat_stat "$dir")" = "$dir_stat"; then
-               echo "unsafe permissions on $dir, aborting" >&2
+               echo "invalid permissions on $dir, aborting" >&2
+               echo "(fix by removing it manually while no sync takes place)" >&2
                exit 1
        fi
+       # Now we're safe. Everything in $dir cannot be controlled by an attacker.
 
-       if test -e "$x" && test "$(compat_stat "$x")" = "$file_stat"; then
+       if test -e "$x"; then
                # Report checksum
                compat_sha512sum "$x"
        else
@@ -175,26 +189,23 @@ f() {
        read upload
 
        if test -n "$upload"; then
-               tmp="$(mktemp "$x.XXXXXX")"
+               # No need for mktemp, directory not attacker-controlled
+               tmp="$x.$$"
                # Report filename for upload
                echo "$tmp"
                # Wait for upload to complete
                read unused
 
-               # Safely create new file (ln does not follow symlinks)
-               rm -f "$x"
-               ln "$tmp" "$x"
-               rm "$tmp"
                # Make file executable
-               chmod 0700 "$x"
-               # Some BSD create files with group wheel in /tmp
-               chgrp "$(id -g)" "$x"
+               chmod 0700 "$tmp"
+               # Atomically update file
+               mv "$tmp" "$x"
        fi
 
        exec "$x" sync
 }
 f
-`, compat, path)
+`, compat, base, dir, path)
        if err != nil {
                return err
        }
@@ -245,9 +256,8 @@ f
                // Get path to temporary file for upload.
                //
                // Write to the temporary file instead of the final path so that a
-               // concurrent run of this function won't use a partially written file.
-               // The rm in the script could still cause a missing file but at least
-               // no file with unknown content is executed.
+               // concurrent run of this function won't execute a partially written
+               // file.
                path, err := stdout.ReadString('\n')
                if err != nil {
                        return err