From 4d6c291eff539f493336e32067fcf05579cfb37f Mon Sep 17 00:00:00 2001 From: Lesley Murfin Date: Tue, 9 Jun 2026 02:47:29 -0600 Subject: [PATCH] fix(windows): stop main-thread PowerShell ACL storm on env-store reads (#5011) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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)` 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) * fix: keep POSIX secure directory hardening metadata-aware --------- Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> --- docs/windows-secure-file-acl-hardening.md | 104 +++++++++ src/shared/secure-file.test.ts | 264 ++++++++++++++++++---- src/shared/secure-file.ts | 152 ++++++++++--- 3 files changed, 445 insertions(+), 75 deletions(-) create mode 100644 docs/windows-secure-file-acl-hardening.md diff --git a/docs/windows-secure-file-acl-hardening.md b/docs/windows-secure-file-acl-hardening.md new file mode 100644 index 000000000..f5036fe1c --- /dev/null +++ b/docs/windows-secure-file-acl-hardening.md @@ -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. diff --git a/src/shared/secure-file.test.ts b/src/shared/secure-file.test.ts index 64b034ca6..c7f479bec 100644 --- a/src/shared/secure-file.test.ts +++ b/src/shared/secure-file.test.ts @@ -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 + }) }) 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 }) 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 { await new Promise((resolve) => setTimeout(resolve, 20)) } + +function statMode(path: string): number { + return statSync(path).mode & 0o777 +} diff --git a/src/shared/secure-file.ts b/src/shared/secure-file.ts index 17d42b939..877d0b1e9 100644 --- a/src/shared/secure-file.ts +++ b/src/shared/secure-file.ts @@ -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() +// 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() + +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() }