From cd15a01994c3637be366fef1f54790a6d36128b7 Mon Sep 17 00:00:00 2001 From: Cuihtlauac ALVARADO Date: Wed, 30 Sep 2026 13:20:07 +0200 Subject: [PATCH] =?UTF-8?q?feat(security):=20add=20invariant=20G7=20?= =?UTF-8?q?=E2=80=94=20no=20unprivileged=20command=20weaker=20than=20the?= =?UTF-8?q?=20Bash=20tool?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sudo-proxy verified one invariant (G1: nothing privileged runs without a human approving the exact command). The unprivileged surface was weaker than the agent's own Bash tool in two ways; this adds a second invariant (G7) and closes both gaps. Gap 1 (F2): pressing `a` at an unprivileged prompt persisted a global `confirm_unprivileged=false`, after which unprivileged commands ran unattended with only a best-effort banner. Replaced with a two-barrier, session-scoped model: - `unattended_eligible` (per-daemon policy, default false, read once at startup and immutable at runtime — no wire field/MCP flag/keypress sets it); and - an in-memory session grant flipped only by `a` when eligible, never persisted, reset when the daemon/SSH tunnel ends. Non-eligible daemons prompt every command; a stale `confirm_unprivileged` key is inert on load. The granted window logs unconditionally (reliable audit). Gap 2 (F5, new): host routing keyed on the literal string "localhost", so `execute(host="127.0.0.1")` opened an SSH-to-self tunnel that bypassed local policy. Added `is_local_host` normalization (loopback /8, ::1, own hostname, user@/trailing-dot forms) at every routing site. Delegate-to-Bash: a local unprivileged `execute()` is refused and redirected to the Bash tool (which already applies the client's rules); sudo-proxy handles privilege escalation and remote hosts. Soundness for undetected self-aliases rests on the composition (C10): every remote daemon still gates unattended execution. Verification: - Rung 2 property/dispatch tests (is_local_host truth table, two barriers, non-persistence, migration inertness, granted-session bypass); rewrote the tests that encoded the old ApprovedAlways behavior. - Rung 4 TLA+ (ApprovalStateMachine): `eligible` immutable input + session `grant` with boundary reset; new properties GrantOnlyByKeypressWhenEligible, NoUnattendedUnprivilegedExec, EligibilityImmutable; TLC-verified with new negative controls NC5/NC6/NC7. - Docs threaded: F2 closed + new F5 in security-audit; REVIEWING C8/C9/C10; assurance-case G7; threat-model, roadmap, README, usage, mcp, architecture. BREAKING: removes `--no-confirm-unprivileged` / `--confirm-unprivileged` (replaced by `--unattended-eligible`); renames the hosts.json policy field `confirm_unprivileged` -> `unattended_eligible` (old value ignored, fail-closed); local unprivileged `execute()` now refused (use the Bash tool). Co-Authored-By: Claude Opus 4.8 (1M context) --- Cargo.lock | 2 +- Cargo.toml | 2 +- README.md | 10 + REVIEWING.md | 71 ++++++-- docs/architecture.md | 13 +- docs/assurance-case.md | 32 +++- docs/formalisation-roadmap.md | 17 +- docs/mcp.md | 7 + docs/security-audit.md | 92 +++++++--- docs/threat-model.md | 18 +- docs/usage.md | 47 +++-- proofs/tla/ApprovalStateMachine.cfg | 5 +- proofs/tla/ApprovalStateMachine.tla | 272 +++++++++++++++++----------- proofs/tla/ConcurrentHandlers.tla | 5 +- proofs/tla/README.md | 54 +++--- src/bin/sudo-proxy.rs | 34 ++-- src/gui.rs | 16 +- src/hosts.rs | 64 ++++--- src/mcp.rs | 86 ++++++++- src/server.rs | 177 +++++++++++++++--- src/tui.rs | 71 ++++++-- tests/approval.rs | 91 ++++++---- tests/common/mod.rs | 33 +++- tests/concurrency.rs | 35 ++-- tests/forward_agent.rs | 1 - tests/hosts.rs | 2 +- tests/slow.rs | 22 ++- tests/transport.rs | 2 +- 28 files changed, 904 insertions(+), 377 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 079b44f..5d7c140 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -573,7 +573,7 @@ checksum = "7da8b5736845d9f2fcb837ea5d9e2628564b3b043a70948a3f0b778838c5fb4f" [[package]] name = "sudo-proxy" -version = "1.1.0" +version = "1.2.0" dependencies = [ "base64", "libc", diff --git a/Cargo.toml b/Cargo.toml index cddcd6e..46bb8d1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "sudo-proxy" -version = "1.1.0" +version = "1.2.0" edition = "2021" license = "MIT" description = "Privileged command execution proxy with human approval via pkexec or sudo" diff --git a/README.md b/README.md index 2ce649b..7fe9950 100644 --- a/README.md +++ b/README.md @@ -53,6 +53,16 @@ system files, manage services, or run any other command — with the human always in the loop, even when Claude Code is run with `--dangerously-skip-permissions`. +It is deliberately **not** a way to run *un*privileged commands with less +scrutiny than the Bash tool: an unprivileged command targeting the local +machine is refused and delegated back to the Bash tool (which already applies +your permission rules), and loopback aliases like `127.0.0.1` cannot dodge that. +sudo-proxy handles privilege escalation and commands on remote hosts; there, +unprivileged commands are gated the same way, and the per-command gate is only +relaxed if you deliberately mark a host eligible in `hosts.json` *and* confirm +once per session (a grant that is never persisted). See +[REVIEWING.md](REVIEWING.md) invariant **G7**. + For how this relates to mcp-firewall, sandboxing, polkit, doas, and other neighboring tools, see [docs/comparison.md](docs/comparison.md). diff --git a/REVIEWING.md b/REVIEWING.md index ad75f61..531f58f 100644 --- a/REVIEWING.md +++ b/REVIEWING.md @@ -9,15 +9,24 @@ code that has to hold it up, and — most importantly — here is where we have If you find a way to break any numbered claim below, that is a security finding; see [SECURITY.md](SECURITY.md) for where to send it. -## The one claim +## The claims -Everything reduces to a single invariant: +The privileged surface reduces to a single invariant (**G1**): > **Nothing privileged runs without a human deliberately approving the exact > command shown.** -We would rather you try to falsify that than "review the project." Below it is -broken into concrete, falsifiable sub-claims. +A second invariant (**G7**) guards the *unprivileged* surface — the part that +was historically weaker than the agent's own Bash tool: + +> **No unprivileged command runs with less human scrutiny than the agent's Bash +> tool would apply: every unprivileged command faces a live human gate unless an +> operator has *out-of-band* made its host eligible AND a human gave a +> *session-scoped* confirmation; and a self/loopback target cannot route around +> that gate by naming an alias of "localhost".** + +We would rather you try to falsify these than "review the project." Below they +are broken into concrete, falsifiable sub-claims (C1–C7 for G1, C8–C10 for G7). ## Falsify one of these @@ -39,10 +48,14 @@ finding. deduplicated and every request must carry a `time` within 60 s. *Attack:* cause the same approval to authorise two executions, or make a stale request pass. → `src/server.rs` (`check_freshness` `:58`, `try_insert` `:213`). -- **C4 — Policy flips only on a keypress.** The `confirm_unprivileged` policy - flag can be changed *only* by an interactive keypress — never by a request - field, MCP tool flag, or replay. *Attack:* flip it from the wire. → - `src/tui.rs` (`classify_key`), `src/server.rs`. +- **C4 — Unattended mode can't be enabled from the wire.** A daemon becomes + eligible for unattended unprivileged execution *only* via an out-of-band edit + of `hosts.json` (`policy.unattended_eligible`), which is read once at startup + and never mutated at runtime; and the session grant flips *only* on an + interactive `a` keypress, and *only* when eligible. No request field, MCP tool + flag, or replay can enable either barrier. *Attack:* enable unattended mode, or + flip the session grant, from the wire. → `src/hosts.rs` (`Policy`), + `src/tui.rs` (`classify_key`), `src/server.rs` (dispatch). - **C5 — No stored credential.** sudo-proxy never stores or caches a secret; authentication is owned entirely by `sudo`/`pkexec`. *Attack:* find any path where sudo-proxy holds, caches, or replays a credential. → `src/executor.rs`. @@ -58,6 +71,34 @@ finding. without approval. → `src/server.rs` (`peer_uid`/`SO_PEERCRED` `:244`, `handle_connection`). +The G7 sub-claims (unprivileged surface): + +- **C8 — No unattended unprivileged exec except behind two human acts; the grant + never persists.** No unprivileged command reaches `exec_direct` without a + per-command keypress *unless* (a) its daemon is `unattended_eligible` (barrier + 1, config-only) *and* (b) a human answered `a` this session (barrier 2). The + grant lives only in memory, is never written to `hosts.json`, and dies with the + daemon/tunnel. *Attack:* reach `exec_direct` unattended on a non-eligible + daemon; make an `a` press grant a session without eligibility; make the grant + survive a session/tunnel boundary or a restart; or make a stale + `confirm_unprivileged` key re-enable it. → `src/server.rs` (dispatch), + `src/hosts.rs` (`Policy`, migration), `tests/approval.rs`. +- **C9 — Self/loopback can't dodge local policy.** A request naming any loopback + alias of the daemon's own machine (`localhost`, `127.0.0.0/8`, `::1`, + `localhost.`, the machine's own hostname) routes to the *local* path, not an + SSH tunnel, and a local unprivileged command is refused (delegated to the Bash + tool). *Attack:* make `execute(host="127.0.0.1")` (or `::1`, or the own + hostname) open an SSH-to-self tunnel, or slip a local unprivileged command past + the Bash-delegation refusal. → `src/server.rs` (`is_local_host`), `src/mcp.rs` + (`normalize_host`, `execute`). +- **C10 — Backstop composition (soundness).** `is_local_host` is best-effort, so + an *undetectable* self-alias (ssh-config alias, NAT hairpin) may still route + SSH-to-self. That cannot yield unattended exec, because C8 holds on *every* + daemon: the tunnel lands on a daemon whose only unattended path is the + eligible + session-confirmed grant. *Attack:* find a host string that is really + the local box, escapes C9, *and* runs unprivileged unattended. → composition of + `src/mcp.rs` routing and `src/server.rs` dispatch. + ## The trust boundary (what to actually read) The security-critical path is four files; almost everything else (the MCP @@ -104,9 +145,17 @@ what carries **no proof**, roughly in order of how much it worries us: 5. **`base64` / `serde_json` decoding.** Panic-freedom of the decode paths rests on the upstream crates' `Result`-returning APIs and our `.unwrap()`-free call sites — an assumption, not a proof. -6. **Auto-approve, if ever enabled.** The "remember this command" surface (audit - finding F2) is off by design; the moment it exists, prefix-matching escapes - become live. → `docs/architecture.md` allowlisting note. +6. **The unattended window on an eligible daemon.** The old persisted, global + auto-approve (finding F2) is gone. What remains is bounded: on a daemon an + operator has *deliberately* made `unattended_eligible`, one `a` keypress opens + a session-scoped, non-persistent window in which unprivileged commands run + with only an audit-log line. Inside that window the gate is the operator's + two prior decisions (the config edit and the `a` press) plus the audit log — + there is no per-command human check. We judge this at Bash parity (a Bash + allow-rule is a similar, and less bounded, operator opt-in), but the window is + real: an operator who enables eligibility and presses `a` on a hostile-looking + command is not protected by the code. → `src/server.rs` dispatch, finding F2 + in `docs/security-audit.md`. ## Run it in a container diff --git a/docs/architecture.md b/docs/architecture.md index 92cb63a..8b29519 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -30,11 +30,14 @@ Functional but minimal. - TUI approval prompt + sudo for privilege escalation (local and remote) - Non-privileged mode (direct execution, no escalation) — also TUI-gated by default - `--verbose` / `-v` on server: prints startup info, logs each request -- Per-host policy in `hosts.json`: pressing `a` at an unprivileged prompt - writes `policy.confirm_unprivileged=false` so the daemon skips the gate - on this host from then on -- `--no-confirm-unprivileged` / `--confirm-unprivileged` on server: - explicit overrides of the persisted policy +- Per-daemon policy in `hosts.json` (`policy.unattended_eligible`, default + false): whether the daemon may offer the session-scoped `a` answer. Read once + at startup, immutable at runtime — an out-of-band operator opt-in (invariant + G7, barrier 1). Pressing `a` on an eligible daemon flips an in-memory, + never-persisted session grant (barrier 2); on a non-eligible daemon `a` + approves just the one command +- `--unattended-eligible` on server: makes the daemon eligible without editing + the file (the session grant still needs the `a` keypress and is never persisted) - `--no-privilege` on client: sends request with `privileged: false` - `--host` flag on server: SSHs into remote, starts sudo-proxy, tunnels socket (used by MCP `start_server`) - `--print` mode for human-readable output on stdout diff --git a/docs/assurance-case.md b/docs/assurance-case.md index 0d6c00d..95a9629 100644 --- a/docs/assurance-case.md +++ b/docs/assurance-case.md @@ -37,6 +37,10 @@ asserted, evidence not yet produced). > **G1 — Nothing privileged runs without a human deliberately approving the > exact command shown.** +> A second, parallel top-level invariant **G7** (below) guards the *unprivileged* +> surface: no unprivileged command runs with less scrutiny than the agent's Bash +> tool. G2–G6 decompose G1; G7 stands beside it with its own sub-goals. + ``` ┌──────────────────────────────────────┐ C1 TOE: daemon + MCP server │ G1 No privileged execution without │ @@ -124,8 +128,8 @@ keypress binds to the command shown.* | **Sn5.1** | TLC-checked: the `PolicyFlipsOnlyOnKeypress` invariant of [`proofs/tla/`](../proofs/tla/) proves the policy flag flips only via an interactive `a` keypress on an unprivileged request — never a request field, replay, MCP flag, or timeout — over all attacker forgeries/replays and operator choices. `src/server.rs`. | 4 | [discharged] (model-checked) | | **G5.2** | The prompt reads a single keypress in non-canonical mode and times out after 60 s (default **deny**). | — | | | **Sn5.2** | `src/tui.rs` prompt; timeout test. | 1 | [discharged] | -| **G5.3** | The `confirm_unprivileged=false` policy relaxes only the **non-privileged** gate, never the privileged one, and only via an interactive `a` keypress. | — | | -| **Sn5.3** | TLC-checked: `NoExecWithoutApproval` + `PrivilegedGateIndependentOfPolicy` ([`proofs/tla/`](../proofs/tla/)) prove the privileged gate requires a `y` keypress for *any* value the policy flag took, so `confirm_unprivileged` relaxes only the non-privileged gate. Audit finding **F2** by-design trade-off still documented; `display_banner` reliability is a separate backlog item. | 4 | [discharged] (model-checked) | +| **G5.3** | No policy value relaxes the **privileged** gate: `privileged:true` requires a `y` keypress regardless of `unattended_eligible` or the session grant. | — | | +| **Sn5.3** | TLC-checked: `PrivilegedGateIndependentOfPolicy` ([`proofs/tla/`](../proofs/tla/)) proves the privileged gate requires a `y` keypress for *any* eligibility/grant state. The *unprivileged* side (eligibility + session grant) is the second invariant **G7** below. | 4 | [discharged] (model-checked) | ### G6 — Adversary cannot bypass the gate @@ -142,6 +146,21 @@ keypress binds to the command shown.* | **G6.4** | Resource exhaustion cannot force-open the gate: 1 MiB request cap, 64 in-flight, 16 MiB output cap. | — | | | **Sn6.4** | `src/server.rs`, `src/executor.rs`; cap test (flaky **S2**, control sound). | 1–2 | [partial] | +### G7 — No unprivileged command runs with less scrutiny than the Bash tool + +*Every unprivileged command faces a live human gate unless an operator made its +host eligible out-of-band AND a human confirmed the session; and a self/loopback +target cannot route around that gate.* + +| Node | Claim / Evidence | Rung | Status | +|------|------------------|------|--------| +| **G7.1** | No unprivileged command runs unattended except behind two barriers — `unattended_eligible` (config-only, immutable at runtime) AND an in-session `a` keypress — and the grant is never persisted. | — | | +| **Sn7.1** | `tests/approval.rs` (`every_unprivileged_request_is_prompted_when_not_eligible`, `non_eligible_approved_always_grants_nothing`, `eligible_approved_always_grants_session_but_never_persists`); `src/hosts.rs` (`stale_confirm_unprivileged_key_is_inert`). TLC: `NoUnattendedUnprivilegedExec` ([`proofs/tla/`](../proofs/tla/)). Closes audit **F2**. | 2, 4 | [discharged] | +| **G7.2** | No persisted policy value enables unattended execution on load (fail-closed default; stale `confirm_unprivileged` inert). | — | | +| **Sn7.2** | `src/hosts.rs` serde tests; `Policy::default` is `unattended_eligible=false`. | 2 | [discharged] | +| **G7.3** | A self/loopback target is classified local and cannot route SSH-to-self around local policy; local unprivileged is delegated to the Bash tool. Soundness of undetected aliases rests on G7.1 holding on every daemon (C10). | — | | +| **Sn7.3** | `src/server.rs` (`is_local_host_classifies_self_and_loopback`), `src/mcp.rs` (`normalize_host_folds_loopback_to_local`). Closes audit **F5**. | 2 | [discharged] | + ## Open items tracked against the case These are the leaves where the argument is currently weakest, drawn from @@ -158,9 +177,12 @@ These are the leaves where the argument is currently weakest, drawn from authenticity to client auth. - **Sn6.4 / S2** — stabilise the flaky resource-cap test so the cap stays covered in CI. -- **Sn4.4 / Sn5.3** — accepted residual risks (look-alike `reason`, the - `confirm_unprivileged` trade-off); revisit if an auto-approve surface is ever - added (see the [allowlisting note](architecture.md#design-note-allowlisting-when-it-lands)). +- **Sn4.4** — accepted residual risk (look-alike `reason`). +- **G7.1 residual** — the bounded unattended window on an eligible daemon after an + `a` keypress (audit **F2**, now closed but with a characterised residual); + judged at Bash-allow-rule parity. Revisit if a command allowlist / auto-approve + surface is ever added (see the + [allowlisting note](architecture.md#design-note-allowlisting-when-it-lands)). ## How to use this document diff --git a/docs/formalisation-roadmap.md b/docs/formalisation-roadmap.md index 7562e94..2dc1af5 100644 --- a/docs/formalisation-roadmap.md +++ b/docs/formalisation-roadmap.md @@ -126,8 +126,11 @@ specification: - `shell_escape` round-trips through `/bin/sh` byte-for-byte; - every field displayed at the approval prompt is dangerous-char-free; - `privileged:true` ⇒ an interactive keypress occurred before exec; -- the `confirm_unprivileged` policy flag is flippable *only* by an interactive - keypress, never by a request field. +- unattended unprivileged execution requires both `unattended_eligible` (config- + only, never set from the wire) and an in-session `a` keypress, and the grant is + never persisted (invariant G7 / C8); a stale `confirm_unprivileged` key is inert; +- self/loopback host targets are classified local and cannot route SSH-to-self + (G7 / C9), tested by `is_local_host`'s truth table. This is the cheap bridge to formal methods: these properties become the proof obligations for the higher rungs. @@ -209,10 +212,12 @@ half with a stable-Rust typestate (rationale below). The most interesting properties are temporal and relational, not per-function: - Model the **approval state machine** — request → freshness check → dedup → - prompt → keypress → exec, plus the `confirm_unprivileged` policy transition - (finding F2) — in **TLA+/PlusCal** or **Alloy**, and model-check: replay is - impossible; no exec without approval; the policy flag transitions *only* on an - interactive keypress (never via a request field, replay, or MCP tool flag). + prompt → keypress → exec, plus the unprivileged eligibility + session-grant + transitions (invariant G7, finding F2) — in **TLA+/PlusCal** or **Alloy**, and + model-check: replay is impossible; no exec without approval; eligibility is a + runtime-immutable input; and `NoUnattendedUnprivilegedExec` — an unprivileged + command runs unattended only when eligible AND a prior in-session `a` keypress + occurred, with the grant reset at the session boundary (never persisted). - For the A3 / SSH-tunnel path, model freshness + replay + channel assumptions in the symbolic-protocol provers **Tamarin** or **ProVerif**. These are the standard instruments for "what does the tunnel actually guarantee," and they diff --git a/docs/mcp.md b/docs/mcp.md index d0f25e8..5642c63 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -26,6 +26,13 @@ sudo-proxy as tools over stdio JSON-RPC. Any MCP-capable AI client - `privileged`: whether to escalate privileges (default `true`). - `env`: environment variables to pass. +> **Local unprivileged commands are refused.** A request with `privileged: false` +> that targets the local machine (including loopback aliases such as `127.0.0.1`, +> `::1`, or this host's own name) is declined with a message pointing the agent +> back to its Bash tool, which already applies the client's permission rules +> (invariant G7). sudo-proxy runs privileged commands (local and remote) and +> unprivileged commands on remote hosts. + **`update_host`** — record metadata about a known host. - `host` (required): hostname to update. - `description`: human-readable description (e.g. "CI server"). diff --git a/docs/security-audit.md b/docs/security-audit.md index 1182b77..8f4ba04 100644 --- a/docs/security-audit.md +++ b/docs/security-audit.md @@ -45,16 +45,17 @@ for exactly this reason); the fix re-applies that existing control consistently. | ID | Severity | Title | Attacker | Status | |----|----------|-------|----------|--------| | F1 | **High** | Approval-prompt ANSI/control-char injection via unvalidated display fields | A1, A2 | **Fixed** | -| F2 | Low | `confirm_unprivileged=false` runs later commands with only a best-effort banner | A1, A2 | Accepted-risk / doc | +| F2 | Low | Persisted global `confirm_unprivileged=false` runs later commands unattended with only a best-effort banner | A1, A2 | **Fixed** (G7) | | F3 | Low | Approval prompt has no length bound; huge argv can push the command off-screen | A1 | **Fixed** | | F4 | Low | `hosts.json` created world-readable (0644); leaks host inventory/UIDs/policy | A2 | **Fixed** | +| F5 | Low–Med | Self/loopback host string (`127.0.0.1`, `::1`, own hostname) routes SSH-to-self, bypassing local policy and Bash delegation | A1, A2 | **Fixed** (G7) | | S1 | Low | Signal handler calls `CString::new` (malloc) — not async-signal-safe | local | **Fixed** | | S2 | Info | `burst_connections_above_cap` test is timing-flaky (~2/3 fail) | — | Note (open) | -**Fixes applied (this audit):** F1, F3, F4, S1 — each with a regression test; -`cargo build --release --features mcp` and the full suite pass (the only failing -test is the pre-existing, unrelated flaky S2). F2 and S2 remain open as a -documentation change and a test-stabilization task respectively. +**Fixes applied (this audit):** F1, F3, F4, S1. **Fixed later under invariant G7** +(the second invariant — "no unprivileged command runs with less scrutiny than the +Bash tool"): F2 and F5, each with regression tests. Only S2 (the pre-existing, +unrelated flaky burst test) remains open. --- @@ -120,25 +121,36 @@ Regression test mirrors the existing argv bidi/control tests in `tests/validatio --- -### F2 — `confirm_unprivileged=false` silent execution (Low, by-design) - -**Location:** `src/server.rs:631-662`, `src/hosts.rs` (policy persistence). - -Once a human presses `a` at an unprivileged prompt, `policy.confirm_unprivileged` is -set `false` and persisted; subsequent **unprivileged** requests skip the Y/N gate and -run after only a best-effort banner (`display_banner`, printed under a `try_lock`, so -it may not appear under TTY contention). An "unprivileged" command still runs arbitrary -code *as the user* — read private files, rewrite `~/.ssh/authorized_keys`, install -cron/systemd-user units, `curl … | sh`. A1/A2 can drive these with misleading -descriptions and no prompt. - -This is a deliberate trust trade-off, correctly never relaxing the **privileged** -gate (verified: `privileged:true` always prompts regardless of policy; only an -interactive keypress can set the policy — no request field, replay, or MCP tool flips -it). Severity **Low**: the residual risk is real but gated behind an explicit human -choice. Recommendations: (1) state the residual risk plainly in the README and at the -`a`-key prompt (it currently reads as benign); (2) make the post-trust banner reliable -(don't drop it on `try_lock` failure) so silent execution is always at least visible. +### F2 — persisted global `confirm_unprivileged=false` silent execution (Low) — FIXED under G7 + +**Original location:** `src/server.rs` dispatch, `src/hosts.rs` (policy persistence). + +**Original finding.** Once a human pressed `a` at an unprivileged prompt, +`policy.confirm_unprivileged` was set `false` and **persisted globally** to +`hosts.json`; thereafter *all* unprivileged requests — on any host — skipped the +Y/N gate and ran after only a best-effort banner. An "unprivileged" command still +runs arbitrary code *as the user* (read private files, rewrite +`~/.ssh/authorized_keys`, `curl … | sh`), and A1/A2 could drive these unattended +with misleading descriptions. This was strictly weaker than the agent's Bash tool, +which gates every command. + +**Fix (invariant G7).** The persisted global flag is removed. Unattended +unprivileged execution now requires **two independent barriers**: (1) the daemon +is `unattended_eligible` — a per-daemon policy read once at startup from +`hosts.json` and immutable at runtime, so no wire field, MCP flag, or keypress can +set it; and (2) a human answers `a` this session, which flips an **in-memory, +session-scoped** grant that is **never persisted** and dies with the daemon/tunnel. +On a non-eligible daemon `a` approves the one command and grants nothing. In the +granted window the audit line is unconditional (`eprintln`, not the try_lock +banner), so unattended execution is always recorded. A stale +`confirm_unprivileged` key in an old config is ignored on load (fail-closed). +Regression tests: `tests/approval.rs` +(`non_eligible_approved_always_grants_nothing`, +`eligible_approved_always_grants_session_but_never_persists`), `src/hosts.rs` +(`stale_confirm_unprivileged_key_is_inert`). The residual — the bounded, +operator-opted-in, non-persistent unattended window — is characterised in +[REVIEWING.md](../REVIEWING.md) (C8, and the "NO assurance" note) and judged at +Bash-allow-rule parity. --- @@ -163,7 +175,7 @@ hidden)` marker) and/or reject individual arguments above a sane size for displa `save_to` uses `create_dir_all` + `File::create` with no umask tightening, so on a typical `umask 022` the config lands at mode `0644` (file) / `0755` (dir). By contrast the socket path explicitly tightens `umask(0o077)` around bind. `hosts.json` holds the -host inventory, cached remote UIDs, and the `confirm_unprivileged` policy — readable by +host inventory, cached remote UIDs, and the `unattended_eligible` policy — readable by *other* local users on a shared host. No escalation (others cannot write it: file 0644, dir owned by the user), and no impact on single-user systems. Severity **Low**. Remediation (proposed fix): tighten `umask` around the write and/or `chmod` the file to @@ -171,6 +183,36 @@ Remediation (proposed fix): tighten `umask` around the write and/or `chmod` the --- +### F5 — self/loopback host routes SSH-to-self, bypassing local policy (Low–Med) — FIXED under G7 + +**Boundary:** B5 (SSH tunnel → remote) primarily, decided at B2 (socket +selection). **Attacker:** A1 (prompt-injected MCP caller), A2 (same-UID). + +**Finding.** Host→transport routing keyed on the literal string `"localhost"`: +`execute`/`start_server` sent any other host string down the SSH path, and there +was **no loopback normalization**. So `execute(host="127.0.0.1", privileged=false)` +(or `::1`, or the machine's own hostname) targeted the *same physical machine* as +the agent's Bash tool but over the "remote" path — a laundering channel that +dodged both the local-only special-casing and (after G7's delegation) the +Bash-delegation refusal, letting an unprivileged command run on the local box +around the Bash tool's controls. It grants no new privilege (SSH-to-self needs the +user's own access) but relocates execution off the controlled path. + +**Fix (invariant G7, C9/C10).** `server::is_local_host` folds loopback literals +(`localhost`, all of `127.0.0.0/8` parsed not string-matched, `::1`), the trailing +FQDN-dot and `user@` forms, and the machine's own hostname to the local target; +`mcp::normalize_host` applies it at every routing site (`execute`, `start_server`, +`status`, and defensively inside `socket_for_host`). A local unprivileged command +is then refused and delegated to the Bash tool. Detection is best-effort +(ssh-config aliases and NAT hairpin can still hide a self-target), so soundness +rests on the C10 composition, not on catching every alias: every remote daemon's +only unattended path is the eligible + session-confirmed grant (C8), so an +undetected self-alias routed SSH-to-self still lands on a gated daemon. Regression +tests: `src/server.rs` (`is_local_host_classifies_self_and_loopback`), `src/mcp.rs` +(`normalize_host_folds_loopback_to_local`). + +--- + ### S1 — Signal handler is not async-signal-safe (Low) **Location:** `src/bin/sudo-proxy.rs:208-220` (`signal_hook_cleanup::handler`). diff --git a/docs/threat-model.md b/docs/threat-model.md index d689b1c..075c66f 100644 --- a/docs/threat-model.md +++ b/docs/threat-model.md @@ -96,9 +96,9 @@ invariant at a given boundary are omitted. | STRIDE | Threat | Control | Ref | |--------|--------|---------|-----| -| Tampering | Another user rewrites `hosts.json` to flip `confirm_unprivileged`. | File owned by the user; dir not group/other-writable; after F4 fix 0600/0700. The **privileged** gate is never relaxed by policy regardless. | G5.3 / F4, F2 | +| Tampering | Another user rewrites `hosts.json` to set `unattended_eligible`. | File owned by the user; dir not group/other-writable (0600/0700). Eligibility only *permits* the session-scoped `a` grant — it does not itself run anything unattended, and a human must still press `a`; the grant is never persisted. The **privileged** gate is never relaxed. A2 with write access to the config is already same-UID and can run unprivileged code directly, so this grants no new capability. | G7 / F4, F2 | | Info disclosure | World-readable `hosts.json` leaks host inventory, cached UIDs, policy. | After F4 fix: 0600 file / 0700 dir, mirroring the socket-bind umask pattern. | F4 | -| Elevation | Flip the privileged gate via the persisted policy. | Only an interactive keypress writes policy; policy relaxes only the **unprivileged** gate. | G5.1, G5.3 / F2 | +| Elevation | Flip the privileged gate, or run unprivileged unattended, via the persisted policy. | No persisted value enables unattended exec on load (a stale `confirm_unprivileged` is inert; `unattended_eligible` only permits, never grants). Only an interactive `a` keypress on an eligible daemon flips the in-memory grant; the **privileged** gate is never relaxed. | G7 (C4, C8) / F2 | ## Attack tree @@ -156,10 +156,16 @@ A A root command runs that the human did NOT approve Every leaf maps to a sub-goal in [assurance-case.md](assurance-case.md); no leaf is unaccounted for. The residuals — already on record and accepted — are: -- **1.4 / 4.4 — `confirm_unprivileged` (F2).** Once a human presses `a`, later - *unprivileged* commands run without a prompt. The **privileged** gate is never - affected. By-design trade-off; see - [security-audit.md](security-audit.md) finding F2. +- **1.4 / 4.4 — unattended unprivileged window (F2, closed; bounded residual).** + The old persisted, global auto-approve is gone (invariant G7). What remains is + bounded: on a daemon an operator has *deliberately* made `unattended_eligible`, + one `a` keypress opens a session-scoped, non-persistent window in which later + *unprivileged* commands run logged-but-unprompted. The **privileged** gate is + never affected. Judged at Bash-allow-rule parity; see + [security-audit.md](security-audit.md) finding F2 and REVIEWING.md C8. +- **loopback self-routing (F5, closed).** `is_local_host` normalizes self/loopback + targets; residual undetected aliases are covered by the C10 composition (every + remote daemon still gates unattended exec). See finding F5. - **3.4 — look-alike `reason` (F1 residual).** Printable Unicode can mislead but cannot conceal the real command line, which is printed separately. - **4.2 — SSH first-contact MITM (assumption A4).** The ssh invocation sets no diff --git a/docs/usage.md b/docs/usage.md index 995576e..2353ebd 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -5,17 +5,31 @@ ## Non-privileged mode With `privileged: false` in the request, sudo-proxy runs the command -directly as the current user, without sudo. The TUI Y/N gate fires by -default — same human review as the privileged path, just no password -step. The prompt offers three keys: `y` to approve once, `N` (default) -to deny, `a` to approve **and** mark the host as trusted for unprivileged -commands. Picking `a` writes a `policy` block into -`~/.config/sudo-proxy/hosts.json` (`{"confirm_unprivileged": false}`) -and from that point on unprivileged commands just print a one-line -banner — privileged commands still require a Y/N. Pass -`--no-confirm-unprivileged` to skip the gate without persisting, or -`--confirm-unprivileged` to force the gate even if the file says -otherwise. +directly as the current user, without sudo. The TUI Y/N gate fires on +**every** unprivileged command by default — same human review as the +privileged path, just no password step. + +Two things narrow this surface (invariant **G7** — "no unprivileged command +runs with less scrutiny than the Bash tool"): + +- **Local unprivileged commands are refused.** Over the MCP `execute` tool, an + unprivileged command targeting the local machine (including loopback aliases + like `127.0.0.1`, `::1`, or this host's own name) is declined with a message + telling the agent to use its Bash tool instead — which already applies your + permission rules. sudo-proxy is for privilege escalation and for remote hosts. +- **Unattended mode needs two deliberate acts.** On a **remote** host you may + want a batch of unprivileged commands to run without a keypress each. That + requires (1) an operator to set `"unattended_eligible": true` in that host's + `~/.config/sudo-proxy/hosts.json` (an out-of-band file edit — nothing on the + wire can set it), and (2) a human to answer `a` at a prompt. Only then do + subsequent unprivileged commands run unattended, logged, **for that session + only** — the grant is in-memory and never persisted, so it is gone when the + daemon or SSH tunnel ends. On a non-eligible daemon, `a` approves just the one + command. The **privileged** gate is never relaxed. + +Pass `--unattended-eligible` when starting a daemon to make it eligible without +editing the file (the session grant still needs the `a` keypress; it is never +persisted). ## Command-line @@ -31,12 +45,11 @@ sudo-proxy -v sudo-proxy --host remotehost sudo-proxy --host remotehost -v # prints the ssh command before connecting -# Skip the confirmation prompt for unprivileged commands -# (default is to prompt for both; press `a` at a prompt to persist this -# choice in hosts.json so future runs of the daemon skip the gate too) -sudo-proxy --no-confirm-unprivileged -# Force the gate even if hosts.json says otherwise -sudo-proxy --confirm-unprivileged +# Allow a human to grant unattended unprivileged execution for the session +# (default is to prompt every command; with this flag the prompt offers `a`, +# which grants an in-memory, never-persisted session grant). Equivalent to +# setting "unattended_eligible": true in this host's hosts.json. +sudo-proxy --unattended-eligible # Custom socket path sudo-proxy --socket /tmp/my-proxy.sock diff --git a/proofs/tla/ApprovalStateMachine.cfg b/proofs/tla/ApprovalStateMachine.cfg index 7ec4bd3..06a6279 100644 --- a/proofs/tla/ApprovalStateMachine.cfg +++ b/proofs/tla/ApprovalStateMachine.cfg @@ -13,8 +13,9 @@ INVARIANTS TypeOK NoExecWithoutApproval ReplayImpossible - PolicyFlipsOnlyOnKeypress + GrantOnlyByKeypressWhenEligible PrivilegedGateIndependentOfPolicy + NoUnattendedUnprivilegedExec PROPERTIES - FlagMonotone + EligibilityImmutable diff --git a/proofs/tla/ApprovalStateMachine.tla b/proofs/tla/ApprovalStateMachine.tla index 61184a4..24fd6a3 100644 --- a/proofs/tla/ApprovalStateMachine.tla +++ b/proofs/tla/ApprovalStateMachine.tla @@ -3,9 +3,12 @@ (* Rung 4 of the sudo-proxy formalisation roadmap: a TLA+/PlusCal model of *) (* the approval state machine, model-checked by TLC. *) (* *) -(* It pins the two accepted residuals the Rung 0 threat model routed here *) -(* (attack-tree leaves 1.4 / 4.4, finding F2): the `confirm_unprivileged` *) -(* policy transition and the unconditional human gate on privileged exec. *) +(* It pins the privileged invariant (G1: an unconditional human gate on *) +(* privileged exec) AND the second invariant (G7: no unprivileged command *) +(* runs unattended except behind two barriers -- an immutable eligibility *) +(* policy and a session-scoped `a` keypress -- with the grant never *) +(* persisted). This is the model form of audit findings F2 (closed) and *) +(* attack-tree leaves 1.4 / 4.4. *) (* *) (* The PlusCal algorithm below is the source of truth; the TLC-checkable *) (* TLA+ translation between BEGIN/END TRANSLATION is generated from it by *) @@ -28,7 +31,7 @@ EXTENDS Naturals, FiniteSets CONSTANTS Ids \* the bounded set of request ids, e.g. {r1, r2} \* The entire input domain of tui::classify_key, abstracted: a keypress 'y', -\* an 'a' (always-allow), any other key, or a poll timeout (no keypress). +\* an 'a' (approve + grant this session), any other key, or a poll timeout. KeyChoice == {"y", "a", "other", "timeout"} \* A request as seen by the daemon. Every gate is abstracted to the boolean @@ -53,10 +56,18 @@ NoReq == [ id |-> CHOOSE i \in Ids : TRUE, (* --algorithm ApprovalStateMachine variables - \* The persisted/in-memory policy flag (Arc), daemon-lifetime. - \* Init is a free boolean so both the default and `--no-confirm-unprivileged` - \* startup are covered. - confirmUnpriv \in BOOLEAN, + \* Barrier 1 (G7): the per-daemon `unattended_eligible` policy. Read once + \* from hosts.json at startup and IMMUTABLE at runtime -- no transition below + \* writes it. Init is a free boolean so both an eligible and a non-eligible + \* daemon are covered. A mutation that wires this from the wire trips + \* EligibilityImmutable. + eligible \in BOOLEAN, + + \* Barrier 2 (G7): the in-memory, session-scoped unattended grant. Starts + \* FALSE (fail-closed), flips to TRUE only via an 'a' keypress on an + \* unprivileged request when the daemon is eligible, and is reset to FALSE at + \* a session boundary (SessionEnd below) -- never persisted. + grant = FALSE, \* Replay dedup (SeenIds): ids the daemon has accepted (whether the dispatch \* then executed, denied, or timed out). Modelled as a monotonically growing @@ -78,9 +89,13 @@ variables vNoExec = FALSE, \* An exec happened for an id that was already executed (a replayed exec). vReplay = FALSE, - \* The policy flag was flipped other than by an 'a' keypress on an - \* unprivileged request. - vFlip = FALSE; + \* The session grant was set other than by an 'a' keypress on an + \* unprivileged request while the daemon was eligible. + vGrant = FALSE, + \* An unprivileged command ran unattended (no keypress consulted this + \* request) while the daemon was NOT eligible -- i.e. an unattended exec that + \* the two barriers should have made unreachable. + vUnattended = FALSE; define \* A direct transcription of tui::classify_key(key, privileged). @@ -92,11 +107,10 @@ define ELSE "Denied" end define; -\* Bookkeeping at every execution site. `isPriv` says whether this is the -\* privileged (exec_sudo) path. Raises the replay flag if this id already ran, -\* and -- on the privileged path -- the no-approval flag if the executing -\* keypress was not 'y'. So if any mutation routes a privileged exec onto a -\* non-'y' key, or lets an id execute twice, the monitor catches it here. +\* Bookkeeping at every *attended* execution site (a keypress was consulted for +\* this request). `isPriv` says whether this is the privileged (exec_sudo) path. +\* Raises the replay flag if this id already ran, and -- on the privileged path +\* -- the no-approval flag if the executing keypress was not 'y'. macro NoteExec(isPriv) begin if req.id \in executed then vReplay := TRUE; @@ -107,31 +121,54 @@ macro NoteExec(isPriv) begin executed := executed \union {req.id}; end macro; -\* The single primitive that writes the policy flag. Any flip must go through -\* it; it raises the flip-provenance flag unless the writer is an 'a' keypress -\* on an unprivileged request -- so wiring a flip into any other branch (a -\* request field, a timeout, the privileged path) trips the monitor. -macro FlipFlag() begin - confirmUnpriv := FALSE; - if req.privileged \/ key # "a" then - vFlip := TRUE; +\* Bookkeeping at the *unattended* execution site: an unprivileged command that +\* ran via the session grant, with no keypress consulted for this request. It is +\* legitimate only when the daemon is eligible (the grant was established earlier +\* by an 'a' keypress). Raise the unattended flag if it ever fires while not +\* eligible -- the backstop that makes G7 sound even if a mutation sets `grant`. +macro NoteUnattended() begin + if ~eligible then + vUnattended := TRUE; end if; + if req.id \in executed then + vReplay := TRUE; + end if; + executed := executed \union {req.id}; end macro; -\* The environment / attacker: submits an arbitrary request (every field free, -\* including an id already in `seen` -- a replay) and an arbitrary operator -\* keypress. TLC's exhaustive search universally quantifies the properties over -\* all attacker forgeries / replays AND all operator choices. +\* The single primitive that sets the session grant. Any grant must go through +\* it; it raises the grant-provenance flag unless the writer is an 'a' keypress +\* on an unprivileged request AND the daemon is eligible -- so wiring a grant +\* into any other branch, or granting without eligibility, trips the monitor. +macro GrantSession() begin + grant := TRUE; + if req.privileged \/ key # "a" \/ ~eligible then + vGrant := TRUE; + end if; +end macro; + +\* The environment / attacker: either submits an arbitrary request (every field +\* free, including an id already in `seen` -- a replay) with an arbitrary operator +\* keypress, OR ends the session (the SSH tunnel / daemon drops), which discards +\* the in-memory grant. TLC's exhaustive search universally quantifies the +\* properties over all attacker forgeries / replays, operator choices, and +\* session boundaries. process Env = "env" begin EnvLoop: while TRUE do await ~busy; - with r \in Requests, k \in KeyChoice do - req := r; - key := k; - busy := TRUE; - end with; + either + with r \in Requests, k \in KeyChoice do + req := r; + key := k; + busy := TRUE; + end with; + or + \* Session boundary: the grant is in-memory only, so it is lost. + \* Models "not longer than the MCP session / SSH tunnel". + grant := FALSE; + end either; end while; end process; @@ -158,7 +195,7 @@ Handle: \* Accept (try_insert -> true), then dispatch. seen := seen \union {req.id}; if req.privileged then - \* PRIVILEGED PATH -- never consults confirmUnpriv. + \* PRIVILEGED PATH -- never consults eligible or grant. \* ApprovedAlways is impossible here (Classify guards on ~priv) \* and the real code defensively folds it to Denied anyway. if Classify(key, TRUE) = "Approved" then @@ -168,21 +205,26 @@ Handle: else skip; \* denied end if; - elsif confirmUnpriv then - \* UNPRIVILEGED + confirmation ON. + elsif grant then + \* UNPRIVILEGED + session granted: run unattended, no prompt. + \* Reachable only after an 'a' keypress on an eligible daemon. + NoteUnattended(); \* exec_direct + reliable log + else + \* UNPRIVILEGED + not (yet) granted: prompt for this command. if Classify(key, FALSE) = "Approved" then NoteExec(FALSE); \* exec_direct elsif Classify(key, FALSE) = "ApprovedAlways" then - FlipFlag(); \* the ONLY flag writer + \* 'a': approve THIS command regardless; grant the session + \* only when eligible (barrier 1 gates barrier 2). + if eligible then + GrantSession(); + end if; NoteExec(FALSE); \* exec_direct elsif Classify(key, FALSE) = "Timeout" then skip; else skip; \* denied end if; - else - \* UNPRIVILEGED + confirmation OFF: no prompt, exec directly. - NoteExec(FALSE); \* exec_direct end if; end if; busy := FALSE; @@ -190,9 +232,9 @@ Handle: end process; end algorithm; *) -\* BEGIN TRANSLATION (chksum(pcal) = "5576622c" /\ chksum(tla) = "3c1dd0ad") -VARIABLES confirmUnpriv, seen, executed, req, key, busy, vNoExec, vReplay, - vFlip +\* BEGIN TRANSLATION +VARIABLES eligible, grant, seen, executed, req, key, busy, vNoExec, vReplay, + vGrant, vUnattended (* define statement *) Classify(k, priv) == @@ -202,13 +244,14 @@ Classify(k, priv) == ELSE "Denied" -vars == << confirmUnpriv, seen, executed, req, key, busy, vNoExec, vReplay, - vFlip >> +vars == << eligible, grant, seen, executed, req, key, busy, vNoExec, vReplay, + vGrant, vUnattended >> ProcSet == {"env"} \cup {"daemon"} Init == (* Global variables *) - /\ confirmUnpriv \in BOOLEAN + /\ eligible \in BOOLEAN + /\ grant = FALSE /\ seen = {} /\ executed = {} /\ req = NoReq @@ -216,46 +259,54 @@ Init == (* Global variables *) /\ busy = FALSE /\ vNoExec = FALSE /\ vReplay = FALSE - /\ vFlip = FALSE + /\ vGrant = FALSE + /\ vUnattended = FALSE Env == /\ ~busy - /\ \E r \in Requests: - \E k \in KeyChoice: - /\ req' = r - /\ key' = k - /\ busy' = TRUE - /\ UNCHANGED << confirmUnpriv, seen, executed, vNoExec, vReplay, vFlip >> + /\ \/ /\ \E r \in Requests: + \E k \in KeyChoice: + /\ req' = r + /\ key' = k + /\ busy' = TRUE + /\ grant' = grant + \/ /\ grant' = FALSE + /\ UNCHANGED <> + /\ UNCHANGED << eligible, seen, executed, vNoExec, vReplay, vGrant, + vUnattended >> Daemon == /\ busy /\ IF ~req.wellFormed THEN /\ TRUE - /\ UNCHANGED << confirmUnpriv, seen, executed, vNoExec, - vReplay, vFlip >> + /\ UNCHANGED << grant, seen, executed, vNoExec, vReplay, + vGrant, vUnattended >> ELSE /\ IF req.forwardAgent /\ req.privileged THEN /\ TRUE - /\ UNCHANGED << confirmUnpriv, seen, executed, - vNoExec, vReplay, vFlip >> + /\ UNCHANGED << grant, seen, executed, vNoExec, + vReplay, vGrant, vUnattended >> ELSE /\ IF ~req.fresh THEN /\ TRUE - /\ UNCHANGED << confirmUnpriv, seen, + /\ UNCHANGED << grant, seen, executed, vNoExec, - vReplay, vFlip >> + vReplay, vGrant, + vUnattended >> ELSE /\ IF ~req.envOk THEN /\ TRUE - /\ UNCHANGED << confirmUnpriv, + /\ UNCHANGED << grant, seen, executed, vNoExec, vReplay, - vFlip >> + vGrant, + vUnattended >> ELSE /\ IF req.id \in seen THEN /\ TRUE - /\ UNCHANGED << confirmUnpriv, + /\ UNCHANGED << grant, seen, executed, vNoExec, vReplay, - vFlip >> + vGrant, + vUnattended >> ELSE /\ seen' = (seen \union {req.id}) /\ IF req.privileged THEN /\ IF Classify(key, TRUE) = "Approved" @@ -274,10 +325,23 @@ Daemon == /\ busy /\ UNCHANGED << executed, vNoExec, vReplay >> - /\ UNCHANGED << confirmUnpriv, - vFlip >> - ELSE /\ IF confirmUnpriv - THEN /\ IF Classify(key, FALSE) = "Approved" + /\ UNCHANGED << grant, + vGrant, + vUnattended >> + ELSE /\ IF grant + THEN /\ IF ~eligible + THEN /\ vUnattended' = TRUE + ELSE /\ TRUE + /\ UNCHANGED vUnattended + /\ IF req.id \in executed + THEN /\ vReplay' = TRUE + ELSE /\ TRUE + /\ UNCHANGED vReplay + /\ executed' = (executed \union {req.id}) + /\ UNCHANGED << grant, + vNoExec, + vGrant >> + ELSE /\ IF Classify(key, FALSE) = "Approved" THEN /\ IF req.id \in executed THEN /\ vReplay' = TRUE ELSE /\ TRUE @@ -287,14 +351,18 @@ Daemon == /\ busy ELSE /\ TRUE /\ UNCHANGED vNoExec /\ executed' = (executed \union {req.id}) - /\ UNCHANGED << confirmUnpriv, - vFlip >> + /\ UNCHANGED << grant, + vGrant >> ELSE /\ IF Classify(key, FALSE) = "ApprovedAlways" - THEN /\ confirmUnpriv' = FALSE - /\ IF req.privileged \/ key # "a" - THEN /\ vFlip' = TRUE + THEN /\ IF eligible + THEN /\ grant' = TRUE + /\ IF req.privileged \/ key # "a" \/ ~eligible + THEN /\ vGrant' = TRUE + ELSE /\ TRUE + /\ UNCHANGED vGrant ELSE /\ TRUE - /\ vFlip' = vFlip + /\ UNCHANGED << grant, + vGrant >> /\ IF req.id \in executed THEN /\ vReplay' = TRUE ELSE /\ TRUE @@ -307,36 +375,27 @@ Daemon == /\ busy ELSE /\ IF Classify(key, FALSE) = "Timeout" THEN /\ TRUE ELSE /\ TRUE - /\ UNCHANGED << confirmUnpriv, + /\ UNCHANGED << grant, executed, vNoExec, vReplay, - vFlip >> - ELSE /\ IF req.id \in executed - THEN /\ vReplay' = TRUE - ELSE /\ TRUE - /\ UNCHANGED vReplay - /\ IF FALSE /\ key # "y" - THEN /\ vNoExec' = TRUE - ELSE /\ TRUE - /\ UNCHANGED vNoExec - /\ executed' = (executed \union {req.id}) - /\ UNCHANGED << confirmUnpriv, - vFlip >> + vGrant >> + /\ UNCHANGED vUnattended /\ busy' = FALSE - /\ UNCHANGED << req, key >> + /\ UNCHANGED << eligible, req, key >> Next == Env \/ Daemon Spec == Init /\ [][Next]_vars -\* END TRANSLATION +\* END TRANSLATION \* ===================== invariants & properties ===================== \* Modeling-hygiene type invariant. TypeOK == - /\ confirmUnpriv \in BOOLEAN + /\ eligible \in BOOLEAN + /\ grant \in BOOLEAN /\ seen \subseteq Ids /\ executed \subseteq Ids /\ req \in Requests @@ -344,7 +403,8 @@ TypeOK == /\ busy \in BOOLEAN /\ vNoExec \in BOOLEAN /\ vReplay \in BOOLEAN - /\ vFlip \in BOOLEAN + /\ vGrant \in BOOLEAN + /\ vUnattended \in BOOLEAN \* P1: a privileged exec happens only on a 'y' keypress -- never on timeout, \* denial, replay, or any policy state. (leaf 1.4 / G5) @@ -353,20 +413,28 @@ NoExecWithoutApproval == ~vNoExec \* P2: the same request id never causes two executions. (leaf 1.1 / G2.1) ReplayImpossible == ~vReplay -\* P3: the policy flag flips only via an ApprovedAlways ('a') keypress on an -\* unprivileged request -- never a request field, replay, MCP flag, or timeout. -\* (leaf 1.4 / 4.4 / F2) -PolicyFlipsOnlyOnKeypress == ~vFlip +\* P3: the session grant is set only via an 'a' keypress on an unprivileged +\* request AND only when the daemon is eligible -- never a request field, replay, +\* MCP flag, timeout, or a non-eligible daemon. (G7 / C4, C8) +GrantOnlyByKeypressWhenEligible == ~vGrant -\* P4: no privileged exec without a 'y' keypress, for ANY value the policy flag -\* took. The privileged branch structurally never reads confirmUnpriv, so this -\* is the "independent of policy" reading of the same witness as P1; stated -\* separately for assurance-case traceability (G5.1) and tripped by the NC2 -\* mutation that wires the flag into the privileged path. (leaf 4.4 / G5.1) +\* P4: no privileged exec without a 'y' keypress, for ANY eligibility/grant +\* state. The privileged branch structurally never reads them, so this is the +\* "independent of policy" reading of the same witness as P1; stated separately +\* for assurance-case traceability (G5.1) and tripped by a mutation that wires +\* the policy into the privileged path. (leaf 4.4 / G5.1) PrivilegedGateIndependentOfPolicy == ~vNoExec -\* P3 (temporal half): once the flag is FALSE it stays FALSE -- it only ever -\* moves true->false, and only via the single writer above. -FlagMonotone == [][confirmUnpriv' => confirmUnpriv]_confirmUnpriv +\* P5 (G7, the second invariant): no unprivileged command runs unattended unless +\* the daemon is eligible. Combined with P3 (the grant's 'a'-keypress provenance) +\* and the session-reset in Env (the grant never outlives a session), this is the +\* model form of "no unprivileged command runs with less scrutiny than the Bash +\* tool". (leaf 1.4 / 4.4 / F2) +NoUnattendedUnprivilegedExec == ~vUnattended + +\* P6: eligibility is a runtime-immutable input -- no transition ever changes it +\* (barrier 1 can only be set out-of-band, at startup, from hosts.json). Tripped +\* by any mutation that writes `eligible` from a request field. (G7 / C4) +EligibilityImmutable == [][eligible' = eligible]_eligible ============================================================================= diff --git a/proofs/tla/ConcurrentHandlers.tla b/proofs/tla/ConcurrentHandlers.tla index 3b72dea..cc4ecbe 100644 --- a/proofs/tla/ConcurrentHandlers.tla +++ b/proofs/tla/ConcurrentHandlers.tla @@ -43,8 +43,9 @@ NoHolder == "none" \* Keypress abstraction restricted to what matters for concurrency: "y" \* approves (so the exec site is reached), "other" denies/times out (so the \* TTY lock is released without executing). The full keypress decision table -\* and the confirm_unprivileged flip are the ApprovalStateMachine model's job; -\* here no "a" is offered, so confirmUnpriv is read-only. +\* and the two-barrier unprivileged gate (eligibility + session grant, G7) are +\* the ApprovalStateMachine model's job; here `confirmUnpriv` is a read-only +\* stand-in for "the unprivileged path may or may not prompt" (no "a" offered). Keys == {"y", "other"} (* --algorithm ConcurrentHandlers diff --git a/proofs/tla/README.md b/proofs/tla/README.md index 41c1aeb..335b19b 100644 --- a/proofs/tla/README.md +++ b/proofs/tla/README.md @@ -4,8 +4,9 @@ This directory holds **three** TLA+/PlusCal models, all model-checked by TLC: -1. **`ApprovalStateMachine`** (below) — the approval state machine: the four - safety properties of the gate chain and `classify_key`. +1. **`ApprovalStateMachine`** (below) — the approval state machine: the safety + properties of the gate chain and `classify_key`, covering both the privileged + invariant (G1) and the unprivileged two-barrier invariant (G7). 2. **[`ReplayWindow`](#window-sizing--freshness--replay-retention) (Extended Rung 4)** — the temporal freshness ↔ replay-retention **window-sizing** property that the first model deliberately abstracts away (it never evicts). Restores a small @@ -25,9 +26,9 @@ A TLA+/PlusCal model of sudo-proxy's approval state machine, model-checked by TLC. It discharges **Rung 4** (state-machine half) of the [formalisation roadmap](../../docs/formalisation-roadmap.md): the temporal / relational properties that are not per-function and that the Rung 0 threat model -routed here — attack-tree leaves **1.4 / 4.4** (the `confirm_unprivileged` -policy transition, finding **F2**) and the unconditional human gate on -privileged execution. +routed here — attack-tree leaves **1.4 / 4.4** (the unprivileged +eligibility + session-grant transitions, invariant **G7** / finding **F2**) and +the unconditional human gate on privileged execution. `ApprovalStateMachine.tla` carries the PlusCal algorithm in a comment block (the source of truth) followed by the `pcal.trans`-generated TLA+ translation that TLC @@ -35,7 +36,7 @@ checks. `ApprovalStateMachine.cfg` is the bounded model. The Rung 3 Kani sibling lives in [`src/proofs.rs`](../../src/proofs.rs); this is the protocol-level analogue. -## The four properties +## The properties Tracked by bounded **monitor variables** — a violation flag is raised at the exact site the bad thing would happen, and the invariant asserts the flag stays @@ -47,8 +48,10 @@ requests are rejected as replays and only revisit states.) |---|-----------|-------|---------| | **P1** | `NoExecWithoutApproval` | A privileged exec happens only on a `y` keypress — never on timeout, denial, replay, or any policy state. | leaf 1.4 · G5 | | **P2** | `ReplayImpossible` | The same request id never causes two executions. | leaf 1.1 · G2.1 | -| **P3** | `PolicyFlipsOnlyOnKeypress` (+ action property `FlagMonotone`) | The policy flag flips only via an `a` keypress on an *unprivileged* request — never a request field, replay, MCP flag, or timeout — and once `FALSE` stays `FALSE`. | leaf 1.4 / 4.4 · F2 | -| **P4** | `PrivilegedGateIndependentOfPolicy` | No privileged exec without a `y` keypress, for *any* value the policy flag took. The privileged branch structurally never reads the flag; stated separately for traceability. | leaf 4.4 · G5.1 | +| **P3** | `GrantOnlyByKeypressWhenEligible` | The session grant (barrier 2) is set only via an `a` keypress on an *unprivileged* request AND only when the daemon is `eligible` — never a request field, replay, MCP flag, timeout, or a non-eligible daemon. | leaf 1.4 / 4.4 · G7 (C4/C8) · F2 | +| **P4** | `PrivilegedGateIndependentOfPolicy` | No privileged exec without a `y` keypress, for *any* eligibility/grant state. The privileged branch structurally never reads them; stated separately for traceability. | leaf 4.4 · G5.1 | +| **P5** | `NoUnattendedUnprivilegedExec` | No unprivileged command runs unattended (no keypress consulted) unless the daemon is `eligible`. With P3 and the session-reset in `Env` (the grant never outlives a session), this is the model form of "no unprivileged command runs with less scrutiny than the Bash tool". | leaf 1.4 / 4.4 · **G7** · F2 | +| **P6** | `EligibilityImmutable` (action property) | Eligibility (barrier 1) is a runtime-immutable input — no transition ever writes it; it can only be set out-of-band at startup from `hosts.json`. | G7 · C4 | `Classify(k, priv)` in the spec is a direct transcription of [`tui::classify_key`](../../src/tui.rs); the daemon's gate chain mirrors the @@ -83,13 +86,16 @@ verified to violate exactly the listed invariant. | # | Mutation (in the PlusCal) | Violates | |---|---------------------------|----------| | **NC1** | Privileged `Timeout` branch: replace `skip;` with `NoteExec(TRUE);` (exec on timeout). | `NoExecWithoutApproval` | -| **NC2** | Wire the flag into the privileged gate: prepend `if confirmUnpriv then NoteExec(TRUE); elsif …` to the privileged dispatch. | `NoExecWithoutApproval` (= `PrivilegedGateIndependentOfPolicy` — same witness) | +| **NC2** | Wire the policy into the privileged gate: prepend `if grant then NoteExec(TRUE); elsif …` to the privileged dispatch. | `NoExecWithoutApproval` (= `PrivilegedGateIndependentOfPolicy` — same witness) | | **NC3** | Remove the dedup gate: change `elsif req.id \in seen then` to `elsif FALSE then`. | `ReplayImpossible` | -| **NC4** | Flip the flag illegitimately: replace the `skip;` in the unprivileged `Timeout` branch with `FlipFlag();`. | `PolicyFlipsOnlyOnKeypress` | +| **NC5** | Grant without eligibility: drop the `if eligible then … end if;` guard around `GrantSession()` so `a` grants on a non-eligible daemon. | `GrantOnlyByKeypressWhenEligible` (verified) | +| **NC6** | Unattended without a keypress: replace the `skip;` in the ungranted unprivileged `Timeout` branch with `NoteUnattended();`. | `NoUnattendedUnprivilegedExec` (verified) | +| **NC7** | Wire eligibility from the wire: add `eligible := req.forwardAgent;` in the daemon accept branch. | `EligibilityImmutable` | -Note NC4 leaves `FlagMonotone` *satisfied* (a `TRUE→FALSE` flip is monotone-OK): -it is the provenance monitor `PolicyFlipsOnlyOnKeypress`, not monotonicity, that -catches an illegitimate flip — which is the point of having both. +NC5 and NC6 are the load-bearing G7 controls: NC5 shows the grant cannot be +established without both barriers, and NC6 shows an unattended exec cannot happen +on a non-eligible daemon — together the model form of "no weaker than the Bash +tool". Both were run and confirmed to fail exactly the listed invariant. ## Faithfulness ledger @@ -103,15 +109,17 @@ note in the [roadmap](../../docs/formalisation-roadmap.md). observable. - `classify_key`'s exact decision table (`Classify`), including that `ApprovedAlways` is emitted iff `a` ∧ unprivileged. -- Both dispatch branches, the defensive `ApprovedAlways → Denied` fold on the - privileged path, and that `confirm_unprivileged` is written by exactly one - primitive (`FlipFlag`). +- All three unprivileged dispatch states (granted → unattended; ungranted → + prompt; `ApprovedAlways` grants only when eligible), the defensive + `ApprovedAlways → Denied` fold on the privileged path, that the session `grant` + is written by exactly one primitive (`GrantSession`), that `eligible` is never + written, and that a session boundary resets `grant` (non-persistence). - Replay rejection via a monotonically-growing `seen` set. - The attacker and the operator are both fully nondeterministic, so the properties are universally quantified over all field forgeries / replays and all operator choices. -**Abstracted (sound for these four properties)** +**Abstracted (sound for these properties)** - The clock / freshness check → a boolean `fresh`; env contents → `envOk`; decode + peer-auth → `wellFormed`. The properties don't depend on the contents @@ -356,19 +364,21 @@ a vacuous pass. NC2 reproduces the PR #22 hazard the TTY lock exists to prevent. - The per-request gate chain (validate → freshness → env allowlist) → assumed passed: it is per-handler and sequential, covered by the - [`ApprovalStateMachine`](#the-four-properties) model. The focus here is the + [`ApprovalStateMachine`](#the-properties) model. The focus here is the shared-state races. - `SeenIds` eviction → never evict (the [`ReplayWindow`](#window-sizing--freshness--replay-retention) model owns the TTL); irrelevant to a concurrency race within the model's horizon. -- `confirm_unprivileged` → a read-only init-free boolean (no `a` key), so its flip - semantics stay with the `ApprovalStateMachine` model; both dispatch paths are - still covered. +- the unprivileged gate → a read-only init-free boolean `confirmUnpriv` (no `a` + key offered here), a stand-in for "the unprivileged path may or may not prompt". + The real two-barrier gate semantics (eligibility + session grant, invariant G7) + stay with the `ApprovalStateMachine` model; here both dispatch paths reach the + exec site, which is all the concurrency properties need. - Handler count bounded to 2 (3 also checked); the keypress set to `{y, other}`. **Out of scope** - The approval/keypress decision table and the flag-flip transition — the - [`ApprovalStateMachine`](#the-four-properties) model. + [`ApprovalStateMachine`](#the-properties) model. - The freshness ↔ retention window sizing — the [`ReplayWindow`](#window-sizing--freshness--replay-retention) model. diff --git a/src/bin/sudo-proxy.rs b/src/bin/sudo-proxy.rs index dfac2b4..7bcba2a 100644 --- a/src/bin/sudo-proxy.rs +++ b/src/bin/sudo-proxy.rs @@ -13,9 +13,9 @@ struct Opts { login: Option, pkexec: bool, verbose: bool, - /// `None` = take the default from `hosts.json` (`policy.confirm_unprivileged`). + /// `None` = take the default from `hosts.json` (`policy.unattended_eligible`). /// `Some(_)` = an explicit CLI flag was passed and overrides the file. - confirm_unprivileged: Option, + unattended_eligible: Option, forward_agent: bool, } @@ -77,20 +77,21 @@ fn main() { let in_flight = Arc::new(AtomicUsize::new(0)); let tty_lock = Arc::new(Mutex::new(())); - // Resolve the per-host policy: explicit CLI flag wins; otherwise the - // persisted `policy.confirm_unprivileged` in hosts.json; otherwise the - // built-in default (on). - let confirm_unprivileged = opts.confirm_unprivileged.unwrap_or_else(|| { + // Resolve unattended eligibility: explicit CLI flag wins; otherwise the + // persisted `policy.unattended_eligible` in hosts.json; otherwise the + // built-in default (off, fail-closed). This is barrier 1 of G7 — an + // out-of-band operator opt-in; nothing on the wire can change it. + let unattended_eligible = opts.unattended_eligible.unwrap_or_else(|| { sudo_proxy::hosts::HostsConfig::load() .policy - .confirm_unprivileged + .unattended_eligible }); let config = server::ServerConfig { mode, pkexec_only: opts.pkexec, verbose: opts.verbose, - confirm_unprivileged, + unattended_eligible, ..Default::default() }; @@ -133,7 +134,7 @@ fn parse_args(args: &[String]) -> Opts { let mut verbose = false; // Tri-state: None means "no CLI flag passed, use hosts.json policy", // Some(_) means an explicit flag overrides the file. - let mut confirm_unprivileged: Option = None; + let mut unattended_eligible: Option = None; let mut forward_agent = false; let mut iter = args.iter().skip(1); while let Some(arg) = iter.next() { @@ -152,24 +153,25 @@ fn parse_args(args: &[String]) -> Opts { } "--pkexec" => pkexec = true, "--verbose" | "-v" => verbose = true, - "--confirm-unprivileged" => confirm_unprivileged = Some(true), - "--no-confirm-unprivileged" => confirm_unprivileged = Some(false), + "--unattended-eligible" => unattended_eligible = Some(true), + "--no-unattended-eligible" => unattended_eligible = Some(false), "--forward-agent" => forward_agent = true, "--version" | "-V" => sudo_proxy::cli::print_version("sudo-proxy"), "--help" | "-h" => { - eprintln!("Usage: sudo-proxy [--socket PATH] [--host HOST] [--pkexec] [-v] [--no-confirm-unprivileged] [--forward-agent]"); + eprintln!("Usage: sudo-proxy [--socket PATH] [--host HOST] [--pkexec] [-v] [--unattended-eligible] [--forward-agent]"); eprintln!(); eprintln!("Privileged command execution proxy."); eprintln!("Listens on a Unix socket for JSON requests and executes them via pkexec or sudo."); - eprintln!("Every command (privileged or not) goes through the TUI Y/N gate by default."); + eprintln!("Every command (privileged or not) goes through the TUI Y/N gate."); eprintln!(); eprintln!("Options:"); eprintln!(" --socket PATH Socket path (default: $XDG_RUNTIME_DIR/sudo-proxy.sock)"); eprintln!(" --host HOST Connect to remote host via SSH tunnel"); eprintln!(" --pkexec Use pkexec directly (no TUI prompt, pkexec handles both auth and approval)"); eprintln!(" --verbose, -v Print startup info and log each request to stderr"); - eprintln!(" --no-confirm-unprivileged Skip the Y/N gate for unprivileged commands (batch/automation)"); - eprintln!(" --confirm-unprivileged Force the Y/N gate for unprivileged commands even if hosts.json says otherwise"); + eprintln!(" --unattended-eligible Allow a human to grant unattended unprivileged execution"); + eprintln!(" for this session via the prompt's `a` answer (overrides hosts.json;"); + eprintln!(" the grant is in-memory only and never persisted)"); eprintln!(" --forward-agent With --host: enable SSH agent forwarding (-A) so unprivileged"); eprintln!(" commands that opt in via forward_agent can use the local agent"); std::process::exit(0); @@ -186,7 +188,7 @@ fn parse_args(args: &[String]) -> Opts { login, pkexec, verbose, - confirm_unprivileged, + unattended_eligible, forward_agent, } } diff --git a/src/gui.rs b/src/gui.rs index 7489b31..282525f 100644 --- a/src/gui.rs +++ b/src/gui.rs @@ -11,14 +11,22 @@ const PROMPT_TIMEOUT: Duration = Duration::from_secs(60); pub struct GuiPrompter; impl Prompter for GuiPrompter { - fn prompt(&self, req: &ValidatedRequest, _timeout: Duration) -> io::Result { - prompt_gui(req) + // The GUI dialogs (zenity/kdialog) are plain yes/no — they never offer the + // session-scoped `a` answer, so `eligible` is honored only on the TUI + // fallback path. + fn prompt( + &self, + req: &ValidatedRequest, + eligible: bool, + _timeout: Duration, + ) -> io::Result { + prompt_gui(req, eligible) } } /// Show a Y/N confirmation dialog for a command request. /// Auto-detects: zenity → kdialog → TUI (/dev/tty) fallback. -pub fn prompt_gui(req: &ValidatedRequest) -> io::Result { +pub fn prompt_gui(req: &ValidatedRequest, eligible: bool) -> io::Result { let text = format_prompt_text(req); if which("zenity").is_some() { @@ -34,7 +42,7 @@ pub fn prompt_gui(req: &ValidatedRequest) -> io::Result { } // Fall back to TUI - tui::prompt_tty(req, PROMPT_TIMEOUT) + tui::prompt_tty(req, eligible, PROMPT_TIMEOUT) } fn format_prompt_text(req: &ValidatedRequest) -> String { diff --git a/src/hosts.rs b/src/hosts.rs index c0a270b..06611b3 100644 --- a/src/hosts.rs +++ b/src/hosts.rs @@ -23,21 +23,26 @@ pub struct HostInfo { pub version: String, } -#[derive(Debug, Serialize, Deserialize, Clone)] +/// Per-daemon policy, read from `hosts.json` at startup and **immutable at +/// runtime** — no wire field, MCP flag, or keypress can change it; only an +/// out-of-band edit of the file does. This is the first of the two barriers +/// guarding unattended unprivileged execution (invariant G7 in REVIEWING.md); +/// the second is the in-memory, session-scoped grant in `server.rs`. +#[derive(Debug, Serialize, Deserialize, Clone, Default)] pub struct Policy { - /// When `true`, unprivileged commands hit the TTY Y/N gate (today's - /// default). When `false`, they take the banner-only path. The - /// interactive `a` answer flips this to `false` and persists. - #[serde(default = "crate::protocol::default_true")] - pub confirm_unprivileged: bool, -} - -impl Default for Policy { - fn default() -> Self { - Self { - confirm_unprivileged: true, - } - } + /// When `true`, this daemon *may* offer "approve for this session" (`a`) + /// at an unprivileged prompt; a human keypress then grants unattended + /// unprivileged execution for the daemon's lifetime — never persisted. + /// When `false` (the default), every unprivileged command is prompted + /// individually and `a` is never offered. + /// + /// Default-false is fail-closed: an absent `policy` block, or a stale + /// `confirm_unprivileged` key from a pre-G7 config, deserializes to `false` + /// (serde ignores the unknown key), so the dangerous persisted opt-out from + /// older builds becomes inert on upgrade and the daemon prompts every + /// command until an operator deliberately edits the file. + #[serde(default)] + pub unattended_eligible: bool, } #[derive(Debug, Serialize, Deserialize, Clone, Default)] @@ -171,7 +176,7 @@ pub fn save_to(path: &Path, config: &HostsConfig) -> io::Result<()> { if let Some(parent) = path.parent() { std::fs::create_dir_all(parent)?; // hosts.json holds the host inventory, cached remote UIDs, and the - // confirm_unprivileged policy — owner-private data. Keep the + // unattended-eligibility policy — owner-private data. Keep the // directory owner-only (0700) so other local users can't enumerate // or read it, mirroring the socket-bind hardening in server.rs. let _ = std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700)); @@ -230,30 +235,37 @@ mod tests { } #[test] - fn policy_defaults_to_confirm_when_field_absent() { - // An older hosts.json (no `policy` block) must round-trip with - // confirm_unprivileged=true so behaviour matches pre-policy builds. + fn policy_defaults_to_not_eligible_when_absent() { + // An older hosts.json (no `policy` block) must deserialize fail-closed: + // not eligible, so every unprivileged command is prompted. let cfg: HostsConfig = serde_json::from_str(r#"{"hosts":{}}"#).unwrap(); - assert!(cfg.policy.confirm_unprivileged); + assert!(!cfg.policy.unattended_eligible); } #[test] - fn policy_with_confirm_false_loads() { + fn stale_confirm_unprivileged_key_is_inert() { + // G7 migration: a pre-G7 config that disabled the gate + // (`confirm_unprivileged:false`) must NOT survive the upgrade as an + // active unattended grant. Serde ignores the unknown key and + // `unattended_eligible` falls back to its fail-closed default. let cfg: HostsConfig = serde_json::from_str( r#"{"hosts":{},"policy":{"confirm_unprivileged":false}}"#, ) .unwrap(); - assert!(!cfg.policy.confirm_unprivileged); + assert!( + !cfg.policy.unattended_eligible, + "a stale confirm_unprivileged key must not enable eligibility" + ); } #[test] fn policy_round_trips_through_serde() { let mut cfg = HostsConfig::default(); - assert!(cfg.policy.confirm_unprivileged); - cfg.policy.confirm_unprivileged = false; + assert!(!cfg.policy.unattended_eligible); + cfg.policy.unattended_eligible = true; let s = serde_json::to_string(&cfg).unwrap(); let back: HostsConfig = serde_json::from_str(&s).unwrap(); - assert!(!back.policy.confirm_unprivileged); + assert!(back.policy.unattended_eligible); } #[test] @@ -266,12 +278,12 @@ mod tests { let path = dir.join("hosts.json"); let mut cfg = HostsConfig::default(); - cfg.policy.confirm_unprivileged = false; + cfg.policy.unattended_eligible = true; save_to(&path, &cfg).unwrap(); let s = std::fs::read_to_string(&path).unwrap(); let back: HostsConfig = serde_json::from_str(&s).unwrap(); - assert!(!back.policy.confirm_unprivileged); + assert!(back.policy.unattended_eligible); let _ = std::fs::remove_dir_all(&dir); } diff --git a/src/mcp.rs b/src/mcp.rs index d4bf167..7764230 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -168,7 +168,25 @@ impl McpProxy { return Ok(error_result(format!("invalid host: {e}"))); } } - let socket_path = socket_for_host(params.host.as_deref()); + // Fold any loopback/self alias to the local target (C9), so a + // self-target cannot route SSH-to-self around local policy. + let target = normalize_host(params.host.as_deref()); + + // Delegate-to-Bash (G7): an unprivileged command on the local machine + // is refused — the agent's own Bash tool already applies the same + // controls, so sudo-proxy running it would only be a second, weaker + // gate. sudo-proxy is for privilege escalation and for remote hosts. + if target.is_none() && !params.privileged { + return Ok(error_result( + "Refusing to run an unprivileged command on the local machine. \ + Use your Bash tool instead — it applies the same controls without a \ + second gate. sudo-proxy is for privilege escalation (privileged: true) \ + and for commands on remote hosts." + .to_string(), + )); + } + + let socket_path = socket_for_host(target.as_deref()); let timeout_ms = params.timeout.unwrap_or(DEFAULT_TIMEOUT_MS).min(MAX_TIMEOUT_MS); if !socket_path.exists() { @@ -189,7 +207,7 @@ impl McpProxy { } }; - let host_name = params.host.clone().unwrap_or_else(|| "localhost".into()); + let host_name = target.clone().unwrap_or_else(|| "localhost".into()); if params.forward_agent && params.privileged { return Ok(error_result( @@ -198,7 +216,7 @@ impl McpProxy { } let req = Request::new( - params.host.unwrap_or_default(), + target.unwrap_or_default(), "sudo-proxy-mcp".to_string(), pipeline, params.env.unwrap_or_default(), @@ -231,14 +249,17 @@ impl McpProxy { return Ok(error_result(format!("invalid host: {e}"))); } } - let result = match ¶ms.host { + // A loopback/self alias starts the LOCAL daemon, not an SSH-to-self + // tunnel (C9) — keeps `start_server` consistent with `execute` routing. + let target = normalize_host(params.host.as_deref()); + let result = match &target { None => start_local().await, Some(host) => start_remote(host, params.forward_agent).await, }; if let Ok(ref r) = result { if r.is_error != Some(true) { - let host_name = params.host.unwrap_or_else(|| "localhost".into()); + let host_name = target.unwrap_or_else(|| "localhost".into()); touch_host(&host_name, ""); } } @@ -341,19 +362,21 @@ impl McpProxy { // The registry key for the local daemon is "localhost" (written by // touch_host); it must be probed at the default socket, never at a // tunnel path. + let own = crate::server::own_hostname(); let targets: Vec> = match params.host { Some(h) => { if let Err(e) = crate::server::validate_host(&h) { return Ok(error_result(format!("invalid host: {e}"))); } - vec![if h == "localhost" { None } else { Some(h) }] + // A loopback/self alias probes the local daemon, not a tunnel. + vec![normalize_host(Some(&h))] } None => std::iter::once(None) .chain( config .hosts .keys() - .filter(|h| h.as_str() != "localhost") + .filter(|h| !crate::server::is_local_host(h, &own)) .cloned() .map(Some), ) @@ -1000,10 +1023,26 @@ fn decode_b64(s: Option<&str>) -> String { // Utilities // --------------------------------------------------------------------------- -fn socket_for_host(host: Option<&str>) -> PathBuf { +/// Normalize a caller-supplied `host` to the routing target: `None` for the +/// local daemon — including any loopback or self alias — and `Some(h)` for a +/// genuine remote. Folding self-aliases to `None` is the C9 half of invariant +/// G7: it stops `execute(host="127.0.0.1")` (or `::1`, or this machine's own +/// hostname) from opening an SSH-to-self tunnel that would route around local +/// policy and the Bash-delegation refusal. +fn normalize_host(host: Option<&str>) -> Option { + let own = crate::server::own_hostname(); match host { + Some(h) if !crate::server::is_local_host(h, &own) => Some(h.to_string()), + _ => None, + } +} + +fn socket_for_host(host: Option<&str>) -> PathBuf { + // Defence-in-depth: even if a caller reaches this with an un-normalized + // self-alias, resolve it to the local socket rather than a tunnel path. + match normalize_host(host) { None => default_socket_path(), - Some(h) => crate::server::remote_socket_path(h), + Some(h) => crate::server::remote_socket_path(&h), } } @@ -1051,3 +1090,32 @@ fn touch_host(host: &str, version: &str) { config.save(); } +#[cfg(test)] +mod tests { + use super::*; + + /// The routing contract behind C9 and the Bash-delegation refusal: a + /// loopback/self alias resolves to the local target (`None`), a genuine + /// remote stays `Some`. (The full self/loopback classification, including + /// the own-hostname cases, is covered exhaustively by `is_local_host` in + /// `server.rs`; here we only pin the None/Some routing decision.) + #[test] + fn normalize_host_folds_loopback_to_local() { + assert_eq!(normalize_host(None), None); + assert_eq!(normalize_host(Some("localhost")), None); + assert_eq!(normalize_host(Some("127.0.0.1")), None); + assert_eq!(normalize_host(Some("127.0.0.9")), None); + assert_eq!(normalize_host(Some("::1")), None); + assert_eq!(normalize_host(Some("user@localhost")), None); + // Genuine remotes and self-alias dodges route remote. + assert_eq!( + normalize_host(Some("example.com")), + Some("example.com".to_string()) + ); + assert_eq!( + normalize_host(Some("127.0.0.1.evil.com")), + Some("127.0.0.1.evil.com".to_string()) + ); + } +} + diff --git a/src/server.rs b/src/server.rs index 44a855d..f8820a2 100644 --- a/src/server.rs +++ b/src/server.rs @@ -11,7 +11,6 @@ use std::thread; use std::time::{Duration, Instant}; use crate::executor::{exec_direct, exec_pkexec, exec_sudo, sanitize_env}; -use crate::hosts::HostsConfig; use crate::mode::Mode; use crate::protocol::{Request, Response, ValidatedRequest}; use crate::tui::{self, Prompter, ResultSink}; @@ -190,6 +189,68 @@ pub fn validate_host(host: &str) -> Result<(), String> { Ok(()) } +/// This machine's hostname (from `/etc/hostname`), used by [`is_local_host`] to +/// recognise a request that names the local box. Falls back to an empty string +/// (which matches nothing) if the file is unreadable, keeping the classifier +/// fail-closed toward "remote". +pub fn own_hostname() -> String { + std::fs::read_to_string("/etc/hostname") + .map(|s| s.trim().to_string()) + .unwrap_or_default() +} + +/// True if `host` denotes the machine this proxy runs on, so a request naming +/// it must route to the *local* path rather than an SSH tunnel. This is the C9 +/// normaliser (invariant G7): it folds loopback literals and the machine's own +/// hostname to "local" so a self-target cannot masquerade as remote — e.g. +/// `execute(host="127.0.0.1")` must not open an SSH-to-self tunnel that routes +/// around local policy. +/// +/// Best-effort by construction: ssh-config aliases and NAT hairpin can hide a +/// self-target it cannot see. G7 does not rest on catching every alias (see +/// C10) — local unprivileged is delegated to the Bash tool and every remote +/// daemon still gates unattended execution, so an undetected self-alias routed +/// SSH-to-self still lands on a gated daemon. +/// +/// `own_hostname` is injected rather than read here, so the classification is a +/// pure function of its inputs and the property test stays hermetic. +pub fn is_local_host(host: &str, own_hostname: &str) -> bool { + // Drop an optional `user@` prefix and a single trailing FQDN dot; compare + // case-insensitively. + let h = host.rsplit('@').next().unwrap_or(host); + let h = h.strip_suffix('.').unwrap_or(h); + let h_lower = h.to_ascii_lowercase(); + + if h_lower == "localhost" { + return true; + } + // The whole 127.0.0.0/8 block is loopback — parse it, don't string-match, + // so `127.0.0.1.evil.com` is NOT treated as local. + if let Ok(v4) = h.parse::() { + return v4.octets()[0] == 127; + } + if let Ok(v6) = h.parse::() { + return v6.is_loopback(); + } + // The machine's own name — full FQDN, or a bare label matching our first + // label (so `box` matches a hostname of `box.local`). A dotted FQDN must + // match in full, so an unrelated `box.example.com` is not caught. An empty + // `own_hostname` (unreadable /etc/hostname) matches nothing. + if !own_hostname.is_empty() { + let own_lower = own_hostname.to_ascii_lowercase(); + if h_lower == own_lower { + return true; + } + if !h_lower.contains('.') { + let own_label = own_lower.split('.').next().unwrap_or(&own_lower); + if !own_label.is_empty() && h_lower == own_label { + return true; + } + } + } + false +} + /// Bounded set of recently-seen request ids, evicted by age. pub(crate) struct SeenIds { set: HashSet, @@ -301,7 +362,12 @@ pub struct ServerConfig { pub mode: Mode, pub pkexec_only: bool, pub verbose: bool, - pub confirm_unprivileged: bool, + /// Whether this daemon *may* offer the session-scoped `a` answer at an + /// unprivileged prompt (invariant G7, first barrier). Read from the + /// `unattended_eligible` policy in `hosts.json` at startup and immutable + /// thereafter — no wire field, MCP flag, or keypress changes it. Default + /// `false` (fail-closed): every unprivileged command is prompted. + pub unattended_eligible: bool, pub max_in_flight: usize, } @@ -311,11 +377,7 @@ impl Default for ServerConfig { mode: Mode::Local, pkexec_only: false, verbose: false, - // Human-in-the-loop is the marketed value of sudo-proxy. - // Unprivileged commands now go through the same Y/N gate as - // privileged ones by default; opt out via - // `--no-confirm-unprivileged` for batch/automation flows. - confirm_unprivileged: true, + unattended_eligible: false, max_in_flight: DEFAULT_MAX_IN_FLIGHT, } } @@ -343,9 +405,13 @@ pub fn run( let our_uid = unsafe { libc::getuid() }; let seen_ids = Arc::new(Mutex::new(SeenIds::new(Instant::now))); - // Shared across handler threads so an interactive `a` (ApprovedAlways) - // can flip the gate for subsequent requests without restarting. - let confirm_unprivileged = Arc::new(AtomicBool::new(config.confirm_unprivileged)); + // Eligibility is a runtime-immutable config input (barrier 1). The session + // grant (barrier 2) starts OFF and is the *only* thing an interactive `a` + // can flip — in memory only, never persisted. It dies with the daemon, + // i.e. with the SSH tunnel for a remote host, so the grant cannot outlive + // the session (invariant G7). + let unattended_eligible = config.unattended_eligible; + let unattended_grant = Arc::new(AtomicBool::new(false)); loop { if shutdown.load(Ordering::Relaxed) { @@ -397,7 +463,7 @@ pub fn run( let mode = config.mode; let pkexec_only = config.pkexec_only; let verbose = config.verbose; - let confirm_unprivileged = Arc::clone(&confirm_unprivileged); + let unattended_grant = Arc::clone(&unattended_grant); let shutdown = Arc::clone(&shutdown); thread::spawn(move || { let _guard = guard; @@ -407,7 +473,8 @@ pub fn run( mode, pkexec_only, verbose, - confirm_unprivileged, + unattended_eligible, + unattended_grant, prompter, sink, seen, @@ -425,7 +492,8 @@ fn handle_connection( mode: Mode, pkexec_only: bool, verbose: bool, - confirm_unprivileged: Arc, + unattended_eligible: bool, + unattended_grant: Arc, prompter: Arc, result_sink: Arc, seen_ids: Arc>, @@ -591,7 +659,7 @@ fn handle_connection( // by ForegroundGuard for the swap. let prompt_result = { let _g = lock_recover(&tty_lock); - prompter.prompt(&req, PROMPT_TIMEOUT) + prompter.prompt(&req, unattended_eligible, PROMPT_TIMEOUT) }; match prompt_result { Ok(tui::PromptResult::Approved) => exec_sudo(&req, &env, &tty_lock), @@ -607,18 +675,41 @@ fn handle_connection( } } } - } else if confirm_unprivileged.load(Ordering::Relaxed) { + } else if unattended_grant.load(Ordering::Relaxed) { + // A session-scoped grant is active: this daemon is eligible AND a human + // pressed `a` earlier in this session. Run unattended. The audit line + // is UNCONDITIONAL — in this no-prompt window it is the reliable record + // of what ran, and unlike the TTY banner it never depends on the + // tty_lock, so an unprivileged command is never blocked behind an + // in-flight privileged prompt. The TTY banner stays best-effort + // (try_lock) for on-screen visibility. The grant lives only in memory + // and dies with the daemon/tunnel; it is never persisted. + eprintln!( + "[{}] [unattended] {}", + req.id, + crate::tui::pipeline_join(&req.pipeline) + ); + if let Ok(_g) = tty_lock.try_lock() { + let _ = tui::display_banner(&req); + } + exec_direct(&req, &env) + } else { + // Default unprivileged path: prompt every command. `a` is offered only + // when eligible; on an eligible daemon an `a` press approves this + // command AND flips the in-memory session grant. When not eligible, + // `a` degrades to approve-once and grants nothing. let prompt_result = { let _g = lock_recover(&tty_lock); - prompter.prompt(&req, PROMPT_TIMEOUT) + prompter.prompt(&req, unattended_eligible, PROMPT_TIMEOUT) }; match prompt_result { Ok(tui::PromptResult::Approved) => exec_direct(&req, &env), Ok(tui::PromptResult::ApprovedAlways) => { - confirm_unprivileged.store(false, Ordering::Relaxed); - let mut hosts = HostsConfig::load(); - hosts.policy.confirm_unprivileged = false; - hosts.save(); + // Grant the session only when eligible — never persisted, no + // hosts.json write. Barrier 1 (eligibility) gates barrier 2. + if unattended_eligible { + unattended_grant.store(true, Ordering::Relaxed); + } exec_direct(&req, &env) } Ok(tui::PromptResult::Denied) => Response::denied(&req.id), @@ -628,16 +719,6 @@ fn handle_connection( Response::error(&req.id, &format!("prompt error: {e}")) } } - } else { - // Non-privileged, no confirmation: print a one-line banner so the - // user can see what the proxy is running on their behalf, then exec. - // Best-effort: try_lock so the banner never queues behind a - // long-running privileged prompt. Skipping the banner under - // contention is preferred to making `ls` wait on a human Y/N. - if let Ok(_g) = tty_lock.try_lock() { - let _ = tui::display_banner(&req); - } - exec_direct(&req, &env) }; // Echo result on the TTY. The privileged path takes the lock blocking @@ -839,6 +920,42 @@ mod tests { assert!(validate_host("root@10.0.0.1").is_ok()); } + #[test] + fn is_local_host_classifies_self_and_loopback() { + // Hermetic: own hostname is injected, never read from /etc/hostname. + let own = "box.local"; + // Local: canonical name, whole 127/8, IPv6 loopback, user@ and trailing + // dot forms, case-insensitivity, own hostname (full + bare first label). + for local in [ + "localhost", "LOCALHOST", "LocalHost", "localhost.", + "127.0.0.1", "127.0.0.5", "127.1.2.3", "user@127.0.0.1", + "::1", "user@::1", + "box.local", "BOX.LOCAL", "box", + ] { + assert!( + is_local_host(local, own), + "is_local_host({local:?}) should be local" + ); + } + // Remote: suffix/prefix/substring dodges must NOT match, and genuine + // remotes stay remote. An unrelated FQDN sharing our first label is + // remote (only a bare label or the full FQDN counts as self). + for remote in [ + "127.0.0.1.evil.com", "notlocalhost", "localhost.evil.com", + "example.com", "10.0.0.4", "128.0.0.1", "::2", + "box.example.com", "buildbox", + ] { + assert!( + !is_local_host(remote, own), + "is_local_host({remote:?}) should be remote" + ); + } + // An unreadable /etc/hostname (empty own) matches nothing by name, but + // loopback literals still resolve local. + assert!(is_local_host("127.0.0.1", "")); + assert!(!is_local_host("box", "")); + } + #[test] fn validate_host_rejects_shell_metacharacters() { // Each of these would, if `host` were ever interpolated into an diff --git a/src/tui.rs b/src/tui.rs index 129806e..e3168c2 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -78,17 +78,28 @@ pub fn truncate_for_display(s: &str) -> String { #[derive(Debug, PartialEq)] pub enum PromptResult { Approved, - /// Approve this request *and* grant log-only mode for unprivileged - /// commands going forward (until reverted). Only emitted for - /// unprivileged requests; privileged prompts never offer this. + /// Approve this request *and*, if the daemon is eligible, grant unattended + /// unprivileged execution for the rest of this session (in-memory, never + /// persisted). Only emitted for unprivileged requests; privileged prompts + /// never offer this. The dispatcher grants the session only when the + /// daemon's `unattended_eligible` policy is set — otherwise this approves + /// the single command and nothing more. ApprovedAlways, Denied, Timeout, } -/// Asks the user to approve or deny a privilege request. +/// Asks the user to approve or deny a privilege request. `eligible` reflects +/// the daemon's `unattended_eligible` policy: when set (and the request is +/// unprivileged) the prompt offers the session-scoped `a` answer, otherwise it +/// shows a plain `[y/N]`. pub trait Prompter: Send + Sync { - fn prompt(&self, req: &ValidatedRequest, timeout: Duration) -> io::Result; + fn prompt( + &self, + req: &ValidatedRequest, + eligible: bool, + timeout: Duration, + ) -> io::Result; } /// Echoes the result of a completed command back to the user. @@ -99,8 +110,13 @@ pub trait ResultSink: Send + Sync { pub struct TtyPrompter; impl Prompter for TtyPrompter { - fn prompt(&self, req: &ValidatedRequest, timeout: Duration) -> io::Result { - prompt_tty(req, timeout) + fn prompt( + &self, + req: &ValidatedRequest, + eligible: bool, + timeout: Duration, + ) -> io::Result { + prompt_tty(req, eligible, timeout) } } @@ -137,7 +153,13 @@ pub(crate) fn classify_key(key: Option, privileged: bool) -> PromptResult { } /// Display a privilege request on /dev/tty and ask for Y/N confirmation. -pub fn prompt_tty(req: &ValidatedRequest, timeout: Duration) -> io::Result { +/// `eligible` gates whether the session-scoped `a` answer is offered (only for +/// unprivileged requests on a daemon whose `unattended_eligible` policy is set). +pub fn prompt_tty( + req: &ValidatedRequest, + eligible: bool, + timeout: Duration, +) -> io::Result { let mut tty_w = OpenOptions::new().write(true).open("/dev/tty")?; let tty_r = File::open("/dev/tty")?; @@ -198,14 +220,22 @@ pub fn prompt_tty(req: &ValidatedRequest, timeout: Duration) -> io::Result io::Result "Timeout", PromptResult::Approved => "Approved", - PromptResult::ApprovedAlways => "Approved (always for this host)", + // `a` on an eligible daemon grants the session; pressed when `a` was + // not offered it degrades to a plain approve-once (the dispatcher will + // not grant), so label it honestly. + PromptResult::ApprovedAlways if offer_always => "Approved (unattended for this session)", + PromptResult::ApprovedAlways => "Approved", PromptResult::Denied => "Denied", }; writeln!(tty_w, "\n→ {label}")?; @@ -467,15 +501,16 @@ fn read_key_timeout(file: &File, timeout: Duration) -> io::Result> { mod tests { use super::*; - // === Property: confirm_unprivileged flips only on an interactive keypress + // === Property: the session grant is set only by an interactive `a` keypress // // Spec clause (Rung 2; proof obligation for the Rung 4 state-machine model - // and the Rung 5 dispatch contract): the *only* signal that can flip the - // `confirm_unprivileged` policy off is `ApprovedAlways`, and `classify_key` - // emits `ApprovedAlways` iff the keypress is `'a'`/`'A'` AND the request is - // unprivileged. No timeout, no other key, and no privileged request can - // produce it. The dispatch side of this clause (only `ApprovedAlways` - // stores the flag) is covered by tests/approval.rs. + // and the Rung 5 dispatch contract): the *only* signal that can set the + // session-scoped unattended grant (invariant G7, barrier 2) is + // `ApprovedAlways`, and `classify_key` emits `ApprovedAlways` iff the + // keypress is `'a'`/`'A'` AND the request is unprivileged. No timeout, no + // other key, and no privileged request can produce it. The dispatch side — + // that `ApprovedAlways` grants only when the daemon is *eligible*, never + // persists, and grants nothing otherwise — is covered by tests/approval.rs. // // The input domain `(Option, bool)` is tiny, so this is checked // exhaustively rather than sampled. diff --git a/tests/approval.rs b/tests/approval.rs index ec8fbcd..a1884fc 100644 --- a/tests/approval.rs +++ b/tests/approval.rs @@ -7,11 +7,14 @@ //! request is always routed through the prompter, and root is executed only //! on `Approved`. Every non-`Approved` outcome (`Denied`, `Timeout`, and the //! defensively-rejected `ApprovedAlways`) ends in no execution. -//! * **Property 5 (dispatch side) — `confirm_unprivileged` flips only on an -//! interactive keypress.** The policy flag turns off only when the prompter -//! returns `ApprovedAlways` (which `tui::classify_key` emits solely for the -//! `'a'` key on an unprivileged request — see the exhaustive unit property -//! in `src/tui.rs`). A plain `Approved` leaves the flag set. +//! * **Property 5 (dispatch side) — unattended runs only behind two barriers +//! (G7/C8).** No unprivileged command runs unattended unless the daemon is +//! `unattended_eligible` (barrier 1, config-only — an out-of-band operator +//! opt-in) AND a human answered `ApprovedAlways` (barrier 2, the `'a'` key, +//! which `tui::classify_key` emits solely for an unprivileged request). The +//! resulting grant is session-scoped and **never persisted**. On a +//! non-eligible daemon `ApprovedAlways` approves the one command and grants +//! nothing. #![cfg(unix)] @@ -77,66 +80,80 @@ fn privileged_approved_always_is_rejected_not_granted() { assert_eq!(s.prompter.call_count(), 1); } -// --- Property 5 (dispatch side): confirm_unprivileged flips only on 'a' --- +// --- Property 5 (dispatch side): two barriers, grant never persisted -------- #[test] -fn plain_approve_keeps_confirm_unprivileged_set() { - // confirm_unprivileged starts true; a plain Approved on an unprivileged - // request must leave it set, so the *next* unprivileged request is still - // prompted. - let s = start_test_server(TestServerOpts { - confirm_unprivileged: true, - ..Default::default() - }); - // Default ScriptedPrompter approves (plain) immediately. +fn every_unprivileged_request_is_prompted_when_not_eligible() { + // The default daemon is not eligible: every unprivileged request is + // prompted, so a plain Approved on one leaves the next still prompted. + let s = start_test_server(TestServerOpts::default()); let r1 = s.send(&make_req("u-approve-1", vec![vec!["true"]])); let r2 = s.send(&make_req("u-approve-2", vec![vec!["true"]])); assert_eq!(r1.status, Status::Ok); assert_eq!(r2.status, Status::Ok); - // Both requests were prompted: the flag never flipped. - assert_eq!(s.prompter.call_count(), 2); + assert_eq!(s.prompter.call_count(), 2, "both must be prompted"); +} + +#[test] +fn non_eligible_approved_always_grants_nothing() { + // Barrier 1: on a non-eligible daemon, even an ApprovedAlways answer + // approves the single command but grants no session — the next request is + // still prompted. This is the clause that makes `a` un-self-grantable + // without the out-of-band config opt-in. + let s = start_test_server(TestServerOpts::default()); + s.prompter + .set_response(|_| (Duration::ZERO, PromptResult::ApprovedAlways)); + + let r1 = s.send(&make_req("u-a-1", vec![vec!["true"]])); + let r2 = s.send(&make_req("u-a-2", vec![vec!["true"]])); + + assert_eq!(r1.status, Status::Ok); + assert_eq!(r2.status, Status::Ok); + assert_eq!( + s.prompter.call_count(), + 2, + "ApprovedAlways must not grant a session when the daemon is not eligible" + ); } #[test] -fn approved_always_flips_confirm_unprivileged_off() { - // Isolate the config directory: the ApprovedAlways dispatch path persists - // the flipped policy via HostsConfig::save(), which writes - // $XDG_CONFIG_HOME/sudo-proxy/hosts.json. Point it at a temp dir so the - // test never touches the real user config. (set_var is process-global; - // only this test in the binary writes config, so there is no race.) +fn eligible_approved_always_grants_session_but_never_persists() { + // Isolate the config dir so we can assert the grant is NOT written to + // hosts.json. (set_var is process-global; only this test in the binary + // touches XDG_CONFIG_HOME.) let cfg_dir = tempfile::tempdir_in("/tmp").expect("tempdir"); std::env::set_var("XDG_CONFIG_HOME", cfg_dir.path()); let s = start_test_server(TestServerOpts { - confirm_unprivileged: true, + unattended_eligible: true, ..Default::default() }); s.prompter .set_response(|_| (Duration::ZERO, PromptResult::ApprovedAlways)); - // First unprivileged request: prompted, answered ApprovedAlways -> runs - // and flips the flag off. + // First unprivileged request: prompted, answered ApprovedAlways -> runs and + // flips the in-memory session grant. let r1 = s.send(&make_req("u-always-1", vec![vec!["true"]])); assert_eq!(r1.status, Status::Ok); assert_eq!(s.prompter.call_count(), 1); - // Second unprivileged request: the flag is now off, so dispatch runs the - // command without prompting. call_count must NOT increase. + // Second unprivileged request: granted, so dispatch runs it without + // prompting. call_count must NOT increase. let r2 = s.send(&make_req("u-always-2", vec![vec!["true"]])); assert_eq!(r2.status, Status::Ok); assert_eq!( s.prompter.call_count(), 1, - "after ApprovedAlways the flag is off, so no further prompt should occur" + "after the session grant, no further prompt should occur" ); - // The flipped policy was persisted to the isolated config, not the real - // user config. - let saved = std::fs::read_to_string(cfg_dir.path().join("sudo-proxy").join("hosts.json")) - .expect("policy was persisted to the isolated config dir"); - assert!( - saved.contains("\"confirm_unprivileged\""), - "persisted config should record the policy flag: {saved}" - ); + // The grant lives only in memory: nothing was persisted to hosts.json. + let cfg = cfg_dir.path().join("sudo-proxy").join("hosts.json"); + if let Ok(saved) = std::fs::read_to_string(&cfg) { + assert!( + !saved.contains("unattended_eligible"), + "the session grant must never be persisted: {saved}" + ); + } } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 33e1e5f..4113e26 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -11,7 +11,7 @@ use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use base64::engine::general_purpose::STANDARD as B64; use base64::Engine; use sudo_proxy::mode::Mode; -use sudo_proxy::protocol::{Request, Response, ValidatedRequest}; +use sudo_proxy::protocol::{Request, Response, Status, ValidatedRequest}; use sudo_proxy::server; use sudo_proxy::tui::{Prompter, PromptResult, ResultSink}; use tempfile::TempDir; @@ -61,7 +61,12 @@ impl ScriptedPrompter { } impl Prompter for ScriptedPrompter { - fn prompt(&self, req: &ValidatedRequest, _timeout: Duration) -> std::io::Result { + fn prompt( + &self, + req: &ValidatedRequest, + _eligible: bool, + _timeout: Duration, + ) -> std::io::Result { self.calls.lock().unwrap().push(RecordedCall { req: req.inner().clone(), at: Instant::now(), @@ -98,7 +103,10 @@ impl ResultSink for RecordingSink { } pub struct TestServerOpts { - pub confirm_unprivileged: bool, + /// Barrier 1 of G7: whether the daemon may offer the session-scoped `a` + /// answer. Default `false` (fail-closed) — every unprivileged command is + /// prompted. Set `true` to exercise the granted (unattended) path. + pub unattended_eligible: bool, pub pkexec_only: bool, pub mode: Mode, pub max_in_flight: usize, @@ -107,7 +115,7 @@ pub struct TestServerOpts { impl Default for TestServerOpts { fn default() -> Self { Self { - confirm_unprivileged: true, + unattended_eligible: false, pkexec_only: false, mode: Mode::Local, max_in_flight: sudo_proxy::server::DEFAULT_MAX_IN_FLIGHT, @@ -147,7 +155,7 @@ pub fn start_test_server(opts: TestServerOpts) -> TestServer { mode: opts.mode, pkexec_only: opts.pkexec_only, verbose: false, - confirm_unprivileged: opts.confirm_unprivileged, + unattended_eligible: opts.unattended_eligible, max_in_flight: opts.max_in_flight, }; // Coercion to Arc happens here at the function-argument @@ -198,6 +206,21 @@ impl TestServer { send_request(&self.socket_path, req) } + /// Establish the session-scoped unattended grant. The server must have been + /// started with `unattended_eligible: true`; this sends one warmup + /// unprivileged request answered `ApprovedAlways`, after which unprivileged + /// requests bypass the prompter for the rest of the session. The warmup + /// counts as one prompter call. Leaves the prompter set to auto-approve; + /// callers needing a different response set it afterward. + pub fn grant_unattended_session(&self) { + self.prompter + .set_response(|_| (Duration::ZERO, PromptResult::ApprovedAlways)); + let resp = self.send(&make_req("grant-warmup", vec![vec!["true"]])); + assert_eq!(resp.status, Status::Ok, "warmup grant request should succeed"); + self.prompter + .set_response(|_| (Duration::ZERO, PromptResult::Approved)); + } + pub fn send_raw(&self, line: &[u8]) -> Vec { send_raw(&self.socket_path, line) } diff --git a/tests/concurrency.rs b/tests/concurrency.rs index 07ad1f8..8699c85 100644 --- a/tests/concurrency.rs +++ b/tests/concurrency.rs @@ -70,7 +70,6 @@ fn long_exec_does_not_wedge_loop() { return; } let opts = TestServerOpts { - confirm_unprivileged: false, ..Default::default() }; let s = start_test_server(opts); @@ -111,17 +110,20 @@ fn long_exec_does_not_wedge_loop() { ); } -/// Failure-mode-3 evidence in fast form: an unprivileged request that -/// doesn't take the prompt path runs to completion while a privileged -/// request is still in its (slow) prompt. Direct proof that the daemon -/// no longer serializes ALL traffic behind the TTY. +/// Failure-mode-3 evidence in fast form: an unprivileged request on the +/// granted (unattended) path runs to completion while a privileged request is +/// still in its (slow) prompt. Direct proof that the granted path's reliable +/// log never takes the tty_lock blocking, so it doesn't serialize behind the +/// TTY. (In the default non-eligible config an unprivileged command DOES prompt +/// and would serialize — that is correct: one human, one terminal.) #[test] fn unprivileged_runs_during_privileged_prompt() { let opts = TestServerOpts { - confirm_unprivileged: false, + unattended_eligible: true, ..Default::default() }; let s = start_test_server(opts); + s.grant_unattended_session(); // warmup counts as one prompter call // Returning Denied avoids triggering exec_sudo for A (which would // need a real sudo configuration). The slow delay holds the TTY @@ -138,10 +140,11 @@ fn unprivileged_runs_during_privileged_prompt() { send_request(&path_a, &req) }); - assert!(wait_until(Duration::from_secs(2), || s.prompter.call_count() >= 1)); + // Warmup already counted as 1; wait until A is also in the prompter. + assert!(wait_until(Duration::from_secs(2), || s.prompter.call_count() >= 2)); let t_b = thread::spawn(move || { - // Unprivileged + confirm_unprivileged=false → skips prompter entirely. + // Unprivileged + granted → bypasses the prompter entirely. let req = make_req("unpriv-B", vec![vec!["true"]]); let start = Instant::now(); let resp = send_request(&path_b, &req); @@ -230,20 +233,26 @@ fn prompter_returns_denied_propagates_to_client() { assert_eq!(resp.status, Status::Denied); } +/// Once a session is granted (eligible daemon + one `a` keypress), later +/// unprivileged commands bypass the prompter for the rest of the session. The +/// grant is in-memory only — see `approval.rs` for the non-persistence and +/// eligibility-required guarantees (C8). #[test] -fn unprivileged_no_confirm_skips_prompter() { +fn granted_session_skips_prompter() { let opts = TestServerOpts { - confirm_unprivileged: false, + unattended_eligible: true, ..Default::default() }; let s = start_test_server(opts); + s.grant_unattended_session(); // one warmup prompter call establishes the grant + let before = s.prompter.call_count(); let resp = s.send(&make_req("noprompt-1", vec![vec!["true"]])); assert_eq!(resp.status, Status::Ok); assert_eq!( s.prompter.call_count(), - 0, - "prompter must not be called when confirm_unprivileged=false" + before, + "a granted session must not call the prompter again" ); } @@ -315,7 +324,6 @@ fn concurrent_distinct_ids_succeed() { return; } let opts = TestServerOpts { - confirm_unprivileged: false, ..Default::default() }; let s = start_test_server(opts); @@ -353,7 +361,6 @@ fn concurrent_distinct_ids_succeed() { fn burst_connections_above_cap_get_busy_response() { let opts = TestServerOpts { max_in_flight: 4, - confirm_unprivileged: false, ..Default::default() }; let s = start_test_server(opts); diff --git a/tests/forward_agent.rs b/tests/forward_agent.rs index 8535b5b..1ddf9a5 100644 --- a/tests/forward_agent.rs +++ b/tests/forward_agent.rs @@ -17,7 +17,6 @@ static ENV_MUTATION_LOCK: Mutex<()> = Mutex::new(()); fn server() -> TestServer { start_test_server(TestServerOpts { - confirm_unprivileged: false, ..TestServerOpts::default() }) } diff --git a/tests/hosts.rs b/tests/hosts.rs index d21bc97..53160a5 100644 --- a/tests/hosts.rs +++ b/tests/hosts.rs @@ -46,7 +46,7 @@ fn concurrent_save_is_atomic_and_loses_no_data() { } /// hosts.json carries the host inventory, cached UIDs, and the -/// confirm_unprivileged policy — it must be owner-only (0600), and its +/// unattended_eligible policy — it must be owner-only (0600), and its /// directory owner-only (0700), regardless of the caller's umask. #[test] fn saved_hosts_file_is_owner_only() { diff --git a/tests/slow.rs b/tests/slow.rs index b67b58f..b7047fd 100644 --- a/tests/slow.rs +++ b/tests/slow.rs @@ -19,10 +19,10 @@ use common::*; /// B's connection thread starts immediately on accept; freshness is /// checked while B is still fresh, and B succeeds. /// -/// Setup: confirm_unprivileged=false, so B (privileged=false) skips the -/// prompter entirely and runs `exec_direct` while A (privileged=true) -/// is still in its 65s prompt. A's prompter returns Denied so we don't -/// drag a real `sudo` into the test. +/// Setup: an eligible daemon with a granted session, so B (privileged=false) +/// bypasses the prompter and runs `exec_direct` while A (privileged=true) is +/// still in its 65s prompt. A's prompter returns Denied so we don't drag a real +/// `sudo` into the test. /// /// Runs ~65s; gated behind --ignored. Real time-passage on A's prompt /// is what proves the freshness check is decoupled from prompt @@ -31,10 +31,11 @@ use common::*; #[ignore] fn request_queued_behind_slow_prompt_completes_normally() { let opts = TestServerOpts { - confirm_unprivileged: false, + unattended_eligible: true, ..Default::default() }; let s = start_test_server(opts); + s.grant_unattended_session(); // warmup counts as one prompter call s.prompter .set_response(|_| (Duration::from_secs(65), PromptResult::Denied)); @@ -49,13 +50,14 @@ fn request_queued_behind_slow_prompt_completes_normally() { }); // Wait until A has actually entered the prompter (so the TTY lock is - // taken and we're in the wedge window). - assert!(wait_until(Duration::from_secs(5), || s.prompter.call_count() == 1)); + // taken and we're in the wedge window). The grant warmup already counted + // as call 1, so A entering the prompter is call 2. + assert!(wait_until(Duration::from_secs(5), || s.prompter.call_count() == 2)); let t_b = thread::spawn(move || { - // privileged=false + confirm_unprivileged=false → no prompter, - // no TTY lock. With the fix, this thread runs to completion - // while A's 65s prompt is still active. + // privileged=false + granted session → no prompter, no TTY lock. + // With the fix, this thread runs to completion while A's 65s prompt + // is still active. let req = make_req("queue-B", vec![vec!["true"]]); let start = Instant::now(); let resp = send_request(&path_b, &req); diff --git a/tests/transport.rs b/tests/transport.rs index 9e49f51..d42c789 100644 --- a/tests/transport.rs +++ b/tests/transport.rs @@ -96,7 +96,7 @@ fn replaces_stale_socket_file() { let handle = thread::spawn(move || { let config = server::ServerConfig { - confirm_unprivileged: true, + unattended_eligible: false, ..Default::default() }; server::run(