fix(windows): stop main-thread PowerShell ACL storm on env-store reads (#5011)
* fix(windows): stop main-thread PowerShell storm on env-store reads Two changes fix the v1.4.52+ Windows performance regression (#4901 regression against #4840) where 49 powershell.exe processes were spawned in 27 seconds during load, saturating the Electron main thread and causing black terminals and runtimeEnvironments:call timeouts. Root cause: `readEnvironmentStore` calls `hardenExistingSecureFile` on every read. The env-store parent directory's mtime churns constantly (every secure write updates it), so the mtime-keyed idempotency cache never matched → `bestEffortRestrictWindowsPath` (powershell, ~1-1.5s synchronous) fired on every call. After #4901, the remote-runtime tab-sync loop reads the store ~2×/s, turning sporadic mtime misses into a continuous main-thread storm. Fix 1 – path-cached directory hardening: add `hardenedDirectoryPathsThisProcess (Set<string>)` that caches directory hardening by PATH for the process lifetime. A directory's required ACL does not change when its mtime changes; only file hardening retains the metadata-keyed cache so post-rename inode changes are detected correctly. Fix 2 – async ACL application: replace `execFileSync(powershell.exe, ...)` with `execFile` (fire-and-forget). PowerShell cold-start is ~1-1.5s; the function is already named `bestEffortRestrictWindowsPath` so async/optimistic caching is correct. `applySecurePathRestriction` returns `true` optimistically on win32 so the cache entry is written before the background process completes. Tests: new regression tests verify the directory is hardened exactly once even when its mtime changes between calls, that unchanged files are not re-hardened, and that ACL application goes through async execFile (not execFileSync). * fix(windows): apply credential-file ACL synchronously on write path Follow-up rigor on the env-store PowerShell ACL storm fix (#5006). The read-path storm fix (path-cached async directory hardening + async file re-harden) is retained, but switching ALL ACL application to async opened a narrow Windows-only security window: because writeFileSync({mode}) is a no-op on Windows, writeSecureFile returned with the credential file still carrying the parent directory's inherited (broader) ACL for the ~1-1.5s PowerShell cold-start, affecting the e2ee keypair, device registry, and runtime env auth store. Fix: apply the credential FILE's ACL synchronously (execFileSync) on the infrequent write path, before the atomic rename publishes it, and cache the path as hardened only on confirmed success so a failed apply retries. Keep the DIRECTORY hardening async + path-cached for the process lifetime (that is what killed the #4901/#5006 main-thread storm). The read path's existing-file re-harden stays async + metadata-cached (fires at most once per file, no storm). Also: - Document the dir-path cache process-lifetime known limitation (deleted+ recreated dir not re-hardened until restart). - Remove the redundant double dir-cache write in writeSecureFile. - Add docs/windows-secure-file-acl-hardening.md describing the sync-file/ async-dir model and a manual Windows e2e test plan (the cross-platform Playwright harness runs on Linux and cannot reach the PowerShell path). Tests (src/shared/secure-file.test.ts, 13 passing): credential file hardened synchronously while dir stays async (no async file-ACL window); failed sync file-ACL apply is not cached and retries; dir hardened exactly once across many writes despite mtime churn; no PowerShell spawned on non-win32. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: keep POSIX secure directory hardening metadata-aware --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com>
This commit is contained in:
Lesley Murfin
GitHub
parent
98d02bca47
commit
4d6c291eff
|
|
@ -0,0 +1,104 @@
|
|||
# Windows Secure-File ACL Hardening
|
||||
|
||||
## Problem
|
||||
|
||||
Credential files Orca writes on Windows (runtime env auth store, device registry,
|
||||
e2ee keypair) must end up readable only by the current user, SYSTEM, and
|
||||
Administrators. On POSIX this is a one-line `chmodSync`, but `writeFileSync`'s
|
||||
`mode` option is a no-op on Windows, so the NTFS ACL has to be rewritten by
|
||||
shelling out to PowerShell (`Get-Acl` / `Set-Acl`). PowerShell cold-start is
|
||||
~1–1.5 s.
|
||||
|
||||
`readEnvironmentStore` calls `hardenExistingSecureFile` on every read. The
|
||||
env-store parent directory's mtime churns constantly (every secure write updates
|
||||
it), so an mtime-keyed idempotency cache never matches and a blocking PowerShell
|
||||
spawn fires on every call. After the remote-runtime tab-sync polling change, the
|
||||
store is read ~2×/s, turning sporadic mtime misses into a continuous main-thread
|
||||
storm (~1.8 powershell.exe spawns/sec) that saturates the Electron main thread
|
||||
and times out `runtimeEnvironments:call`. See #4901 / #5006.
|
||||
|
||||
## Model
|
||||
|
||||
`src/shared/secure-file.ts` applies two different caching + execution strategies,
|
||||
chosen by whether the target is a directory and whether it is on the write path:
|
||||
|
||||
- **Directories — async + path-cached for the process lifetime.**
|
||||
A directory's required ACL does not change when its mtime changes, so once a
|
||||
directory has been hardened in this process it is trusted for the rest of the
|
||||
process. Directory hardening uses fire-and-forget `execFile` so it never blocks
|
||||
the main thread. This is the change that kills the #4901 storm.
|
||||
- _Known limitation:_ a directory that is deleted and recreated mid-process is
|
||||
not re-hardened until the next restart. The `.orca` secure dirs are not
|
||||
deleted at runtime, so this is acceptable.
|
||||
|
||||
- **Credential files on the write path — synchronous, cache only on success.**
|
||||
Because `writeFileSync({ mode })` is a no-op on Windows, a freshly written file
|
||||
carries the parent directory's inherited (broader) ACL. `writeSecureFile` must
|
||||
therefore restrict the file's ACL **synchronously** (via `execFileSync`) on the
|
||||
temp file and on the renamed target before it returns — otherwise the function
|
||||
would return with the credential briefly readable under inherited ACLs during
|
||||
the ~1–1.5 s PowerShell cold-start window. The write path is infrequent, so the
|
||||
synchronous cost is acceptable. The path is cached as hardened **only on
|
||||
confirmed success**, so a failed apply is retried on the next write.
|
||||
|
||||
- **Existing files on the read path — async + metadata-cached.**
|
||||
`hardenExistingSecureFile` re-asserts the ACL on an already-existing file at
|
||||
most once per process (keyed on inode/size/timestamps so post-rename inode
|
||||
changes are detected). Async is safe here because it only re-asserts an ACL on a
|
||||
file that already exists; new files are hardened synchronously on the write path
|
||||
above. Because it fires at most once per file, it does not storm.
|
||||
|
||||
The net effect: the frequent read path never blocks the main thread, while the
|
||||
infrequent write path closes the async window so credential files are never
|
||||
published with a broader-than-intended ACL.
|
||||
|
||||
## Requirements
|
||||
|
||||
- Read-path directory and existing-file hardening must not spawn PowerShell more
|
||||
than once per path per process, regardless of mtime churn.
|
||||
- `writeSecureFile` must apply the credential file's ACL synchronously before
|
||||
returning; the file ACL must not be left to a background process.
|
||||
- A failed synchronous file-ACL apply must not crash the write and must not be
|
||||
cached as hardened (so it retries).
|
||||
- No PowerShell is spawned on non-win32 platforms.
|
||||
|
||||
## Manual Windows end-to-end test plan
|
||||
|
||||
The automated e2e harness (`pnpm test:e2e`, Playwright `electron-headless`) runs
|
||||
on `ubuntu-latest`, where `applySecurePathRestriction` short-circuits to
|
||||
`chmodSync` and never reaches the PowerShell path. The ACL storm therefore cannot
|
||||
be reproduced in the cross-platform e2e harness; verify it manually on Windows.
|
||||
|
||||
Pre-req: a Windows client paired to a remote `orca serve` runtime.
|
||||
|
||||
Watcher (PowerShell, run before launching Orca):
|
||||
|
||||
```powershell
|
||||
while ($true) {
|
||||
$n = (Get-CimInstance Win32_Process -Filter "Name='powershell.exe'").Count
|
||||
"{0} powershell.exe count = {1}" -f (Get-Date -Format HH:mm:ss), $n
|
||||
Start-Sleep -Milliseconds 500
|
||||
}
|
||||
```
|
||||
|
||||
Steps:
|
||||
|
||||
1. Launch the **stock v1.4.52/v1.4.53** build and open the remote workspace.
|
||||
- **Before fix:** the watcher oscillates ~1–2 powershell.exe processes/sec
|
||||
continuously through the load window; the app is unresponsive and the
|
||||
`[web-session-tabs-sync] … RemoteRuntimeClientError: Timed out` error
|
||||
appears in the console.
|
||||
2. Launch the **fixed** build and open the same remote workspace.
|
||||
- **After fix:** the watcher stays at `0` (no continuous powershell churn); the
|
||||
env-store directory is hardened at most once; the app loads without the
|
||||
session-tabs timeout.
|
||||
3. Write a credential (e.g. sign in / register a device so a secure file is
|
||||
written), then immediately inspect the file ACL:
|
||||
|
||||
```powershell
|
||||
icacls "$env:APPDATA\orca\orca-environments.json"
|
||||
```
|
||||
|
||||
- **Expected:** only the current user, `SYSTEM`, and `Administrators` have
|
||||
access the instant the write completes (no inherited entries), confirming the
|
||||
synchronous file-ACL apply closed the async window.
|
||||
|
|
@ -1,5 +1,5 @@
|
|||
import { execFileSync } from 'child_process'
|
||||
import { mkdtempSync, rmSync, writeFileSync } from 'fs'
|
||||
import { execFile, execFileSync } from 'child_process'
|
||||
import { chmodSync, mkdtempSync, rmSync, statSync, writeFileSync } from 'fs'
|
||||
import { tmpdir } from 'os'
|
||||
import { join } from 'path'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
|
@ -12,7 +12,8 @@ import {
|
|||
} from './secure-file'
|
||||
|
||||
vi.mock('child_process', () => ({
|
||||
execFileSync: vi.fn()
|
||||
execFileSync: vi.fn(),
|
||||
execFile: vi.fn()
|
||||
}))
|
||||
|
||||
describe('hardenSecurePath', () => {
|
||||
|
|
@ -27,12 +28,23 @@ describe('hardenSecurePath', () => {
|
|||
__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"'
|
||||
}
|
||||
// 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>
|
||||
})
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
|
|
@ -62,16 +74,16 @@ describe('hardenSecurePath', () => {
|
|||
platform: 'win32'
|
||||
})
|
||||
|
||||
// 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' })
|
||||
)
|
||||
const [, powershellArgs, powershellOptions] = vi.mocked(execFileSync).mock.calls[1]!
|
||||
expect(vi.mocked(execFileSync).mock.calls[1]![0]).toBe(
|
||||
'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe'
|
||||
)
|
||||
// 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',
|
||||
|
|
@ -87,24 +99,25 @@ describe('hardenSecurePath', () => {
|
|||
expect(script).toContain('SetAccessRuleProtection($true, $false)')
|
||||
expect(script).toContain('RemoveAccessRuleSpecific')
|
||||
expect(script).toContain('Unexpected ACL entry')
|
||||
expect(powershellOptions).toEqual(
|
||||
expect.objectContaining({ stdio: 'ignore', windowsHide: true, timeout: 5000 })
|
||||
)
|
||||
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(execFileSync).mock.calls[1]![1] as string[]
|
||||
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', () => {
|
||||
vi.mocked(execFileSync).mockImplementationOnce(() => '"USER","S-1-5-21-1000"')
|
||||
vi.mocked(execFileSync).mockImplementationOnce(() => {
|
||||
throw new Error('access denied')
|
||||
// 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(() =>
|
||||
|
|
@ -125,6 +138,7 @@ describe('hardenSecurePath', () => {
|
|||
hardenExistingSecureFile(targetPath)
|
||||
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])
|
||||
})
|
||||
|
|
@ -141,35 +155,7 @@ describe('hardenSecurePath', () => {
|
|||
writeFileSync(targetPath, '{"changed":true}')
|
||||
hardenExistingSecureFile(targetPath)
|
||||
|
||||
expect(getPowerShellCalls()).toHaveLength(3)
|
||||
expect(getPowerShellCalls().map(getPowerShellTarget)).toEqual([
|
||||
userDataPath,
|
||||
targetPath,
|
||||
targetPath
|
||||
])
|
||||
})
|
||||
|
||||
it('retries existing-file hardening after a failed ACL rewrite', () => {
|
||||
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, '{}')
|
||||
let powershellCalls = 0
|
||||
vi.mocked(execFileSync).mockImplementation((file) => {
|
||||
if (file === 'C:\\Windows\\System32\\whoami.exe') {
|
||||
return '"USER","S-1-5-21-1000"'
|
||||
}
|
||||
powershellCalls += 1
|
||||
if (powershellCalls === 2) {
|
||||
throw new Error('access denied')
|
||||
}
|
||||
return ''
|
||||
})
|
||||
|
||||
hardenExistingSecureFile(targetPath)
|
||||
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([
|
||||
userDataPath,
|
||||
|
|
@ -187,17 +173,195 @@ describe('hardenSecurePath', () => {
|
|||
writeSecureFile(targetPath, 'first')
|
||||
writeSecureFile(targetPath, 'second')
|
||||
|
||||
const powershellTargets = getPowerShellCalls().map(getPowerShellTarget)
|
||||
expect(powershellTargets).toHaveLength(5)
|
||||
expect(powershellTargets.filter((entry) => entry === userDataPath)).toHaveLength(1)
|
||||
expect(powershellTargets.filter((entry) => entry === targetPath)).toHaveLength(2)
|
||||
// The DIRECTORY is hardened async + path-cached: exactly once across both writes.
|
||||
const asyncTargets = getPowerShellCalls().map(getPowerShellTarget)
|
||||
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)
|
||||
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
|
||||
// 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 () => {
|
||||
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, '{}')
|
||||
|
||||
// Simulate the env-store read loop: hardenExistingSecureFile called many times while
|
||||
// another part of Orca writes to the same directory (changing its mtime).
|
||||
hardenExistingSecureFile(targetPath)
|
||||
await waitForFileTimestampTick()
|
||||
// Simulate a write to another file in the same dir (changes dir mtime)
|
||||
writeFileSync(join(userDataPath, 'other.json'), '{}')
|
||||
hardenExistingSecureFile(targetPath)
|
||||
hardenExistingSecureFile(targetPath)
|
||||
|
||||
// The parent directory must be hardened exactly ONCE despite its mtime changing
|
||||
const dirCalls = getPowerShellCalls().filter(
|
||||
(call) => getPowerShellTarget(call) === userDataPath
|
||||
)
|
||||
expect(dirCalls).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('does not re-harden an unchanged file on repeated reads', () => {
|
||||
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, '{}')
|
||||
|
||||
hardenExistingSecureFile(targetPath)
|
||||
hardenExistingSecureFile(targetPath)
|
||||
hardenExistingSecureFile(targetPath)
|
||||
|
||||
const fileCalls = getPowerShellCalls().filter(
|
||||
(call) => getPowerShellTarget(call) === targetPath
|
||||
)
|
||||
expect(fileCalls).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('applies the read-path ACL asynchronously without blocking (async execFile)', () => {
|
||||
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)
|
||||
})
|
||||
|
||||
// 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.
|
||||
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-'))
|
||||
tempDirs.push(userDataPath)
|
||||
const targetPath = join(userDataPath, 'secret.json')
|
||||
|
||||
writeSecureFile(targetPath, 'contents')
|
||||
|
||||
// Directory: async only.
|
||||
expect(getPowerShellCalls().map(getPowerShellTarget)).toEqual([userDataPath])
|
||||
// File (tmpFile + renamed target): synchronous only — no async file ACL window.
|
||||
const syncTargets = getSyncPowerShellCalls().map(getPowerShellTarget)
|
||||
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)
|
||||
})
|
||||
|
||||
// Nit #1 (review): the synchronous file path must cache as hardened ONLY on confirmed
|
||||
// success, so a failed ACL apply is retried on the next write instead of being silently
|
||||
// trusted.
|
||||
it('retries the credential-file ACL on the next write when the sync apply fails', () => {
|
||||
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')
|
||||
|
||||
// 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"'
|
||||
}
|
||||
throw new Error('access denied')
|
||||
})
|
||||
expect(() => writeSecureFile(targetPath, 'first')).not.toThrow()
|
||||
const firstWriteTargetCalls = getSyncPowerShellCalls()
|
||||
.map(getPowerShellTarget)
|
||||
.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"'
|
||||
}
|
||||
return ''
|
||||
})
|
||||
writeSecureFile(targetPath, 'second')
|
||||
const allTargetCalls = getSyncPowerShellCalls()
|
||||
.map(getPowerShellTarget)
|
||||
.filter((entry) => entry === targetPath)
|
||||
expect(allTargetCalls).toHaveLength(2)
|
||||
})
|
||||
|
||||
// Nit #2 (review) / hardening: the process-lifetime directory cache hardens a directory
|
||||
// exactly once even when its mtime churns across many writes (the #4901 storm condition,
|
||||
// exercised through the write path rather than the read path).
|
||||
it('hardens the directory exactly once across many writes despite mtime churn', () => {
|
||||
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
|
||||
const userDataPath = mkdtempSync(join(tmpdir(), 'orca-secure-file-'))
|
||||
tempDirs.push(userDataPath)
|
||||
|
||||
for (let i = 0; i < 5; i++) {
|
||||
// Each write changes the directory's mtime (a new file lands in it).
|
||||
writeSecureFile(join(userDataPath, `secret-${i}.json`), `contents-${i}`)
|
||||
}
|
||||
|
||||
const dirCalls = getPowerShellCalls().filter(
|
||||
(call) => getPowerShellTarget(call) === userDataPath
|
||||
)
|
||||
expect(dirCalls).toHaveLength(1)
|
||||
})
|
||||
|
||||
// win32-only guard: on non-win32 platforms no PowerShell is ever spawned (sync or async);
|
||||
// POSIX hardening uses chmodSync only.
|
||||
it('never spawns PowerShell on non-win32 platforms', () => {
|
||||
Object.defineProperty(process, 'platform', { configurable: true, value: 'linux' })
|
||||
const userDataPath = mkdtempSync(join(tmpdir(), 'orca-secure-file-'))
|
||||
tempDirs.push(userDataPath)
|
||||
const targetPath = join(userDataPath, 'secret.json')
|
||||
|
||||
writeSecureFile(targetPath, 'contents')
|
||||
hardenExistingSecureFile(targetPath)
|
||||
|
||||
expect(getPowerShellCalls()).toHaveLength(0)
|
||||
expect(getSyncPowerShellCalls()).toHaveLength(0)
|
||||
})
|
||||
|
||||
it('re-hardens a POSIX directory when its metadata changes after caching', () => {
|
||||
Object.defineProperty(process, 'platform', { configurable: true, value: 'linux' })
|
||||
const userDataPath = mkdtempSync(join(tmpdir(), 'orca-secure-file-'))
|
||||
tempDirs.push(userDataPath)
|
||||
const targetPath = join(userDataPath, 'secret.json')
|
||||
writeFileSync(targetPath, '{}')
|
||||
|
||||
hardenExistingSecureFile(targetPath)
|
||||
expect(statMode(userDataPath)).toBe(0o700)
|
||||
|
||||
chmodSync(userDataPath, 0o755)
|
||||
hardenExistingSecureFile(targetPath)
|
||||
|
||||
expect(statMode(userDataPath)).toBe(0o700)
|
||||
})
|
||||
})
|
||||
|
||||
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))
|
||||
}
|
||||
|
||||
// Synchronous PowerShell calls (credential-file ACL on the write path).
|
||||
function getSyncPowerShellCalls(): unknown[][] {
|
||||
return vi
|
||||
.mocked(execFileSync)
|
||||
.mock.calls.filter(([file]) => String(file).endsWith('WindowsPowerShell\\v1.0\\powershell.exe'))
|
||||
.mock.calls.filter(([file]) => String(file).endsWith(POWERSHELL_SUFFIX))
|
||||
}
|
||||
|
||||
function getPowerShellTarget(call: unknown[]): string {
|
||||
|
|
@ -207,3 +371,7 @@ function getPowerShellTarget(call: unknown[]): string {
|
|||
async function waitForFileTimestampTick(): Promise<void> {
|
||||
await new Promise((resolve) => setTimeout(resolve, 20))
|
||||
}
|
||||
|
||||
function statMode(path: string): number {
|
||||
return statSync(path).mode & 0o777
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import { execFileSync } from 'child_process'
|
||||
import { execFile, execFileSync } from 'child_process'
|
||||
import { randomBytes } from 'crypto'
|
||||
import { chmodSync, existsSync, mkdirSync, renameSync, rmSync, statSync, writeFileSync } from 'fs'
|
||||
import { dirname, win32 as pathWin32 } from 'path'
|
||||
|
|
@ -21,7 +21,39 @@ type HardenedPathCacheEntry = {
|
|||
// always re-hardens (new inode) and then refreshes the cache entry.
|
||||
const hardenedPathsThisProcess = new Map<string, HardenedPathCacheEntry>()
|
||||
|
||||
// Why: a directory's required ACL does not change because its mtime changed (child writes
|
||||
// update the directory's mtime constantly). Keying the directory cache on mtime/ctime causes
|
||||
// a cache miss — and a blocking PowerShell spawn — on every read-path call. We instead cache
|
||||
// directory hardening by PATH for the entire process lifetime: once a directory's ACL has
|
||||
// been applied in this process we trust it stays correct. Files keep the metadata-keyed cache
|
||||
// so that post-rename inode changes are detected correctly. See #4901 regression report.
|
||||
//
|
||||
// Known limitation: because this is a process-lifetime path cache, a directory that is deleted
|
||||
// and recreated during the same process will NOT be re-hardened. The secure dirs we own
|
||||
// (.orca runtime/auth/device stores) are not deleted at runtime, so this is acceptable; the
|
||||
// next process restart re-hardens. Do not rely on this cache if a path's lifecycle changes.
|
||||
const hardenedDirectoryPathsThisProcess = new Set<string>()
|
||||
|
||||
function hardenSecureDirectoryOnce(dirPath: string): void {
|
||||
// Why: directory hardening is async + path-cached. The directory ACL is broad-but-bounded
|
||||
// (current user + SYSTEM + Administrators) and re-applying it is what stormed the main
|
||||
// thread (#4901), so we never block on it. A pending dir ACL on first run is acceptable
|
||||
// because the credential FILES inside it are hardened synchronously on the write path.
|
||||
if (hardenedDirectoryPathsThisProcess.has(dirPath)) {
|
||||
return
|
||||
}
|
||||
applySecurePathRestriction(dirPath, true, process.platform, false)
|
||||
// Optimistic: cache even though the async ACL may still be in flight. The dir restriction
|
||||
// is best-effort and re-running it does not improve security, so we accept no-retry here.
|
||||
hardenedDirectoryPathsThisProcess.add(dirPath)
|
||||
}
|
||||
|
||||
function hardenSecurePathOnce(targetPath: string, isDirectory: boolean): boolean {
|
||||
if (isDirectory && process.platform === 'win32') {
|
||||
hardenSecureDirectoryOnce(targetPath)
|
||||
return true
|
||||
}
|
||||
|
||||
const currentEntry = getHardenedPathCacheEntry(targetPath, isDirectory)
|
||||
if (!currentEntry) {
|
||||
hardenedPathsThisProcess.delete(targetPath)
|
||||
|
|
@ -30,7 +62,11 @@ function hardenSecurePathOnce(targetPath: string, isDirectory: boolean): boolean
|
|||
if (currentEntry && cachedEntry && hardenedPathCacheEntriesMatch(currentEntry, cachedEntry)) {
|
||||
return true
|
||||
}
|
||||
if (applySecurePathRestriction(targetPath, isDirectory, process.platform)) {
|
||||
// Why: the read path re-hardens an existing file at most once per process (metadata-cached
|
||||
// above), so async file hardening here does not storm. The async restriction only re-asserts
|
||||
// an ACL on a file that already exists; new credential files are hardened synchronously on
|
||||
// the write path (see writeSecureFile), so there is no async window on creation.
|
||||
if (applySecurePathRestriction(targetPath, isDirectory, process.platform, false)) {
|
||||
rememberHardenedPath(targetPath, isDirectory)
|
||||
return true
|
||||
}
|
||||
|
|
@ -46,7 +82,9 @@ export function writeSecureFile(targetPath: string, contents: string): void {
|
|||
if (!existsSync(dir)) {
|
||||
mkdirSync(dir, { recursive: true, mode: 0o700 })
|
||||
}
|
||||
const directoryWasHardened = hardenSecurePathOnce(dir, true)
|
||||
// Windows directory hardening stays async + path-cached: it is what stormed the main thread
|
||||
// (#4901). POSIX keeps the metadata cache so chmod/ctime changes are corrected.
|
||||
hardenSecurePathOnce(dir, true)
|
||||
|
||||
const tmpFile = `${targetPath}.${process.pid}.${Date.now()}.${randomBytes(4).toString('hex')}.tmp`
|
||||
try {
|
||||
|
|
@ -54,16 +92,20 @@ export function writeSecureFile(targetPath: string, contents: string): void {
|
|||
encoding: 'utf-8',
|
||||
mode: 0o600
|
||||
})
|
||||
hardenSecurePath(tmpFile, { isDirectory: false, platform: process.platform })
|
||||
// Why: on Windows writeFileSync({mode:0o600}) is a no-op, so the file is created carrying
|
||||
// the parent directory's inherited (broader) ACL. We must restrict the credential file's
|
||||
// ACL SYNCHRONOUSLY before the atomic rename publishes it — otherwise writeSecureFile
|
||||
// would return with the credential readable under inherited ACLs for the ~1-1.5s PowerShell
|
||||
// cold-start window. The write path is infrequent, so the synchronous cost is acceptable;
|
||||
// it is the READ path that stormed (#4901), and that stays async + cached.
|
||||
applySecurePathRestriction(tmpFile, false, process.platform, true)
|
||||
renameSync(tmpFile, targetPath)
|
||||
// Why: these files carry runtime auth/device credentials; the published
|
||||
// path must remain current-user only after the atomic rename.
|
||||
if (applySecurePathRestriction(targetPath, false, process.platform)) {
|
||||
// path must remain current-user only after the atomic rename. Apply synchronously and only
|
||||
// cache on confirmed success so a failed ACL apply is retried on the next read/write.
|
||||
if (applySecurePathRestriction(targetPath, false, process.platform, true)) {
|
||||
rememberHardenedPath(targetPath, false)
|
||||
}
|
||||
if (directoryWasHardened) {
|
||||
rememberHardenedPath(dir, true)
|
||||
}
|
||||
} catch (error) {
|
||||
rmSync(tmpFile, { force: true })
|
||||
throw error
|
||||
|
|
@ -85,18 +127,36 @@ export function hardenSecurePath(
|
|||
options: {
|
||||
isDirectory: boolean
|
||||
platform: NodeJS.Platform
|
||||
sync?: boolean
|
||||
}
|
||||
): void {
|
||||
applySecurePathRestriction(targetPath, options.isDirectory, options.platform)
|
||||
applySecurePathRestriction(
|
||||
targetPath,
|
||||
options.isDirectory,
|
||||
options.platform,
|
||||
options.sync ?? false
|
||||
)
|
||||
}
|
||||
|
||||
function applySecurePathRestriction(
|
||||
targetPath: string,
|
||||
isDirectory: boolean,
|
||||
platform: NodeJS.Platform
|
||||
platform: NodeJS.Platform,
|
||||
sync: boolean
|
||||
): boolean {
|
||||
if (platform === 'win32') {
|
||||
return bestEffortRestrictWindowsPath(targetPath, isDirectory)
|
||||
if (sync) {
|
||||
// Why: the write path must apply the credential FILE's ACL before returning, otherwise
|
||||
// the file is briefly readable under inherited (broader) ACLs (writeFileSync mode is a
|
||||
// no-op on Windows). Run PowerShell synchronously and report real success so callers only
|
||||
// cache the path as hardened when the ACL actually applied. The write path is infrequent.
|
||||
return restrictWindowsPathSync(targetPath, isDirectory)
|
||||
}
|
||||
// Why: the directory and read-path re-harden are async (fire-and-forget) to avoid blocking
|
||||
// the main thread (#4901). We optimistically return true because the restriction is
|
||||
// best-effort; the cache entry is written immediately so we do not re-spawn on the next call.
|
||||
bestEffortRestrictWindowsPath(targetPath, isDirectory)
|
||||
return true
|
||||
}
|
||||
chmodSync(targetPath, isDirectory ? 0o700 : 0o600)
|
||||
return true
|
||||
|
|
@ -149,35 +209,72 @@ function hardenedPathCacheEntriesMatch(
|
|||
)
|
||||
}
|
||||
|
||||
function bestEffortRestrictWindowsPath(targetPath: string, isDirectory: boolean): boolean {
|
||||
function buildWindowsRestrictAclArgs(
|
||||
targetPath: string,
|
||||
currentUserSid: string,
|
||||
isDirectory: boolean
|
||||
): string[] {
|
||||
return [
|
||||
'-NoProfile',
|
||||
'-NonInteractive',
|
||||
'-ExecutionPolicy',
|
||||
'Bypass',
|
||||
'-Command',
|
||||
WINDOWS_RESTRICT_ACL_SCRIPT,
|
||||
targetPath,
|
||||
currentUserSid,
|
||||
isDirectory ? '1' : '0'
|
||||
]
|
||||
}
|
||||
|
||||
function bestEffortRestrictWindowsPath(targetPath: string, isDirectory: boolean): void {
|
||||
const currentUserSid = getCurrentWindowsUserSid()
|
||||
if (!currentUserSid) {
|
||||
return
|
||||
}
|
||||
// Why: execFile (async) is used instead of execFileSync to avoid blocking the Electron main
|
||||
// thread. PowerShell cold-start is ~1–1.5 s; spawning it synchronously on every read-path
|
||||
// call saturated the main thread in v1.4.52+ where the env-store is read ~2×/s by the
|
||||
// remote-runtime tab-sync loop (#4901 regression). The restriction is best-effort
|
||||
// (see function name), so it is safe to apply it in the background.
|
||||
execFile(
|
||||
getWindowsSystemToolPath('WindowsPowerShell\\v1.0\\powershell.exe'),
|
||||
buildWindowsRestrictAclArgs(targetPath, currentUserSid, isDirectory),
|
||||
{
|
||||
windowsHide: true,
|
||||
timeout: 5000
|
||||
},
|
||||
() => {
|
||||
// Why: errors are intentionally ignored — credential-file hardening should not
|
||||
// prevent Orca from starting on Windows machines where PowerShell ACL APIs are
|
||||
// unavailable or locked down.
|
||||
}
|
||||
)
|
||||
}
|
||||
|
||||
function restrictWindowsPathSync(targetPath: string, isDirectory: boolean): boolean {
|
||||
const currentUserSid = getCurrentWindowsUserSid()
|
||||
if (!currentUserSid) {
|
||||
return false
|
||||
}
|
||||
// Why: synchronous variant for the credential-FILE write path only. The file must not be
|
||||
// published (renamed into place / returned to the caller) until its ACL has actually been
|
||||
// restricted, so we block here and report real success. This is the rare path; the frequent
|
||||
// read path stays async (bestEffortRestrictWindowsPath) to avoid the #4901 main-thread storm.
|
||||
try {
|
||||
execFileSync(
|
||||
getWindowsSystemToolPath('WindowsPowerShell\\v1.0\\powershell.exe'),
|
||||
[
|
||||
'-NoProfile',
|
||||
'-NonInteractive',
|
||||
'-ExecutionPolicy',
|
||||
'Bypass',
|
||||
'-Command',
|
||||
WINDOWS_RESTRICT_ACL_SCRIPT,
|
||||
targetPath,
|
||||
currentUserSid,
|
||||
isDirectory ? '1' : '0'
|
||||
],
|
||||
buildWindowsRestrictAclArgs(targetPath, currentUserSid, isDirectory),
|
||||
{
|
||||
stdio: 'ignore',
|
||||
stdio: ['ignore', 'ignore', 'ignore'],
|
||||
windowsHide: true,
|
||||
timeout: 5000
|
||||
}
|
||||
)
|
||||
return true
|
||||
} catch {
|
||||
// Why: credential-file hardening should not prevent Orca from starting on
|
||||
// Windows machines where PowerShell ACL APIs are unavailable or locked down.
|
||||
// Why: best-effort — a failed ACL apply (locked-down PowerShell, etc.) must not crash the
|
||||
// write. Returning false leaves the path uncached so a later read/write retries it.
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
|
@ -270,4 +367,5 @@ export function __resetSecureFileWindowsUserSidForTests(): void {
|
|||
|
||||
export function __resetSecureFileHardenedPathsForTests(): void {
|
||||
hardenedPathsThisProcess.clear()
|
||||
hardenedDirectoryPathsThisProcess.clear()
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue