diff --git a/TODO.md b/TODO.md index 49e9dc9..8b6d1c1 100644 --- a/TODO.md +++ b/TODO.md @@ -24,7 +24,7 @@ ## Security - **Privsep the dashboard from the privileged daemon**: hive-c0re runs as root (it has to — `nixos-container` create / start / destroy, the meta git repo, every per-agent bind mount). The HTTP server lives in the same process, so every read-endpoint (`/api/state-file`, `/api/journal/{name}`, `/api/agent-config/{name}`) is one allow-list bug away from serving arbitrary host files. Split the architecture: keep the privileged daemon doing lifecycle + git + ipc, run the web UI as an unprivileged user that talks to the daemon over a unix socket with a narrow request surface (`ReadAgentStateFile { agent, rel_path }` etc.). The unprivileged process can't read `/etc/shadow` even if every check in `get_state_file` is bypassed — it doesn't have the bits. Container-lifecycle POSTs (`/restart`, `/destroy`, etc.) become forwarded RPCs the privileged side authorises on its terms. -- **Defense in depth on `get_state_file`**: until privsep lands, the allow-list is load-bearing. Worth adding: refuse files whose mode is not world-readable (so an agent writing a 0600 file inside `state/` can't have its contents proxied through the endpoint to a different operator), and refuse symlinks at any path component (`O_NOFOLLOW`-style — `canonicalize` resolves them, but we currently don't reject if the original path had symlinks). +- ~~**Defense in depth on `get_state_file`**~~ ✓ landed — `resolve_state_path` (shared by `get_state_file` + `scan_validated_paths`) now: (a) walks each path component below the matched root via `symlink_metadata` and refuses outright if any is a symlink (so an agent planting `ln -s /var/lib/hyperhive/agents/other/state/secret /agents/me/state/peek` can't have its target proxied — `canonicalize` would happily resolve past the allow-list check otherwise); (b) refuses any `..` traversal below the root with a friendlier error than "escapes allow-list"; (c) refuses files whose mode isn't world-readable (`mode & 0o004 == 0`) so a 0600 file inside `state/` doesn't leak via the endpoint; (d) bundles the metadata fetch into the resolve helper so callers don't restat. New tests in `hive-c0re/src/dashboard.rs::tests` cover leaf-symlink, mid-path-symlink, `..` traversal, and plain-dir-passthrough cases. ## Harness Ergonomics (agent-side wishlist) diff --git a/hive-c0re/src/dashboard.rs b/hive-c0re/src/dashboard.rs index 6badad4..2790cbd 100644 --- a/hive-c0re/src/dashboard.rs +++ b/hive-c0re/src/dashboard.rs @@ -958,23 +958,66 @@ struct StateFileQuery { /// that has been verified against the allow-list. Returns `Err` /// with a human-readable reason for every failure mode (path /// outside roots, canonicalize failure, escape via symlink, -/// per-agent subdir not `state`). Shared by `get_state_file` (read) -/// and `post_state_file_check` (existence probe) so both endpoints -/// apply identical security rules. -fn resolve_state_path(raw: &str) -> std::result::Result { +/// per-agent subdir not `state`, symlink anywhere below the root, +/// file not world-readable). Shared by `get_state_file` (read) and +/// `scan_validated_paths` (linkify candidates in message bodies) +/// so both apply identical security rules and the linkifier +/// doesn't render a path the reader will refuse to serve. +/// +/// Defense-in-depth layers (in order): +/// 1. Caller-supplied prefix has to match the allow-list (agents/ +/// or shared/), else reject without touching the fs. +/// 2. No symlinks below the matched root. Walked pre-canonicalize +/// via `symlink_metadata` on each component so a sub-agent that +/// plants `ln -s /var/lib/hyperhive/agents/other/state/secret +/// /agents/me/state/peek` can't proxy a different agent's file +/// through this endpoint (canonicalize would happily resolve +/// the symlink to a path inside the allow-list). +/// 3. Canonicalize is run anyway as a belt-and-braces check — +/// resolves `..`/`.` traversal and rejects if the result +/// escapes the roots. +/// 4. Under `AGENTS_ROOT`, the second path component must be +/// `state/` — agents' applied/proposed git repos and config dirs +/// are off-limits. +/// 5. The target's metadata is fetched once and returned to the +/// caller so they don't restat. If the target is a regular +/// file it must be world-readable (mode & 0o004); a 0600 file +/// inside `state/` could leak through this endpoint to anyone +/// holding the dashboard URL otherwise. +fn resolve_state_path( + raw: &str, +) -> std::result::Result<(std::path::PathBuf, std::fs::Metadata), String> { + use std::os::unix::fs::PermissionsExt as _; const AGENTS_ROOT: &str = "/var/lib/hyperhive/agents"; const SHARED_ROOT: &str = "/var/lib/hyperhive/shared"; let raw = raw.trim(); - let mapped: std::path::PathBuf = if let Some(rest) = raw.strip_prefix("/agents/") { - std::path::PathBuf::from(format!("{AGENTS_ROOT}/{rest}")) - } else if let Some(rest) = raw.strip_prefix("/shared/") { - std::path::PathBuf::from(format!("{SHARED_ROOT}/{rest}")) - } else if raw.starts_with(AGENTS_ROOT) || raw.starts_with(SHARED_ROOT) { - std::path::PathBuf::from(raw) - } else { - return Err(format!("path not in allow-list: {raw}")); - }; - let canonical = std::fs::canonicalize(&mapped).map_err(|e| format!("{}: {e}", mapped.display()))?; + let (mapped, root): (std::path::PathBuf, &str) = + if let Some(rest) = raw.strip_prefix("/agents/") { + ( + std::path::PathBuf::from(format!("{AGENTS_ROOT}/{rest}")), + AGENTS_ROOT, + ) + } else if let Some(rest) = raw.strip_prefix("/shared/") { + ( + std::path::PathBuf::from(format!("{SHARED_ROOT}/{rest}")), + SHARED_ROOT, + ) + } else if let Some(rest) = raw.strip_prefix(&format!("{AGENTS_ROOT}/")) { + ( + std::path::PathBuf::from(format!("{AGENTS_ROOT}/{rest}")), + AGENTS_ROOT, + ) + } else if let Some(rest) = raw.strip_prefix(&format!("{SHARED_ROOT}/")) { + ( + std::path::PathBuf::from(format!("{SHARED_ROOT}/{rest}")), + SHARED_ROOT, + ) + } else { + return Err(format!("path not in allow-list: {raw}")); + }; + reject_symlinks_below(std::path::Path::new(root), &mapped)?; + let canonical = std::fs::canonicalize(&mapped) + .map_err(|e| format!("{}: {e}", mapped.display()))?; if !(canonical.starts_with(AGENTS_ROOT) || canonical.starts_with(SHARED_ROOT)) { return Err(format!( "resolved path escapes allow-list: {}", @@ -992,7 +1035,134 @@ fn resolve_state_path(raw: &str) -> std::result::Result std::result::Result<(), String> { + let Ok(rel) = mapped.strip_prefix(root) else { + return Ok(()); + }; + let mut cumulative = root.to_path_buf(); + for component in rel.components() { + match component { + std::path::Component::Normal(name) => { + cumulative.push(name); + match std::fs::symlink_metadata(&cumulative) { + Ok(m) if m.file_type().is_symlink() => { + return Err(format!( + "symlink at {} not allowed (canonicalize would resolve it past the \ + allow-list check; refuse outright)", + cumulative.display() + )); + } + Ok(_) | Err(_) => {} + } + } + std::path::Component::ParentDir => { + return Err(format!( + "path contains `..` traversal below {}; refuse outright", + root.display() + )); + } + _ => {} + } + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::os::unix::fs::symlink; + + /// Make a unique tmp subdir for the calling test. Caller is responsible + /// for cleanup (we leak on panic, fine for ephemeral CI runs). + fn tmproot(tag: &str) -> std::path::PathBuf { + let ts = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0); + let p = std::env::temp_dir().join(format!("hyperhive-test-{tag}-{ts}")); + std::fs::create_dir_all(&p).unwrap(); + p + } + + #[test] + fn reject_symlinks_below_accepts_plain_dirs_and_files() { + let root = tmproot("symlink-ok"); + std::fs::create_dir_all(root.join("alice/state")).unwrap(); + std::fs::write(root.join("alice/state/notes.md"), b"hi").unwrap(); + assert!(reject_symlinks_below(&root, &root.join("alice/state/notes.md")).is_ok()); + } + + #[test] + fn reject_symlinks_below_rejects_leaf_symlink() { + let root = tmproot("symlink-leaf"); + std::fs::create_dir_all(root.join("alice/state")).unwrap(); + // Plant a symlink that points anywhere; resolve_state_path's + // canonicalize would happily resolve it past the allow-list + // check, so we have to refuse at the un-canonical layer. + symlink("/etc/shadow", root.join("alice/state/peek")).unwrap(); + let err = reject_symlinks_below(&root, &root.join("alice/state/peek")).unwrap_err(); + assert!(err.contains("symlink at"), "msg = {err}"); + assert!(err.contains("peek"), "msg = {err}"); + } + + #[test] + fn reject_symlinks_below_rejects_directory_symlink_in_middle() { + let root = tmproot("symlink-mid"); + std::fs::create_dir_all(root.join("real/state")).unwrap(); + std::fs::write(root.join("real/state/secret.md"), b"hi").unwrap(); + // alice's "state" dir is actually a symlink to real/state — a + // sub-agent shouldn't be able to plant this and proxy real's + // private files via the dashboard. + std::fs::create_dir_all(root.join("alice")).unwrap(); + symlink(root.join("real/state"), root.join("alice/state")).unwrap(); + let err = reject_symlinks_below(&root, &root.join("alice/state/secret.md")).unwrap_err(); + assert!(err.contains("symlink at"), "msg = {err}"); + } + + #[test] + fn reject_symlinks_below_rejects_parent_dir_traversal() { + let root = tmproot("symlink-dotdot"); + // `..` doesn't survive canonicalize anyway, but we want a + // friendlier error than "path escapes allow-list" — refusing + // upfront also avoids walking ancestors with `symlink_metadata`. + let p = root.join("alice/state/../escape"); + let err = reject_symlinks_below(&root, &p).unwrap_err(); + assert!(err.contains("`..`"), "msg = {err}"); + } + + #[test] + fn reject_symlinks_below_passes_through_when_path_not_under_root() { + // resolve_state_path's earlier allow-list check would reject + // this; reject_symlinks_below stays a no-op so the caller + // surfaces the better-fit error. + let root = std::path::Path::new("/var/lib/hyperhive/agents"); + assert!(reject_symlinks_below(root, std::path::Path::new("/etc/shadow")).is_ok()); + } } /// Snapshot the current tombstone list and emit a @@ -1063,10 +1233,10 @@ pub(crate) fn scan_validated_paths(body: &str) -> Vec { if out.iter().any(|s| s == token) { continue; } - if let Ok(canonical) = resolve_state_path(token) { - if std::fs::metadata(&canonical).is_ok_and(|m| m.is_file()) { - out.push(token.to_owned()); - } + if let Ok((_canonical, meta)) = resolve_state_path(token) + && meta.is_file() + { + out.push(token.to_owned()); } } out @@ -1076,14 +1246,10 @@ async fn get_state_file( axum::extract::Query(q): axum::extract::Query, ) -> Response { const MAX_BYTES: usize = 1 << 20; // 1 MiB - let canonical = match resolve_state_path(&q.path) { - Ok(p) => p, + let (canonical, meta) = match resolve_state_path(&q.path) { + Ok(pair) => pair, Err(e) => return error_response(&format!("state-file: {e}")), }; - let meta = match std::fs::metadata(&canonical) { - Ok(m) => m, - Err(e) => return error_response(&format!("state-file: stat {}: {e}", canonical.display())), - }; if !meta.is_file() { return error_response(&format!( "state-file: {} is not a regular file",