diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 54e67c7f..9c4adcd2 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -535,6 +535,39 @@ async fn control_infra_container( )) } +/// Create/overwrite `dir/filename` at 0600 without following a symlink at the +/// leaf, returning the open fd for the caller to `fchown`. `filename` must be a +/// single plain component (no `/`, `.`, `..`) — the leaf sits in an +/// agent-writable dir, so `O_NOFOLLOW` refuses a planted symlink (`ELOOP`) +/// instead of letting this root-privileged write/chmod be redirected at another +/// file; `O_WRONLY` refuses a directory leaf (`EISDIR`); `O_TRUNC` keeps the +/// overwrite semantics for an existing regular file. `.mode(0o600)` sets the +/// create mode; the explicit `fchmod` after (on the fd, not a re-resolved path) +/// tightens an already-existing file and dodges umask. The returned fd is the +/// exact inode the write hit, so the caller's `fchown` is TOCTOU-immune. +fn write_state_file_nofollow(dir: &Path, filename: &str, content: &str) -> Result { + use std::io::Write as _; + use std::os::unix::fs::{OpenOptionsExt as _, PermissionsExt as _}; + + if filename.is_empty() || filename == "." || filename == ".." || filename.contains('/') { + bail!("write_state_file_nofollow: refusing non-plain filename {filename:?}"); + } + let path = dir.join(filename); + let mut file = std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .custom_flags(libc::O_NOFOLLOW) + .open(&path) + .with_context(|| format!("open (no-follow) {}", path.display()))?; + file.write_all(content.as_bytes()) + .with_context(|| format!("write {}", path.display()))?; + file.set_permissions(std::fs::Permissions::from_mode(0o600)) + .with_context(|| format!("chmod 600 {}", path.display()))?; + Ok(file) +} + /// Shared helper for `WriteAgentForgeToken` and `WriteAgentMatrixToken`. /// Writes `content` to `AGENT_STATE_ROOT//state/`, /// chowns to the agent user (derived from the state dir's existing owner), @@ -545,7 +578,8 @@ fn write_agent_state_file( filename: &str, content: &str, ) -> Result<(String, String)> { - use std::os::unix::fs::{MetadataExt as _, PermissionsExt as _}; + use std::os::fd::AsRawFd as _; + use std::os::unix::fs::MetadataExt as _; let state_dir = PathBuf::from(AGENT_STATE_ROOT) .join(agent_name) @@ -560,23 +594,29 @@ fn write_agent_state_file( std::fs::create_dir_all(&state_dir) .with_context(|| format!("create state dir {}", state_dir.display()))?; + // Security-critical: refuses a symlink the (state/-owning) agent may have + // planted at the leaf, so this root-privileged create/write/chmod/chown + // can't be redirected at an arbitrary file. See `write_state_file_nofollow`. let path = state_dir.join(filename); - std::fs::write(&path, content).with_context(|| format!("write {}", path.display()))?; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) - .with_context(|| format!("chmod 600 {}", path.display()))?; + let file = write_state_file_nofollow(&state_dir, filename, content)?; // Chown to the state dir's owner so the agent process can read the file. - // If stat fails (e.g. dir just created, owner is root), the file stays - // root-owned and 0600 — still unreadable by others, just not agent-readable. - // Log a warning so operators can diagnose. + // fchown on the same fd — TOCTOU-immune (the inode the write hit, never a + // swapped path). If stat fails (e.g. dir just created, owner is root), the + // file stays root-owned and 0600 — still unreadable by others, just not + // agent-readable. Log a warning so operators can diagnose. match std::fs::metadata(&state_dir) { Ok(meta) => { - if let Err(e) = std::os::unix::fs::chown(&path, Some(meta.uid()), Some(meta.gid())) { + // SAFETY: `file` is an open, owned fd live for the whole call; + // `fchown` only mutates that inode's uid/gid. + let rc = unsafe { libc::fchown(file.as_raw_fd(), meta.uid(), meta.gid()) }; + if rc != 0 { + let e = std::io::Error::last_os_error(); tracing::warn!( agent = %agent_name, path = %path.display(), error = %e, - "write_agent_state_file: chown failed" + "write_agent_state_file: fchown failed" ); } } @@ -1591,3 +1631,68 @@ async fn sync_agent_tmpfiles(agents: &[String]) -> Result<(String, String)> { } Ok((String::new(), String::new())) } + +#[cfg(test)] +mod tests { + use super::write_state_file_nofollow; + use std::path::PathBuf; + use std::sync::atomic::{AtomicU32, Ordering}; + + /// Unique scratch dir per test, no external tempfile dep. + fn scratch() -> PathBuf { + static CTR: AtomicU32 = AtomicU32::new(0); + let n = CTR.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!( + "hive-priv-nofollow-test-{}-{n}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + dir + } + + #[test] + fn rejects_non_plain_filenames() { + let dir = scratch(); + for bad in ["", ".", "..", "a/b", "/etc/passwd", "../escape", "sub/tok"] { + assert!( + write_state_file_nofollow(&dir, bad, "x").is_err(), + "must reject filename {bad:?}" + ); + } + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn refuses_symlink_leaf_and_leaves_target_untouched() { + let dir = scratch(); + let target = dir.join("target"); + std::fs::write(&target, "original").unwrap(); + // Agent plants a symlink where the token would be written. + std::os::unix::fs::symlink(&target, dir.join("forge-token")).unwrap(); + + let res = write_state_file_nofollow(&dir, "forge-token", "PWNED"); + assert!(res.is_err(), "O_NOFOLLOW must refuse a symlink leaf"); + // The root-privileged write must NOT have followed the link. + assert_eq!( + std::fs::read_to_string(&target).unwrap(), + "original", + "symlink target must be untouched" + ); + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn writes_plain_file_0600() { + use std::os::unix::fs::PermissionsExt as _; + let dir = scratch(); + write_state_file_nofollow(&dir, "forge-token", "secret").unwrap(); + let path = dir.join("forge-token"); + assert_eq!(std::fs::read_to_string(&path).unwrap(), "secret"); + let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "token file must be 0600"); + // Overwrite truncates cleanly (O_TRUNC), no residue. + write_state_file_nofollow(&dir, "forge-token", "new").unwrap(); + assert_eq!(std::fs::read_to_string(&path).unwrap(), "new"); + std::fs::remove_dir_all(&dir).ok(); + } +}