From f434e9b7edbdaaaeab51accb24fbfa15ccfef0a9 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Mon, 14 Sep 2026 15:51:57 -0400 Subject: [PATCH] fix(agentd,pds)!: compare a directory's owner instead of inferring it secure_dir and the write-ahead log's create_dir read ownership off whether set_permissions returned an error, which succeeds for a process holding CAP_FOWNER, and followed a symlink planted at the path. Both now stat with symlink_metadata, compare the owner against the uid this process creates files as, refuse a directory whose sticky bit marks it as shared, and say so in the log when they do tighten one. Change-Id: I25df3e581cdb9fccf89f3682a42c991ee84c0e9a --- crates/didbot-agentd/src/socket.rs | 253 +++++++++++++++++++++++++++-- crates/didbot-pds/src/wal/mod.rs | 243 ++++++++++++++++++++++++--- 2 files changed, 458 insertions(+), 38 deletions(-) diff --git a/crates/didbot-agentd/src/socket.rs b/crates/didbot-agentd/src/socket.rs index 4bddad19..3376360f 100644 --- a/crates/didbot-agentd/src/socket.rs +++ b/crates/didbot-agentd/src/socket.rs @@ -83,11 +83,14 @@ pub struct Listener { impl Listener { /// Binds `path`, hardening the directory and the socket file. /// - /// The directory is created at `0700`, or tightened to `0700` if it is - /// already there — which fails rather than proceeding when this process - /// does not own it, and that refusal is the point. An attacker who - /// pre-creates the directory is caught here, and one who tries to bind - /// first inside a directory they do not own never gets the chance. + /// The directory is created at `0700`. One that is already there is + /// checked before anything is done to it — a symlink, something that is + /// not a directory, a directory shared between users, or one belonging to + /// another user is refused and left as it was — and tightened to `0700` + /// only once it is this process's own. That refusal is the point. An + /// attacker who pre-creates the directory is caught here, and one who + /// tries to bind first inside a directory they do not own never gets the + /// chance. /// /// The socket file is then set to `0600` explicitly, rather than left to /// whatever `umask` this process happens to be running under. @@ -188,20 +191,149 @@ pub fn remove_if_stale(path: &Path) { } } -/// Creates `dir` at `0700`, or tightens an existing one to `0700`. +/// Creates `dir` at `0700`, or takes up an existing one it is allowed to. /// -/// Tightening fails when this process does not own the directory, which is -/// the check that closes the pre-creation race. The state directory holding -/// the node key gets the same treatment, from the same function. +/// A directory that was already there is checked before anything is done to +/// it, and refused when it is a symlink, when it is not a directory, when its +/// sticky bit marks it as shared between users, or when it belongs to another +/// user. Ownership is compared rather than inferred from whether a chmod +/// succeeded: `chmod(2)` succeeds for a process holding `CAP_FOWNER`, so +/// under root an attacker's pre-created directory was being narrowed and used +/// instead of refused. +/// +/// One that passes all of that and is wider than `0700` is tightened to it, +/// with a warning naming the directory: whatever umask made it left the node +/// key only as private as that guess, and a run that quietly re-permissions a +/// directory somebody named on the command line should at least say so. +/// +/// The state directory holding the node key gets the same treatment, from +/// the same function. pub(crate) fn secure_dir(dir: &Path) -> io::Result<()> { - if dir.is_dir() { - return std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o700)); - } use std::os::unix::fs::DirBuilderExt; - std::fs::DirBuilder::new() - .recursive(true) + + if let Some(parent) = dir.parent().filter(|parent| !parent.as_os_str().is_empty()) { + std::fs::DirBuilder::new() + .recursive(true) + .mode(0o700) + .create(parent)?; + } + // The last component alone, and not recursive, so `mkdir(2)`'s own + // atomicity answers "was this already here": a racing pre-creation + // arrives as `AlreadyExists` and goes through the checks below rather + // than being silently adopted. + match std::fs::DirBuilder::new() + .recursive(false) .mode(0o700) .create(dir) + { + Ok(()) => Ok(()), + Err(error) if error.kind() == io::ErrorKind::AlreadyExists => accept_existing(dir), + Err(error) => Err(error), + } +} + +/// Takes up a directory that was already there, or says why it will not. +fn accept_existing(dir: &Path) -> io::Result<()> { + // `symlink_metadata`, not `metadata`: a symlink planted at this path + // redirects everything that follows, and `Path::is_dir` would follow it + // and answer about the target. `remove_if_stale` takes the same care for + // the socket file. + let metadata = std::fs::symlink_metadata(dir)?; + if metadata.file_type().is_symlink() { + return Err(refused(dir, "is a symlink")); + } + if !metadata.is_dir() { + return Err(refused(dir, "is not a directory")); + } + let mode = metadata.permissions().mode() & 0o7777; + // The sticky bit is what marks a directory shared between users -- + // `/tmp` is the case -- and tightening one takes it away from every + // other process on the host. A restored data directory carries whatever + // mode the snapshot had and no sticky bit, so it is tightened below the + // way `docs/operations.md` says it is. + if mode & 0o1000 != 0 { + return Err(refused( + dir, + &format!( + "is mode {mode:04o}: its sticky bit marks a directory shared between users. \ + Name a directory of this deployment's own -- making this one private would \ + take it away from everything else on this host." + ), + )); + } + // Before the chmod, not inferred from it. + refuse_unless_owned(dir, creating_uid(dir)?)?; + if mode & 0o077 != 0 { + warn!( + dir = %dir.display(), + found = format!("{mode:04o}"), + "tightening a directory this process did not create to 0700" + ); + std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o700))?; + } + Ok(()) +} + +/// Refuses `dir` unless it belongs to `ours`. +/// +/// Split out from [`accept_existing`] because this is the refusal that +/// closes the pre-creation race, and an unprivileged test cannot make a +/// directory belong to somebody else to reach it any other way. +fn refuse_unless_owned(dir: &Path, ours: u32) -> io::Result<()> { + let owner = std::fs::symlink_metadata(dir)?.uid(); + if owner != ours { + return Err(refused( + dir, + &format!("belongs to uid {owner}, and this process writes as uid {ours}"), + )); + } + Ok(()) +} + +/// The uid this process creates files as, read off one it creates in `dir`. +/// +/// The same way [`Listener::bind`] learns it from the socket it just bound: +/// a file this process created is owned by this process, so its own metadata +/// answers "who am I" without a second way to ask. Doing it inside `dir` +/// also settles that this process can write there, which everything holding +/// this directory goes on to need. +fn creating_uid(dir: &Path) -> io::Result { + use std::os::unix::fs::OpenOptionsExt; + + let probe = dir.join(probe_name()); + let file = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&probe) + .map_err(|error| refused(dir, &format!("cannot be written to: {error}")))?; + let uid = file.metadata()?.uid(); + drop(file); + let _ = std::fs::remove_file(&probe); + Ok(uid) +} + +/// A name for the ownership probe that no other caller is using. +/// +/// The process id alone is not enough: several threads open the same +/// directory at once, and a name they shared would make the second one’s +/// `create_new` collide with the first one’s probe. +fn probe_name() -> String { + use std::sync::atomic::{AtomicU64, Ordering}; + static NEXT: AtomicU64 = AtomicU64::new(0); + format!( + ".didbot-owner-check-{}-{}", + std::process::id(), + NEXT.fetch_add(1, Ordering::Relaxed) + ) +} + +/// A refusal naming the path and what is wrong with it. +fn refused(dir: &Path, why: &str) -> io::Error { + io::Error::new( + io::ErrorKind::PermissionDenied, + format!("{} {why}", dir.display()), + ) } #[cfg(test)] @@ -229,17 +361,106 @@ mod tests { assert_eq!(mode_of(listener.path()), 0o600, "socket"); } + /// A readable directory this process owns is tightened, as before. #[tokio::test] - async fn binding_tightens_a_directory_that_is_already_open() { + async fn binding_tightens_a_readable_directory_it_owns() { let scratch = Scratch::new("tighten"); std::fs::create_dir_all(&scratch.0).unwrap(); - std::fs::set_permissions(&scratch.0, std::fs::Permissions::from_mode(0o777)).unwrap(); + std::fs::set_permissions(&scratch.0, std::fs::Permissions::from_mode(0o755)).unwrap(); let _listener = Listener::bind(scratch.socket()).unwrap(); assert_eq!(mode_of(&scratch.0), 0o700); } + /// A directory shared between users is not one to tighten. `/tmp` is the + /// case: as an ordinary user the chmod failed and startup was refused + /// though nothing was wrong with `/tmp`, and under root it succeeded and + /// took the sticky world-writable mode off it for the whole host. + #[tokio::test] + async fn binding_refuses_a_shared_directory_and_changes_nothing() { + let scratch = Scratch::new("shared"); + std::fs::create_dir_all(&scratch.0).unwrap(); + std::fs::set_permissions(&scratch.0, std::fs::Permissions::from_mode(0o1777)).unwrap(); + + let refusal = Listener::bind(scratch.socket()).expect_err("a shared directory is refused"); + assert_eq!(refusal.kind(), io::ErrorKind::PermissionDenied); + assert!( + refusal.to_string().contains("1777"), + "the refusal must name the mode it found: {refusal}" + ); + assert_eq!( + std::fs::metadata(&scratch.0).unwrap().permissions().mode() & 0o7777, + 0o1777, + "left exactly as it was" + ); + assert!( + !scratch.socket().exists(), + "nothing was bound inside a directory this process refused" + ); + } + + /// `Path::is_dir` follows a symlink and answers about its target, so a + /// link planted at the socket's parent used to redirect both the chmod + /// and the bind. `remove_if_stale` already takes this care for the + /// socket file; the directory around it got none. + #[tokio::test] + async fn binding_refuses_a_symlink_at_the_sockets_directory() { + let real = Scratch::new("symlink-target"); + std::fs::create_dir_all(&real.0).unwrap(); + std::fs::set_permissions(&real.0, std::fs::Permissions::from_mode(0o700)).unwrap(); + let link = real.0.with_extension("link"); + let _ = std::fs::remove_file(&link); + std::os::unix::fs::symlink(&real.0, &link).unwrap(); + + let refusal = + Listener::bind(link.join(SOCKET_NAME)).expect_err("a symlink here is refused"); + assert!( + refusal.to_string().contains("symlink"), + "the refusal must say what it found: {refusal}" + ); + assert!( + std::fs::read_dir(&real.0).unwrap().next().is_none(), + "nothing was written through the link" + ); + let _ = std::fs::remove_file(&link); + } + + /// The ownership refusal, driven directly: an unprivileged test cannot + /// make a directory belong to somebody else, but it can ask the check + /// what it does when the two uids differ. Under root this is the case + /// `set_permissions` never failed for, because `chmod(2)` succeeds for a + /// process holding `CAP_FOWNER`. + #[tokio::test] + async fn a_directory_belonging_to_another_user_is_refused() { + let scratch = Scratch::new("foreign"); + std::fs::create_dir_all(&scratch.0).unwrap(); + std::fs::set_permissions(&scratch.0, std::fs::Permissions::from_mode(0o700)).unwrap(); + + let ours = std::fs::metadata(&scratch.0).unwrap().uid(); + let refusal = refuse_unless_owned(&scratch.0, ours.wrapping_add(1)) + .expect_err("a directory belonging to another uid is refused"); + assert!( + refusal.to_string().contains("belongs to uid"), + "the refusal must name both uids: {refusal}" + ); + assert_eq!(mode_of(&scratch.0), 0o700, "left exactly as it was"); + } + + #[tokio::test] + async fn a_path_that_is_not_a_directory_is_refused() { + let scratch = Scratch::new("notadir"); + std::fs::create_dir_all(&scratch.0).unwrap(); + let file = scratch.0.join("ordinary"); + std::fs::write(&file, b"").unwrap(); + + let refusal = secure_dir(&file).expect_err("a file is not a directory to bind in"); + assert!( + refusal.to_string().contains("not a directory"), + "the refusal must say what it found: {refusal}" + ); + } + #[tokio::test] async fn an_accepted_peer_is_this_process() { let scratch = Scratch::new("peer"); diff --git a/crates/didbot-pds/src/wal/mod.rs b/crates/didbot-pds/src/wal/mod.rs index f01cf9b9..c5aa029d 100644 --- a/crates/didbot-pds/src/wal/mod.rs +++ b/crates/didbot-pds/src/wal/mod.rs @@ -1706,34 +1706,147 @@ fn replay(dir: &Path, from: Mark, on_disk: &[(u64, u64)]) -> Result<(Replay, u64 /// Creates the data directory, private to its owner. /// -/// A directory this call creates gets `0700` from `DirBuilder::mode` and -/// nothing more is needed. One that is already there — a systemd unit's +/// A directory this call creates gets [`DIR_MODE`] from `DirBuilder::mode` +/// and nothing more is needed. One that is already there — a systemd unit's /// `StateDirectory`, a mounted volume, a directory a deployment script made -/// with `mkdir` — was never passed through that builder, so it keeps whatever -/// the umask that created it happened to be, and the signing keys this -/// directory is about to hold are only as private as that guess. So an -/// existing directory is tightened explicitly rather than trusted: the -/// permission is set every time this runs, which costs one syscall on the -/// common path where the directory was already `0700` and is what actually -/// closes the gap on the path where it was not. +/// with `mkdir`, a restored snapshot — was never passed through that builder, +/// so it keeps whatever the umask that created it happened to be, and the +/// signing keys this directory is about to hold are only as private as that +/// guess. It is checked before anything is done to it, and refused when it is +/// a symlink, when it is not a directory, when its sticky bit marks it as +/// shared between users, or when it belongs to another user. Ownership is +/// compared rather than inferred from whether a chmod succeeded: `chmod(2)` +/// succeeds for a process holding `CAP_FOWNER`. One that passes all of that +/// and is wider than [`DIR_MODE`] is tightened to it, with a warning naming +/// the directory — `docs/operations.md`'s point that a restore's step is +/// `chown` and not `chmod` still holds. pub(crate) fn create_dir(dir: &Path) -> Result<(), WalError> { - if dir.is_dir() { - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; + let mut leaf = std::fs::DirBuilder::new(); + leaf.recursive(false); + let mut parents = std::fs::DirBuilder::new(); + parents.recursive(true); + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt; + leaf.mode(DIR_MODE); + parents.mode(DIR_MODE); + } + + if let Some(parent) = dir.parent().filter(|parent| !parent.as_os_str().is_empty()) { + parents + .create(parent) + .map_err(|err| WalError::io(parent, err))?; + } + // The last component alone, and not recursive, so `mkdir(2)`'s own + // atomicity answers "was this already here": a racing pre-creation + // arrives as `AlreadyExists` and goes through the checks below rather + // than being adopted silently. + match leaf.create(dir) { + Ok(()) => Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => accept_existing_dir(dir), + Err(err) => Err(WalError::io(dir, err)), + } +} + +/// Whether a directory that was already there is one this process may use. +fn accept_existing_dir(dir: &Path) -> Result<(), WalError> { + let refuse = |why: String| { + WalError::io( + dir, + std::io::Error::new(std::io::ErrorKind::PermissionDenied, why), + ) + }; + // `symlink_metadata`, not `metadata`: a symlink planted at this path + // redirects every file the log is about to write, and `Path::is_dir` + // would follow it and answer about the target. + let metadata = std::fs::symlink_metadata(dir).map_err(|err| WalError::io(dir, err))?; + if metadata.file_type().is_symlink() { + return Err(refuse("is a symlink".to_owned())); + } + if !metadata.is_dir() { + return Err(refuse("is not a directory".to_owned())); + } + #[cfg(unix)] + { + use std::os::unix::fs::{MetadataExt, OpenOptionsExt, PermissionsExt}; + + let mode = metadata.permissions().mode() & 0o7777; + // The sticky bit is what marks a directory shared between users -- + // `/tmp` is the case -- and tightening one takes it away from every + // other process on the host. A restored data directory carries + // whatever mode the snapshot had and no sticky bit, so it is + // tightened below the way `docs/operations.md` says it is. + if mode & 0o1000 != 0 { + return Err(refuse(format!( + "is mode {mode:04o}: its sticky bit marks a directory shared between users. \ + Name a directory of this deployment's own -- making this one private would \ + take it away from everything else on this host." + ))); + } + // A file this process creates is owned by this process, so its own + // metadata answers "who am I" without a second way to ask. Doing it + // inside `dir` also settles that this process can write there, which + // every later append needs. Before the chmod, not inferred from it. + let probe = dir.join(probe_name()); + let file = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(FILE_MODE) + .open(&probe) + .map_err(|err| refuse(format!("cannot be written to: {err}")))?; + let ours = file + .metadata() + .map_err(|err| WalError::io(&probe, err))? + .uid(); + drop(file); + let _ = std::fs::remove_file(&probe); + refuse_unless_owned(dir, metadata.uid(), ours)?; + if mode & 0o077 != 0 { + tracing::warn!( + dir = %dir.display(), + found = format!("{mode:04o}"), + "tightening a data directory this process did not create to {DIR_MODE:o}" + ); std::fs::set_permissions(dir, std::fs::Permissions::from_mode(DIR_MODE)) .map_err(|err| WalError::io(dir, err))?; } - return Ok(()); } - let mut builder = std::fs::DirBuilder::new(); - builder.recursive(true); - #[cfg(unix)] - { - use std::os::unix::fs::DirBuilderExt; - builder.mode(DIR_MODE); + Ok(()) +} + +/// A name for the ownership probe that no other caller is using. +/// +/// The process id alone is not enough: several threads open the same +/// directory at once, and a name they shared would make the second one’s +/// `create_new` collide with the first one’s probe. +#[cfg(unix)] +fn probe_name() -> String { + use std::sync::atomic::{AtomicU64, Ordering}; + static NEXT: AtomicU64 = AtomicU64::new(0); + format!( + ".didbot-owner-check-{}-{}", + std::process::id(), + NEXT.fetch_add(1, Ordering::Relaxed) + ) +} + +/// Refuses a directory belonging to `owner` unless that is `ours`. +/// +/// Split out because this is the refusal that closes the pre-creation race, +/// and an unprivileged test cannot make a directory belong to somebody else +/// to reach it any other way. +#[cfg(unix)] +fn refuse_unless_owned(dir: &Path, owner: u32, ours: u32) -> Result<(), WalError> { + if owner == ours { + return Ok(()); } - builder.create(dir).map_err(|err| WalError::io(dir, err)) + Err(WalError::io( + dir, + std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + format!("belongs to uid {owner}, and this process writes as uid {ours}"), + ), + )) } /// Opens the log for appending, `0600`, whether or not it was already there. @@ -2309,7 +2422,7 @@ mod tests { use std::os::unix::fs::PermissionsExt; let dir = scratch("preexisting"); - // Made with the default mode a plain `mkdir` would give it, wide open + // Made with the default mode a plain `mkdir` would give it, readable // rather than private, before `create_dir` ever sees it. std::fs::create_dir_all(&dir).expect("mkdir"); std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).expect("chmod"); @@ -2325,6 +2438,92 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + /// A directory whose sticky bit marks it as shared between users — + /// `/tmp` is the case — is refused rather than tightened: making it + /// private would take it away from every other process on the host. + #[cfg(unix)] + #[test] + fn a_shared_directory_is_refused_and_left_as_it_was() { + use std::os::unix::fs::PermissionsExt; + + let dir = scratch("shared"); + std::fs::create_dir_all(&dir).expect("mkdir"); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o1777)).expect("chmod"); + + let refusal = create_dir(&dir).expect_err("a shared directory must be refused"); + assert!( + refusal.to_string().contains("1777"), + "the refusal must name the mode it found: {refusal}" + ); + assert_eq!( + std::fs::metadata(&dir) + .expect("stat the directory") + .permissions() + .mode() + & 0o7777, + 0o1777, + "a refused directory must be left exactly as it was found" + ); + let _ = std::fs::remove_dir_all(&dir); + } + + /// The refusal the documented one never was: `set_permissions` succeeds + /// for a process holding `CAP_FOWNER`, so under root an attacker's + /// pre-created directory was tightened and used rather than refused. + /// Driven directly because an unprivileged test cannot make a directory + /// belong to somebody else. + #[cfg(unix)] + #[test] + fn a_directory_belonging_to_another_user_is_refused() { + let dir = scratch("foreign"); + create_dir(&dir).expect("a directory this process owns"); + + let refusal = refuse_unless_owned(&dir, 4242, 4243) + .expect_err("a directory belonging to another uid must be refused"); + assert!( + refusal.to_string().contains("belongs to uid 4242"), + "the refusal must name both uids: {refusal}" + ); + refuse_unless_owned(&dir, 4242, 4242).expect("and accept one that is this process's"); + let _ = std::fs::remove_dir_all(&dir); + } + + /// The ownership check leaves nothing behind in a directory it accepts. + #[cfg(unix)] + #[test] + fn taking_up_an_existing_directory_leaves_nothing_in_it() { + let dir = scratch("already-private"); + create_dir(&dir).expect("create it"); + create_dir(&dir).expect("and take it up again on the next open"); + assert!( + std::fs::read_dir(&dir).expect("list it").next().is_none(), + "the ownership check must leave nothing behind" + ); + let _ = std::fs::remove_dir_all(&dir); + } + + /// `Path::is_dir` follows a symlink and answers about its target, so a + /// link planted at the data directory's path used to redirect every file + /// the log writes. + #[cfg(unix)] + #[test] + fn a_symlink_at_the_data_directory_is_refused() { + let real = scratch("symlink-target"); + create_dir(&real).expect("a real directory this process owns"); + let link = scratch("symlink-link"); + let _ = std::fs::remove_file(&link); + std::os::unix::fs::symlink(&real, &link).expect("plant the link"); + + let refusal = create_dir(&link).expect_err("a symlink must be refused"); + assert!( + refusal.to_string().contains("symlink"), + "the refusal must say what it found: {refusal}" + ); + + let _ = std::fs::remove_file(&link); + let _ = std::fs::remove_dir_all(&real); + } + /// A log file this process did not create — one written by a build from /// before the mode was set, restored from a backup, or copied into place /// by hand — is tightened rather than trusted, for the same reason -- 2.51.2