Skip to content

security: gate IPC on peer credentials and require out-of-band destructive confirmation - #226

Open
megabyte0x wants to merge 7 commits into
tcballard:mainfrom
megabyte0x:fix/ipc-peer-cred-confirmation
Open

security: gate IPC on peer credentials and require out-of-band destructive confirmation#226
megabyte0x wants to merge 7 commits into
tcballard:mainfrom
megabyte0x:fix/ipc-peer-cred-confirmation

Conversation

@megabyte0x

Copy link
Copy Markdown
Contributor

Fixes #225.

The daemon accepted every IPC connection without checking peer credentials, and its two destructive commands were guarded by in-band constants — "ERASE" for panic_erase, the handle echoed back to itself for ClaimRegistryHandle. Any same-uid process that knew the socket path also knew the constants, so a one-line script could impersonate the account or destroy the master key and every sealed record. The socket was also chmodded after bind() rather than being published private.

What changed

Peer-credential gate. IpcServer::run reads SO_PEERCRED on every accepted connection and drops any peer whose uid differs from the daemon's effective uid before serve_client is spawned — no protocol bytes, not even an error frame. uid only: pid is subject to reuse races. Root is not exempted; a root peer can bypass the socket anyway, so accepting it would only widen the accepted-input surface. The decision is a pure function so the rejection branch is unit-testable.

Socket published private (finding 7). UnixListener::bind honours the process umask, so bind-then-chmod leaves a window whenever the umask is permissive — a manual launch without the packaged unit's UMask=0077. Overriding the umask around bind would close that window but is not safe: umask is process-wide and would strip bits from files and directories other threads create concurrently (this reproduces as EACCES on directories created by parallel tests). Instead the socket is bound inside a freshly created 0700 staging directory, tightened to 0600 there, and renamed into place. rename is atomic and preserves the inode and its mode, so the final path never exists world-accessible and clients connect to the same listening socket through it.

Out-of-band destructive confirmation. A new confirmation module mints 32 random bytes per request, writes them to a 0600 file inside a 0700 <state_dir>/confirmations/, and redeems them exactly once within 120 seconds. Redemption burns the outstanding token on every attempt, matched or not, so a wrong guess costs the token instead of enabling retries; claim tokens are bound to the exact handle. panic and claim-registry-handle now require such a token, and two new commands (request-panic-confirmation, request-registry-claim-confirmation) mint them.

IPC VERSION 1 → 2. Panic and ClaimRegistryHandle changed incompatibly, so a version-1 client's destructive flow is semantically broken and negotiation now says so instead of failing at use time. Every in-repo client and fixture moves in lockstep; the protocol has never shipped (0.0.1, no release).

Client UX is unchanged. omachat-ctl panic --confirm ERASE, claim-handle H --confirm H, and the TUI's /panic ERASE all still work. A shared request_with_confirmation helper validates the typed intent locally, requests a token, reads it out of band from the state directory, and commits. A mistyped intent is refused before anything reaches the daemon. panic also prints a warning that erasure is local-only and cannot retract replicated data.

Documentation. SECURITY.md and docs/installation.md now state plainly that these are deliberate-two-step and freshness guarantees, not an authorization boundary: a same-user process can ptrace the daemon or read the state directory holding the tokens, so protecting the account from hostile same-user code — including coding agents — requires OS-level sandboxing of that code, not daemon-side checks.

rustix (already a workspace dependency, used the same way by omachat-registry-host) provides geteuid because the workspace sets unsafe_code = "forbid".

Test plan

  • cargo fmt --all -- --check (clean)
  • cargo clippy --workspace --all-targets --locked -- -D warnings (clean)
  • cargo test --workspace --locked — 534 tests pass, including:
    • ipc_server::tests::only_the_daemon_uid_is_permitted (uid gate, incl. root rejection)
    • ipc_server::tests::the_published_socket_is_private_and_leaves_no_staging_directory
    • ipc_socket_mode::socket_is_published_private_even_under_a_permissive_umask — binds under umask 0, asserts 0600 at the final path, then completes a hello exchange through it to prove the rename did not break the listener
    • confirmation::tests::* — private single-use token file, burn-on-wrong-guess, expiry, handle binding
    • confirmation_ipc::* — issuance shape/mode, invalid handles mint nothing, "ERASE" no longer erases, a minted token authorizes exactly once
    • omachat-ctl confirmation_flow::* — two-phase orchestration against a scripted stub daemon, and local refusal of a mistyped intent
  • RUSTDOCFLAGS=-D warnings cargo doc --workspace --no-deps --locked, cargo build --workspace --bins --locked, ./scripts/check-version-contract.sh, ./scripts/test-check-release-size.sh, release build + ./scripts/check-release-size.sh (aggregate 4944 KiB of a 10 MiB ceiling), sh ./scripts/check-packaging.sh — all clean.

Cargo.lock gains no new packages; only two dependency edges to crates already in the graph.

UnixListener::bind honours the process umask, so binding at the final path
and chmodding afterwards leaves a window in which another uid can connect
whenever the umask is permissive (a manual launch without the packaged
unit's UMask=0077). Overriding the umask around bind is not safe here: it
is process-wide and would strip bits from files created concurrently by
other threads.

The socket is now bound inside a freshly created 0700 staging directory,
tightened to 0600 there, and renamed into place. rename is atomic and
preserves the inode and its mode, so the final path never exists in a
world-accessible state and clients connect to the same listening socket
through it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IPC grants full account control to any same-user process

1 participant