fix(op): contain raw-file output paths inside the package directory - #1165
Conversation
`extract_raw_file` had no containment check at all. It did
let rel_path = path.strip_prefix('/').unwrap_or(path);
let candidate = dir.path().join(rel_path);
if !candidate.exists() { continue; }
return deliver_file(&candidate, sink);
`strip_prefix('/')` removes one leading slash and nothing else, so `..`
survived untouched into the join and walked straight out of the cache
directory. `.exists()` then greenlit the result and `deliver_file` streamed
it back to the caller.
The path comes from `[outputs.x] type = "raw-file"` in `minimal.toml`, which
the daemon reads out of a client-uploaded workspace, so it is
attacker-influenceable: `path = "../../../etc/shadow"` is a daemon-side
arbitrary file read delivered over the client's own channel. No symlink
needed, and it bypasses `lcache::LocalDir`'s guards entirely by taking the
raw `&Path` from `DirCacheEntry::path()`.
Two checks, because either alone is insufficient:
- Lexically normalize the request and reject an escape before touching the
filesystem. Covers both `../../etc/passwd` and `/../../etc/passwd`.
- Decide containment on the *resolved* path too: a package shipping
`escape -> /etc` makes `<pkg>/escape/passwd` lexically contained while
resolving outside it.
A containment violation is an `Err`, deliberately not a `continue`:
falling through to the next package would hide the fact that a package
tried to serve a file it does not own, and make the refusal indistinguishable
from a cache miss.
`common::archive::normalize_within_root` becomes public rather than gaining a
second implementation here — that is how this class of bug got reintroduced
before (#651). Its doc now says plainly that it is lexical, so a caller that
touches the filesystem must also check the resolved path.
Regression tests verified to fail without the fix; the symlink case delivers
10 bytes of a file outside the package when the check is removed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
Found by the audit that followed #1162.
op::materialize::extract_raw_filehad no containment check at all.strip_prefixremoves one leading slash and nothing else, so..survives into the join and walks out of the cache dir;.exists()then greenlights it anddeliver_filestreams it back.Impact
The path comes from
[outputs.x] type = "raw-file"inminimal.toml, which the daemon reads out of a client-uploaded workspace — so it is attacker-influenceable:That is a daemon-side arbitrary file read, delivered over the client its own channel. No symlink required. It also bypasses
lcache::LocalDirentirely by taking the raw&PathfromDirCacheEntry::path().Fix
Two checks, because neither alone suffices:
../../etc/passwdand/../../etc/passwd.escape -> /etcmakes<pkg>/escape/passwdlexically contained while resolving outside, so containment is also decided on the canonicalized path.A violation is an
Err, deliberately not acontinue: falling through to the next package would hide that a package tried to serve a file it does not own, and make the refusal indistinguishable from a cache miss.common::archive::normalize_within_rootis made public rather than growing a second implementation here — a second copy is exactly how this class got reintroduced before (#651). Its docs now state that it is lexical-only, so a caller touching the filesystem must also check the resolved path.Verification
Three regression cases, all verified to fail without the fix — with the check neutralized the symlink case returns
Report { bytes: 10 }, i.e. it really does deliver a file from outside the package.cargo test -p op -p commongreen (44 + 19). Pre-existingsandbox2clippy noise on macOS is unrelated (identical count on a clean tree; that code is Linux-only and cfg-d out here).Independent of #1162 and #1164.
Note
Reject raw-file output paths that escape the package directory in
extract_raw_filecommon::archive::normalize_within_rootto reject paths with..components or absolute paths before joining.archive::normalize_within_rootin archive.rs public so it can be reused cross-crate.Macroscope summarized 7557a55.