From 6dde36b02d20ad6a7c05141c668b9b0b9364aaaa Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 14 Aug 2026 11:41:55 -0500 Subject: [PATCH 1/3] fix(node): refuse a p2p key or key directory owned by another user Mode bits were the only thing checked, and they do not make something node-owned. A 0700 directory or a 0600 key file belonging to a different user passes every permission check here while that user keeps the ability to replace what is inside it, which means they choose the node's libp2p identity. That is the capability the persisted key exists to take away. Both sites now bail rather than warn. Unlike a loose mode this is not repairable: chown needs privilege the node should not have, and taking ownership of someone else's file would be wrong even if it could. In ensure_key_dir the check runs before the mode repair, because a directory we do not own fails its chmod with EPERM and reports "could not be tightened", which describes the symptom and sends the operator at the wrong thing. Testing this needed a seam. A test cannot chown a fixture to another user without root, so a fixture-based test could only ever exercise the matching case, which is a guard nobody has watched refuse anything. So the decision is a pure function taking both uids, and a #[cfg(test)] euid override (the same thread-local shape as the existing FAIL_KEY_WRITE injector) lets the wiring tests drive the real read and directory paths while pretending to be a different user. A further test pins that the seam defaults to the real geteuid, since one that quietly stopped consulting it would leave every other ownership test passing against nothing. Found during review of the key-persistence change by two independent reviewers. --- crates/gitlawb-node/src/p2p/mod.rs | 229 +++++++++++++++++++++++++++-- 1 file changed, 219 insertions(+), 10 deletions(-) diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index dff49971..587db471 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -244,6 +244,43 @@ pub(crate) fn names_no_usable_directory(key_path: &Path) -> bool { !named_a_directory } +/// Why a key file or key directory owned by another user is refused, if it is. +/// +/// Mode bits alone do not make something node-owned. A `0700` directory or a +/// `0600` file belonging to a different user passes every permission check here +/// while that user keeps the ability to replace what is inside it, which means +/// they choose the node's libp2p identity. That is the capability the persisted +/// key exists to take away, so it is refused rather than warned about. +/// +/// Unlike a loose mode this is not repairable: `chown` needs privilege the node +/// should not have, and taking ownership of another user's file would be the +/// wrong move even if it could. So the callers bail instead of tightening. +/// +/// Pure, and takes both uids as arguments, so both directions are testable +/// without privilege. A test cannot `chown` a fixture to another user without +/// root, and a guard that can only be exercised in one direction is the shape +/// that ships unproven. +#[cfg(unix)] +fn foreign_ownership_error(what: &str, path: &Path, owner_uid: u32, euid: u32) -> Option { + if owner_uid == euid { + return None; + } + Some(format!( + "p2p {what} {} is owned by uid {} but this node runs as uid {}; that user can \ + replace it and so decides the node's libp2p identity, which is what the persisted \ + key exists to prevent. Point {} at a location this user owns, or have the owner \ + hand it over; the node will not adopt it.", + path.display(), + owner_uid, + euid, + if what == "key directory" { + "GITLAWB_P2P_KEY's directory" + } else { + "GITLAWB_P2P_KEY" + } + )) +} + /// Load the node's persistent libp2p identity from `key_path`, generating and /// storing a fresh Ed25519 keypair the first time. pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result { @@ -336,13 +373,24 @@ fn ensure_key_dir(dir: &Path) -> Result<()> { #[cfg(unix)] { + use std::os::unix::fs::MetadataExt; use std::os::unix::fs::PermissionsExt; - let mode = std::fs::metadata(dir) - .with_context(|| format!("failed to stat key directory {}", dir.display()))? - .permissions() - .mode() - & 0o777; + let md = std::fs::metadata(dir) + .with_context(|| format!("failed to stat key directory {}", dir.display()))?; + + // Before the mode repair below, not after. A directory we do not own + // cannot be repaired by us: the chmod would fail with EPERM and report + // "could not be tightened", which describes the symptom and hides the + // cause. It also matters that a foreign directory sitting at 0700 + // passes the mode check silently today, so ownership is the only thing + // that catches it. + let euid = effective_uid(); + if let Some(err) = foreign_ownership_error("key directory", dir, md.uid(), euid) { + anyhow::bail!(err); + } + + let mode = md.permissions().mode() & 0o777; if mode & 0o077 != 0 { warn!( dir = %dir.display(), @@ -455,6 +503,27 @@ thread_local! { /// Test-only fault injection for the key write. Thread-local so an armed /// test cannot disturb the others running beside it. static FAIL_KEY_WRITE: std::cell::Cell = const { std::cell::Cell::new(false) }; + + /// Test-only override for the process effective uid, same thread-local + /// shape and for the same reason as `FAIL_KEY_WRITE`. + /// + /// The ownership refusal is otherwise untestable end to end: proving that + /// `read_p2p_keypair` and `ensure_key_dir` actually consult it needs a + /// fixture owned by a different user, and a test cannot `chown` one without + /// root. Pretending to be a different uid against a fixture we do own + /// exercises the identical branch and needs no privilege. + static EUID_OVERRIDE: std::cell::Cell> = const { std::cell::Cell::new(None) }; +} + +/// The effective uid the ownership checks compare against. +#[cfg(unix)] +fn effective_uid() -> u32 { + #[cfg(test)] + if let Some(uid) = EUID_OVERRIDE.with(|c| c.get()) { + return uid; + } + // SAFETY: `geteuid` only reads the calling process's effective uid. + unsafe { libc::geteuid() } } /// Read an existing key file, refusing one whose permissions or contents make @@ -463,13 +532,24 @@ thread_local! { fn read_p2p_keypair(key_path: &Path) -> Result { #[cfg(unix)] { + use std::os::unix::fs::MetadataExt; use std::os::unix::fs::PermissionsExt; - let mode = std::fs::metadata(key_path) - .with_context(|| format!("failed to stat p2p key at {}", key_path.display()))? - .permissions() - .mode() - & 0o777; + // One stat feeds both checks. Statting twice would leave a window in + // which the file the ownership check approved is not the file the mode + // check measured. + let md = std::fs::metadata(key_path) + .with_context(|| format!("failed to stat p2p key at {}", key_path.display()))?; + + // Ownership first: a key owned by someone else is not made safe by its + // mode, and saying "mode is fine" about a file we do not own would be + // the more misleading error of the two. + let euid = effective_uid(); + if let Some(err) = foreign_ownership_error("key", key_path, md.uid(), euid) { + anyhow::bail!(err); + } + + let mode = md.permissions().mode() & 0o777; if mode & 0o077 != 0 { anyhow::bail!( "p2p key at {} has mode {:04o}, which grants access beyond its owner; \ @@ -973,6 +1053,135 @@ mod tests { /// direction is covered above, where nothing is created by construction, /// and the gate and the backstop call this same function so they cannot /// disagree. + /// Guard for the seam itself: with no override armed, the checks use the + /// real process uid. Without this, every ownership test below could pass + /// against a seam that had quietly stopped consulting `geteuid` at all. + #[cfg(unix)] + #[test] + fn effective_uid_defaults_to_the_real_process_uid() { + // SAFETY: `geteuid` only reads the calling process's effective uid. + assert_eq!(effective_uid(), unsafe { libc::geteuid() }); + } + + /// `read_p2p_keypair` actually consults the ownership check. + /// + /// The fixture is owned by this user (a test cannot chown one to anyone + /// else without root), so the override supplies a different euid instead. + /// That drives the identical branch: the file's uid and the process uid + /// disagree. + #[cfg(unix)] + #[test] + fn read_p2p_keypair_refuses_a_key_owned_by_another_user() { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("p2p.key"); + let kp = identity::Keypair::generate_ed25519(); + std::fs::write(&path, kp.to_protobuf_encoding().unwrap()).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + + let real_uid = std::fs::metadata(&path).unwrap().uid(); + let other = real_uid.wrapping_add(1); + + EUID_OVERRIDE.with(|c| c.set(Some(other))); + let result = read_p2p_keypair(&path); + EUID_OVERRIDE.with(|c| c.set(None)); + + let err = format!( + "{:#}", + result.expect_err("a foreign-owned key must be refused") + ); + assert!( + err.contains("owned by uid") && err.contains("will not adopt it"), + "must be refused for ownership, not something else, got: {err}" + ); + + // And the same file loads once the uids agree, so the refusal is about + // ownership and not about the fixture being broken. + assert!( + read_p2p_keypair(&path).is_ok(), + "the same key must load when the owner matches" + ); + } + + /// `ensure_key_dir` consults it too, and does so BEFORE trying to repair the + /// mode. A loose directory we do not own must report ownership, not a failed + /// chmod, and a 0700 directory we do not own must still be refused even + /// though the mode check alone would pass it. + #[cfg(unix)] + #[test] + fn ensure_key_dir_refuses_a_directory_owned_by_another_user() { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + for mode in [0o700, 0o777] { + let dir = tempfile::tempdir().unwrap(); + let keys = dir.path().join("keys"); + std::fs::create_dir(&keys).unwrap(); + std::fs::set_permissions(&keys, std::fs::Permissions::from_mode(mode)).unwrap(); + + let real_uid = std::fs::metadata(&keys).unwrap().uid(); + EUID_OVERRIDE.with(|c| c.set(Some(real_uid.wrapping_add(1)))); + let result = ensure_key_dir(&keys); + EUID_OVERRIDE.with(|c| c.set(None)); + + let err = format!( + "{:#}", + result.expect_err("a foreign-owned key directory must be refused") + ); + assert!( + err.contains("owned by uid"), + "mode {mode:04o} must be refused for ownership, got: {err}" + ); + assert!( + !err.contains("could not be tightened"), + "ownership must be reported before the chmod is attempted, got: {err}" + ); + } + } + + /// Both directions of the ownership refusal, without needing root. + /// + /// The reason this is a pure function taking two uids rather than a stat of + /// a real fixture: a test cannot chown a file to another user without + /// privilege, so a fixture-based version could only ever exercise the + /// matching case. That is the shape that ships a guard nobody has seen + /// refuse anything. + #[cfg(unix)] + #[test] + fn foreign_ownership_is_refused_and_matching_ownership_is_not() { + let path = Path::new("/data/keys/p2p.key"); + + // Same user: no complaint, whatever the uid happens to be. + for uid in [0u32, 1000, 65534] { + assert!( + foreign_ownership_error("key", path, uid, uid).is_none(), + "uid {uid} owning its own key must not be refused" + ); + } + + // Different user: refused, and the message has to name both uids or an + // operator cannot tell which side is wrong. + let err = foreign_ownership_error("key", path, 1000, 1001) + .expect("a key owned by another uid must be refused"); + assert!( + err.contains("1000") && err.contains("1001"), + "the refusal must name both the owner and the running uid, got: {err}" + ); + assert!( + err.contains("/data/keys/p2p.key"), + "the refusal must name the path, got: {err}" + ); + + // Root running against a user-owned file is still a mismatch. This is + // the case worth pinning: root can read it anyway, so it is tempting to + // treat it as fine, but the other user can still replace the file and + // therefore still chooses the identity. + assert!( + foreign_ownership_error("key directory", path, 1000, 0).is_some(), + "a user-owned path under a root-run node is still foreign" + ); + } + #[test] fn names_no_usable_directory_covers_both_directions() { for path in [ From 5a93f2435c27c4d7f10786e24bf4b35256a4ef86 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 14 Aug 2026 12:19:53 -0500 Subject: [PATCH 2/3] fix(review): close the ancestor path and the stat/read window Review found the leaf checks were not the trust boundary. Both fixes come from the same observation: what the guard inspects and what the node then uses were not provably the same thing. An ancestor the node does not control launders an unsafe path into a safe looking one. A user owning /home/them/base can have the node use /home/them/base/keys/p2p.key; the node creates keys and the key, so both are node-owned, 0700 and 0600, and pass every check. That owner can then rename keys aside, let the node generate a fresh identity, and move the old directory back before a restart. They never own anything the leaf checks look at, and they decide which identity the node presents and when it rolls back. ensure_key_dir now walks the existing ancestors first, before creating anything, since a directory the node made would pass afterwards by construction. Root counts as trusted, or /data under a root-owned / refuses on every normal deployment. The mode rule is world-writable-without-sticky rather than group too: group write is a narrower capability that needs group membership, and refusing it would reject an ordinary umask-002 directory. Someone in an ancestor's group can still rename the key directory; that residual is real and stated rather than papered over. read_p2p_keypair statted the path and then read the path again, so the file approved by uid was not provably the file whose bytes became the identity. It now opens once with O_NOFOLLOW, takes uid and mode from that handle, and reads from it. The flag also refuses a symlink at the final component instead of following it. Two of the tests were weaker than their names. The ordering assertion passed under either ordering, because a test-owned fixture makes the chmod succeed so "could not be tightened" never appears; it now asserts the mode is untouched, which is what actually separates them. The call-site assertions matched a shared substring, so a swapped argument or a uid/gid mixup would have gone unnoticed; they now name both uids in order and check which knob the remediation points at. Also moves a doc comment that had drifted onto the wrong test. --- crates/gitlawb-node/src/p2p/mod.rs | 238 ++++++++++++++++++++++++++--- 1 file changed, 219 insertions(+), 19 deletions(-) diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index 587db471..f4d8ff81 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -281,6 +281,85 @@ fn foreign_ownership_error(what: &str, path: &Path, owner_uid: u32, euid: u32) - )) } +/// Refuse a key directory whose existing ancestors are controlled by someone +/// else, before creating anything inside them. +/// +/// Checking only the key directory and the key file is not enough, and the way +/// it fails is worth spelling out because it looks safe. A user who owns +/// `/home/them/base` can have the node use `/home/them/base/keys/p2p.key`. On +/// first start the node creates `keys` and the key itself, so both are +/// node-owned, `0700` and `0600`, and pass every check here. That owner never +/// needs to own either one: they can rename `keys` aside, let the node generate +/// a fresh identity in a new `keys`, and move the old directory back before a +/// later restart. Both directories pass, and they decide which identity the +/// node presents and when it rolls back. +/// +/// So the trust boundary is the whole existing chain, not the leaf. Walking up +/// from the deepest component that exists today, every ancestor must be owned by +/// this user or by root, and must not be group or other writable. +/// +/// Root counts as trusted on purpose. Requiring every ancestor to be +/// node-owned would refuse `/data/keys` under a root-owned `/data`, and `/` +/// itself, which is most real deployments. Root can already replace the binary, +/// so treating it as an attacker here would buy nothing. +/// +/// Only existing ancestors are inspected. The ones this call is about to create +/// inherit their parent, which the walk has already cleared. +#[cfg(unix)] +fn foreign_ancestor_error(dir: &Path, euid: u32) -> Option { + use std::os::unix::fs::MetadataExt; + use std::os::unix::fs::PermissionsExt; + + // `ancestors()` yields the path itself first, and that one is deliberately + // skipped. `ensure_key_dir` exists to create and tighten the key directory, + // so judging it here would refuse exactly the loose-but-ours case the repair + // is written for. + for ancestor in dir.ancestors().skip(1) { + let md = match std::fs::metadata(ancestor) { + Ok(md) => md, + // Does not exist yet, so it is one of the directories this call + // creates; keep walking up to the part of the path that is real. + Err(_) => continue, + }; + + let owner = md.uid(); + if owner != euid && owner != 0 { + return Some(format!( + "p2p key directory {} sits under {}, which is owned by uid {} rather than this \ + node (uid {}) or root; that user can rename or replace the directory holding \ + the key and so control which identity the node presents. Put the key somewhere \ + this user or root owns the whole path.", + dir.display(), + ancestor.display(), + owner, + euid + )); + } + + let mode = md.permissions().mode() & 0o777; + // The sticky bit is what makes a shared directory like /tmp survivable, + // since it stops non-owners removing entries someone else created. + let sticky = md.permissions().mode() & 0o1000 != 0; + // World-writable only, not group-writable. Group write on an ancestor is + // a real but much narrower capability (it needs group membership), and + // refusing it would reject an ordinary umask-002 home or service + // directory, which is most of them. The residual is stated in the PR + // rather than papered over: someone in the group of an ancestor can + // still rename the key directory. + if mode & 0o002 != 0 && !sticky { + return Some(format!( + "p2p key directory {} sits under {}, which has mode {:04o} and is writable \ + beyond its owner; anyone with that write access can rename or replace the \ + directory holding the key and so control which identity the node presents.", + dir.display(), + ancestor.display(), + mode + )); + } + } + None +} + /// Load the node's persistent libp2p identity from `key_path`, generating and /// storing a fresh Ed25519 keypair the first time. pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result { @@ -359,6 +438,19 @@ pub fn load_or_create_p2p_keypair(key_path: &Path) -> Result /// keys. Issue #231 owns the sibling gap in `main.rs`'s own creation path for /// that file; nothing here touches it. fn ensure_key_dir(dir: &Path) -> Result<()> { + // Before anything is created. Creating the directory first and checking + // afterwards is what lets a foreign-owned ancestor launder an unsafe path + // into a node-owned 0700 child: the child passes every check precisely + // because the node made it, while the ancestor's owner keeps the ability to + // swap it out. + #[cfg(unix)] + { + let euid = effective_uid(); + if let Some(err) = foreign_ancestor_error(dir, euid) { + anyhow::bail!(err); + } + } + let mut builder = std::fs::DirBuilder::new(); builder.recursive(true); #[cfg(unix)] @@ -505,7 +597,8 @@ thread_local! { static FAIL_KEY_WRITE: std::cell::Cell = const { std::cell::Cell::new(false) }; /// Test-only override for the process effective uid, same thread-local - /// shape and for the same reason as `FAIL_KEY_WRITE`. + /// shape and for the same reason as `FAIL_KEY_WRITE`. Unix-gated like its + /// only reader, or a non-unix test build carries it as dead code. /// /// The ownership refusal is otherwise untestable end to end: proving that /// `read_p2p_keypair` and `ensure_key_dir` actually consult it needs a @@ -530,15 +623,40 @@ fn effective_uid() -> u32 { /// it untrustworthy. Never regenerates: a node that silently replaces an /// unreadable key file would change its PeerId without the operator knowing. fn read_p2p_keypair(key_path: &Path) -> Result { + // One open, then everything is answered from that handle: the ownership + // check, the mode check, and the read itself. + // + // Statting the path and then reading the path again is the window that + // matters, and an earlier version of this had it. Between the two lookups + // the name can be pointed somewhere else, so the file approved by uid is + // not provably the file whose bytes become the identity. `fstat` on the fd + // cannot drift like that. + // + // `O_NOFOLLOW` refuses a symlink at the final component outright rather + // than reading through it. Planting one needs write access to the key + // directory, which the checks above are meant to deny, so this is the + // belt to that brace. #[cfg(unix)] - { + let bytes = { + use std::io::Read; use std::os::unix::fs::MetadataExt; + use std::os::unix::fs::OpenOptionsExt; use std::os::unix::fs::PermissionsExt; - // One stat feeds both checks. Statting twice would leave a window in - // which the file the ownership check approved is not the file the mode - // check measured. - let md = std::fs::metadata(key_path) + let mut file = std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW) + .open(key_path) + .with_context(|| { + format!( + "failed to open p2p key at {} (a symlink here is refused rather than \ + followed)", + key_path.display() + ) + })?; + + let md = file + .metadata() .with_context(|| format!("failed to stat p2p key at {}", key_path.display()))?; // Ownership first: a key owned by someone else is not made safe by its @@ -559,10 +677,16 @@ fn read_p2p_keypair(key_path: &Path) -> Result { key_path.display() ); } - } - // Same reason as the write path: this is the private key, so it gets - // scrubbed on drop instead of lingering in a heap buffer. + // Same reason as the write path: this is the private key, so it gets + // scrubbed on drop instead of lingering in a heap buffer. + let mut buf = Zeroizing::new(Vec::new()); + file.read_to_end(&mut buf) + .with_context(|| format!("failed to read p2p key from {}", key_path.display()))?; + buf + }; + + #[cfg(not(unix))] let bytes = Zeroizing::new( std::fs::read(key_path) .with_context(|| format!("failed to read p2p key from {}", key_path.display()))?, @@ -1045,14 +1169,6 @@ mod tests { } } - /// The predicate itself, over the whole input space in both directions. - /// - /// Deliberately does not call `load_or_create_p2p_keypair` on the accepted - /// paths: that would create directories and write a real key relative to - /// whatever directory the test process happens to run in. The rejected - /// direction is covered above, where nothing is created by construction, - /// and the gate and the backstop call this same function so they cannot - /// disagree. /// Guard for the seam itself: with no override armed, the checks use the /// real process uid. Without this, every ownership test below could pass /// against a seam that had quietly stopped consulting `geteuid` at all. @@ -1091,9 +1207,18 @@ mod tests { "{:#}", result.expect_err("a foreign-owned key must be refused") ); + // Naming both uids in the expected order is what makes this fail if the + // last two arguments are ever swapped, or if the check reads gid rather + // than uid. A shared substring like "owned by uid" passes under both. + assert!( + err.contains(&format!( + "owned by uid {real_uid} but this node runs as uid {other}" + )), + "the refusal must name the file's owner and the running uid in that order, got: {err}" + ); assert!( - err.contains("owned by uid") && err.contains("will not adopt it"), - "must be refused for ownership, not something else, got: {err}" + err.contains("GITLAWB_P2P_KEY") && !err.contains("GITLAWB_P2P_KEY's directory"), + "the key path refusal must point at the key knob, not the directory one, got: {err}" ); // And the same file loads once the uids agree, so the refusal is about @@ -1136,7 +1261,74 @@ mod tests { !err.contains("could not be tightened"), "ownership must be reported before the chmod is attempted, got: {err}" ); + // The message check above passes under EITHER ordering, because the + // fixture is test-owned so the chmod would succeed and never emit + // "could not be tightened". This is the assertion that actually + // separates them: if the ownership check ran after the repair, the + // mode would have been rewritten to 0700 before the bail. + assert_eq!( + std::fs::metadata(&keys).unwrap().permissions().mode() & 0o777, + mode, + "a refused directory must not have been chmodded first" + ); + } + } + + /// A foreign-owned ancestor is refused before anything is created under it. + /// + /// This is the case that survived the leaf checks: the node creates the key + /// directory and the key itself, so both are node-owned and correctly moded + /// and pass every other guard, while whoever owns the directory above can + /// rename the whole thing aside and swap an older one back. They choose the + /// identity without ever owning anything the leaf checks look at. + #[cfg(unix)] + #[test] + fn ensure_key_dir_refuses_a_foreign_owned_ancestor() { + use std::os::unix::fs::MetadataExt; + + let base = tempfile::tempdir().unwrap(); + let nested = base.path().join("keys"); + + let real_uid = std::fs::metadata(base.path()).unwrap().uid(); + EUID_OVERRIDE.with(|c| c.set(Some(real_uid.wrapping_add(1)))); + let result = ensure_key_dir(&nested); + EUID_OVERRIDE.with(|c| c.set(None)); + + let err = format!( + "{:#}", + result.expect_err("a directory under a foreign-owned ancestor must be refused") + ); + assert!( + err.contains("sits under") && err.contains("control which identity"), + "must be refused for the ancestor, got: {err}" + ); + // Refused BEFORE creation: this is the whole point, since a directory + // the node created would pass the leaf ownership check afterwards. + assert!( + !nested.exists(), + "the key directory must not have been created under a foreign ancestor" + ); + } + + /// Root-owned ancestors are trusted, or nearly every real deployment breaks. + #[cfg(unix)] + #[test] + fn ensure_key_dir_accepts_a_root_owned_ancestor() { + use std::os::unix::fs::MetadataExt; + + // /usr is root-owned and not group/other writable on any sane system; + // skip rather than assert if this box disagrees. + let probe = Path::new("/usr"); + let Ok(md) = std::fs::metadata(probe) else { + return; + }; + if md.uid() != 0 { + return; } + assert!( + foreign_ancestor_error(&probe.join("nonexistent-gitlawb-keys"), 1000).is_none(), + "a root-owned ancestor must be trusted for a non-root node" + ); } /// Both directions of the ownership refusal, without needing root. @@ -1182,6 +1374,14 @@ mod tests { ); } + /// The predicate itself, over the whole input space in both directions. + /// + /// Deliberately does not call `load_or_create_p2p_keypair` on the accepted + /// paths: that would create directories and write a real key relative to + /// whatever directory the test process happens to run in. The rejected + /// direction is covered above, where nothing is created by construction, + /// and the gate and the backstop call this same function so they cannot + /// disagree. #[test] fn names_no_usable_directory_covers_both_directions() { for path in [ From 66fb3ffc954fc094733465b6e1bd931eb5609a42 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 14 Aug 2026 13:08:37 -0500 Subject: [PATCH 3/3] test(node): make the ownership guards actually observable Re-running the mutation matrix after the ancestor check landed turned three entries from load-bearing into inconclusive. The guards had not changed; the tests had stopped being able to see them. The ancestor walk masked both leaf checks. With the euid override armed the whole path chain looks foreign, so a nested fixture tripped the ancestor error first, and that message also contains "owned by uid", so the assertions matched either way. Remove the leaf ownership check entirely and the tests stayed green. They now target the tempdir itself, whose ancestors are /tmp: root-owned and sticky, therefore trusted, so only the leaf is foreign. The uid/gid mixup was invisible because uid equals gid on an ordinary single-user machine, which makes reading the wrong field indistinguishable from reading the right one. The fixture now chgrps to a supplementary group, which needs no privilege, and degrades to the old behaviour where no such group exists rather than quietly proving less. With those two fixed and the ancestor and ordering mutations reshaped to name the assertion that actually separates the cases, all eight entries come back load-bearing. --- crates/gitlawb-node/src/p2p/mod.rs | 38 ++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/crates/gitlawb-node/src/p2p/mod.rs b/crates/gitlawb-node/src/p2p/mod.rs index f4d8ff81..058b43c6 100644 --- a/crates/gitlawb-node/src/p2p/mod.rs +++ b/crates/gitlawb-node/src/p2p/mod.rs @@ -1196,6 +1196,17 @@ mod tests { std::fs::write(&path, kp.to_protobuf_encoding().unwrap()).unwrap(); std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + // Give the fixture a group that is NOT its owning uid, where the box + // allows it. On a machine where uid == gid (this one, and most + // single-user boxes) a check that read gid instead of uid would be + // indistinguishable from the correct one, so the mutation covering that + // mixup cannot fail. chgrp to a group we already belong to needs no + // privilege. If no such group exists the test still runs and simply + // does not carry that particular distinction. + if let Some(gid) = other_group() { + let _ = std::os::unix::fs::chown(&path, None, Some(gid)); + } + let real_uid = std::fs::metadata(&path).unwrap().uid(); let other = real_uid.wrapping_add(1); @@ -1239,9 +1250,15 @@ mod tests { use std::os::unix::fs::{MetadataExt, PermissionsExt}; for mode in [0o700, 0o777] { + // The tempdir ITSELF is the key directory under test, not a child of + // it. With the euid override armed the whole path chain looks + // foreign, so a nested fixture would trip the ancestor walk first + // and this test would pass through that error instead, leaving the + // leaf ownership check unbound. /tmp is root-owned and sticky, so + // the ancestors of the tempdir are trusted and only the leaf is + // foreign. let dir = tempfile::tempdir().unwrap(); - let keys = dir.path().join("keys"); - std::fs::create_dir(&keys).unwrap(); + let keys = dir.path().to_path_buf(); std::fs::set_permissions(&keys, std::fs::Permissions::from_mode(mode)).unwrap(); let real_uid = std::fs::metadata(&keys).unwrap().uid(); @@ -1274,6 +1291,23 @@ mod tests { } } + /// A group this process belongs to that is not its effective gid, if there + /// is one. Used to build a fixture whose uid and gid differ so a uid/gid + /// mixup is observable; returns None on a box with no secondary groups, + /// where that distinction simply cannot be drawn. + #[cfg(unix)] + fn other_group() -> Option { + // SAFETY: getegid only reads the calling process's effective gid. + let egid = unsafe { libc::getegid() }; + let mut buf = [0 as libc::gid_t; 64]; + // SAFETY: writes at most buf.len() entries into buf and returns the count. + let n = unsafe { libc::getgroups(buf.len() as libc::c_int, buf.as_mut_ptr()) }; + if n <= 0 { + return None; + } + buf[..n as usize].iter().copied().find(|g| *g != egid) + } + /// A foreign-owned ancestor is refused before anything is created under it. /// /// This is the case that survived the leaf checks: the node creates the key