From a1c713fa5e4f279b50025577565cb1e2945c691e Mon Sep 17 00:00:00 2001 From: jg Date: Sat, 25 Jul 2026 20:34:33 -0500 Subject: [PATCH 1/2] fix(sec): untrusted text cannot drive the operator's terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A terminal reads text as a command language. Model prose, tool output, kernel audit targets, and skill metadata are all attacker-reachable, and every one of them reached the screen verbatim: `ESC [ 2 J` clears it, `ESC ] 0 ; … BEL` retitles the window, `\r` rewrites the line just printed, and a bidi override reorders characters after they are drawn. The most valuable thing to forge that way is the y/N capability prompt, which is the last thing standing between a project-supplied skill and a granted capability (f040, f046, absorbing f047). `safe_text` escapes control characters, C1 introducers, and bidi/invisible formatting characters into their textual Rust form (`\x1b`, `\u{202e}`), leaving clean text borrowed rather than copied. `safe_block` keeps `\n`/`\t` for prose; `safe_line` escapes those too, so untrusted text interpolated into a composed line cannot start a line of its own and impersonate the harness. It is applied where untrusted text *enters* a front-end, before any styling — sanitize the payload, then paint it, never the reverse, or the escaping would eat bee's own color: * `repl::terminal` — every text method of the `ReplOutput` impl, including the DENIED audit line, whose target the model chooses. * `tui::app::handle_session` — ratatui stores each grapheme as a cell symbol and flushes it, so the alternate screen is no shield. The triage did not name the TUI, but it shares the exposure exactly. * `app::repl::prompt_consent` — the consent boundary itself, which escapes what it prints rather than trusting an upstream check. Skill frontmatter goes further and fails closed: a `name`, `description`, required tool, or filesystem path carrying a control character is refused at load. Metadata is identity, it is short, and it is what the consent prompt quotes; no legitimate skill needs a character that moves a cursor. The body is markdown and stays as authored — it is escaped where it is displayed. Also deletes docs/NEXT-read-write-modes-PROMPT.md, which briefs work that landed some time ago (read/write mode enforcement in crates/ebpf/src/main.rs, the fail-closed BTF offset guard in crates/userspace/src/kbtf.rs). Co-Authored-By: Claude Opus 5 (1M context) --- TRIAGE.json | 24 ++++- TRIAGE.md | 1 + docs/NEXT-read-write-modes-PROMPT.md | 107 ----------------------- docs/design-system.md | 8 ++ src/app/repl.rs | 15 +++- src/lib.rs | 2 + src/repl/terminal.rs | 99 +++++++++++++++++++-- src/safe_text.rs | 126 +++++++++++++++++++++++++++ src/skills.rs | 68 +++++++++++++++ src/tui/app.rs | 66 ++++++++++++-- 10 files changed, 393 insertions(+), 123 deletions(-) delete mode 100644 docs/NEXT-read-write-modes-PROMPT.md create mode 100644 src/safe_text.rs diff --git a/TRIAGE.json b/TRIAGE.json index 654ff40..e2adde1 100644 --- a/TRIAGE.json +++ b/TRIAGE.json @@ -577,7 +577,12 @@ "owner_hint": "top committer: jg (1/1 recent commits); no CODEOWNERS entry", "missing_fields": [ "preconditions" - ] + ], + "remediation": { + "status": "fixed", + "where": "014-terminal-safety", + "note": "Untrusted text is escaped where it enters a front-end (`safe_text::safe_block`/`safe_line`, applied in repl::terminal's ReplOutput impl and tui::app::handle_session) and only then styled; the interactive consent prompt escapes every field it prints; and skill frontmatter carrying control or bidi characters is refused at load." + } }, { "id": "f041", @@ -1122,7 +1127,12 @@ "owner_hint": "top committer: jg (1/1 recent commits); no CODEOWNERS entry", "missing_fields": [ "preconditions" - ] + ], + "remediation": { + "status": "fixed", + "where": "014-terminal-safety", + "note": "Untrusted text is escaped where it enters a front-end (`safe_text::safe_block`/`safe_line`, applied in repl::terminal's ReplOutput impl and tui::app::handle_session) and only then styled; the interactive consent prompt escapes every field it prints; and skill frontmatter carrying control or bidi characters is refused at load." + } }, { "id": "f051", @@ -2132,6 +2142,16 @@ "status": "fixed", "where": "013-attenuation-inheritance", "note": "Policy::derive now returns the effective child policy \u2014 an omitted filesystem/exec/network dimension is inherited from the parent, parent deny rules and inode pins are re-added, and a child grant reaching into an FR-008 protected region is refused (research R15)." + }, + { + "findings": [ + "f040", + "f046", + "f047" + ], + "status": "fixed", + "where": "014-terminal-safety", + "note": "Untrusted text is escaped where it enters a front-end (`safe_text::safe_block`/`safe_line`, applied in repl::terminal's ReplOutput impl and tui::app::handle_session) and only then styled; the interactive consent prompt escapes every field it prints; and skill frontmatter carrying control or bidi characters is refused at load." } ] } diff --git a/TRIAGE.md b/TRIAGE.md index 4848781..3968fa1 100644 --- a/TRIAGE.md +++ b/TRIAGE.md @@ -13,6 +13,7 @@ are left in place unedited for provenance. |----------|-----| | f018, f020, f022, f026, f028, f030 (all six HIGHs) | `8e2cdbb` + `3225d44` — closed and VM-verified (31/31 matrix) | | f001, f002, f012, f014, f025 (+ absorbed f006, f032) | branch `013-attenuation-inheritance` — one root cause: attenuation validated only what a child *stated*, so omission widened authority. `Policy::derive` now returns the *effective* child policy (silence inherits, it does not reset) and refuses child grants reaching into FR-008 protected regions. See research R15; regression tests in `crates/core/tests/attenuation.rs`. | +| f040, f046 (+ absorbed f047) | branch `014-terminal-safety` — untrusted text is escaped where it enters a front-end (`safe_text`, applied in `repl::terminal` and `tui::app::handle_session`) and only then styled; the consent prompt escapes every field it prints; skill frontmatter carrying control or bidi characters is refused at load. | ## Act on these ### [HIGH] Provider TOML can send an arbitrary environment secret to an attacker endpoint (f018) diff --git a/docs/NEXT-read-write-modes-PROMPT.md b/docs/NEXT-read-write-modes-PROMPT.md deleted file mode 100644 index e6ba206..0000000 --- a/docs/NEXT-read-write-modes-PROMPT.md +++ /dev/null @@ -1,107 +0,0 @@ -# Task: enforce read/write/deny modes in bee's `file_open` hook - -You are continuing work on **bee**, a Rust + eBPF-LSM sandbox for AI coding agents, at -`/home/jg/git/bee`. Read this whole prompt before touching code. The core is done and tested; your job -is one focused increment on the kernel filesystem hook. - -## What bee already does (don't rebuild it) - -- 6-crate Cargo workspace. `bee-core` (policy/compiler/attenuation, 47 host tests), `bee-common` - (`#[repr(C)]` map layouts + matcher primitives, `no_std`), `bee-ebpf` (LSM programs, **excluded from - the workspace**), `bee-userspace` (aya loader/maps/spawn), `bee-cli`, `bee-hardening`. -- Three LSM hooks enforce on a real kernel today: `file_open` (path deny — subtree + postfix `*.ext`), - `socket_connect` (address allowlist), `bprm_check_security` (exec allowlist). Per-cgroup scoping, - observe/dry-run mode, JSON audit over a ring buffer, subagent attenuation (`bee run --parent`). -- Full design + status: `specs/001-ebpf-agent-sandbox/` (spec.md, plan.md, research.md, tasks.md — - 45/53 done). Read `plan.md` and `research.md` (esp. R5/R6) first. - -## The gap you're closing - -Filesystem enforcement is currently **deny-list only**. A policy rule's *mode* (`read`/`write`/`deny`) -is not fully honored: `deny` blocks, but a `read`-marked path does **not block writes** to it. So -US2's AS-2 ("subagent gets read-only project source; writes are denied") is not yet enforced in-kernel. -Your task: make the `file_open` hook enforce read-only — **block write-opens of paths marked `read`**, -while allowing reads. This completes FR-001 and US2 AS-2. - -### The mechanism -- `file_open`'s hook argument is `struct file *`. The requested access is in `file->f_mode` - (`FMODE_WRITE` bit = `0x2`). You need `offsetof(struct file, f_mode)` — get it from the target - kernel's BTF: `pahole -C file | grep f_mode` (run it in the test VM, see below). Add it as a - compile-time `const` next to `FILE_F_PATH_OFF = 152` in `bee-ebpf/src/main.rs`, and read it with a - **direct load** (`*((file as usize + off) as *const )`) — NOT `bpf_probe_read` (that - yields an untyped scalar; see gotchas). Mask/So bound any derived index. -- Extend the per-scope rule model so the kernel knows, for each path rule, its **mode**. Today - `FS_DENY` (`DenyList` of `DenyRule{kind,len,bytes}` in `bee-common/src/layout.rs`) carries deny - prefixes only. Add a `mode` byte to `DenyRule` (or a parallel "read-only rules" list), populated by - `Engine::create_scope` in `bee-userspace/src/lib.rs` from the compiled `PolicySet.fs` - (`FsPrimitive::Prefix { mode, .. }` — use `bee_common::AccessMode::is_deny()` / check for read-only). -- In `file_open`: after resolving the path (via `bpf_d_path`), find the **most-specific** matching - rule and apply: `deny` → block; `read` → block iff the open requests write; `write` → allow. - -### The design decision you MUST make explicitly (and document) -What is the default when **no rule matches**? The constitution says deny-by-default, but pure -deny-by-default on reads breaks every toolchain (can't open libc, etc.). The pragmatic, defensible -model (used by real agent sandboxes) is: -- **Writes**: deny-by-default — allowed only where a `write` rule grants it. -- **Reads**: allow-by-default — allowed unless a `deny` rule matches. - -Decide this deliberately, implement it, and write it down in `research.md` (a new decision entry) and -the policy schema contract. If you diverge from the above, justify it against the constitution -(`.specify/memory/constitution.md`, Principle I). Do not silently pick a default. - -## Critical gotchas (learned the hard way — do not rediscover) - -1. **Verifier instruction budget (1M).** Do NOT scan the 4 KB path buffer for a NUL to get its length - — it explodes the budget. `bpf_d_path` **returns** the length (bytes incl. NUL); use that - (`resolved_len()` already exists). -2. **Unbounded memory access.** Any buffer index derived from a runtime value must be masked - `& (PATH_MAX - 1)` (PATH_MAX = 4096, a power of two) or the verifier rejects it. -3. **Struct field reads need constant offsets + direct loads.** The verifier requires *constant* - offsets into a trusted BTF pointer; `bpf_probe_read` returns an untyped scalar that `bpf_d_path` - won't accept. Offsets are hardcoded per-kernel — **CO-RE auto-relocation is not achievable** with - aya-ebpf 0.2.1 (bindgen emits no `preserve_access_index`); see the `bee-ebpf` module docs. -4. **Fail closed, always.** If a rule kind/mode can't be enforced in-kernel, make `create_scope` - **refuse** (return `ScopeError`) rather than silently under-enforce — matches the existing - segment/bounded-star handling. -5. `pre_exec` (post-fork) code must be async-signal-safe: raw syscalls only, no allocation/locks. - -## Build + test loop (host builds, VM runs) - -The eBPF only enforces on a real BPF-LSM kernel. There's a KubeVirt VM for this — see the memory file -`~/.claude/projects/-home-jg-git-bee/memory/bee-bpf-lsm-test-vm.md` for full access details -(`virtctl ssh ubuntu@vmi/ac-matrix-vm/ac-matrix -i ~/.ssh/ac-matrix-vm`). The host has the nightly bpf -toolchain and the same glibc as the VM, so: - -```bash -# iterate the eBPF quickly (catches verifier-facing compile errors, not verifier itself): -cd bee-ebpf && RUSTFLAGS="-C link-arg=--btf" cargo +nightly build \ - --target bpfel-unknown-none -Z build-std=core --release - -cargo build -p bee-cli --features enforce --release # builds+embeds the eBPF -cargo test --workspace --tests # default host tests must stay green -cargo clippy --workspace # and --features enforce; keep both clean - -bash test/vm/matrix.sh # host: build → ship → run the 17-case matrix, gated exit -BEE_SKIP_BUILD=1 bash test/vm/matrix.sh # reuse target/release/bee -``` - -Run the harness in the background (it makes many VM round-trips) and wait for it — don't foreground it. -If the eBPF fails to load, capture the verifier log: `sudo bee run --policy

-- true 2>&1 | tail -20`. - -## Definition of done - -- [ ] `file_open` enforces read-only: writing to a `read`-marked subtree is denied (`EACCES`), reading - it succeeds, and the default-access decision is implemented as chosen. -- [ ] `create_scope` fails closed on any file mode/kind it can't enforce. -- [ ] New harness cases in `test/vm/remote-matrix.sh` prove it: e.g. a policy granting `write` to a - scratch dir and `read` to a source dir — writing the source file is blocked, reading it works, - writing scratch works. Update the case count in `test/vm/README.md`. -- [ ] Default `cargo test`/clippy green in both build modes; harness all-green on the VM. -- [ ] The default-access decision recorded in `research.md` + policy schema contract. -- [ ] Mark the relevant task(s) `[X]` in `specs/001-ebpf-agent-sandbox/tasks.md`; update the memory - file with any new verifier lessons. - -## Other open items (not this task, for context) -Segment (`**/name`) / bounded-star globs via `bpf_loop`; the boot-time **offset fail-closed guard** -(read running kernel BTF, refuse if compiled offsets don't match — closes the CO-RE fail-open); -5µs perf benchmark (SC-003); GitHub CI wiring the harness (T006). diff --git a/docs/design-system.md b/docs/design-system.md index 35a6b4d..99999e0 100644 --- a/docs/design-system.md +++ b/docs/design-system.md @@ -29,6 +29,13 @@ These are non-negotiable and already hold across the codebase — keep them hold basic-ANSI output; Catppuccin ×4, Dracula, and Nord ship alongside it. 5. **Spatial stability.** Banners, status lines, and dot grids keep fixed positions and fixed column order. Chrome doesn't rearrange itself between frames. +6. **The terminal is ours; untrusted text is data.** Model prose, tool output, audit targets, and + skill metadata are attacker-reachable, and a terminal reads text as a command language — an ESC + can clear the screen, a `\r` can rewrite the line above, a bidi override can reorder what was + already drawn. Every such string passes through `safe_text::safe_block`/`safe_line` at the point + it enters a front-end (`repl::terminal`'s `ReplOutput` impl; `tui::app::handle_session`), and + only *then* gets styled. Sanitize the payload, then paint it — never the reverse, or the escaping + would eat bee's own color. ## Identity @@ -212,3 +219,4 @@ not get to pick bee's. - [ ] Motion is decoration: with `BEE_NO_ANIMATION` set, the same content is on screen immediately. - [ ] Any new effect goes through `tui::effects::resolve` — never registered at a call site directly. - [ ] New chrome effects register **unkeyed**, so the agent cannot address them. +- [ ] Untrusted text is sanitized where it enters the front-end, before any styling is applied. diff --git a/src/app/repl.rs b/src/app/repl.rs index 9d8a0a6..5633dfc 100644 --- a/src/app/repl.rs +++ b/src/app/repl.rs @@ -380,12 +380,21 @@ fn terminal_rows() -> Option { /// session's whole contribution to the otherwise-shared grant path. fn prompt_consent(request: &bee::skills::GrantRequest<'_>) -> bool { use std::io::Write; - eprintln!("\nskill '{}' requests extra capabilities:", request.skill); + // Every field here is skill-authored. Skill loading already refuses control characters in + // frontmatter, but this prompt is the consent boundary itself and takes a `GrantRequest` from + // any source, so it escapes what it prints rather than trusting an upstream check: a forged + // prompt is a granted capability. One line per field, so nothing can smuggle in a second line. + let safe = bee::safe_text::safe_line; + eprintln!( + "\nskill '{}' requests extra capabilities:", + safe(request.skill) + ); if !request.tools.is_empty() { - eprintln!(" tools: {}", request.tools.join(", ")); + let tools: Vec<_> = request.tools.iter().map(|t| safe(t)).collect(); + eprintln!(" tools: {}", tools.join(", ")); } for (path, access) in request.filesystem { - eprintln!(" filesystem: {path} = {access}"); + eprintln!(" filesystem: {} = {}", safe(path), safe(access)); } eprint!("grant these (within the ceiling)? [y/N] "); let _ = std::io::stderr().flush(); diff --git a/src/lib.rs b/src/lib.rs index b7cf7be..a9814e9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -33,6 +33,8 @@ pub mod provider; pub mod render_api; pub mod render_spec; pub mod repl; +// Terminal-safety for untrusted text. Ungated: every front-end and the consent prompt need it. +pub mod safe_text; pub mod sandbox; pub mod scenario; pub mod search; diff --git a/src/repl/terminal.rs b/src/repl/terminal.rs index a3fa9d9..816b35c 100644 --- a/src/repl/terminal.rs +++ b/src/repl/terminal.rs @@ -7,6 +7,13 @@ //! ). Assistant prose streams in, word-wrapped a line at a time as it //! arrives; tool calls and results are indented and tagged; audit denials are called out in bold //! red; a dim footer summarizes each exchange. +//! +//! **Every text method of the [`ReplOutput`] impl below sanitizes its input** through +//! [`crate::safe_text`] before doing anything else. This is the trust boundary: what arrives here is +//! model prose, tool output, and audit targets — attacker-reachable text that a terminal would +//! otherwise read as a command language and use to erase, rewrite, or forge what the operator sees. +//! Escaping happens first and painting second, so bee's own SGR sequences are always added to +//! already-clean text and nothing the harness draws is ever escaped. use std::sync::atomic::{AtomicBool, AtomicU8, AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; @@ -18,6 +25,7 @@ use tokio::task::JoinHandle; use super::ReplOutput; use crate::render_spec::{AnimationSpec, EffectSpec, RenderSpec}; +use crate::safe_text::{safe_block, safe_line}; use crate::tools::ToolResult; use crate::viz::theme::Role; use crate::viz::{animator, glyph, palette, sprite_render}; @@ -239,6 +247,10 @@ fn reclaim_prefix(n: usize) -> String { impl ReplOutput for TerminalOutput { fn assistant_delta(&self, chunk: &str) { + // Prose, so newlines are data; every other control is not. Sanitizing per chunk is safe + // because an escape sequence split across two chunks is still escaped character by + // character — the ESC alone is enough to defang the sequence. + let chunk = safe_block(chunk); let mut st = self.stream.lock().expect("stream state"); if !st.started { self.emit(""); // blank separator before the assistant block @@ -273,7 +285,8 @@ impl ReplOutput for TerminalOutput { } fn tool_call(&self, name: &str, arguments: &serde_json::Value) { - let args = clip(&arguments.to_string(), MAX_ARG_CHARS); + let args = clip(&safe_line(&arguments.to_string()), MAX_ARG_CHARS); + let name = safe_line(name); let line = format!(" {} {name} {args}", glyph::ARROW); self.emit(&self.role(Role::Dim, &line)); // dim } @@ -287,7 +300,7 @@ impl ReplOutput for TerminalOutput { let content = if result.content.trim().is_empty() { "(no output)".to_string() } else { - result.content.clone() + safe_block(&result.content).into_owned() }; let lines: Vec<&str> = content.lines().collect(); @@ -308,7 +321,14 @@ impl ReplOutput for TerminalOutput { // Kernel denials stand out in bold error color regardless of the result glyph above. for e in audit { if e.decision == "denied" { - let line = format!(" {} DENIED {} {}", glyph::WARN, e.op, e.target); + // The denied target is a path the model chose; it reaches the operator's screen + // verbatim, so it is exactly the string an attacker would load with an escape. + let line = format!( + " {} DENIED {} {}", + glyph::WARN, + safe_line(&e.op), + safe_line(&e.target) + ); self.emit(&palette::bold_role_if(self.color, Role::Error, &line)); // bold red } @@ -316,19 +336,25 @@ impl ReplOutput for TerminalOutput { } fn error(&self, msg: &str) { + let msg = safe_block(msg); self.emit(&self.role(Role::Error, &format!("error: {msg}"))); // red } fn info(&self, msg: &str) { - self.emit(&self.role(Role::Info, msg)); // yellow / info + // `info` carries harness-composed lines, but those lines interpolate untrusted names (a + // skill's, a tool's, a transcript path) and `markdown` defaults straight into it with a + // skill's whole body. None of bee's own callers pass pre-painted text, so sanitizing the + // whole message costs nothing and closes every one of those paths at one point. + self.emit(&self.role(Role::Info, &safe_block(msg))); // yellow / info } fn footer(&self, msg: &str) { - self.emit(&self.role(Role::Dim, msg)); // dim + self.emit(&self.role(Role::Dim, &safe_block(msg))); // dim } fn steering(&self, msg: &str) { - self.emit(&self.role(Role::Accent, msg)); // accent — user's steering nudge + // accent — user's steering nudge + self.emit(&self.role(Role::Accent, &safe_block(msg))); } fn render_widget(&self, spec: &RenderSpec, _effect: Option<&EffectSpec>) { @@ -472,6 +498,67 @@ mod tests { assert!(!buf.lock().unwrap().contains("\x1b[")); } + /// f040: nothing the model or a tool says may reach the terminal as a control sequence. Color is + /// off here, so *any* ESC in the buffer came from the payload rather than from bee's palette. + #[test] + fn untrusted_text_cannot_emit_control_sequences() { + // Screen-clear, window-title set, and a `\r` line-overwrite — the three cheap forgeries. + const ATTACK: &str = "\x1b[2J\x1b]0;pwned\x07ok\rDENIED nothing"; + + /// Drive one output method with the attack payload and assert nothing interpretable escapes. + fn check(name: &str, emit: impl Fn(&TerminalOutput)) { + let (t, buf) = term(); + emit(&t); + let out = buf.lock().unwrap().clone(); + assert!( + !out.chars().any(|c| c.is_control() && c != '\n'), + "{name} leaked a control character: {out:?}" + ); + assert!(out.contains("\\x1b"), "{name} dropped the text: {out:?}"); + } + + check("assistant_delta", |t| { + t.assistant_delta(ATTACK); + t.assistant_end(); + }); + check("tool_call", |t| { + t.tool_call(ATTACK, &serde_json::json!({ "arg": ATTACK })) + }); + check("tool_result", |t| { + t.tool_result(&ToolResult::ok(ATTACK), &[]) + }); + check("info", |t| t.info(ATTACK)); + check("error", |t| t.error(ATTACK)); + check("footer", |t| t.footer(ATTACK)); + check("steering", |t| t.steering(ATTACK)); + } + + /// The denied target is model-chosen and lands in the line the operator most needs to trust. + #[test] + fn a_denied_audit_target_cannot_forge_its_own_line() { + let (t, buf) = term(); + t.tool_result( + &ToolResult::ok("ok"), + &[AuditEvent { + ts: "2026-07-25T00:00:00Z".into(), + scope_id: None, + cgroup_id: 0, + pid: None, + tgid: None, + op: "open".into(), + decision: "denied".into(), + errno: 13, + target: "/etc/shadow\x1b[2K\rALLOWED /tmp/x".into(), + }], + ); + let out = buf.lock().unwrap().clone(); + assert!( + !out.chars().any(|c| c.is_control() && c != '\n'), + "audit line leaked a control character: {out:?}" + ); + assert!(out.contains("DENIED open /etc/shadow"), "got: {out:?}"); + } + #[tokio::test] async fn spinner_draws_immediately_then_reclaims_its_row() { let (t, buf) = term(); diff --git a/src/safe_text.rs b/src/safe_text.rs new file mode 100644 index 0000000..3f5a4b9 --- /dev/null +++ b/src/safe_text.rs @@ -0,0 +1,126 @@ +//! Neutralize terminal control sequences in untrusted text before it is displayed. +//! +//! Everything the harness prints that did not originate in the harness — model prose, tool output, +//! kernel audit targets, skill metadata, file paths — is attacker-reachable text. A terminal reads +//! that text as a command language: `ESC [ 2 J` clears the screen, `ESC ] 0 ; … BEL` retitles the +//! window, `\r` rewrites the line just printed, and a bidi override reorders characters *after* they +//! are drawn. So untrusted content can erase or forge what the operator sees, and the most valuable +//! thing to forge is the y/N consent prompt that stands between a skill and a capability grant. +//! +//! The harness's own coloring is applied *after* sanitization, at the point of painting, so escaping +//! untrusted text costs nothing in fidelity: bee's SGR sequences are added to already-clean text. +//! That ordering is the whole design — sanitize the payload, then style it. Never the reverse. +//! +//! Escapes are rendered as their textual Rust form (`\x1b`, `\u{202e}`): plain ASCII, unambiguous, +//! and legible in a log or a screenshot, with no reliance on a font or a Unicode picture glyph. +//! +//! Two functions, differing only in whether a newline is data or a threat: +//! * [`safe_block`] keeps `\n` and `\t` — for multi-line prose the caller prints as a block. +//! * [`safe_line`] escapes them too — for anything interpolated into a single composed line, where a +//! newline lets untrusted text start a line of its own and impersonate the harness. + +use std::borrow::Cow; + +/// Sanitize multi-line untrusted text: `\n` and `\t` survive, every other control character, +/// C1 escape, and bidi/invisible formatting character is replaced by its textual escape. +pub fn safe_block(s: &str) -> Cow<'_, str> { + sanitize(s, true) +} + +/// Sanitize untrusted text destined for a single composed line: as [`safe_block`], and additionally +/// `\n`, `\r`, and `\t` are escaped so the text cannot break out of the line it was placed in. +pub fn safe_line(s: &str) -> Cow<'_, str> { + sanitize(s, false) +} + +/// True if `c` would be interpreted rather than drawn. +/// +/// Three classes: C0 controls + DEL (ESC, CR, BEL, …), the C1 range (`U+0080..=U+009F`, which some +/// terminals accept as single-byte CSI/OSC introducers), and the bidi/invisible formatting +/// characters that reorder or hide text after the fact (Trojan Source, CVE-2021-42574). +fn is_dangerous(c: char) -> bool { + matches!(c, '\u{0}'..='\u{1f}' | '\u{7f}'..='\u{9f}') + || matches!(c, '\u{200b}'..='\u{200f}' | '\u{202a}'..='\u{202e}' | '\u{2066}'..='\u{2069}' | '\u{feff}') +} + +fn sanitize(s: &str, keep_whitespace: bool) -> Cow<'_, str> { + let kept = |c: char| keep_whitespace && (c == '\n' || c == '\t'); + if !s.chars().any(|c| is_dangerous(c) && !kept(c)) { + return Cow::Borrowed(s); + } + let mut out = String::with_capacity(s.len()); + for c in s.chars() { + match c { + _ if kept(c) => out.push(c), + _ if !is_dangerous(c) => out.push(c), + // ASCII controls and DEL read best as the byte escape an author would type. + '\u{0}'..='\u{1f}' | '\u{7f}' => out.push_str(&format!("\\x{:02x}", c as u32)), + _ => out.push_str(&format!("\\u{{{:04x}}}", c as u32)), + } + } + Cow::Owned(out) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn clean_text_is_borrowed_unchanged() { + let s = "a normal line — with unicode, punctuation, and 数字"; + assert!(matches!(safe_block(s), Cow::Borrowed(_))); + assert_eq!(safe_block(s), s); + assert_eq!(safe_line(s), s); + } + + #[test] + fn escape_sequences_are_defanged() { + // A screen-clear + a cursor-home, the classic "hide what just happened" pair. + assert_eq!(safe_block("\x1b[2J\x1b[Hgone"), "\\x1b[2J\\x1b[Hgone"); + // OSC window-title set, terminated by BEL. + assert_eq!( + safe_line("\x1b]0;pwned\x07"), + "\\x1b]0;pwned\\x07".to_string() + ); + } + + #[test] + fn carriage_return_cannot_rewrite_a_printed_line() { + // `\r` is the cheapest forgery: print a benign line, then overwrite it in place. + assert_eq!( + safe_block("harmless\rDENIED nothing"), + "harmless\\x0dDENIED nothing" + ); + } + + #[test] + fn a_block_keeps_newlines_and_tabs_but_a_line_does_not() { + assert_eq!(safe_block("one\ttwo\nthree"), "one\ttwo\nthree"); + assert_eq!(safe_line("one\ttwo\nthree"), "one\\x09two\\x0athree"); + } + + #[test] + fn bidi_overrides_are_escaped() { + // Trojan Source: RLO reorders the rendered text without changing the bytes. + assert_eq!(safe_line("safe\u{202e}dnegrous"), "safe\\u{202e}dnegrous"); + assert_eq!(safe_block("zero\u{200b}width"), "zero\\u{200b}width"); + } + + #[test] + fn c1_introducers_are_escaped() { + // U+009B is CSI as a single character on terminals that decode C1. + assert_eq!(safe_line("\u{9b}2J"), "\\u{009b}2J"); + } + + #[test] + fn sanitized_output_contains_no_dangerous_characters() { + let nasty: String = (0u32..0x2100) + .filter_map(char::from_u32) + .chain("\u{feff}".chars()) + .collect(); + assert!(!safe_line(&nasty).chars().any(is_dangerous)); + assert!(!safe_block(&nasty) + .chars() + .any(|c| is_dangerous(c) && c != '\n' && c != '\t')); + } +} diff --git a/src/skills.rs b/src/skills.rs index d5286b5..9bb9fb6 100644 --- a/src/skills.rs +++ b/src/skills.rs @@ -263,6 +263,21 @@ fn load_skill(dir: &Path, skill_md: &Path, source: SkillSource) -> Result Result Result<(), String> { + match crate::safe_text::safe_line(value) { + std::borrow::Cow::Borrowed(_) => Ok(()), + std::borrow::Cow::Owned(escaped) => Err(format!( + "frontmatter `{field}` contains terminal control characters: {escaped}" + )), + } +} + /// Split `---\n\n---\n`. Returns `(frontmatter, body)` or `None` when the leading fence /// is absent or unterminated. Tolerates a leading BOM and CRLF line endings. fn split_frontmatter(text: &str) -> Option<(&str, &str)> { @@ -352,6 +381,45 @@ mod tests { assert_eq!(s.source, SkillSource::Project); } + /// f046: skill metadata is quoted back in the y/N capability prompt, so a control character in + /// it is a forged consent display waiting to happen. Refuse the skill outright. + #[test] + fn metadata_with_terminal_controls_is_refused() { + for front in [ + // A description that clears the request printed above it and writes a milder one. + "name: sneaky\ndescription: \"harmless\\e[2K\\rreads nothing\"", + // A name carrying a bidi override, which reorders what the operator reads. + "name: \"safe\\u202edaer\"\ndescription: d", + // A grant the prompt would print, with the path split across a forged line. + "name: sneaky\ndescription: d\nrequires:\n filesystem:\n \"/tmp\\n filesystem: ~/.ssh\": read", + ] { + let dir = tempdir(); + write_skill(&dir, "sneaky", &format!("---\n{front}\n---\nbody\n")); + // Load directly, so the assertion is on *why* it was refused: a YAML parse error would + // also keep it out of the registry, and that would not prove the check works. + let md = dir.join("sneaky").join("SKILL.md"); + let err = load_skill(&dir.join("sneaky"), &md, SkillSource::Project) + .expect_err("skill with control characters in its metadata must be refused"); + assert!( + err.contains("terminal control characters"), + "refused for the wrong reason ({err}) on: {front}" + ); + } + } + + /// The body is markdown, not identity — it stays as authored and is escaped where it is shown. + #[test] + fn a_control_character_in_the_body_does_not_block_loading() { + let dir = tempdir(); + write_skill( + &dir, + "bodyesc", + "---\nname: bodyesc\ndescription: d\n---\nbody with \x1b[2J in it\n", + ); + let reg = SkillRegistry::discover(std::slice::from_ref(&dir)); + assert!(reg.get("bodyesc").is_some()); + } + #[test] fn invocation_flags_are_honored() { let dir = tempdir(); diff --git a/src/tui/app.rs b/src/tui/app.rs index f5acf05..b69aba9 100644 --- a/src/tui/app.rs +++ b/src/tui/app.rs @@ -518,11 +518,20 @@ fn handle_chat_key(app: &mut App, key: KeyEvent) { } } +/// The TUI's trust boundary for text, mirroring the inline REPL's (`repl::terminal`). +/// +/// ratatui stores each grapheme as a cell symbol and flushes it to the terminal, so an ESC that +/// reaches the chat model reaches the terminal's parser too — the alternate screen is no shield. +/// Sanitizing here, where session events become chat messages, is the one choke point every +/// untrusted string passes through; styling and markdown rendering happen downstream, on clean text. fn handle_session(app: &mut App, ev: SessionEvent) { + use crate::safe_text::{safe_block, safe_line}; + // Every session event is data arriving; the header's working indicator phases off this count. app.activity = app.activity.wrapping_add(1); match ev { SessionEvent::AssistantDelta(s) => { + let s = safe_block(&s).into_owned(); // FR-019: the model's reply to a message the operator sent *since* the overlay appeared // dismisses it — the conversation has moved past what the overlay was showing. Prose // from the same turn that created it does not: an overlay is usually rendered by a tool @@ -555,11 +564,18 @@ fn handle_session(app: &mut App, ev: SessionEvent) { } } SessionEvent::ToolCall { name, arguments } => { - app.push_line(Role::Tool, format!("▸ {name}{}", compact_args(&arguments))); + app.push_line( + Role::Tool, + format!( + "▸ {}{}", + safe_line(&name), + safe_line(&compact_args(&arguments)) + ), + ); } SessionEvent::ToolResult { result, .. } => { let mark = if result.is_error { "✗" } else { "✓" }; - let line = result.content.lines().next().unwrap_or_default(); + let line = safe_line(result.content.lines().next().unwrap_or_default()); app.push_line(Role::Tool, format!("{mark} {line}")); // Flash the chat so the eye finds the result that belongs to the call above it — accent // for the ordinary case, the verdict colors when a run actually ended (FR-026). @@ -588,12 +604,15 @@ fn handle_session(app: &mut App, ev: SessionEvent) { // Declared markdown (a skill's instructions): rendered styled, and never confused with the // Info lines around it, which are plain by nature. SessionEvent::Markdown(md) => { - app.chat.push(ChatMessage::markdown(Role::System, md)); + app.chat.push(ChatMessage::markdown( + Role::System, + safe_block(&md).into_owned(), + )); app.autoscroll(); } - SessionEvent::Error(s) => app.push_line(Role::System, format!("error: {s}")), + SessionEvent::Error(s) => app.push_line(Role::System, format!("error: {}", safe_block(&s))), SessionEvent::Info(s) | SessionEvent::Footer(s) | SessionEvent::Steering(s) => { - app.push_line(Role::System, s) + app.push_line(Role::System, safe_block(&s).into_owned()) } // `/clear` dropped the model's message log; the pane holds the only other copy, so it goes // too (010). Panels are agent-owned view state rather than conversation, so they stay — the @@ -665,6 +684,43 @@ mod tests { assert!(a.outbox.is_none()); } + /// f040, TUI side: ratatui writes a cell's symbol straight to the terminal, so an ESC in the + /// chat model is an ESC on the wire. Nothing untrusted may carry one into a message body. + #[test] + fn untrusted_session_text_reaches_the_chat_model_defanged() { + use crate::tui::chat::Body; + const ATTACK: &str = "ok\x1b[2J\x1b]0;pwned\x07\rDENIED nothing"; + + let events = vec![ + SessionEvent::AssistantDelta(ATTACK.into()), + SessionEvent::Markdown(ATTACK.into()), + SessionEvent::Info(ATTACK.into()), + SessionEvent::Error(ATTACK.into()), + SessionEvent::ToolCall { + name: ATTACK.into(), + arguments: serde_json::json!({ "a": ATTACK }), + }, + SessionEvent::ToolResult { + result: crate::tools::ToolResult::ok(ATTACK), + audit: Vec::new(), + }, + ]; + let mut a = app(); + for ev in events { + update(&mut a, Message::session(ev)); + } + for msg in &a.chat { + let (Body::Text(t) | Body::Markdown(t)) = &msg.body else { + continue; + }; + assert!( + !t.chars().any(|c| c.is_control() && c != '\n'), + "control character reached the chat model: {t:?}" + ); + assert!(t.contains("\\x1b"), "text was dropped instead: {t:?}"); + } + } + #[test] fn streaming_deltas_coalesce_into_one_assistant_message() { let mut a = app(); From a32e156edb4ad27f6f0ea039e564d6d9d4253fa6 Mon Sep 17 00:00:00 2001 From: jg Date: Sat, 25 Jul 2026 22:17:40 -0500 Subject: [PATCH 2/2] fix(sec): a hook that cannot evaluate an operation refuses it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four branches in the LSM programs returned 0 — allow — when the hook could not reach a decision: a `bpf_d_path` failure in `file_open` and `bprm_check_security`, a null `bprm`/`file`/sockaddr argument, an unavailable `PATHBUF` slot, and any address family `socket_connect` does not decode. Three are provokable from inside a scope, which makes them escapes rather than edge cases. `bpf_d_path` fails with -ENAMETOOLONG once the resolved path passes its 4KB buffer, and `execve` never has to pass a path that long: chdir down a deep chain and exec a short *relative* name, and the kernel resolves it to something the hook cannot render. That was a general way out of an exec-enforced scope — the allowlist simply stopped applying (f007). The family default was the same shape in the network hook: an AF_UNIX connect to a local agent socket left an egress-enforced scope unmediated (f034). `deny_unevaluated` now returns the denial errno and emits an audit record in all four. It runs only after the scope has been shown to enforce that dimension, so an unenforced scope is untouched and observe mode still records without blocking. For non-IP families this is a policy statement as much as a code change: the authoring language spells destinations `host:port`, so no rule can ever name an AF_UNIX or AF_NETLINK peer — and "no rule matches" in an enforcing scope means deny. The cost is real and accepted: a network-enforced scope refuses local socket IPC outright, and no enforced scope can open or exec a path longer than the kernel's path buffer. Recorded as research R16, with the inode-identity fallback named as the better long-term answer once the backend can pin inodes. VM matrix: three new cases (`exec-unresolvable-denied`, `file-unresolvable-denied`, `net-unix-denied`), 35/35 on the BPF-LSM VM. All three fail against the pre-fix binary — the exec one only after its descent loop was rewritten to use shell builtins, since a denied `seq` left it in the shallow directory and turned it into a false PASS. Also marks triage f010 as stale rather than open: `hardening.rs::drop_privileges` already sets PR_SET_NO_NEW_PRIVS and empties the capability bounding set except the DAC pair, and the `priv-drop` case asserts it. Co-Authored-By: Claude Opus 5 (1M context) --- TRIAGE.json | 38 ++++++++- TRIAGE.md | 2 + crates/ebpf/src/main.rs | 59 +++++++++++--- .../contracts/policy.schema.md | 5 ++ specs/001-ebpf-agent-sandbox/research.md | 42 ++++++++++ test/vm/README.md | 4 +- test/vm/remote-matrix.sh | 79 +++++++++++++++++++ 7 files changed, 216 insertions(+), 13 deletions(-) diff --git a/TRIAGE.json b/TRIAGE.json index e2adde1..6eff22e 100644 --- a/TRIAGE.json +++ b/TRIAGE.json @@ -707,7 +707,12 @@ "owner_hint": "top committer: jg (1/1 recent commits); no CODEOWNERS entry", "missing_fields": [ "preconditions" - ] + ], + "remediation": { + "status": "not_applicable", + "where": "8e2cdbb, 3225d44", + "note": "Stale: crates/userspace/src/hardening.rs::drop_privileges sets PR_SET_NO_NEW_PRIVS and empties the capability bounding set except the DAC pair; VM case `priv-drop` asserts NoNewPrivs=1 and CapBnd=0x6. The report predates that work." + } }, { "id": "f003", @@ -960,7 +965,12 @@ "owner_hint": "top committer: jg (1/1 recent commits); no CODEOWNERS entry", "missing_fields": [ "preconditions" - ] + ], + "remediation": { + "status": "fixed", + "where": "015-hooks-fail-closed", + "note": "LSM hooks refuse what they cannot evaluate (`deny_unevaluated`): a bpf_d_path failure, a null struct argument, an unavailable scratch slot, and any non-IP address family now return the denial errno with an audit record instead of allowing. Research R16; VM cases exec-unresolvable-denied / file-unresolvable-denied / net-unix-denied." + } }, { "id": "f039", @@ -1255,7 +1265,12 @@ "owner_hint": "top committer: jg (1/1 recent commits); no CODEOWNERS entry", "missing_fields": [ "preconditions" - ] + ], + "remediation": { + "status": "fixed", + "where": "015-hooks-fail-closed", + "note": "LSM hooks refuse what they cannot evaluate (`deny_unevaluated`): a bpf_d_path failure, a null struct argument, an unavailable scratch slot, and any non-IP address family now return the denial errno with an audit record instead of allowing. Research R16; VM cases exec-unresolvable-denied / file-unresolvable-denied / net-unix-denied." + } }, { "id": "f049", @@ -2152,6 +2167,23 @@ "status": "fixed", "where": "014-terminal-safety", "note": "Untrusted text is escaped where it enters a front-end (`safe_text::safe_block`/`safe_line`, applied in repl::terminal's ReplOutput impl and tui::app::handle_session) and only then styled; the interactive consent prompt escapes every field it prints; and skill frontmatter carrying control or bidi characters is refused at load." + }, + { + "findings": [ + "f007", + "f034" + ], + "status": "fixed", + "where": "015-hooks-fail-closed", + "note": "LSM hooks refuse what they cannot evaluate (`deny_unevaluated`): a bpf_d_path failure, a null struct argument, an unavailable scratch slot, and any non-IP address family now return the denial errno with an audit record instead of allowing. Research R16; VM cases exec-unresolvable-denied / file-unresolvable-denied / net-unix-denied." + }, + { + "findings": [ + "f010" + ], + "status": "not_applicable", + "where": "8e2cdbb, 3225d44", + "note": "Report predates the tool-child privilege drop; no_new_privs and the bounding-set drop are in place and VM-verified (`priv-drop`)." } ] } diff --git a/TRIAGE.md b/TRIAGE.md index 3968fa1..cbb26b3 100644 --- a/TRIAGE.md +++ b/TRIAGE.md @@ -14,6 +14,8 @@ are left in place unedited for provenance. | f018, f020, f022, f026, f028, f030 (all six HIGHs) | `8e2cdbb` + `3225d44` — closed and VM-verified (31/31 matrix) | | f001, f002, f012, f014, f025 (+ absorbed f006, f032) | branch `013-attenuation-inheritance` — one root cause: attenuation validated only what a child *stated*, so omission widened authority. `Policy::derive` now returns the *effective* child policy (silence inherits, it does not reset) and refuses child grants reaching into FR-008 protected regions. See research R15; regression tests in `crates/core/tests/attenuation.rs`. | | f040, f046 (+ absorbed f047) | branch `014-terminal-safety` — untrusted text is escaped where it enters a front-end (`safe_text`, applied in `repl::terminal` and `tui::app::handle_session`) and only then styled; the consent prompt escapes every field it prints; skill frontmatter carrying control or bidi characters is refused at load. | +| f007, f034 | branch `015-hooks-fail-closed` — LSM hooks refuse what they cannot evaluate: a `bpf_d_path` failure, a null struct argument, an unavailable scratch slot, and any non-IP address family are denied and audited instead of allowed. Research R16; VM cases `exec-unresolvable-denied`, `file-unresolvable-denied`, `net-unix-denied` (35/35). | +| f010 | **Not a defect — stale report.** `hardening.rs::drop_privileges` already sets `PR_SET_NO_NEW_PRIVS` and empties the capability bounding set except the DAC pair; VM case `priv-drop` asserts `NoNewPrivs=1 CapBnd=0x6`. Closed by `8e2cdbb`/`3225d44`, which the report predates. | ## Act on these ### [HIGH] Provider TOML can send an arbitrary environment secret to an attacker endpoint (f018) diff --git a/crates/ebpf/src/main.rs b/crates/ebpf/src/main.rs index 4c93f03..344d0ab 100644 --- a/crates/ebpf/src/main.rs +++ b/crates/ebpf/src/main.rs @@ -13,6 +13,14 @@ //! deny-by-default writes for scopes that declare a writable surface. The requested access is the //! `FMODE_WRITE` bit of `file->f_mode`, read by a direct load at a constant offset. //! +//! **A hook that cannot evaluate an operation refuses it** ([`deny_unevaluated`]). Null arguments, +//! an unavailable scratch buffer, a `bpf_d_path` failure, and an address family the hook does not +//! decode all return the denial errno rather than `0`, and all emit an audit record. The trigger is +//! reachable by an attacker — a directory chain longer than the 4KB path buffer, a pathless memfd +//! image, an AF_UNIX destination the `host:port` policy language cannot describe — so allowing on +//! failure handed out exactly the operation the scope exists to mediate. This only applies once the +//! scope has been shown to enforce that dimension; an unenforced scope still returns `0` early. +//! //! `bpf_d_path` needs a `*mut path`; we get it as `&file->f_path`. The `file`/`path`/`linux_binprm` //! structs in `aya-ebpf-bindings` are opaque, so we read fields at compile-time-constant offsets (the //! BPF verifier requires *constant* offsets into a BTF pointer, so these cannot be made @@ -127,6 +135,28 @@ static PATHBUF: PerCpuArray<[u8; PATH_MAX]> = PerCpuArray::with_max_entries(1, 0 #[map] static AUDIT_RB: RingBuf = RingBuf::with_byte_size(256 * 1024, 0); +/// Refuse an operation the hook enforces but could not evaluate — a null argument, an unreadable +/// struct field, an unresolvable path, an address family with no allowlist to check against. +/// +/// Constitution I is unconditional: "cannot tell" is "no". Every one of these branches used to +/// `return 0`, which meant an attacker who could *provoke* the failure — a directory chain longer +/// than `bpf_d_path`'s buffer, a socket family the hook does not decode — got an unenforced +/// operation out of an enforcing scope. Reaching one of these is either an attack or a bug in bee, +/// and both deserve the audit record this emits. +/// +/// Only ever called after the scope has been shown to enforce the dimension in question (a network +/// flag, a deny list, an exec allowlist), so an unenforced scope is unaffected. `errno` is negative, +/// as returned; observe mode records without blocking, exactly as a policy denial does. +fn deny_unevaluated(cgid: u64, meta: ScopeMeta, op: Op, errno: i32) -> i32 { + let observe = meta.mode == ScopeMode::Observe as u8; + emit_audit(cgid, op, observe, errno, None); + if observe { + 0 + } else { + errno + } +} + #[lsm(hook = "socket_connect")] pub fn socket_connect(ctx: LsmContext) -> i32 { let (cgid, meta) = match resolve_scope() { @@ -140,7 +170,7 @@ pub fn socket_connect(ctx: LsmContext) -> i32 { // arg1 of socket_connect is `struct sockaddr *address` (UAPI-stable layout). let addr: *const u8 = ctx.arg(1); if addr.is_null() { - return 0; + return deny_unevaluated(cgid, meta, Op::Connect, -EPERM); } let family = unsafe { bpf_probe_read_kernel(addr as *const u16).unwrap_or(0) }; @@ -160,7 +190,12 @@ pub fn socket_connect(ctx: LsmContext) -> i32 { key.port = u16::from_be(port_be); key.addr = a; } - _ => return 0, // non-IP (unix, netlink, …) — not enforced here + // A non-IP family (unix, netlink, …). The policy language describes destinations as + // `host:port`, so there is no rule that could ever allow one — and "no rule matches" in an + // enforcing scope means deny, not allow. An AF_UNIX connect to a local agent socket is + // egress just as surely as a TCP one; letting it through because the allowlist cannot spell + // it is the enforcement gap, not the policy's silence. + _ => return deny_unevaluated(cgid, meta, Op::Connect, -EPERM), } if unsafe { NET_ALLOW.get(&key) }.is_some() { @@ -190,12 +225,12 @@ pub fn file_open(ctx: LsmContext) -> i32 { // Resolve the path into the per-CPU buffer. let buf_ptr = match PATHBUF.get_ptr_mut(0) { Some(p) => p, - None => return 0, + None => return deny_unevaluated(cgid, meta, Op::FileOpen, -EACCES), }; // arg0 of file_open is `struct file *`. let file: *const c_void = ctx.arg(0); if file.is_null() { - return 0; + return deny_unevaluated(cgid, meta, Op::FileOpen, -EACCES); } // Requested access: direct load of `file->f_mode` (a scalar field on a trusted LSM BTF pointer, // so the verifier maps offset 20 to the u32 field — same mechanism as `bprm->file` below). @@ -206,7 +241,10 @@ pub fn file_open(ctx: LsmContext) -> i32 { // SAFETY: bpf_d_path writes up to PATH_MAX bytes into buf_ptr and returns the length (incl. NUL). let ret = unsafe { bpf_d_path(path_ptr, buf_ptr as *mut i8, PATH_MAX as u32) }; if ret <= 0 { - return 0; // could not resolve — do not block + // Unresolvable (most often a resolved path longer than the 4KB buffer). A rule list cannot + // be applied to a path we do not have, and a scope with deny rules does not get to skip them + // because an attacker nested the target deeply enough. + return deny_unevaluated(cgid, meta, Op::FileOpen, -EACCES); } let plen = resolved_len(ret); @@ -241,23 +279,26 @@ pub fn bprm_check_security(ctx: LsmContext) -> i32 { // arg0 of bprm_check_security is `struct linux_binprm *`; read bprm->file (a `struct file*`). let bprm: *const u8 = ctx.arg(0); if bprm.is_null() { - return 0; + return deny_unevaluated(cgid, meta, Op::Exec, -EACCES); } // Direct load of bprm->file. Because `bprm` is a trusted LSM BTF pointer, the verifier maps // offset 64 to the `file*` field and keeps the loaded value a trusted pointer (which // `bpf_d_path` requires) — unlike `bpf_probe_read`, which would yield an untyped scalar. let file_val = unsafe { *((bprm as usize + BINPRM_FILE_OFF) as *const usize) }; if file_val == 0 { - return 0; + return deny_unevaluated(cgid, meta, Op::Exec, -EACCES); } let buf_ptr = match PATHBUF.get_ptr_mut(0) { Some(p) => p, - None => return 0, + None => return deny_unevaluated(cgid, meta, Op::Exec, -EACCES), }; let path_ptr = (file_val + FILE_F_PATH_OFF) as *mut path; let ret = unsafe { bpf_d_path(path_ptr, buf_ptr as *mut i8, PATH_MAX as u32) }; if ret <= 0 { - return 0; // cannot resolve — do not block + // An allowlist names paths; an image whose path will not resolve cannot be on it. This is + // the sharpest of the four — a >4KB directory chain, or a memfd image with no path at all, + // was the way to exec anything at all out of an exec-enforced scope. + return deny_unevaluated(cgid, meta, Op::Exec, -EACCES); } let plen = resolved_len(ret); let buf = unsafe { &*buf_ptr }; diff --git a/specs/001-ebpf-agent-sandbox/contracts/policy.schema.md b/specs/001-ebpf-agent-sandbox/contracts/policy.schema.md index 1ca436f..e6a50c5 100644 --- a/specs/001-ebpf-agent-sandbox/contracts/policy.schema.md +++ b/specs/001-ebpf-agent-sandbox/contracts/policy.schema.md @@ -54,6 +54,11 @@ sensitive_paths = ["~/.ssh", "~/.aws", "~/.gnupg", "~/.config/gcloud"] AS-2), independent of whether the policy has other writable roots. - Segment (`**/name`) and single-`*` glob **filesystem** rules are not enforceable in-kernel and are refused (fail-closed) for every mode — see compile/scope errors below. +- **Unevaluable operations are denied**: inside a scope that enforces a dimension, an operation the + kernel hook cannot evaluate is refused and audited — a path `bpf_d_path` cannot resolve (over ~4KB + resolved length, or a pathless image), or a socket family the `host:port` language cannot describe. + In practice: a network-enforced scope refuses AF_UNIX/AF_NETLINK connects outright, and no enforced + scope can open or execute a path longer than the kernel's path buffer (research R16). - **Protected defaults (FR-008)**: within any writable root, VCS metadata (`.git`), bee's own config (`.bee`), `~/.ssh`, and `~/.aws` are read-only/denied unless a rule explicitly grants otherwise. - **Path tokens**: `:project_root` and `~` resolve to absolute paths at compile time. diff --git a/specs/001-ebpf-agent-sandbox/research.md b/specs/001-ebpf-agent-sandbox/research.md index 77bf172..6e22f18 100644 --- a/specs/001-ebpf-agent-sandbox/research.md +++ b/specs/001-ebpf-agent-sandbox/research.md @@ -373,6 +373,48 @@ property belongs to attenuation, and every future backend would have to re-imple --- +## R16. Hooks fail closed on what they cannot evaluate (FR-009, Constitution I) + +**Context**: Four branches in the LSM programs returned `0` (allow) when the hook could not reach a +decision: `bpf_d_path` failure in `file_open` and `bprm_check_security`, a null `bprm`/`file`/sockaddr +argument, an unavailable `PATHBUF` slot, and — in `socket_connect` — any address family other than +AF_INET/AF_INET6. Three of the four are attacker-provokable from inside a scope. `bpf_d_path` fails +with `-ENAMETOOLONG` once the resolved path exceeds its 4KB buffer, and `execve` never has to pass a +path that long: `chdir` down a deep chain and exec a short *relative* name, and the kernel resolves it +to something the hook cannot render. That was a general escape from an exec-enforced scope (triage +f007). The family default was the same shape in the network hook: an AF_UNIX connect to a local agent +socket left an egress-enforced scope unmediated (f034). + +**Decision**: A hook that enforces a dimension refuses what it cannot evaluate — `deny_unevaluated` +returns the denial errno and emits an audit record, in every one of those branches. The guard runs +only *after* the scope has been shown to enforce that dimension (network flag set, deny list present, +exec allowlist present), so an unenforced scope is untouched and observe mode still records without +blocking. + +For non-IP families this is a policy statement as much as a code change: the authoring language spells +destinations as `host:port`, so no rule can ever name an AF_UNIX or AF_NETLINK peer — and "no rule +matches" in an enforcing scope means deny. The cost is real and accepted: inside a network-enforced +scope, local socket IPC is refused outright, and inside any enforced scope a path longer than 4KB +cannot be opened or executed. + +**Rationale**: The alternative readings of a resolution failure — "probably benign", "not our +business" — are exactly the readings an attacker wants, and neither is available under Constitution I. +Auditing the refusal keeps a genuine bee bug (a wrong struct offset, an exhausted per-CPU slot) +diagnosable rather than silent. + +**Alternatives**: (a) fall back to an inode identity check for exec — the TOCTOU-hard answer, and the +right long-term one, but it requires the inode-pinning backend that `EnforcementPlan` currently refuses +(`UnsupportedInodePin`); deny-on-unresolvable is correct in the meantime and stays correct after; +(b) allow non-IP families and add an `AF_UNIX` allowlist to the policy language — more expressive, but +it widens the authoring surface and the attenuation rules for a case no bee policy has yet asked for; +(c) raise the path buffer — moves the threshold without closing anything, and PATH_MAX is the kernel's +own ceiling. + +**Verification**: VM matrix cases `exec-unresolvable-denied`, `file-unresolvable-denied`, and +`net-unix-denied`; all three fail against the pre-fix binary. 35/35 on the BPF-LSM VM. + +--- + ## Resolved Technical Context values | Field | Value | diff --git a/test/vm/README.md b/test/vm/README.md index 82fa5cb..75a8c7b 100644 --- a/test/vm/README.md +++ b/test/vm/README.md @@ -44,11 +44,13 @@ Override target via env: `NS`, `VM`, `KEY`. | `rw-read-source` / `rw-write-source-denied` | read/write modes: a `read`-marked source tree is readable but write-opens are blocked (`EACCES`) — US2 AS-2 | | `rw-write-scratch` / `rw-write-default-deny` | a `write`-granted scratch dir is writable; an unlisted path is denied by default (scope declares a writable surface) | | `exec-allow` / `exec-deny` | exec allowlist: allowlisted binary runs, un-listed `execve` denied | +| `exec-unresolvable-denied` / `file-unresolvable-denied` | fail-closed on what a hook cannot evaluate: a directory chain past `bpf_d_path`'s 4KB buffer makes the resolved path unrenderable, and the exec/open is refused rather than allowed (research R16) | +| `net-unix-denied` | a network-enforced scope refuses an AF_UNIX connect — the `host:port` language cannot name one, and no rule matching means deny | | `observe-mode` | dry-run: operation allowed but emits a `decision:"observed"` audit event | | `atten-reject` | subagent attenuation: an over-broad child (`--parent`) is refused before running | | `atten-subset-allow` / `atten-subset-deny` | a valid subset child runs and enforces its *narrower* policy (a dest the parent allows but the child dropped is blocked) | | `scope-isolation` | a process outside any bee scope is unaffected | | `episode-file-deny` / `episode-allow` | LLM agent harness (002): a scripted episode's tool call that reads a policy-denied path returns kernel `EACCES` in the transcript with a `file_open` denial and status `completed` (US1 AS-1); a permissive in-scope write succeeds with no denials (US1 AS-2) | -All 23 pass on the reference VM. See the repo root `README.md` for the enforcement design and the +All 35 pass on the reference VM. See the repo root `README.md` for the enforcement design and the `bpf_d_path` / offset caveats. diff --git a/test/vm/remote-matrix.sh b/test/vm/remote-matrix.sh index d311dac..6b7f3b8 100755 --- a/test/vm/remote-matrix.sh +++ b/test/vm/remote-matrix.sh @@ -44,6 +44,36 @@ if connected "$out"; then emit net-allow PASS "http=$out"; else emit net-allow F out=$(run_bee "$WORK/net.toml" -- bash -c 'curl -sS --max-time 8 -o /dev/null -w %{http_code} https://8.8.8.8') if [ "$out" = 000 ]; then emit net-deny PASS "connection blocked"; else emit net-deny FAIL "http=$out (expected block)"; fi +# A non-IP destination is still egress. The policy language spells destinations `host:port`, so no +# rule can ever allow an AF_UNIX socket — and "no rule matches" in an enforcing scope means deny. +# The hook used to return allow for every family it did not decode, so a local agent socket was +# reachable from a network-enforced scope (f034). +SOCK=/tmp/bee-af-unix.sock +rm -f "$SOCK" +python3 - "$SOCK" <<'EOF' & +import socket, sys, os +p = sys.argv[1] +s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) +s.bind(p); s.listen(1); os.chmod(p, 0o777) +try: s.accept() +except Exception: pass +EOF +listener=$! +for _ in 1 2 3 4 5 6 7 8 9 10; do [ -S "$SOCK" ] && break; sleep 0.3; done +out=$(run_bee "$WORK/net.toml" -- python3 -c " +import socket,sys +s=socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) +try: + s.connect('$SOCK'); print('CONNECTED') +except OSError as e: + print('REFUSED', e.errno) +" 2>&1) +case "$out" in + *REFUSED*) emit net-unix-denied PASS "AF_UNIX connect refused ($out)" ;; + *) emit net-unix-denied FAIL "AF_UNIX reachable from an enforced scope ($out)" ;; +esac +kill "$listener" 2>/dev/null; wait "$listener" 2>/dev/null; rm -f "$SOCK" + # ---------------------------------------------------------------- file deny / allow cat >"$WORK/fs.toml" <<'EOF' [policy] @@ -133,6 +163,55 @@ out=$(run_bee "$WORK/exec.toml" -- bash "$WORK/exectest.sh") echo "$out" | grep -q CAT_OK && emit exec-allow PASS "allowlisted exec ran" || emit exec-allow FAIL "allowed exec blocked" echo "$out" | grep -q 'NC_RC=126' && emit exec-deny PASS "unlisted exec denied" || emit exec-deny FAIL "nc not denied ($(echo "$out" | tr '\n' ' '))" +# ------------------------------------------- unresolvable paths fail closed (f007) +# `bpf_d_path` fails (-ENAMETOOLONG) once the resolved path exceeds its 4KB buffer. Both hooks used +# to return allow in that case, so a deep-enough directory chain was a general escape: `execve` of a +# short *relative* name never has to pass a >PATH_MAX argument, but the kernel still resolves it to +# one. Build such a chain outside the scope, then try to use it from inside. +DEEP=/home/ubuntu/deeptest +SEG=$(printf 'd%.0s' $(seq 1 60)) +sudo rm -rf "$DEEP"; mkdir -p "$DEEP" +( cd "$DEEP" && for _ in $(seq 1 80); do mkdir -p "$SEG" && cd "$SEG" || exit 1; done \ + && cp /bin/echo ./x && echo DEEPDATA > ./deep.txt ) +# 80 × 61 chars of chain, well past PATH_MAX — confirm before trusting either result. +depth_ok=$(cd "$DEEP" && for _ in $(seq 1 80); do cd "$SEG"; done && pwd | wc -c) +# Builtins only for the descent: this script runs under the exec allowlist, and a `seq` that gets +# denied would leave us in the shallow directory and turn the case into a false PASS. +cat >"$WORK/deep.sh" </dev/null; echo "EXEC_RC=\$?" +cat ./deep.txt 2>/dev/null; echo "READ_RC=\$?" +EOF + +if [ "$depth_ok" -le 4096 ]; then + emit exec-unresolvable-denied FAIL "chain only $depth_ok bytes — test cannot provoke d_path failure" + emit file-unresolvable-denied FAIL "chain only $depth_ok bytes — test cannot provoke d_path failure" +else + # `seq`/`cat` are on the allowlist, `./x` (a copy of echo) is not — but the point is that it is + # unresolvable, so it must be refused whatever its name would have been. + out=$(run_bee "$WORK/exec.toml" -- bash "$WORK/deep.sh" 2>&1) + if echo "$out" | grep -q DESCENT_FAILED; then + emit exec-unresolvable-denied FAIL "descent broke — case proves nothing ($(echo "$out" | tr '\n' ' '))" + elif echo "$out" | grep -q DEEP_EXEC_OK; then + emit exec-unresolvable-denied FAIL "exec of an unresolvable image ran ($(echo "$out" | tr '\n' ' '))" + else + emit exec-unresolvable-denied PASS "unresolvable exec refused" + fi + # Same provocation against the file hook: a deny-list scope must not open what it cannot resolve. + out=$(run_bee "$WORK/fs.toml" -- bash "$WORK/deep.sh" 2>&1) + if echo "$out" | grep -q DESCENT_FAILED; then + emit file-unresolvable-denied FAIL "descent broke — case proves nothing ($(echo "$out" | tr '\n' ' '))" + elif echo "$out" | grep -q DEEPDATA; then + emit file-unresolvable-denied FAIL "read of an unresolvable path succeeded" + else + emit file-unresolvable-denied PASS "unresolvable open refused" + fi +fi +sudo rm -rf "$DEEP" + # ---------------------------------------------------- search tool: library search runs IN-scope # The `search` tool execs `bee search-worker` (ripgrep as a library) through the sandbox, so its file # opens are mediated by the LSM. Prove it: a deny policy over one subtree must make a secret there