fix(node): refuse a p2p identity key owned by another user - #335
Open
beardthelion wants to merge 3 commits into
Open
fix(node): refuse a p2p identity key owned by another user#335beardthelion wants to merge 3 commits into
beardthelion 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.
Contributor
|
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.