Skip to content

fix(permissions): judge a write by where its links lead - #189

Open
TheCromazone wants to merge 1 commit into
jadenfix:mainfrom
TheCromazone:fix/symlink-write-boundary
Open

TheCromazone wants to merge 1 commit into
jadenfix:mainfrom
TheCromazone:fix/symlink-write-boundary

Conversation

@TheCromazone

Copy link
Copy Markdown
Contributor

What was wrong or missing

Project writes were judged by the path as written. Ctx::resolve normalises it lexically and writable tests it with starts_with, then write_file and edit_file hand the original path to the filesystem, which follows links. So a symlink inside the project that points outside it let a write that looks local land somewhere else, without the approval that writes outside the project need (#144). A .. after a link did the same, because normalising applied it before the link: with system -> /usr, system/../etc/x looks like ./etc/x but writes /etc/x.

I reproduced it on main (5d6e269) with the new end-to-end test below: a project with escape -> ~/outside and notes.md -> ~/outside/notes.md, and a scripted model that writes escape/new.txt and edits notes.md in auto mode. On main the test fails with "a write followed a link out of the project". /permissions check shows the same for a redirect:

main
› /permissions check echo planted > escape/new.txt
  ✓ runs · changes the project · auto mode

this branch
› /permissions check echo planted > escape/new.txt
  ? asks first · writes outside the project through a link: escape/new.txt → /private/tmp/…/home/outside/new.txt · auto mode
› /permissions check echo ok > kept.txt
  ✓ runs · changes the project · auto mode

What this changes, and why this way

When a write is lexically inside the project or a temp directory, rusty now also works out where it really lands, the way the system resolves it: part by part, replacing each link with its target as it is reached, keeping a last link whose target doesn't exist yet (a write would create it), and taking .. from what is really there. It uses read_link rather than canonicalize, so it handles paths that don't exist yet. If the result is outside the (also resolved) project root and its temp directories, the call is judged as a write outside the project: it asks in auto and ask mode, is refused in read-only mode, and runs in yolo, the same as any other write outside the project. The reason names both paths: writes outside the project through a link: escape/new.txt → /Users/me/outside/new.txt.

Links that stay inside the project are unaffected, as are links into a temp directory. On macOS /tmp, /var/tmp and $TMPDIR live under /private, so the temp roots and home are resolved the same way before comparing; otherwise ordinary temp writes would have started asking. A link loop stops after 40 links, as the system does, and is treated as a plain name; the write itself then fails.

This covers write_file, edit_file and the shell writes that already go through the same check: redirects, dd of= and dump output files. To keep the scope clear, as #145 asks, here is what it doesn't do:

  • It isn't race-resistant. The link is checked when the call is judged, so a link swapped between then and the write isn't caught. Closing that would mean opening files without following links (O_NOFOLLOW and friends) in the tools, which is a bigger change I'd rather discuss first.
  • It isn't a shell sandbox. Other shell commands that write, such as cp, mv and scripts, are still judged lexically.

The README's "Asks first" list says the same in one line.

How I checked it

  • New permissions::tests::writes_go_where_links_lead, against a temp-directory fixture with links to a directory outside (/usr), a file outside (/etc/hosts), a dangling target outside, links that stay inside (absolute and relative), and a loop. It checks that each escaping write is risky with the link reason (including .. after a link in both directions), that the in-project and loop cases stay ordinary workspace writes, that redirects take the same check, and what auto, ask, read-only and yolo decide. It only classifies; nothing outside the fixture is written. With the new check switched off, it fails on the first escaping case.
  • New a_link_out_of_the_project_does_not_carry_a_write_with_it drives the real binary in auto mode with a scripted model that writes through escape -> ~/outside and edits notes.md -> ~/outside/notes.md, then writes an ordinary file. Both escaping calls are refused, the outside fixture is unchanged, the ordinary write lands, and /permissions check echo planted > escape/new.txt shows the link reason.
  • cargo xtask qa --full passes on this branch on macOS: format, lint, tests, binaries, memory, terminal and terminal-inspection.

Fixes #144
Refs #145

Project writes were judged by the path as written: it was normalised
lexically and tested with starts_with, then written through the normal
filesystem. A symlink inside the project that points outside it, to a
directory, a file, or a target that doesn't exist yet, therefore let
write_file, edit_file and shell redirects change files outside the
project without the approval such writes need. A ".." after a link was
applied before the link, so it could leave the project the same way.

Before a write that is lexically inside the project or a temp directory
is allowed, rusty now resolves the path the way the system will: part by
part, replacing each link with its target, keeping a last link whose
target a write would create, and applying ".." to what is really there.
If that lands outside the project and its temp directories, the call is
judged as a write outside the project and asks, or is refused in
read-only mode. Links that stay inside the project are unaffected.

The check happens when the call is judged, so a link swapped before the
write is not caught; it is a boundary against mistakes, not a sandbox,
and other shell commands that write (cp, mv, scripts) are not covered.

Refs: jadenfix#144, jadenfix#145

@jadenfix jadenfix left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a real hole and real_path handles it carefully: it resolves component by component including the last, applies .. after a link as the system does, resolves dangling links to where the write would land, caps the loop at 40 links, and resolves the roots too so normal in-workspace writes aren't denied. The README is honest about the check-then-write gap.

The check only covers some write paths, though. Common shell writes and hard links still get around it, and those should be closed before this lands. Details inline.

Comment thread src/permissions.rs
if !ctx.writable(path) {
return risky(format!("writes outside the project: {path}"));
}
if let Some(real) = ctx.leaves_through_link(path) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only write_file/edit_file, redirects, dd of= and pg_dump -f reach this. For cp/ln/install, file_tool still checks only ctx.writable, and mv/tee/touch/truncate go through paths_inside. So ln -s ~/.ssh k followed by cp key k/authorized_keys or tee k/x is still allowed as a workspace write. Please route those destinations through leaves_through_link too.

Related: a single command like ln -s ~ h && echo x >> h/.zshrc is judged before the link exists, so the redirect passes. Treating an ln -s whose target is outside the project as risky would cover it.

Comment thread src/permissions.rs
/// by part, each link replaced by its target as it is reached (a last link
/// too, whose target a write would create) and `..` taken from what is
/// really there. Parts that don't exist yet are kept as written.
fn real_path(p: &Path) -> PathBuf {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard links aren't covered. On macOS, home and the project are usually on the same volume, so after ln ~/.zshrc z (allowed, since only the destination is checked), edit_file z modifies ~/.zshrc. Checking nlink > 1 on an existing regular file (or refusing ln without -s for sources outside the project) would catch this.

Comment thread README.md
- deploys, `terraform apply`, `kubectl apply`;
- opening or merging pull requests;
- writes outside the project;
- writes outside the project, including a write that a symlink inside it

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"file tools ... follow the link" reads as if it covers the shell file commands too (cp, mv, tee), which this doesn't. Please name write_file/edit_file explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(permissions): resolve filesystem boundaries before allowing project writes

2 participants