Repository navigation
CRW-631: open hook trust manifest and hook files inside an os.Root - #585
Merged
Merged
Conversation
hookTrustEntriesReadContained checked a path, opened it by its resolved path, then resolved the path a second time and compared the two observations with os.SameFile. A link swapped to a file outside the plugin root at the open and again at the stat made both observations the outside file, so it was read and listed. Resolve the plugin root once, open it with os.OpenRoot and open the manifest or hook file through the Root: a link that stays inside the root is followed, one that leaves it is refused at the open with the oracle's text. The opened handle is checked with fstat (a directory is EISDIR, any other non-regular file is refused) and read from that handle; the second resolution is gone. The Root is narrower than the old resolution (an absolute link, a link that leaves the root and returns, and a chain of more than 8 links are refused; a dangling link that points outside reports the escape), which is recorded in docs/port-cxc/known-defects.md with the one window that stays: the plugin root's own path is resolved once and opened once. An unexported observe parameter is the test seam of the new table test, which also holds the double swap of the finding.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbb33900da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A review of the Root-based reader found that os.OpenRoot opens the plugin root directory for reading, so a root that the process may search but not read (mode 0111) is refused with a permission error, where opening the file by its path needs search permission only. This follows from using os.Root; record it as the sixth difference in docs/port-cxc/known-defects.md and pin it with a row of the table test (skipped when the tests run as root).
The coordinator refreshed the base. The only conflict was in docs/port-cxc/known-defects.md, where both sides appended lines at the same place (1 block(s)); both sets are kept, dev's lines first, then this branch's.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
hookTrustEntriesReadContained(the reader behindListHookTrustEntries, #567) read a plugin's manifest and hook files after a path check. It opened the file by its resolved path, then resolved the path a second time and compared the two observations withos.SameFile. A link swapped to a file outside the plugin root at the open and again at the stat made both observations that outside file, so its content was read and listed.The reader now resolves the plugin root once, opens it with
os.OpenRoot, opens the manifest or hook file through the Root, checks the opened handle with fstat and reads from that handle. The second resolution is removed.Expected behaviour (acceptance criteria)
plugin manifest symlink escapes plugin root: <ref>, however it is swapped around the check.plugin manifest path must be relative: <ref>, a lexical escapeplugin manifest path escapes plugin root: <ref>(checked before the open, as before); all other failures are engine errors. TheListHookTrustEntriessignature and texts are unchanged.EISDIR, any other file that is not regular is refused; the content is read from the opened handle.entries-oracle.json, the existing swap test and every other existing test pass unedited. No Node, no CGO, noinit()and no package-level initializer that does work.Ported from
CXC v0.2.40 (commit 3c1459ac),
plugins/codexclaw/components/cxc-ops/src/hook-trust.tslines 140-156 (normalizeHookPath,containedPluginFile) and 162-216 (listHookEntries); parity is kept except for the security fix, which was already recorded asport: fixedfor this function (an earlier review finding) and is strengthened here.docs/port-cxc/known-defects.md: the fix line is rewritten and a new last section records what is new.Evidence
Red before, green after. The new table test (
internal/runtime/doctor/hooktrust_entries_race_test.go, packagedoctor) driveshookTrustEntriesReadContainedthrough an unexported trailing parameterobserve func(stage string)that is called with"open"just before the open (nil in production). Row 1 replays the finding: a regular file at the first resolution, a link to the outside file at the open, the regular file at the re-resolution, the link again at the stat.read the file outside the plugin root (error <nil>)for row 1, and the absolute-link row read"inside"where the escape is now expected; the named-pipe row was skipped there because the old body would block on it.observecall fails the swap rows (the seam never observed the open).Commands, all with temporary
HOME,CODEX_HOMEandCRW_HOME,GOFLAGS=-p=4, run on the head1dcca3e(exit codes 0):go test -count=1 ./internal/runtime/doctorand, once,go test -race -count=1 ./internal/runtime/doctorgo test -count=1 -run TestHookTrustEntriesReadContained_rootOpen -v ./internal/runtime/doctor(14 of 14 rows)go test -count=1 ./internal/contracttest -run 'TestDomain/cxc'go vet ./...,GOOS=darwin go vet ./...,go run honnef.co/go/tools/cmd/staticcheck ./internal/runtime/doctor,CGO_ENABLED=0 go build ./...find . -name '*.go' -not -path './.git/*' -exec gofmt -l {} +(no output),git diff --checkandgit diff --check origin/dev...HEADgo run -tags dev ./cmd/crw-dev ci plugin(no--record-version): package digest635105c68ea07f89, 216 files, unchanged from the base;ci validateandci contractspassdevtip builds and itsinternal/runtime/doctortests pass (the sibling change that callsListHookTrustEntriesis on that tip)Hosted CI and the external reviews are listed at the end of this description.
Corpus fixtures (c3)
The fixtures that reach this unit, with their entry points. This PR alone drives none of them (the reader is an internal function; the commands are other changes' work), so it claims none:
contract/notes/cxc/CRW-631.jsonhas emptyidenticalandintentionally-changedlists, like other notes files of this kind.cli__hooks__retrust_needs_bootstrap_then_writescrw hooks retrustcli__hooks__retrust_safety_pin_refuses_all_driftedcrw hooks retrustcli__doctor__codex_bin_and_surface_envcrw doctorThe unit's own replay is the recorded
internal/runtime/doctor/testdata/hooktrust/entries-oracle.json(206 sub-tests), unedited.Diff size
236 changed lines (212 added, 24 removed) against the base: implementation 60 (
hooktrust_entries.go+38/-22), tests 166, documentation 9 (known-defects.md+7/-2), notes 1. No generated data, no verbatim copies, no moved files.Defects and limits (also in
known-defects.md)port: kept: the plugin root path is the anchor of the containment. It is resolved once withEvalSymlinksand opened once withos.OpenRoot, which follows links in the root's own name, so a link swapped into a parent directory of the root between those two calls makes the Root hold another directory. Once the root is open no swap under it leads outside it. The oracle has the same window. Closing it needs a descriptor walk that follows no link (follow-up).port: fixed, consequences of the security fix: the Root refuses an absolute link even when it points inside the root, a relative link that leaves the root and comes back, a chain of more than 8 links (the earlier resolution followed up to 255), a dangling link that points outside the root reports the escape where the oracle reports ENOENT, a file that is not regular is refused, and a plugin root that the process may search but not read (mode 0111) is refused with the permission error becauseos.OpenRootopens the directory for reading (found by the Codex review). The recorded cases use relative in-root links only and pass unchanged.port: kept: a FIFO that nothing holds open for writing still blocks the open itself (already recorded; the test holds the pipe open to reach the refusal).hooktrust_entries_test.go(above the swap test) still describes the removed re-resolution. Existing tests are not edited by this change; a follow-up can fix the comment.known-defects.mdtakes appended sections from several changes, so this branch may show a conflict at the end of that file; it is an append-only conflict.Out of scope
The activation surface (
plugins/crw/wiring/hooks,plugins/crw/skills, every.codex-plugindirectory) is untouched (the plugin digest above is unchanged). No other package, existing test or fixture,go.mod, and no installation, merge or release.Review process disclosure
The plan was audited by an independent reviewer in two rounds and the finished change by a third review, all read-only. Round 1 returned FAIL (a non-regular-file path the plan had excluded, verifiers that did not observe the change, and a document that mixed research with the implementation design); all were folded. The follow-up of that reviewer then ended in a provider refusal (code
cyber_policy, reported and left as recorded), the next managed dispatch started with a configured model that the host'sspawn_agentrejected (no child created, reported and left as recorded), and the global role settings were not changed. Round 2 and the check review therefore ran as explicit model overrides outside the managed dispatch path, with the same model that had served round 1 (gpt-6-astra, xhigh); the architect consultation and round 1 ran through the managed dispatch.Hosted CI and external reviews
1dcca3efbe98e6ed50426b471a18971b4e7aae2f: workflow run 37285087092 (CRW CI, eventworkflow_dispatch, attempt 1; dispatched on the exact head because this branch conflicts withdevonly at the end ofknown-defects.md, an append-only conflict, so GitHub starts nopull_requestrun). All ten jobs succeeded:validate,secrets,go-product (lint),go-product (test-1)to(test-4),go-product (test-rest),go-product (dist)anddev-gate. The first headfbb33900had passed the same ten jobs in run 37283737480.fbb33900; it reviews a pull request once, so the later head has no Devin run.fbb33900(08:37 UTC) with one P2 inline comment, a plugin root that can be searched but not read (verified; recorded as the sixth difference inknown-defects.md, pinned by a test row in1dcca3e, replied to and resolved). Codex Security Review: skipped, "You have reached your Codex usage limits for security reviews."