mirror of
https://github.com/stablyai/orca.git
synced 2026-10-01 08:01:56 +00:00
fix(security): apply the Windows path-hardening ACL that never ran
`buildWindowsRestrictAclArgs` invoked the hardening script as `powershell.exe -Command <script> <path> <sid> <isDir>`. `-Command` does not populate `$args`; it appends the trailing tokens to the command text. The script therefore read `$args[1]` as `$null`, threw `NullArrayIndex` at `$allowedSids[$sidText] = $true` under `$ErrorActionPreference = 'Stop'`, and exited 1. Both callers swallowed that: the async callback was empty and `applySecurePathRestriction` returned `true` regardless, while the sync `catch` returned `false` and nobody logged. Every Windows secure path has been left on its inherited ACL since the ACL was introduced (#5006), and nothing said so. Replace PowerShell with `icacls.exe`, which takes plain argv. That removes the quoting surface entirely rather than escaping it: interpolating a path into the command text would have turned a dead no-op into arbitrary PowerShell on a filesystem path, since `-Command` executes what it appends. It also drops the execution-policy dependency and the `powershell.exe` spawn an EDR flags, and runs ~25x faster than the PowerShell cold start. Hardening is now three passes: `/reset` to purge explicit ACEs that `/inheritance:r` leaves behind, `/inheritance:r` plus a `/grant:r` per allowed SID, then a read-back that checks the DACL is protected and grants only the intended rights. The predecessor's verification block was equally dead, and an apply that is never read back is only half a control. Failures stay non-fatal — non-NTFS volumes, network paths and restricted tokens fail legitimately and must not break startup — but they are no longer invisible: every failure is logged, and a failed async apply now evicts its cache entry so the next call retries instead of trusting a success that never happened. Routing through `runProcess`/`runProcessSync` also retires this file's `node:child_process` allowlist entry.
This commit is contained in:
@@ -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()
|
||||
}
|
||||
})
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
+275
-116
@@ -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<string, string>()
|
||||
|
||||
/**
|
||||
* Stands in for icacls across all three passes. The verify pass has to answer with a real-shaped
|
||||
* `icacls <path>` 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<typeof execFile>
|
||||
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<typeof execFile>
|
||||
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<void> {
|
||||
for (let i = 0; i < 4; i++) {
|
||||
await new Promise((resolve) => setTimeout(resolve, 0))
|
||||
}
|
||||
}
|
||||
|
||||
async function waitForFileTimestampTick(): Promise<void> {
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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 <path>`.
|
||||
*
|
||||
* 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<boolean> {
|
||||
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(/"$/, ''))
|
||||
}
|
||||
|
||||
@@ -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 <script> <path> <sid>` leaves `$args`
|
||||
* empty, so the script died on its first statement and every caller was told
|
||||
* the path had been hardened. Asserting on the constructed arguments passed
|
||||
* happily throughout. Only reading the resulting ACL back off a real file
|
||||
* catches it, so that is what this does.
|
||||
*
|
||||
* Runs only on win32; skipped elsewhere.
|
||||
*/
|
||||
const describeOnWindows = process.platform === 'win32' ? describe : describe.skip
|
||||
|
||||
const EVERYONE_SID = 'S-1-1-0'
|
||||
|
||||
function icacls(...args: string[]): { code: number | null; stdout: string } {
|
||||
const result = runProcessSync({
|
||||
program: windowsSystem32Binary('icacls.exe'),
|
||||
args,
|
||||
timeoutMs: 10_000
|
||||
})
|
||||
return { code: result.code, stdout: result.stdout }
|
||||
}
|
||||
|
||||
/**
|
||||
* The `Principal:(flags)` entries of `icacls <path>`. The first line carries the path, which is
|
||||
* stripped by the exact string passed in so its own spaces cannot be mistaken for the separator.
|
||||
*/
|
||||
function readAclEntries(path: string): string[] {
|
||||
const arg = path.length < 260 ? path : `\\\\?\\${path}`
|
||||
const { stdout } = icacls(arg)
|
||||
const entries: string[] = []
|
||||
for (const [index, rawLine] of stdout.split(/\r?\n/).entries()) {
|
||||
const line = index === 0 ? rawLine.slice(arg.length) : rawLine
|
||||
const trimmed = line.trim()
|
||||
if (!trimmed && index > 0) {
|
||||
break
|
||||
}
|
||||
if (trimmed.includes(':(')) {
|
||||
entries.push(trimmed)
|
||||
}
|
||||
}
|
||||
return entries
|
||||
}
|
||||
|
||||
describeOnWindows('restrictWindowsPathSync against a real filesystem', () => {
|
||||
let root: string
|
||||
|
||||
beforeAll(() => {
|
||||
resetSecureFileWindowsUserSidForTests()
|
||||
root = mkdtempSync(join(tmpdir(), 'orca-acl-win32-'))
|
||||
})
|
||||
|
||||
afterAll(() => {
|
||||
rmSync(root, { recursive: true, force: true })
|
||||
})
|
||||
|
||||
it('actually applies the ACL to a real file, dropping inherited and foreign ACEs', () => {
|
||||
const file = join(root, 'credential.json')
|
||||
writeFileSync(file, '{"token":"secret"}')
|
||||
// A planted explicit ACE: /inheritance:r alone does not remove these.
|
||||
expect(icacls(file, '/grant', `*${EVERYONE_SID}:(R)`).code).toBe(0)
|
||||
|
||||
const before = readAclEntries(file)
|
||||
expect(before.some((entry) => entry.startsWith('Everyone:'))).toBe(true)
|
||||
expect(before.some((entry) => entry.includes('(I)'))).toBe(true)
|
||||
|
||||
expect(restrictWindowsPathSync(file, false)).toBe(true)
|
||||
|
||||
const after = readAclEntries(file)
|
||||
// No inherited ACE survives: the DACL is protected.
|
||||
expect(after.every((entry) => !entry.includes('(I)'))).toBe(true)
|
||||
expect(after.some((entry) => entry.startsWith('Everyone:'))).toBe(false)
|
||||
// Exactly the three intended principals, each with FullControl.
|
||||
expect(after).toHaveLength(3)
|
||||
expect(after.every((entry) => entry.endsWith(':(F)'))).toBe(true)
|
||||
})
|
||||
|
||||
it('gives a real directory inheritable rules so files created inside stay restricted', () => {
|
||||
const dir = join(root, 'secure-dir')
|
||||
mkdirSync(dir)
|
||||
|
||||
expect(restrictWindowsPathSync(dir, true)).toBe(true)
|
||||
|
||||
const after = readAclEntries(dir)
|
||||
expect(after).toHaveLength(3)
|
||||
expect(after.every((entry) => entry.endsWith(':(OI)(CI)(F)'))).toBe(true)
|
||||
expect(after.every((entry) => !entry.includes('(I)'))).toBe(true)
|
||||
|
||||
// The point of the inheritance flags: a child written afterwards is already restricted.
|
||||
const child = join(dir, 'inherited.json')
|
||||
writeFileSync(child, '{}')
|
||||
const childEntries = readAclEntries(child)
|
||||
expect(childEntries).toHaveLength(3)
|
||||
expect(childEntries.every((entry) => entry.includes('(I)'))).toBe(true)
|
||||
expect(childEntries.some((entry) => entry.startsWith('Everyone:'))).toBe(false)
|
||||
})
|
||||
|
||||
// Paths reach this code from user-chosen workspace locations, so the quoting hazards that
|
||||
// ruled out interpolating them into a PowerShell command line get exercised for real.
|
||||
it.each([
|
||||
['spaces', 'a b c'],
|
||||
['single quote and dollar', "quo'te $var"],
|
||||
['backtick', 'back`tick'],
|
||||
['brackets', 'brack[et]s'],
|
||||
['semicolon and ampersand', 'semi;colon & amp'],
|
||||
['comma', 'com,ma'],
|
||||
['parentheses', 'paren(s)'],
|
||||
['caret and percent', 'car^et %PATH%']
|
||||
])('hardens a path containing %s', (_label, segment) => {
|
||||
const dir = join(root, segment)
|
||||
mkdirSync(dir, { recursive: true })
|
||||
const file = join(dir, 'secret.json')
|
||||
writeFileSync(file, '{}')
|
||||
|
||||
expect(restrictWindowsPathSync(file, false)).toBe(true)
|
||||
|
||||
const after = readAclEntries(file)
|
||||
expect(after).toHaveLength(3)
|
||||
expect(after.every((entry) => !entry.includes('(I)'))).toBe(true)
|
||||
})
|
||||
|
||||
it('hardens a path longer than MAX_PATH', () => {
|
||||
let dir = join(root, 'long')
|
||||
while (dir.length < 280) {
|
||||
dir = join(dir, 'x'.repeat(40))
|
||||
}
|
||||
mkdirSync(dir, { recursive: true })
|
||||
const file = join(dir, 'secret.json')
|
||||
expect(file.length).toBeGreaterThan(260)
|
||||
writeFileSync(file, '{}')
|
||||
|
||||
expect(restrictWindowsPathSync(file, false)).toBe(true)
|
||||
expect(readAclEntries(file)).toHaveLength(3)
|
||||
})
|
||||
|
||||
it('reports failure for a path that does not exist', () => {
|
||||
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
|
||||
expect(restrictWindowsPathSync(join(root, 'absent.json'), false)).toBe(false)
|
||||
expect(warn).toHaveBeenCalledWith(
|
||||
'[secure-path.windows-acl] failed to restrict path',
|
||||
expect.objectContaining({ stage: 'reset' })
|
||||
)
|
||||
warn.mockRestore()
|
||||
})
|
||||
|
||||
it('reports failure for a path it has no permission to modify', () => {
|
||||
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
|
||||
// Owned by TrustedInstaller; a non-elevated user cannot rewrite its DACL.
|
||||
const systemFile = windowsSystem32Binary('drivers\\etc\\hosts')
|
||||
|
||||
const restricted = restrictWindowsPathSync(systemFile, false)
|
||||
|
||||
if (restricted) {
|
||||
// Elevated runner: nothing to assert about denial, but never leave the box altered.
|
||||
expect(icacls(systemFile, '/reset').code).toBe(0)
|
||||
} else {
|
||||
expect(warn).toHaveBeenCalledWith(
|
||||
'[secure-path.windows-acl] failed to restrict path',
|
||||
expect.objectContaining({ detail: expect.stringContaining('denied') })
|
||||
)
|
||||
}
|
||||
warn.mockRestore()
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user