From 63a6a1c1305de5bd541fc74fd9d3c443bb837525 Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sat, 25 Oct 2025 12:22:40 +0200 Subject: [PATCH] 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. --- README.adoc | 2 +- cmd/safcm/main_sync_test.go | 10 ++++-- rpc/dial.go | 64 +++++++++++++++++++++---------------- 3 files changed, 46 insertions(+), 30 deletions(-) diff --git a/README.adoc b/README.adoc index 57e7125..aa17d98 100644 --- 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 ac1ace0..1adb5e1 100644 --- 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 3b6a956..49ce49e 100644 --- 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 -- 2.49.1