fix(#2314): refuse symlink leaf in hive-priv write_agent_state_file (O_NOFOLLOW + fd-based chmod/chown)

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<std::fs::File> {
+    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/<agent_name>/state/<filename>`,
 /// 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();
+    }
+}