From 91d3ed6c5acf7e7af8f3c300a5ca1f5a73c64526 Mon Sep 17 00:00:00 2001 From: Huanqi Cao Date: Sat, 8 Aug 2026 14:32:01 +0800 Subject: [PATCH] =?UTF-8?q?fix(sandbox):=20address=20the=20ACL=20backend?= =?UTF-8?q?=20review=20findings=20=E2=80=94=20quoting,=20DACL=20merge+lock?= =?UTF-8?q?,=20failure=20paths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit quoteArg doubles end-of-string backslashes (real CommandLineToArgvW round-trip test; first-token exemption noted); grantWrite merges into the current DACL instead of replacing it, and both grant/revoke hold a LockFileEx per-path lock under GetTempPathW/dsh-acl-locks (koffi crashes on NULL lpOverlapped — a zeroed OVERLAPPED is used); GRANT_MASK gains DELETE|FILE_DELETE_CHILD (never WRITE_DAC/WRITE_OWNER) though the win32 26200 second check only constrains the WRITE bit; failure paths close all handles (CreateProcessAsUserW pipe set, ResumeThread thread/process/job) with stub-api tests; getTempPathW refuses undersized buffers; drainPipe backs off; runner.spec pwshAvailable uses the resolvePwshPath probe; NTSTATUS exit codes mirror bit-exact (verified end-to-end); WinLocalSid JSDoc re-attributed to WinLocalLogonSid. All gates green: 42 passed/2 skipped, typecheck, oxlint, 0 clones, constraints, knip. --- .../sandbox/sandbox-windows-acl/package.json | 1 + .../sandbox/sandbox-windows-acl/src/acl.ts | 206 +++++++++++---- .../sandbox/sandbox-windows-acl/src/ffi.ts | 56 +++++ .../sandbox/sandbox-windows-acl/src/index.ts | 9 +- .../sandbox/sandbox-windows-acl/src/runner.ts | 9 + .../sandbox/sandbox-windows-acl/src/spawn.ts | 46 +++- .../sandbox-windows-acl/src/win32-abi.ts | 64 ++++- .../sandbox-windows-acl/tests/acl.spec.ts | 234 ++++++++++++++++++ .../tests/failure-paths.spec.ts | 112 +++++++++ .../sandbox-windows-acl/tests/quote.spec.ts | 88 +++++++ .../sandbox-windows-acl/tests/runner.spec.ts | 37 ++- .../sandbox-windows-acl/verify/abi-probe.cpp | 23 +- pnpm-lock.yaml | 3 + 13 files changed, 805 insertions(+), 83 deletions(-) create mode 100644 packages/sandbox/sandbox-windows-acl/tests/acl.spec.ts create mode 100644 packages/sandbox/sandbox-windows-acl/tests/failure-paths.spec.ts create mode 100644 packages/sandbox/sandbox-windows-acl/tests/quote.spec.ts diff --git a/packages/sandbox/sandbox-windows-acl/package.json b/packages/sandbox/sandbox-windows-acl/package.json index 3f4d6cc065..2f13b71296 100644 --- a/packages/sandbox/sandbox-windows-acl/package.json +++ b/packages/sandbox/sandbox-windows-acl/package.json @@ -38,6 +38,7 @@ }, "devDependencies": { "@deepseek-ai/dsh-invariants": "workspace:^", + "@deepseek-ai/dsh-pwsh-local": "workspace:^", "@deepseek-ai/dsh-sandbox-local": "workspace:^", "cordis": "^4.0.0-rc.7" } diff --git a/packages/sandbox/sandbox-windows-acl/src/acl.ts b/packages/sandbox/sandbox-windows-acl/src/acl.ts index 44020ae0c3..7700474d5f 100644 --- a/packages/sandbox/sandbox-windows-acl/src/acl.ts +++ b/packages/sandbox/sandbox-windows-acl/src/acl.ts @@ -4,10 +4,19 @@ * the failure handling the POC lacks). Every API call is checked and every * failure is reported with the API name, the exact Win32 code, the formatted * system text, and the affected path. + * + * 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 + * LockFileEx lock (see {@link withPathLock}) so concurrent sandbox instances + * cannot clobber each other's ACEs. * @module @deepseek-ai/dsh-sandbox-windows-acl/acl */ -import { allocPtrSlot, decodePtr, isNullPtr, ptrAddress, throwLastError, throwWin32 } from './ffi.ts' +import { createHash } from 'node:crypto' +import { mkdirSync } from 'node:fs' +import { dirname, join } from 'node:path' + +import { allocOverlapped, allocPtrSlot, decodePtr, getTempPath, isInvalidHandle, isNullPtr, ptrAddress, throwLastError, throwWin32 } from './ffi.ts' import type { NativePtr, Win32Bindings } from './ffi.ts' import * as abi from './win32-abi.ts' @@ -18,7 +27,7 @@ import * as abi from './win32-abi.ts' * `permissions` is the access mask; the POC passes 0 for REVOKE_ACCESS, which * removes every ACE for the trustee. */ -function buildExplicitAccess(sidPtr: NativePtr, mode: number, permissions: number): Buffer { +export function buildExplicitAccess(sidPtr: NativePtr, mode: number, permissions: number): Buffer { const entry = Buffer.alloc(abi.EXPLICIT_ACCESS_W_SIZE) entry.writeUInt32LE(permissions, 0) // grfAccessPermissions entry.writeUInt32LE(mode, 4) // grfAccessMode @@ -31,47 +40,82 @@ function buildExplicitAccess(sidPtr: NativePtr, mode: number, permissions: numbe } /** - * Grant `FILE_GENERIC_WRITE & ~READ_CONTROL` (displays as "Write") to the - * orphan SID on `path`, inheriting to subcontainers and objects. The directory - * must be owned by the caller (owner implicit WRITE_DAC) — same precondition - * as the POC. + * One lock file per protected path: `\dsh-acl-locks\.lock`. The lock root derives from + * GetTempPathW (never from runner argv or DSH_HOME), and the lowercasing + * maps Windows's case-insensitive path spellings onto one lock. * @param api - the binding table. - * @param path - the directory whose DACL gains the grant (the workspace or temp root). - * @param sidPtr - the orphan write SID the ACE names. + * @param path - the protected directory (absolute). + * @returns the lock file path for that directory. */ -export function grantWrite(api: Win32Bindings, path: string, sidPtr: NativePtr): void { - const newAclSlot = allocPtrSlot() - const mergeResult = api.setEntriesInAclW(1, buildExplicitAccess(sidPtr, abi.GRANT_ACCESS, abi.GRANT_MASK), null, newAclSlot) - if (mergeResult !== abi.ERROR_SUCCESS) throwWin32(api, 'SetEntriesInAclW', mergeResult, path) - const newAcl = decodePtr(newAclSlot) - if (newAcl === null) throwWin32(api, 'SetEntriesInAclW', api.getLastError(), `null ACL for ${path}`) - - const applyResult = api.setNamedSecurityInfoW( - path, abi.SE_FILE_OBJECT, abi.DACL_SECURITY_INFORMATION, - null, null, newAcl, null, - ) - // Free the LocalAlloc'd ACL before any throw; capture both outcomes first. - const freed = api.localFree(newAcl) - if (applyResult !== abi.ERROR_SUCCESS) throwWin32(api, 'SetNamedSecurityInfoW', applyResult, path) - if (!isNullPtr(freed)) throwLastError(api, 'LocalFree', `grantWrite(${path})`) +export function lockFilePath(api: Win32Bindings, path: string): string { + const digest = createHash('sha256').update(path.toLowerCase()).digest('hex').slice(0, 16) + return join(getTempPath(api), 'dsh-acl-locks', `${digest}.lock`) } /** - * Remove every ACE for the orphan SID from the directory DACL (REVOKE_ACCESS - * merge — other entries are preserved). Returns whether an ACE removal was - * attempted (false when the directory carries no DACL at all). - * - * Allocation contract (the POC's RevokeAccess, minus its missing checks): - * GetNamedSecurityInfoW returns the DACL pointer INSIDE the security - * descriptor allocation — only the descriptor may be LocalFree'd, and it must - * not be freed before SetEntriesInAclW has consumed the ACL. Freeing the ACL - * pointer itself corrupts the heap (verified the hard way). + * Run `action` holding the per-path exclusive lock: CreateFileW + * (OPEN_ALWAYS, shared read/write but NOT delete — a deletable lock file + * could be removed and recreated under the holder, letting two processes + * hold "the same" lock), then a one-byte LockFileEx + * (LOCKFILE_EXCLUSIVE_LOCK, zeroed OVERLAPPED = lock from offset 0 on the + * synchronous handle — see allocOverlapped for why not NULL), then + * UnlockFileEx + CloseHandle. Fail-closed: open/lock/unlock/close failures + * throw like every other Win32 call in this package; an `action` failure + * still unlocks (best-effort) and rethrows the original error. * @param api - the binding table. - * @param path - the directory whose DACL loses the orphan-SID ACEs. - * @param sidPtr - the orphan write SID whose ACEs are removed. - * @returns whether an ACE removal was attempted (false when the directory carries no DACL at all). + * @param path - the protected directory (absolute). + * @param action - the get-merge-set sequence to serialize. + * @returns the action's result. */ -export function revokeWrite(api: Win32Bindings, path: string, sidPtr: NativePtr): boolean { +export function withPathLock(api: Win32Bindings, path: string, action: () => T): T { + const lockPath = lockFilePath(api, path) + mkdirSync(dirname(lockPath), { recursive: true }) + const handle = api.createFileW( + lockPath, + abi.GENERIC_READ | abi.GENERIC_WRITE, + abi.FILE_SHARE_READ | abi.FILE_SHARE_WRITE, + null, abi.OPEN_ALWAYS, 0, null, + ) + if (isInvalidHandle(handle)) throwLastError(api, 'CreateFileW', lockPath) + const overlapped = allocOverlapped() // stays zeroed: offset 0, hEvent NULL + if (api.lockFileEx(handle, abi.LOCKFILE_EXCLUSIVE_LOCK, 0, 1, 0, overlapped) === 0) { + const win32Code = api.getLastError() + api.closeHandle(handle) // best-effort on the lock-failure path + throwWin32(api, 'LockFileEx', win32Code, lockPath) + } + + let result: T + try { + result = action() + } catch (error) { + // Best-effort release on the action-failure path: cleanup failures must + // not mask the action's error. + api.unlockFileEx(handle, 0, 1, 0, overlapped) + api.closeHandle(handle) + throw error + } + if (api.unlockFileEx(handle, 0, 1, 0, overlapped) === 0) { + const win32Code = api.getLastError() + api.closeHandle(handle) // best-effort on the unlock-failure path + throwWin32(api, 'UnlockFileEx', win32Code, lockPath) + } + if (api.closeHandle(handle) === 0) throwLastError(api, 'CloseHandle', `lock file ${lockPath}`) + return result +} + +/** + * Read the directory's current explicit DACL via GetNamedSecurityInfoW. + * Allocation contract (the POC's RevokeAccess, minus its missing checks): the + * returned ACL pointer sits INSIDE the security descriptor allocation — only + * the descriptor may be LocalFree'd, and it must not be freed before + * SetEntriesInAclW has consumed the ACL. Freeing the ACL pointer itself + * corrupts the heap (verified the hard way). + * @param api - the binding table. + * @param path - the directory whose DACL is read. + * @returns the current explicit DACL (null when the directory carries none) and its owning descriptor. + */ +function readCurrentDacl(api: Win32Bindings, path: string): { oldAcl: NativePtr | null; descriptor: NativePtr | null } { const ownerSlot = allocPtrSlot() const groupSlot = allocPtrSlot() const daclSlot = allocPtrSlot() @@ -82,27 +126,39 @@ export function revokeWrite(api: Win32Bindings, path: string, sidPtr: NativePtr) ownerSlot, groupSlot, daclSlot, saclSlot, descriptorSlot, ) if (readResult !== abi.ERROR_SUCCESS) throwWin32(api, 'GetNamedSecurityInfoW', readResult, path) - const oldAcl = decodePtr(daclSlot) - const descriptor = decodePtr(descriptorSlot) - - if (oldAcl === null) { - if (descriptor !== null) { - const freed = api.localFree(descriptor) - if (!isNullPtr(freed)) throwLastError(api, 'LocalFree', `revokeWrite(${path}) descriptor`) - } - return false - } + return { oldAcl: decodePtr(daclSlot), descriptor: decodePtr(descriptorSlot) } +} +/** + * Shared tail of grantWrite and revokeWrite: merge `entry` into `oldAcl` + * (null = no explicit DACL yet; SetEntriesInAclW builds one from scratch), + * free the descriptor before applying the merged ACL, apply it, then free the + * merged ACL — checking every call and reporting with the caller's label. + * @param api - the binding table. + * @param path - the directory the DACL edit applies to. + * @param entry - the EXPLICIT_ACCESS_W to merge (grant or revoke). + * @param oldAcl - the current explicit DACL (from {@link readCurrentDacl}). + * @param descriptor - the descriptor allocation owning `oldAcl`. + * @param label - the caller's name for error details. + */ +function mergeAndApply( + api: Win32Bindings, + path: string, + entry: Buffer, + oldAcl: NativePtr | null, + descriptor: NativePtr | null, + label: string, +): void { const newAclSlot = allocPtrSlot() - const mergeResult = api.setEntriesInAclW(1, buildExplicitAccess(sidPtr, abi.REVOKE_ACCESS, 0), oldAcl, newAclSlot) + const mergeResult = api.setEntriesInAclW(1, entry, oldAcl, newAclSlot) if (mergeResult !== abi.ERROR_SUCCESS) { if (descriptor !== null) api.localFree(descriptor) // frees the ACL block too - throwWin32(api, 'SetEntriesInAclW', mergeResult, `revokeWrite(${path})`) + throwWin32(api, 'SetEntriesInAclW', mergeResult, `${label}(${path})`) } const newAcl = decodePtr(newAclSlot) if (newAcl === null) { if (descriptor !== null) api.localFree(descriptor) - throwWin32(api, 'SetEntriesInAclW', api.getLastError(), `revokeWrite(${path}): null new ACL`) + throwWin32(api, 'SetEntriesInAclW', api.getLastError(), `${label}(${path}): null new ACL`) } // The descriptor block (oldAcl included) is dead after the merge — free it @@ -113,8 +169,52 @@ export function revokeWrite(api: Win32Bindings, path: string, sidPtr: NativePtr) null, null, newAcl, null, ) const freedNew = api.localFree(newAcl) - if (applyResult !== abi.ERROR_SUCCESS) throwWin32(api, 'SetNamedSecurityInfoW', applyResult, `revokeWrite(${path})`) - if (freedDescriptor !== null && !isNullPtr(freedDescriptor)) throwLastError(api, 'LocalFree', `revokeWrite(${path}) descriptor`) - if (!isNullPtr(freedNew)) throwLastError(api, 'LocalFree', `revokeWrite(${path}) new ACL`) - return true + if (applyResult !== abi.ERROR_SUCCESS) throwWin32(api, 'SetNamedSecurityInfoW', applyResult, `${label}(${path})`) + if (freedDescriptor !== null && !isNullPtr(freedDescriptor)) throwLastError(api, 'LocalFree', `${label}(${path}) descriptor`) + if (!isNullPtr(freedNew)) throwLastError(api, 'LocalFree', `${label}(${path}) new ACL`) +} + +/** + * Grant `GRANT_MASK` (Write+Delete, displays as "Modify") to the orphan SID + * on `path`, inheriting to subcontainers and objects. Read-merge-write: the + * new ACE merges 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 precondition as the POC. + * @param api - the binding table. + * @param path - the directory whose DACL gains the grant (the workspace or temp root). + * @param sidPtr - the orphan write SID the ACE names. + */ +export function grantWrite(api: Win32Bindings, path: string, sidPtr: NativePtr): void { + withPathLock(api, path, () => { + const { oldAcl, descriptor } = readCurrentDacl(api, path) + mergeAndApply(api, path, buildExplicitAccess(sidPtr, abi.GRANT_ACCESS, abi.GRANT_MASK), oldAcl, descriptor, 'grantWrite') + }) +} + +/** + * Remove every ACE for the orphan SID from the directory DACL (REVOKE_ACCESS + * merge — other entries are preserved). Returns whether an ACE removal was + * attempted (false when the directory carries no DACL at all). + * + * Runs under the per-path lock (the whole get-merge-set sequence); the + * descriptor/ACL allocation contract lives on {@link readCurrentDacl}. + * @param api - the binding table. + * @param path - the directory whose DACL loses the orphan-SID ACEs. + * @param sidPtr - the orphan write SID whose ACEs are removed. + * @returns whether an ACE removal was attempted (false when the directory carries no DACL at all). + */ +export function revokeWrite(api: Win32Bindings, path: string, sidPtr: NativePtr): boolean { + return withPathLock(api, path, () => { + const { oldAcl, descriptor } = readCurrentDacl(api, path) + if (oldAcl === null) { + if (descriptor !== null) { + const freed = api.localFree(descriptor) + if (!isNullPtr(freed)) throwLastError(api, 'LocalFree', `revokeWrite(${path}) descriptor`) + } + return false + } + mergeAndApply(api, path, buildExplicitAccess(sidPtr, abi.REVOKE_ACCESS, 0), oldAcl, descriptor, 'revokeWrite') + return true + }) } diff --git a/packages/sandbox/sandbox-windows-acl/src/ffi.ts b/packages/sandbox/sandbox-windows-acl/src/ffi.ts index 6d778f7b11..1d9665a8fc 100644 --- a/packages/sandbox/sandbox-windows-acl/src/ffi.ts +++ b/packages/sandbox/sandbox-windows-acl/src/ffi.ts @@ -26,6 +26,17 @@ export function isNullPtr(value: NativePtr | null | undefined): value is null | return value === null || value === undefined || (value as bigint) === 0n } +/** + * True for CreateFileW's INVALID_HANDLE_VALUE failure marker (-1, which + * koffi hands back as the unsigned 64-bit all-ones pointer). + * @param handle - the handle CreateFileW returned. + * @returns whether the handle signals failure. + */ +export function isInvalidHandle(handle: NativePtr | null | undefined): boolean { + if (isNullPtr(handle)) return true + return (handle as bigint) === 0xFFFFFFFFFFFFFFFFn || (handle as bigint) === -1n +} + type Ptr = ReturnType /** Field subset written into a zeroed STARTUPINFOW (layout verified: size 104). */ @@ -86,6 +97,12 @@ export interface Win32Bindings { ): number // ---- environment / io ---------------------------------------------------- getTempPathW(length: number, buffer: Buffer): number + createFileW( + fileName: string, desiredAccess: number, shareMode: number, attributes: null, + creationDisposition: number, flagsAndAttributes: number, templateFile: null, + ): NativePtr + lockFileEx(file: NativePtr, flags: number, reserved: number, bytesLow: number, bytesHigh: number, overlapped: NativePtr): number + unlockFileEx(file: NativePtr, reserved: number, bytesLow: number, bytesHigh: number, overlapped: NativePtr): number createPipe(readHandle: NativePtr, writeHandle: NativePtr, attributes: null, size: number): number setHandleInformation(handle: NativePtr, mask: number, flags: number): number createProcessAsUserW( @@ -231,6 +248,18 @@ export function allocBytes(length: number): NativePtr { return value as NativePtr } +/** + * Allocate one zeroed OVERLAPPED (32 bytes on x64: Internal@0, InternalHigh@8, + * Offset@16, OffsetHigh@20, hEvent@24). LockFileEx/UnlockFileEx receive this + * instead of a NULL lpOverlapped: koffi 3.1.1 crashes on NULL there, and a + * zeroed OVERLAPPED on a synchronous file handle is the documented equivalent + * (the byte range locks from offset 0, hEvent stays NULL). + * @returns the zeroed block pointer. + */ +export function allocOverlapped(): NativePtr { + return allocBytes(32) +} + /** * Decode a pointer VALUE stored in memory at `buffer[offset]` (e.g. TOKEN_GROUPS entries). * @param buffer - the buffer holding the pointer value. @@ -313,6 +342,14 @@ function bindings(): Win32Bindings { setNamedSecurityInfoW: bind(advapi32, 'SetNamedSecurityInfoW', 'uint32', ['str16', 'int', 'uint32', PVOID, PVOID, PVOID, PVOID]), getNamedSecurityInfoW: bind(advapi32, 'GetNamedSecurityInfoW', 'uint32', ['str16', 'int', 'uint32', PPVOID, PPVOID, PPVOID, PPVOID, PPVOID]), getTempPathW: bind(kernel32, 'GetTempPathW', 'uint32', ['uint32', PVOID]), + // fileapi.h line ~64: HANDLE CreateFileW(LPCWSTR, DWORD, DWORD, + // LPSECURITY_ATTRIBUTES, DWORD, DWORD, HANDLE). + createFileW: bind(kernel32, 'CreateFileW', PVOID, ['str16', 'uint32', 'uint32', PVOID, 'uint32', 'uint32', PVOID]), + // fileapi.h lines ~177/~185: BOOL LockFileEx(HANDLE, DWORD, DWORD, DWORD, + // DWORD, LPOVERLAPPED); BOOL UnlockFileEx(HANDLE, DWORD, DWORD, DWORD, + // LPOVERLAPPED). lpOverlapped is NULL for synchronous locking. + lockFileEx: bind(kernel32, 'LockFileEx', 'int', [PVOID, 'uint32', 'uint32', 'uint32', 'uint32', PVOID]), + unlockFileEx: bind(kernel32, 'UnlockFileEx', 'int', [PVOID, 'uint32', 'uint32', 'uint32', PVOID]), createPipe: bind(kernel32, 'CreatePipe', 'int', [PPVOID, PPVOID, PVOID, 'uint32']), setHandleInformation: bind(kernel32, 'SetHandleInformation', 'int', [PVOID, 'uint32', 'uint32']), createProcessAsUserW: bind(advapi32, 'CreateProcessAsUserW', 'int', [ @@ -357,6 +394,25 @@ export function errorText(api: Win32Bindings, win32Code: number): string { return buffer.subarray(0, length * 2).toString('utf16le').trim() } +/** + * Read the process temp directory via GetTempPathW (fileapi.h line ~188). + * Defensive against an overlong system temp path: GetTempPathW reports the + * REQUIRED length (including NUL) without writing the buffer when it is too + * small, so a reported length beyond the buffer's capacity means the buffer + * was never filled and must not be decoded. + * @param api - the binding table. + * @returns the NUL-terminated temp path decoded as a string. + */ +export function getTempPath(api: Win32Bindings): string { + const buffer = Buffer.alloc((abi.MAX_PATH + 1) * 2) + const length = api.getTempPathW(buffer.length / 2, buffer) + if (length === 0) throwLastError(api, 'GetTempPathW') + if (length > buffer.length / 2) { + throw new Win32Error('GetTempPathW', abi.ERROR_INSUFFICIENT_BUFFER, `required ${length} chars exceed the ${buffer.length / 2}-char buffer; nothing was written`) + } + return buffer.subarray(0, length * 2).toString('utf16le') +} + /** * Throw a Win32Error for a BOOL-style API failure. MUST be called immediately * after the failed call so GetLastError is not clobbered by other Win32 calls. diff --git a/packages/sandbox/sandbox-windows-acl/src/index.ts b/packages/sandbox/sandbox-windows-acl/src/index.ts index f597ebbc72..e312a40e09 100644 --- a/packages/sandbox/sandbox-windows-acl/src/index.ts +++ b/packages/sandbox/sandbox-windows-acl/src/index.ts @@ -29,7 +29,7 @@ import { resolve } from 'node:path' import { grantWrite, revokeWrite } from './acl.ts' import { Win32Error } from './errors.ts' -import { allocPtrSlot, decodePtr, isNullPtr, throwLastError, win32 } from './ffi.ts' +import { allocPtrSlot, decodePtr, getTempPath, isNullPtr, throwLastError, win32 } from './ffi.ts' import type { NativePtr, Win32Bindings } from './ffi.ts' import { drainPipe, spawnSandboxed, spawnSandboxedInherited, waitForExit } from './spawn.ts' import { createRestrictedToken, findLogonSid, makeWellKnownSid, openCurrentProcessToken } from './token.ts' @@ -88,13 +88,6 @@ function randomWriteSid(): string { return `S-1-4-${randomInt(1, 2 ** 30)}-${randomInt(1, 2 ** 30)}` } -function getTempPath(api: Win32Bindings): string { - const buffer = Buffer.alloc((abi.MAX_PATH + 1) * 2) - const length = api.getTempPathW(buffer.length / 2, buffer) - if (length === 0) throwLastError(api, 'GetTempPathW') - return buffer.subarray(0, length * 2).toString('utf16le') -} - /** * One write-restricted sandbox instance: token + orphan-SID grants + spawn. * `init()` is fail-closed — any Win32 failure revokes whatever was granted diff --git a/packages/sandbox/sandbox-windows-acl/src/runner.ts b/packages/sandbox/sandbox-windows-acl/src/runner.ts index 02fd416f7f..578a615f84 100644 --- a/packages/sandbox/sandbox-windows-acl/src/runner.ts +++ b/packages/sandbox/sandbox-windows-acl/src/runner.ts @@ -124,6 +124,15 @@ async function main(): Promise { main().then( (exitCode) => { + // Exit-code mirroring is full-width on Windows, verified empirically on + // this machine (Windows 11 build 26200, Node 24): a child that exits + // with the NTSTATUS 0xC0000005 (STATUS_ACCESS_VIOLATION) is read back + // by GetExitCodeProcess as the uint32 3221225477, and after + // process.exitCode = 3221225477 the parent observes exactly + // 3221225477 (spawnSync status). PowerShell's $LASTEXITCODE and cmd + // print the signed view (-1073741819), but no truncation or masking + // happens anywhere in the chain — the mirror contract holds for the + // full 32-bit range, so no re-mapping is needed. process.exitCode = exitCode }, (error: unknown) => { diff --git a/packages/sandbox/sandbox-windows-acl/src/spawn.ts b/packages/sandbox/sandbox-windows-acl/src/spawn.ts index d9c7c70564..9c0436f29f 100644 --- a/packages/sandbox/sandbox-windows-acl/src/spawn.ts +++ b/packages/sandbox/sandbox-windows-acl/src/spawn.ts @@ -14,9 +14,12 @@ import type { NativePtr, Win32Bindings } from './ffi.ts' import * as abi from './win32-abi.ts' /** - * Quote one argument per the CommandLineToArgvW parsing rules (backslash - * escaping only before quotes; a trailing backslash before the closing quote - * is doubled). + * Quote one argument per the CommandLineToArgvW parsing rules: backslashes + * are doubled only before a quote character — including the closing quote + * this function appends, so a trailing backslash run is doubled as well + * (otherwise an odd run would escape the closing quote into a literal + * character and corrupt the rest of the command line). Mirrors the CRT + * ArgvQuote behavior Microsoft documents for command-line arguments. * @param argument - one argv entry to quote. * @returns the quoted entry (bare when quoting is unnecessary). */ @@ -30,10 +33,13 @@ export function quoteArg(argument: string): string { backslashes++ index++ } - if (index < argument.length && argument.charAt(index) === '"') { + if (index === argument.length) { + // Trailing backslash run: doubled so it cannot escape the closing quote. + quoted += '\\'.repeat(backslashes * 2) + } else if (argument.charAt(index) === '"') { quoted += '\\'.repeat(backslashes * 2 + 1) + '"' } else { - quoted += '\\'.repeat(backslashes) + (index < argument.length ? argument.charAt(index) : '') + quoted += '\\'.repeat(backslashes) + argument.charAt(index) } } return quoted + '"' @@ -119,8 +125,19 @@ export function spawnSandboxed( null, options.cwd, startupInfo, processInfo, ) - // Capture the failure before CloseHandle calls clobber GetLastError. - if (created === 0) throwLastError(api, 'CreateProcessAsUserW', `command: ${options.command}, cwd: ${options.cwd}`) + // Capture the failure before CloseHandle calls clobber GetLastError, then + // close every pipe handle created so far — the six-close contract this test + // surface pins (tests/failure-paths.spec.ts). + if (created === 0) { + const win32Code = api.getLastError() + api.closeHandle(stdIn.read) + api.closeHandle(stdIn.write) + api.closeHandle(stdOut.read) + api.closeHandle(stdOut.write) + api.closeHandle(stdErr.read) + api.closeHandle(stdErr.write) + throwWin32(api, 'CreateProcessAsUserW', win32Code, `command: ${options.command}, cwd: ${options.cwd}`) + } const info = decodeProcessInfo(processInfo) const processHandle = info.hProcess @@ -172,7 +189,9 @@ export async function drainPipe(api: Win32Bindings, handle: NativePtr): Promise< } chunks.push(chunk.subarray(0, decodeUint32(readSlot))) } - await new Promise(resolve => setImmediate(resolve)) + // Small backoff instead of setImmediate: a bare next-tick would busy-poll + // the pipe at full event-loop speed while the child produces no output. + await new Promise(resolve => setTimeout(resolve, 1)) } api.closeHandle(handle) return Buffer.concat(chunks) @@ -314,7 +333,16 @@ export function spawnSandboxedInherited( api.closeHandle(job) throwWin32(api, 'AssignProcessToJobObject', win32Code, `pid ${info.dwProcessId}`) } - if (api.resumeThread(threadHandle) === 0xFFFFFFFF) throwLastError(api, 'ResumeThread', `pid ${info.dwProcessId}`) + if (api.resumeThread(threadHandle) === 0xFFFFFFFF) { + // Closing the job triggers kill-on-close, so the suspended child dies + // instead of hanging until this process exits; the process/thread handles + // must go too. + const win32Code = api.getLastError() + api.closeHandle(threadHandle) + api.closeHandle(processHandle) + api.closeHandle(job) + throwWin32(api, 'ResumeThread', win32Code, `pid ${info.dwProcessId}`) + } api.closeHandle(threadHandle) return { pid: info.dwProcessId, process: processHandle, job } diff --git a/packages/sandbox/sandbox-windows-acl/src/win32-abi.ts b/packages/sandbox/sandbox-windows-acl/src/win32-abi.ts index 7696605c4e..942862ceb9 100644 --- a/packages/sandbox/sandbox-windows-acl/src/win32-abi.ts +++ b/packages/sandbox/sandbox-windows-acl/src/win32-abi.ts @@ -49,13 +49,26 @@ export const SE_GROUP_LOGON_ID = 0xC0000000 export const STANDARD_RIGHTS_WRITE = 0x00020000 // == READ_CONTROL /** FILE_GENERIC_WRITE: every file-write permission bit plus SYNCHRONIZE. */ export const FILE_GENERIC_WRITE = 0x00120116 -// What the POC grants: FILE_GENERIC_WRITE minus READ_CONTROL; displays as -// "Write" in Explorer/icacls (windows-acl-restrict-poc.cpp line 16). +/** DELETE: remove or rename the object (winnt.h line ~3009). */ +export const DELETE = 0x00010000 +/** FILE_DELETE_CHILD: remove or rename a directory's children (winnt.h line ~5907). */ +export const FILE_DELETE_CHILD = 0x0040 +// The POC granted FILE_GENERIC_WRITE minus READ_CONTROL, which displays as +// "Write" in Explorer/icacls (windows-acl-restrict-poc.cpp line 16). The +// sandbox grant adds DELETE and FILE_DELETE_CHILD so confined +// delete/rename/git operations inside the granted trees pass the token's +// access check too; Write+DELETE displays as "Modify" in icacls. +// WRITE_DAC/WRITE_OWNER stay OUT deliberately — granting them would let the +// child take ownership or rewrite DACLs and escape the allowlist (the +// security boundary). /** - * GRANT_MASK: FILE_GENERIC_WRITE minus READ_CONTROL — the write-access mask - * the orphan-SID ACEs grant (displays as "Write" in Explorer/icacls). + * GRANT_MASK: FILE_GENERIC_WRITE minus READ_CONTROL plus DELETE and + * FILE_DELETE_CHILD — the write+delete access mask the orphan-SID ACEs grant + * (displays as "Modify" in Explorer/icacls). WRITE_DAC/WRITE_OWNER are + * deliberately excluded: they would let the confined child take ownership or + * rewrite DACLs. */ -export const GRANT_MASK = FILE_GENERIC_WRITE & ~STANDARD_RIGHTS_WRITE // 0x00100116 +export const GRANT_MASK = (FILE_GENERIC_WRITE | DELETE | FILE_DELETE_CHILD) & ~STANDARD_RIGHTS_WRITE // 0x00110156 // CreateRestrictedToken flags (winnt.h lines ~4284) /** DISABLE_MAX_PRIVILEGE: strip the token's maximum-privilege elevation so the confined child cannot escalate. */ @@ -68,7 +81,13 @@ export const WRITE_RESTRICTED = 0x8 // WELL_KNOWN_SID_TYPE (winnt.h lines ~3369-3407) /** WinWorldSid: S-1-1-0 (Everyone). */ export const WinWorldSid = 1 -/** WinLocalSid: S-1-2-0 (LOCAL); CreateWellKnownSid(WinLocalSid) fails with ERROR_INVALID_PARAMETER on Windows 11 build 26200. */ +/** + * WinLocalSid: S-1-2-0 (LOCAL) — safe, created successfully on every init + * (it sits in every restricted token's restricting list). The + * CreateWellKnownSid ERROR_INVALID_PARAMETER failure documented in this + * module's header comment belongs to WinLocalLogonSid (S-1-2-1), NOT to + * this type. + */ export const WinLocalSid = 2 /** WinInteractiveSid: S-1-5-4 (INTERACTIVE). */ export const WinInteractiveSid = 11 @@ -155,6 +174,39 @@ export const ERROR_INSUFFICIENT_BUFFER = 122 export const ERROR_BROKEN_PIPE = 109 /** ERROR_NO_DATA: the pipe is being closed. */ export const ERROR_NO_DATA = 232 +/** ERROR_LOCK_VIOLATION: a byte-range lock conflicts with an existing lock (winerror.h line ~78). */ +export const ERROR_LOCK_VIOLATION = 33 + +// ---- lock files (fileapi.h / minwinbase.h / winnt.h) ----------------------- + +// CreateFileW dwDesiredAccess for the ACL lock files: plain read+write is +// enough to take byte-range locks. +/** GENERIC_READ: generic read access (winnt.h line ~3028). */ +export const GENERIC_READ = 0x80000000 +/** GENERIC_WRITE: generic write access (winnt.h line ~3029). */ +export const GENERIC_WRITE = 0x40000000 +// CreateFileW dwShareMode: the lock file is shared for read/write but NOT +// for delete — if a locked file could be deleted and recreated underneath the +// lock holder, two processes could hold "the same" lock on different files. +/** FILE_SHARE_READ: other opens may read (winnt.h line ~5949). */ +export const FILE_SHARE_READ = 0x00000001 +/** FILE_SHARE_WRITE: other opens may write (winnt.h line ~5950). */ +export const FILE_SHARE_WRITE = 0x00000002 +/** FILE_SHARE_DELETE: other opens may delete (winnt.h line ~5951) — deliberately NOT used for lock files. */ +export const FILE_SHARE_DELETE = 0x00000004 +/** OPEN_ALWAYS: create the lock file if absent, open it otherwise (fileapi.h line ~21). */ +export const OPEN_ALWAYS = 4 +// LockFileEx dwFlags (minwinbase.h lines ~180-181, included by winbase.h). +/** LOCKFILE_EXCLUSIVE_LOCK: request an exclusive byte-range lock. */ +export const LOCKFILE_EXCLUSIVE_LOCK = 0x2 +/** LOCKFILE_FAIL_IMMEDIATELY: fail with ERROR_LOCK_VIOLATION instead of waiting. */ +export const LOCKFILE_FAIL_IMMEDIATELY = 0x1 + +// ACE_HEADER.AceFlags (winnt.h lines ~3477-3524): inherited ACEs shown when +// reading a DACL are marked with this bit and are not part of the explicit +// DACL edits this module makes. +/** INHERITED_ACE: the ACE was inherited from the parent object, not stored explicitly. */ +export const INHERITED_ACE = 0x10 // ---- job object (winnt.h lines ~4859-4866, ~5138, ~5190-5199) -------------- diff --git a/packages/sandbox/sandbox-windows-acl/tests/acl.spec.ts b/packages/sandbox/sandbox-windows-acl/tests/acl.spec.ts new file mode 100644 index 0000000000..fe0bc13acc --- /dev/null +++ b/packages/sandbox/sandbox-windows-acl/tests/acl.spec.ts @@ -0,0 +1,234 @@ +/** + * ACL edit tests: the read-merge-write grant keeps pre-existing explicit + * ACEs, interleaved sandbox instances do not clobber each other, the + * per-path lock primitive is deterministic, and the grant mask carries + * DELETE + FILE_DELETE_CHILD (never WRITE_DAC/WRITE_OWNER). + * + * All state lives in %TEMP% mkdtemp scratch directories; the only exception + * is the mandated lock infrastructure under \dsh-acl-locks, + * whose per-test lock file is removed in cleanup. + */ + +import { mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import koffi from 'koffi' + +import { buildExplicitAccess, grantWrite, lockFilePath, revokeWrite, withPathLock } from '../src/acl.ts' +import { AclSandbox } from '../src/index.ts' +import { allocOverlapped, allocPtrSlot, decodePtr, isInvalidHandle, isNullPtr, win32 } from '../src/ffi.ts' +import type { NativePtr, Win32Bindings } from '../src/ffi.ts' +import * as abi from '../src/win32-abi.ts' + +const isWin32 = process.platform === 'win32' + +/** FILE_READ_DATA (winnt.h line ~5895): the harmless mask the explicit test ACE grants. */ +const FILE_READ_DATA = 0x0001 + +/** koffi SID layout: revision@0, subAuthorityCount@1, identifierAuthority@2 (6 bytes, big-endian), subAuthority@8. */ +const SID_STRUCT = koffi.struct('DSH_ACL_SPEC_SID', { + revision: 'uint8', + subAuthorityCount: 'uint8', + identifierAuthority: 'uint8[6]', + subAuthority: 'uint32[8]', +}) + +interface SidLayout { + revision: number + subAuthorityCount: number + identifierAuthority: number[] + subAuthority: number[] +} + +/** One direct (explicit, non-inherited) allow ACE of a directory DACL. */ +interface DirectAce { + sid: string + mask: number +} + +/** Convert one SID string to a LocalAlloc'd SID pointer (caller frees). */ +function sidFromString(api: Win32Bindings, sid: string): NativePtr { + const slot = allocPtrSlot() + if (api.convertStringSidToSidW(sid, slot) === 0) throw new Error(`ConvertStringSidToSidW failed for ${sid}`) + const ptr = decodePtr(slot) + if (ptr === null) throw new Error(`ConvertStringSidToSidW returned null for ${sid}`) + return ptr +} + +/** Stringify a decoded SID layout (identifierAuthority bytes 2..5 are the big-endian value). */ +function sidString(sid: SidLayout): string { + const authority = ((sid.identifierAuthority[2] ?? 0) << 24) + | ((sid.identifierAuthority[3] ?? 0) << 16) + | ((sid.identifierAuthority[4] ?? 0) << 8) + | (sid.identifierAuthority[5] ?? 0) + const subs = sid.subAuthority.slice(0, sid.subAuthorityCount).join('-') + return `S-${sid.revision}-${authority}${sid.subAuthorityCount > 0 ? `-${subs}` : ''}` +} + +/** + * Read the directory's explicit allow ACEs (inherited ACEs excluded): each + * ACE header is AceType@0, AceFlags@1, AceSize@2 (winnt.h lines ~3477-3480); + * ACCESS_ALLOWED_ACE stores Mask@4 and the inline SID@8. The ACL pointer sits + * inside the descriptor allocation — only the descriptor is LocalFree'd. + */ +function readDirectAces(api: Win32Bindings, path: string): DirectAce[] { + const ownerSlot = allocPtrSlot() + const groupSlot = allocPtrSlot() + const daclSlot = allocPtrSlot() + const saclSlot = allocPtrSlot() + const descriptorSlot = allocPtrSlot() + const readResult = api.getNamedSecurityInfoW( + path, abi.SE_FILE_OBJECT, abi.DACL_SECURITY_INFORMATION, + ownerSlot, groupSlot, daclSlot, saclSlot, descriptorSlot, + ) + if (readResult !== abi.ERROR_SUCCESS) throw new Error(`GetNamedSecurityInfoW failed (${readResult}) for ${path}`) + const acl = decodePtr(daclSlot) + const descriptor = decodePtr(descriptorSlot) + try { + if (acl === null) return [] + const aclSize = koffi.decode(acl, 2, 'uint16') as number + const aces: DirectAce[] = [] + for (let offset = 8; offset + 8 <= aclSize;) { + const flags = koffi.decode(acl, offset + 1, 'uint8') as number + const aceSize = koffi.decode(acl, offset + 2, 'uint16') as number + if ((flags & abi.INHERITED_ACE) === 0) { + aces.push({ sid: sidString(koffi.decode(acl, offset + 8, SID_STRUCT) as SidLayout), mask: koffi.decode(acl, offset + 4, 'uint32') as number }) + } + offset += aceSize + } + return aces + } finally { + if (descriptor !== null) api.localFree(descriptor) + } +} + +describe.skipIf(!isWin32)('ACL editing', () => { + const scratchDirs: string[] = [] + afterEach(() => { + for (const dir of scratchDirs.splice(0)) rmSync(dir, { recursive: true, force: true }) + }) + + function scratch(): string { + const dir = mkdtempSync(join(tmpdir(), 'dsh-acl-edit-')) + scratchDirs.push(dir) + return dir + } + + it('grantWrite merges into the current DACL: an explicit Users ACE survives grant+revoke', async () => { + const api = await win32() + const dir = scratch() + const usersSid = sidFromString(api, 'S-1-5-32-545') + const orphanSid = sidFromString(api, 'S-1-4-4242-1') + try { + // Install one explicit ACE (Users + benign read mask) with the + // package's own bindings, exactly like a pre-existing explicit DACL + // entry another sandbox instance or administrator added. + const newAclSlot = allocPtrSlot() + const mergeResult = api.setEntriesInAclW(1, buildExplicitAccess(usersSid, abi.GRANT_ACCESS, FILE_READ_DATA), null, newAclSlot) + expect(mergeResult, `SetEntriesInAclW setup (${mergeResult})`).toBe(abi.ERROR_SUCCESS) + const newAcl = decodePtr(newAclSlot) + expect(newAcl).not.toBeNull() + const applyResult = api.setNamedSecurityInfoW( + dir, abi.SE_FILE_OBJECT, abi.DACL_SECURITY_INFORMATION, null, null, newAcl, null, + ) + const freed = newAcl === null ? null : api.localFree(newAcl) + expect(applyResult, `SetNamedSecurityInfoW setup (${applyResult})`).toBe(abi.ERROR_SUCCESS) + expect(isNullPtr(freed)).toBe(true) + + grantWrite(api, dir, orphanSid) + revokeWrite(api, dir, orphanSid) + + const aces = readDirectAces(api, dir) + expect(aces.some(ace => ace.sid === 'S-1-5-32-545')).toBe(true) // explicit ACE preserved + expect(aces.some(ace => ace.sid === 'S-1-4-4242-1')).toBe(false) // orphan grant fully removed + } finally { + if (!isNullPtr(usersSid)) api.localFree(usersSid) + if (!isNullPtr(orphanSid)) api.localFree(orphanSid) + } + }) + + it('interleaved sandbox instances: A.init → B.init → A.dispose → B.dispose leaves neither ACE', async () => { + const api = await win32() + const dir = scratch() + const sandboxA = new AclSandbox({ writableDirs: [dir], tempDir: null, writeSid: 'S-1-4-9000-1' }) + const sandboxB = new AclSandbox({ writableDirs: [dir], tempDir: null, writeSid: 'S-1-4-9000-2' }) + await sandboxA.init() + await sandboxB.init() + sandboxA.dispose() + sandboxB.dispose() + const aces = readDirectAces(api, dir) + expect(aces.some(ace => ace.sid === 'S-1-4-9000-1')).toBe(false) + expect(aces.some(ace => ace.sid === 'S-1-4-9000-2')).toBe(false) + }) + + it('the per-path lock is exclusive: a second immediate lock attempt fails with ERROR_LOCK_VIOLATION until release', async () => { + const api = await win32() + const dir = scratch() + const lockPath = lockFilePath(api, dir) + const open = (): NativePtr => api.createFileW( + lockPath, abi.GENERIC_READ | abi.GENERIC_WRITE, + abi.FILE_SHARE_READ | abi.FILE_SHARE_WRITE, null, abi.OPEN_ALWAYS, 0, null, + ) + const first = open() + const second = open() + expect(isInvalidHandle(first)).toBe(false) + expect(isInvalidHandle(second)).toBe(false) + try { + expect(api.lockFileEx(first, abi.LOCKFILE_EXCLUSIVE_LOCK, 0, 1, 0, allocOverlapped())).toBe(1) + expect(api.lockFileEx(second, abi.LOCKFILE_EXCLUSIVE_LOCK | abi.LOCKFILE_FAIL_IMMEDIATELY, 0, 1, 0, allocOverlapped())).toBe(0) + expect(api.getLastError()).toBe(abi.ERROR_LOCK_VIOLATION) + expect(api.unlockFileEx(first, 0, 1, 0, allocOverlapped())).toBe(1) + expect(api.lockFileEx(second, abi.LOCKFILE_EXCLUSIVE_LOCK | abi.LOCKFILE_FAIL_IMMEDIATELY, 0, 1, 0, allocOverlapped())).toBe(1) + expect(api.unlockFileEx(second, 0, 1, 0, allocOverlapped())).toBe(1) + } finally { + api.closeHandle(first) + api.closeHandle(second) + rmSync(lockPath, { force: true }) + } + }) + + it('withPathLock serializes the action and releases the lock even when the action throws', async () => { + const api = await win32() + const dir = scratch() + const lockPath = lockFilePath(api, dir) + let attempts = 0 + expect(() => withPathLock(api, dir, () => { + attempts++ + throw new Error('action failure') + })).toThrow('action failure') + expect(attempts).toBe(1) + // The lock was released: a fresh immediate lock succeeds. + const handle = api.createFileW( + lockPath, abi.GENERIC_READ | abi.GENERIC_WRITE, + abi.FILE_SHARE_READ | abi.FILE_SHARE_WRITE, null, abi.OPEN_ALWAYS, 0, null, + ) + expect(isInvalidHandle(handle)).toBe(false) + try { + expect(api.lockFileEx(handle, abi.LOCKFILE_EXCLUSIVE_LOCK | abi.LOCKFILE_FAIL_IMMEDIATELY, 0, 1, 0, allocOverlapped())).toBe(1) + expect(api.unlockFileEx(handle, 0, 1, 0, allocOverlapped())).toBe(1) + } finally { + api.closeHandle(handle) + rmSync(lockPath, { force: true }) + } + }) + + it('the applied grant mask carries DELETE and FILE_DELETE_CHILD (never WRITE_DAC/WRITE_OWNER)', async () => { + const api = await win32() + const dir = scratch() + const sandbox = new AclSandbox({ writableDirs: [dir], tempDir: null, writeSid: 'S-1-4-1234-5' }) + try { + await sandbox.init() + const grant = readDirectAces(api, dir).find(ace => ace.sid === 'S-1-4-1234-5') + expect(grant).toBeDefined() + const mask = grant?.mask ?? 0 + expect(mask).toBe(abi.GRANT_MASK) + expect(mask & abi.DELETE).toBe(abi.DELETE) + expect(mask & abi.FILE_DELETE_CHILD).toBe(abi.FILE_DELETE_CHILD) + expect(mask & 0x00040000).toBe(0) // WRITE_DAC must never be granted + expect(mask & 0x00080000).toBe(0) // WRITE_OWNER must never be granted + } finally { + sandbox.dispose() + } + }) +}) diff --git a/packages/sandbox/sandbox-windows-acl/tests/failure-paths.spec.ts b/packages/sandbox/sandbox-windows-acl/tests/failure-paths.spec.ts new file mode 100644 index 0000000000..14f370602c --- /dev/null +++ b/packages/sandbox/sandbox-windows-acl/tests/failure-paths.spec.ts @@ -0,0 +1,112 @@ +/** + * Failure-path unit tests with minimal stub binding tables: the spawn + * helpers must close every handle they created before throwing, and + * getTempPath must refuse to decode a buffer GetTempPathW never wrote. + * Pure stubs — no real Win32 calls, so these run on every platform. + */ + +import { describe, expect, it, vi } from 'vitest' +import koffi from 'koffi' + +import { PROCESS_INFORMATION, getTempPath } from '../src/ffi.ts' +import type { NativePtr, Win32Bindings } from '../src/ffi.ts' +import { Win32Error } from '../src/errors.ts' +import { spawnSandboxed, spawnSandboxedInherited } from '../src/spawn.ts' + +const PVOID = koffi.pointer('void') + +/** The stub the CreateProcessAsUserW failure branch needs: pipes "succeed", the spawn fails with Win32 5. */ +function pipeFailureApi(): { api: Win32Bindings; closed: bigint[]; closeHandle: ReturnType } { + const closed: bigint[] = [] + let next = 1n + const closeHandle = vi.fn((handle: NativePtr) => { + closed.push(handle) + return 1 + }) + const api = { + createPipe: vi.fn((readSlot: NativePtr, writeSlot: NativePtr) => { + koffi.encode(readSlot, PVOID, next++) + koffi.encode(writeSlot, PVOID, next++) + return 1 + }), + setHandleInformation: vi.fn(() => 1), + createProcessAsUserW: vi.fn(() => 0), + getLastError: vi.fn(() => 5), // ERROR_ACCESS_DENIED: the failure the branch reports + closeHandle, + formatMessageW: vi.fn(() => 0), + } as unknown as Win32Bindings + return { api, closed, closeHandle } +} + +/** The stub the ResumeThread failure branch needs: everything succeeds until ResumeThread returns 0xFFFFFFFF. */ +function resumeFailureApi(): { api: Win32Bindings; closed: bigint[]; closeHandle: ReturnType } { + const closed: bigint[] = [] + let std = 50n + const closeHandle = vi.fn((handle: NativePtr) => { + closed.push(handle) + return 1 + }) + const api = { + createJobObjectW: vi.fn(() => 100n), + setInformationJobObject: vi.fn(() => 1), + getStdHandle: vi.fn(() => std++), + setHandleInformation: vi.fn(() => 1), + createProcessAsUserW: vi.fn(( + _token: unknown, _app: unknown, _cmd: unknown, _pa: unknown, _ta: unknown, + _inherit: unknown, _flags: unknown, _env: unknown, _cwd: unknown, _si: unknown, processInfo: NativePtr, + ) => { + koffi.encode(processInfo, PROCESS_INFORMATION, { hProcess: 200n, hThread: 201n, dwProcessId: 1234, dwThreadId: 5678 }) + return 1 + }), + assignProcessToJobObject: vi.fn(() => 1), + resumeThread: vi.fn(() => 0xFFFFFFFF), + getLastError: vi.fn(() => 5), + closeHandle, + formatMessageW: vi.fn(() => 0), + } as unknown as Win32Bindings + return { api, closed, closeHandle } +} + +describe('spawn failure paths close their handles', () => { + // A dummy token value; the stubbed spawn never reads it. + const token = 1n as NativePtr + + it('spawnSandboxed closes all six pipe handles before throwing when CreateProcessAsUserW fails', () => { + const { api, closed, closeHandle } = pipeFailureApi() + let caught: unknown + try { + spawnSandboxed(api, token, { command: 'probe.exe', args: [], cwd: 'C:\\' }) + } catch (error) { + caught = error + } + expect(caught).toBeInstanceOf(Win32Error) + expect((caught as Win32Error).api).toBe('CreateProcessAsUserW') + expect((caught as Win32Error).win32Code).toBe(5) + expect(closeHandle).toHaveBeenCalledTimes(6) + expect(closed).toEqual([1n, 2n, 3n, 4n, 5n, 6n]) + }) + + it('spawnSandboxedInherited closes thread, process, and kill-on-close job before throwing when ResumeThread fails', () => { + const { api, closed, closeHandle } = resumeFailureApi() + let caught: unknown + try { + spawnSandboxedInherited(api, token, { command: 'probe.exe', args: [], cwd: 'C:\\' }) + } catch (error) { + caught = error + } + expect(caught).toBeInstanceOf(Win32Error) + expect((caught as Win32Error).api).toBe('ResumeThread') + expect((caught as Win32Error).win32Code).toBe(5) + // thread, process, job — closing the job triggers kill-on-close so the + // suspended child dies instead of hanging until this process exits. + expect(closeHandle).toHaveBeenCalledTimes(3) + expect(closed).toEqual([201n, 200n, 100n]) + }) +}) + +describe('getTempPath buffer defense', () => { + it('throws a clear error instead of decoding a buffer GetTempPathW never wrote', () => { + const api = { getTempPathW: vi.fn(() => 300) } as unknown as Win32Bindings // 300 > the 261-char buffer + expect(() => getTempPath(api)).toThrow(/GetTempPathW failed \(Win32 122\): required 300/u) + }) +}) diff --git a/packages/sandbox/sandbox-windows-acl/tests/quote.spec.ts b/packages/sandbox/sandbox-windows-acl/tests/quote.spec.ts new file mode 100644 index 0000000000..5af00fb9cb --- /dev/null +++ b/packages/sandbox/sandbox-windows-acl/tests/quote.spec.ts @@ -0,0 +1,88 @@ +/** + * quoteArg unit tests plus a round-trip through the REAL CommandLineToArgvW + * parser (shell32.dll, shellapi.h line ~867: + * `LPWSTR *CommandLineToArgvW(LPCWSTR lpCmdLine, int *pNumArgs)`) on win32. + * + * CommandLineToArgvW applies the documented backslash rule (2n backslashes + * before a quote produce n backslashes and toggle quoting; 2n+1 produce n + * backslashes and a literal quote) to every token EXCEPT the first — the + * first token is parsed with backslashes literal and quotes toggling + * (verified empirically on this machine, Windows 11 build 26200). The + * round-trip therefore prepends a plain program token, exactly like + * buildCommandLine's real callers do, so the arguments under test land on + * the rule-applying tokens. + * + * Reading argv from CommandLineToArgvW: koffi cannot decode the returned + * LPWSTR* contents directly (the pointed-to strings are not koffi-registered + * references), so each string is copied with lstrcpynW (winbase.h line + * ~1500) into a Node Buffer and read as UTF-16LE; lengths come from + * lstrlenW (winbase.h line ~1506); the argv block is freed with LocalFree + * (winbase.h line ~1127) — CommandLineToArgvW's documented contract. + */ + +import { describe, expect, it } from 'vitest' + +import { buildCommandLine, quoteArg } from '../src/spawn.ts' + +const isWin32 = process.platform === 'win32' + +/** + * Table cases: input argv entry → the exact command-line fragment quoteArg + * must produce. Trailing-backslash inputs are the regression: the closing + * quote must be preceded by DOUBLED backslashes, or the parser reads them as + * escaping the closing quote. + */ +const cases: Array<[input: string, quoted: string]> = [ + ['', '""'], + ['a', 'a'], + ['a b', '"a b"'], + ['a"b', '"a\\"b"'], + ['a\\b', 'a\\b'], + ['a b\\', '"a b\\\\"'], + ['a b\\\\', '"a b\\\\\\\\"'], + ['a b\\\\\\', '"a b\\\\\\\\\\\\"'], + ['a\\\\"b', '"a\\\\\\\\\\"b"'], +] + +describe('quoteArg', () => { + it.each(cases)('quotes %j as %j', (input, quoted) => { + expect(quoteArg(input)).toBe(quoted) + }) +}) + +describe.skipIf(!isWin32)('CommandLineToArgvW round-trip', () => { + it('parses quoteArg+join back to the exact original argv', async () => { + const { default: koffi } = await import('koffi') + const PVOID = koffi.pointer('void') + const shell32 = koffi.load('shell32.dll') + const kernel32 = koffi.load('kernel32.dll') + const commandLineToArgvW = shell32.func('__stdcall', 'CommandLineToArgvW', PVOID, ['str16', koffi.pointer('int')]) + const lstrcpynW = kernel32.func('__stdcall', 'lstrcpynW', PVOID, [PVOID, PVOID, 'int']) + const lstrlenW = kernel32.func('__stdcall', 'lstrlenW', 'int', [PVOID]) + const localFree = kernel32.func('__stdcall', 'LocalFree', PVOID, [PVOID]) + + const parse = (commandLine: string): string[] => { + const countSlot = koffi.alloc('int', 1) as unknown + const argvBlock = commandLineToArgvW(commandLine, countSlot) as unknown + try { + if (argvBlock === null) throw new Error('CommandLineToArgvW returned NULL') + const count = koffi.decode(countSlot, 0, 'int') as number + const table = Buffer.from(koffi.view(argvBlock, count * 8)) + const parsed: string[] = [] + for (let index = 0; index < count; index++) { + const stringAddress = table.readBigUInt64LE(index * 8) + const copied = Buffer.alloc(2048) + lstrcpynW(copied, stringAddress, copied.length / 2) + const length = lstrlenW(copied) as number + parsed.push(copied.subarray(0, length * 2).toString('utf16le')) + } + return parsed + } finally { + localFree(argvBlock) + } + } + + const argv = ['', 'a', 'a b', 'a"b', 'a\\b', 'a b\\', 'a b\\\\', 'a b\\\\\\', 'a\\\\"b'] + expect(parse(buildCommandLine('prog.exe', argv))).toEqual(['prog.exe', ...argv]) + }) +}) diff --git a/packages/sandbox/sandbox-windows-acl/tests/runner.spec.ts b/packages/sandbox/sandbox-windows-acl/tests/runner.spec.ts index f188423425..680d65b253 100644 --- a/packages/sandbox/sandbox-windows-acl/tests/runner.spec.ts +++ b/packages/sandbox/sandbox-windows-acl/tests/runner.spec.ts @@ -12,16 +12,16 @@ import { join } from 'node:path' import { fileURLToPath } from 'node:url' import { afterAll, beforeAll, describe, expect, it } from 'vitest' +import { resolvePwshPath } from '@deepseek-ai/dsh-pwsh-local' + const isWin32 = process.platform === 'win32' const runnerEntry = fileURLToPath(new URL('../src/runner.ts', import.meta.url)) +// Functional probe, not where.exe: spawnSync never throws on a missing +// binary (status null) and where.exe exits 1 without pwsh — only an actual +// pwsh invocation's exit status is truth. function pwshAvailable(): boolean { - try { - spawnSync('where.exe', ['pwsh'], { stdio: 'ignore' }) - return true - } catch { - return false - } + return spawnSync(resolvePwshPath(), ['-NoLogo', '-NoProfile', '-NonInteractive', '-Command', '$true'], { encoding: 'utf8' }).status === 0 } function runRunner(args: string[], timeoutMs = 30_000) { @@ -99,6 +99,31 @@ describe.skipIf(!isWin32 || !pwshAvailable())('windows-acl runner', () => { expect(existsSync(join(writableDir, 'readonly-child-wrote.txt'))).toBe(false) }, 30_000) + it('workspace-write: Remove-Item and Rename-Item succeed in the granted workspace (DELETE + FILE_DELETE_CHILD)', () => { + // Deleting a file and renaming a directory both hit the second access + // check on the workspace itself: the grant must carry DELETE (on the + // object) and FILE_DELETE_CHILD (on its parent). + const victimFile = join(writableDir, 'delete-me.txt') + writeFileSync(victimFile, 'remove me') + const victimDir = join(writableDir, 'rename-me') + mkdirSync(victimDir) + const renamedDir = join(writableDir, 'renamed-by-child') + const probe = [ + "$ErrorActionPreference='SilentlyContinue';", + `try{Remove-Item -LiteralPath '${victimFile}' -ErrorAction Stop;'DELETE-FILE: OK'}catch{'DELETE-FILE: DENIED'};`, + `try{Rename-Item -LiteralPath '${victimDir}' -NewName 'renamed-by-child' -ErrorAction Stop;'RENAME-DIR: OK'}catch{'RENAME-DIR: DENIED'}`, + ].join('') + const result = runRunner([ + '--workspace', writableDir, '--temp', isolatedTemp, '--mode', 'workspace-write', + '--', 'pwsh', '/NoLogo', '/NonInteractive', '/NoProfile', '/Command', probe, + ]) + expect(result.status, `stderr: ${result.stderr}`).toBe(0) + expect(result.stdout).toContain('DELETE-FILE: OK') + expect(result.stdout).toContain('RENAME-DIR: OK') + expect(existsSync(victimFile)).toBe(false) + expect(existsSync(renamedDir)).toBe(true) + }, 30_000) + it('runner-side failure: signature on stderr and exit 127, the command never runs', () => { const result = runRunner(['--workspace', writableDir, '--temp', isolatedTemp, '--mode', 'workspace-write']) expect(result.status).toBe(127) diff --git a/packages/sandbox/sandbox-windows-acl/verify/abi-probe.cpp b/packages/sandbox/sandbox-windows-acl/verify/abi-probe.cpp index 914326acdc..866fda7160 100644 --- a/packages/sandbox/sandbox-windows-acl/verify/abi-probe.cpp +++ b/packages/sandbox/sandbox-windows-acl/verify/abi-probe.cpp @@ -88,6 +88,20 @@ int wmain() P(FILE_GENERIC_WRITE); P((FILE_GENERIC_WRITE & ~STANDARD_RIGHTS_WRITE)); P(STANDARD_RIGHTS_WRITE); + P(DELETE); + P(FILE_DELETE_CHILD); + P(((FILE_GENERIC_WRITE | DELETE | FILE_DELETE_CHILD) & ~STANDARD_RIGHTS_WRITE)); + + P(FILE_SHARE_READ); + P(FILE_SHARE_WRITE); + P(FILE_SHARE_DELETE); + P(GENERIC_READ); + P(GENERIC_WRITE); + P(OPEN_ALWAYS); + P(LOCKFILE_EXCLUSIVE_LOCK); + P(LOCKFILE_FAIL_IMMEDIATELY); + P(ERROR_LOCK_VIOLATION); + P(INHERITED_ACE); P(DISABLE_MAX_PRIVILEGE); P(SANDBOX_INERT); @@ -163,7 +177,14 @@ int wmain() static_assert(TOKEN_QUERY == 0x8 && TOKEN_DUPLICATE == 0x2 && TOKEN_ADJUST_DEFAULT == 0x80 && TOKEN_ASSIGN_PRIMARY == 0x1, "token rights"); static_assert(SE_GROUP_LOGON_ID == 0xC0000000, "logon id attr"); static_assert(FILE_GENERIC_WRITE == 0x120116, "generic write"); - static_assert((FILE_GENERIC_WRITE & ~STANDARD_RIGHTS_WRITE) == 0x100116, "grant mask"); + static_assert((FILE_GENERIC_WRITE & ~STANDARD_RIGHTS_WRITE) == 0x100116, "poc grant mask"); + static_assert(DELETE == 0x10000 && FILE_DELETE_CHILD == 0x40, "delete rights"); + static_assert(((FILE_GENERIC_WRITE | DELETE | FILE_DELETE_CHILD) & ~STANDARD_RIGHTS_WRITE) == 0x110156, "sandbox grant mask"); + static_assert(FILE_SHARE_READ == 0x1 && FILE_SHARE_WRITE == 0x2 && FILE_SHARE_DELETE == 0x4, "share modes"); + static_assert(OPEN_ALWAYS == 4, "open always"); + static_assert(LOCKFILE_EXCLUSIVE_LOCK == 0x2 && LOCKFILE_FAIL_IMMEDIATELY == 0x1, "lockfile flags"); + static_assert(ERROR_LOCK_VIOLATION == 33, "lock violation"); + static_assert(INHERITED_ACE == 0x10, "inherited ace flag"); static_assert(GRANT_ACCESS == 1 && REVOKE_ACCESS == 4, "access modes"); static_assert(SUB_CONTAINERS_AND_OBJECTS_INHERIT == 0x3, "inheritance"); static_assert(CREATE_NO_WINDOW == 0x08000000, "create no window"); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b06386f144..d08966c17a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -4654,6 +4654,9 @@ importers: '@deepseek-ai/dsh-invariants': specifier: workspace:^ version: link:../../support/invariants + '@deepseek-ai/dsh-pwsh-local': + specifier: workspace:^ + version: link:../../bash/pwsh-local '@deepseek-ai/dsh-sandbox-local': specifier: workspace:^ version: link:../sandbox-local