From cd40e998982037c27c7ee4450329b510369f3f66 Mon Sep 17 00:00:00 2001 From: Paavo Pokkinen Date: Wed, 23 Sep 2026 14:58:11 +0300 Subject: [PATCH] fix(daemon): serialize the umask swap around the socket bind CI fails now and then with "socket ... has insecure permissions 0o755". It hit run_embedded_exits_on_cancel on #14 and synchronous_startup_explicit_path_uses_parent_as_sandbox_root on #13. The socket gets its mode from the umask at bind time, and the umask is process-wide. synchronous_startup and four socket tests each did umask(077) / bind / umask(old) with no lock, and cargo test runs them on parallel threads. If one thread restores 022 between another thread's swap and its bind, that socket comes out 0755. The post-bind check then rightly refuses it. The swap now lives in bind_owner_only, behind a static mutex, and synchronous_startup and the tests both go through it. The lock is in production code, not a test-only mutex, so the integration test binaries that call synchronous_startup in parallel are covered too. The real daemon binds before any other thread exists, so it never waits on the lock. Stress run of `daemon::` lib tests, 200 iterations at 16 threads: 5 failures before, 0 after. --- CLAUDE.md | 2 +- src/daemon.rs | 48 ++++++++++++++++++++++++++---------------------- 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 470e820..67d6940 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -16,7 +16,7 @@ Read [README.md](README.md), [ARCHITECTURE.md](ARCHITECTURE.md), and [SECURITY.m - **`main()` is synchronous.** No `#[tokio::main]`. Daemonization forks; forking after tokio spawns runtime threads leaves them in undefined state in the child. The entire `synchronous_startup()` must complete before any tokio runtime exists. See [src/daemon.rs:14-19](src/daemon.rs#L14-L19). - **Double-fork with readiness pipe.** `daemon start` returns only after one of two things: the grandchild signals that it accepts connections, or `daemon start` prints the grandchild's startup error and exits non-zero. Every startup step in `async_main` that can fail runs before the readiness signal and must send its error through the pipe. It must never fail silently. See `daemonize` and `ReadinessPipe` in [src/daemon.rs](src/daemon.rs). -- **Trust boundary is the Unix socket.** The daemon is trusted; the client is not. Socket is mode `0700` and verified post-bind ([src/daemon.rs:416-435](src/daemon.rs#L416-L435)) — refuse to start if filesystem doesn't honor it. +- **Trust boundary is the Unix socket.** The daemon is trusted; the client is not. Socket is mode `0700` and verified post-bind ([src/daemon.rs:418-430](src/daemon.rs#L418-L430)) — refuse to start if filesystem doesn't honor it. - **Secrets are wrapped in `Secret`** ([src/secrets.rs](src/secrets.rs)) which zeroizes on drop and refuses to `Debug`-print. Never log a `Secret` value, never put one in a `format!`. - **Pre-exec closures must be async-signal-safe and zero-alloc.** The closure passed to `Command::pre_exec()` in [src/exec.rs](src/exec.rs) — no allocation, no mutex, no `println!`, only raw libc calls. Errors from pre-exec abort the spawn. - **Redaction is mandatory on the output path.** All bytes leaving the daemon to the client go through the Aho-Corasick redactor in [src/redact.rs](src/redact.rs), including base64 / URL-encoded / hex variants of secrets. diff --git a/src/daemon.rs b/src/daemon.rs index efa226d..956466a 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -418,15 +418,10 @@ pub fn synchronous_startup( // 6. Socket binding with restrictive permissions (owner-only). // Set umask to 0o077 so the socket is created with mode 0o700. // This prevents other local users from connecting to the daemon. - let old_umask = rustix::process::umask(rustix::fs::Mode::RWXG | rustix::fs::Mode::RWXO); - let bind_result = - unix_net::UnixListener::bind(&config.socket_path).map_err(|e| DaemonError::SocketBind { - path: config.socket_path.clone(), - source: e, - }); - // Restore the original umask immediately, even if bind failed. - rustix::process::umask(old_umask); - let listener = bind_result?; + let listener = bind_owner_only(&config.socket_path).map_err(|e| DaemonError::SocketBind { + path: config.socket_path.clone(), + source: e, + })?; // 7. Verify socket permissions are owner-only. // Bail out immediately if the filesystem didn't honor the umask — we @@ -512,6 +507,24 @@ pub fn remove_runtime_files<'a>( .collect() } +/// Serializes the umask swap in [`bind_owner_only`]. +static UMASK_LOCK: Mutex<()> = Mutex::new(()); + +/// Bind a Unix socket at `path` with mode `0700`. +/// +/// A socket's mode comes from the umask at `bind` time, and the umask is +/// process-wide. The daemon binds before any other thread exists, but tests +/// bind from parallel threads. Without the lock, one thread could restore a +/// permissive umask between another thread's swap and its `bind`. +fn bind_owner_only(path: &Path) -> std::io::Result { + let _guard = UMASK_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let old_umask = rustix::process::umask(rustix::fs::Mode::RWXG | rustix::fs::Mode::RWXO); + let result = unix_net::UnixListener::bind(path); + // Restore the original umask even if bind failed. + rustix::process::umask(old_umask); + result +} + /// Verify the socket file has owner-only permissions. /// /// Refuses to proceed if other users have any access. This is not something @@ -2354,10 +2367,7 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); let socket_path = tmp.path().join("test.sock"); - // Reproduce the same umask pattern used in synchronous_startup(). - let old_umask = rustix::process::umask(rustix::fs::Mode::RWXG | rustix::fs::Mode::RWXO); - let _listener = unix_net::UnixListener::bind(&socket_path).unwrap(); - rustix::process::umask(old_umask); + let _listener = bind_owner_only(&socket_path).unwrap(); let metadata = std::fs::symlink_metadata(&socket_path).unwrap(); let mode = metadata.permissions().mode() & 0o777; @@ -2374,9 +2384,7 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); let socket_path = tmp.path().join("good.sock"); - let old_umask = rustix::process::umask(rustix::fs::Mode::RWXG | rustix::fs::Mode::RWXO); - let _listener = unix_net::UnixListener::bind(&socket_path).unwrap(); - rustix::process::umask(old_umask); + let _listener = bind_owner_only(&socket_path).unwrap(); let result = verify_socket_permissions(&socket_path); assert!(result.is_ok(), "owner-only socket should pass: {result:?}"); @@ -2390,9 +2398,7 @@ mod tests { let socket_path = tmp.path().join("bad.sock"); // Create socket then widen permissions to simulate a bad filesystem. - let old_umask = rustix::process::umask(rustix::fs::Mode::RWXG | rustix::fs::Mode::RWXO); - let _listener = unix_net::UnixListener::bind(&socket_path).unwrap(); - rustix::process::umask(old_umask); + let _listener = bind_owner_only(&socket_path).unwrap(); std::fs::set_permissions(&socket_path, std::fs::Permissions::from_mode(0o750)).unwrap(); @@ -2419,9 +2425,7 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); let socket_path = tmp.path().join("bad.sock"); - let old_umask = rustix::process::umask(rustix::fs::Mode::RWXG | rustix::fs::Mode::RWXO); - let _listener = unix_net::UnixListener::bind(&socket_path).unwrap(); - rustix::process::umask(old_umask); + let _listener = bind_owner_only(&socket_path).unwrap(); std::fs::set_permissions(&socket_path, std::fs::Permissions::from_mode(0o755)).unwrap();