Reject symlink aliases for cancel source and destination#94
Merged
Conversation
Using realpath-with-leaf-dereference on the user-provided mv source and destination lets a symlink aliasing <loop>/state.md or <loop>/cancel-state.md pass authorization. For the destination case the vulnerability is exploitable: `mv <loop>/state.md /tmp/link` (where /tmp/link -> <loop>/cancel-state.md) canonicalizes to the expected path and passes the check, but `mv` replaces the link at /tmp/link rather than creating <loop>/cancel-state.md, corrupting loop state and depositing state.md outside the loop dir. Introduce canonicalize_path_prefix in project-root.sh that resolves symlinks ONLY in the parent directory and preserves the basename verbatim. Symlinked project prefixes (e.g. /var vs /private/var) still match a canonical expected path, but a symlink at the leaf no longer impersonates the real filename. Rewire is_cancel_authorized to use the prefix-only helper for both src and dest, and document why the distinction matters in-place. Update the on-disk src_original probe to reference canonical_loop_dir so the symlink-rejection check runs against the real path rather than any user-supplied non-canonical form. Add two regression tests covering the destination and source symlink alias attacks.
Contributor
Author
|
@codex review this PR |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The main FILE_PATH vs CORRECT_PATH comparison in loop-read-validator.sh and loop-write-validator.sh previously used canonicalize_path, which dereferences symlinks at the leaf. A planted symlink at the correct filename inside the loop dir would then canonicalize to its target and let the validator approve a Read or Write that follows the link out of the loop dir, expanding Claude's effective file-access reach beyond what the hook intends to permit. Switch both validators to canonicalize_path_prefix so a symlinked project ancestor still resolves correctly (the original bug this feature fixes) while a symlink at the leaf no longer impersonates the expected filename. This matches the same discipline applied to is_cancel_authorized and preserves the intended semantics that the basename is compared verbatim. Methodology-analysis checks in these validators intentionally keep full-path realpath because they use prefix-containment (path starts with loop-dir/) rather than equality, which correctly catches symlinks escaping the loop dir.
Contributor
Author
|
@codex review this PR as entirety |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Open
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.
Summary
Follow-up to the canonicalize-paths work already merged into dev via #93. Codex review on that PR flagged a P1 path-restriction bypass: using full-path
realpathon the user-providedmvsource and destination inis_cancel_authorizedlets a symlink aliasing<loop>/state.mdor<loop>/cancel-state.mdpass authorization. For the destination case this is exploitable —mv <loop>/state.md /tmp/linkwhere/tmp/link -> <loop>/cancel-state.mdcanonicalizes to the expected path and passes the check, butmvreplaces the link at/tmp/linkrather than creating<loop>/cancel-state.md, corrupting loop state and depositingstate.mdoutside the loop dir.canonicalize_path_prefixinhooks/lib/project-root.shthat resolves symlinks only in the parent directory and preserves the basename verbatim. Symlinked project prefixes (e.g./varvs/private/var) still match a canonical expected path, but a symlink at the leaf no longer impersonates the real filename.is_cancel_authorizedto use the prefix-only helper for bothsrcanddest. Document the distinction in-place. Update the on-disksrc_originalprobe to referencecanonical_loop_dirso the symlink-rejection check runs against the real path.Test plan
HELPER TEST 9covers the destination symlink alias attackHELPER TEST 10covers the source symlink alias attackHELPER TEST 8(symlinked prefix) still passes, proving prefix resolution still worksbash tests/run-all-tests.sh-> 1725 passed / 0 failed