🏯 fix: Guard Worker Credentials From Local Accounts - #112
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21c893a27a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!isRecord(error) || error.code !== 'EEXIST') throw error; | ||
| /* Already present: judge what is there instead of the placeholder. */ | ||
| await assertOwnedByWorker(path); | ||
| await assertOwnerOnlyPath(path); |
There was a problem hiding this comment.
When re-pairing over an owner-only identity whose parent is private but not writable by the worker, such as an existing file in a 0500 directory, this EEXIST branch passes because it checks only ownership, mode, and shared writability. pair() then redeems the one-time code, after which saveBridgeIdentity() fails to create its sibling temporary file with EACCES, leaving the remote pairing orphaned—the exact failure the preflight is intended to prevent. Verify that the temporary-file-and-rename workflow is usable before allowing the pairing request.
Useful? React with 👍 / 👎.
| if (!isRecord(error) || error.code !== 'EEXIST') throw error; | ||
| /* Already present: judge what is there instead of the placeholder. */ | ||
| await assertOwnedByWorker(path); | ||
| await assertOwnerOnlyPath(path); |
There was a problem hiding this comment.
When two pair invocations target the same previously absent identity path, the first creates the placeholder, but the second handles its EEXIST as though it were a pre-existing identity and proceeds with pairing. If the second saves its credential and the first subsequently fails, the first reservation's release() removes the second command's completed identity, consuming its code and leaving no local credential. Distinguish an active placeholder from an existing identity or make release conditional on the path still naming the reserved file.
Useful? React with 👍 / 👎.
|
Both fixed in Treat the placeholder as an active reservation — this was the serious one, and it was mine: Now the placeholder's inode is recorded at creation, and Probe parent writability for an existing identity — right, and it is the same shape as the Verified end to end: first pairing, re-pairing over an existing identity (the |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d54ec234a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ((mode & 0o1000) !== 0 && (metadata.uid === uid || metadata.uid === 0)) { | ||
| return; |
There was a problem hiding this comment.
When an identity path is a symlink owned by another account inside a sticky shared directory such as /tmp (on macOS or Linux with protected-symlink enforcement disabled), this return treats the entry as protected merely because the directory is sticky. Sticky semantics still allow the symlink's owner to remove and recreate that entry, while readGuardedFile checks only the followed target's owner and mode; consequently that account can redirect loadBridgeIdentity to another worker-owned identity or repeatedly break startup. Check the entry's ownership when relying on the sticky bit, especially for symlinks.
Useful? React with 👍 / 👎.
| await chmod(locked, 0o500); | ||
| await assert.rejects( | ||
| assertIdentityPathIsPrivate(identityPath), | ||
| /could not be published there/, |
There was a problem hiding this comment.
When this suite runs as root or with CAP_DAC_OVERRIDE, as is common in containerized test jobs, changing the directory to 0500 does not prevent creation of the sibling probe. assertIdentityPathIsPrivate therefore resolves successfully and this assertion fails with Missing expected rejection; running node --test dist/storage.test.js as uid 0 reproduces it. Run this case as an unprivileged user or skip it when the process can bypass discretionary access controls.
Useful? React with 👍 / 👎.
| /* Already present: judge what is there instead of the placeholder, and | ||
| * prove the publish itself is possible - an unwritable parent would | ||
| * otherwise surface as EACCES only after the code was spent. */ | ||
| await assertOwnedByWorker(path); |
There was a problem hiding this comment.
When the identity lives in a shared sticky directory owned by the worker account and another account has pre-created the destination, POSIX sticky-directory rules allow the directory owner to replace that entry safely. assertIdentityDestinationIsReplaceable explicitly recognizes this case, but this unconditional target-ownership check rejects it immediately afterward; as a result, another local account can block pairing merely by reserving the predictable filename even though the eventual rename would succeed and the replacement would be protected. Skip this ownership requirement when the caller owns the sticky parent while retaining it for identity loads.
Useful? React with 👍 / 👎.
Everything here defends a threat the merged owner-only fix does not: another account on the same host. It is separate deliberately, because BYOM's stated model is a worker on the user's own machine or VM with the sandboxed command as the adversary, and these checks buy that defence by trading deployment flexibility for it. Ownership. Mode bits do not establish trust: a 0600 file owned by another account is unreadable by others yet fully rewritable by its owner, who then controls the credential the worker loads - or, for a quarantine marker, can delete it and let mutations resume. The containing directory is judged the same way, since an owner lacking write bits today can grant them tomorrow. Root counts as the trust root. `--default-workspace` is application-owned by contract, so a pre-existing one under another account is refused too. Containers. A 0600 file in a directory others can write can be unlinked and replaced. Publishing goes through `rename`, which replaces the named entry, so the write path judges the entry's directory; reading follows the link, so both ends are judged. The sticky bit counts as protection, keeping /tmp-style parents usable. Only the immediate container is inspected. Pairing preflight. `pair` redeemed the one-time code before the destination was known usable, so an unusable path cost the code and left an orphaned remote pairing. The destination is now validated - rejecting a directory, a foreign-owned file under a sticky bit that `rename` could not replace, and a parent that denies the sibling temporary file the publish needs - and then claimed, so another account cannot take the name while the pairing request is in flight. The claim records its inode and is released only if the file is still that inode and still empty, so a concurrent pairing that published a real identity over the name is never destroyed by another invocation's unwind. Known gaps, left explicit rather than half-done: only the immediate container is checked, so a writable ancestor could still rename a private directory out from under the worker; a bind-mounted destination still fails at `rename` with EBUSY because every way to detect it either false-positives on btrfs subvolumes or races; and macOS extended ACLs and Windows ACLs are both outside what a mode check can see, which needs real ACL inspection on a host that can validate it.
7d54ec2 to
a2b6c3c
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
danny-avila
changed the title
2 participants
Follow-up to #107, now merged. That PR fixed a reproducible defect —
chmodsilently no-ops on WSL2 DrvFs, so the worker's Ed25519 private key was written world-readable while pairing reported success. This one is different in kind, and needs its own decision.The question this PR is really asking
Is another local account on the same host in scope for BYOM?
Everything here defends that threat. BYOM's stated model is a worker on the user's own machine or VM, with the trusted worker outside the sandbox and the sandboxed command as the adversary. Under that model none of this is needed. Under a shared or managed host, all of it is.
It is separate because these checks buy that defence by trading deployment flexibility, and that trade should be made deliberately rather than accumulated under review pressure. Two of them can refuse setups that work today:
If the answer to the question above is "no", most of this should be dropped rather than merged. I would rather ask than assume.
What it adds
Ownership. Mode bits do not establish trust: a
0600file owned by another account is unreadable by others yet fully rewritable by its owner, who then controls the credential the worker loads — or, for a quarantine marker, can delete it and let mutations resume. The containing directory is judged the same way, since an owner lacking write bits today can grant them tomorrow. Root counts as the trust root.--default-workspaceis application-owned by contract, so a pre-existing one under another account is refused too.Containers. A
0600file in a directory others can write can be unlinked and replaced. Publishing goes throughrename, which replaces the named entry, so the write path judges the entry's directory; reading follows the link, so both ends are judged. The sticky bit counts as protection, keeping/tmp-style parents usable.Pairing preflight.
pairredeemed the one-time code before the destination was known usable, so an unusable path cost the code and left an orphaned remote pairing — reproduced with--identity <existing dir>, which surfaced a rawEISDIRafter the code was spent. The destination is now validated and then claimed, so another account cannot take the name while the request is in flight. The claim records its inode and is released only if the file is still that inode and still empty, so a concurrent pairing that published a real identity over the name is never destroyed by another invocation's unwind.Verification
pair --identity <existing dir>EISDIR, code spentpairinto a0500directoryEACCESat save, code spentpair, first one unwindspairwith an invalid codepairto ext4 / default~/.config, and re-pairing06000600(unchanged)Measured against real paths before committing to the rule:
~is0750and~/.config0751here, both accepted;/tmpat1777is accepted via the sticky bit. A naive& 0o077ancestor check would have rejected the default identity path, which is why the rule is write-bits-plus-sticky rather than mode-wide.41/41 focused tests (
storage,cli,workspace-cli— the only dependents of the changed functions).tsc --noEmitclean.Known gaps, left explicit rather than half-done
renamewithEBUSY. Every detection I found either false-positives on btrfs subvolumes (st_dev) or races (/proc/mounts), and a test-rename would clobber the file it protects.The sticky-plus-foreign-owner reject path cannot be exercised without root, so the test there covers the regression instead: an owned file in a sticky directory stays replaceable.