fix(security): 0600 and 0700 were doing nothing at all on Windows - #96
xizhuomengcontin wants to merge 13 commits into
Conversation
`chmod(path, 0o600)` on Windows sets the read-only attribute and discards the
mode. `stat` then answers 0o666 whatever was asked for. A test had already
noticed and skipped itself over it, on the reading that this was an assertion
"the platform cannot provide".
It cannot provide it *as mode bits*. The file still has permissions there, and
what it gets is whatever the workspace hands down. Recorded through a real
`orca record --tls-intercept`, before this change:
ca.key :: Administrators:(I)(F) | SYSTEM:(I)(F)
Authenticated Users:(I)(M) | Users:(I)(RX)
Every local account could read the run CA's private key, and every
authenticated one could rewrite it. That key signs the certificates the agent
has been told to trust for the life of the run, so reading it is enough to
impersonate every intercepted host to that agent, and writing it is enough to
substitute a CA of one's own. `config.json` held a gateway API key under the
same non-guarantee, and SECURITY.md stated the 0600/0700 as fact.
`restrictToOwner(path, mode)` keeps the promise in whatever currency the
filesystem has: `chmod` on POSIX, and on Windows an ACL with inheritance
dropped and only the owner, SYSTEM and Administrators left — the rule OpenSSH
for Windows enforces on its own keys, and the closest the platform comes to
0600, under which root can read the file too.
Three call sites, and the middle one carries most of the weight:
- `ensureRunsDir` — restricting `.orca/runs` once propagates to every run
directory, blob, `.incoming` staging area and `tls/` written beneath it.
One call, not one per file for a trace that may hold tens of thousands.
Applied only when this call created the directory, which is what passing
`mode` to mkdir already meant.
- `RunCa.create` — the CA files explicitly as well, since they are the ones
that matter most and a store predating this still needs them.
- `writeConfig` — the API key, which lives outside `.orca` entirely.
By SID, never by name: `BUILTIN\Administrators` is localized, and a grant
naming a principal that does not resolve matches nothing — which, after
inheritance has been dropped, would leave no usable ACE at all. `icacls.exe`
and `whoami.exe` are invoked by absolute path, because resolving them through
PATH while securing a private key is how the key reaches the reader this is
shutting out. (The test found that one: Git for Windows ships a POSIX `whoami`
earlier on PATH.)
Verified through a real `orca record`, the agent reading the ACL of the very
key it had been told to trust, from inside the run: the CA directory and all
three files grant the owner, SYSTEM and Administrators and nobody else. A
second recording confirmed `events.jsonl`, `manifest.json`, `redactions.json`,
`fs/`, `py/` and `shims/` all inherit it without being touched individually.
POSIX behaviour is unchanged — `restrictToOwner` is `chmod` there — except that
`ensureRunsDir` now sets the mode it always documented rather than leaving it
to umask.
Windows: 4 failures before, 3 after. The config test that had skipped itself
now runs there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![orcacode-review[bot]](https://avatars.githubusercontent.com/in/4254738?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 7 issues in this PR: 🟠 7 P1.
Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.
packages/cli/src/commands/quickstart.ts (line 306): 🟠 P1 orca quickstart must create the store through ensureRunsDir, or its store never gets the Windows ACL
The commit makes ensureRunsDir the single place that applies the Windows ACL — and it only applies it when that call is the one that created .orca/runs (paths.ts:48). orca quickstart is the one non-test caller that creates the store by hand: await mkdir(join(target, '.orca', 'runs'), { recursive: true }), then copies a trace into it. It was not switched over.\n\nConsequence, Windows only: target/.orca/runs keeps the ACL inherited from the workspace (BUILTIN\\Users read, Authenticated Users modify — the ACL private-files.ts documents). Every later orca record in that directory finds the store already present, so ensureRunsDir returns undefined, skips restrictToOwner, and the new run directory, trace, blobs, snapshot store and mcp-frames.jsonl inherit the broad ACL too. Nothing else ever repairs it (the only icacls call in the tree is the one in private-files.ts). So the store created by the README's headline one-liner is exactly the store SECURITY.md now says is owner-only, and orca record inside it leaves the traces readable by every account on the machine — the vulnerability this commit exists to close.\n\nThe same path also misses the second half of ensureRunsDir: no .orca/.gitignore, so the store shows up in git status as untracked in the demo directory, contradicting the layout README documents (orca/.gitignore # just *``) and defeating the accident ensureRunsDir's doc comment and `sync.ts:1123` both describe.\n\nFix: create the store through core rather than by hand —\n\nts\nimport { ensureRunsDir } from '@orcareplay/core';\n...\nconst runsDir = await ensureRunsDir(target);\nconst runDir = join(runsDir, runId);\n\n\n`ensureRunsDir` is `mkdir(dir, { recursive: true })`, so it also creates `target` on the way as the current line does, and it writes the `.gitignore` too. `orca pull` was fixed the same way for the same two reasons (`sync.ts:1123-1137`); quickstart is the remaining caller.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 419 calls · 31.5M tokens · 99% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
| // call, rather than one per file for a trace that may hold tens of thousands. | ||
| if ((await mkdir(dir, { recursive: true, mode: 0o700 })) !== undefined) { | ||
| await restrictToOwner(dir, 0o700); | ||
| } |
There was a problem hiding this comment.
🟠 P1 Existing .orca/runs is never re-restricted, so upgrading Windows installs keep the broad ACL
The ACL is applied only when this call created .orca/runs. mkdir(dir, { recursive: true }) returns undefined for a directory that already exists, so a store that is already on disk is never restricted — and there is no migration anywhere: the only icacls invocation in the tree is inside private-files.ts.\n\nInput/state → wrong behaviour: a Windows machine that has recorded with any earlier orca has cwd/.orca/runs with the inherited broad ACL — that is precisely the state SECURITY.md now describes as fixed (".orca/runs is created granting only its owner, SYSTEM and Administrators, and everything written beneath it inherits that"). After upgrading, the first orca record in that workspace calls ensureRunsDir, finds the directory present, restricts nothing, and TraceWriter.create makes a new run directory that inherits BUILTIN\\Users:(I)(RX) / Authenticated Users:(I)(M) from the existing store root. Every trace written there from then on — model requests, shell output, workspace snapshots, unredacted mcp-frames.jsonl — stays readable (and rewritable) by every account on the machine, and nothing ever reports it: orca doctor has no store-permissions check.\n\nThe created-only rule is deliberate on POSIX ("a directory the user has deliberately opened up is theirs to have opened up") and should stay there. It does not translate: on Windows an inherited ACL from the workspace is not a statement of intent by the user, and the pre-existing store is the exact population the fix targets.\n\nFix: keep the POSIX behaviour, and on Windows restrict the store whether or not this call created it —\n\nts\nconst created = (await mkdir(dir, { recursive: true, mode: 0o700 })) !== undefined;\nif (created || process.platform === 'win32') await restrictToOwner(dir, 0o700);\n\n\nIf that is judged too blunt for a store a Windows user has deliberately widened, the alternative is an explicit repair the operator can see: a checkStorePermissions in orca doctor (untouched by this commit) that flags a .orca/runs whose DACL still contains inherited entries, plus a one-time fix-up on the next orca record. What must not happen is what happens now — the store stays broad forever and the new SECURITY.md sentence reads as though it does not.
There was a problem hiding this comment.
Confirmed and fixed in a2f149a. Reproduced on Windows before taking it as read — a hand-made .orca/runs, then a real orca record:
events.jsonl :: Administrators:(I)(F) | SYSTEM:(I)(F)
Authenticated Users:(I)(M) | Users:(I)(RX)
That is the state the SECURITY.md sentence in this very commit called fixed. Your reading of why the gate does not translate is right and is now the comment in the code: on POSIX the directory mode is not load-bearing (every file gets its own 0600 from the writer) and an opened-up directory is a choice; on Windows the ACL is the only thing there is and an inherited one is not a choice.
Taken as written:
const created = (await mkdir(dir, { recursive: true, mode: 0o700 })) !== undefined;
if (created || process.platform === 'win32') await restrictToOwner(dir, 0o700);Your parenthetical about existing children is in SECURITY.md now rather than left implied — this covers what is written from now on; icacls on a parent does not re-propagate, and rewriting every file in a store already on disk is not something record should do behind someone's back.
New test, red against the old gate: ensureRunsDir restricts a directory it did not create.
| // call, rather than one per file for a trace that may hold tens of thousands. | ||
| if ((await mkdir(dir, { recursive: true, mode: 0o700 })) !== undefined) { | ||
| await restrictToOwner(dir, 0o700); | ||
| } |
There was a problem hiding this comment.
🟠 P1 Restrict .orca/runs on an already-existing store too, not only when this call created it
The Windows guarantee this commit adds now depends entirely on one inherited ACL set on .orca/runs at creation: TraceWriter.create (writer.ts:113), BlobStore.put (blobs.ts:55), sync's .incoming staging/.lock (sync.ts:830,848,852) and scrub's rewrites (scrub.ts:355) all pass mode: and nothing else, and mode is discarded on Windows. The other two call sites apply restrictToOwner to a path that may already exist — writeConfig does it on the config directory every time, with the comment "an existing directory keeps whatever mode it had" (config.ts:107-111), and RunCa.create does it on <run>/tls unconditionally — but ensureRunsDir applies it only when mkdir created the directory (mkdir(..., {recursive:true}) returns undefined when the directory already exists; verified on node v22.23.2).
Consequence on Windows for any store that already exists — every upgraded install, since .orca/runs is created by the previous version and never deleted — restrictToOwner is never called: the incoming run directory inherits the workspace ACL that this commit's own SECURITY.md text calls "whatever the workspace handed down — typically readable by every account on the machine", and events.jsonl, manifest.json, redactions.json, blobs/, fs/ and mcp-frames.jsonl inherit it in turn. So the source, shell output and workspace snapshots the commit exists to protect stay readable by every local account and writable by every authenticated one. The commit message shows the author knew: it restricts the CA files explicitly because "a store predating this still needs them", and leaves the trace files themselves to an inheritance that a pre-existing store does not have. orca quickstart makes the same hole by a different route (quickstart.ts:304 creates .orca/runs with a plain mkdir, so ensureRunsDir also sees it as pre-existing afterwards). SECURITY.md's new sentence (".orca/runs is created granting only its owner, SYSTEM and Administrators, and everything written beneath it inherits that") reads as fact and is not true on either path.
The guard is right for POSIX (where an existing 0755 directory may be the user's deliberate choice, and each file still gets its own 0600 from the writer) and wrong for Windows, where the pre-existing ACL is inherited junk rather than a choice. Concrete fix — keep POSIX create-only semantics and make the ACL unconditional on Windows:
const created = (await mkdir(dir, { recursive: true, mode: 0o700 })) !== undefined;
if (created || process.platform === 'win32') await restrictToOwner(dir, 0o700);
(That protects runs recorded from now on; runs already on disk keep their old ACEs, since icacls on the parent does not re-propagate to existing children — that would need /T, or a one-shot icacls … /reset /T migration.)
There was a problem hiding this comment.
Same as #4023659902 — fixed in a2f149a with the two-line form you gave. Reproduction and the note on existing children are there.
| // call, rather than one per file for a trace that may hold tens of thousands. | ||
| if ((await mkdir(dir, { recursive: true, mode: 0o700 })) !== undefined) { | ||
| await restrictToOwner(dir, 0o700); | ||
| } |
There was a problem hiding this comment.
🟠 P1 Apply the owner-only ACL to .orca/runs unconditionally, not only when this call created it
This is the one call that carries the whole Windows fix (the commit message and SECURITY.md both say so: mode is discarded on Windows, so restricting .orca/runs is what makes every run dir, blob, .incoming staging area and tls/ beneath it owner-only). It is now gated on mkdir having created the directory, and nothing in the repo ever re-applies it: mkdir returns undefined for as long as the directory exists, so once the restriction is skipped it is skipped forever (I checked: restrictToOwner and the ACL work appear only in private-files.ts, paths.ts, config.ts and ca.ts; orca doctor and orca gc never repair a store).
Paths where the store root keeps the workspace's inherited ACL (Authenticated Users:(I)(M), Users:(I)(RX)) — i.e. readable by every local account and writable by every authenticated one, exactly the exposure this commit exists to remove:
orca quickstart:packages/cli/src/commands/quickstart.ts:304creates<target>/.orca/runswith a baremkdir(..., { recursive: true })and copies the trace in, so the root already exists by the time anything callsensureRunsDir.quickstartthen runsreplayCommandin that directory (which callsensureRunsDir→ skipped), and the comment inadoptTracesays the obvious next thing to type there is the command from the top of the README —orca record claude. That recording, its blobs, its workspace snapshot, its env allowlist andmcp-frames.jsonlall inherit the loose ACL, under a root this code has now decided is not its to restrict.- A store created by any earlier version on Windows: the fix is a no-op on it — and the commit message itself concedes such stores exist ("a store predating this still needs them"), which is why the CA files are restricted explicitly. The trace files got no such treatment.
- A create race or a partial failure: two processes call
ensureRunsDirat once; the loser seesundefinedand skips, and if the winner dies (Ctrl-C, SIGKILL, OOM) or itsrestrictToOwnerthrows between themkdirand theicaclscall — first use even spawnswhoami.exebeforeicacls.exe, so the window is tens of milliseconds — the directory survives with the workspace ACL and step two is never retried. The throw also propagates out ofensureRunsDirand fails the wholeorca record, leaving that half-done state behind. - A workspace whose
.orca/runsthe user deliberately opened up is indistinguishable from 2 and 3, which is the stated reason for the gate — but the code cannot tell them apart, so it protects the one case it cannot verify at the price of the three it can.
On POSIX the gate is harmless (mkdir's mode is applied to every directory it creates), so this is a Windows-only hole — and Windows is the platform the commit is for. Fix: drop the condition and always call restrictToOwner(dir, 0o700) (it is idempotent, and RunCa.create already re-applies it to a directory that already exists for exactly this reason), or at minimum re-apply it when the existing ACL is not already owner-only; and create quickstart's store through ensureRunsDir (packages/cli/src/commands/quickstart.ts:304) rather than a bare recursive mkdir.
There was a problem hiding this comment.
Same as #4023659902 — fixed in a2f149a. Point 3 (the create race / partial failure window) is covered by the same change, since the ACL is no longer conditional on this call having created the directory.
| * hand that key to exactly the reader the call exists to shut out. | ||
| */ | ||
| function system32(exe: string): string { | ||
| return join(process.env['SystemRoot'] ?? 'C:\Windows', 'System32', exe); |
There was a problem hiding this comment.
🟠 P1 Fix the C:\Windows fallback: '\W' is an escape, so it becomes the drive-relative C:Windows
'C:\Windows' in a JS/TS string literal is C:Windows (\W is not an escape, so the backslash is dropped). Verified: node -e "console.log('C:\Windows')" prints C:Windows, and path.win32.join('C:Windows','System32','icacls.exe') is C:Windows\System32\icacls.exe.
A path that begins with a drive letter and no separator is drive-relative: CreateProcess resolves it against the current directory on that drive. orca's cwd is the workspace being recorded (or the quickstart directory), so with SystemRoot absent this helper names <cwd>\Windows\System32\icacls.exe — a path inside the tree orca is recording, which a cloned/malicious repository can contain (Windows/System32/icacls.exe is a perfectly storable git path).
Consequence when SystemRoot is not in the environment (a service/scheduled-task/minimal-env launch, or a harness that spawns orca with a curated env): restrictToOwner — the function whose whole job is to lock down tls/ca.key, ca.crt and the gateway API key in config.json — spawns that binary with the user's token. It only checks the exit status, so a stub that exits 0 leaves the CA private key and the API key exactly as exposed as before while orca reports success; with nothing planted the same call fails ENOENT and ensureRunsDir / RunCa.create / writeConfig throw, so the recording or setup aborts. This is precisely the hijack the comment above the function says it exists to prevent ("a lookup that landed on someone else's icacls.exe — in the workspace being recorded, say").
Two supporting facts: packages/adapters/test/adapters.test.ts:20 already spells this fallback correctly, process.env.SystemRoot ?? 'C:\\Windows', which confirms the intent; and the same broken literal was copied into the three new test helpers (packages/cli/test/config.test.ts:29, packages/proxy/test/ca.test.ts:23, packages/core/test/private-files.test.ts:14), where it hides the same bug from those tests.
Fix: return join(process.env['SystemRoot'] ?? 'C:\\Windows', 'System32', exe); — or, better, refuse rather than guess, since a wrong path here is worse than an error:
const root = process.env['SystemRoot'];
if (root === undefined || root === '') throw new Error('SystemRoot is not set; cannot locate ' + exe);
return join(root, 'System32', exe);
and correct the three test copies to match.
There was a problem hiding this comment.
Confirmed and fixed in a2f149a. Measured rather than reasoned about:
値 : "C:Windows"
length : 9 (10 if the backslash had survived)
join : "C:Windows\System32\icacls.exe"
isAbsolute: false
Drive-relative, resolved against the cwd on C: — which during a recording is the workspace being recorded. Exactly the hijack the comment above the function claims absolute paths prevent.
Rather than spelling it correctly, the fallback is gone. A correct C:\Windows is still a guess, and wrong on a machine whose Windows is not on C:. SystemRoot is set by the OS in every normal environment; absent, refusing is the only answer that cannot be silently incorrect. The three test helpers are corrected too.
New test, verified to fail against the old code: restrictToOwner rejects with /SystemRoot is not set/ rather than resolving a workspace-relative path.
| */ | ||
| function system32(exe: string): string { | ||
| return join(process.env['SystemRoot'] ?? 'C:\Windows', 'System32', exe); | ||
| } |
There was a problem hiding this comment.
🟠 P1 Escape the Windows fallback path so icacls/whoami can never be resolved from the cwd
'C:\Windows' is not C:\Windows. \W is not a recognised escape sequence, so the string literal's value is C:Windows — verified in Node: JSON.stringify('C:\Windows') prints "C:Windows". system32() therefore returns the drive-relative C:Windows\System32\icacls.exe / C:Windows\System32\whoami.exe, which CreateProcess resolves against the process's current directory on drive C: (the workspace orca was pointed at) rather than against the system directory. This is exactly the failure the function's own doc comment says the absolute path exists to prevent: "a lookup that landed on someone else's icacls.exe — in the workspace being recorded, say — would hand that key to exactly the reader the call exists to shut out."
It only matters when process.env.SystemRoot is absent (a scrubbed/minimal environment: env -i, a service or CI runner, a launcher that builds a fresh env), and then every caller goes wrong:
whoami.exealso misses, soreadSid()rejects andrestrictToOwnerthrows. InwriteConfigthe directory call's failure is swallowed (packages/cli/src/config.ts:111), the API key is then written intoconfig.jsonwith the profile's inherited ACL, and only the file-level call throws — soorca setupexits with a spawn error while the gateway credential is already on disk readable by the accounts the change was meant to shut out.orca record --tls-interceptdies beforeRunCa.createwrites anything.- If the cwd contains
Windows\System32\icacls.exe(andwhoami.exe), that binary runs instead of the system one. A decoy that prints a SID and exits 0 makesrestrictToOwnerreport success with no ACL applied at all, silently — the CA private key, the trace store root andconfig.jsonkeep the machine-wide inherited ACL while every caller believes the promise was kept.
Same broken string in the three new test helpers (packages/core/test/private-files.test.ts:14, packages/cli/test/config.test.ts:29, packages/proxy/test/ca.test.ts:23), so the Windows tests would exercise the fallback with it. packages/adapters/test/adapters.test.ts:20 has the same expression escaped correctly, which is the form to copy.
Fix (one line): return join(process.env['SystemRoot'] ?? 'C:\\Windows', 'System32', exe); — or String.raw — and preferably stop guessing: this value is only ever read on win32, where an absent SystemRoot means the environment was not inherited normally, so throw a clear error rather than resolving a path that can name a file the workspace controls. Escaping it in the three test helpers is part of the fix.
There was a problem hiding this comment.
Same finding as #4023659920 — fixed in a2f149a, no fallback at all now rather than a corrected one. Detail there.
| // run directory, blob, `.incoming` staging area and `tls/` written under it afterwards — one | ||
| // call, rather than one per file for a trace that may hold tens of thousands. | ||
| if ((await mkdir(dir, { recursive: true, mode: 0o700 })) !== undefined) { | ||
| await restrictToOwner(dir, 0o700); |
There was a problem hiding this comment.
🟠 P1 Don't assume ensureRunsDir is the only creator of .orca/runs — quickstart creates it without the ACL and the guard then skips it forever
mkdir(dir, {recursive:true}) returns undefined whenever the directory already exists, so the new condition means "apply the ACL only if this call is the one that created the store". That is an assumption about every other writer of .orca/runs, and one caller violates it: orca quickstart creates the store itself.
packages/cli/src/commands/quickstart.ts:304:
await mkdir(join(target, '.orca', 'runs'), { recursive: true });
— no mode, no restrictToOwner — and then copies the shipped recording into join(target, '.orca', 'runs', runId) (lines 305-307). quickstart then replays that run, and the replay writes its own trace through openReplayTrace → ensureRunsDir(ctx.cwd) (packages/cli/src/commands/replay.ts:851) → mkdir returns undefined → the ACL is skipped → TraceWriter.create mkdirs a new run directory beneath the unprotected parent. Every later orca record, attach or fork in that directory does the same, because the directory now always exists. The store is never healed.
On Windows those directories and files carry the ACL inherited from the workspace — NT AUTHORITY\Authenticated Users:(I)(M) and BUILTIN\Users:(I)(RX) under a workspace on C:\, exactly the inherited ACEs the commit message records — so the shipped trace (events.jsonl with full model requests and responses, blobs, mcp-frames.jsonl, the environment allowlist) and the replay trace are readable by every account on the machine. That directly contradicts the sentence this commit adds to SECURITY.md: ".orca/runs is created granting only its owner, SYSTEM and Administrators, and everything written beneath it inherits that". (On POSIX the same path is outside the promise too: the bare mkdir ignores DIR_MODE, so that store is created with the umask's 0755 rather than 0700.)
Fix: let quickstart create the store through the function that owns it —
const dir = await ensureRunsDir(target); // imported from '@orcareplay/core'
... await cp(join(ASSET, 'trace'), join(dir, runId), { recursive: true });
(or, minimally, follow the bare mkdir with await restrictToOwner(join(target, '.orca', 'runs'), 0o700)). If "never touch a directory we did not create" is really wanted, the check has to distinguish a directory the user deliberately opened up from one orca itself created without the ACL — e.g. on Windows apply the ACL whenever the store's DACL still carries inherited entries, rather than keying off this call's return value.
There was a problem hiding this comment.
Confirmed and fixed in a2f149a. orca quickstart --dir … before the change:
.orca/runs :: Authenticated Users:(I)(M) | Users:(I)(RX)
so the store the README's first line lays down was the broad one, and every later record in it inherited that. Now await ensureRunsDir(target), as you suggested and as orca pull already does. After:
.orca/runs :: Administrators:(OI)(CI)(F) | SYSTEM:(OI)(CI)(F) | <owner>:(OI)(CI)(F)
.../run_…/events.jsonl :: (I) of the same three
One correction to the finding: .orca/.gitignore was present after quickstart even before this — a later ensureRunsDir from the replay writes it. The ACL was the half that was genuinely lost, because by then the directory already existed. The new test asserts both anyway, since the .gitignore half is the part that can be checked on every platform.
All three are real, all three were reproduced on Windows, and the first two
left the fix doing nothing at all for most installs.
**The fallback path was drive-relative.** `'C:\Windows'` is not `C:\Windows`:
`\W` is not an escape, so the literal's value is `C:Windows` — nine characters,
and `isAbsolute` false. A drive letter with no separator is resolved by
CreateProcess against the current directory on C:, which during a recording is
the workspace being recorded, and `Windows/System32/icacls.exe` is a storable
git path. The guess reintroduced precisely the hijack the comment above it says
absolute paths exist to prevent. It is gone: `SystemRoot` is set by the OS in
every normal environment, and where it is not, refusing is the only answer that
cannot be silently wrong. Spelling it correctly would still be a guess, and
wrong on a machine whose Windows is not on C:. The same broken literal had been
copied into three test helpers; those are corrected.
**The ACL was applied only to a store this call created.** `mkdir(recursive)`
returns undefined for a directory that already exists, so a store skipped once
was skipped forever — every install predating this, and nothing anywhere
repairs it. Reproduced: a hand-made `.orca/runs`, then `orca record`, and the
new trace comes out
events.jsonl :: Authenticated Users:(I)(M) | Users:(I)(RX)
which is the state SECURITY.md had just been edited to call fixed. The
create-only rule is right on POSIX — the mode is not load-bearing there, every
file gets its own 0600 from the writer, and a directory someone deliberately
opened up is theirs. It does not translate: on Windows an inherited ACL is what
the workspace happened to hand down, not a choice. So on Windows it is applied
either way.
**`orca quickstart` made the store by hand.** A bare recursive `mkdir`, so the
directory the README's first line produces had the broad ACL and `ensureRunsDir`
saw it as pre-existing ever after. Reproduced, before this: quickstart's store
came out `Users:(RX)` / `Authenticated Users:(M)`, and so did everything later
recorded in it. It now goes through `ensureRunsDir` like every other caller —
`orca pull` was moved off its own bare mkdir for the same two reasons.
Bounds worth stating rather than implying: this covers what is written from now
on. `icacls` on a parent does not re-propagate, and rewriting the ACL of every
file in a store already on disk is not something `record` should do behind
someone's back. SECURITY.md says so.
Three regression tests, each verified to fail against the reverted
implementation: quickstart's store carries `.gitignore` and the owner-only ACL;
`ensureRunsDir` restricts a directory it did not create; `restrictToOwner`
refuses when `SystemRoot` is absent instead of resolving a workspace-relative
path.
Windows: 10 failures on main, 9 with this branch, no new ones.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 493 calls · 50.8M tokens · 99% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
| '/grant:r', | ||
| ...trustees.map((sid) => `*${sid}:${inherit}(F)`), | ||
| '/q', | ||
| ]); |
There was a problem hiding this comment.
🟠 P1 Windows branch adds an allowlist to the existing DACL instead of replacing it, so pre-existing explicit grants survive
The POSIX side of this function is chmod(path, mode): it replaces the whole permission state. (Where a POSIX ACL already exists, chmod is still a replacement — the new group bits become the ACL mask, which nullifies the named user/group entries.) The Windows side is modelled on it but does not replace anything: icacls /inheritance:r removes only ACEs that carry the inherited flag, and /grant:r replaces "previously granted explicit permissions" for the three trustees named, nothing else. So the resulting DACL is {owner, SYSTEM, Administrators} ∪ {whatever explicit ACEs the path already had}, with their inheritance flags intact.
That contradicts both the function's own doc comment ("the only trustees left are the owner, SYSTEM and Administrators") and the SECURITY.md sentence this same change adds (".orca/runs grants only its owner, SYSTEM and Administrators, and everything written beneath it inherits that"). Reachable whenever the path carries a non-inherited grant: a directory whose permissions an administrator set with "Replace all child object permissions" (which turns inherited ACEs into explicit ones, keeping (OI)(CI)), a tree restored with robocopy /SEC or copied between machines, a store on a share where someone ran icacls /grant by hand on the store or on the workspace root, etc. On such a path:
- if the surviving ACE is inheritable (the "replace child permissions" case), everything written afterwards inherits it: every run directory, blob,
tls/and — viawriteConfigrestricting~/.config/orcaand then creatingconfig.json— the gateway API key, and the run CA'sca.key, stay readable by that principal. That is precisely the state this commit exists to end. - if it is not inheritable, the store root itself stays listable and, with
(F), creatable/deletable by that principal: runs can be deleted or planted in it by a non-owner even though the new runs cannot be read.
None of the tests can see this: private-files.test.ts asserts the trustee set for a file it creates in a fresh mkdtemp (inherited-only DACL), and the three expectOwnerOnly helpers only assert that no (I) entry is left — which is exactly the check a surviving explicit ACE passes.
Fix (fail closed): make the Windows branch replace the DACL rather than add to it. The existing test helper already shows the route — enumerate the current DACL by SID (icacls <path> /save <tmp>, parse the SDDL) and /remove:g *SID / /remove:d *SID every trustee outside the allowlist before granting, or issue /reset and then the grants; at the minimum, re-read the DACL after the grant and throw if a trustee outside {owner, SYSTEM, Administrators} is present.
There was a problem hiding this comment.
Confirmed and fixed in 81ba561. You are right on the mechanism and on the tests being blind to it.
Reproduced first — plant an explicit BUILTIN\Users:(F) (by SID, so no name has to resolve), then restrict:
before D:AI(A;;FA;;;BU)(A;ID;…)…
after D:PAI(A;;FA;;;BA)(A;;FA;;;SY)(A;;FA;;;<owner>)(A;;FA;;;BU) <- BU survived
and the inheritable case is the worse one, exactly as you describe:
runs/ D:PAI(A;OICI;FA;;;BA)(A;OICI;FA;;;SY)(A;OICI;FA;;;<owner>)(A;OICI;FA;;;BU)
events.jsonl D:AI(A;ID;FA;;;BA)(A;ID;FA;;;SY)(A;ID;FA;;;<owner>)(A;ID;FA;;;BU)
Fixed with /reset before the restrict rather than by enumerating and removing: it drops every explicit ACE and leaves what the parent hands down, /inheritance:r then takes that away too, and /grant:r writes onto an empty DACL. Two invocations, because icacls rejects /reset alongside /inheritance:r — I tried the single call first and it errored out, which is worth recording since it fails in a way that leaves the path untouched.
One property worth naming, since it was the reason not to prefer enumerate-and-remove: /reset does not widen anything on the way through. A directory is restricted before anything is written into it, so what a child inherits back from /reset is already owner-only. Measured rather than assumed.
After:
file D:PAI(A;;FA;;;BA)(A;;FA;;;SY)(A;;FA;;;<owner>)
dir D:PAI(A;OICI;FA;;;BA)(A;OICI;FA;;;SY)(A;OICI;FA;;;<owner>)
child D:AI(A;ID;FA;;;BA)(A;ID;FA;;;SY)(A;ID;FA;;;<owner>)
Tests, both halves of your point:
- a new case plants an inheritable explicit grant and asserts the exact trustee set, on the directory and on a file written into it afterwards. Verified red without the
/reset— four trustees where three were expected. - the three
expectOwnerOnlyhelpers checked only that nothing was inherited, which a surviving explicit ACE passes by being explicit. They now also require exactly three grants, counted rather than named since the names icacls prints are localized.
I did not take the "re-read the DACL after the grant and throw" option: with /reset the end state follows from icacls semantics I have now measured, and the guarantee is pinned by a test that goes red if those change, rather than by a third spawn on every path.
Review round two, and it is right. `chmod` replaces the whole permission state;
the Windows branch was modelled on it and replaced nothing.
`/inheritance:r` removes only the ACEs carrying the inherited flag, and
`/grant:r` replaces previously granted explicit permissions *for the trustees it
names*. An explicit ACE for anyone else survives both. Measured — grant
BUILTIN\Users explicitly, then restrict:
D:PAI(A;;FA;;;BA)(A;;FA;;;SY)(A;;FA;;;<owner>)(A;;FA;;;BU)
BU is still there, with full access, on a DACL the doc comment above it says
holds "only the owner, SYSTEM and Administrators". On a directory the survivor
keeps its (OI)(CI), so it is worse than one stale ACE — everything written
afterwards inherits it:
runs/ D:PAI(A;OICI;FA;;;BA)(A;OICI;FA;;;SY)(A;OICI;FA;;;<owner>)(A;OICI;FA;;;BU)
events.jsonl D:AI(A;ID;FA;;;BA)(A;ID;FA;;;SY)(A;ID;FA;;;<owner>)(A;ID;FA;;;BU)
Not a hypothetical shape. "Replace all child object permissions" converts
inherited ACEs into explicit ones; so do `robocopy /SEC` and a tree carried
between machines.
`/reset` first, which drops every explicit ACE and leaves what the parent hands
down; `/inheritance:r` then takes that away too, and `/grant:r` writes onto an
empty DACL. The two cannot be combined in one invocation — icacls rejects
`/reset` alongside `/inheritance:r`, which is how the first attempt at this was
caught. Nothing is widened on the way through: a directory is restricted before
anything is written into it, so what a child inherits back from `/reset` is
already owner-only. Verified, not assumed.
The tests could not see any of this, which the review also said. Two changes:
- a case that plants an inheritable explicit grant for BUILTIN\Users and then
asserts the exact trustee set, on the directory and on a file written into
it afterwards. Red without the `/reset`, with four trustees where three
were expected.
- the three `expectOwnerOnly` helpers checked only that nothing was inherited
— which a surviving explicit ACE passes, being explicit. They now also
require exactly three grants. Counted rather than named, because the names
icacls prints are localized.
Windows: 10 failures on main, 9 with this branch, no new ones.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 5 issues in this PR: 🟠 5 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 470 calls · 44.9M tokens · 99% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
| // | ||
| // Nothing is widened on the way through. A directory is restricted before anything is written | ||
| // into it, so what a child inherits back from `/reset` is already owner-only. | ||
| await run(icacls, [path, '/reset', '/q']); |
There was a problem hiding this comment.
🟠 P1 Do not widen the DACL: /reset runs on a live store root before the narrowing grant
restrictToOwner now starts with icacls <path> /reset, which replaces the object's DACL with whatever the parent hands down, and only afterwards (a second icacls spawn later) drops inheritance and grants the three trustees. Between the two calls the object holds the parent's ACL — under a workspace on C:\ that is Authenticated Users:(I)(M) / BUILTIN\Users:(I)(RX), the state SECURITY.md and this commit exist to remove.
The comment on lines 56–57 asserts nothing is widened because "a directory is restricted before anything is written into it, so what a child inherits back from /reset is already owner-only". That holds for <run>/tls/ca.key, whose parent was restricted one line earlier, and for config.json after its directory. It is false for the two call sites this change reaches whose parent is an ordinary, world-readable directory:
ensureRunsDir(packages/core/src/paths.ts:61) now runsrestrictToOwner(dir, 0o700)on every Windows invocation, including on a store that already exists and is in use (that is the deliberate part of the change). Its parent,<workspace>/.orca, is made by a plainmkdirand is never restricted.writeConfig(packages/cli/src/config.ts:111) does the same to~/.config/orca, whose parent.configwas not restricted either.
So on Windows, from this commit, record, attach, replay and pull each briefly restore the workspace's ACL on the store root while the store may be in use. Windows copies the parent's inheritable ACEs into a new object at creation time, and nothing in orca re-propagates afterwards (restrictToOwner is only ever called on those three paths, and the writer's mode: 0o600 is discarded on Windows — packages/core/src/writer.ts:113,138,255) — the commit's own test replaces the DACL rather than adding to it demonstrates exactly that inheritance for a file written after a restriction. Objects created during the window therefore keep the broad ACL permanently: a concurrent record's blobs, fs/ capture and events.jsonl, a concurrent pull's .incoming staging, and orca/runs/<run>.lock, the file withRunLock (packages/cli/src/commands/sync.ts:516) rests mutual exclusion on — a lock another local account may overwrite is not a lock. Any account that opens a handle during the window also keeps that access for the life of the handle.
Fix (never widen): read the current DACL first (icacls <path> /save gives SDDL, and the ACE trustees are SIDs), remove every trustee outside {owner, SYSTEM, Administrators} with icacls <path> /remove:g *<sid>, then run the existing /inheritance:r + /grant:r. Every step then narrows, the order of the two calls stops mattering, and a failure part-way through leaves the object at least as protected as it started.
There was a problem hiding this comment.
Confirmed and fixed in e96e069. All five findings are the same defect and it is mine — the /reset I added in the previous round made a function that had only ever narrowed into one that widens first.
Reproduced on a real store before changing anything:
.orca (A;ID;0x1301bf;;;AU)(A;ID;0x1200a9;;;BU) <- never restricted, as you say
.orca/runs restricted D:PAI(A;OICI;FA;;;BA)(A;OICI;FA;;;SY)(A;OICI;FA;;;<owner>)
after /reset D:AI(...;AU)(...;BU) <- the workspace ACL
child created then D:AI(...;AU)(...;BU) <- and keeps it permanently
so the comment claiming nothing is widened was true only for tls/ and config.json, exactly as you split it.
Rather than hoisting the SID lookup or restoring a snapshot, the widening is gone: one invocation, and every part of it narrows —
icacls <path> /inheritance:r /remove *<foreign> ... /grant:r *<owner>:(F) *SY:(F) *BA:(F) /q
Everything that can fail — the SID lookup, the read of the current DACL, its temp file — happens before it, so a failure leaves the path exactly as found. That answers the ordering point and the snapshot point together, since there is no longer an intermediate state to be caught in.
Two things worth recording from doing it:
- Explicit trustees only. A directory under
%TEMP%here carries inherited ACEs for accounts that no longer exist, andicacls ... /inheritance:r /remove *<orphan-SID>exits 1332 and applies nothing — after inheritance is stripped the SID no longer maps to a name./inheritance:rwas always going to remove those anyway, so filtering the removal list to non-inherited ACEs is both the more accurate ask and how that failure disappears. /restorewith a hand-written SDDL would have been the tidiest form of "replace the DACL outright". It needs SeSecurityPrivilege and fails unelevated, so it is out. Trustees are instead passed back in whatever spelling the descriptor used —/removeaccepts*BUas readily as*S-1-5-32-545— so there is no abbreviation table to fall out of date and silently pass a trustee through.
On your point 3 in the last finding, re-verifying whether icacls accepts /reset together with /inheritance:r: it does not, it errors and applies nothing. But that is moot now — there is nothing to combine.
New test: a store under a parent granting BUILTIN\Users inheritably, hammered with child creation while being restricted five times over; no child may carry anything but the three trustees. Red against the /reset version, with six. A detector rather than a proof — it can only fail while such a window exists, never spuriously.
| // Inheritable, for a directory, so that everything written inside it afterwards is covered | ||
| // without a further call — which is the only affordable way to secure a tree of many files. | ||
| const inherit = mode === 0o700 ? '(OI)(CI)' : ''; | ||
| const trustees = [await currentAccountSid(), SYSTEM, ADMINISTRATORS]; |
There was a problem hiding this comment.
🟠 P1 Resolve the account SID before /reset, so a lookup failure cannot leave the ACL widened
The sequence is: /reset (widens), then resolve this account's SID by spawning whoami.exe, then narrow. The SID lookup is the one step that can fail for reasons unrelated to icacls — whoami.exe missing from SystemRoot\System32 (stripped/server-core images, some Windows containers), a non-zero exit under a restricted token, or output the /S-1-[\d-]+/ regex does not match (readSid throws deliberately in each case). When it throws, the second icacls never runs and the object is left holding the parent's inherited ACL — a state the previous implementation could not produce, since it only ever narrowed.
Two call sites reach that state:
ensureRunsDir(packages/core/src/paths.ts:61) leaves the store root inheritable and then rethrows, so the store is broad until some later command happens to succeed in restricting it again.writeConfig(packages/cli/src/config.ts:111) swallows the directory failure with.catch(() => {}), so the run continues:config.jsonis written into a directory that is momentarily holding~/.config's inherited ACL, inherits it, and onlyrestrictToOwner(path, 0o600)(which repeats the/resetand the samewhoami) stands between the gateway API key and every account on the machine. If that lookup fails too,writeConfigthrows with the key already on disk and readable, and the user sees a failure rather than a leftover credential.
The SID does not depend on the path, so reordering costs nothing and removes the whole class: nothing is left widened unless icacls itself fails, and icacls is invoked with the path already in hand.
Fix: hoist the trustee list above the widening call —
const trustees = [await currentAccountSid(), SYSTEM, ADMINISTRATORS];
await run(icacls, [path, '/reset', '/q']);
await run(icacls, [path, '/inheritance:r', '/grant:r', ...trustees.map(...), '/q']);
and, since the directory call in writeConfig now widens before it can throw, stop swallowing that error (or re-restrict before writing the file) so a failed restriction cannot be followed by writing the credential.
There was a problem hiding this comment.
Confirmed and fixed in e96e069. All five findings are the same defect and it is mine — the /reset I added in the previous round made a function that had only ever narrowed into one that widens first.
Reproduced on a real store before changing anything:
.orca (A;ID;0x1301bf;;;AU)(A;ID;0x1200a9;;;BU) <- never restricted, as you say
.orca/runs restricted D:PAI(A;OICI;FA;;;BA)(A;OICI;FA;;;SY)(A;OICI;FA;;;<owner>)
after /reset D:AI(...;AU)(...;BU) <- the workspace ACL
child created then D:AI(...;AU)(...;BU) <- and keeps it permanently
so the comment claiming nothing is widened was true only for tls/ and config.json, exactly as you split it.
Rather than hoisting the SID lookup or restoring a snapshot, the widening is gone: one invocation, and every part of it narrows —
icacls <path> /inheritance:r /remove *<foreign> ... /grant:r *<owner>:(F) *SY:(F) *BA:(F) /q
Everything that can fail — the SID lookup, the read of the current DACL, its temp file — happens before it, so a failure leaves the path exactly as found. That answers the ordering point and the snapshot point together, since there is no longer an intermediate state to be caught in.
Two things worth recording from doing it:
- Explicit trustees only. A directory under
%TEMP%here carries inherited ACEs for accounts that no longer exist, andicacls ... /inheritance:r /remove *<orphan-SID>exits 1332 and applies nothing — after inheritance is stripped the SID no longer maps to a name./inheritance:rwas always going to remove those anyway, so filtering the removal list to non-inherited ACEs is both the more accurate ask and how that failure disappears. /restorewith a hand-written SDDL would have been the tidiest form of "replace the DACL outright". It needs SeSecurityPrivilege and fails unelevated, so it is out. Trustees are instead passed back in whatever spelling the descriptor used —/removeaccepts*BUas readily as*S-1-5-32-545— so there is no abbreviation table to fall out of date and silently pass a trustee through.
On your point 3 in the last finding, re-verifying whether icacls accepts /reset together with /inheritance:r: it does not, it errors and applies nothing. But that is moot now — there is nothing to combine.
New test: a store under a parent granting BUILTIN\Users inheritably, hammered with child creation while being restricted five times over; no child may carry anything but the three trustees. Red against the /reset version, with six. A detector rather than a proof — it can only fail while such a window exists, never spuriously.
| '/grant:r', | ||
| ...trustees.map((sid) => `*${sid}:${inherit}(F)`), | ||
| '/q', | ||
| ]); |
There was a problem hiding this comment.
🟠 P1 Resolve the SID and snapshot the DACL before /reset, and put the DACL back if the grant fails
The Windows branch now destroys the current DACL in one step and rebuilds it in a second, with no way to undo the first. /reset replaces the ACL with the default inherited one — i.e. whatever the parent hands down — and only the following /grant:r call makes it owner-only again. If that second call fails, the path is left exactly in the state this commit exists to remove.
Two concrete triggers, both between the two calls:
await currentAccountSid()(line 63) runs after the reset. It is a spawn ofwhoami.exewhose output must parse (readSidthrows “could not read this account's SID from …” if the regex misses, andexecFilecan also fail on spawn, EMPFILE/EAGAIN under load, or an AppLocker/WDAC policy). Before this commit the SID lookup was the first thing the Windows branch did, so a failure there left the ACL untouched and the caller aborted with the path still restricted — fail-closed. Now the identical failure leaves the DACL already dropped.- The second
icaclsinvocation itself can exit non-zero (antivirus holding the just-written file, a transient spawn/exit failure); the grant is not applied, and there is no catch that restores anything.
What is left behind, for the caller that matters most — ensureRunsDir on Windows, which now calls this on .orca/runs on every record/attach/replay/pull/quickstart (packages/core/src/paths.ts:61) — is the ACL .orca/runs would have had if it had just been created inside the workspace: the broad inherited ACEs (BUILTIN\Users:(I)(RX), Authenticated Users:(I)(M), the ones quoted at the top of this very file, and the ones the pre-fix reproduction in commit a2f149a shows on events.jsonl). .orca itself is never restricted, only runs is, so /reset on runs yields the workspace's ACL. The result: a store that was owner-only is opened to every account on the machine, and everything written into it from then on (run directories, blobs/, .incoming, a fresh tls/ca.key, events.jsonl) inherits that instead. The command still throws, so the operator sees a failure and has no reason to think the store's protection was removed; and because nothing else ever repairs the store (ensureRunsDir is the only place that restricts it, and every retry does /reset first), it stays that way until an attempt happens to succeed.
The window is also live in the success case: any child created between the two spawns inherits the broad ACL while the store is momentarily unprotected — and under the semantics this repository states in SECURITY.md ("icacls on a parent does not re-propagate to existing children") such a child keeps it.
Fix: do everything that can fail before touching the existing DACL, and do not drop it until the replacement is in hand, e.g.
const icacls = system32('icacls.exe');
const inherit = mode === 0o700 ? '(OI)(CI)' : '';
const ownerSid = await currentAccountSid(); // first — before anything is modified
const grants = [ownerSid, SYSTEM, ADMINISTRATORS].map((s) => `*${s}:${inherit}(F)`);
const backup = join(await mkdtemp(join(tmpdir(), 'orca-acl-')), 'acl');
await run(icacls, [path, '/save', backup]); // so the old DACL can be put back
try {
await run(icacls, [path, '/reset', '/q']);
await run(icacls, [path, '/inheritance:r', '/grant:r', ...grants, '/q']);
} catch (err) {
await run(icacls, [path, '/restore', backup]).catch(() => undefined); // leave it as found, then fail closed
throw err;
} finally {
await rm(dirname(backup), { recursive: true, force: true }).catch(() => undefined);
}
A minimal version, if the snapshot is judged too heavy, is at least to hoist await currentAccountSid() above the /reset so the SID lookup can no longer fail after the DACL has been discarded (that removal alone closes trigger 1).
There was a problem hiding this comment.
Confirmed and fixed in e96e069. All five findings are the same defect and it is mine — the /reset I added in the previous round made a function that had only ever narrowed into one that widens first.
Reproduced on a real store before changing anything:
.orca (A;ID;0x1301bf;;;AU)(A;ID;0x1200a9;;;BU) <- never restricted, as you say
.orca/runs restricted D:PAI(A;OICI;FA;;;BA)(A;OICI;FA;;;SY)(A;OICI;FA;;;<owner>)
after /reset D:AI(...;AU)(...;BU) <- the workspace ACL
child created then D:AI(...;AU)(...;BU) <- and keeps it permanently
so the comment claiming nothing is widened was true only for tls/ and config.json, exactly as you split it.
Rather than hoisting the SID lookup or restoring a snapshot, the widening is gone: one invocation, and every part of it narrows —
icacls <path> /inheritance:r /remove *<foreign> ... /grant:r *<owner>:(F) *SY:(F) *BA:(F) /q
Everything that can fail — the SID lookup, the read of the current DACL, its temp file — happens before it, so a failure leaves the path exactly as found. That answers the ordering point and the snapshot point together, since there is no longer an intermediate state to be caught in.
Two things worth recording from doing it:
- Explicit trustees only. A directory under
%TEMP%here carries inherited ACEs for accounts that no longer exist, andicacls ... /inheritance:r /remove *<orphan-SID>exits 1332 and applies nothing — after inheritance is stripped the SID no longer maps to a name./inheritance:rwas always going to remove those anyway, so filtering the removal list to non-inherited ACEs is both the more accurate ask and how that failure disappears. /restorewith a hand-written SDDL would have been the tidiest form of "replace the DACL outright". It needs SeSecurityPrivilege and fails unelevated, so it is out. Trustees are instead passed back in whatever spelling the descriptor used —/removeaccepts*BUas readily as*S-1-5-32-545— so there is no abbreviation table to fall out of date and silently pass a trustee through.
On your point 3 in the last finding, re-verifying whether icacls accepts /reset together with /inheritance:r: it does not, it errors and applies nothing. But that is moot now — there is nothing to combine.
New test: a store under a parent granting BUILTIN\Users inheritably, hammered with child creation while being restricted five times over; no child may carry anything but the three trustees. Red against the /reset version, with six. A detector rather than a proof — it can only fail while such a window exists, never spuriously.
| // Inheritable, for a directory, so that everything written inside it afterwards is covered | ||
| // without a further call — which is the only affordable way to secure a tree of many files. | ||
| const inherit = mode === 0o700 ? '(OI)(CI)' : ''; | ||
| const trustees = [await currentAccountSid(), SYSTEM, ADMINISTRATORS]; |
There was a problem hiding this comment.
🟠 P1 Do not reset the DACL before the trustee list is known: the widening step must not be able to outlive a failure
/reset is the widening half of this function: it drops every explicit ACE and leaves the object with what its parent hands down. Only the second invocation narrows it again (/inheritance:r + /grant:r owner/SYSTEM/BA), and between the two there is an await currentAccountSid() — a whoami.exe subprocess that can reject — plus a second CreateProcess that can fail for unrelated reasons (ENOENT on images without whoami/icacls, transient ERROR_NOT_ENOUGH_MEMORY/ERROR_NO_SYSTEM_RESOURCES, the icacls child killed).
Before this commit the order was safe: nothing was mutated until the SID was in hand, so a failure left the ACL exactly as it was. Now a failure leaves the path in the state /reset produced. Concretely, on Windows:
- The call site that matters is
.orca/runs(packages/core/src/paths.ts:61), and its parent.orcais never restricted — nothing in the repo callsrestrictToOwneron it (only.orca/runs,~/.config/orcaand<run>/tlsare restricted). So/reseton.orca/runshands the store the workspace's inherited ACL — theBUILTIN\Users:(I)(RX)/Authenticated Users:(I)(M)shape this series exists to remove. IfcurrentAccountSid()or the grant invocation then throws,ensureRunsDirpropagates that and everyrecord,attach,replayandpullin that workspace now fails while the store is left more permissive than it was before the call: the directory, and every run directory/blob/trace created afterwards, inherits something any local account can read. Nothing repairs it until a later invocation happens to succeed. - Even when nothing fails, this is a per-invocation window, because
ensureRunsDirresets.orca/runson every Windows command, not just at creation. Windows propagates a changed inheritable ACL to descendants that are not protected — which is exactly the set orca creates (the run directories are made bymkdirafter the parent is narrowed, so their ACEs are inherited, not protected). The comment two lines above ("Nothing is widened on the way through… what a child inherits back from/resetis already owner-only") holds only for the CA files, where the parenttls/is restricted first; it does not hold for the store, and SECURITY.md's "icaclson the parent does not re-propagate" is the assumption the race is trusted to. - The same shape applies to
writeConfig(packages/cli/src/config.ts:111-113): the directory call's error is swallowed, so a failure there meansconfig.json's/resetinherits the profile's broad ACL, and if the file's grant then fails too (same cause) the gateway API key is left on disk readable by every account while the command reports a SystemRoot/icacls error.
Fix (concrete):
- Hoist everything that can fail above the first mutation:
const trustees = [await currentAccountSid(), SYSTEM, ADMINISTRATORS];(andconst icacls = system32('icacls.exe');) must be computed beforeawait run(icacls, [path, '/reset', '/q']). - Make the reset state safe rather than broad at the call site: restrict the parent (
.orca) before.orca/runs, the wayRunCa.createrestrictstls/before writingca.key, so/resetcan only inherit owner-only ACEs. - Do not leave the widened ACL behind on failure: wrap the narrowing invocation so that if it rejects, the non-widening form (
/inheritance:r+/grant:r, which never widened anything) is applied as a fallback before rethrowing — or refuse to run/resetat all when the trustees could not be resolved.
There was a problem hiding this comment.
Confirmed and fixed in e96e069. All five findings are the same defect and it is mine — the /reset I added in the previous round made a function that had only ever narrowed into one that widens first.
Reproduced on a real store before changing anything:
.orca (A;ID;0x1301bf;;;AU)(A;ID;0x1200a9;;;BU) <- never restricted, as you say
.orca/runs restricted D:PAI(A;OICI;FA;;;BA)(A;OICI;FA;;;SY)(A;OICI;FA;;;<owner>)
after /reset D:AI(...;AU)(...;BU) <- the workspace ACL
child created then D:AI(...;AU)(...;BU) <- and keeps it permanently
so the comment claiming nothing is widened was true only for tls/ and config.json, exactly as you split it.
Rather than hoisting the SID lookup or restoring a snapshot, the widening is gone: one invocation, and every part of it narrows —
icacls <path> /inheritance:r /remove *<foreign> ... /grant:r *<owner>:(F) *SY:(F) *BA:(F) /q
Everything that can fail — the SID lookup, the read of the current DACL, its temp file — happens before it, so a failure leaves the path exactly as found. That answers the ordering point and the snapshot point together, since there is no longer an intermediate state to be caught in.
Two things worth recording from doing it:
- Explicit trustees only. A directory under
%TEMP%here carries inherited ACEs for accounts that no longer exist, andicacls ... /inheritance:r /remove *<orphan-SID>exits 1332 and applies nothing — after inheritance is stripped the SID no longer maps to a name./inheritance:rwas always going to remove those anyway, so filtering the removal list to non-inherited ACEs is both the more accurate ask and how that failure disappears. /restorewith a hand-written SDDL would have been the tidiest form of "replace the DACL outright". It needs SeSecurityPrivilege and fails unelevated, so it is out. Trustees are instead passed back in whatever spelling the descriptor used —/removeaccepts*BUas readily as*S-1-5-32-545— so there is no abbreviation table to fall out of date and silently pass a trustee through.
On your point 3 in the last finding, re-verifying whether icacls accepts /reset together with /inheritance:r: it does not, it errors and applies nothing. But that is moot now — there is nothing to combine.
New test: a store under a parent granting BUILTIN\Users inheritably, hammered with child creation while being restricted five times over; no child may carry anything but the three trustees. Red against the /reset version, with six. A detector rather than a proof — it can only fail while such a window exists, never spuriously.
| // Inheritable, for a directory, so that everything written inside it afterwards is covered | ||
| // without a further call — which is the only affordable way to secure a tree of many files. | ||
| const inherit = mode === 0o700 ? '(OI)(CI)' : ''; | ||
| const trustees = [await currentAccountSid(), SYSTEM, ADMINISTRATORS]; |
There was a problem hiding this comment.
🟠 P1 Don't leave the widened state: /reset re-inherits the parent's ACL before the grant, and the SID lookup sits inside that window
The new first step is destructive in the widening direction: icacls <path> /reset replaces the DACL with what the parent hands down, and only the following invocation narrows it. The comment on lines 56-57 ("Nothing is widened on the way through. A directory is restricted before anything is written into it, so what a child inherits back from /reset is already owner-only") holds for tls/ and config.json, whose parents are restricted first — but not for the call that carries the whole fix.
ensureRunsDir (packages/core/src/paths.ts:61) applies this on Windows to <cwd>/.orca/runs on every writing command, and .orca itself is never restricted — it is created by that same mkdir and keeps the workspace's ACL. So on a workspace under C:\ the sequence is: .orca/runs carries Authenticated Users:(OI)(CI)(M) / Users:(OI)(CI)(RX) → whoami is spawned → a second icacls is spawned → only then is it owner-only again. For that window the store root holds exactly the ACL this commit exists to remove, and every orca record/attach/fork/replay --trace/pull/quickstart in that directory reopens it.
Anything created under the root inside the window inherits those ACEs and keeps them permanently — icacls does not re-propagate to existing children, which is the property this change itself asserts (SECURITY.md lines 38-39, paths.ts lines 57-59) — so a second orca record running in the same workspace, or any local process that wins the race, leaves a run directory whose events.jsonl, blobs and fs snapshots are readable by every account on the machine, silently and for good. The same hole is the failure path: if currentAccountSid()/readSid() (whoami missing, unparsable output) or the granting icacls throws, restrictToOwner rejects after the DACL has already been replaced, leaving .orca/runs holding the workspace's inherited ACL while ensureRunsDir aborts the command.
Concrete fix, in order of value: (1) hoist const trustees = [await currentAccountSid(), SYSTEM, ADMINISTRATORS]; above the two icacls calls so no unrelated step can fail once the DACL is replaced; (2) make the widened state impossible to leave behind — run the granting icacls in a try, and on failure re-narrow with a SID-free grant (/inheritance:r /grant:r *S-1-5-18:(OI)(CI)(F) *S-1-5-32-544:(OI)(CI)(F), no whoami needed) before rethrowing, so the path is never left inheriting the workspace; (3) remove the widening at the source — restrict .orca first (or otherwise guarantee the parent is already owner-only) so /reset can only inherit owner-only ACEs, or, if icacls in fact accepts them together, issue /reset and /inheritance:r /grant:r in one invocation (the comment on lines 52-54 claims it rejects that combination; worth re-verifying, because it is the only way to close the window entirely); (4) correct the comment on lines 56-57.
There was a problem hiding this comment.
Confirmed and fixed in e96e069. All five findings are the same defect and it is mine — the /reset I added in the previous round made a function that had only ever narrowed into one that widens first.
Reproduced on a real store before changing anything:
.orca (A;ID;0x1301bf;;;AU)(A;ID;0x1200a9;;;BU) <- never restricted, as you say
.orca/runs restricted D:PAI(A;OICI;FA;;;BA)(A;OICI;FA;;;SY)(A;OICI;FA;;;<owner>)
after /reset D:AI(...;AU)(...;BU) <- the workspace ACL
child created then D:AI(...;AU)(...;BU) <- and keeps it permanently
so the comment claiming nothing is widened was true only for tls/ and config.json, exactly as you split it.
Rather than hoisting the SID lookup or restoring a snapshot, the widening is gone: one invocation, and every part of it narrows —
icacls <path> /inheritance:r /remove *<foreign> ... /grant:r *<owner>:(F) *SY:(F) *BA:(F) /q
Everything that can fail — the SID lookup, the read of the current DACL, its temp file — happens before it, so a failure leaves the path exactly as found. That answers the ordering point and the snapshot point together, since there is no longer an intermediate state to be caught in.
Two things worth recording from doing it:
- Explicit trustees only. A directory under
%TEMP%here carries inherited ACEs for accounts that no longer exist, andicacls ... /inheritance:r /remove *<orphan-SID>exits 1332 and applies nothing — after inheritance is stripped the SID no longer maps to a name./inheritance:rwas always going to remove those anyway, so filtering the removal list to non-inherited ACEs is both the more accurate ask and how that failure disappears. /restorewith a hand-written SDDL would have been the tidiest form of "replace the DACL outright". It needs SeSecurityPrivilege and fails unelevated, so it is out. Trustees are instead passed back in whatever spelling the descriptor used —/removeaccepts*BUas readily as*S-1-5-32-545— so there is no abbreviation table to fall out of date and silently pass a trustee through.
On your point 3 in the last finding, re-verifying whether icacls accepts /reset together with /inheritance:r: it does not, it errors and applies nothing. But that is moot now — there is nothing to combine.
New test: a store under a parent granting BUILTIN\Users inheritably, hammered with child creation while being restricted five times over; no child may carry anything but the three trustees. Red against the /reset version, with six. A detector rather than a proof — it can only fail while such a window exists, never spuriously.
…so widened the store
Review round three, and it is the more serious of the two. `/reset` was the
wrong way to clear explicit ACEs: it replaces the DACL with whatever the
*parent* hands down, so between it and the grant the path holds the workspace's
ACL.
The comment claimed that could not widen anything, because "a directory is
restricted before anything is written into it". That holds for `<run>/tls` and
for `~/.config/orca`, and not for the call that carries the whole fix. `.orca`
is never restricted — only `.orca/runs` is — and `ensureRunsDir` now runs on
every Windows command. Measured on a real store:
.orca (A;ID;0x1301bf;;;AU)(A;ID;0x1200a9;;;BU) — never restricted
.orca/runs restricted D:PAI(A;OICI;FA;;;BA)(A;OICI;FA;;;SY)(A;OICI;FA;;;<owner>)
after /reset D:AI(…;AU)(…;BU) — the workspace's ACL
child born then D:AI(…;AU)(…;BU) — and keeps it, for good
So every `record`, `attach`, `replay` and `pull` reopened the store for a
window, and anything created inside it inherited that permanently, since icacls
does not re-propagate afterwards. On the failure path it is worse: the SID
lookup sat between the two calls, so a `whoami` that could not run left the
store wide open and the command aborted. The implementation before `/reset`
only ever narrowed; this had made it fail open.
Now there is one invocation and every part of it narrows: `/inheritance:r`,
a `/remove` for each explicit trustee outside the allowlist, and `/grant:r`.
Everything that can fail — the SID lookup, the read of the current DACL — runs
before it, so a failure leaves the path exactly as it was found.
Explicit trustees only, which is both more accurate and how the orphan-SID
problem goes away: a directory under %TEMP% here carries inherited ACEs for
accounts that no longer exist, and `icacls … /inheritance:r /remove *<orphan>`
exits 1332 and applies nothing, because after inheritance is stripped the SID
no longer maps to a name. `/inheritance:r` was always going to remove those;
naming them too was asking for something that had already happened.
`/restore` with a hand-written SDDL would have been the tidiest and needs
SeSecurityPrivilege, so it is out. Trustees are passed back in whatever spelling
the descriptor used — `/remove` takes `*BU` as readily as `*S-1-5-32-545` — so
there is no abbreviation table to fall out of date and silently pass something
through.
New test: a store under a parent that grants BUILTIN\Users inheritably, hammered
with child creation while it is restricted five times over. Zero of the children
may carry anything but the three trustees. Red against the `/reset` version with
six. It is a detector rather than a proof — it can only fail while such a window
exists, never spuriously.
Windows: 10 failures on main, 9 with this branch, no new ones.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 2 issues in this PR: 🟠 2 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 533 calls · 57.2M tokens · 99% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
| await restrictToOwner(dirname(path), 0o700).catch(() => {}); | ||
| await writeFile(path, `${JSON.stringify(config, null, 2)}\n`, { mode: 0o600 }); | ||
| await chmod(path, 0o600); | ||
| await restrictToOwner(path, 0o600); |
There was a problem hiding this comment.
🟠 P1 Narrow the config file before the gateway key lands in it, and stop swallowing the call the new comment calls load-bearing
Steps: (1) line 111 applies the directory's ACL with its failure swallowed, (2) line 112 writes the credential into config.json, (3) line 113 narrows the file — and only step 3 can report anything.
On Windows all three are the same ACL application, so a cause that is not specific to one path (icacls.exe erroring, SystemRoot unset — a condition this very commit adds a test asserting throws — an unwritable %TEMP% for the /save scratch file, or a volume that cannot carry a security descriptor such as exFAT/FAT32) makes step 1 a silent no-op and then makes step 3 throw after the API key is on disk. The file is left with whatever ACL ~/.config/orca handed down — on the tree the new comment and SECURITY.md describe, Authenticated Users:(I)(M) / Users:(I)(RX), i.e. readable by every account on the machine; orca setup reports the failure, so the operator has no reason to suspect the key was written at all, and the obvious next move (--key-env) leaves it there.
Note this is not self-healing: nothing removes the file, and the stale key survives until some later successful writeConfig overwrites it.
Fix (order the operations so the secret never exists unprotected, and treat the failure as fatal):
- create the file, narrow it, then write the secret:
await writeFile(path, '', { mode: 0o600 }); await restrictToOwner(path, 0o600); await writeFile(path, json, { mode: 0o600 });— the second write keeps the file's ACL, and an empty file leaks nothing; - or keep the current order and
await rm(path, { force: true }).catch(() => undefined)in acatcharound line 113 before rethrowing; - and drop the
.catch(() => {})on line 111 (or at leastout.warn), since the comment directly above it now says that call, not the mkdir mode, is what keeps the key from every account.
There was a problem hiding this comment.
Confirmed and fixed in 12c3873. Reproduced first, with SystemRoot unset so restrictToOwner throws for a reason that is not specific to the path:
writeConfig threw : cannot locate icacls.exe: SystemRoot is not set …
on disk : {"gateway":{"url":"https://x","api_key":"sk-SECRET-MARKER"}}
Plain text, under the inherited ACL, and the error the operator sees mentions none of it.
I took neither of the two shapes you offered, because both destroy the config that was already there when the restriction fails — truncating in place leaves it empty, and rm on the catch path deletes it. Instead the key goes through a staging file that is narrowed while it is still empty, then renamed over the target:
const staging = `${path}.incoming`;
await writeFile(staging, "", { mode: 0o600 });
try {
await restrictToOwner(staging, 0o600);
await writeFile(staging, json, { mode: 0o600 });
await rename(staging, path);
} catch (err) {
await rm(staging, { force: true }).catch(() => undefined);
throw err;
}Two properties measured rather than assumed: rewriting a file keeps its ACL (so the content write does not undo the narrowing), and a rename over an existing file carries the ACL of the file being moved (so the target ends up with the restricted one, not the old broad one). After the change the same failing run leaves no file at all and the previous config intact.
On the .catch(() => {}) for the directory: kept, but the comment above it no longer claims to be what stands between the key and the machine — that is now the staging narrowing, which is not swallowed. The directory call is what keeps the directory from being listable, and a benign chmod failure on an existing directory is the POSIX case it was written for.
Test, deterministic because SystemRoot is a failure this branch can already inject: unset it, expect the rejection, then assert sk-must-not-land appears in no file under the config directory (the staging file included) and that the earlier config is still there. Red against the old order, with the key in config.json.
| await run(icacls, [path, '/save', saved]); | ||
| // UTF-16LE: the entry's name, then its descriptor. | ||
| const text = await readFile(saved, 'utf16le'); | ||
| const descriptor = text.split(/\r?\n/).find((line) => /^[OGDS]:/.test(line.trim())) ?? ''; |
There was a problem hiding this comment.
🟠 P1 Pick the ACL descriptor by line position, not by a prefix the entry's own path can match
icacls <path> /save <file> writes two lines per entry: the entry's name first, then its SDDL descriptor (the file's own comment on line 97 and private-files.test.ts's sddl() helper, which reads lines[1], both rely on that). explicitTrustees recovers the descriptor with find(line => /^[OGDS]:/.test(line.trim())) — and a name line that begins with O:, G:, D: or S: matches that regex. That is exactly a path on an O:/G:/S:/D: drive, e.g. D:\work\proj\.orca\runs.
Reproduced against the implementation's own code (regex + split, verbatim):
name = "D:\work\proj.orca\runs", descriptor = "D:PAI(A;OICI;FA;;;BA)(A;OICI;FA;;;SY)(A;OICI;FA;;;BU)"
-> descriptor = "D:\work\proj.orca\runs", foreign = [] <-- wrong
name = "C:\work\proj.orca\runs"
-> foreign = ["BA","SY","BU"] <-- correct
Because the wrong line yields no ACE matches, foreign is empty and the icacls invocation at line 67 loses every /remove argument. /inheritance:r still strips the inherited ACEs, and /grant:r only replaces ACLs for the trustees it names — so a pre-existing explicit foreign ACE survives, exactly the hole commit 81ba561 was written to close ("Measured: grant BUILTIN\Users explicitly, then restrict, and (A;;FA;;;BU) is still on the DACL"), and it survives silently: icacls exits 0, restrictToOwner resolves, nothing is reported.
Consequences on a workspace/config directory on a D: (or G:/S:/O:) drive:
restrictToOwner(ca.dir, 0o700)+ the three CA files (packages/proxy/src/ca.ts:282-298): the run CA's private key, which signs the certificates the agent is told to trust — readable/writable by every local account while the code and SECURITY.md say owner-only.ensureRunsDir→restrictToOwner('.orca/runs', 0o700)(packages/core/src/paths.ts:61) on a drive-letter store: a survivingBU:(OI)(CI)(F)keeps propagating to every run directory, blob andtls/written afterwards, since icacls does not re-propagate on later calls.writeConfig(packages/cli/src/config.ts:113) for a home on D::config.jsonkeeps its explicit ACE and with it the gateway API key.
Nothing upstream enforces the assumption: the drive letter comes from process.cwd()/HOME/--dir and is whatever the user's workspace is on. The tests cannot see it — the author's machine and %TEMP% are on C:, and private-files.test.ts's sddl() reads lines[1] directly.
Fix: don't scan for a section prefix, take the descriptor the format actually defines, and refuse rather than silently returning "no foreign trustees" when it cannot be read, e.g.:
const lines = (await readFile(saved, 'utf16le')).split(/\r?\n/).map((l) => l.trim()).filter(Boolean);
const descriptor = lines.at(-1) ?? ''; // the entry's name is first, its descriptor last
if (!/^[OGDS]:/.test(descriptor) || !descriptor.includes('(')) {
throw new Error(could not read the ACL of ${path} from ${saved});
}
(the last-line read is also what keeps a name line containing ( or ; from being mistaken for a descriptor).
There was a problem hiding this comment.
Fixed in 12c3873, but the mechanism in the finding does not hold — worth recording, because the fix is different from the one proposed.
icacls <path> /save <file> writes the basename, not the full path. Measured:
D:\project_mxz\PR_test\fmt-XXXX\runs -> [0] "runs"
[1] "D:(A;ID;FA;;;BA)…"
D:\…\runs\events.jsonl -> [0] "events.jsonl"
D:\ (drive root) -> [0] "D:(A;;FA;;;BA)…" (one line, no name)
So the /^[OGDS]:/ scan cannot match a name line: a Windows filename cannot contain a colon. Ran the planted-ACE case on both a C: path and a D: path — BU is removed in both, foreign is not empty.
The failure direction is right, though, and that is what got fixed: an unrecognised line meant descriptor fell back to "", foreign came back empty, every foreign trustee stayed, and icacls exited 0 with nothing reported. For a function whose whole job is removing them, "could not parse" must not read as "nothing to remove".
So the descriptor is taken by position — lines.at(-1), which is right for both the two-line and the one-line form above — and a line that is not a descriptor throws:
const descriptor = lines.at(-1) ?? "";
if (!/^[OGDS]:/.test(descriptor) || !descriptor.includes("(")) {
throw new Error(`could not read the ACL of ${path}: ${JSON.stringify(descriptor.slice(0, 80))}`);
}Your lines.at(-1) suggestion is what I used. I did not keep the reasoning that a name line could carry a drive prefix — the comment says instead that it cannot today, and that this is a fact about a neighbouring tool rather than about this parse, which is the reason to take the line by position anyway.
No test: with /save writing basenames there is no reachable input that produces an unparsed descriptor, so a test would have to fake the file rather than the condition. The change is defensive and the comment says so.
…arrowed
Review round four. Two findings; the first is a real leak and the second is
right about the failure direction while being wrong about the mechanism.
**The key reached disk before anything protected it.** `writeConfig` wrote
`config.json` at its final path, then narrowed it. `mode: 0o600` is discarded on
Windows, so until that second call the file carried whatever the directory
handed down — and if the call throws (icacls unavailable, `SystemRoot` unset, an
unwritable %TEMP% for its scratch file) the key is on disk, readable, while
`orca setup` reports an error that mentions none of it. Measured, with
`SystemRoot` unset:
writeConfig threw : cannot locate icacls.exe: SystemRoot is not set …
on disk : {"gateway":{"url":"https://x","api_key":"sk-SECRET-MARKER"}}
It is written through a staging file now, narrowed while it is still empty, and
renamed over the target — a rename carries the ACL of the file being moved, so
the key is never on disk under any ACL but the intended one. Neither of the two
simpler shapes would do: truncating in place and deleting on failure both throw
away the config that was already there. After, the same run leaves no file at
all and the previous config intact.
**The ACL descriptor is taken by position, and an unreadable one is refused.**
The review's specific claim does not hold: `icacls /save` writes the *basename*,
not the full path, so a store on `D:\` yields `runs` and the `/^[OGDS]:/` scan
could not match it — checked on C: and on D:, both remove the planted ACE. But
the failure direction it names is real: an unrecognised line meant `foreign`
came back empty, every foreign trustee stayed, and `icacls` still exited 0. The
descriptor is the last line — two for an ordinary path, one for a drive root —
and a line that is not one throws rather than reading as "nothing to remove".
Both tested. The config one is deterministic, because `SystemRoot` is a failure
this branch can already inject: unset it, expect the rejection, then assert the
key is nowhere under the config directory and the earlier config still is. Red
against the old order, with `sk-must-not-land` in `config.json`.
The window detector from the previous round also needed fixing — it deleted each
probe directory as it went and raced `icacls`'s own handle, which is how it
failed under a full suite rather than how the code did. It leaves them now.
Windows: 10 failures on main, 9 with this branch, no new ones.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 2 issues in this PR: 🟠 2 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 596 calls · 68.1M tokens · 99% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
| // existing children, and rewriting the ACL of every file in a store already on disk is not | ||
| // something a `record` should do behind the user's back. | ||
| const created = (await mkdir(dir, { recursive: true, mode: 0o700 })) !== undefined; | ||
| if (created || process.platform === 'win32') await restrictToOwner(dir, 0o700); |
There was a problem hiding this comment.
🟠 P1 Narrow .orca too — the store's parent is left writable by every account, so the .orca/runs ACL can be replaced with a directory someone else owns
The change narrows the store root, <cwd>/.orca/runs, and nothing narrows its parent <cwd>/.orca. On Windows mode: is discarded, so .orca is created with whatever ACL the workspace hands down — the measurements in this very series are NT AUTHORITY\Authenticated Users:(I)(M) / BUILTIN\Users:(I)(RX) for a workspace under a drive root, i.e. every authenticated local account has Modify on <cwd>/.orca. (mkdir gives it 0700 on POSIX, so this is Windows-only, exactly the class of defect this change exists for.)
Consequence: the narrowing on .orca/runs does not hold. Such an account can delete .orca\runs (Modify on the parent includes DELETE_CHILD, which is what decides deleting a child regardless of the child's own DACL) and put a junction in its place pointing at a directory it owns — junctions need no privilege on Windows. On the victim's next orca record, mkdir(…, { recursive: true }) accepts the junction as an existing directory and icacls follows the reparse point, so the ACL restrictToOwner writes lands on the attacker's directory; an object's owner keeps the implicit READ_CONTROL/WRITE_DAC, so the attacker re-grants itself access whenever it likes. Every trace written afterwards (events.jsonl, blobs, shell output, and the run CA under tls/) goes through the junction and is readable by that account, and dropping its directory makes the store disappear.
Evidence in files this change did not touch: packages/core/src/private-files.ts:62-64 states the omission as fact (".orca is not restricted — only .orca/runs is"), and SECURITY.md's new bullet promises the store root "grants only its owner, SYSTEM and Administrators" without saying that the parent it stands in does not. Note writeConfig gets this right — it narrows the container (~/.config/orca) as well as the file — so the pattern is already in the change.
Fix: narrow the container as well, unconditionally on Windows (like .orca/runs, since a store predating the fix is exactly the case this series keeps repairing):
if (created || process.platform === 'win32') await restrictToOwner(dir, 0o700);
if (process.platform === 'win32') await restrictToOwner(orcaDir(cwd), 0o700);
restrictToOwner grants (OI)(CI) to owner/SYSTEM/Administrators, so .orca/runs and .gitignore created afterwards inherit it, and POSIX behaviour is unchanged.
There was a problem hiding this comment.
Confirmed and fixed in 66555f9 — and reproducing it turned up a sharper mechanism than the finding describes.
icacls does not follow the reparse point. Measured, with a junction planted where .orca/runs was:
mkdir(runs, { recursive: true }) -> undefined (accepted as an existing directory)
restrictToOwner(runs) -> succeeds
junction itself -> D:PAI(A;OICI;FA;;;BA)(A;OICI;FA;;;SY)(A;OICI;FA;;;<owner>)
what it points at -> unchanged, still (A;ID;…;AU)(A;ID;…;BU)
events.jsonl written through it -> (A;ID;0x1301bf;;;AU)(A;ID;0x1200a9;;;BU)
That is worse rather than better than "the ACL lands on the attacker directory": the narrowing lands on the junction, the trace lands in someone else's directory under their ACL, and restrictToOwner reports success the whole way. Nothing anywhere says the protection missed.
Fixed as you suggest, unconditionally on Windows. The framing I took from your note is the one that generalises: orca creates .orca, so orca owns its protection — the same reason writeConfig narrows ~/.config/orca and not only the file in it, which you rightly pointed out was already the pattern in the change.
Also removed the comment in private-files.ts that stated the omission as fact. It was load-bearing in the wrong direction: it recorded the gap instead of closing it.
Test, red against the reverted implementation: after ensureRunsDir, neither .orca nor .orca/runs carries an inherited entry — on a fresh workspace and on a store that already existed, since a store predating this is exactly the case this series keeps repairing.
| */ | ||
| function system32(exe: string): string { | ||
| const root = process.env['SystemRoot']; | ||
| if (root === undefined || root === '') { |
There was a problem hiding this comment.
🟠 P1 system32() trusts SystemRoot without checking it is absolute, so a relative value reintroduces the workspace-relative icacls lookup
The function's own doc comment says a lookup that lands on someone else's icacls.exe — "in the workspace being recorded, say" — "would hand that key to exactly the reader the call exists to shut out", and it correctly identifies why 'C:\Windows' cannot be used as a fallback: the literal's value is C:Windows, a drive-relative path, and "CreateProcess resolves it against the current directory on C:, which during a recording is the workspace", where Windows/System32/icacls.exe is a storable git path.
The implementation removed the hardcoded literal but kept the same hole: it only checks that SystemRoot is present, never that it is absolute. join(root, 'System32', exe) with SystemRoot=C:Windows (or any relative value, e.g. Windows) produces exactly that cwd-relative path again, so a workspace (or a direnv/.env wrapper that sets the variable for the orca process) can plant Windows/System32/icacls.exe. A planted binary can exit 0 without touching the ACL; execFile then resolves, the key/ACL code carries on, and the run CA private key plus the gateway API key are written under the workspace's inherited ACL — readable by every local account, with no error anywhere. That is the same silent non-narrowing the surrounding comments say absolute paths exist to prevent, so the env var is trusted where the sibling PATH is not.
Fix: validate the value, not just its presence. import { isAbsolute } from 'node:path'; and then if (root === undefined || root === '' || !isAbsolute(root)) throw new Error(...). On win32 isAbsolute('C:Windows') is false and isAbsolute('C:\\Windows') is true, so a drive-relative or workspace-relative SystemRoot is refused in the same way an absent one is.
There was a problem hiding this comment.
Confirmed and fixed in 66555f9. This one is squarely fair: the comment above system32 spends a paragraph on why C:Windows is dangerous, and then the code accepted exactly that value from the environment. Removing the literal closed the spelling and left the shape.
isAbsolute separates them exactly, as you say — measured on win32:
"C:\Windows" -> true
"C:Windows" -> false
"Windows" -> false
"" -> false
So a relative SystemRoot is now refused the same way an absent one is, and the message says which it was:
cannot locate icacls.exe: SystemRoot is not an absolute path ("C:Windows"), so there is no
trustworthy path to it. orca will not fall back to a guess here — this call is what keeps a
private key private.
Test extended rather than added: the existing "refuses rather than guessing where icacls lives" case now also sets C:Windows and Windows and expects /not an absolute path/. Red against the presence-only check.
…SystemRoot to be absolute
Review round five. Two findings, both correct; the first has a sharper mechanism
than the finding describes.
**`.orca` was left with the workspace's ACL.** Only `.orca/runs` was narrowed,
and this file's own comment said so as though it were a detail. Modify on the
parent is enough to delete `runs` — a child's own DACL does not decide whether
its parent may remove it — and to leave a junction in its place, which needs no
privilege on Windows. From there, measured:
mkdir(runs, { recursive: true }) -> undefined (the junction is an existing directory)
restrictToOwner(runs) -> succeeds
junction itself -> D:PAI(…BA)(…SY)(…owner)
what it points at -> unchanged, still (…;AU)(…;BU)
events.jsonl written through it -> (A;ID;0x1301bf;;;AU)(A;ID;0x1200a9;;;BU)
So `icacls` does *not* follow the reparse point, which is worse rather than
better than the review supposed: the narrowing lands on the junction, the trace
lands in someone else's directory under their ACL, and `restrictToOwner` reports
success the whole way. `ensureRunsDir` narrows `.orca` too now — orca creates
it, so orca owns its protection, the same way `writeConfig` narrows
`~/.config/orca` and not just the file in it.
**`SystemRoot` was trusted for being present, not for being absolute.** The
comment above `system32` explains at length why `'C:\Windows'` cannot be a
fallback — the literal's value is `C:Windows`, which CreateProcess resolves
against the current directory on C:, which during a recording is the workspace —
and then the code accepted exactly that value from the environment. A wrapper
that sets the variable is an ordinary thing to run into, and a planted
`Windows/System32/icacls.exe` that exits 0 leaves every ACL unapplied with
nothing reported. `isAbsolute` separates them exactly: false for `C:Windows` and
`Windows`, true for `C:\Windows`.
Both tested, both red against the reverted implementation: `.orca` carries no
inherited entry after `ensureRunsDir`, on a fresh store and on one that already
existed; and `restrictToOwner` rejects a relative `SystemRoot` with the same
refusal as a missing one.
Windows: 10 failures on main, 9 with this branch, no new ones.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 4 issues in this PR: 🟠 4 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 535 calls · 56.7M tokens · 99% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
| await ensureRunsDir(target); | ||
| await cp(join(ASSET, 'project'), target, { recursive: true }); | ||
| const runDir = join(target, '.orca', 'runs', runId); | ||
| await cp(join(ASSET, 'trace'), runDir, { recursive: true }); |
There was a problem hiding this comment.
🟠 P1 quickstart installs its run with the package's file modes, unlike every other command that puts a run in the store
The change routes quickstart through ensureRunsDir(target) and its own comment/test assert the store is now made "the way every other command does" — but ensureRunsDir only fixes the store root; the run itself is then installed by cp, and Node's cp reproduces the source's permissions (verified locally with node 22: a 0644 source file lands 0644, a 0755 source directory lands 0755).
The siblings that put a run into the store all apply the store's modes to what they install:
record/attach/replaywrite it:TraceWriter.createdoesmkdir(runDir, {mode: DIR_MODE})+writeFile(..., {mode: FILE_MODE})+BlobStore.put(created 0600/0700), i.e. run dir 0700, files 0600.pull— the other command that installs a run rather than writing it — chmods every entry it writes, and its comment says why: "a pulled run is exactly as sensitive as a recorded one ... the explicit chmod after the fact is what actually makes the promise true" (sync.ts:831,:849,:854).
quickstart has no such step. Reproduced here with the real assets (node cp of packages/cli/quickstart/trace into .orca/runs/<runId> after ensureRunsDir):
700 /.orca/runs
755 /.orca/runs/run_abc123
644 /.orca/runs/run_abc123/events.jsonl
644 /.orca/runs/run_abc123/manifest.json
644 /.orca/runs/run_abc123/blobs/f7/f7c1592…a118
644 /.orca/runs/run_abc123/fs/objects/1b/1f956f3…ece
(git stores every one of those assets as 100644; git ls-tree -r HEAD packages/cli/quickstart.)
So on any POSIX machine the directory orca quickstart is documented to produce is 0755 and its recording is 0644 — the recording is readable by every account on the machine, which is the opposite of the promise this change edits into SECURITY.md at that very spot ("Trace files and blobs are written mode 0600, run directories 0700") and of the claim in the new test ('creates the store the way every other command does'). The new test cannot see it: on POSIX its helper returns immediately (if (process.platform !== 'win32') return;) and only the .gitignore is asserted.
Fix: apply the store's modes to what is installed, the way pull does — either chmod the copied tree (chmod(runDir, 0o700) and every file beneath it to 0o600, or a recursive walk after the cp), or copy the archive through a writer that sets the modes (as stageRunEntries does), and add a POSIX assertion on the run directory/file modes to the new test.
There was a problem hiding this comment.
Confirmed and fixed in b2f94e5. Checked the premise first: git ls-tree -r HEAD packages/cli/quickstart is 100644 for all 23 assets, so cp reproduces 0644 and the run lands under a 0755 directory. The new test could not see it, exactly as you say — its helper returns early on POSIX.
Fixed the way you point at, and for the reason you name: pull is the other command that installs a run rather than writing it, and it chmods every entry. quickstart now walks the copied tree and applies 0700/0600 after the cp. On Windows the modes are discarded and the tree already inherits the ACL ensureRunsDir put on .orca/runs, so this is the POSIX half of the same promise — which is the half the test was missing, and the test now asserts the run directory and its files on POSIX.
| } catch (err) { | ||
| await rm(staging, { force: true }).catch(() => undefined); | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
🟠 P1 Give writeConfig's staging file a per-writer name: two writers share ${path}.incoming and one can install the empty file the other just truncated
The new staging path is derived only from the destination (${path}.incoming, i.e. <config dir>/config.json.incoming), so every writer of that config addresses the same scratch file and nothing serialises them: no lock, and no uniqueness. That is a departure from the repo's own convention for write-elsewhere-then-rename — BlobStore.put (core/src/blobs.ts:57) and scrub's commit (cli/src/commands/scrub.ts:356) both use a randomised temp name, and sync uses the deterministic <dest>.incoming only while holding withRunLock (cli/src/commands/sync.ts:504), with a recovery pass for exactly the mess a shared scratch name makes.
Two overlapping writers (two orca setup/setupCommand invocations against the same config dir, or any two callers of writeConfig) interleave as follows:
A: writeFile(staging, '') -> staging is 0 bytes
A: restrictToOwner(staging)
A: writeFile(staging, ) -> staging holds A's config
B: writeFile(staging, '') -> B truncates the file A is about to install
A: rename(staging, path) -> config.json is now EMPTY
B: restrictToOwner(staging) -> ENOENT, the file was renamed away
B: catch -> rm(staging) (no-op) -> throws
The result is a 0-byte ~/.config/orca/config.json installed over the user's existing one, while A reports config.saved success. readConfig then returns {} (JSON.parse of '' throws, and that is deliberately swallowed), so the gateway URL and the stored API key are silently gone: orca compare --models … no longer uses the configured gateway, orca push/pull say "no gateway configured", and the credential has to be obtained again. The mirror interleaving has the same outcome with the writers swapped.
The same shared name also breaks the failure path the comment explicitly promises ("a failure leaves the previous config untouched"): if B fails any of its three steps, line 131 removes the staging file — which may by then be A's, so A's rename fails with ENOENT and A's update is lost with an error naming a file neither writer owns.
Concrete fix — make the staging name belong to one writer:
import { randomBytes } from 'node:crypto';
const staging = ${path}.${randomBytes(6).toString('hex')}.incoming;
Then the truncate/create, the narrowing and the rename all address a file no other writer can name, and the rm in the catch can only ever remove its own staging file. (Serialising with a lock, as sync does, would work too but is heavier for one small file.)
There was a problem hiding this comment.
Confirmed and fixed in b2f94e5 with the randomised name, and the repo convention you cite is the right argument — BlobStore.put and scrub's commit both do it, and sync uses the deterministic name only under withRunLock.
One thing worth recording, because it changes what the accompanying test can honestly claim: the interleaving does not reproduce on Windows. I restored the shared name and ran 30 concurrent writers over 8 rounds — no empty config, no corrupt one. Windows refuses the concurrent open rather than truncating, so the shared name produces loud failures there (EPERM on the rename, which is what the test surfaced first) instead of silent corruption. Your sequence is a POSIX shape.
So the fix is by convention rather than by a test that happens to catch it, and the test says that rather than implying it reproduced something. What it does guard either way: whatever ends up installed parses, and no scratch file is left behind to be mistaken for a config.
Also worth naming the trade, since it is not free: writing straight to the destination never failed on a concurrent write — it interleaved two configs into one file in silence. Now a losing writer fails loudly and leaves the previous config intact.
| } | ||
| // (type;flags;rights;object;inherit_object;trustee) — `ID` in the flags marks it inherited. | ||
| const aces = [...descriptor.matchAll(/\(.;([^;]*);[^;]*;[^;]*;[^;]*;([^)]+)\)/g)]; | ||
| return [...new Set(aces.filter((ace) => !ace[1]!.includes('ID')).map((ace) => ace[2]!))]; |
There was a problem hiding this comment.
🟠 P1 Match every ACE type, or the narrowing silently leaves a foreign grant in place
explicitTrustees decides which trustees to name in /remove, and the regex it uses is \(.;([^;]*);... — a single character for the ACE type. SDDL ACE types are not all one character: conditional/callback ACEs are XA/XD (and ZA/ZD), object ACEs are OA/OD, and audits are AU/AL. Any ACE whose type is more than one character is not matched at all (verified with node: descriptor D:PAI(A;OICI;FA;;;BA)(XA;OICI;FA;;;S-1-5-21-1-2-3-1001;(WIN://SYS/APPID)) yields captured trustees ["BA"] only), and the same happens to the trustee capture for any ACE that carries a condition, because [^)]+ stops at the first ).
The guard two lines above does not notice: it only checks that the descriptor starts with O:/G:/D:/S: and contains a (, so a descriptor with ACEs the parser cannot read is still read as "these are all the trustees". The function's own comment at lines 63–67 states the consequence of missing one — /inheritance:r removes only inherited ACEs and /grant:r replaces explicit permissions only for the trustees it names, so "an explicit ACE for anybody else survives both" — and that is exactly what happens: foreign comes back short, no /remove is issued for that trustee, icacls exits 0, and restrictToOwner reports success while the third party keeps the access its ACE grants. Where the path is <run>/tls (whole directory, with (OI)(CI)), ~/.config/orca, .orca or .orca/runs, that means the run CA's private key, the stored gateway key and the whole trace store remain fully readable/writable by the account named in the skipped ACE — the leak this whole commit series exists to close.
Fix: read the type as a field rather than a character, e.g. /\(([^;]*);([^;]*);[^;]*;[^;]*;[^;]*;([^;)]+)/g (verified to capture the conditional ACE's trustee as well), and keep the ace[2].includes('ID') filter on the flags group. More importantly, make the result a check that can only fail closed: after the icacls call, re-read the DACL (icacls <path> /save again is already a code path in this file) and throw unless the only trustees left are keep; anything the parser did not understand then aborts the write instead of reporting a security property that is not there.
There was a problem hiding this comment.
Confirmed and fixed in b2f94e5. Reproduced against your descriptor before changing anything:
D:PAI(A;OICI;FA;;;BA)(XA;OICI;FA;;;S-1-5-21-1-2-3-1001;(WIN://SYS/APPID))(OA;;CCDC;bf94;;S-1-5-32-545)
old pattern -> ["BA"]
new pattern -> ["BA","S-1-5-21-1-2-3-1001","S-1-5-32-545"]
so two foreign trustees would have kept their access with icacls exiting 0 and restrictToOwner resolving.
The type is read as a field now, and the trustee stops at ; as well as ). But the more useful half of your finding is the second paragraph, and I took that too: restrictToOwner reads the DACL back and checks it — deliberately not through the parser, which this file has now been wrong about twice. It matches the whole descriptor, anchored, against the shape the function writes:
const asWritten = new RegExp("^D:[A-Z]*P[A-Z]*(?:\(A;[A-Z]*;[^;]*;;;(?:" + [...ours].join("|") + ")\))+$");An ACE of a type nobody anticipated fails that rather than being skipped by it, which is the property a parser-based check could not give. Nothing is escaped into the pattern because a trustee here is a SID or a two-letter abbreviation, both [A-Za-z0-9-].
| // and `icacls` does not follow a reparse point, so the narrowing lands on the junction while | ||
| // every trace is written *through* it into whatever it points at, under that directory's ACL. | ||
| // `restrictToOwner` reports success the whole way. | ||
| if (process.platform === 'win32') await restrictToOwner(orcaDir(cwd), 0o700); |
There was a problem hiding this comment.
🟠 P1 Refuse a store whose .orca/runs is already a reparse point, not narrow the link
The comment immediately above line 71 records the mechanism precisely: icacls does not follow a reparse point, mkdir(dir, { recursive: true }) accepts a junction as an existing directory, restrictToOwner reports success, and "every trace is written through it into whatever it points at, under that directory's ACL". The code then narrows orcaDir(cwd) — which only stops an attacker from replacing runs with a junction going forward. It does nothing about a link that is already there, and that is stated to be the case being repaired ("every install predating this, every store orca quickstart laid down, and any directory whose creation raced or half-failed").
Concretely, on Windows: an attacker with Modify on the workspace (the same assumption the comment makes, and no extra privilege is needed to create a junction) pre-creates .orca as a junction into a directory they own before orca's first run there, or replaces .orca/runs with one at any point while .orca is still broad. Then ensureRunsDir runs mkdir('.orca/runs', { recursive: true }) (returns undefined — the link reads as an existing directory), narrows the link entry itself with restrictToOwner(dir, 0o700) at line 61 and the link (or the attacker-owned parent) at line 71, and reports success. Every run directory, blob, events.jsonl, pulled run and <run>/tls/ca.key written afterwards is created inside the attacker's directory and inherits its ACL — so the attacker can read the recordings and the run CA's private key, and because they own the enclosing directory they keep delete rights over the narrowed entries. This is the junction case the round-five commit claims to close, still open for any link already in place.
Fix: on Windows, resolve before restricting and refuse where the two disagree — e.g. const real = await realpath(dir).catch(() => undefined); if (real !== undefined && resolve(real) !== resolve(dir)) throw new Error(...) (and the same for orcaDir(cwd)), so a store that is a reparse point aborts the command loudly instead of returning a store whose protection landed on a link. Narrowing the resolved target instead is the alternative if refusing is considered too strict.
There was a problem hiding this comment.
Confirmed and fixed in b2f94e5. You are right that round five only closed the door going forward — and the comment I wrote there described the mechanism while leaving the case it described open, which is the second time in this PR a comment of mine recorded a gap instead of closing it.
ensureRunsDir now refuses a reparse point at either .orca or .orca/runs, via realpath + resolve as you suggest. Measured, with a junction planted at each in turn:
.orca/runs is a junction -> refused: "…\.orca\runs is a link to …\attacker"
.orca is a junction -> refused: "…\.orca is a link to …\attacker"
I took refusing over narrowing the target. Narrowing it would leave the attacker owning the directory, and an owner keeps WRITE_DAC — so they re-grant themselves whenever they like, and orca would have reported success. Refusing says the one true thing: orca creates both directories, so a link standing where one should be was not put there by orca, and a store it cannot vouch for is not one to quietly write a recording into.
Windows only, deliberately. On POSIX chmod follows a symlink already, and pointing a store at another disk is an ordinary thing to do — refusing there would break a working setup to close an attack that needs the Windows reparse-point semantics.
…rusted alone
Review round six. All four hold; two were verified by reproducing them and two
by reading what the tools actually emit.
**quickstart installed its run with the package's modes.** `ensureRunsDir` fixes
the store root; the run is then `cp`'d in, and `cp` reproduces the source's
permissions. Every shipped asset is `100644` in git (23 of 23), so the recording
landed 0644 under a 0755 directory — in the store SECURITY.md calls 0600/0700,
and under a test that asserted "the way every other command does" while
returning early on POSIX. `pull` is the other command that *installs* a run
rather than writing it, and it chmods every entry for exactly this reason;
quickstart now does the same.
**The staging file was named after its destination.** `${path}.incoming` is one
scratch path every writer addresses with nothing serialising them, so B's
truncate could land on the file A was about to rename into place. `BlobStore.put`
and scrub's `commit` both randomise; sync uses the deterministic name only
under a lock. Randomised here too. Worth recording what measuring it showed:
the interleaving does not reproduce on Windows — thirty writers over eight
rounds produced no empty and no corrupt config, because Windows refuses the
concurrent open rather than truncating. It is a POSIX shape, which is the reason
to fix it by convention rather than by a test that happens to catch it, and the
test says so instead of implying otherwise.
**A single-character ACE type matched none of the two-character ones.** `XA`/`XD`
are conditional ACEs, `OA`/`OD` object ones, `AU`/`AL` audits — and `[^)]+` for
the trustee stops at the first `)`, which a conditional ACE reaches inside its
own expression. Verified: on `D:PAI(A;OICI;FA;;;BA)(XA;OICI;FA;;;S-1-5-21-1-2-3-1001;(WIN://SYS/APPID))(OA;;CCDC;bf94;;S-1-5-32-545)`
the old pattern captured `["BA"]` and nothing else, so two foreign trustees would
have kept their access with `icacls` exiting 0.
The type is a field now. But the more useful half of that finding is the part
about failing closed, so `restrictToOwner` reads the DACL back and matches the
whole descriptor, anchored, against the shape it writes. Deliberately not
through the parser, which this file has now been wrong about twice: an ACE of a
type nobody anticipated fails the check rather than being skipped by it.
**A junction already in place was still followed.** Narrowing `.orca` stops one
being put there from now on and says nothing about one already there — which is
the population the whole change is for. `ensureRunsDir` refuses a reparse point
at either `.orca` or `.orca/runs` on Windows, since orca creates both and a link
standing where one should be was not put there by orca. POSIX keeps its
behaviour, where `chmod` follows a symlink and putting a store on another disk
is an ordinary thing to do.
Windows: 10 failures on main, 9 with this branch, no new ones.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 2 issues in this PR: 🟠 2 P1.
Not attached to a line. GitHub only accepts an inline comment on a line this PR changes. The findings below point somewhere else, so they are listed here instead of being dropped.
packages/shell-shim/src/install.ts (line 70): 🟠 P1 Apply the new ACL to the shell-frames / agent-spans transports, whose only protection is the mode bits this change proved meaningless on Windows
The change establishes, in packages/core/src/private-files.ts and SECURITY.md, that mode:/chmod do nothing on Windows ("the file really gets … the ACL it inherits from wherever it was written") and adds restrictToOwner for the three sinks it audited: the store root, the CA files and config.json. The shell-shim frames transport is the fourth, and it was left on the mechanism the change just refuted.
await mkdtemp(join(tmpdir(), 'orca-shell-')) discards no mode on Windows — CreateDirectory with no security descriptor, so the directory carries %TEMP%'s inherited ACL — and writeFile(framesPath, '', { flag: 'a', mode: 0o600 }) (line 108) does the same for shell-frames.jsonl. That file is the one this package's own doc comment calls "the most secret-dense text in a run": argv and cwd verbatim, i.e. curl -H 'Authorization: Bearer …', git clone https://user:token@host/…, an inline AWS_SECRET_ACCESS_KEY=…. On a stock Windows profile %TEMP% is user-private, so this is latent there; where TEMP is redirected or machine-wide (managed images, CI agents that set TEMP=C:\Temp, the C:\Windows\Temp fallback when a profile has no temp), any account the ACL admits can read the file, and the recording's own credential-bearing commands with it. packages/cli/src/agent-spans.ts:271 is the same shape, with the comment at line 261 asserting the now-refuted premise outright ("mkdtemp gives a directory only this user can enter"), and owner.pid/spansPath written with mode: 0o600 that Windows discards. SECURITY.md's neighbouring bullet (lines 38-45, untouched by this change) still promises "a private temporary directory" for exactly these two files.
Fix: after the mkdtemp, await restrictToOwner(transportDir, 0o700) — the CLI already holds the directory (shell.transportDir / agentSpans.transportDir, used at packages/cli/src/commands/record.ts:226 and :242), so it can be done there without a new dependency, or @orcareplay/shell-shim can take @orcareplay/core. Also correct SECURITY.md's sentence, since it is the claim that now depends on an environment variable nobody chose.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 551 calls · 66.2M tokens · 99% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
| if (process.platform === 'win32') { | ||
| for (const path of [orcaDir(cwd), dir]) { | ||
| const real = await realpath(path).catch(() => undefined); | ||
| if (real !== undefined && resolve(real) !== resolve(path)) { |
There was a problem hiding this comment.
🟠 P1 Stop skipping the reparse-point check when realpath fails for any reason other than absence
The reparse-point check was added to refuse a store that is (or is reached through) a link, because icacls does not follow a reparse point: the narrowing lands on the link entry while every trace is written through it into a directory whose ACL orca does not control. The check is expressed as an error handler that swallows every failure of realpath, treating "I could not resolve this path" as "this path is not a link":
const real = await realpath(path).catch(() => undefined);
if (real !== undefined && resolve(real) !== resolve(path)) { throw new Error(`${path} is a link to …`); }realpath on Windows fails for reasons other than "no such path": EACCES/EPERM, EINVAL/ENOTSUP/UNKNOWN on filesystems that cannot answer GetFinalPathNameByHandle (some SMB shares, substituted drives, network redirections), ELOOP, ENAMETOOLONG, EIO. In each of those the resolved value is undefined, the branch is skipped, and ensureRunsDir proceeds to restrictToOwner(dir, 0o700) and restrictToOwner(orcaDir(cwd), 0o700) — i.e. the check commits exactly the failure it exists to prevent, and reports success while it does. The comment directly above the code ("Refused rather than followed: … a store it cannot vouch for is not one it should quietly write a recording into") states the opposite posture, and every other unreadable/ambiguous input in this file fails closed (descriptorOf throws on an unrecognised descriptor rather than reading it as "nothing to remove"; system32 refuses rather than guessing). A check that cannot complete must not resolve to "carry on".
Consequence if it does happen: with a junction at .orca or .orca/runs (which needs no privilege to create) the store's ACL is applied to the link, the recording is written through it into the linked directory, and the trace — source, shell output, workspace snapshots, secrets — is readable by whoever that directory's ACL allows, which is precisely the state commit 66555f9 fixed.
Fix: fail closed — only the "not there yet" case may be skipped, everything else must refuse (or the check must not depend on a call that can fail):
const real = await realpath(path).catch((err: NodeJS.ErrnoException) => {
if (err.code === 'ENOENT') return undefined; // arguably unreachable: mkdir above created both
throw new Error(
`${path} could not be resolved (${err.code ?? err.message}), and orca will not write a ` +
'trace store through a path whose target it cannot verify',
);
});If a realpath failure must not stop a recording on an exotic filesystem, the equivalent test is lstat(path) and refusing when the entry itself is a reparse point — an entry-level answer that does not depend on resolving the whole chain.
There was a problem hiding this comment.
Confirmed and fixed in 1201791 — and verifying it turned up a second defect in the same six lines.
You are right that the catch-all fails open, and right that it is the one posture this file does not otherwise take. But realpath was also answering a different question than the one being asked. Where does this path end up is not is this path a link, and a drive that is not a link ends up somewhere else all the same. Measured:
subst Y: D:\work Y:\ws\.orca\runs -> D:\work\ws\.orca\runs refused
8.3 short name D:\PROJEC~1\... -> D:\project_mxz\... refused
junction ...\.orca\runs -> ...\attacker refused
Two of those three are ordinary workspaces — a subst drive is a common way to shorten a path on Windows, and a mapped network drive is the same shape — and orca would not record in them at all. So the check had both failure directions at once: it skipped itself when it could not resolve, and refused when it resolved correctly.
Your second suggestion is the one I took, for exactly the reason you give: lstat is an entry-level answer that does not depend on resolving the chain. It says no link for the subst drive and the short name, yes for the junction, and a failure to answer is a real failure — refused like any other, which closes the fail-open half:
const entry = await lstat(path).catch((err: unknown) => {
throw new Error(`${path} could not be examined (${(err as NodeJS.ErrnoException).code ?? String(err)}), and orca will not write a trace store into a path it cannot vouch for.`);
});
if (entry.isSymbolicLink()) throw new Error(`${path} is a link, …`);All four cases checked end to end through ensureRunsDir: plain allowed, junction at .orca refused, junction at .orca/runs refused, subst allowed.
The error message lost its to <target> half, because lstat deliberately does not resolve one — the existing test asserted on that wording and is updated, with a comment saying why the wording changed rather than just changing it.
No regression test for the false positive: reproducing it needs a drive letter or an 8.3 name, neither of which belongs in a test suite. The junction test guards the security half, and the comment now says why realpath is the wrong call, which is the thing that actually stops it coming back.
…ere never narrowed
Review round seven. Both hold, and verifying the first turned up a second defect
in the same six lines that the review did not mention.
**The link check refused the wrong things and skipped itself on the rest.**
`realpath(path).catch(() => undefined)` read "I could not resolve this" as
"this is not a link", so an EACCES, an ELOOP, or the EINVAL/UNKNOWN that some
SMB shares and redirected drives answer with left the check un-run and
`ensureRunsDir` carrying on into exactly the state it exists to refuse — while
every other unreadable input in this series fails closed.
And `realpath` was answering a different question than the one being asked.
*Where does this path end up* is not *is this path a link*, and a drive that is
not a link ends up somewhere else all the same. Measured:
subst Y: D:\work Y:\ws\.orca\runs -> D:\work\ws\.orca\runs refused
8.3 short name D:\PROJEC~1\... -> D:\project_mxz\... refused
junction ...\.orca\runs -> ...\attacker refused
Two of those three are ordinary workspaces, and orca would not record in them
at all. A mapped network drive is the same shape.
`lstat` asks about the entry that is right here. It says no link for the subst
drive and the short name, yes for the junction, and a failure to answer is a
real failure, refused like any other. All four cases checked end to end through
`ensureRunsDir`: plain allowed, junction at `.orca` refused, junction at
`.orca/runs` refused, subst allowed.
**The shell and agent-span transports were still on the mechanism this series
refuted.** `mkdtemp` plus `mode: 0o600`, which is 0700 on POSIX and, on Windows,
whatever `%TEMP%` hands down. Not hypothetical here — measured on this machine
before the change:
shell-frames.jsonl CodexSandboxUsers:(I)(M) S-1-5-21-…-3900188003:(I)(M)
with Modify, on the file this package's own comment calls the most secret-dense
text in a run: argv and cwd verbatim, so a `curl -H "Authorization: …"` or a
`git clone https://user:token@host/…` is in it in full. `agent-spans.ts` had the
refuted premise written down as fact — "`mkdtemp` gives a directory only this
user can enter".
Both are narrowed the moment the directory is made and before a byte goes into
it, because `icacls` does not re-propagate: narrowing after `owner.pid` and the
frames file exist would leave both holding what they inherited. A failure there
fails the install, and the caller already degrades to no capture — which is the
right way round, since not capturing beats writing every command the agent runs
somewhere this cannot vouch for.
`@orcareplay/shell-shim` takes `@orcareplay/core` for it. `runner-bin.ts`
imports only `./runner.js`, so the shim child's runtime graph is unchanged.
Verified through a real `orca record --tls-intercept` with shell capture on:
the store, run directory, `events.jsonl` and `fs/` are owner-only by
inheritance, the CA files are, `config.json` is, both transports are — and
shell capture still records what it is for, `argv` intact and `exit_code: 3`.
Push and pull to the live gateway both work, and a store created by `pull` comes
out owner-only too.
No regression test for the subst/short-name false positive: reproducing it needs
a drive letter or an 8.3 name, neither of which belongs in a test. The junction
test guards the security half, and the comment says why `realpath` is the wrong
call so it does not come back.
Windows: 10 failures on main, 9 with this branch, no new ones.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
xizhuomengcontin
left a comment
xizhuomengcontin
left a comment
There was a problem hiding this comment.
Confirmed and fixed in 1201791. Not latent on this machine — measured before changing anything:
shell-frames.jsonl
DESKTOP-…\CodexSandboxUsers:(I)(M)
S-1-5-21-…-3900188003:(I)(M)
SYSTEM:(I)(F) Administrators:(I)(F) Dotc:(I)(F)
A local group and a second account, both with Modify, on the file your finding correctly names as the most secret-dense text in a run. Your reading of %TEMP% being user-private on a stock profile is the optimistic case; this is a developer box with no special setup.
Both transports are narrowed now — and narrowed at mkdtemp, before a byte goes in, rather than at the call site where the CLI holds them. That ordering is the part worth naming: installShellShim writes owner.pid and an empty frames file immediately, and icacls does not re-propagate to children that already exist, so a directory narrowed afterwards would leave both of them holding what they inherited. Verified by asserting the files too, which now come out (I) of an owner-only parent.
A failure there fails the install, and both call sites already degrade to "this layer is unavailable" — which is the right way round: not capturing beats writing every command the agent runs into a directory orca cannot vouch for. The directory is removed on that path rather than left behind.
@orcareplay/shell-shim takes @orcareplay/core for it. I checked the concern that implies: runner-bin.ts imports only ./runner.js, never ./install.ts, so the shim child that gets exec'd on every shell command has an unchanged runtime graph.
SECURITY.md corrected as you say — the sentence now states what "private" means on each platform instead of assuming one of them.
Tests on both, each red against the reverted implementation. And verified end to end through a real orca record --tls-intercept with shell capture on: transports owner-only, and shell capture still records what it is for — argv intact, exit_code: 3.
|
@orcacode-review |
1 similar comment
|
@orcacode-review |
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 838 calls · 107.8M tokens · 99% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
| // and `icacls` does not follow a reparse point, so the narrowing lands on the junction while | ||
| // every trace is written *through* it into whatever it points at, under that directory's ACL. | ||
| // `restrictToOwner` reports success the whole way. | ||
| if (process.platform === 'win32') await restrictToOwner(orcaDir(cwd), 0o700); |
There was a problem hiding this comment.
🟠 P1 Narrow .orca before runs, and re-check the entry after the parent is closed — the link check is stranded between mkdir and the ACL, so a junction swapped in during that window is still followed
The link check is a check-then-use across three separate syscall sequences, and the ordering leaves the guarded window open at both ends.
What the code does on win32: (1) mkdir(dir, {recursive:true}) — line 60, which already follows a junction at .orca/.orca/runs; (2) lstat both entries and refuse a link — lines 90–103; (3) restrictToOwner(dir) — line 105; (4) restrictToOwner(orcaDir(cwd)) — line 115, i.e. the parent is narrowed last; (5) .gitignore. Nothing re-examines either entry after step (2).
Why the window is wide and reachable: restrictToOwner gets to its one modifying call only after a SID lookup and a full descriptor read (private-files.ts:40-49) — whoami.exe and icacls /save are separate child processes with a mkdtemp/readFile between them, so the gap between the lstat at line 90 and the DACL write for that same path is several process spawns, not a single instruction. Throughout that gap, and until line 115 has finished, .orca still carries the ACL the workspace handed down — which the change's own doc comment says is NT AUTHORITY\Authenticated Users:(I)(M), i.e. Modify, and the comment at paths.ts:109-110 states that Modify on .orca is enough to delete runs and leave a junction in its place ("a child's own DACL does not decide whether its parent may delete it"). So another local account on the machine can, in a loop, rmdir .orca\runs (or .orca itself) and mklink /J it at anything it can read; whenever the swap lands in the window, the check has already passed and the narrowing at line 105/115 lands on the link entry — icacls does not follow a reparse point — while every trace, blob, tls/ca.key and fs/ object for the whole run is written through it into the attacker's directory, under that directory's ACL. restrictToOwner's read-back verification (private-files.ts:83-91) inspects the same link entry, so it matches and reports success, exactly the silent-success failure mode the comments at paths.ts:63-68 and 108-114 describe as the thing being fixed. Node's own test only covers a junction that is already there before the call.
Consequence: the recorded run — source, shell output, workspace snapshots, and the run CA private key that lets its holder impersonate every intercepted host to the agent — ends up readable/writable by the account that planted the junction, with orca reporting a clean, protected recording. This is the same data the whole change exists to keep owner-only.
Fix (either half, ideally both):
- Close the parent first, then check the child under it:
mkdir(orcaDir(cwd), {recursive:true, mode:0o700})→lstat.orca, refuse a link →restrictToOwner(orcaDir(cwd), 0o700)→mkdir(dir, {recursive:true, mode:0o700})→lstatdiragain, refuse a link →restrictToOwner(dir, 0o700). Once.orcagrants only owner/SYSTEM/Administrators, no third party can delete or replace.orca/runs, so the second check-then-use is genuinely race-free; the current order does the reverse, sorunsis protected and never re-verified while the only thing that decides whether it can be swapped is still wide open. - Or make check and use one operation: open the directory with
FILE_FLAG_OPEN_REPARSE_POINT(refusing a reparse point on the handle fromGetFileInformationByHandle/FileAttributeTagInfo) and apply the DACL by handle, so no path is re-resolved between the check and the narrowing.
There was a problem hiding this comment.
Confirmed and fixed in 7f8cc4f, taking option 1. The ordering was the whole of it, and the window is as wide as you say — measured: restrictToOwner costs ~27ms a path here (a whoami spawn, an icacls /save read, the write, and a second /save to verify), so the gap between checking runs and owning it ran past 50ms with .orca still granting the workspace's Modify.
Reordered exactly as you set it out:
mkdir(.orca) -> refuseLink(.orca) -> restrictToOwner(.orca)
mkdir(runs) -> refuseLink(.orca) -> refuseLink(runs) -> restrictToOwner(runs)
Once the container grants nobody else anything, nothing can delete or replace what sits under it, so the second check-then-use is not a race — that is the part of your fix that does the work.
The extra refuseLink(.orca) after mkdir(runs) is for the half your fix does not close and I did not want to leave implied: .orca was itself open between its own check and its own narrowing. Re-checking does not close that window — option 2 would, and node cannot open a directory with FILE_FLAG_OPEN_REPARSE_POINT or apply a DACL by handle — but it turns a swap that lands there into a refusal rather than a recording written somewhere else. The comment says that rather than claiming the race is gone.
POSIX moved into its own branch instead of riding a condition, since it shares none of this: create-only, no link check, because chmod follows a symlink and pointing a store at another disk is an ordinary thing to do.
Verified end to end before committing rather than only by the suite: fresh workspace, a store that already existed, a junction at .orca and at .orca/runs (both refused), a subst drive (still records — that was the earlier false refusal), four repeat calls, a real orca record --tls-intercept (shell capture recorded exit_code: 3, model 200, every path in the store at three trustees), and orca quickstart (3/3 turns replayed, store at three trustees).
New test: a container that is a link is refused before anything is made inside it, asserted by the target still being empty. Red against the old order, as is round six's test.
…the store cannot be raced Review round eight. The link check was a check-then-use with the guarded window open at both ends, and the ordering was what held it open. The order was: create both, check both, narrow `runs`, narrow `.orca` — the parent last. `restrictToOwner` costs about 27ms a path here, measured (a `whoami` spawn, an `icacls /save` read, the write, and a second `/save` to verify), so the gap between checking `runs` and owning it ran past 50ms with `.orca` still granting the workspace's Modify. Modify on a parent is enough to delete a child whatever the child's own DACL says, so another account can `rmdir` and `mklink /J` in a loop: whenever a swap lands in that gap the check has already passed, the narrowing goes onto the link entry — `icacls` does not follow a reparse point — and the whole run is written through it, with the read-back verification inspecting that same link entry and reporting success. Reordered as the review sets it out. `.orca` is created, checked and narrowed before `runs` exists; only then is `runs` created, checked and narrowed. Once the container grants nobody else anything, nothing can delete or replace what sits under it, so the second check-then-use is not a race. `.orca` is checked a second time after `runs` is made, because it was itself open between its own check and its own narrowing. That does not close the window — closing it needs the DACL applied to an open handle rather than to a path, which node cannot do — but it turns a swap that lands there into a refusal rather than a recording written somewhere else. POSIX is untouched and now says so in its own branch rather than through a condition: create-only, no link check, because `chmod` follows a symlink and pointing a store at another disk is an ordinary thing to do. Verified end to end before committing, not only by the suite: a fresh workspace, a store that already existed, a junction at `.orca` and at `.orca/runs` (both refused), a `subst` drive (still records — that was the earlier false refusal), four repeat calls, a real `orca record --tls-intercept` (shell capture recorded `exit_code: 3`, model 200, every path in the store at three trustees), and `orca quickstart` (3/3 turns replayed, store at three trustees). The new test is red against the old order, as is the one from round six. Windows: 10 failures on main, 9 with this branch, no new ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🐳 OrcaCode Review
Found 3 issues in this PR: 🟠 3 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 673 calls · 82.2M tokens · 99% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
| const dir = await mkdtemp(join(tmpdir(), 'orca-acl-')); | ||
| try { | ||
| const saved = join(dir, 'acl'); | ||
| await run(icacls, [path, '/save', saved]); |
There was a problem hiding this comment.
🟠 P1 Narrow the orca-acl- scratch directory before writing the security descriptor into it
descriptorOf is the third temporary directory this feature mints in %TEMP%, and the only one that is not narrowed immediately after mkdtemp. The two siblings added in the same series do narrow: secureTransport in packages/shell-shim/src/install.ts (await restrictToOwner(dir, 0o700) right after mkdtemp, "made private the moment it is created and before anything is written into it") and the identical block in packages/cli/src/agent-spans.ts. Their doc comment states the reason, measured by the author: on Windows mkdtemp discards the mode and the directory takes whatever %TEMP% hands down — on the machine they measured, a local group and a second account, both with Modify.
That matters more here than it does for a transport, because this scratch file is not merely a spill of sensitive bytes: it is the input to both halves of the only guard this function has. restrictToOwner reads it twice through this helper — line 49 (explicitTrustees, which decides which foreign ACEs get named in /remove) and line 83 (the read-back verification, added precisely because "the parser cannot be trusted alone"). An account with Modify on %TEMP% — the very precondition the author measured and used to justify the transport fixes — can write inside orca-acl-XXXXXX: Modify includes FILE_DELETE_CHILD and write on the directory's children, so they can replace or rewrite acl (written by /save under the scratch directory's inherited ACL) between the icacls call and the readFile, exactly the way the author's own store finding describes a second account doing rmdir + mklink /J in a loop.
Consequences, on Windows only (on POSIX mkdtemp gives 0700 and this code is not even reached):
- a substituted descriptor in the first read hides foreign explicit ACEs from
foreign, so they are never named in/removeand survive the narrowing (an explicit ACE is not removed by/inheritance:r— the case the author measured and wrote the parser fix for); - a substituted descriptor in the second read makes the anchored read-back check pass on an ACL orca did not write, so
restrictToOwnerreports success while the store, the run CA's private key (ca.key,ca.ts) or the gateway API key (config.ts) is left readable/rewritable by another local account. All three of those are the secrets the change exists to protect, andorca record --tls-interceptwould proceed with a CA whose key someone else can read.
A crash or SIGKILL also leaves the un-narrowed orca-acl-XXXXXXXX directory behind for good: it is invisible to sweepStaleTransports, whose table (TRANSPORT_PREFIXES = ['orca-spans-', 'orca-shell-'], agent-spans.ts:297) is the registry the other two temp directories put themselves in, and restrictToOwner creates and destroys one of these on every call (2 per ensureRunsDir, 4 per RunCa.create), so the races above are plentiful rather than rare.
Concrete fix: make the scratch directory owner-only before anything is written into it, and fail closed if that cannot be established. It must not go through restrictToOwner, which would recurse (restrictToOwner -> explicitTrustees -> descriptorOf -> restrictToOwner); the narrowing has to be applied directly, once, and the directory reused:
let aclScratch: Promise | undefined;
async function aclScratchDir(icacls: string): Promise {
aclScratch ??= (async () => {
const dir = await mkdtemp(join(tmpdir(), 'orca-acl-'));
try {
// No read-back: this is the file the read-back is read from, and restrictToOwner
// would call back into here. execFile rejects on a non-zero exit, so a narrowing that
// did not apply is a throw rather than a descriptor read from a directory anyone can
// write.
await run(icacls, [
dir, '/inheritance:r', '/grant:r',
*${await currentAccountSid()}:(OI)(CI)(F), '/q',
]);
return dir;
} catch (err) {
await rm(dir, { recursive: true, force: true }).catch(() => undefined);
throw err;
}
})();
return aclScratch;
}
with descriptorOf writing join(await aclScratchDir(icacls), 'acl') and removing it in the process's exit handler instead of a per-call finally (or keep per-call dirs but narrow each one the same way). Either shape makes the descriptor this function parses and verifies unsubstitutable by any account but the owner, SYSTEM and Administrators — which is the bar the two transports already meet.
There was a problem hiding this comment.
Confirmed and fixed in 5a3e1ac. This one is mine and the framing is exactly right — I narrowed two transports for this reason two commits ago and walked past the third. Measured before changing anything:
D:AI(A;OICIID;0x1301bf;;;S-1-5-21-…-1002)
(A;OICIID;0x1301bf;;;S-1-5-21-2493905818-…)
(A;OICIID;FA;;;SY)(A;OICIID;FA;;;BA)(A;OICIID;FA;;;S-1-5-21-…-1001)
Two further local accounts, inheritable Modify, and a file written inside inherits all of it. And your point about which file it is, is the part that makes it worse than a spill: it is the input to both halves of the only guard the function has.
Fixed as you sketched, with two differences worth naming:
- One directory per process, random filename per call, rather than a shared
acl. A single cached path would be a scratch file every concurrentrestrictToOwneraddresses — the same defect as the${path}.incomingstaging one from round five. This way the extraicaclsis once for the life of the run, not two per path narrowed. - Owner, SYSTEM and Administrators, matching what
restrictToOwneritself grants, rather than owner alone.
The recursion warning was right and is why the narrowing is a direct icacls call and not restrictToOwner: it reads a descriptor, which is read from here. execFile rejects on a non-zero exit, so a narrowing that did not apply is a throw rather than a descriptor read out of a directory anyone can write.
Cleanup is a process.on("exit") rmSync, since as you say sweepStaleTransports does not know the orca-acl- prefix.
Test: after restrictToOwner, the newest orca-acl- in %TEMP% must be protected and at exactly the three trustees. Red against the un-narrowed version.
| // than in a recording written somewhere else. | ||
| await refuseLink(orca); | ||
| await refuseLink(dir); | ||
| await restrictToOwner(dir, 0o700); |
There was a problem hiding this comment.
🟠 P1 Check that .orca is ours (not merely not-a-link) before relying on it to protect the store
The reorder's stated invariant (lines 54-55: "nothing else can delete or replace what is under it, so the check on runs cannot be raced afterwards") rests on the container being orca's own directory, and nothing checks that. refuseLink only rejects reparse points, so a real directory put at .orca passes refuseLink(orca) and is then narrowed by restrictToOwner(orca) as if it were orca's — and nothing else in this function ever looks at the container again. Two facts the code itself states make that reachable: (1) lines 49-50 — "Modify on the parent is enough to delete a child whatever the child's own DACL says" — and the parent here is the workspace, which orca never narrows, so another account holding the workspace's usual Authenticated Users:(I)(M) can rmdir .orca and create its own directory at that path before any orca command (the narrowed .orca from the previous run does not stop the delete, because DELETE_CHILD is granted by the workspace, not by .orca); (2) the account that creates that directory owns it, and Windows lets an owner re-write its DACL, so the narrowing icacls applies to the substituted .orca binds nobody — on the next record/attach/replay/pull the owner re-grants itself FILE_DELETE_CHILD on .orca, deletes .orca/runs (whose own DACL, as the same comment says, does not decide that) and replaces it with its own directory in the wide window between ensureRunsDir returning and TraceWriter.create doing mkdir(<runs>/<runId>). The run directory then inherits the substitute's ACL, so every event, blob and fs/ snapshot is readable by that account while restrictToOwner reported success on both paths.
Fix: stop treating "exists and is not a reparse point" as "this is ours". Give the store an identity that survives a substitution and re-check it where the store is actually written: in ensureRunsDir record fs.stat(orca)/fs.stat(dir) (dev+ino, on Windows the volume serial and NTFS file index) after narrowing, and have the writer (and RunCa.create) re-stat both immediately before creating/writing into the store and refuse when the identity has changed — a substituted directory cannot have the original file index. That catches the replaced container and the replaced store where the ACL cannot, and it costs one stat per write compared with the whoami+two icacls spawns the narrowing already pays.
There was a problem hiding this comment.
Fixed in 5a3e1ac, but one load-bearing step of the finding does not hold and it changes the severity rather than the fix.
"restrictToOwner reported success on both paths" is not what happens unelevated. Measured, by stripping the owner's implicit WRITE_DAC with an OWNER_RIGHTS (S-1-3-4) ACE — which is the shape of a directory someone else owns:
constructed: D:PAI(A;OICI;FA;;;BA)(A;OICI;0x1301bf;;;OW)
restrictToOwner -> icacls fails, the call rejects
A substituted .orca gives me the workspace's inherited Authenticated Users:(M) — 0x1301bf, which does not carry WRITE_DAC — so the narrowing is refused loudly, not applied quietly. BUILTIN\Administrators:(F) would carry it, so elevated the attack does go through; that half stands. I could not test the elevated case directly (icacls /setowner needs a privilege I do not have here), so that is reasoning, not measurement, and I would rather say so than imply otherwise.
The check was still answering the wrong question, so it answers the right one now: vouchFor returns dev+ino and stillTheSame re-checks after each narrowing —
mkdir(.orca) -> vouchFor -> restrictToOwner -> stillTheSame
mkdir(runs) -> stillTheSame(.orca) -> vouchFor(runs) -> restrictToOwner -> stillTheSame
Verified that the index changes when a directory of the same name is deleted and remade, so a substitute cannot carry the original's. Only a genuine difference refuses: a filesystem with no file index answers zero for both and the check lapses rather than refusing a network share out of hand — the opposite of what the link check does with a failure, and deliberately, because one means "we could not look" and the other "there is nothing to compare".
What I did not do is thread the identity through to TraceWriter.create and RunCa.create. That closes the window after ensureRunsDir returns, which is real, but it is an API change across two packages on a branch that is already ten commits of hardening — I would rather it be its own change than be bolted onto this one. Saying that plainly rather than leaving it looking closed.
| // is 0644 in git, so the run landed 0644 under a 0755 directory — world-readable, in the store | ||
| // SECURITY.md says is 0600/0700. `pull` is the other command that *installs* a run rather than | ||
| // writing it, and it chmods every entry for exactly this reason. | ||
| await applyStoreModes(runDir); |
There was a problem hiding this comment.
🟠 P1 Narrow the copied quickstart run on Windows too, instead of assuming a copy inherits the store ACL
applyStoreModes is chmod, and the comment above it (lines 74-75) says that on Windows "the modes are discarded and the tree already inherits the ACL ensureRunsDir put on .orca/runs". Nothing makes that true: chmod on Windows touches only the read-only attribute, so the only thing that can protect these bytes is inheritance, and fs.cp does not create the files by writing them into the destination — it copies them, and on Windows a copy carries the source's security descriptor (libuv uses CopyFileExW/CopyFileW, documented as preserving "security resource attributes"; the chmod Node's cp applies afterwards is exactly the mode-only half the author already describes for POSIX). So the run's events.jsonl, manifest.json, fs/ snapshot and blobs take the ACL of the installed package — which for the common case (npm i orcareplay/npx, or a checkout of this repo) sits inside the project tree, the same tree the commits' own measurements show carrying Authenticated Users:(I)(M) / Users:(I)(RX). Result: orca quickstart — the README's first line, and the command this series exists to make safe — leaves a world-readable recording under a store that reports three trustees. The added test cannot see it: expectOwnerOnly checks only join(result.dir, '.orca', 'runs'), and the per-file mode assertions are inside if (process.platform !== 'win32').
Fix: give applyStoreModes a Windows half — after the copy, walk the run directory and call restrictToOwner(path, entry.isDirectory() ? 0o700 : 0o600) for every entry (the same shape ensureRunsDir/RunCa use), or install the trace through TraceWriter/the same write-then-narrow path pull uses rather than cp. And assert it: the Windows branch of the new quickstart test should check a file inside the run directory, not just .orca/runs.
There was a problem hiding this comment.
The conclusion here does not hold, and I measured it rather than arguing from the docs.
CopyFileEx's "security resource attributes" line is about ATTRIBUTE_SECURITY_INFORMATION, not the DACL. A file created by a copy takes the destination directory's inherited ACL, the same as any other newly created file:
source ACL : BA SY AU BU (broad)
copy ACL : BA SY <owner> (the destination's)
The previous round's quickstart e2e already showed events.jsonl at three trustees — I had read that as luck rather than as the answer, so thank you for making me go and check which it was.
The test-coverage half is fair and is fixed (5a3e1ac). expectOwnerOnly was applied only to .orca/runs, so the property was believed rather than checked. It now checks a file inside the run — which needed the assertion split in two, because they are not the same property: a path orca narrows itself may carry nothing inherited (an inherited entry there is the defect), while everything beneath it is protected by inheritance and must carry it. One helper asserting "no (I)" for both would have been wrong in one direction or the other.
So no change to applyStoreModes itself. Its doc comment did need fixing for a related reason and got it in the follow-up commit: it asserted the Windows half instead of stating it as measured, which is exactly the gap that let the claim above look plausible. (It had also been inserted between isEmptyish's doc comment and isEmptyish, leaving that one documenting nothing — my error from the round that added it.)
… of a shared directory
Review round nine. Three findings: one real and mine, one whose conclusion is
right on a mechanism that is not, one whose mechanism does not hold at all.
**The scratch directory was the only temp directory in this series left
un-narrowed.** `descriptorOf` writes `icacls /save` output into `mkdtemp(…,
'orca-acl-')`, and `mkdtemp` discards its mode on Windows. Measured on this
machine:
D:AI(A;OICIID;0x1301bf;;;S-1-5-21-…-1002)
(A;OICIID;0x1301bf;;;S-1-5-21-2493905818-…)
(A;OICIID;FA;;;SY)(A;OICIID;FA;;;BA)(A;OICIID;FA;;;S-1-5-21-…-1001)
— two further local accounts, inheritable Modify, and a file written inside
inherits all of it. That file is the input to *both* halves of the only guard
this function has: the descriptor `explicitTrustees` builds its `/remove` list
from, and the descriptor the read-back compares against what was asked for.
Substituting it hides a foreign trustee from the first and satisfies the second,
with `restrictToOwner` reporting success. I narrowed two transports for exactly
this reason two commits ago and walked past this one.
Narrowed directly rather than through `restrictToOwner`, which reads a
descriptor, which is read from here — that would recurse. One directory per
process with a random filename per call, so it costs one extra `icacls` for the
life of the run rather than two per path narrowed, and it takes itself with it
on exit, since nothing sweeps an `orca-acl-` the way `sweepStaleTransports`
sweeps the two named transports.
**"Not a link" was being read as "ours".** A real directory another account puts
at `.orca` passes a link check and is then narrowed as though it were orca's.
The review's consequence — that this reports success — does not hold where it
matters most, and the measurement is worth recording: strip the owner's implicit
WRITE_DAC with an OWNER_RIGHTS ACE, which is the shape of a directory someone
else owns, and `restrictToOwner` fails loudly rather than quietly succeeding.
Unelevated, a substituted container is inherited `Authenticated Users:(M)` —
Modify, which does not carry WRITE_DAC — so icacls refuses. Elevated it would
go through, and that half is real.
Either way the check was answering the wrong question, so it answers the right
one now: `vouchFor` returns `dev`+`ino` — the volume serial and the NTFS file
index, verified to change when a directory of the same name is deleted and
remade — and `stillTheSame` re-checks after each narrowing. A substitute cannot
carry the original's index, so a swap that lands in the window ends in a refusal
instead of a recording written somewhere else. It does not close the window;
that needs the DACL applied to an open handle, which node cannot do.
Only a genuine difference refuses. A filesystem with no file index answers zero
for both and the check lapses, rather than refusing a network share out of hand
— the opposite of what the link check does with a failure, and deliberately: one
means "we could not look", the other "there is nothing to compare".
**`fs.cp` does not carry the source's DACL.** The review read `CopyFileEx`'s
"security resource attributes" line as the security descriptor travelling with
the file. Measured: a file copied out of a world-readable package into a
narrowed directory comes out `BA SY <owner>` — the destination's inherited ACL,
not the source's. The quickstart e2e in the previous round already showed this
at three trustees and I had read it as luck rather than as the answer.
The test-coverage half of that finding is fair and is fixed: the Windows branch
checked only `.orca/runs`. It now checks a file inside the run — which needed
the assertion split in two, because a path orca narrows itself may carry nothing
inherited while everything beneath it is protected *by* inheritance and must.
Verified end to end before committing: the scratch directory at three trustees
and one per process; fresh workspace; existing store; junction at `.orca` and at
`.orca/runs` (both refused); `subst` drive (still records); four repeat calls;
`orca record --tls-intercept` (shell `exit_code: 3`, model 200, every path at
three trustees); `orca quickstart` (3/3 turns, same). New test red against the
un-narrowed scratch directory.
Windows: 10 failures on main, 9 with this branch, no new ones.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…asured Two repairs to the same comment. It had been inserted between `isEmptyish`'s doc and `isEmptyish`, leaving that one documenting nothing. And it asserted that on Windows "the tree already inherits the ACL" — true, but believed rather than checked, which is what let a review read `CopyFileEx`'s "security resource attributes" line as the source's DACL travelling with the file. It does not: a file copied out of a world-readable source into a narrowed directory comes out at the destination's three trustees. The comment says that, and says the test asserts it on a file inside the run rather than on the store root. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
⚠️ Outdated review — the PR head changed from 5a3e1ac to 9efe8cd during the review; these findings describe a diff the PR no longer has.
These findings describe5a3e1acf99f5. Re-run the review to check the PR as it stands now.
The gate evaluated to blocked, but it is not enforced here: branch protection will not see it.
🐳 OrcaCode Review
Found 3 issues in this PR: 🟠 3 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 574 calls · 67.9M tokens · 99% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
| `*${SYSTEM}:(OI)(CI)(F)`, | ||
| `*${ADMINISTRATORS}:(OI)(CI)(F)`, | ||
| '/q', | ||
| ]); |
There was a problem hiding this comment.
🟠 P1 Narrow the ACL scratch directory with the same three steps restrictToOwner uses (/remove + read-back)
aclScratch is the new sibling of restrictToOwner (same file, 80 lines above) and is written for the same job: mint an owner-only directory under %TEMP%. restrictToOwner performs three steps, and the code comment at lines 55-61 says exactly why each is needed: /inheritance:r only strips ACEs flagged inherited, /grant:r only replaces explicit grants for the trustees it names, so an explicit ACE for anyone else survives — and it then reads the DACL back (lines 78-93) because "an ACE of a type nobody anticipated fails the check rather than being skipped by it". aclScratch does /inheritance:r and /grant:r and nothing else: no /remove of foreign explicit trustees (explicitTrustees/lines 51+72) and no read-back. execFile only tells it that icacls exited 0, which is precisely the assurance the sibling's comment refuses to rely on.
Why that matters here more than anywhere: the file descriptorOf writes into this directory is the input to both halves of the only guard restrictToOwner has — the descriptor explicitTrustees builds the /remove list from, and the descriptor the read-back compares against what was asked for. If a foreign explicit ACE (with (OI)(CI), so it also applies to files created inside) is on that directory — planted by the account that already has WRITE_DAC on %TEMP% in the window between mkdtemp (line 173) and the icacls call, or left there by a machine-wide ACL propagation of the "Replace all child object permissions" shape the sibling calls "an ordinary shape rather than an exotic one" — it is not removed, not replaced by /grant:r, and not noticed. That account can then read and substitute the descriptor files, which hides it from explicitTrustees and satisfies the read-back, so restrictToOwner returns success while a foreign trustee keeps access to .orca/runs, the run CA's private key and config.json. That is verbatim the failure this last commit was written to close, left open one step short of its own sibling.
Concrete fix: run the same invocation restrictToOwner runs — /inheritance:r, then /remove *<trustee> for each foreign trustee, then /grant:r, then the anchored read-back — or, since the recursion the comment describes only forbids reading a descriptor into the not-yet-narrowed directory, clear it in the same invocation with /reset /inheritance:r /grant:r … (/reset drops explicit ACEs; it is only rejected for the store root because of the window, and nothing is written into the scratch directory during it) and verify the result with the sibling's asWritten check. Factor the shared argument list so the two cannot drift again.
There was a problem hiding this comment.
Confirmed and fixed in ec70484, and the framing is the right one: they were one job written twice, eighty lines apart, and the second was missing precisely the parts the first's comment argues for.
The grants and the expected shape are factored now — grantsFor() and asWritten() — so the two cannot drift again, which was your closing point.
One half of the finding does not apply here, and I would rather record the measurement than fix blind: a directory mkdtemp has just created carries no explicit ACE to remove. Measured — five ACEs, every one flagged ID:
D:AI(A;OICIID;0x1301bf;;;S-1-5-21-…-1002)(A;OICIID;0x1301bf;;;S-1-5-21-2493905818-…)
(A;OICIID;FA;;;SY)(A;OICIID;FA;;;BA)(A;OICIID;FA;;;S-1-5-21-…-1001)
So /inheritance:r clears all of them and the /remove list is empty by construction. The only way an explicit ACE gets onto that directory is the window you describe in the sibling finding, which is fixed there rather than here.
Your /reset alternative is out: icacls rejects it alongside /inheritance:r with code 87. I measured that in round four when /reset was briefly the fix for the store, and re-checked it now rather than trusting the note.
So what changed is the half that mattered: the anchored read-back, read straight rather than through descriptorOf — that writes into this directory and would await the promise it is running inside.
| `*${SYSTEM}:(OI)(CI)(F)`, | ||
| `*${ADMINISTRATORS}:(OI)(CI)(F)`, | ||
| '/q', | ||
| ]); |
There was a problem hiding this comment.
🟠 P1 Vouch for the scratch directory before making the ACL guard depend on what is read out of it
The scratch directory is now the single input to restrictToOwner's only guard, and it is narrowed after its name exists, with nothing verifying that the entry at that path is the one mkdtemp made. mkdtemp creates %TEMP%/orca-acl-XXXXXX, and the narrowing only takes effect tens of milliseconds later: restrictToOwner→aclScratch brackets the icacls with a whoami spawn (currentAccountSid) and the process launch. The name is not secret — a local account with Modify on %TEMP% may list the directory (this series measured exactly that ACL: (A;OICIID;0x1301bf;;;S-1-5-21-…-1002), "two further local accounts, both with inheritable Modify"), and Modify on a parent is enough to rmdir a child whatever the child's own DACL says — the same sentence commit 7f8cc4f uses to justify its own reordering. mklink /J needs no privilege, and per this series' own measurement icacls does not follow a reparse point, so the narrowing lands on the junction entry and exits 0.
Unlike ensureRunsDir — which, in this same commit, added vouchFor (refuse isSymbolicLink) and stillTheSame (dev+ino before/after) precisely so that "such a swap ends in a refusal rather than in a recording written somewhere else" — aclScratch does neither. So a swap that lands in the window leaves the real directory uninherited and the attacker's directory receiving every icacls /save output. That is the file the doc comment calls "the input to both halves of the only guard this function has": explicitTrustees decides the /remove list from it, and the anchored read-back compares against it. Substituting it hides a foreign trustee from the first and satisfies the second, and restrictToOwner returns success — while the gateway API key in config.json, the run CA's private key in <run>/tls/ca.key and the whole trace store keep their inherited grants.
Fix (the pair already in this commit, reused):
- right after
mkdtemp,lstatthe entry and throw ifisSymbolicLink()—vouchFor(dir)is not usable here as-is (it needsicacls, which is available in scope, but the error text mentions "a trace store"); amkdtemp-created directory is never a link, so this is a sound refusal; - capture
dev/inothere and re-check them after theicaclsreturns (astillTheSame-style comparison), so a swap landing between the check and the narrowing is a refusal rather than a forged descriptor.
There was a problem hiding this comment.
Confirmed and fixed in ec70484. This one is the same defect as the commit immediately before it, in the sibling I wrote at the same time and did not carry it across to.
vouchFor and stillTheSame now bracket the narrowing: lstat before, icacls, lstat after, and a refusal on isSymbolicLink() or on a changed dev/ino. Same pair, same reason, as ensureRunsDir.
Two details from doing it:
- The link check is sound here in a way it is not in
ensureRunsDir, exactly as you say:mkdtemphas just made this directory, so a link at that path can only be a swap. There is no legitimate case to preserve. - The read-back is written out rather than routed through
descriptorOf, becausedescriptorOfwrites into this directory and would await the promise it is running inside — a deadlock rather than the recursion the old comment warned about. Same conclusion, different mechanism, so the comment says the new one.
What this does not close, and the comment says so: the window itself. Closing it needs the DACL applied to an open handle rather than to a path, which node cannot do. What it does is turn a swap landing there into a refusal rather than a forged descriptor.
| await stillTheSame(orca, orcaWas); | ||
| const dirWas = await vouchFor(dir); | ||
| await restrictToOwner(dir, 0o700); | ||
| await stillTheSame(dir, dirWas); |
There was a problem hiding this comment.
🟠 P1 Vouch for who owns the store, not merely that it is not a link and has not changed
vouchFor/stillTheSame answer "is this a reparse point?" and "is this the same entry as a moment ago?". Neither asks who owns the directory, and on Windows the DACL is not the whole story: an object's owner keeps implicit WRITE_DAC whatever the DACL says (that is what the OWNER RIGHTS SID exists to take away, and this change deliberately removes every foreign ACE, including any OWNER RIGHTS ACE, from the paths it narrows). So a directory another account put there before the check is its own baseline: it passes vouchFor (a real directory, not a link), passes restrictToOwner — which succeeds, because the DACL is what grants the caller WRITE_DAC, and the other account can simply grant the recording account full control when it plants the directory — passes the read-back verification (the DACL really is owner/SYSTEM/Administrators), and passes stillTheSame (it compares the directory with itself). The comment at lines 64-68 states this exact case as the reason the identity check exists ("A real directory another account put there … its creator owns it, so it can re-grant itself afterwards"), but the code cannot see it: the baseline is taken from whatever is already at the path, so a planted substitute is indistinguishable from orca's own directory. The same holds for packages/core/src/private-files.ts:restrictToOwner, whose success is read everywhere as "this path is ours".
Concretely, in a workspace another local account can write to (the threat model the whole series is written for — "another account can rmdir and mklink /J in a loop"): that account deletes .orca/.orca/runs (Modify on the workspace grants FILE_DELETE_CHILD) and recreates it as a directory it owns, with icacls <path> /grant *<recording-account>:(OI)(CI)(F) and nothing else. orca record then narrows the ACL, verifies it, reports nothing, and writes the whole run into a directory the other account still controls: it can re-grant itself (OI)(CI)(F) at any point to read the store, and since it owns the directory it can add an inheritable ACE between ensureRunsDir and the writer's mkdir(<runId>) so that everything created afterwards — the run directory and therefore its blobs, fs store and CA — is born readable by it. The recorded data is exactly what SECURITY.md says to treat as a shell history plus a heap dump, and the commit's own claim is that "a store it cannot vouch for is not one it should quietly write a recording into".
Concrete fix, and it does not need any new query: make the narrowing take ownership as well, so the DACL and the ownership agree — add /setowner, '*' + await currentAccountSid() to the same icacls invocation in restrictToOwner (it succeeds when the caller has WRITE_OWNER, which is precisely when the narrowing applied, and when the recording account owns the path already; when it does not, icacls exits non-zero and execFile rejects, which is the fail-closed direction this module already takes). If taking ownership is not wanted, the alternative is to read the owner and refuse on a mismatch — note that icacls /save (used by descriptorOf) reports the DACL only and never names the owner, so the owner has to come from a source that reports it; refusing when it cannot be read is the same fail-closed answer system32() gives for SystemRoot.
There was a problem hiding this comment.
You are right, and this retires the pushback I made on the equivalent finding last round — so let me put that first.
Last round I argued a substituted container fails closed, because the caller would hold only Authenticated Users:(M) and Modify does not carry WRITE_DAC. Your point that the planting account can simply grant the recording account full control defeats that, and I measured it rather than conceding on paper:
planted D:PAI(A;OICI;FA;;;BU)(A;OICI;FA;;;<recording account>)
narrowed D:PAI(A;OICI;FA;;;BA)(A;OICI;FA;;;SY)(A;OICI;FA;;;<recording account>)
restrictToOwner succeeds, the read-back verifies, stillTheSame compares the directory with itself, and nothing anywhere says the store is not orca's. Exactly as you set out.
The proposed fix does not work on this machine, and I checked before saying so:
icacls <path> /setowner *<current account SID>exits 5, even naming the account that already owns the directory. Unelevated there is no SeTakeOwnership/SeRestore to draw on.- Combined with the narrowing in one invocation it exits 87.
The alternative — read the owner and refuse on a mismatch — has no source I can depend on: icacls /save emits the DACL only (confirmed: the saved line starts D: with no O: segment); Get-Acl cannot autoload Microsoft.PowerShell.Security here, which is why the tests read SDDL through /save in the first place; and dir /q does report it, but as a name in a localized column layout rather than a SID. Name-and-layout parsing is the shape that has gone wrong twice already in this PR, and I am not going to make the ownership check the third.
So this is not fixed, and I have not papered over it. It needs a mechanism this module does not have — a native call that can read the owner, or a design answer such as refusing to record where the workspace grants non-owner write at all.
That makes it the clearest argument yet for splitting this branch, which I had already recommended: the demonstrated vulnerability (mode bits doing nothing on Windows, the CA private key and the gateway key world-readable) is one change, and defending a workspace another local account can write is a threat model that should be designed rather than accreted over twelve commits. I would rather land the first and open the second as its own issue with your last two findings as its starting point.
…bling gets Review round ten. Three findings against 5a3e1ac. Two are mine and are fixed; the third is right, its proposed fix does not work here, and it retires a pushback I made last round. **`aclScratch` was `restrictToOwner` with the hard-won parts left off.** Written eighty lines below a comment explaining why `/inheritance:r` plus `/grant:r` is not enough on its own and why exiting 0 is not an answer — and then written with exactly `/inheritance:r`, `/grant:r`, and no read-back. The review is right that the two were one job and had already drifted, so the grants and the expected shape are factored now and both call the same pair. One half of that finding does not apply, and it is worth recording rather than fixing blind: a directory `mkdtemp` has just created carries **no explicit ACE to remove** — measured, five ACEs and every one flagged inherited — so `/inheritance:r` clears them all. The `/remove` list is empty by construction there, which is why the missing read-back is the part that mattered. (The review's alternative, `/reset` in the same invocation, is rejected by icacls: code 87, as measured in round four.) **Nothing vouched for the scratch directory.** `ensureRunsDir` grew `vouchFor` and `stillTheSame` in the commit immediately before this one, for the window between making a path and owning it, and the sibling with the same window did not get them. `%TEMP%` here grants two further local accounts inheritable Modify — Modify on a parent is enough to `rmdir` a child whatever the child's DACL says — and `icacls` does not follow a reparse point, so a junction swapped in during that window takes every descriptor this module reads. `lstat` either side now, identity compared, and the narrowing read back. **Ownership: the review is right and I was wrong last round.** I argued that a substituted container fails closed because the caller has no WRITE_DAC. Measured, and it does not: an attacker planting a directory grants the recording account Full Control when they plant it, and then planted D:PAI(A;OICI;FA;;;BU)(A;OICI;FA;;;<recording account>) narrowed D:PAI(A;OICI;FA;;;BA)(A;OICI;FA;;;SY)(A;OICI;FA;;;<recording account>) succeeds, verifies, and reports nothing — while they keep the ownership, and with it the implicit WRITE_DAC to re-grant later. `vouchFor` and `stillTheSame` cannot see it: the baseline is taken from whatever is already there, so a substitute is compared with itself. The proposed fix does not work on this machine: `icacls /setowner` exits 5 even naming the current account, and cannot be combined with the narrowing (code 87). Nor can the owner be read — `icacls /save` emits the DACL only, `Get-Acl` cannot autoload here, and `dir /q` gives a localized-layout *name* rather than a SID, which is the shape of parsing that has already gone wrong twice in this PR. So it is not fixed, and it is not papered over either. It needs a mechanism this module does not have, which is the clearest argument yet for the split I recommended last round: the demonstrated vulnerability is one thing, and defending a workspace another local account can write is a threat model that should be designed rather than accreted. Verified end to end: scratch directory at three trustees and one per process; fresh workspace; existing store; junction at `.orca` and at `.orca/runs` (both refused); `subst` drive; four repeat calls; `orca record --tls-intercept` (shell `exit_code: 3`, model 200, every path at three trustees); `orca quickstart` (3/3 turns, same). The scratch test is red against an un-narrowed directory. Windows: 10 failures on main, 9 with this branch, no new ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
⚠️ Outdated review — the PR head changed from 9efe8cd to ec70484 during the review; these findings describe a diff the PR no longer has.
These findings describe9efe8cdd3c80. Re-run the review to check the PR as it stands now.
The gate evaluated to blocked, but it is not enforced here: branch protection will not see it.
🐳 OrcaCode Review
Found 1 issue in this PR: 🟠 1 P1.
OrcaCode Review — Route Smarter. Ship Safer. Spend Less.
Engine-reported: 884 calls · 116.9M tokens · 99% cached
❤️ Share · Install OrcaCode Review
Free on GitHub — the review runs on your own OrcaRouter key. If it helped, a shout-out goes a long way.
Share: X · Reddit · LinkedIn
Follow: X · Discord · LinkedIn · OrcaRouter
| `*${SYSTEM}:(OI)(CI)(F)`, | ||
| `*${ADMINISTRATORS}:(OI)(CI)(F)`, | ||
| '/q', | ||
| ]); |
There was a problem hiding this comment.
🟠 P1 Verify the ACL scratch directory's identity, not just that icacls ran: another account with Delete-Subfolders on %TEMP% can replace it and forge every narrowing
The existing equivalent — ensureRunsDir's win32 branch in the same change — does three things around this narrowing that this new one does not: vouchFor (refuse a reparse point) before narrowing, stillTheSame (compare dev/ino) after it, and it makes the narrowed directory non-empty (runs) before its last check, precisely so that a holder of the parent's Delete-Subfolders right can no longer delete it ("whatever the child's own DACL says" — paths.ts). aclScratch instead mints the descriptor directory straight in %TEMP% and narrows it with a bare icacls call.
State: an account that another user's orca mints its orca-acl-<6 random> directory under is %TEMP% — the very parent this change documents as granting other local accounts Modify ("measured on this machine: two further local accounts, both with inheritable Modify"), and Windows' Modify includes "Delete subfolders and files". The directory is empty from mkdtemp until the first icacls /save writes into it. So another account can watch %TEMP%, rmdir the empty orca-acl-… and mklink /J the same name (no privilege needed), and restrictToOwner/descriptorOf then report success exactly as the code's own vouchFor doc records ("icacls does not follow a reparse point… the narrowing lands on the link entry… and restrictToOwner would report success").
Two consequences, both from that: (a) descriptorOf reads the descriptor back out of a directory the attacker owns, so the substitution this directory was narrowed to prevent is still available — an attacker who makes the path unwritable, or rewrites the saved file (it appears in their directory, so a directory watcher on their own tree is enough), can empty foreign (nothing is /removed) and satisfy the anchored read-back with the same file, and because the scratch directory is cached for the process's life, every later narrowing of that run — the CA private key's, config.json's, the store's, the transports' — reports success over a DACL that still grants them read; (b) even without a rewrite, a directory the victim cannot write makes /save fail, so descriptorOf throws and every orca record/pull/quickstart on Windows refuses — a persistent, cheap denial.
Fix: stop putting the guard's own input in a directory whose parent lets another account delete it. Mint it inside a container this process has already narrowed (or the user's profile), and — matching ensureRunsDir — capture vouchFor(dir)'s identity immediately after mkdtemp, re-check it with stillTheSame after the narrowing, and create the first file inside before anything is read back, so the directory is no longer deletable by an account that only holds the parent's Delete-Subfolders.
There was a problem hiding this comment.
Most of this landed in ec70484 while the review was running — vouchFor before the narrowing, stillTheSame after, and the anchored read-back — so those are already in. One part was not, and it is the part that outlives the call, so thank you for it: it is fixed in the commit above.
The identity check closes the window up to the moment aclScratch returns, and does nothing about the rest of the run. The directory is reused for every narrowing the process makes, and between them it sat empty. Your Delete-Subfolders point is the one that matters there, and it is specifically about an empty child. Measured:
rmdir on an empty directory -> succeeds
rmdir on a non-empty directory -> ENOTEMPTY
So one file, written once and kept, is the whole fix. It is out of reach in turn: it inherits the directory's ACL, so deleting it would need Delete-Subfolders here, which is what the narrowing just took away.
Checking the same property on the paths that already existed: .orca holds runs and .gitignore, and runs sits under a narrowed parent — so both were already safe, by luck rather than by design. The comment says so now, because that is the reasoning a future reader needs and it was nowhere in the code.
I did not take the other half of the fix — minting the directory somewhere already narrowed. The sentinel gives the same property without adding a second location to reason about, and this module has acquired enough surface already.
On the denial half of (b): a scratch directory the process cannot write makes descriptorOf throw, and orca record refuses rather than proceeding. That is the direction this module fails in everywhere else, so I left it there rather than adding a fallback that would have to be trusted too.
…emptied and replaced Review round eleven, raised against the commit before last. Most of it — `vouchFor` before the narrowing and `stillTheSame` after — landed in ec70484 while the review was running. One part did not, and it is the part that outlives the call. The identity check closes the window up to the moment `aclScratch` returns. It does nothing about the rest of the run: the directory is reused for every narrowing the process makes, and between them it sat **empty** in `%TEMP%` — where Modify, which two further local accounts hold here, carries Delete-Subfolders, and Delete-Subfolders on a parent removes a child whatever the child's own DACL says. An *empty* child. Measured: rmdir on an empty directory -> succeeds rmdir on a non-empty directory -> ENOTEMPTY So one file, written once and kept, is the whole fix. It is out of reach in turn: it inherits this directory's ACL, so deleting it would need Delete-Subfolders *here*, which is exactly what the narrowing took away. `.orca` and `.orca/runs` were already safe this way and it was luck rather than design, so the comment now says why: `.orca` holds `runs` and `.gitignore`, and `runs` sits under a narrowed parent. This one was the only path left where an attacker's `rmdir` would have been answered with "certainly". The review also proposes minting the directory somewhere already narrowed instead. The sentinel gets the same property without adding a second location to reason about, so that is the shape taken. Verified: the scratch directory comes back at three trustees, protected, one per process, and non-empty; the store scenarios, `orca record --tls-intercept` and `orca quickstart` all unchanged. The test is red against the version without the file, on the assertion that names the reason. Windows: 10 failures on main, 9 with this branch, no new ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Orca-Code-Review — push 4
❌ 3 findings block merge
chmod(path, 0o600)on Windows sets the read-only attribute and throws the mode away.statthenanswers
0o666whatever was asked for.A test in
config.test.tshad already noticed, and skipped itself with this reasoning:It cannot provide it as mode bits. The file has permissions there all the same — they are just
an ACL — and without anyone setting one, what it gets is whatever the workspace hands down.
What that actually meant
Recorded through a real
orca record --tls-intercept, by an agent reading the ACL of the very keyit had been told to trust, from inside the run:
(I)is inherited — nobody chose any of this. Every local account could read the run CA'sprivate key; every authenticated one could rewrite it.
That key signs the certificates the agent has been told to trust for the life of the run. Reading
it is enough to impersonate every intercepted host to that agent. Writing it is enough to
substitute a CA of one's own.
Two more things sat under the same non-guarantee:
config.json— a gateway API key, in a file the machine could read.SECURITY.mdasks you to treat as a shell history plus a heapdump, and which the same file stated was
0600/0700as a matter of fact.The fix
restrictToOwner(path, mode)keeps the promise in whatever currency the filesystem has:chmodonPOSIX, and on Windows an ACL with inheritance dropped and only the owner, SYSTEM and
Administrators left. That is the rule OpenSSH for Windows enforces on its own private keys, and
the closest the platform comes to 0600 — under which root can read the file too.
After, through the identical path:
Three call sites, and the middle one carries most of the weight
RunCa.createensureRunsDir.orca/runsonce propagates to everything written beneath itwriteConfig.orcaentirelyThe
ensureRunsDircall is why this is affordable. A second recording, with nothing restrictedindividually:
One call covers the run directories, blobs,
.incomingstaging areas andtls/— rather than oneicaclsspawn per file for a trace that may hold tens of thousands. It applies only when thiscall created the directory, which is what passing
modetomkdiralready meant: a storesomeone has deliberately opened up is theirs to have opened up.
Two details that are load-bearing
By SID, never by name.
BUILTIN\Administratorsis localized on a localized Windows, and agrant naming a principal that does not resolve matches nothing — which, after inheritance has
been dropped, would leave the file with no usable ACE at all.
icacls.exeandwhoami.exeby absolute path. Resolving them through PATH while securing aprivate key is one way that key reaches exactly the reader this exists to shut out. The tests found
this one for real: Git for Windows ships a POSIX
whoamiearlier on PATH, and the first draft ofthe test helper picked it up.
Testing
packages/core/test/private-files.test.tscovers the mechanism on both platforms. On Windows itreads the ACL as SDDL via
icacls /save, because that is SIDs rather than the localized namesicaclsprints by default — a test matchingBUILTIN\Userswould pass on a German machine byfailing to find what it was looking for. (
Get-Aclwould be the obvious tool and is notdependable: PowerShell could not autoload
Microsoft.PowerShell.Securityon the machine this waswritten on.)
It asserts against a control: a file written the ordinary way, showing the inherited grants,
before the same file is restricted. And it checks that a file written after a directory is
restricted, and never restricted itself, still comes out owner-only.
The two call-site tests assert the property that belongs there — nothing was inherited — and
config.test.ts's now runs on Windows instead of skipping.prettier --check,tsc --build --force,scripts/conformance.mjs(63 events, 0 failures) andscripts/fidelity.mjs --check(0 regressions) are clean. Windows: 4 failures before, 3 after,no new ones. POSIX behaviour is unchanged —
restrictToOwnerischmodthere — except thatensureRunsDirnow sets the mode it always documented rather than leaving it to umask.SECURITY.mdsays what is true now, including that it did not used to be.🤖 Generated with Claude Code
Review round 2 — three holes, all real (
a2f149a)Review found seven findings that collapse to three. All three reproduced on Windows, and the first
two left this fix doing nothing at all for most installs. Worth being blunt about that: the
vulnerability above is real and demonstrated, and the first attempt at closing it did not close
it except in a workspace with no
.orca/runsyet.1. The fallback path was drive-relative.
'C:\Windows'is notC:\Windows—\Wis not anescape, so the literal's value is
C:Windows: nine characters,isAbsolutefalse. CreateProcessresolves that against the current directory on C:, which during a recording is the workspace being
recorded, and
Windows/System32/icacls.exeis a perfectly storable git path. The guessreintroduced exactly the hijack the comment above it says absolute paths exist to prevent.
There is now no fallback. A correctly spelled
C:\Windowsis still a guess and still wrong on amachine whose Windows is not on C:;
SystemRootis set by the OS in every normal environment, andwhere it is not, refusing is the only answer that cannot be silently wrong. Three test helpers had
the same broken literal copied into them; corrected.
2. The ACL was applied only to a store this call created.
mkdir(recursive)returnsundefinedfor a directory that already exists, so a store skipped once was skipped forever —which is every install predating this. Reproduced: a hand-made
.orca/runs, then a realorca record:— the state the SECURITY.md sentence in the first commit called fixed. The create-only rule is
right on POSIX (the directory mode is not load-bearing there; every file gets its own 0600 from the
writer, and a directory someone opened up deliberately is theirs) and does not translate: on
Windows an inherited ACL is what the workspace handed down, not a choice. Applied either way there
now.
3.
orca quickstartmade the store by hand. A bare recursivemkdir, so the directory theREADME's first line produces carried the broad ACL — and
ensureRunsDirsaw it as pre-existingever after. It goes through
ensureRunsDirnow, asorca pullalready does. Before / after:Bounds, stated rather than implied
This covers what is written from now on.
icaclson a parent does not re-propagate to existingchildren, and rewriting the ACL of every file in a store already on disk is not something
recordshould do behind someone's back. SECURITY.md now says that too.
Tests
Three, each verified to fail against the reverted implementation and pass with it:
.orca/.gitignoreand the owner-only ACLensureRunsDirrestricts a directory it did not createrestrictToOwnerrefuses whenSystemRootis absent rather than resolving a workspace-relativepath
One correction to the review:
.orca/.gitignorewas already present after quickstart — a laterensureRunsDirfrom the replay writes it. The ACL was the half genuinely lost. The test assertsboth, since the
.gitignorehalf is what can be checked on every platform.Windows: 10 failures on main, 9 with this branch, no new ones. All four gates clean.
Review round 3 — the Windows branch was an allowlist, not a replacement (
81ba561)One finding, and it goes to the heart of the function.
chmodreplaces the whole permission state.This was modelled on it and replaced nothing:
/inheritance:rremoves only the ACEs carrying the inherited flag/grant:rreplaces previously granted explicit permissions for the trustees it namesSo an explicit ACE for anyone else survives both. Planted one and measured:
on a DACL whose doc comment claims "the only trustees left are the owner, SYSTEM and
Administrators". The inheritable case is the one that matters, because it is not a single stale ACE
— everything written afterwards picks it up:
Reachable without anyone doing anything exotic: "Replace all child object permissions" converts
inherited ACEs into explicit ones, and so do
robocopy /SECand a tree carried between machines.The fix
/resetbefore the restrict. It drops every explicit ACE and leaves what the parent hands down;/inheritance:rthen takes that away too, and/grant:rwrites onto an empty DACL.Two invocations, because icacls rejects
/resetalongside/inheritance:r— the single-callversion was tried first and errors out in a way that leaves the path untouched, which is worth
knowing.
/resetwidens nothing on the way through, which is why this beat enumerate-and-remove: adirectory is restricted before anything is written into it, so what a child inherits back from
/resetis already owner-only. Measured, not assumed.The tests were blind to it, which the review also said
BUILTIN\Usersand asserts the exact trusteeset, on the directory and on a file written into it afterwards. Verified red without the
/reset— four trustees where three were expected.expectOwnerOnlyhelpers asserted only that nothing was inherited, which asurviving explicit ACE passes by being explicit. They now also require exactly three grants,
counted rather than named because the names icacls prints are localized.
I did not take the offered "re-read the DACL after the grant and throw" option. With
/resettheend state follows from icacls semantics that are now measured, and the guarantee is pinned by a
test that goes red if they change — rather than by a third spawn on every path.
Windows: 10 failures on main, 9 with this branch, no new ones. All four gates clean.
Review round 4 — the
/resetthat cleared explicit ACEs also widened the store (e96e069)Five findings, one defect, and it is mine: the
/resetadded in round 3 turned a function that hadonly ever narrowed into one that widens first.
/resetreplaces the DACL with whatever the parent hands down. The comment claimed that couldnot widen anything because "a directory is restricted before anything is written into it" — true
for
<run>/tlsand~/.config/orca, and false for the call that carries the whole fix..orcaisnever restricted; only
.orca/runsis. Measured on a real store:So every
record,attach,replayandpullon Windows reopened the store for a window, andanything created inside it inherited that permanently — icacls does not re-propagate afterwards.
The failure path was worse: the SID lookup sat between the two calls, so a
whoamithat could notrun left the store wide open and aborted the command.
Narrow only
One invocation, every part of it narrowing:
Everything that can fail — the SID lookup, the read of the current DACL, its temporary file —
happens before it. A failure leaves the path exactly as it was found, which answers the ordering
finding and the snapshot finding together: there is no intermediate state left to be caught in.
Two things learned doing it
Explicit trustees only. A directory under
%TEMP%here carries inherited ACEs for accountsthat no longer exist, and
icacls … /inheritance:r /remove *<orphan-SID>exits 1332 andapplies nothing — once inheritance is stripped the SID no longer maps to a name.
/inheritance:rwas always going to remove those, so filtering the removal list to non-inherited ACEs is both the
more accurate ask and how that failure disappears.
/restorewith a hand-written SDDL would have been the tidiest "replace the DACL outright". Itneeds
SeSecurityPrivilegeand fails unelevated. Trustees are instead passed back in whateverspelling the descriptor used —
/removetakes*BUas readily as*S-1-5-32-545— so there is noabbreviation table to fall out of date and silently let a trustee through.
Test
A store under a parent that grants
BUILTIN\Usersinheritably, hammered with child creation whilebeing restricted five times over; no child may carry anything but the three trustees. Red against
the
/resetversion, with six. A detector rather than a proof — it can only fail while such awindow exists, never spuriously.
Windows: 10 failures on main, 9 with this branch, no new ones. All four gates clean.
Review round 5 — the key reached disk before anything protected it (
12c3873)Two findings. The first is a real leak; the second is right about the failure direction and wrong
about the mechanism.
The gateway key was written unprotected
writeConfigwroteconfig.jsonat its final path and narrowed it afterwards.mode: 0o600isdiscarded on Windows, so until that second call the file carries whatever the directory handed
down — and if the call throws, the key is already there. Measured, with
SystemRootunset sorestrictToOwnerfails for a reason that is not specific to the path:Plain text, under the inherited ACL, and the error the operator sees mentions none of it.
Neither of the two shapes the review offered would do, because both destroy the config that was
already there when the restriction fails — truncating in place leaves it empty,
rmon the catchpath deletes it. The key goes through a staging file instead, narrowed while it is still empty,
then renamed over the target:
Two properties measured rather than assumed: rewriting a file keeps its ACL, and a rename over an
existing file carries the ACL of the file being moved. After the change the same failing run
leaves no file at all and the previous config intact.
The descriptor is taken by position, and an unreadable one is refused
The review's mechanism does not hold —
icacls /savewrites the basename, not the full path:so the
/^[OGDS]:/scan cannot match a name line: a Windows filename cannot contain a colon. Theplanted-ACE case removes
BUon a C: path and on a D: path alike.The failure direction is real, though, and that is what changed: an unrecognised line meant
foreigncame back empty, every foreign trustee stayed, andicaclsstill exited 0. For afunction whose job is removing them, "could not parse" must not read as "nothing to remove". The
descriptor is now
lines.at(-1)— right for both forms above — and a line that is not one throws.And a fix to the previous round's test
The window detector deleted each probe directory as it went and raced
icacls's own handle on it,which is how it failed under a full suite rather than how the code failed (
EBUSY … rmdir). Itleaves them now; the tree is a
mkdtemp.Windows: 10 failures on main, 9 with this branch, no new ones. All four gates clean.
Review round 6 — the store's container, and a
SystemRootthat need not be absolute (66555f9)Two findings, both correct. The first turned out to have a sharper mechanism than the finding
describes.
.orcawas left with the workspace's ACLOnly
.orca/runswas narrowed — andprivate-files.tsstated the omission as fact, which was acomment recording a gap instead of closing it. Modify on the parent is enough to delete
runs(achild's own DACL does not decide whether its parent may remove it) and leave a junction there,
which needs no privilege on Windows.
Measured from there:
icaclsdoes not follow the reparse point, which is worse than the finding supposed ratherthan better: the narrowing lands on the junction, the trace lands in someone else's directory under
their ACL, and
restrictToOwnerreports success the whole way.ensureRunsDirnarrows.orcatoo now. The rule that generalises: orca creates it, so orca ownsits protection — the same reason
writeConfignarrows~/.config/orcaand not only the file in it.SystemRootwas trusted for being present, not for being absoluteThe comment above
system32spends a paragraph on why'C:\Windows'cannot be a fallback — theliteral's value is
C:Windows, which CreateProcess resolves against the current directory on C:,which during a recording is the workspace — and then the code accepted exactly that value from the
environment. Removing the literal had closed the spelling and left the shape.
isAbsoluteseparates them exactly:isAbsoluteC:\WindowsC:WindowsWindows''A relative
SystemRootis refused the same way an absent one is, and the message says which it was.Tests
Both red against the reverted implementation:
ensureRunsDir, neither.orcanor.orca/runscarries an inherited entry — on a freshworkspace and on a store that already existed
restrictToOwnerrejectsC:WindowsandWindowswith/not an absolute path/Windows: 10 failures on main, 9 with this branch, no new ones. All four gates clean.
Review rounds 7-9 — kept short, because the commit messages carry the detail
The body above is long enough. These three rounds are summarised; each commit message has the
measurements.
b2f94e5— four findingsquickstartcpreproduces the source's modes, and all 23 shipped assets are100644in git — so the run landed 0644 under a 0755 directory.pullchmods what it installs; quickstart now does too.writeConfigstaging${path}.incomingis one scratch path every writer addresses. Randomised, asBlobStore.putand scrub'scommitalready do.XA/OA/AU. On a descriptor carrying a conditional ACE the old pattern captured["BA"]and nothing else — two foreign trustees kept their access withicaclsexiting 0..orcastopped one being put there and said nothing about one already there.The lasting part of that round is the read-back:
restrictToOwnernow matches the whole descriptor,anchored, against the shape it writes — deliberately not through the parser, which this file had
by then been wrong about twice. An ACE nobody anticipated fails the check rather than being skipped
by it.
Worth recording from measuring the staging one: the interleaving does not reproduce on Windows.
Thirty writers over eight rounds produced no empty and no corrupt config, because Windows refuses
the concurrent open rather than truncating. It is a POSIX shape, which is why it is fixed by
convention and why the test says so instead of implying it caught something.
1201791— the link check could skip itselfrealpath(...).catch(() => undefined)read "I could not resolve this" as "this is not a link", inexactly the conditions the check exists for. It also answered the wrong question: a
substdriveresolves elsewhere without being a link, and the check refused to record at all on a perfectly
normal workspace. Replaced with
lstaton the entry, which asks about the thing that is righthere — so a failure is a real failure and is refused like any other.
7f8cc4f— the ordering held the window openThe check was a check-then-use, and the parent was narrowed last.
restrictToOwnercosts about27ms a path here, measured, so the gap between checking
runsand owning it ran past 50ms with.orcastill granting the workspace's Modify — and Modify on a parent is enough to delete a childwhatever the child's own DACL says.
Now:
.orcais created, checked and narrowed beforerunsexists; only then isrunscreated,checked and narrowed. Once the container grants nobody else anything, the second check-then-use is
not a race.
.orcais checked once more afterrunsis made, because it was itself open between its own checkand its own narrowing. That does not close the window — closing it needs the DACL applied to an open
handle rather than to a path, which node cannot do — but it turns a swap landing there into a
refusal rather than a recording written somewhere else. The comment says that rather than claiming
the race is gone.
Verified end to end, not only by the suite
Fresh workspace; a store that already existed; a junction at
.orcaand at.orca/runs(bothrefused); a
substdrive (still records); four repeat calls; a realorca record --tls-intercept(shell capture recorded
exit_code: 3, model 200, every path in the store at three trustees); andorca quickstart(3/3 turns replayed, store at three trustees).Windows: 10 failures on main, 9 with this branch, no new ones. All four gates clean, CI 6/6.
Rounds 10-11 — and one finding that should stop this branch rather than extend it
5a3e1ac/9efe8cd— the guard's own inputdescriptorOfwroteicacls /saveoutput into amkdtempdirectory, andmkdtempdiscards itsmode on Windows. Measured: two further local accounts with inheritable Modify. That file is the
input to both halves of the only guard
restrictToOwnerhas — the descriptor the/removelist is built from, and the descriptor the read-back compares against. Narrowed now, directly
(going through
restrictToOwnerwould recurse), one directory per process with a random filenameper call, removed on exit.
Two findings in the same round did not hold, and the measurements are in the commit message:
restrictToOwnerdoes not quietly succeed on a container the caller cannot re-DACL (it throws),and
fs.cpdoes not carry the source's DACL on Windows (the copy takes the destination's). Thetest-coverage half of the second was fair and is fixed — the Windows assertion now checks a file
inside the run, which needed the assertion split in two, because a path orca narrows itself may
carry nothing inherited while everything beneath it is protected by inheritance and must.
ec70484— the sibling had been written without the hard-won partsaclScratchwasrestrictToOwnerwith/inheritance:rand/grant:rand no read-back — writteneighty lines below the comment that explains why exiting 0 is not an answer. The grants and the
expected shape are factored now so the two cannot drift again, and the scratch directory gets the
same
vouchFor/stillTheSamepairensureRunsDirgot one commit earlier.One half did not apply and is recorded rather than fixed blind: a freshly
mkdtemp'd directorycarries no explicit ACE — five, every one flagged inherited — so the
/removelist is empty byconstruction there.
The one that is not fixed
A directory another account plants at
.orca, granting the recording account Full Control as itdoes so, passes everything: not a link, narrows successfully, verifies, and
stillTheSamecomparesit with itself. They keep the ownership and with it the implicit WRITE_DAC. Measured:
This also retires a pushback I made a round earlier, where I argued such a container fails closed
for want of WRITE_DAC. It does not, once the planter grants it.
It is not fixed, and the reason is not reluctance.
icacls /setownerexits 5 here even namingthe current account, and 87 when combined with the narrowing. The owner cannot be read either:
/saveemits the DACL only,Get-Aclcannot autoload on this machine, anddir /qgives alocalized-layout name rather than a SID — the shape of parsing that has already gone wrong twice
in this PR.
What I would do with this branch
Split it. The demonstrated vulnerability is one change: mode bits do nothing on Windows, so the run
CA's private key and the gateway API key were readable by every account on the machine, and the
store root now carries an ACL instead. Defending a workspace another local account can write —
junctions, substituted containers, ownership, the TOCTOU around each — is a threat model, and over
eleven commits it has been accreted rather than designed. It deserves its own change, with the last
two findings as its starting point.
74d7706— and the one that outlives the callThe round after that raised the scratch directory again, and most of it had landed while the review
was running. One part had not: the identity check closes the window up to the moment
aclScratchreturns, and does nothing about the rest of the run, where the directory sat empty in
%TEMP%between narrowings. Modify there carries Delete-Subfolders, and Delete-Subfolders on a parent
removes a child whatever the child's own DACL says — an empty child:
One file, written once and kept, is the whole fix, and it is out of reach in turn: it inherits the
directory's ACL, so deleting it would need the right the narrowing just took away.
.orcaand.orca/runswere already safe this way —.orcaholdsrunsand.gitignore, andrunssitsunder a narrowed parent — by luck rather than design, so the comment now says why.