Skip to content

windows: let the sandbox user stat the folders above a granted path - #661

Open
karljamoralin wants to merge 1 commit into
anthropics:mainfrom
karljamoralin:kjamoralin/srt-win-ancestor-read-attributes
Open

karljamoralin wants to merge 1 commit into
anthropics:mainfrom
karljamoralin:kjamoralin/srt-win-ancestor-read-attributes

Conversation

@karljamoralin

Copy link
Copy Markdown

Fixes part 1 of #657.

Cause

acl grant puts the sandbox user's ACE on each granted path and nothing on the folders above it, so the sandbox user can't stat them. Anything that stats each path component fails: git's repository discovery (fatal: failed to stat 'C:\Users\me\code': Permission denied) and Node's JS fs.realpathSync (EPERM ... lstat).

Listing a folder's parent lets an account read the folder's attributes by path (GetFileAttributesExW, which git uses), but not open a handle to it. CreateFileW always adds SYNCHRONIZE | FILE_READ_ATTRIBUTES to the requested access, so a handle-based stat (libuv where GetFileInformationByName is missing) fails even on the profile root.

Change

  • New ACE kind read_attrs: (A;;FILE_READ_ATTRIBUTES|SYNCHRONIZE;;;<sb>), with no inheritance flags. It's refcounted in working_aces like the other kinds, and is part of KIND_GRANT, so acl revoke and crash recovery remove it. It grants no listing, contents, write or DACL rights.

  • Which ancestors: after a successful grant batch, grant_ancestor_attrs walks each target's ancestors up to, but not including, the volume root. It skips ancestors that are themselves grant targets. A new StatProbe checks whether the sandbox user already holds both bits on each ancestor. The probe runs AuthzAccessCheck with a client context built from the sandbox SID (no logon), on a copy of the DACL that leaves out:

    • every sandbox-SID ACE, so one session's answer never depends on another session's refcounted ACE;
    • Administrators allow ACEs, which the child token holds deny-only.

    A failure on one ancestor logs a warning and releases the hold if this call added it.

  • No subtree walk: a converge triggered by a read_attrs hold (take, release, crash recovery) goes through the new apply_read_attrs. When the sandbox ACEs on disk differ from the wanted ones by at most this ACE, it writes the one object with SetKernelObjectSecurity, or nothing if the folder already matches.

    • It keeps the inherited ACEs and the DACL's protected and auto-inherited bits.
    • Another session taking or releasing the same hold costs no walk, even on a folder that also carries a grant.
    • Every other converge, and junctions and symlinks, take the existing path unchanged, so a grant or deny still re-derives its subtree as before.
    • Other reparse points take the new path. OneDrive's root folder carries a cloud tag, and falling back there would walk the whole OneDrive tree on every grant under it.
    • Why not the obvious writers:
  • Smoke test: new smoke-aces.ps1 row A32 grants a folder under %LOCALAPPDATA%\Temp and checks three things:

    • a sandboxed child can stat each ancestor both by path ([IO.Directory]::Exists) and through a handle (CreateFileW);
    • every sandbox ACE on an ancestor is a non-inheritable ReadAttributes, Synchronize allow;
    • acl revoke leaves none behind.

Overlap: #523 adds a non-propagating writer for object-only ACEs, and #579 changes deny handling in the same functions. This PR doesn't change how grants and denies are written.

Testing

On Windows 11 x64 (26200), with srt-win.exe cross-built for x86_64-pc-windows-gnu at the pinned 1.98.1 (CI builds MSVC), granting C:\Users\<me>\srt657\repo:

before (main) after
git rev-parse in a non-repo allowed folder, via srt-win exec fatal: failed to stat '...\srt657': Permission denied not a git repository
node -e "fs.realpathSync(...)" EPERM ... lstat '...\srt657' resolves the path
fs.readdirSync('..') from the allowed folder EPERM EPERM (no listing granted)
sandboxed CreateFileW(FILE_READ_ATTRIBUTES) on ...\srt657 and on the profile root error 5 on both succeeds on both
icacls on ...\srt657 and the profile root after grant inherited ACEs only adds srt-sandbox:(S,RA) without (OI)(CI)
icacls after acl revoke as before the grant; profile root SDDL control unchanged (D:P)
acl grant / acl revoke time 30-70 ms / 25-75 ms
second holder granting the same folder 32 ms; the profile root ACE stays until the last holder revokes
A32 run by hand through srt-win exec STAT_FAIL on Temp, Local, AppData and a handle failure on the profile root pass; revoke releases 5 paths

New unit tests:

  • read_attrs_is_object_only_and_does_not_propagate covers:
    • the ACE's exact type, flags and mask;
    • inherited ACEs and DACL control bits kept on an unprotected and a protected folder;
    • a marker inheritable ACE never reaching the child on add, re-add, release, a toggle next to an inheritable grant, or a second read_attrs hold next to that grant;
    • a full converge next to that grant still propagating;
    • a junction taking the existing path without error;
    • a folder with a non-surrogate reparse tag written in place, not re-derived from its parent (fails with the old any-reparse-point fallback);
    • a propagating control.
  • stat_probe_ignores_sandbox_aces checks that the probe ignores its own ACE, needs both bits, and doesn't count listing the parent.

cargo clippy --all-targets -D warnings and cargo fmt --check are clean, and all 55 lib tests pass on the box. I have not run smoke-aces.ps1 locally, because it reinstalls with the test sublayer; A32 runs in CI.

Known limits:

  • An older srt-win reading a state DB that holds read_attrs rows fails to parse them, so downgrading mid-session is not supported.
  • When a granted ancestor is renamed while two sessions hold it, releasing one session strips the moved folder's ACE for both. This is the existing moved-grant handling and it fails closed.

This branch has not been deployed

No deployments
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.

1 participant