diff --git a/src/main/artifacts/artifact-create-intent-store.test.ts b/src/main/artifacts/artifact-create-intent-store.test.ts index 70cdbec4bb1..2ffe57e2dc3 100644 --- a/src/main/artifacts/artifact-create-intent-store.test.ts +++ b/src/main/artifacts/artifact-create-intent-store.test.ts @@ -1,4 +1,3 @@ -import { execFileSync } from 'node:child_process' import { mkdtemp, readFile, readdir, rm, stat, truncate, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -16,9 +15,14 @@ import { getOrCreateArtifactCreateIntent, removeArtifactCreateIntent } from './artifact-create-intent-store' +import { runProcessSync } from '../../shared/child-process/run-process' +import { __resetSecureFileWindowsUserSidForTests } from '../../shared/secure-file' import type { ArtifactShareScope } from './artifact-share-record-store' -vi.mock('node:child_process', () => ({ execFile: vi.fn(), execFileSync: vi.fn() })) +vi.mock('../../shared/child-process/run-process', () => ({ + runProcess: vi.fn(), + runProcessSync: vi.fn() +})) const createdPaths: string[] = [] const scope: ArtifactShareScope = { @@ -168,12 +172,27 @@ describe('artifact create intent store', () => { expect((await readdir(directory)).some((name) => name.endsWith('.tmp'))).toBe(false) }) - it('hardens one Windows journal directory without per-file PowerShell launches', async () => { + it('hardens one Windows journal directory without per-file ACL launches', async () => { const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform') Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) - vi.mocked(execFileSync).mockImplementation((file) => - String(file).endsWith('whoami.exe') ? '"USER","S-1-5-21-1000"' : '' - ) + const ok = { code: 0, signal: null, stdout: '', stderr: '', timedOut: false } + // Earlier cases in this file already resolved (and cached) the SID against an unstubbed mock. + __resetSecureFileWindowsUserSidForTests() + vi.mocked(runProcessSync).mockImplementation((spec) => { + if (spec.program.endsWith('whoami.exe')) { + return { ...ok, stdout: '"USER","S-1-5-21-1000"' } + } + const args = spec.args ?? [] + if (args.length > 1) { + return ok // /reset and the /grant:r pass + } + // The verify pass re-reads the DACL; answer with the three protected inheritable rules. + const rules = ['host\\me', 'NT AUTHORITY\\SYSTEM', 'BUILTIN\\Administrators'].map( + (name, index) => + index === 0 ? `${args[0]} ${name}:(OI)(CI)(F)` : ` ${name}:(OI)(CI)(F)` + ) + return { ...ok, stdout: `${rules.join('\r\n')}\r\n\r\nSuccessfully processed 1 files\r\n` } + }) try { const userDataPath = await createUserDataPath() getOrCreateArtifactCreateIntent( @@ -193,16 +212,20 @@ describe('artifact create intent store', () => { body ) - const powershellCalls = vi - .mocked(execFileSync) - .mock.calls.filter(([file]) => String(file).endsWith('powershell.exe')) - expect(powershellCalls).toHaveLength(1) - expect((powershellCalls[0]![1] as string[]).at(-1)).toBe('1') + // One harden across both intents: counted by its /reset pass, which opens each harden. + const aclCalls = vi + .mocked(runProcessSync) + .mock.calls.map(([spec]) => spec) + .filter((spec) => spec.program.endsWith('icacls.exe')) + expect(aclCalls.filter((spec) => spec.args?.includes('/reset'))).toHaveLength(1) + // The child intent files rely on inheritance, so the directory rules must carry (OI)(CI). + const grant = aclCalls.find((spec) => spec.args?.includes('/grant:r')) + expect(grant?.args?.filter((arg) => arg.endsWith(':(OI)(CI)(F)'))).toHaveLength(3) } finally { if (originalPlatform) { Object.defineProperty(process, 'platform', originalPlatform) } - vi.mocked(execFileSync).mockReset() + vi.mocked(runProcessSync).mockReset() } }) diff --git a/src/shared/child-process/__fixtures__/child-process-import-allowlist.txt b/src/shared/child-process/__fixtures__/child-process-import-allowlist.txt index f634c5b8b56..2507bc6a9a2 100644 --- a/src/shared/child-process/__fixtures__/child-process-import-allowlist.txt +++ b/src/shared/child-process/__fixtures__/child-process-import-allowlist.txt @@ -178,5 +178,4 @@ src/shared/fish-binary-requirement.ts src/shared/process-table-snapshot.ts src/shared/pty-slave-line-discipline-echo.ts src/shared/ripgrep-process-availability.ts -src/shared/secure-path-windows-acl.ts src/shared/shell-process-readiness.ts diff --git a/src/shared/secure-file.test.ts b/src/shared/secure-file.test.ts index d7b7e9e4415..0645f6c5feb 100644 --- a/src/shared/secure-file.test.ts +++ b/src/shared/secure-file.test.ts @@ -1,8 +1,8 @@ -import { execFile, execFileSync } from 'node:child_process' import { chmodSync, mkdirSync, mkdtempSync, rmSync, statSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { runProcess, runProcessSync } from './child-process/run-process' import { __getSecureFileHardeningCacheStateForTests, __resetSecureFileHardenedPathsForTests, @@ -14,11 +14,45 @@ import { const posixModeIt = process.platform === 'win32' ? it.skip : it -vi.mock('child_process', () => ({ - execFileSync: vi.fn(), - execFile: vi.fn() +vi.mock('./child-process/run-process', () => ({ + runProcess: vi.fn(), + runProcessSync: vi.fn() })) +const OK = { code: 0, signal: null, stdout: '', stderr: '', timedOut: false } + +type FakeSpec = { program: string; args?: readonly string[] } + +// Rights handed to the last /grant:r pass per path, so the verify pass can echo a matching readback. +const grantedRights = new Map() + +/** + * Stands in for icacls across all three passes. The verify pass has to answer with a real-shaped + * `icacls ` readback or every harden would report failure, so this models the format. + */ +function fakeIcacls(spec: FakeSpec): typeof OK { + const args = spec.args ?? [] + const path = args[0] ?? '' + const grantIndex = args.indexOf('/grant:r') + if (grantIndex !== -1) { + const grant = args[grantIndex + 1]! + grantedRights.set(path, grant.slice(grant.lastIndexOf(':(') + 1)) + return OK + } + if (args.length > 1) { + return OK // /reset + } + const rights = grantedRights.get(path) ?? '(F)' + const principals = ['host\\me', 'NT AUTHORITY\\SYSTEM', 'BUILTIN\\Administrators'] + const aceLines = principals.map((name, index) => + index === 0 ? `${path} ${name}:${rights}` : ` ${name}:${rights}` + ) + return { + ...OK, + stdout: `${aceLines.join('\r\n')}\r\n\r\nSuccessfully processed 1 files; Failed processing 0 files\r\n` + } +} + describe('hardenSecurePath', () => { const originalSystemRoot = process.env.SystemRoot const originalWindir = process.env.WINDIR @@ -30,24 +64,18 @@ describe('hardenSecurePath', () => { delete process.env.WINDIR __resetSecureFileWindowsUserSidForTests() __resetSecureFileHardenedPathsForTests() - vi.mocked(execFileSync).mockReset() - vi.mocked(execFile).mockReset() - // execFileSync handles whoami.exe (SID lookup) and the SYNCHRONOUS PowerShell file-ACL - // path used by writeSecureFile. The directory + read-path re-harden use async execFile. - vi.mocked(execFileSync).mockImplementation((file) => { - if (file === 'C:\\Windows\\System32\\whoami.exe') { - return '"USER","S-1-5-21-1000"' + vi.mocked(runProcessSync).mockReset() + vi.mocked(runProcess).mockReset() + grantedRights.clear() + // runProcessSync serves whoami.exe (SID lookup) and the SYNCHRONOUS icacls file-ACL path + // used by writeSecureFile. Directory + read-path re-hardens use async runProcess. + vi.mocked(runProcessSync).mockImplementation((spec) => { + if (spec.program === 'C:\\Windows\\System32\\whoami.exe') { + return { ...OK, stdout: '"USER","S-1-5-21-1000"' } } - // Synchronous PowerShell ACL apply succeeds (returns empty stdout). - return '' - }) - // Directory + read-path PowerShell is called asynchronously; simulate immediate success - vi.mocked(execFile).mockImplementation((_file, _args, _opts, callback) => { - if (typeof callback === 'function') { - callback(null, '', '') - } - return {} as ReturnType + return fakeIcacls(spec) }) + vi.mocked(runProcess).mockImplementation((spec) => Promise.resolve(fakeIcacls(spec))) }) afterEach(() => { @@ -71,64 +99,180 @@ describe('hardenSecurePath', () => { } }) - it('rewrites Windows ACLs through the system PowerShell path', () => { + it('rewrites Windows ACLs through icacls, purging explicit ACEs before granting', async () => { hardenSecurePath('C:\\Users\\me\\.orca\\secret.json', { isDirectory: false, platform: 'win32' }) + await flushAsyncAcl() // whoami.exe called synchronously to obtain SID - expect(execFileSync).toHaveBeenNthCalledWith( - 1, - 'C:\\Windows\\System32\\whoami.exe', - ['/user', '/fo', 'csv', '/nh'], - expect.objectContaining({ encoding: 'utf-8' }) - ) - // PowerShell called asynchronously - const [powershellFile, powershellArgs, powershellOptions] = vi.mocked(execFile).mock.calls[0]! - expect(powershellFile).toBe('C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe') - expect(powershellArgs).toEqual( - expect.arrayContaining([ - '-NoProfile', - '-NonInteractive', - '-ExecutionPolicy', - 'Bypass', - 'C:\\Users\\me\\.orca\\secret.json', - 'S-1-5-21-1000', - '0' - ]) - ) - const script = (powershellArgs as string[])[5]! - expect(script).toContain('SetAccessRuleProtection($true, $false)') - expect(script).toContain('RemoveAccessRuleSpecific') - expect(script).toContain('Unexpected ACL entry') - expect(powershellOptions).toEqual(expect.objectContaining({ windowsHide: true, timeout: 5000 })) - }) - - it('adds inheritable rules when hardening a Windows directory', () => { - hardenSecurePath('C:\\Users\\me\\.orca', { isDirectory: true, platform: 'win32' }) - - const powershellArgs = vi.mocked(execFile).mock.calls[0]![1] as string[] - expect(powershellArgs.at(-1)).toBe('1') - expect(powershellArgs[5]).toContain('ContainerInherit') - expect(powershellArgs[5]).toContain('ObjectInherit') - }) - - it('keeps Windows hardening best-effort when ACL rewriting fails', () => { - // Simulate async PowerShell failure — the callback receives an error - vi.mocked(execFile).mockImplementationOnce((_file, _args, _opts, callback) => { - if (typeof callback === 'function') { - callback(new Error('access denied'), '', '') - } - return {} as ReturnType + expect(vi.mocked(runProcessSync).mock.calls[0]![0]).toMatchObject({ + program: 'C:\\Windows\\System32\\whoami.exe', + args: ['/user', '/fo', 'csv', '/nh'] }) + const specs = vi.mocked(runProcess).mock.calls.map(([spec]) => spec) + expect(specs.map((spec) => spec.program)).toEqual([ + 'C:\\Windows\\System32\\icacls.exe', + 'C:\\Windows\\System32\\icacls.exe', + 'C:\\Windows\\System32\\icacls.exe' + ]) + expect(specs[0]!.args).toEqual(['C:\\Users\\me\\.orca\\secret.json', '/reset', '/q']) + expect(specs[1]!.args).toEqual([ + 'C:\\Users\\me\\.orca\\secret.json', + '/inheritance:r', + '/grant:r', + '*S-1-5-21-1000:(F)', + '/grant:r', + '*S-1-5-18:(F)', + '/grant:r', + '*S-1-5-32-544:(F)', + '/q' + ]) + // The apply is read back: a loosened ACL has to be detectable, not just overwritten. + expect(specs[2]!.args).toEqual(['C:\\Users\\me\\.orca\\secret.json']) + expect(specs[1]!.timeoutMs).toBe(5000) + }) + + it('reports failure when the applied ACL does not read back as expected', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.mocked(runProcess).mockImplementation((spec) => { + if ((spec.args ?? []).length === 1) { + // An inherited ACE survived: the DACL was never protected — the shipped failure mode. + return Promise.resolve({ + ...OK, + stdout: `${spec.args![0]} host\\me:(I)(F)\r\n\r\nSuccessfully processed 1 files\r\n` + }) + } + return Promise.resolve(OK) + }) + + hardenSecurePath('C:\\Users\\me\\.orca\\secret.json', { + isDirectory: false, + platform: 'win32' + }) + await flushAsyncAcl() + + expect(warn).toHaveBeenCalledWith( + '[secure-path.windows-acl] failed to restrict path', + expect.objectContaining({ stage: 'verify' }) + ) + warn.mockRestore() + }) + + // The async branch cannot return its outcome, so a failed apply must not stay cached as success. + it('re-hardens on the next read when an async apply failed', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-secure-file-')) + tempDirs.push(userDataPath) + const targetPath = join(userDataPath, 'secret.json') + writeFileSync(targetPath, '{}') + vi.mocked(runProcess).mockResolvedValue({ ...OK, code: 5, stderr: 'Access is denied.' }) + + hardenExistingSecureFile(targetPath) + await flushAsyncAcl() + hardenExistingSecureFile(targetPath) + await flushAsyncAcl() + + // Both the directory and the file are retried rather than trusted from the failed first pass. + expect(getHardenAclCalls().map(getAclTarget)).toEqual([ + userDataPath, + targetPath, + userDataPath, + targetPath + ]) + warn.mockRestore() + }) + + // /c makes icacls exit 0 while printing "Failed processing 1 files" — a silent no-op by another route. + it('never passes the icacls /c continue-on-error flag', async () => { + hardenSecurePath('C:\\Users\\me\\.orca\\secret.json', { + isDirectory: false, + platform: 'win32' + }) + await flushAsyncAcl() + + for (const [spec] of vi.mocked(runProcess).mock.calls) { + expect(spec.args).not.toContain('/c') + } + }) + + it('adds inheritable rules when hardening a Windows directory', async () => { + hardenSecurePath('C:\\Users\\me\\.orca', { isDirectory: true, platform: 'win32' }) + await flushAsyncAcl() + + const grantArgs = vi.mocked(runProcess).mock.calls[1]![0].args as string[] + expect(grantArgs).toContain('*S-1-5-21-1000:(OI)(CI)(F)') + expect(grantArgs).toContain('*S-1-5-18:(OI)(CI)(F)') + }) + + it('keeps Windows hardening best-effort when ACL rewriting fails', async () => { + vi.mocked(runProcess).mockRejectedValue(new Error('access denied')) + expect(() => hardenSecurePath('C:\\Users\\me\\.orca\\secret.json', { isDirectory: false, platform: 'win32' }) ).not.toThrow() + await expect(flushAsyncAcl()).resolves.toBeUndefined() + }) + + // The old PowerShell command line never reached the grant step at all, so a failure had to be + // visible somewhere; "best effort" may not mean "undetectable". + it('logs when a Windows ACL apply fails instead of swallowing it', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.mocked(runProcess).mockResolvedValue({ ...OK, code: 5, stderr: 'Access is denied.' }) + + hardenSecurePath('C:\\Users\\me\\.orca\\secret.json', { + isDirectory: false, + platform: 'win32' + }) + await flushAsyncAcl() + + expect(warn).toHaveBeenCalledWith( + '[secure-path.windows-acl] failed to restrict path', + expect.objectContaining({ + targetPath: 'C:\\Users\\me\\.orca\\secret.json', + stage: 'reset', + detail: 'Access is denied.' + }) + ) + warn.mockRestore() + }) + + it('reports a failed synchronous ACL apply to the caller and the log', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + vi.mocked(runProcessSync).mockImplementation((spec) => { + if (spec.program === 'C:\\Windows\\System32\\whoami.exe') { + return { ...OK, stdout: '"USER","S-1-5-21-1000"' } + } + return { ...OK, code: 5, stderr: 'Access is denied.' } + }) + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-secure-file-')) + tempDirs.push(userDataPath) + + writeSecureFile(join(userDataPath, 'secret.json'), 'contents') + + expect(warn).toHaveBeenCalledWith( + '[secure-path.windows-acl] failed to restrict path', + expect.objectContaining({ stage: 'reset', detail: 'Access is denied.' }) + ) + warn.mockRestore() + }) + + // Paths past MAX_PATH make icacls report "cannot find the path specified"; the extended prefix is the escape. + it('uses the extended-length prefix for paths past MAX_PATH', async () => { + const longPath = `C:\\Users\\me\\.orca\\${'d'.repeat(300)}\\secret.json` + hardenSecurePath(longPath, { isDirectory: false, platform: 'win32' }) + await flushAsyncAcl() + + for (const [spec] of vi.mocked(runProcess).mock.calls) { + expect(spec.args![0]).toBe(`\\\\?\\${longPath}`) + } }) it('caches successful existing-file hardening within a process', () => { @@ -142,8 +286,8 @@ describe('hardenSecurePath', () => { hardenExistingSecureFile(targetPath) // dir hardened once (path-cached), file hardened once (metadata-cached) — 2 total - expect(getPowerShellCalls()).toHaveLength(2) - expect(getPowerShellCalls().map(getPowerShellTarget)).toEqual([userDataPath, targetPath]) + expect(getHardenAclCalls()).toHaveLength(2) + expect(getHardenAclCalls().map(getAclTarget)).toEqual([userDataPath, targetPath]) }) it('LRU-evicts Windows file hardening entries and safely re-hardens an evicted path', () => { @@ -165,8 +309,8 @@ describe('hardenSecurePath', () => { hardenExistingSecureFile(paths[0]!) - const fileTargets = getPowerShellCalls() - .map(getPowerShellTarget) + const fileTargets = getHardenAclCalls() + .map(getAclTarget) .filter((path) => paths.includes(path)) expect(fileTargets).toEqual([...paths, paths[0]]) expect(__getSecureFileHardeningCacheStateForTests().paths).toMatchObject({ @@ -196,8 +340,8 @@ describe('hardenSecurePath', () => { hardenExistingSecureFile(files[0]!) - const directoryTargets = getPowerShellCalls() - .map(getPowerShellTarget) + const directoryTargets = getHardenAclCalls() + .map(getAclTarget) .filter((path) => directories.includes(path)) expect(directoryTargets).toEqual([...directories, directories[0]]) expect(__getSecureFileHardeningCacheStateForTests().directories).toMatchObject({ @@ -218,8 +362,8 @@ describe('hardenSecurePath', () => { hardenExistingSecureFile(targetPath) // call 1: dir + file. call 2: dir skipped (path-cached), file re-hardened (new mtime) - expect(getPowerShellCalls()).toHaveLength(3) - expect(getPowerShellCalls().map(getPowerShellTarget)).toEqual([ + expect(getHardenAclCalls()).toHaveLength(3) + expect(getHardenAclCalls().map(getAclTarget)).toEqual([ userDataPath, targetPath, targetPath @@ -236,19 +380,19 @@ describe('hardenSecurePath', () => { writeSecureFile(targetPath, 'second') // The DIRECTORY is hardened async + path-cached: exactly once across both writes. - const asyncTargets = getPowerShellCalls().map(getPowerShellTarget) + const asyncTargets = getHardenAclCalls().map(getAclTarget) expect(asyncTargets).toEqual([userDataPath]) // The credential FILES (tmpFile + renamed target) are hardened SYNCHRONOUSLY on each write. // write 1: tmpFile(1) + targetFile(1) = 2; write 2: tmpFile(1) + targetFile(1) = 2; total 4. - const syncTargets = getSyncPowerShellCalls().map(getPowerShellTarget) + const syncTargets = getSyncHardenAclCalls().map(getAclTarget) expect(syncTargets).toHaveLength(4) expect(syncTargets.filter((entry) => entry === targetPath)).toHaveLength(2) // No directory should be hardened via the synchronous path. expect(syncTargets.filter((entry) => entry === userDataPath)).toHaveLength(0) }) - // Regression test: #4901 — env-store reads at ~2×/s caused a PowerShell storm because the + // Regression test: #4901 — env-store reads at ~2×/s caused an ACL-spawn storm because the // parent directory mtime churned (every secure write updates it), so the mtime-keyed cache // never matched. Directories must be path-cached for the process lifetime. it('does not re-harden the parent directory when its mtime changes between reads', async () => { @@ -268,8 +412,8 @@ describe('hardenSecurePath', () => { hardenExistingSecureFile(targetPath) // The parent directory must be hardened exactly ONCE despite its mtime changing - const dirCalls = getPowerShellCalls().filter( - (call) => getPowerShellTarget(call) === userDataPath + const dirCalls = getHardenAclCalls().filter( + (call) => getAclTarget(call) === userDataPath ) expect(dirCalls).toHaveLength(1) }) @@ -285,27 +429,27 @@ describe('hardenSecurePath', () => { hardenExistingSecureFile(targetPath) hardenExistingSecureFile(targetPath) - const fileCalls = getPowerShellCalls().filter( - (call) => getPowerShellTarget(call) === targetPath + const fileCalls = getHardenAclCalls().filter( + (call) => getAclTarget(call) === targetPath ) expect(fileCalls).toHaveLength(1) }) - it('applies the read-path ACL asynchronously without blocking (async execFile)', () => { + it('applies the read-path ACL asynchronously without blocking (async runProcess)', () => { hardenSecurePath('C:\\Users\\me\\.orca\\secret.json', { isDirectory: false, platform: 'win32' }) - // The default (read/dir) path must launch PowerShell via execFile (async), never sync. - expect(getSyncPowerShellCalls()).toHaveLength(0) - expect(getPowerShellCalls()).toHaveLength(1) + // The default (read/dir) path must launch icacls via runProcess (async), never sync. + expect(getSyncHardenAclCalls()).toHaveLength(0) + expect(getHardenAclCalls()).toHaveLength(1) }) // Security regression guard (#5006 review finding): writeSecureFile must restrict the // credential FILE's ACL SYNCHRONOUSLY before returning. On Windows writeFileSync({mode}) // is a no-op, so an async file ACL would leave the credential briefly readable under the - // parent's inherited (broader) ACL for the ~1-1.5s PowerShell cold-start window. + // parent's inherited (broader) ACL for the duration of the spawn. it('hardens the credential file synchronously while keeping the directory async', () => { Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) const userDataPath = mkdtempSync(join(tmpdir(), 'orca-secure-file-')) @@ -315,13 +459,13 @@ describe('hardenSecurePath', () => { writeSecureFile(targetPath, 'contents') // Directory: async only. - expect(getPowerShellCalls().map(getPowerShellTarget)).toEqual([userDataPath]) + expect(getHardenAclCalls().map(getAclTarget)).toEqual([userDataPath]) // File (tmpFile + renamed target): synchronous only — no async file ACL window. - const syncTargets = getSyncPowerShellCalls().map(getPowerShellTarget) + const syncTargets = getSyncHardenAclCalls().map(getAclTarget) expect(syncTargets).toContain(targetPath) expect(syncTargets.filter((entry) => entry === userDataPath)).toHaveLength(0) // The final published target's ACL must have been applied via the synchronous path. - expect(getPowerShellCalls().map(getPowerShellTarget)).not.toContain(targetPath) + expect(getHardenAclCalls().map(getAclTarget)).not.toContain(targetPath) }) // Nit #1 (review): the synchronous file path must cache as hardened ONLY on confirmed @@ -333,30 +477,30 @@ describe('hardenSecurePath', () => { tempDirs.push(userDataPath) const targetPath = join(userDataPath, 'secret.json') - // First write: the synchronous PowerShell ACL apply throws for every powershell call. - vi.mocked(execFileSync).mockImplementation((file) => { - if (file === 'C:\\Windows\\System32\\whoami.exe') { - return '"USER","S-1-5-21-1000"' + // First write: the synchronous icacls ACL apply throws for every icacls call. + vi.mocked(runProcessSync).mockImplementation((spec) => { + if (spec.program === 'C:\\Windows\\System32\\whoami.exe') { + return { ...OK, stdout: '"USER","S-1-5-21-1000"' } } throw new Error('access denied') }) expect(() => writeSecureFile(targetPath, 'first')).not.toThrow() - const firstWriteTargetCalls = getSyncPowerShellCalls() - .map(getPowerShellTarget) + const firstWriteTargetCalls = getSyncHardenAclCalls() + .map(getAclTarget) .filter((entry) => entry === targetPath) expect(firstWriteTargetCalls).toHaveLength(1) // Second write: ACL apply now succeeds. Because the failed apply was NOT cached, the // target file is hardened again rather than skipped. - vi.mocked(execFileSync).mockImplementation((file) => { - if (file === 'C:\\Windows\\System32\\whoami.exe') { - return '"USER","S-1-5-21-1000"' + vi.mocked(runProcessSync).mockImplementation((spec) => { + if (spec.program === 'C:\\Windows\\System32\\whoami.exe') { + return { ...OK, stdout: '"USER","S-1-5-21-1000"' } } - return '' + return OK }) writeSecureFile(targetPath, 'second') - const allTargetCalls = getSyncPowerShellCalls() - .map(getPowerShellTarget) + const allTargetCalls = getSyncHardenAclCalls() + .map(getAclTarget) .filter((entry) => entry === targetPath) expect(allTargetCalls).toHaveLength(2) }) @@ -374,15 +518,15 @@ describe('hardenSecurePath', () => { writeSecureFile(join(userDataPath, `secret-${i}.json`), `contents-${i}`) } - const dirCalls = getPowerShellCalls().filter( - (call) => getPowerShellTarget(call) === userDataPath + const dirCalls = getHardenAclCalls().filter( + (call) => getAclTarget(call) === userDataPath ) expect(dirCalls).toHaveLength(1) }) - // win32-only guard: on non-win32 platforms no PowerShell is ever spawned (sync or async); + // win32-only guard: on non-win32 platforms no icacls is ever spawned (sync or async); // POSIX hardening uses chmodSync only. - it('never spawns PowerShell on non-win32 platforms', () => { + it('never spawns icacls on non-win32 platforms', () => { Object.defineProperty(process, 'platform', { configurable: true, value: 'linux' }) const userDataPath = mkdtempSync(join(tmpdir(), 'orca-secure-file-')) tempDirs.push(userDataPath) @@ -391,8 +535,8 @@ describe('hardenSecurePath', () => { writeSecureFile(targetPath, 'contents') hardenExistingSecureFile(targetPath) - expect(getPowerShellCalls()).toHaveLength(0) - expect(getSyncPowerShellCalls()).toHaveLength(0) + expect(getHardenAclCalls()).toHaveLength(0) + expect(getSyncHardenAclCalls()).toHaveLength(0) }) posixModeIt('re-hardens a POSIX directory when its metadata changes after caching', () => { @@ -440,22 +584,37 @@ describe('hardenSecurePath', () => { }) }) -const POWERSHELL_SUFFIX = 'WindowsPowerShell\\v1.0\\powershell.exe' - -// Async PowerShell calls (directory hardening + read-path file re-harden). -function getPowerShellCalls(): unknown[][] { - return vi.mocked(execFile).mock.calls.filter(([file]) => String(file).endsWith(POWERSHELL_SUFFIX)) +// Each harden is two icacls passes (/reset then /inheritance:r + grants); counting the /reset +// pass keeps "one harden = one entry" and stays observable synchronously on the async path. +function isAclResetSpec(spec: { program: string; args?: readonly string[] }): boolean { + return spec.program.endsWith('icacls.exe') && (spec.args?.includes('/reset') ?? false) } -// Synchronous PowerShell calls (credential-file ACL on the write path). -function getSyncPowerShellCalls(): unknown[][] { +// Async icacls calls (directory hardening + read-path file re-harden). +function getHardenAclCalls(): { args?: readonly string[] }[] { return vi - .mocked(execFileSync) - .mock.calls.filter(([file]) => String(file).endsWith(POWERSHELL_SUFFIX)) + .mocked(runProcess) + .mock.calls.map(([spec]) => spec) + .filter(isAclResetSpec) } -function getPowerShellTarget(call: unknown[]): string { - return (call[1] as string[])[6]! +// Synchronous icacls calls (credential-file ACL on the write path). +function getSyncHardenAclCalls(): { args?: readonly string[] }[] { + return vi + .mocked(runProcessSync) + .mock.calls.map(([spec]) => spec) + .filter(isAclResetSpec) +} + +function getAclTarget(spec: { args?: readonly string[] }): string { + return spec.args![0]! +} + +// The async harden awaits three icacls passes, so let the chain settle before asserting on it. +async function flushAsyncAcl(): Promise { + for (let i = 0; i < 4; i++) { + await new Promise((resolve) => setTimeout(resolve, 0)) + } } async function waitForFileTimestampTick(): Promise { diff --git a/src/shared/secure-file.ts b/src/shared/secure-file.ts index 639630c7c65..7f54c63580e 100644 --- a/src/shared/secure-file.ts +++ b/src/shared/secure-file.ts @@ -61,9 +61,11 @@ function hardenSecureDirectoryOnce(dirPath: string): void { if (hardenedDirectoryPathsThisProcess.get(dirPath)) { return } - applySecurePathRestriction(dirPath, true, process.platform, false) - // Cache even though the async ACL may still be in flight — dir restriction is best-effort, no retry. + // Cache before the ACL lands so concurrent writes don't restorm; the callback evicts it if the apply failed. hardenedDirectoryPathsThisProcess.set(dirPath, true) + applySecurePathRestriction(dirPath, true, process.platform, false, () => { + hardenedDirectoryPathsThisProcess.delete(dirPath) + }) } function hardenSecurePathOnce(targetPath: string, isDirectory: boolean): boolean { @@ -81,7 +83,11 @@ function hardenSecurePathOnce(targetPath: string, isDirectory: boolean): boolean return true } // Why: async re-harden is safe here — read path hardens each file at most once/process; new files harden synchronously on the write path. - if (applySecurePathRestriction(targetPath, isDirectory, process.platform, false)) { + if ( + applySecurePathRestriction(targetPath, isDirectory, process.platform, false, () => { + hardenedPathsThisProcess.delete(targetPath) + }) + ) { rememberHardenedPath(targetPath, isDirectory) return true } @@ -191,20 +197,29 @@ export function hardenSecurePath( ) } -/** Applies hardening; async Windows calls only report that best-effort ACL work was accepted. */ +/** + * Applies hardening. The async Windows branch cannot know the outcome in time to return it, so it + * reports through `onAsyncFailure` instead — the caller uses that to drop the cache entry rather + * than leave a failed apply recorded as a success. + */ function applySecurePathRestriction( targetPath: string, isDirectory: boolean, platform: NodeJS.Platform, - sync: boolean + sync: boolean, + onAsyncFailure?: () => void ): boolean { if (platform === 'win32') { if (sync) { // Why: apply the ACL synchronously so the credential file isn't briefly readable under inherited ACLs (writeFileSync mode is a no-op on Windows). return restrictWindowsPathSync(targetPath, isDirectory) } - // Why: dir/read-path re-harden runs async to avoid blocking the main thread (#4901); return true optimistically since it's best-effort. - bestEffortRestrictWindowsPath(targetPath, isDirectory) + // Why: dir/read-path re-harden runs async to avoid blocking the main thread (#4901). + bestEffortRestrictWindowsPath(targetPath, isDirectory, (restricted) => { + if (!restricted) { + onAsyncFailure?.() + } + }) return true } chmodSync(targetPath, isDirectory ? 0o700 : 0o600) diff --git a/src/shared/secure-path-windows-acl.ts b/src/shared/secure-path-windows-acl.ts index 4d61dff79e1..7540f88e929 100644 --- a/src/shared/secure-path-windows-acl.ts +++ b/src/shared/secure-path-windows-acl.ts @@ -1,146 +1,255 @@ -import { execFile, execFileSync } from 'node:child_process' import { win32 as pathWin32 } from 'node:path' +import { runProcess, runProcessSync } from './child-process/run-process' +import { windowsSystem32Binary } from './child-process/windows-system-binary' let cachedWindowsUserSid: string | null | undefined -function buildWindowsRestrictAclArgs( +const ACL_TIMEOUT_MS = 5000 + +/** SYSTEM and the local Administrators group: they can take ownership regardless, so denying them buys nothing. */ +const LOCAL_SYSTEM_SID = 'S-1-5-18' +const BUILTIN_ADMINISTRATORS_SID = 'S-1-5-32-544' + +type WindowsAclStep = { + stage: 'reset' | 'grant' | 'verify' + args: string[] + /** Returns a failure reason, or null when the observed state is correct. */ + validate?: (stdout: string) => string | null +} + +/** + * Hardening a path is three `icacls` passes. + * + * `reset` drops every *explicit* ACE, which `/inheritance:r` alone leaves in place — a planted + * `Everyone:(R)` survives the grant pass otherwise. It momentarily restores the inherited ACL, + * which is the ACL the path already has when nothing has hardened it yet. + * + * `verify` re-reads the result. The predecessor had an equivalent check and it never ran, so a + * loosened ACL went unreported for as long as the apply did; an apply that is not read back is + * only half a control. + */ +function buildWindowsRestrictAclSteps( targetPath: string, currentUserSid: string, isDirectory: boolean -): string[] { +): WindowsAclStep[] { + const icaclsPath = toIcaclsPath(targetPath) + // Directories propagate to children (artifact-intent files rely on inheritance); files take no flags. + const rights = isDirectory ? '(OI)(CI)(F)' : '(F)' + const sids = [...new Set([currentUserSid, LOCAL_SYSTEM_SID, BUILTIN_ADMINISTRATORS_SID])] return [ - '-NoProfile', - '-NonInteractive', - '-ExecutionPolicy', - 'Bypass', - '-Command', - WINDOWS_RESTRICT_ACL_SCRIPT, - targetPath, - currentUserSid, - isDirectory ? '1' : '0' + // Never add /c: it makes icacls exit 0 on "Failed processing 1 files", which is a silent no-op by another route. + { stage: 'reset', args: [icaclsPath, '/reset', '/q'] }, + { + stage: 'grant', + args: [ + icaclsPath, + '/inheritance:r', + ...sids.flatMap((sid) => ['/grant:r', `*${sid}:${rights}`]), + '/q' + ] + }, + { + stage: 'verify', + args: [icaclsPath], + validate: (stdout) => validateAcl(stdout, icaclsPath, rights, sids.length) + } ] } -export function bestEffortRestrictWindowsPath(targetPath: string, isDirectory: boolean): void { +/** + * Checks the applied DACL without depending on account-name resolution, which is localized. + * The `(I)` inherited marker and the rights tokens are not. + */ +function validateAcl( + stdout: string, + icaclsPath: string, + expectedRights: string, + expectedCount: number +): string | null { + const observed = parseIcaclsAceRights(stdout, icaclsPath) + if (observed.length !== expectedCount) { + return `expected ${expectedCount} access rules, found ${observed.length}` + } + if (observed.some((rights) => rights.includes('(I)'))) { + return 'inherited access rules survived; the DACL is not protected' + } + const unexpected = observed.filter((rights) => rights !== expectedRights) + if (unexpected.length > 0) { + return `access rules do not grant ${expectedRights}: ${unexpected.join(' ')}` + } + return null +} + +/** + * Pulls the `(I)(F)`-style rights off each ACE line of `icacls `. + * + * The first line carries the path, whose own characters must not be mistaken for an ACE, so it is + * stripped by the exact string that was passed in. A blank line ends the list, before the + * localized "Successfully processed" summary. + */ +function parseIcaclsAceRights(stdout: string, icaclsPath: string): string[] { + const rights: string[] = [] + const lines = stdout.split(/\r?\n/) + for (const [index, rawLine] of lines.entries()) { + const line = + index === 0 && rawLine.startsWith(icaclsPath) ? rawLine.slice(icaclsPath.length) : rawLine + const trimmed = line.trim() + if (!trimmed) { + if (index === 0) { + continue + } + break + } + const separator = trimmed.lastIndexOf(':(') + if (separator !== -1) { + rights.push(trimmed.slice(separator + 1)) + } + } + return rights +} + +/** + * icacls resolves through the MAX_PATH-limited API and fails with "cannot find the path + * specified" past 259 characters; the extended prefix is the documented escape. + */ +function toIcaclsPath(targetPath: string): string { + if (targetPath.length < 260 || targetPath.startsWith('\\\\?\\')) { + return targetPath + } + const normalized = pathWin32.normalize(targetPath) + if (/^[A-Za-z]:\\/.test(normalized)) { + return `\\\\?\\${normalized}` + } + if (normalized.startsWith('\\\\')) { + return `\\\\?\\UNC\\${normalized.slice(2)}` + } + return targetPath +} + +function icaclsProgram(): string { + return windowsSystem32Binary('icacls.exe') +} + +/** + * Why loud: hardening stays best-effort — non-NTFS volumes, network paths and restricted tokens + * fail legitimately and must not crash a write — but a swallowed failure leaves credentials under + * inherited ACLs while every caller believes otherwise. Undetectable best-effort is not a control. + */ +function reportAclFailure(targetPath: string, stage: string, detail: string): void { + console.warn('[secure-path.windows-acl] failed to restrict path', { + targetPath, + stage, + detail: detail.trim().slice(0, 500) + }) +} + +function checkAclStep( + targetPath: string, + step: WindowsAclStep, + result: { code: number | null; stdout: string; stderr: string } +): boolean { + if (result.code !== 0) { + reportAclFailure(targetPath, step.stage, result.stderr || `icacls exited ${result.code}`) + return false + } + const invalid = step.validate?.(result.stdout) + if (invalid) { + reportAclFailure(targetPath, step.stage, invalid) + return false + } + return true +} + +/** + * Applies the ACL without blocking. `onSettled` reports the real outcome, which the caller needs + * because the return value cannot: the cache must not keep claiming a path is hardened when the + * apply that was supposed to harden it failed. + */ +export function bestEffortRestrictWindowsPath( + targetPath: string, + isDirectory: boolean, + onSettled?: (restricted: boolean) => void +): void { const currentUserSid = getCurrentWindowsUserSid() if (!currentUserSid) { + reportAclFailure(targetPath, 'sid-lookup', 'could not resolve the current user SID') + onSettled?.(false) return } - // Why: async to avoid blocking the main thread — sync PowerShell cold-start (~1-1.5s) on the frequent read path stormed it (#4901). - execFile( - getWindowsSystemToolPath('WindowsPowerShell\\v1.0\\powershell.exe'), - buildWindowsRestrictAclArgs(targetPath, currentUserSid, isDirectory), - { - windowsHide: true, - timeout: 5000 - }, - () => { - // Why: ignore errors — hardening is best-effort; PowerShell ACL APIs may be unavailable or locked down. + // Why async: hardening runs on the read path, and blocking it on a spawn stormed the main thread (#4901). + void runRestrictAclStepsAsync( + targetPath, + buildWindowsRestrictAclSteps(targetPath, currentUserSid, isDirectory) + ).then(onSettled) +} + +async function runRestrictAclStepsAsync( + targetPath: string, + steps: readonly WindowsAclStep[] +): Promise { + for (const step of steps) { + try { + const result = await runProcess({ + program: icaclsProgram(), + args: step.args, + timeoutMs: ACL_TIMEOUT_MS + }) + if (!checkAclStep(targetPath, step, result)) { + return false + } + } catch (error) { + reportAclFailure(targetPath, step.stage, String(error)) + return false } - ) + } + return true } export function restrictWindowsPathSync(targetPath: string, isDirectory: boolean): boolean { const currentUserSid = getCurrentWindowsUserSid() if (!currentUserSid) { + reportAclFailure(targetPath, 'sid-lookup', 'could not resolve the current user SID') return false } - // Why: file must not be published until its ACL is actually restricted, so block and report real success (read path stays async, #4901). - try { - execFileSync( - getWindowsSystemToolPath('WindowsPowerShell\\v1.0\\powershell.exe'), - buildWindowsRestrictAclArgs(targetPath, currentUserSid, isDirectory), - { - stdio: ['ignore', 'ignore', 'ignore'], - windowsHide: true, - timeout: 5000 + // Why sync: the file must not be published until its ACL is actually restricted (read path stays async, #4901). + for (const step of buildWindowsRestrictAclSteps(targetPath, currentUserSid, isDirectory)) { + try { + const result = runProcessSync({ + program: icaclsProgram(), + args: step.args, + timeoutMs: ACL_TIMEOUT_MS + }) + if (!checkAclStep(targetPath, step, result)) { + return false } - ) - return true - } catch { - // Why: best-effort — a failed ACL apply must not crash the write; false leaves the path uncached to retry later. - return false + } catch (error) { + // Why not fatal: a failed ACL apply must not crash the write; false leaves the path uncached to retry later. + reportAclFailure(targetPath, step.stage, String(error)) + return false + } } + return true } -const WINDOWS_RESTRICT_ACL_SCRIPT = ` -$ErrorActionPreference = 'Stop' -$path = $args[0] -$currentUserSid = $args[1] -$isDirectory = $args[2] -eq '1' -$allowedSidTexts = @($currentUserSid, 'S-1-5-18', 'S-1-5-32-544') -$allowedSids = @{} -foreach ($sidText in $allowedSidTexts) { - $allowedSids[$sidText] = $true -} -$acl = Get-Acl -LiteralPath $path -$acl.SetAccessRuleProtection($true, $false) -foreach ($rule in @($acl.Access)) { - [void]$acl.RemoveAccessRuleSpecific($rule) -} -$inheritanceFlags = [System.Security.AccessControl.InheritanceFlags]::None -if ($isDirectory) { - $inheritanceFlags = [System.Security.AccessControl.InheritanceFlags]::ContainerInherit -bor [System.Security.AccessControl.InheritanceFlags]::ObjectInherit -} -foreach ($sidText in $allowedSidTexts) { - $sid = [System.Security.Principal.SecurityIdentifier]::new($sidText) - $rule = [System.Security.AccessControl.FileSystemAccessRule]::new( - $sid, - [System.Security.AccessControl.FileSystemRights]::FullControl, - $inheritanceFlags, - [System.Security.AccessControl.PropagationFlags]::None, - [System.Security.AccessControl.AccessControlType]::Allow - ) - [void]$acl.AddAccessRule($rule) -} -Set-Acl -LiteralPath $path -AclObject $acl -$verifiedAcl = Get-Acl -LiteralPath $path -if (-not $verifiedAcl.AreAccessRulesProtected) { - throw 'ACL inheritance is still enabled' -} -$fullControl = [System.Security.AccessControl.FileSystemRights]::FullControl -foreach ($rule in @($verifiedAcl.Access)) { - $sid = $rule.IdentityReference.Translate([System.Security.Principal.SecurityIdentifier]).Value - if (-not $allowedSids.ContainsKey($sid)) { - throw "Unexpected ACL entry $sid" - } - if ($rule.AccessControlType -ne [System.Security.AccessControl.AccessControlType]::Allow) { - throw "Unexpected ACL deny entry $sid" - } - if (($rule.FileSystemRights -band $fullControl) -ne $fullControl) { - throw "ACL entry $sid does not grant FullControl" - } -} -`.trim() - function getCurrentWindowsUserSid(): string | null { if (cachedWindowsUserSid !== undefined) { return cachedWindowsUserSid } try { - const output = execFileSync( - getWindowsSystemToolPath('whoami.exe'), - ['/user', '/fo', 'csv', '/nh'], - { - encoding: 'utf-8', - stdio: ['ignore', 'pipe', 'ignore'], - windowsHide: true, - timeout: 5000 - } - ).trim() - const columns = parseCsvLine(output) - cachedWindowsUserSid = columns[1] ?? null + const result = runProcessSync({ + program: windowsSystem32Binary('whoami.exe'), + args: ['/user', '/fo', 'csv', '/nh'], + timeoutMs: ACL_TIMEOUT_MS + }) + const columns = parseCsvLine(result.stdout.trim()) + cachedWindowsUserSid = result.code === 0 ? (columns[1] ?? null) : null } catch { cachedWindowsUserSid = null } return cachedWindowsUserSid } -function getWindowsSystemToolPath(relativeSystem32Path: string): string { - const systemRoot = process.env.SystemRoot || process.env.WINDIR || 'C:\\Windows' - return pathWin32.join(systemRoot, 'System32', relativeSystem32Path) -} - function parseCsvLine(line: string): string[] { return line.split(/","/).map((part) => part.replace(/^"/, '').replace(/"$/, '')) } diff --git a/src/shared/secure-path-windows-acl.win32.test.ts b/src/shared/secure-path-windows-acl.win32.test.ts new file mode 100644 index 00000000000..227f57c197d --- /dev/null +++ b/src/shared/secure-path-windows-acl.win32.test.ts @@ -0,0 +1,177 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest' +import { runProcessSync } from './child-process/run-process' +import { windowsSystem32Binary } from './child-process/windows-system-binary' +import { + resetSecureFileWindowsUserSidForTests, + restrictWindowsPathSync +} from './secure-path-windows-acl' + +/** + * The half of the proof a mocked argv test cannot give. + * + * The shipped bug was not a wrong argv — it was an argv the *callee* never + * received: `powershell.exe -Command