diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 571a7976..2cb567f3 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -1355,6 +1355,24 @@ async fn control_infra_container( )) } +/// A leaf safe to join onto an agent-owned directory: one plain component, so +/// it can neither climb out of the directory nor name the directory itself. +fn ensure_plain_filename(who: &str, filename: &str) -> Result<()> { + if filename.is_empty() || filename == "." || filename == ".." || filename.contains('/') { + bail!("{who}: refusing non-plain filename {filename:?}"); + } + Ok(()) +} + +/// Name the content is written under before being renamed onto `filename`. +/// The leading dot is load-bearing rather than tidy: `nix/agent-modules/ +/// matrix.nix` starts the agent's matrix daemon on the glob `matrix-token*`, +/// which a `.partial` suffix would match — waking it on precisely the +/// empty file the rename exists to hide. +fn partial_name(filename: &str) -> String { + format!(".{filename}.partial") +} + /// 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 @@ -1369,9 +1387,7 @@ fn write_state_file_nofollow(dir: &Path, filename: &str, content: &str) -> Resul 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:?}"); - } + ensure_plain_filename("write_state_file_nofollow", filename)?; let path = dir.join(filename); let mut file = std::fs::OpenOptions::new() .write(true) @@ -1469,6 +1485,11 @@ fn remove_marker_in(dir: &Path, filename: &str) -> Result<()> { /// writes (which target `state/`) and the pause marker (which targets /// `harness/`) — both write into a directory owned by the agent, which is /// precisely why they need hive-priv at all. +/// +/// The file is published by `rename`, so a reader woken by its appearance +/// cannot catch it empty or half-written: several of these paths have a +/// `systemd.path` unit watching them, and one of those triggers on the file +/// existing rather than changing. fn write_agent_dir_file( agent_name: &str, dir: &Path, @@ -1478,6 +1499,10 @@ fn write_agent_dir_file( use std::os::fd::AsRawFd as _; use std::os::unix::fs::MetadataExt as _; + // The leaf is validated here as well as in `write_state_file_nofollow`: + // that call now sees the temp name, which is plain whatever `filename` is. + ensure_plain_filename("write_agent_dir_file", filename)?; + let state_dir = dir.to_path_buf(); // NOTE: `create_dir_all` is normally a no-op — lifecycle creates and chowns // the state dir during spawn. On the rare edge where the dir doesn't exist @@ -1493,7 +1518,9 @@ fn write_agent_dir_file( // 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); - let file = write_state_file_nofollow(&state_dir, filename, content)?; + let tmp_name = partial_name(filename); + let tmp_path = state_dir.join(&tmp_name); + let file = write_state_file_nofollow(&state_dir, &tmp_name, content)?; // Chown to the state dir's owner so the agent process can read the file. // fchown on the same fd — TOCTOU-immune (the inode the write hit, never a @@ -1523,6 +1550,14 @@ fn write_agent_dir_file( ); } } + // After the chown, never before: the file must never be visible under its + // final name while still root-owned. Leaving the temp behind on failure + // would also leave a credential readable by nobody but root, so it goes. + if let Err(e) = std::fs::rename(&tmp_path, &path) { + let _ = std::fs::remove_file(&tmp_path); + return Err(e).with_context(|| format!("publishing {}", path.display())); + } + tracing::info!( agent = %agent_name, dir = %state_dir.display(), @@ -1539,9 +1574,7 @@ fn write_agent_dir_file( /// label into a fixed `forge-