From b769a9a73a9d69bb7a88ea7c158439528bec4798 Mon Sep 17 00:00:00 2001 From: ant-kurt <209710463+ant-kurt@users.noreply.github.com> Date: Thu, 20 Aug 2026 22:56:01 -0700 Subject: [PATCH] windows: recompute deny set per exec and pin .git ancestor chain Mandatory and configured denies are computed at wrap time and passed through srt-win exec --deny-*, held under the exec PID and released when it ends. Session initialize only applies grants. srt-win pins each real ancestor between a deny target and its modify-grant root with an object-only DELETE|WRITE_DAC deny. --- README.md | 2 + src/sandbox/sandbox-manager.ts | 210 ++++++++------------- src/sandbox/windows-sandbox-utils.ts | 61 ++++++ test/sandbox/windows-per-exec-deny.test.ts | 160 ++++++++++++++++ vendor/srt-win-src/src/acl.rs | 32 +++- vendor/srt-win-src/src/cli.rs | 22 ++- vendor/srt-win-src/src/state_db.rs | 50 ++++- 7 files changed, 387 insertions(+), 150 deletions(-) create mode 100644 test/sandbox/windows-per-exec-deny.test.ts diff --git a/README.md b/README.md index 14d89686..2646b93e 100644 --- a/README.md +++ b/README.md @@ -678,6 +678,8 @@ $ srt 'echo "bad" > .git/hooks/pre-commit' /bin/bash: .git/hooks/pre-commit: Operation not permitted ``` +**Note (Windows):** On Windows, mandatory deny paths cover existing files only and are rescanned per command (paths up to `mandatoryDenySearchDepth` components below the working directory, default 3, same counting as Linux, skipping `node_modules`), with the deny ACEs applied for the command's lifetime. A repo's `.git` directory and its ancestors up to the write-granted root are also pinned against rename/delete for the command's duration. + **Note (Linux):** On Linux, mandatory deny paths only block files that already exist. Non-existent files in these patterns cannot be blocked by bubblewrap's bind-mount approach. macOS uses glob patterns which block both existing and new files. **Linux search depth:** On Linux, the sandbox uses `ripgrep` to scan for dangerous files in subdirectories within allowed write paths. By default, it searches up to 3 levels deep for performance. You can configure this with `mandatoryDenySearchDepth`: diff --git a/src/sandbox/sandbox-manager.ts b/src/sandbox/sandbox-manager.ts index a7d57319..c832a454 100644 --- a/src/sandbox/sandbox-manager.ts +++ b/src/sandbox/sandbox-manager.ts @@ -61,7 +61,7 @@ import { wrapCommandWithSandboxWindows, parseWindowsBinShell, expandWindowsFsPaths, - stampWindowsAcl, + windowsGetMandatoryDenyPaths, restoreWindowsAcl, grantWindowsAcl, revokeWindowsAcl, @@ -809,9 +809,6 @@ async function initialize( // catch's best-effort revoke/restore can address whatever // partially landed. windowsFsSbUserSid = sb - // Grant FIRST so the sandbox user has working-tree access by - // the time the deny stamp runs. The two are independent - // refcounted state-DB sets keyed on the same holder PID. if (acc.grantRead.length > 0 || acc.grantWrite.length > 0) { grantWindowsAcl({ sandboxUserSid: sb, @@ -819,33 +816,12 @@ async function initialize( write: acc.grantWrite, srtWin, }) - } - if (acc.denyRead.length > 0 || acc.denyWrite.length > 0) { - stampWindowsAcl({ - sandboxUserSid: sb, - denyRead: acc.denyRead, - denyWrite: acc.denyWrite, - srtWin, - }) - } - // Only record when something was actually applied — gates - // running revoke/restore at reset(). Recorded AFTER success — - // the catch below clears `config`, and a non-undefined - // stampedSet would leave reset()/updateConfig() seeing state - // that never landed. - const anyApplied = - acc.grantRead.length > 0 || - acc.grantWrite.length > 0 || - acc.denyRead.length > 0 || - acc.denyWrite.length > 0 - if (anyApplied) { + // Recorded after success: the catch below clears `config`. windowsFsStampedSet = acc logForDebugging( - `[Sandbox Windows] fs applied: ` + + `[Sandbox Windows] fs granted: ` + `${acc.grantWrite.length} grantWrite, ` + - `${acc.grantRead.length} grantRead, ` + - `${acc.denyRead.length} denyRead, ` + - `${acc.denyWrite.length} denyWrite`, + `${acc.grantRead.length} grantRead`, ) } windowsFsRawInputs = rawWindowsFsInputs(runtimeConfig) @@ -1257,51 +1233,21 @@ function getFsWriteConfig(): FsWriteRestrictionConfig { } /** - * Build the Windows file-access set (deny stamps + sandbox-user - * grants) from `runtimeConfig`. Globs are expanded to concrete - * paths (point-in-time — a path appearing after this returns is NOT - * covered). Directory targets are accepted (the `(OI)(CI)` ACEs - * cover the subtree). - * - * The sandbox user has no inherent rights on real-user-owned files, - * so `allowWrite` (the working-tree roots) becomes a per-session - * `MODIFY_NO_FDC` ALLOW ACE for ``, `allowRead` a - * `READ|EXECUTE` ALLOW ACE, and `denyRead`/`denyWrite` become an - * explicit DENY ACE for `` on the target plus a - * `(OI)(CI) FILE_DELETE_CHILD` DENY on its parent. + * Session-level grant set: `allowWrite` → `MODIFY_NO_FDC`, `allowRead` + * → `READ|EXECUTE` ALLOW ACEs for ``, globs expanded at + * initialize(). Denies are per exec — {@link computeWindowsPerExecDenySet}. */ function computeWindowsFsAccessSet(c: SandboxRuntimeConfig): { grantRead: string[] grantWrite: string[] - denyRead: string[] - denyWrite: string[] } { const fs = c.filesystem // filesystem.disabled bypasses ALL filesystem rule generation — - // same as the macOS/Linux wrapWithSandbox path (readConfig / - // writeConfig left undefined). On Windows this means no ACL - // stamp/grant; credential FILE denies are dropped along with the - // rest (credential ENV: mode:'deny' is structural under the - // fresh srt-sandbox env; mode:'mask' sentinels are passed via - // the --env overlay). + // same as the macOS/Linux wrapWithSandbox path. if (fs?.disabled) { - return { grantRead: [], grantWrite: [], denyRead: [], denyWrite: [] } + return { grantRead: [], grantWrite: [] } } const expand = expandWindowsFsPaths - // `mode: 'deny'` — non-existent literals reach srt-win, which - // creates a placeholder chain and stamps it (deny lands on the - // exact target path). `mode: 'grant'` drops them (a grant on - // nothing is meaningless). - const denyRead = expand( - [ - ...new Set([ - ...(fs?.denyRead ?? []), - ...getCredentialDenyReadPaths(c.credentials), - ]), - ], - { mode: 'deny' }, - ) - const denyWrite = expand(fs?.denyWrite ?? [], { mode: 'deny' }) return { // `allowRead` also serves as `allowWithinDeny`: a file under a // denied dir gets an explicit ALLOW ACE for the sandbox user, @@ -1309,11 +1255,53 @@ function computeWindowsFsAccessSet(c: SandboxRuntimeConfig): { // the recompose chokepoint orders deny-before-allow per-path. grantRead: expand(fs?.allowRead ?? [], { mode: 'grant' }), grantWrite: expand(fs?.allowWrite ?? [], { mode: 'grant' }), - denyRead, - denyWrite, } } +/** + * Deny set for one exec: session + per-exec `denyRead`/`denyWrite`, + * credential files, and the mandatory set under `cwd`. Applied via + * `srt-win exec --deny-*` under the exec's PID, so a `.git` the host + * creates or rewrites between commands is covered by the next one. + */ +export function computeWindowsPerExecDenySet( + c: SandboxRuntimeConfig | undefined, + custom: Partial | undefined, + cwd: string, +): { denyRead: string[]; denyWrite: string[] } { + const sessFs = c?.filesystem + const fsCfg = custom?.filesystem + if (sessFs?.disabled || fsCfg?.disabled) { + return { denyRead: [], denyWrite: [] } + } + const expand = expandWindowsFsPaths + const denyRead = expand( + [ + ...new Set([ + ...(sessFs?.denyRead ?? []), + ...(fsCfg?.denyRead ?? []), + ...getCredentialDenyReadPaths(c?.credentials), + ...getCredentialDenyReadPaths(custom?.credentials), + ]), + ], + { mode: 'deny' }, + ) + const mandatory = windowsGetMandatoryDenyPaths(cwd, { + maxDepth: c?.mandatoryDenySearchDepth ?? 3, + allowGitConfig: c?.filesystem?.allowGitConfig ?? false, + }) + const read = new Set(denyRead) + const denyWrite = [ + ...new Set([ + ...expand([...(sessFs?.denyWrite ?? []), ...(fsCfg?.denyWrite ?? [])], { + mode: 'deny', + }), + ...mandatory, + ]), + ].filter(p => !read.has(p)) + return { denyRead, denyWrite } +} + /** * Snapshot the raw config fields that feed * {@link computeWindowsFsAccessSet}. Used by updateConfig() to @@ -1322,15 +1310,10 @@ function computeWindowsFsAccessSet(c: SandboxRuntimeConfig): { */ function rawWindowsFsInputs(c: SandboxRuntimeConfig) { // Keyed exactly on what {@link computeWindowsFsAccessSet} reads. - // `network.allowedDomains` does NOT feed file-deny (only mask - // injectHosts), so a network-only updateConfig hits the cache. return { disabled: c.filesystem.disabled ?? false, - denyRead: [...c.filesystem.denyRead], - denyWrite: [...c.filesystem.denyWrite], allowRead: [...(c.filesystem.allowRead ?? [])], allowWrite: [...c.filesystem.allowWrite], - credFiles: getCredentialDenyReadPaths(c.credentials), } } @@ -1346,17 +1329,14 @@ function sameRawWindowsFsInputs( ): boolean { return ( a.disabled === b.disabled && - setEq(a.denyRead, b.denyRead) && - setEq(a.denyWrite, b.denyWrite) && setEq(a.allowRead, b.allowRead) && - setEq(a.allowWrite, b.allowWrite) && - setEq(a.credFiles, b.credFiles) + setEq(a.allowWrite, b.allowWrite) ) } /** - * True when `newConfig`'s file-deny inputs match what was - * stamped at initialize(). Compares raw inputs only (cheap, + * True when `newConfig`'s grant inputs match what was applied at + * initialize(). Compares raw inputs only (cheap, * order-insensitive); never re-expands globs — updateConfig is * warn-only on Windows and the resolved set wouldn't be used. */ @@ -1789,66 +1769,28 @@ async function wrapWithSandboxArgv( customConfig?.credentials ?? config?.credentials, customConfig?.network?.allowedDomains ?? config?.network?.allowedDomains, ) - // Per-exec FILE denies (customConfig only — the session-level - // config's denies were already stamped at initialize()). - // Paths go through `expandWindowsFsPaths` — the SAME - // chokepoint the session-level set uses (point-in-time glob - // expand, normalize, missing→drop) — so a per-exec entry - // resolves identically to its session-level equivalent. - // macOS/Linux per-exec already reuses session-level expansion; - // Windows now matches. - // - // The dedup against `windowsFsStampedSet` is an OPTIMIZATION, - // not a correctness gate: re-stamping a session-held path - // under the exec's distinct holder is refcount-safe but wastes - // a SetSecurityInfo round-trip. - // - // filesystem.disabled bypasses ALL filesystem rule generation - // — including credential-derived file denies — same ordering - // as session-level `computeWindowsFsAccessSet` (credential - // ENV: mode:'deny' is structural under the fresh srt-sandbox - // env; mode:'mask' sentinels are passed via the --env - // overlay). // Per-exec allowRead/allowWrite throw — `srt-win exec` only // exposes `--deny-*`; per-exec grants are not implemented. const fsCfg = customConfig?.filesystem - let perExecDenyRead: string[] = [] - let perExecDenyWrite: string[] = [] - if (!fsCfg?.disabled) { - if (fsCfg?.allowRead?.length || fsCfg?.allowWrite?.length) { - throw new Error( - `Per-exec filesystem.allowRead/allowWrite is not supported ` + - `on Windows — \`srt-win exec\` only exposes per-exec ` + - `denies. Set them at the session level (initialize()).`, - ) - } - const rawRead = [ - ...(fsCfg?.denyRead ?? []), - ...getCredentialDenyReadPaths(customConfig?.credentials), - ] - const rawWrite = fsCfg?.denyWrite ?? [] - // Skip on the dominant path (no per-exec fs or - // credential-file deny). - if (rawRead.length > 0 || rawWrite.length > 0) { - const sessRead = new Set(windowsFsStampedSet?.denyRead ?? []) - const sessWrite = new Set(windowsFsStampedSet?.denyWrite ?? []) - const expand = expandWindowsFsPaths - perExecDenyRead = expand(rawRead, { mode: 'deny' }).filter( - p => !sessRead.has(p), - ) - perExecDenyWrite = expand(rawWrite, { mode: 'deny' }).filter( - p => !sessRead.has(p) && !sessWrite.has(p), - ) - } + if ( + !fsCfg?.disabled && + (fsCfg?.allowRead?.length || fsCfg?.allowWrite?.length) + ) { + throw new Error( + `Per-exec filesystem.allowRead/allowWrite is not supported ` + + `on Windows — \`srt-win exec\` only exposes per-exec ` + + `denies. Set them at the session level (initialize()).`, + ) } + const perExec = computeWindowsPerExecDenySet( + config, + customConfig, + cwd ?? process.cwd(), + ) // Per-exec deny rides on argv (`acl stamp` reads stdin, but // exec's stdin belongs to the child). The CreateProcessW // length check lives in `wrapCommandWithSandboxWindows` // where the full argv (incl. shell + user command) is known. - // - // The `denyReadPaths` half of the SESSION-level credentials - // is already unioned into the stamp set at initialize() time - // via `computeWindowsFsAccessSet`. registerCommandText(command, options) return wrapCommandWithSandboxWindows({ command, @@ -1861,8 +1803,8 @@ async function wrapWithSandboxArgv( // passed via the --env overlay so the sandboxed child sees // the sentinel value, same as macOS/Linux. setEnvVars: credentialRestrictions.setEnvVars, - denyRead: perExecDenyRead, - denyWrite: perExecDenyWrite, + denyRead: perExec.denyRead, + denyWrite: perExec.denyWrite, // safe.directory: cwd + the resolved session-level write // grants + explicit git.safeDirectories — the working-tree // roots the sandbox user has MODIFY on plus any repo top-level @@ -1931,10 +1873,10 @@ function updateConfig(newConfig: SandboxRuntimeConfig): void { !sameWindowsStampSet(newConfig) ) { logForDebugging( - `[Sandbox Windows] updateConfig: the resolved file-access set ` + - `(filesystem.* ∪ credentials.files) changed but the ACL ` + - `stamp/grant is session-wide — call reset() then initialize() ` + - `to apply. The previously-applied set stays in effect.`, + `[Sandbox Windows] updateConfig: filesystem.allowRead/allowWrite ` + + `changed but the ACL grant is session-wide — call reset() then ` + + `initialize() to apply. The previously-applied grants stay in ` + + `effect; denies are recomputed per command.`, { level: 'warn' }, ) } diff --git a/src/sandbox/windows-sandbox-utils.ts b/src/sandbox/windows-sandbox-utils.ts index 02124721..7faef178 100644 --- a/src/sandbox/windows-sandbox-utils.ts +++ b/src/sandbox/windows-sandbox-utils.ts @@ -13,6 +13,9 @@ import { containsGlobCharsWin, expandGlobPattern, isUncPath, + DANGEROUS_FILES, + getDangerousDirectories, + normalizeCaseForComparison, } from './sandbox-utils.js' // Re-export so existing tests (glob-expand.test.ts) and any // out-of-tree caller keep their import path. `buildGitConfigEnv` is @@ -1723,6 +1726,64 @@ export function expandWindowsFsPaths( return [...out] } +/** + * Mandatory write-deny paths under `cwd` (dangerous files/dirs, + * `.git\\hooks`, `.git\\config` unless `allowGitConfig`). Existing + * paths only; `maxDepth` counts a path's components below `cwd`, + * like `rg --max-depth`. Cheap enough to rerun per exec. + */ +export function windowsGetMandatoryDenyPaths( + cwd: string, + opts: { maxDepth?: number; allowGitConfig?: boolean } = {}, +): string[] { + const maxDepth = opts.maxDepth ?? 3 + const files = new Set(DANGEROUS_FILES.map(normalizeCaseForComparison)) + const dirs = getDangerousDirectories().map(d => + normalizeCaseForComparison(d.split('/').join(path.sep)), + ) + const out: string[] = [] + const gitDir = (dir: string) => { + for (const leaf of opts.allowGitConfig ? ['hooks'] : ['hooks', 'config']) { + const p = path.join(dir, leaf) + if (fs.statSync(p, { throwIfNoEntry: false })) out.push(p) + } + } + const depthOk = (p: string) => + path.relative(cwd, p).split(path.sep).length <= maxDepth + const walk = (dir: string, depth: number) => { + let entries: fs.Dirent[] + try { + entries = fs.readdirSync(dir, { withFileTypes: true }) + } catch { + return + } + for (const e of entries) { + const name = normalizeCaseForComparison(e.name) + const full = path.join(dir, e.name) + if (!e.isDirectory()) { + if (files.has(name)) out.push(full) + continue + } + if (name === 'node_modules') continue + if (name === '.git') { + if (depth < maxDepth) gitDir(full) + continue + } + const rel = normalizeCaseForComparison(path.relative(cwd, full)) + if (dirs.some(d => rel === d || rel.endsWith(path.sep + d))) { + // rg only sees a dir through a file inside it (one level + // deeper); cwd-level dirs are added unconditionally on Linux. + if (depth === 1 || depth < maxDepth) out.push(full) + continue + } + if (depth < maxDepth) walk(full, depth + 1) + } + } + cwd = path.resolve(cwd) + walk(cwd, 1) + return [...new Set(out)].filter(depthOk) +} + /** * {@link WindowsSandboxError} narrowed to `mapped_drive_cwd`, * carrying the `DRIVE_REMOTE` root that made the launch fail diff --git a/test/sandbox/windows-per-exec-deny.test.ts b/test/sandbox/windows-per-exec-deny.test.ts new file mode 100644 index 00000000..d5374677 --- /dev/null +++ b/test/sandbox/windows-per-exec-deny.test.ts @@ -0,0 +1,160 @@ +import { describe, it, expect, beforeEach, afterEach } from 'bun:test' +import { + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + writeFileSync, +} from 'node:fs' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { windowsGetMandatoryDenyPaths } from '../../src/sandbox/windows-sandbox-utils.js' +import { computeWindowsPerExecDenySet } from '../../src/sandbox/sandbox-manager.js' + +let root: string + +function repo(dir: string, withConfig = true) { + mkdirSync(join(dir, '.git', 'hooks'), { recursive: true }) + if (withConfig) writeFileSync(join(dir, '.git', 'config'), '') +} + +beforeEach(() => { + root = realpathSync(mkdtempSync(join(tmpdir(), 'win-deny-'))) +}) +afterEach(() => { + rmSync(root, { recursive: true, force: true }) +}) + +describe('windowsGetMandatoryDenyPaths', () => { + it('collects cwd-level and nested targets within depth', () => { + repo(root) + writeFileSync(join(root, '.bashrc'), '') + mkdirSync(join(root, '.vscode')) + mkdirSync(join(root, '.claude', 'commands'), { recursive: true }) + repo(join(root, 'packages')) + mkdirSync(join(root, 'packages', '.idea'), { recursive: true }) + mkdirSync(join(root, 'packages', 'app', '.idea'), { recursive: true }) + const got = new Set(windowsGetMandatoryDenyPaths(root)) + for (const p of [ + join(root, '.git', 'hooks'), + join(root, '.git', 'config'), + join(root, '.bashrc'), + join(root, '.vscode'), + join(root, '.claude', 'commands'), + join(root, 'packages', '.git', 'hooks'), + join(root, 'packages', '.git', 'config'), + join(root, 'packages', '.idea'), + ]) { + expect(got.has(p)).toBe(true) + } + expect(got.has(join(root, 'packages', 'app', '.idea'))).toBe(false) + expect(got.has(join(root, '.git'))).toBe(false) + expect(got.has(join(root, '.claude'))).toBe(false) + }) + + it('returns only existing paths', () => { + mkdirSync(join(root, '.git')) + expect(windowsGetMandatoryDenyPaths(root)).toEqual([]) + }) + + it('respects maxDepth and skips node_modules; cwd is always covered', () => { + repo(join(root, 'a', 'b')) + repo(join(root, 'a')) + repo(join(root, 'node_modules', 'pkg')) + repo(root) + const d3 = windowsGetMandatoryDenyPaths(root, { maxDepth: 3 }) + expect(d3).toContain(join(root, 'a', '.git', 'hooks')) + expect(d3).not.toContain(join(root, 'a', 'b', '.git', 'hooks')) + expect(d3.some(p => p.includes('node_modules'))).toBe(false) + const d2 = windowsGetMandatoryDenyPaths(root, { maxDepth: 2 }) + expect(d2).toEqual( + expect.arrayContaining([ + join(root, '.git', 'hooks'), + join(root, '.git', 'config'), + ]), + ) + expect(d2.some(p => p.startsWith(join(root, 'a')))).toBe(false) + }) + + it('allowGitConfig leaves .git/config out', () => { + repo(root) + const got = windowsGetMandatoryDenyPaths(root, { allowGitConfig: true }) + expect(got).toEqual([join(root, '.git', 'hooks')]) + }) + + it('matches names case-insensitively', () => { + mkdirSync(join(root, '.VSCode')) + writeFileSync(join(root, '.ZshRC'), '') + const got = windowsGetMandatoryDenyPaths(root) + expect(got).toContain(join(root, '.VSCode')) + expect(got).toContain(join(root, '.ZshRC')) + }) +}) + +describe('computeWindowsPerExecDenySet', () => { + const cfg = (fs: Record) => + ({ + filesystem: { allowWrite: [root], denyRead: [], denyWrite: [], ...fs }, + network: { allowedDomains: [], deniedDomains: [] }, + }) as never + + it('merges mandatory, session and per-exec denies', () => { + repo(root) + const secret = join(root, 'secret.txt') + const notes = join(root, 'notes.txt') + writeFileSync(secret, '') + writeFileSync(notes, '') + const set = computeWindowsPerExecDenySet( + cfg({ denyRead: [secret] }), + { filesystem: { denyWrite: [notes] } } as never, + root, + ) + expect(set.denyRead).toEqual([secret]) + expect(set.denyWrite).toEqual( + expect.arrayContaining([ + notes, + join(root, '.git', 'hooks'), + join(root, '.git', 'config'), + ]), + ) + }) + + it('a denyRead target is not duplicated as denyWrite', () => { + repo(root) + const set = computeWindowsPerExecDenySet( + cfg({ denyRead: [join(root, '.git', 'config')] }), + undefined, + root, + ) + expect(set.denyRead).toEqual([join(root, '.git', 'config')]) + expect(set.denyWrite).not.toContain(join(root, '.git', 'config')) + }) + + it('honors allowGitConfig and mandatoryDenySearchDepth', () => { + repo(root) + repo(join(root, 'a', 'b')) + const c = { + ...cfg({ allowGitConfig: true }), + mandatoryDenySearchDepth: 2, + } as never + const set = computeWindowsPerExecDenySet(c, undefined, root) + expect(set.denyWrite).toEqual([join(root, '.git', 'hooks')]) + }) + + it('filesystem.disabled yields an empty set', () => { + repo(root) + expect( + computeWindowsPerExecDenySet(cfg({ disabled: true }), undefined, root), + ).toEqual({ + denyRead: [], + denyWrite: [], + }) + expect( + computeWindowsPerExecDenySet( + cfg({}), + { filesystem: { disabled: true } } as never, + root, + ), + ).toEqual({ denyRead: [], denyWrite: [] }) + }) +}) diff --git a/vendor/srt-win-src/src/acl.rs b/vendor/srt-win-src/src/acl.rs index 0efb5956..69648ea7 100644 --- a/vendor/srt-win-src/src/acl.rs +++ b/vendor/srt-win-src/src/acl.rs @@ -663,14 +663,19 @@ pub enum SbAce { /// `BUILTIN\Users:(F)` (which the sandbox user, a Users member, /// would otherwise pick up). DenyFdc, - /// `(D;;DELETE;;;)` — object-only (`NO_INHERIT`) DELETE deny - /// on a placeholder INTERMEDIATE directory. Blocks the sandbox - /// from renaming/rmdir'ing the intermediate (which would bypass - /// the leaf's stamp) without leaking any semantics onto children: - /// a full-mask `(OI)(CI)` deny here would deny reads over the - /// whole subtree if the placeholder later becomes a real user - /// directory. + /// `(D;;DELETE|WRITE_DAC;;;)` — object-only (`NO_INHERIT`) + /// deny on a placeholder INTERMEDIATE directory. Blocks the + /// sandbox from renaming/rmdir'ing the intermediate (which would + /// bypass the leaf's stamp) without leaking any semantics onto + /// children: a full-mask `(OI)(CI)` deny here would deny reads + /// over the whole subtree if the placeholder later becomes a + /// real user directory. DenyDelete, + /// Same ACE on a REAL directory between a `Deny` target and its + /// modify-grant root, so the ancestor chain cannot be renamed + /// aside. No parent-FDC side ACE: that would re-propagate an + /// inheritable ACE over the tree on every exec. + DenyPin, } impl GrantMask { @@ -710,6 +715,7 @@ impl SbAce { SbAce::Deny(_) => "deny", SbAce::DenyFdc => "deny_fdc", SbAce::DenyDelete => "deny_delete", + SbAce::DenyPin => "deny_pin", } } /// `'read' | 'modify' | 'denyRead' | 'denyWrite' | 'fdc'` — the @@ -723,6 +729,7 @@ impl SbAce { SbAce::Deny(DenyMask::WriteDeny) => "denyWrite", SbAce::DenyFdc => "fdc", SbAce::DenyDelete => "delete", + SbAce::DenyPin => "pin", } } pub fn parse(kind: &str, mask: &str) -> Result { @@ -733,6 +740,7 @@ impl SbAce { ("deny", "denyWrite") => SbAce::Deny(DenyMask::WriteDeny), ("deny_fdc", _) => SbAce::DenyFdc, ("deny_delete", _) => SbAce::DenyDelete, + ("deny_pin", _) => SbAce::DenyPin, (k, m) => bail!("unknown SbAce kind={k:?} mask={m:?}"), }) } @@ -768,15 +776,19 @@ pub struct SbAceSet { impl SbAceSet { /// The set's entries as [`NewAce`]s for `sid`, in canonical /// deny → deny-fdc → allow order. `Deny`/`DenyFdc`/`Grant` carry - /// [`OICI`]; `DenyDelete` is object-only ([`NO_INHERIT`]) — see - /// [`SbAce::DenyDelete`]. + /// [`OICI`]; `DenyDelete`/`DenyPin` is object-only + /// ([`NO_INHERIT`]) — see [`SbAce::DenyDelete`]. fn head_aces(&self, sid: PSID) -> Vec { let mut v = Vec::with_capacity(4); if let Some(m) = self.deny { v.push(NewAce::Deny(sid, m.bits(), OICI)); } if self.deny_delete { - v.push(NewAce::Deny(sid, Mask::DELETE.bits(), NO_INHERIT)); + v.push(NewAce::Deny( + sid, + Mask::DELETE.with(Mask::WRITE_DAC).bits(), + NO_INHERIT, + )); } if self.deny_fdc { v.push(NewAce::Deny(sid, Mask::FILE_DELETE_CHILD.bits(), OICI)); diff --git a/vendor/srt-win-src/src/cli.rs b/vendor/srt-win-src/src/cli.rs index 25c32ff3..523ee34c 100644 --- a/vendor/srt-win-src/src/cli.rs +++ b/vendor/srt-win-src/src/cli.rs @@ -404,7 +404,9 @@ struct AceTargets { /// [`placeholder_ancestors_of`], any earlier holder's — get /// [`SbAce::DenyDelete`], so every holder holds the FULL chain and /// releasing any one holder cannot strip an intermediate another -/// holder still depends on. +/// holder still depends on. Real ancestors strictly between the +/// target and its enclosing modify-grant root get +/// [`SbAce::DenyPin`]. /// /// A `Deny` target the broker cannot create (`PermissionDenied` — /// e.g. under `Program Files` non-elevated) or that names a UNC @@ -429,7 +431,8 @@ fn canonicalize_ace_targets( use anyhow::anyhow; use srt_win::acl::SbAce; use srt_win::path_id::{ - CanonError, canonicalize_path, create_placeholder_chain, is_unc_path, strip_extended_prefix, + CanonError, canonical_parent_of, canonicalize_path, create_placeholder_chain, is_unc_path, + strip_extended_prefix, }; use std::io::ErrorKind; let mut targets = Vec::new(); @@ -520,8 +523,19 @@ fn canonicalize_ace_targets( // harmless — `apply_aces` is idempotent per // `(path, kind, holder)`. if matches!(ace, SbAce::Deny(_)) { - for anc in db.placeholder_ancestors_of(&canon)? { - targets.push((anc, SbAce::DenyDelete)); + let placeholders = db.placeholder_ancestors_of(&canon)?; + for anc in &placeholders { + targets.push((anc.clone(), SbAce::DenyDelete)); + } + // Pin real ancestors below the grant root; never above it. + if let Some(root) = db.grant_root_of(&canon)? { + let mut cur = canonical_parent_of(&canon); + while let Some(anc) = cur.filter(|a| *a != root && a.len() > root.len()) { + if !placeholders.contains(&anc) { + targets.push((anc.clone(), SbAce::DenyPin)); + } + cur = canonical_parent_of(&anc); + } } } } diff --git a/vendor/srt-win-src/src/state_db.rs b/vendor/srt-win-src/src/state_db.rs index d3778f4e..60ae60fb 100644 --- a/vendor/srt-win-src/src/state_db.rs +++ b/vendor/srt-win-src/src/state_db.rs @@ -297,7 +297,7 @@ pub fn open_db() -> Result { } /// Filter on `release_aces` for the deny-ACE lifecycle. -pub const KIND_DENY: &[&str] = &["deny", "deny_fdc", "deny_delete"]; +pub const KIND_DENY: &[&str] = &["deny", "deny_fdc", "deny_delete", "deny_pin"]; /// Filter on `release_aces` for the grant lifecycle. pub const KIND_GRANT: &[&str] = &["grant"]; @@ -1011,6 +1011,23 @@ impl Locked { .filter(|p| cb.get(p.len()) == Some(&b'\\') && canon.starts_with(p.as_str())) .collect()) } + /// Deepest held modify-grant that is a STRICT ancestor of `canon` + /// — the upper bound of the pin chain. Read-only grants carry no + /// DELETE, so they are not roots. + pub fn grant_root_of(&self, canon: &str) -> Result> { + let all: Vec = query_vec( + &self.conn, + "SELECT canonical_path FROM working_aces \ + WHERE kind = 'grant' AND mask = 'modify'", + [], + |r| r.get(0), + )?; + let cb = canon.as_bytes(); + Ok(all + .into_iter() + .filter(|p| cb.get(p.len()) == Some(&b'\\') && canon.starts_with(p.as_str())) + .max_by_key(|p| p.len())) + } } /// Read all `working_aces` rows for `canon` and converge the on-disk @@ -1032,7 +1049,7 @@ fn recompose_at(conn: &Connection, canon: &str, sandbox_sid: &str) -> Result<()> SbAce::Grant(g) => set.grant = Some(g), SbAce::Deny(d) => set.deny = Some(d), SbAce::DenyFdc => set.deny_fdc = true, - SbAce::DenyDelete => set.deny_delete = true, + SbAce::DenyDelete | SbAce::DenyPin => set.deny_delete = true, } } // Install-time ambient write-deny (HKLM AmbientDenies) folds @@ -1420,4 +1437,33 @@ mod tests { // PID 0x7FFF_FFFE is well above any plausible live PID. assert!(!is_process_alive(0x7FFF_FFFE, 0)); } + + #[test] + fn grant_root_of_picks_deepest_modify_grant() { + with_mem_db(|db| { + for (p, m) in [ + (r"\\?\C:\proj", "modify"), + (r"\\?\C:\proj\pkg", "modify"), + (r"\\?\C:\proj\pkg\app\ro", "read"), + ] { + db.conn + .execute( + "INSERT INTO working_aces \ + (canonical_path, kind, file_id, mask) \ + VALUES (?1, 'grant', x'00', ?2)", + params![p, m], + ) + .unwrap(); + } + let root = db + .grant_root_of(r"\\?\C:\proj\pkg\app\ro\.git\config") + .unwrap(); + assert_eq!(root.as_deref(), Some(r"\\?\C:\proj\pkg")); + assert_eq!( + db.grant_root_of(r"\\?\C:\proj\pkg").unwrap().as_deref(), + Some(r"\\?\C:\proj") + ); + assert!(db.grant_root_of(r"\\?\C:\project\x").unwrap().is_none()); + }); + } }