fix: path traversal vulnerability in symlink destinations#280
fix: path traversal vulnerability in symlink destinations#280yacosta738 merged 10 commits intomainfrom
Conversation
- Implement `ensure_safe_destination` to reject absolute paths and `..` components. - Apply validation to all sync types: Symlink, SymlinkContents, NestedGlob, and ModuleMap. - Add integration tests verifying rejection of malicious destination paths. - Harden `clean` operation with the same path validation logic. - Remove redundant validation in ModuleMap. Co-authored-by: yacosta738 <33158051+yacosta738@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded strict destination validation via Linker::ensure_safe_destination and applied it across symlink-related sync flows, nested-glob expansion, module-map handling, and cleanup; removed an old helper; added Unix-only security regression tests for traversal/absolute-path/symlink-escape cases; added a journal entry documenting the issue and mitigations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/journal/sentinel.md:
- Around line 3-4: The H2 "2025-05-15 - Path Traversal in Symlink Destinations"
is missing a blank line which triggers markdownlint MD022; add a single blank
line immediately after that H2 line in .agents/journal/sentinel.md so the
heading is followed by an empty line before the paragraph that begins with
"**Vulnerability:**".
In `@src/linker.rs`:
- Around line 246-248: The current code only validates the destination template
(self.ensure_safe_destination(&target.destination)) before expansion, but some
templates expand to different paths (e.g., ".") so you must validate each
expanded path: after calling expand_destination_template(...) (the place that
produces dest_str/PathBuf), call
self.ensure_safe_destination(&dest_str_or_pathbuf) and use the returned PathBuf
for the subsequent logic instead of the original template; apply the same change
in both the sync() closure and the mirrored clean() closure so both code paths
validate the expanded nested-glob paths and use the validated PathBuf.
- Around line 90-112: The current ensure_safe_destination method only performs
lexical checks and can be bypassed via symlinked ancestors; resolve this by
canonicalizing paths and verifying the resolved destination remains inside the
canonical project root: either canonicalize and store self.project_root (using
canonicalize_cached or std::fs::canonicalize) when the struct is initialized, or
canonicalize the result of self.project_root.join(path) inside
ensure_safe_destination and then assert the canonicalized destination
starts_with the canonical project root before returning; update
ensure_safe_destination to use the canonicalized paths and return the resolved
PathBuf to prevent symlink-ancestor escapes.
- Around line 90-112: The ensure_safe_destination method currently allows empty
destinations and Windows root/drive-relative paths; update
ensure_safe_destination to reject empty/zero "normal" components and to bail if
any path component is RootDir or Prefix (in addition to ParentDir and
is_absolute), mirroring the pattern used in install.rs; specifically, check that
Path::components().filter(|c| matches!(c,
Component::Normal(_))).next().is_some() (i.e., not empty) and that no component
matches Component::RootDir or Component::Prefix before returning
self.project_root.join(path).
In `@tests/security_repro/mod.rs`:
- Around line 17-29: The test uses hard-coded malicious destinations
("../traversal.md" and "/tmp/malicious.md") which can collide across runs;
update the code that builds config_content (the multi-line TOML string used
alongside config_path) to construct unique paths from the test's temp_dir (e.g.,
temp_dir.join("traversal.md") or temp_dir.join("malicious.md")) and inject them
via format! (or equivalent) into the TOML so each test gets a unique
sibling/absolute path; apply the same change for the other occurrences mentioned
(the other config_content blocks and any places that assert or remove those
files) so references to agents.malicious.targets.* use the dynamically
constructed path instead of the hard-coded literal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 312228b0-7a0b-497d-ad3e-937447853233
📒 Files selected for processing (4)
.agents/journal/sentinel.mdsrc/linker.rstests/all_tests.rstests/security_repro/mod.rs
yacosta738
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/security_repro/mod.rs`:
- Around line 118-163: The test test_symlinked_destination_ancestor_is_rejected
creates escaped_dir outside the TempDir and never removes it; add deterministic
cleanup for escaped_dir (or keep it inside temp_dir) so leftover directories
aren't left on the filesystem. Modify the test to either create escaped_dir as a
TempDir (or TempDir::new_in(...)) so it is auto-cleaned, or wrap the test body
in a scope/guard and call fs::remove_dir_all(&escaped_dir) in a finally-style
block (e.g., via a Drop guard or catch_unwind) to ensure escaped_dir is removed
on both success and panic; reference escaped_dir, temp_dir, symlink and the test
function name to locate where to add the cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad575f78-045d-4d18-8ac4-ff62a427fbdf
📒 Files selected for processing (3)
.agents/journal/sentinel.mdsrc/linker.rstests/security_repro/mod.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/security_repro/mod.rs`:
- Around line 50-68: Update the test to detect any filesystem entry that escaped
the project root (not just symlinks) by checking whether traversal_file's
canonicalized path is outside the expected root and fail if so; remove the stale
comments that describe pre-fix vulnerable behavior and clarify the expectation,
and replace the unconditional result.unwrap() with handling that accepts either
an Err or an Ok(SyncResult) with sync_result.errors > 0 (using the existing
variables result and sync_result) while ensuring proper cleanup of the
traversal_file (remove_file or remove_dir as appropriate) instead of only
attempting fs::remove_file on symlinks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e9f4188f-2b42-4e81-996b-063029894cba
📒 Files selected for processing (1)
tests/security_repro/mod.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/linker.rs`:
- Around line 120-156: The helper performs a TOCTOU-prone preflight check and
then returns the lexical path `joined`, which can be swapped before later
filesystem mutations (`create_dir_all`, `rename`, `remove_file`, symlink).
Instead of returning `joined`, anchor operations to a project-root handle: open
`self.project_root` as a directory handle (e.g., cap-std or openat-style
Dir/File) and return or expose that handle (or a relative path + dir handle) so
subsequent mutations use openat-style calls relative to the canonicalized
`canonical_project_root`/`canonical_ancestor` (use `canonicalize_cached`,
`existing_ancestor`, and `joined` only to validate, but perform
`create_dir_all`/`rename`/`remove_file`/symlink via the dir handle to eliminate
the TOCTOU window). Ensure callers of this helper are updated to accept/use the
dir handle for all filesystem mutations.
- Around line 131-147: The security check in ensure_safe_destination() must not
use the shared cache; replace the calls to
self.canonicalize_cached(&self.project_root) and
self.canonicalize_cached(existing_ancestor) with a fresh, non-cached
canonicalization (e.g., use a direct canonicalize variant or a new method that
does not consult the Linker cache) so each authorization decision computes
current realpaths anew; update the two bindings canonical_project_root and
canonical_ancestor in ensure_safe_destination() to call the uncached
canonicalizer instead of canonicalize_cached().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
1 participant
Severity: CRITICAL
Vulnerability
Path traversal and absolute path usage in symlink destinations derived from untrusted
agentsync.tomlconfiguration.Impact
A malicious configuration could cause AgentSync to create symbolic links at arbitrary locations on the user's filesystem, potentially overwriting sensitive files (e.g.,
.ssh/authorized_keys,.bashrc) or creating bait files outside the project root.Fix
Introduced a centralized
ensure_safe_destinationmethod inLinkerthat usesPath::components()to ensure all destination paths are relative and remain within the project root (no..components). This validation is applied to all target synchronization and cleanup operations.Source
src/linker.rs:ensure_safe_destination,process_target,clean, andprocess_module_map.Verification
tests/security_repro/specifically targeting traversal and absolute paths.PR created automatically by Jules for task 8485299863779722211 started by @yacosta738