[RSDK-13582] Copying the binary to a temporary directory before calling it as the …#30
Conversation
SebastianMunozP
changed the title
run.sh
Outdated
| # Copy binary to /tmp so the console user can traverse the path. | ||
| # The module may be installed under /var/root/ (drwx------), which | ||
| # the console user cannot traverse even if they own the binary. | ||
| TMPBIN=$(mktemp /tmp/audio-module-XXXXXX) |
There was a problem hiding this comment.
[major] how is this bin ever cleaned up?
There was a problem hiding this comment.
It gets cleaned automatically by the OS every so often, for example on each reboot of the system.
For instance this is my current contents of /tmp:
ls -ltrh /tmp/
total 118800
drwxr-xr-x 2 root wheel 64B Mar 12 23:32 powerlog
drwx------ 3 sebastian.munoz wheel 96B Mar 12 23:32 com.apple.launchd.ov5gSWpIfH
drwx------ 3 sebastian.munoz wheel 96B Mar 12 23:32 com.apple.launchd.o26uSWy27i
drwxrwxrwx@ 2 root wheel 64B Mar 12 23:32 viam-module-1037151200
-rwxr-xr-x@ 1 root wheel 58M Mar 12 23:32 audio-module-tXfANV
srwxr-xr-x 1 sebastian.munoz wheel 0B Mar 12 23:33 zeb_def_ipc_1553
srwxr-xr-x 1 sebastian.munoz wheel 0B Mar 12 23:33 zeb_def_ipc_3976
drwxrwxrwx@ 5 root wheel 160B Mar 13 13:44 viam-module-2320196085
drwxr-xr-x@ 4 sebastian.munoz wheel 128B Mar 13 15:25 claude-501
There was a problem hiding this comment.
mktemp uses a random suffix to ensure it never collides with an existing file. Considering run.sh is the module's entrypoint, if the module is stuck in a restart loop, we will be non-stop writing unique files to tmp. Unless my understanding is wrong or incomplete, this is unacceptable
There was a problem hiding this comment.
let's use a consistent filename please, instead of mktemp
There was a problem hiding this comment.
That is a good point, hadn't thought of restart loops. Let me fix this.
There was a problem hiding this comment.
OK, now the file gets copied to a file name that consists only on viam-audio-module-+VIAM_MACHINE_PART_ID
This will avoid filling the disk on a restart loop
ls -ltrh /tmp/
total 237600
drwxr-xr-x 2 root wheel 64B Mar 12 23:32 powerlog
drwx------ 3 sebastian.munoz wheel 96B Mar 12 23:32 com.apple.launchd.ov5gSWpIfH
drwx------ 3 sebastian.munoz wheel 96B Mar 12 23:32 com.apple.launchd.o26uSWy27i
drwxrwxrwx@ 2 root wheel 64B Mar 12 23:32 viam-module-1037151200
-rwxr-xr-x@ 1 root wheel 58M Mar 12 23:32 audio-module-tXfANV
srwxr-xr-x 1 sebastian.munoz wheel 0B Mar 12 23:33 zeb_def_ipc_1553
srwxr-xr-x 1 sebastian.munoz wheel 0B Mar 12 23:33 zeb_def_ipc_3976
drwxr-xr-x@ 4 sebastian.munoz wheel 128B Mar 13 15:25 claude-501
-rwxr-xr-x@ 1 root wheel 58M Mar 13 16:53 viam-audio-module-d1dc4b55-e712-4ff0-9eb7-314a5c997c77
drwxrwxrwx@ 4 root wheel 128B Mar 13 16:53 viam-module-2320196085
There was a problem hiding this comment.
I had Gemini review this as a final check and it brought up an interesting point:
if the module is in a restart loop, or the OS hasn't fully reaped the old process yet, the OS holds a read-lock on /tmp/viam-audio-module-default. When cp tries to overwrite it, it will fail with Text file busy. The script will crash, and the module will fail to start.
How to actually fix it (The Atomic Swap)
If we must copy this to /tmp to bypass the /var/root traversal issue, they have to combine the safety of mktemp with the stability of a deterministic path using an atomic mv.
Replace lines 21-23 with this exact block:
# Define the deterministic target path
TARGET_BIN="/tmp/viam-audio-module-${VIAM_MACHINE_PART_ID:-default}"
# 1. Create a secure, unpredictable temp file to prevent symlink attacks
SAFE_TMP=$(mktemp /tmp/audio-module-XXXXXX)
# 2. Copy the binary to the secure temp file and set permissions
cp "$MODULE_BIN" "$SAFE_TMP"
chmod 755 "$SAFE_TMP"
# 3. Atomically move the temp file to the deterministic path.
# This replaces the inode directly, bypassing "Text file busy" locks
# from old running processes, and safely overwriting any malicious symlinks.
mv -f "$SAFE_TMP" "$TARGET_BIN"
# Execute
exec sudo -u "$CONSOLE_USER" "$TARGET_BIN" "$@"
This gives you the best of all worlds:
No infinite disk leaking (it always ends up at $TARGET_BIN).
No Text file busy crashes (because mv replaces the inode instead of modifying the locked file).
No symlink vulnerabilities (because mktemp creates a safe target, and mv destroys any existing symlinks at the destination).
There was a problem hiding this comment.
Unfortunately I have to log off. Is this urgent? I can approve if we make a high-ish priority ticket to make this less hacky.
There was a problem hiding this comment.
if the module is in a restart loop, or the OS hasn't fully reaped the old process yet,
Doesnt modmanager handle this for us? (we are confident we wont have concurrent system-audio processes)
There was a problem hiding this comment.
modmanager
Hm. If it handles this for us then, then I think the AI's point is a non-issue.
…e, this avoids the risk of filling the dist on a viam restart loop
3 participants
When viam-agent runs as root, it installs modules under /var/root/.viam/, which has drwx------ permissions. The previous approach of chown-ing the module directory to the console user didn't work because the console user still can't traverse /var/root/ itself — ownership of subdirectories doesn't help if the top-level directory blocks traversal.
Fix: Copy the binary to a unique path under /tmp (world-accessible) before re-executing as the console user via sudo -u. This sidesteps the traversal problem entirely.
This issue only manifests with viam-agent because it installs packages under root's home directory (/var/root/.viam/). When running viam-server directly as sudo, the module root is typically under the invoking user's home directory (e.g. /Users/you/.viam/), which the console user can traverse without issue.
Jira ticket: RSDK-13582