fix(node): refuse a p2p identity key owned by another user - #335
fix(node): refuse a p2p identity key owned by another user#335beardthelion wants to merge 14 commits into
Conversation
Add p2p_key_path (--p2p-key-path / GITLAWB_P2P_KEY, default ~/.gitlawb/p2p.key) with a resolver mirroring resolved_key_path, and load_or_create_p2p_keypair, which generates an Ed25519 keypair on first start, persists it 0600, and loads it thereafter. Mirrors the existing load_or_create_keypair idiom for the node identity PEM. A corrupt or unreadable key file is a hard error naming the path rather than a silent regeneration, so a disk problem cannot quietly rotate the node's network identity. Not yet wired into p2p::start; that follows.
p2p::start now takes the Ed25519 keypair loaded by load_or_create_p2p_keypair instead of computing one from the node DID, so a node's network identity is generated once from the OS RNG and kept on disk rather than recomputed from a public value on every start. The node DID parameter is gone from start; the call site loads the key first and continues without p2p if the key file cannot be read, matching how a swarm-start failure is already handled. The gossipsub message_id_fn is untouched and keeps its own hasher.
…e one Open the key file with create_new and the mode set at creation, then fsync, instead of writing it and narrowing the mode afterwards. The secret is never on disk under a wider mode, an interrupted start cannot leave it readable, and the exclusive open also refuses a pre-existing entry at the path and makes a concurrent start take the key that landed rather than clobber it. Refuse to load a key file whose mode grants group or other access, and name the observed mode so the operator can fix it. Report an empty key file as empty rather than surfacing a protobuf decode error that blames a missing rsa feature. Pin GITLAWB_P2P_KEY onto the mounted volume in the Docker and fly configs and document it, so the key does not depend on home-directory resolution to land on persistent storage.
Write the key to a scratch file in the same directory and hard-link it onto the final path. The bytes are durable before any name points at them, so a crash cannot leave a partial key that fails to load on the next start and takes the node off the network until someone reads the logs. A concurrent reader can no longer observe a half-written file either, since the final name appears complete or not at all. hard_link rather than rename: rename replaces its destination silently, so refusing to clobber an existing key would depend on a check followed by a separate rename, and a concurrent start can land in that gap. hard_link is atomic and refuses an occupied path, including a symlink, which it does not follow. Create the key directory 0700 and tighten it when an existing one grants group or other access. A 0600 key under a writable directory can still be replaced or unlinked. Tightening rather than refusing to start, because existing installs already have 0755 there and refusing would take p2p down on all of them through a path that only warns. Formatting on the branch is swept up here; it was already failing cargo fmt --check before this change.
House style avoids em dashes in text we write. The swarm-failure warning beside it predates this branch and is left alone.
A bare filename in GITLAWB_P2P_KEY put the key in whatever directory the
process started from, and the directory guard was skipped entirely on that
path: Path::parent returns Some("") for a bare filename, which the caller
filtered out before ever reaching ensure_key_dir. The key file was created
0600 inside a directory that kept whatever mode it already had.
Config::validate now rejects a p2p key path that names no directory, so the
node says so at boot instead of starting with a key it cannot protect. That
placement is the point: an error raised in the p2p start path is logged and
stepped over, leaving the node running without p2p and reporting healthy.
The check is lexical on the tilde-resolved path. canonicalize would fail on a
parent that does not exist yet, which is the shipped ~/.gitlawb default and
every container's first boot, and comparing against the working directory
would reject /data/p2p.key under the image's WORKDIR, an absolute directory
the operator did name.
Three sites answered the parent question differently, which is how the gap
arose: one filtered the empty case out, one already normalized it, and one
opened "" and silently skipped its fsync. They now share key_parent, and
Config::validate calls it rather than adding a fourth answer.
load_or_create_p2p_keypair also refuses a path naming no directory. That is a
backstop behind the config gate, not the gate, so a later caller that skips
validation cannot quietly restore the old behaviour.
The probe zeroes the umask so the assertion means something: under a restrictive ambient umask the bits are masked to 0600 regardless of whether the code pins the mode, and the check passes either way. Zeroing it in the shared test process is the problem. umask is process-global and cargo runs these tests on threads, so any test creating a file in that window inherits 000. Measured before this change: an unrelated concurrent test's file was created 0666. The probe now runs in a child process, where the zeroed umask cannot reach a sibling and dies with the child. The parent is an ordinary test that runs concurrently with everything else. Double-gated with #[ignore] plus an env check so a bare --ignored sweep does not zero the umask in the shared process after all. The parent asserts the child ran exactly one test and that it passed, not just that it exited 0. A libtest filter matching nothing runs zero tests and still exits 0, so without that assertion a renamed fixture would read as a green permission check while asserting nothing. Verified by pointing the filter at a name that does not exist and watching the parent fail.
The old wording said the key file is "created with owner-only permissions" without qualification, which is only true on Unix: every permission path in p2p/mod.rs is cfg(unix), so on other platforms the file inherits whatever the directory gives it and nothing is enforced. Say what is actually enforced and where. Also document what operators now have to do rather than leaving them to discover it: - GITLAWB_P2P_KEY must name a directory, since a bare filename is refused at startup. - The PeerId rotates once on the first start after upgrading, so a GITLAWB_P2P_BOOTSTRAP multiaddr pinning a peer's old id with a /p2p/<PeerId> suffix needs updating or dropping. Suffix-less addresses and the HTTP seed list are unaffected. - If the node reports tightening a loose key directory, the key that was in it should be treated as possibly exposed and deleted so a fresh one is generated.
Two reviewers found the same hole independently: the check rejected a path naming no directory, but a relative parent that walks back out through `..` named one and still landed in the working directory. `a/../p2p.key` and `./keys/../p2p.key` resolve to the cwd itself and `../p2p.key` resolves above it, so all three put the key exactly where the check exists to keep it out of, and had ensure_key_dir chmod that directory to 0700 on the way. Verified by running the paths through the predicate and printing where each parent lands. The rule is now that a relative key path must name a directory and must not walk back out: at least one Normal component, no ParentDir. `..` inside an absolute path stays accepted, since it cannot depend on where the process started. The predicate moves into names_no_usable_directory next to key_parent, and the config gate and the load_or_create_p2p_keypair backstop both call it, so they cannot drift apart. Also fixes two smaller gaps found in the same pass: - The permission fixture could report "1 passed" while asserting nothing. Its env gate returns early, and an early return is a passing test, so a renamed variable would look green. It now prints a sentinel after its assertions and the parent requires it. Confirmed by pointing the child at a different variable and watching the parent fail. - A GITLAWB_P2P_KEY starting with `~/` is refused when no home directory resolves, instead of creating a literal `~` directory relative to wherever the node happened to start. The backstop had no test, so it has one now, along with a both-directions test for the predicate. That test cleans up after itself: with the guard removed it really does write a key next to the source, which broke a later run once.
The previous commit closed this for relative paths and exempted absolute ones, reasoning that an absolute path cannot depend on the working directory. That is true and it is not the hazard. `key_parent` hands `ensure_key_dir` the lexical parent, so `/data/keys/../p2p.key` chmods `/data` rather than the `keys` directory the path appears to name, and `/data/../p2p.key` run as root would try to tighten `/` to 0700. The exemption also had a test asserting the first of those was fine, so the gap was written down as intended behaviour. `..` is now rejected wherever it appears. An absolute path's root counts as naming a directory, so `/p2p.key` still validates and `/data/keys/p2p.key` is unaffected. Found by a second-model review pass after the in-process reviewers had cleared the relative half.
Two buffers held the private key in its protobuf form and dropped without scrubbing: the encoding produced when a new identity is generated, and the file contents read back on every subsequent start. Both are now Zeroizing, matching what gitlawb-core already does for its own key material. Scope worth being honest about: this scrubs our copies of the serialized form, not the libp2p Keypair itself, which owns the secret for the process lifetime and exposes no way to zeroize it. The gain is that the encoded bytes do not outlive the write and the read. zeroize was already in the tree through gitlawb-core, so this promotes it to a direct dependency of gitlawb-node and adds no packages; the lockfile change is the one line recording that.
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.
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.
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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
beardthelion
added
crate:node
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Merge readiness
- [P1] Rebase onto current
mainbefore merge
The PR head66fb3ffc954fc094733465b6e1bd931eb5609a42is not contained in livemain(e4c7458fb3da87c8bca81e43e2db18a3e9a5a60e); its merge base is96d8123e0f84e85a7b0234ad84f5e603a5a303aa. The current target does not alter this module, but the stale head still needs a rebase and a rerun of the resolved diff's checks before merge.
Findings
-
[P1] Reject writable group ancestors as well
crates/gitlawb-node/src/p2p/mod.rs:343
The new check rejects only other-writable ancestors, so a0770service directory is accepted even when another account belongs to its group. That account can renamebase/keyswhile the node is stopped, retain the directory containing an earlier legitimate key, and put a different node-createdkeysdirectory in its place before the next start. Each replacement leaf is still owned by the node and is0700; itsp2p.keyis likewise node-owned and0600. Consequently, the leaf ownership and mode checks all pass, but the group member can select or roll back the PeerId on every restart.Please address the root cause—untrusted write authority over any ancestor—not just the leaf mode. Group write is an identity-control capability in this path just as other-write is. The conservative fix is to reject both group- and other-writable ancestors (with the existing sticky-directory exception only where its semantics are deliberately supported). If shared group directories are a required deployment model, introduce an explicit, documented trusted-group policy and test both its allowed and denied members; accepting every group-writable ancestor cannot establish the claimed trust boundary.
-
[P1] Bind the ancestor validation and key operations to trusted directory handles
crates/gitlawb-node/src/p2p/mod.rs:317
The ancestor walk skips missing components and validates only pathnames, thenDirBuilder, metadata, scratch creation, and reads reopen those pathnames. For a configured path such as/tmp/created-later/keys/p2p.key,/tmppasses because it is sticky andcreated-lateris skipped because it does not yet exist. An attacker can createcreated-laterafter the walk but before recursive creation, so the node creates a correctly ownedkeysleaf under the attacker's parent. The attacker can then swap that leaf between restarts and choose the identity. Directory symlinks have the same root problem:metadatafollows them, whileO_NOFOLLOWprotects only the finalp2p.keycomponent; an attacker-owned intermediate symlink can be retargeted after validation.Please fix this as a pathname-resolution problem, rather than adding another check after
DirBuilder::create. Each check on a pathname creates another check-then-use window. Resolve and create the directory chain through trusted directory file descriptors without following untrusted components (for example,openat/mkdirat-style operations with no-follow semantics), verify ownership and permissions on those descriptors, and perform scratch-file creation, publication, and existing-key opens relative to the verified directory descriptor. Add race-oriented coverage for a missing intermediate component created after validation and for an intermediate directory symlink that changes target; both tests should prove that no attacker-selected key can be loaded or created.
3a29648 to
b049ee5
Compare
|
Closing this: everything it adds is already on #324, which has since carried the same work through four more review rounds. Checked rather than assumed. All four of this branch's own commits have their distinctive markers present in #324's current head (
I went through The one place the two differ, #324 is stricter. This branch deliberately allowed group-write on ancestors ( The 14 commits this branch shows ahead of #324 are #324's own older history under different SHAs, from before it was rebased. A trial rebase onto the current head conflicts on the first of them, which adds the persistent-key feature #324 already has. The branch is also 121 commits behind main. One thing worth not losing: |
2 participants
Stacked on #324, deliberately. The base is
fix/p2p-keypair-derivation, notmain. Everything this touches (read_p2p_keypair,ensure_key_dir,load_or_create_p2p_keypair) is introduced by #324 and does not exist on main, so it cannot branch independently.Deployable on its own. Merging #324 without this leaves the key exactly as #324 ships it, persisted with owner-only modes and no ownership check, which is already better than deriving it from public data. This adds a check on top rather than restoring something #324 removed.
The defect
#324 pins the key file to
0600and its directory to0700, and verifies both on load. Neither check asks who owns them. A0600file owned by a different user passes every check on the read path, and the node adopts it, so that user chooses the node's libp2p identity. Mode answers who may read the file; it says nothing about who may replace it.The fix, and the part worth reviewing
The leaf check is the easy half: refuse when the key file or its directory is owned by someone other than the running user.
The ancestor walk is the half I would look at. Checking only the leaf is not enough, and the way it fails looks safe. A user who owns
/home/them/basecan point the node at/home/them/base/keys/p2p.key. On first start the node createskeysand the key itself, so both are node-owned,0700and0600, and pass every leaf check. That owner never needs to own either one. They can renamekeysaside, let the node generate a fresh identity in a newkeys, and move the old directory back before a later restart. Both directories pass at every point, 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, every ancestor must be owned by this user or by root, and must not be world-writable unless it is sticky.
Root counts as trusted on purpose. Requiring every ancestor to be node-owned would refuse
/data/keysunder a root-owned/data, and/itself, which is most real deployments. Root can already replace the binary, so treating it as an attacker here buys nothing.The read also became a single
O_NOFOLLOWdescriptor feeding the ownership check, the mode check, and the read. Before this, the mode came from a separatemetadatacall and the bytes from a laterfs::read, which is a stat-then-read window and follows a symlink at the final component.Testing it
A test cannot chown to another user without root, so both directions run through a
cfg(test)override of the effective uid, and the override is thread-local so an armed test cannot disturb the ones beside it. The seam itself carries a mutation, because a seam that stopped consultinggeteuidwould leave every other ownership test passing against nothing.Eight mutations, each reverting one guard and requiring the failure to match a named message, so a red is attributable to the property rather than to removing the seam the test injects at. Two of them exist because earlier versions were misattributed: gating the ordering check on the directory mode made the
0777case skip the check entirely, so the observed failure was "no refusal happened" rather than "the repair ran first", and the uid-versus-gid mutation was invisible on a machine where the two are equal until the test named the expected owner explicitly.Known limitation, and why it is not in here
The refusal is still non-fatal. It reaches
main.rs, which logs it and continues without p2p, so a foreign-owned key takes the node off the network for the run rather than stopping it. Moving the check toConfig::validate, where it would stop the process, is its own change. It needs a look at what actually owns the data volume on a deployed node first: a boot-fatal version against an unexpected volume ownership refuses to start, and that is a worse outcome than the one it prevents.