From 7450e1eb24c8269d1336db95ceea1044dc2beef8 Mon Sep 17 00:00:00 2001 From: Huanqi Cao Date: Sat, 19 Sep 2026 02:24:48 +0800 Subject: [PATCH] docs(sandbox): tighten the comments added by the delete-confinement fix One home per fact: the parent-FILE_DELETE_CHILD rationale now lives in the acl module doc and the package README instead of being retold in restrictTokenIntegrity, init(), and grantWrite. Drops the duplicated inline narration; behavior unchanged. --- .../sandbox/sandbox-windows-acl/src/acl.ts | 71 +++++++------------ .../sandbox/sandbox-windows-acl/src/index.ts | 22 ++---- .../sandbox/sandbox-windows-acl/src/token.ts | 18 ++--- .../sandbox-windows-acl/tests/runner.spec.ts | 26 +++---- 4 files changed, 46 insertions(+), 91 deletions(-) diff --git a/packages/sandbox/sandbox-windows-acl/src/acl.ts b/packages/sandbox/sandbox-windows-acl/src/acl.ts index f9ce88d700..199e9e394b 100644 --- a/packages/sandbox/sandbox-windows-acl/src/acl.ts +++ b/packages/sandbox/sandbox-windows-acl/src/acl.ts @@ -5,17 +5,14 @@ * failure is reported with the API name, the exact Win32 code, the formatted * system text, and the affected path. * - * Each grant is a PAIR of edits applied in ONE SetNamedSecurityInfoW call: - * a Deny ACE removing the ambient parent-directory delete right - * (`Everyone` / `FILE_DELETE_CHILD`) plus the capability-SID allow ACE in the - * DACL, and a Low mandatory label with the no-write-up policy in the label - * ACL ({@link buildLowLabelAcl}). The deny makes the capability ACE's DELETE - * bit the only delete authority inside the granted roots — Windows otherwise - * authorizes a delete from the parent directory's `FILE_DELETE_CHILD`, which - * the token's write-restricted intersection does not reach, so one confined - * child could delete files in another granted root. The label denies - * write-class access to every object that is not labeled Low, and the kernel - * applies that policy inside the access check itself. + * Each grant applies three edits in ONE SetNamedSecurityInfoW call: the + * capability-SID allow ACE, a Deny ACE that removes the ambient + * `FILE_DELETE_CHILD` right from the world SID, and a Low no-write-up + * mandatory label ({@link buildLowLabelAcl}). The deny is what keeps one + * granted root out of another's reach: Windows also authorizes a delete from + * the parent directory's `FILE_DELETE_CHILD` right, which the token's + * write-restricted intersection does not reach, and every granted root carries + * the Low label that clears the integrity check. * * Concurrency: grants are read-merge-write against the directory's CURRENT * DACL, and the whole get-merge-set sequence runs under a per-path exclusive @@ -152,12 +149,10 @@ function readCurrentSecurity( /** * Build the Low mandatory label applied with every write grant: one * SYSTEM_MANDATORY_LABEL_ACE naming `lowLabelSidPtr` with the no-write-up - * policy, inheriting to subcontainers and objects so files created later - * inside the granted tree carry the same label automatically. - * - * The caller frees the returned ACL with LocalFree; SetNamedSecurityInfoW - * copies it. Fail-closed: every Win32 call is checked and the half-built ACL - * is released before the error is thrown. + * policy, inheriting to subcontainers and objects so later children carry the + * same label. The caller frees the returned ACL with LocalFree + * (SetNamedSecurityInfoW copies it); every Win32 call is checked and a + * half-built ACL is released before the error is thrown. * @param api - the binding table. * @param lowLabelSidPtr - the Low integrity SID (S-1-16-4096) the label names. * @returns the ACL carrying the single inheritable label ACE. @@ -311,11 +306,10 @@ function hasExactGrant(oldAcl: NativePtr, sidPtr: NativePtr): boolean { } /** - * True when the merge's ambient-delete deny is already present: the exact - * Deny ACE for {@link abi.FILE_DELETE_CHILD} naming the world SID. Together - * with {@link hasExactGrant} and the label check this gates the idempotent - * skip, so a root granted by an earlier build receives the deny on its next - * provision instead of keeping the parent-`FILE_DELETE_CHILD` route open. + * True when the explicit DACL already carries the EXACT ambient-delete deny: + * the Deny ACE for {@link abi.FILE_DELETE_CHILD} naming the world SID. It is + * part of the idempotent skip, so a root granted by an earlier build receives + * the deny on its next provision. * @param oldAcl - the current explicit DACL pointer (from {@link readCurrentSecurity}). * @param worldSidPtr - the Everyone SID the deny names. * @returns whether the exact deny ACE is already present. @@ -326,29 +320,18 @@ function hasExactDeny(oldAcl: NativePtr, worldSidPtr: NativePtr): boolean { /** * Grant `GRANT_MASK` (Write+Delete, displays as "Modify") to the capability SID - * on `path`, deny the ambient `FILE_DELETE_CHILD` right to the world SID, and - * apply the Low mandatory label — all in one SetNamedSecurityInfoW call. + * on `path`, deny the world SID the ambient `FILE_DELETE_CHILD` right, and + * apply the Low mandatory label — one merge, inheriting to subcontainers and + * objects. The capability ACE's DELETE bit is then the only delete authority + * inside the root, so a file whose own DACL grants no DELETE is no longer + * deletable through its parent's rights. * - * The deny closes the parent-directory delete route inside the granted roots: - * Windows authorizes a delete from the object's own DELETE right OR from the - * parent's `FILE_DELETE_CHILD`, and the token's write-restricted intersection - * reaches only the first, so without the deny one confined child could delete - * files in ANOTHER granted root (another workspace, a live sibling temp - * directory) whose Low label clears the integrity check. With the deny in - * place the capability ACE's DELETE bit is the only delete authority inside - * the granted roots, for the confined child and the ambient user alike; a file - * whose own DACL grants no DELETE is therefore no longer deletable by its - * parent's default rights. - * - * Idempotent: when the directory's current explicit DACL already carries the - * exact ACE and deny AND its label ACL the exact label (the per-workspace - * grant surviving from a previous server lifetime), the SetNamedSecurityInfoW - * apply is SKIPPED — it would otherwise re-propagate the identical security - * descriptor across the whole tree (eager inheritance; minutes on large - * workspaces). Otherwise read-merge-write: the entries merge into the - * directory's CURRENT explicit DACL (same shape as {@link revokeWrite}), so - * pre-existing explicit ACEs survive. Runs under the per-path lock. The - * directory must be owned by the caller (owner implicit WRITE_DAC) — same + * Idempotent: the exact ACE, deny, and label together SKIP the + * SetNamedSecurityInfoW apply, which would otherwise re-propagate the + * identical descriptor across the whole tree (eager inheritance; minutes on + * large workspaces). Otherwise read-merge-write, so pre-existing explicit ACEs + * survive (same shape as {@link revokeWrite}). Runs under the per-path lock. + * The directory must be owned by the caller (owner implicit WRITE_DAC) — same * precondition as the POC. * @param api - the binding table. * @param path - the directory whose DACL and label gain the grant (the workspace or temp root). diff --git a/packages/sandbox/sandbox-windows-acl/src/index.ts b/packages/sandbox/sandbox-windows-acl/src/index.ts index 3ecaee8458..75dd23e7a5 100644 --- a/packages/sandbox/sandbox-windows-acl/src/index.ts +++ b/packages/sandbox/sandbox-windows-acl/src/index.ts @@ -10,14 +10,10 @@ * keep-alive group logon SID + Everyone; Authenticated Users, INTERACTIVE, * and LOCAL are absent from both lists — see the seam's dual-list contract * in `packages/sandbox/sandbox-local` and the package README's Modes section - * for the complete boundary). The intersection covers only the write bits of - * the object's OWN access check: Windows can also authorize a write or delete - * from the parent directory's FILE_DELETE_CHILD right, which no restricting - * SID has to co-sign. The token is therefore ALSO lowered to Low integrity - * and every granted directory carries a Low no-write-up label, so the - * kernel's mandatory-integrity check — evaluated inside the access check - * regardless of which right supplied the authority — confines the child to - * the labeled roots. The write SID is the per-WORKSPACE identity + * for the complete boundary). The intersection covers only the object's own + * access check, so the token is also lowered to Low integrity and every + * granted directory carries a Low no-write-up label and the ambient-delete + * deny the `acl` module documents. The write SID is the per-WORKSPACE identity * ({@link workspaceWriteSid}): deterministic from the canonical workspace * path, so the workspace-root ACE materializes once per workspace per * machine and every later provision hits the exact-ACE skip — the @@ -263,9 +259,7 @@ export class AclSandbox { // provision would re-propagate the whole tree) and the temp ACE is // REVOCABLE (dispose() removes it before the private directory is // deleted; the ambient temp root is never granted). - // The Low integrity SID every granted directory is labeled with (and the - // confined token is lowered to) plus the world SID the grants deny the - // ambient parent-directory delete right to. + // The Low label SID and the world SID the grants deny and label with. const lowLabelSid = makeWellKnownSid(api, abi.WinLowLabelSid) const worldSid = makeWellKnownSid(api, abi.WinWorldSid) this.sidAllocations.push(lowLabelSid, worldSid) @@ -292,12 +286,6 @@ export class AclSandbox { { world: worldSid }, this.mode, ) - // Lower the confined token to Low integrity. The write-SID intersection - // does not reach the parent directory's FILE_DELETE_CHILD authority, so - // without the matching label a confined child could delete anything its - // ambient user SIDs may delete; the label plus this level close that - // route, and the granted directories stay writable because - // grantWrite labels them Low too. restrictTokenIntegrity(api, restrictedToken, lowLabelSid) this.token = restrictedToken // The restricted token's default DACL still names only the user's diff --git a/packages/sandbox/sandbox-windows-acl/src/token.ts b/packages/sandbox/sandbox-windows-acl/src/token.ts index 27e8fd848b..10674afb26 100644 --- a/packages/sandbox/sandbox-windows-acl/src/token.ts +++ b/packages/sandbox/sandbox-windows-acl/src/token.ts @@ -145,20 +145,10 @@ export function setTokenDefaultDaclGrant(api: Win32Bindings, token: NativePtr, s } /** - * Lower the restricted token's integrity level to Low (S-1-16-4096). - * - * `WRITE_RESTRICTED` intersects only the write bits of the access mask it - * evaluates against an object's DACL. Windows can also authorize a write or a - * delete from the PARENT directory's FILE_DELETE_CHILD right, and that - * authority is granted without the restricting-SID intersection — so a - * confined child could still delete files anywhere its ambient user SIDs hold - * Modify. The mandatory label closes that route: the kernel applies the - * integrity policy inside the access check, wherever the authority came from, - * so a Low child only writes where the object is labeled Low (see - * `grantWrite`). - * - * Requires TOKEN_ADJUST_DEFAULT on the token (the caller opens it with the - * right). Fails closed: a failure throws before any child is spawned. + * Lower the restricted token's integrity level to Low (S-1-16-4096), the level + * the mandatory labels `grantWrite` applies are matched against; a token left + * at Medium would ignore them. Requires TOKEN_ADJUST_DEFAULT on the token; + * fails closed before any child is spawned. * @param api - the binding table. * @param token - the restricted token to lower. * @param lowLabelSidPtr - the Low integrity SID (S-1-16-4096). diff --git a/packages/sandbox/sandbox-windows-acl/tests/runner.spec.ts b/packages/sandbox/sandbox-windows-acl/tests/runner.spec.ts index fb734ba0ec..f539b99f44 100644 --- a/packages/sandbox/sandbox-windows-acl/tests/runner.spec.ts +++ b/packages/sandbox/sandbox-windows-acl/tests/runner.spec.ts @@ -400,16 +400,11 @@ describe.skipIf(!isWin32 || !pwshAvailable())('windows-acl runner', () => { }, 30_000) it('delete-escape regression: deletes outside the granted roots are denied in BOTH modes, through every delete authority', () => { - // Windows authorizes a delete from EITHER the object's own DELETE right OR - // the parent directory's FILE_DELETE_CHILD right, and the - // WRITE_RESTRICTED pass-2 intersection only covers the first. Every - // ordinary directory the ambient user SIDs control (a project checkout, - // %TEMP%, the profile) grants FILE_DELETE_CHILD, so `cmd /c del` deleted - // files outside the workspace even though a plain write from the same - // child was denied. The Low mandatory label closes both routes because the - // kernel applies the integrity policy inside the access check, whichever - // right supplied the authority; the remaining delete paths below are the - // ones a confined child reaches for. + // Windows authorizes a delete from the object's own DELETE right OR the + // parent directory's FILE_DELETE_CHILD, and the write-restricted + // intersection only covers the first — so `cmd /c del` used to delete + // outside the workspace. The Low label plus the grant's deny close both + // routes; these are the deleters a confined child reaches for. for (const mode of ['read-only', 'workspace-write'] as const) { const victims = { cmd: join(scratchRoot, `delete-${mode}-cmd.txt`), @@ -451,12 +446,11 @@ describe.skipIf(!isWin32 || !pwshAvailable())('windows-acl runner', () => { }, 60_000) it('cross-root delete regression: one session cannot delete inside ANOTHER granted root', () => { - // Both roots carry the Low label, so the integrity check alone would let a - // confined child delete in the other one: Windows authorizes a delete from - // the parent directory's FILE_DELETE_CHILD, which the write-restricted - // intersection does not reach. The grant therefore denies that right to the - // world SID, leaving the capability ACE's DELETE bit as the only delete - // authority inside a granted root. + // Both roots carry the Low label, so the integrity check alone would not + // stop this: Windows also authorizes a delete from the parent directory's + // FILE_DELETE_CHILD right, which the capability intersection never reaches. + // The grant denies that right, leaving the capability DELETE bit as the + // only delete authority inside a granted root. const otherWorkspace = join(scratchRoot, 'other-workspace') const otherTemp = join(scratchRoot, 'other-temp') mkdirSync(otherWorkspace)