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 3 commits into
Conversation
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.
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.