From 0abf4eec274ba48fb41c2a6c7e143d0383d4c22b Mon Sep 17 00:00:00 2001 From: Orca Worker Date: Tue, 1 Sep 2026 01:38:23 -0700 Subject: [PATCH] fix(security): re-probe hardening instead of latching a transient failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-process attempt cap added for the read-path storm was a permanent latch: one AV scan, momentary lock or %TEMP% blip and every later credential write in that session went unhardened, silently, on a host where hardening would now succeed. Same defect class as #17858's computer-use host, and worse here because what stops happening is security hardening on credential files and nothing said so. The retry budget now bounds the *rate*, not the lifetime: at most three attempts per path per minute, re-probing in every later window, forever. The transition is announced in both directions — `throttled` once per window on entry, `recovered` when a rate-limited path hardens again — so a host stuck in the degraded state is diagnosable rather than merely quiet. The reporter type covers both, and the main process ends the `recovered` span successfully rather than failing it. Extracted to secure-path-hardening-retry-budget.ts, which keeps secure-file.ts under its line cap without a max-lines disable. Also confirms the second flagged risk rather than assuming it: a real unwritable %TEMP% is now covered by a test proving verification fails closed, reports at the `verify` stage, and still leaves the ACL applied — so that path loses proof, not protection, and with the lifetime cap gone it can no longer combine into a permanent-off state. --- src/main/observability/index.ts | 26 +++++-- src/shared/secure-file.test.ts | 62 +++++++++++++-- src/shared/secure-file.ts | 48 +++--------- .../secure-path-hardening-retry-budget.ts | 76 +++++++++++++++++++ src/shared/secure-path-windows-acl.ts | 40 +++++++--- .../secure-path-windows-acl.win32.test.ts | 39 ++++++++++ 6 files changed, 225 insertions(+), 66 deletions(-) create mode 100644 src/shared/secure-path-hardening-retry-budget.ts diff --git a/src/main/observability/index.ts b/src/main/observability/index.ts index 55a4bc6f3b8..589f5c13589 100644 --- a/src/main/observability/index.ts +++ b/src/main/observability/index.ts @@ -49,7 +49,7 @@ import { type UploadBundleResult } from './diagnostic-bundle-upload' import { setActiveSink, startSpan } from './tracer' -import { setSecurePathHardeningFailureReporter } from '../../shared/secure-path-windows-acl' +import { setSecurePathHardeningReporter } from '../../shared/secure-path-windows-acl' const CI_ENV_VARS = [ 'CI', @@ -162,19 +162,29 @@ export function initObservability(): ObservabilityConsent { * Why route it here: Windows path hardening lives in `src/shared` and defaults to `console.warn`, * which reaches nothing in a packaged build — the main process is GUI-subsystem and owns no * console. A credential file left on inherited ACLs is exactly what a diagnostic bundle should - * show, so the failure becomes a failed span in the trace sink. + * show, so it becomes a span in the trace sink. + * + * `recovered` ends successfully rather than failing: a host that climbs back out of the + * rate-limited state has to be as visible as one that fell into it, or the degraded state is only + * ever half-diagnosable. */ function installSecurePathHardeningReporter(): void { - setSecurePathHardeningFailureReporter((failure) => { - startSpan('secure-path.windows-acl.failure', { - attributes: { targetPath: failure.targetPath, stage: failure.stage } - }).fail(failure.detail) - console.warn('[secure-path.windows-acl] failed to restrict path', failure) + setSecurePathHardeningReporter((entry) => { + const span = startSpan('secure-path.windows-acl', { + attributes: { targetPath: entry.targetPath, stage: entry.stage, detail: entry.detail } + }) + if (entry.stage === 'recovered') { + span.end() + console.info('[secure-path.windows-acl] path hardening recovered', entry) + return + } + span.fail(entry.detail) + console.warn('[secure-path.windows-acl] failed to restrict path', entry) }) } export async function shutdownObservability(): Promise { - setSecurePathHardeningFailureReporter(null) + setSecurePathHardeningReporter(null) // Order matters: tracer first so no new pushes arrive while the local sink // is closing and flushing buffered lines. setActiveSink(null) diff --git a/src/shared/secure-file.test.ts b/src/shared/secure-file.test.ts index b5bf0fc4cc3..99bc6a74ccd 100644 --- a/src/shared/secure-file.test.ts +++ b/src/shared/secure-file.test.ts @@ -200,35 +200,81 @@ describe('hardenSecurePath', () => { Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) const targetPath = writeFailingHardenTarget() - // The read-path polling loop the storm came from. + // The read-path polling loop the storm came from: 25 reads, not 25 spawns. for (let read = 0; read < 25; read++) { hardenExistingSecureFile(targetPath) await flushAsyncAcl() } - expect(attemptsFor(targetPath)).toHaveLength(1) + expect(attemptsFor(targetPath)).toHaveLength(3) + // Entering the degraded state is announced once, not once per read. + expect(throttleReports(warn, targetPath)).toHaveLength(1) warn.mockRestore() }) - it('retries a failing path once the retry floor has passed, then gives up', async () => { + /** + * A cap that never expires latches a transient failure: one AV scan or momentary lock and every + * later credential write in the session is unprotected, on a host where hardening would now + * work. The rate is bounded; the lifetime is not. + */ + it('keeps re-probing a failing path in every later window', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) const targetPath = writeFailingHardenTarget() let clock = Date.now() const now = vi.spyOn(Date, 'now').mockImplementation(() => clock) - for (let read = 0; read < 10; read++) { - hardenExistingSecureFile(targetPath) - await flushAsyncAcl() + for (let window = 0; window < 6; window++) { + for (let read = 0; read < 6; read++) { + hardenExistingSecureFile(targetPath) + await flushAsyncAcl() + } clock += 61_000 } - // Three attempts spread over ten minutes, not ten — and then silence. - expect(attemptsFor(targetPath)).toHaveLength(3) + // Bounded per window, but never abandoned: 36 reads, 3 attempts in each of 6 windows. + expect(attemptsFor(targetPath)).toHaveLength(18) now.mockRestore() warn.mockRestore() }) + it('reports recovery when a previously throttled path hardens again', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const info = vi.spyOn(console, 'info').mockImplementation(() => {}) + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const targetPath = writeFailingHardenTarget() + let clock = Date.now() + const now = vi.spyOn(Date, 'now').mockImplementation(() => clock) + + for (let read = 0; read < 5; read++) { + hardenExistingSecureFile(targetPath) + await flushAsyncAcl() + } + expect(throttleReports(warn, targetPath)).toHaveLength(1) + + // The transient condition clears; the next window's re-probe must notice. + clock += 61_000 + vi.mocked(runProcess).mockImplementation((spec) => Promise.resolve(fakeIcacls(spec))) + hardenExistingSecureFile(targetPath) + await flushAsyncAcl() + + expect(info).toHaveBeenCalledWith( + '[secure-path.windows-acl] path hardening recovered', + expect.objectContaining({ targetPath, stage: 'recovered' }) + ) + now.mockRestore() + info.mockRestore() + warn.mockRestore() + }) + + // Scoped to one path: the parent directory is hardened too, and reports its own transition. + function throttleReports(warn: ReturnType, targetPath: string): unknown[] { + return warn.mock.calls.filter((call) => { + const entry = call[1] as { stage?: string; targetPath?: string } | undefined + return entry?.stage === 'throttled' && entry.targetPath === targetPath + }) + } + function writeFailingHardenTarget(): string { const userDataPath = mkdtempSync(join(tmpdir(), 'orca-secure-file-')) tempDirs.push(userDataPath) diff --git a/src/shared/secure-file.ts b/src/shared/secure-file.ts index eb5a019c8c8..cb6436acbfe 100644 --- a/src/shared/secure-file.ts +++ b/src/shared/secure-file.ts @@ -16,6 +16,11 @@ import { SecurePathHardeningCache, type SecurePathHardeningCacheBounds } from './secure-path-hardening-cache' +import { + configureHardeningRetryBudget, + mayAttemptHardening, + recordHardeningOutcome +} from './secure-path-hardening-retry-budget' import { bestEffortRestrictWindowsPath, resetSecureFileWindowsUserSidForTests, @@ -56,50 +61,15 @@ let hardenedDirectoryPathsThisProcess = new SecurePathHardeningCache( DEFAULT_HARDENING_CACHE_BOUNDS ) -type HardeningFailureRecord = { at: number; attempts: number } - -/** - * Why a retry floor and not a plain eviction: hardening fails permanently on hosts where it simply - * cannot work — FAT32/exFAT have no ACLs, network paths and redirected profiles refuse, restricted - * tokens lack WRITE_DAC. The env store re-hardens on the *read* path at ~2/s (#4901), so evicting - * on every failure turns those hosts into a permanent icacls-and-log storm. - */ -const HARDENING_RETRY_FLOOR_MS = 60_000 -const MAX_HARDENING_ATTEMPTS = 3 - -let hardeningFailuresThisProcess = new SecurePathHardeningCache( - DEFAULT_HARDENING_CACHE_BOUNDS -) - -function mayAttemptHardening(targetPath: string): boolean { - const failure = hardeningFailuresThisProcess.get(targetPath) - if (!failure) { - return true - } - if (failure.attempts >= MAX_HARDENING_ATTEMPTS) { - return false - } - return Date.now() - failure.at >= HARDENING_RETRY_FLOOR_MS -} - -function recordHardeningOutcome(targetPath: string, restricted: boolean): void { - if (restricted) { - hardeningFailuresThisProcess.delete(targetPath) - return - } - const previous = hardeningFailuresThisProcess.get(targetPath) - hardeningFailuresThisProcess.set(targetPath, { - at: Date.now(), - attempts: (previous?.attempts ?? 0) + 1 - }) -} +// Bounds the retry rate for paths whose hardening keeps failing; see the module for why. +configureHardeningRetryBudget(DEFAULT_HARDENING_CACHE_BOUNDS) function hardenSecureDirectoryOnce(dirPath: string): void { // Why: dir hardening stays async — re-applying it stormed the main thread (#4901); files inside are hardened synchronously anyway. if (hardenedDirectoryPathsThisProcess.get(dirPath)) { return } - // Cache before the ACL lands so concurrent writes don't restorm; a failure drops it, under the retry floor. + // Cache before the ACL lands so concurrent writes don't restorm; a failure drops it, under the retry budget. hardenedDirectoryPathsThisProcess.set(dirPath, true) applySecurePathRestriction(dirPath, true, process.platform, false, (restricted) => { if (!restricted) { @@ -358,7 +328,7 @@ export function __resetSecureFileHardenedPathsForTests( ): void { hardenedPathsThisProcess = new SecurePathHardeningCache(bounds) hardenedDirectoryPathsThisProcess = new SecurePathHardeningCache(bounds) - hardeningFailuresThisProcess = new SecurePathHardeningCache(bounds) + configureHardeningRetryBudget(bounds) } export function __getSecureFileHardeningCacheStateForTests(): { diff --git a/src/shared/secure-path-hardening-retry-budget.ts b/src/shared/secure-path-hardening-retry-budget.ts new file mode 100644 index 00000000000..b66522e8539 --- /dev/null +++ b/src/shared/secure-path-hardening-retry-budget.ts @@ -0,0 +1,76 @@ +import { + SecurePathHardeningCache, + type SecurePathHardeningCacheBounds +} from './secure-path-hardening-cache' +import { reportSecurePathHardening } from './secure-path-windows-acl' + +type HardeningFailureRecord = { windowStartedAt: number; attempts: number } + +/** + * How often a path whose hardening keeps failing may be retried. + * + * Why a rate limit and not a lifetime cap: the env store re-hardens on the *read* path at ~2/s + * (#4901), so retrying every failure is a permanent icacls-and-log storm on hosts where hardening + * cannot work — FAT32/exFAT have no ACLs, and network paths, redirected profiles and restricted + * tokens refuse. But a cap that never expires latches a *transient* failure: one AV scan or + * momentary lock, and every later credential write in the session is unprotected, silently, on a + * host where hardening would now succeed. So bound the retry *rate*, never the lifetime, and + * announce both directions of the transition so a stuck host is diagnosable. + */ +const HARDENING_RETRY_WINDOW_MS = 60_000 +const MAX_HARDENING_ATTEMPTS_PER_WINDOW = 3 + +let hardeningFailures: SecurePathHardeningCache | null = null + +function failures(): SecurePathHardeningCache { + if (!hardeningFailures) { + throw new Error('secure path hardening retry budget used before it was configured') + } + return hardeningFailures +} + +export function configureHardeningRetryBudget(bounds: SecurePathHardeningCacheBounds): void { + hardeningFailures = new SecurePathHardeningCache(bounds) +} + +export function mayAttemptHardening(targetPath: string): boolean { + const failure = failures().get(targetPath) + if (!failure) { + return true + } + // A stale window always re-probes: recovery must never require a restart to be noticed. + if (Date.now() - failure.windowStartedAt >= HARDENING_RETRY_WINDOW_MS) { + return true + } + return failure.attempts < MAX_HARDENING_ATTEMPTS_PER_WINDOW +} + +export function recordHardeningOutcome(targetPath: string, restricted: boolean): void { + const previous = failures().get(targetPath) + if (restricted) { + failures().delete(targetPath) + if (previous && previous.attempts >= MAX_HARDENING_ATTEMPTS_PER_WINDOW) { + reportSecurePathHardening( + targetPath, + 'recovered', + 'hardening succeeded again after being rate-limited' + ) + } + return + } + const now = Date.now() + const staleWindow = !previous || now - previous.windowStartedAt >= HARDENING_RETRY_WINDOW_MS + const attempts = staleWindow ? 1 : previous.attempts + 1 + failures().set(targetPath, { + windowStartedAt: staleWindow ? now : previous.windowStartedAt, + attempts + }) + // Fires exactly once per window: further attempts inside it are refused before they run. + if (attempts === MAX_HARDENING_ATTEMPTS_PER_WINDOW) { + reportSecurePathHardening( + targetPath, + 'throttled', + `hardening failed ${attempts} times; retrying at most ${MAX_HARDENING_ATTEMPTS_PER_WINDOW} times per ${HARDENING_RETRY_WINDOW_MS / 1000}s until it succeeds` + ) + } +} diff --git a/src/shared/secure-path-windows-acl.ts b/src/shared/secure-path-windows-acl.ts index bbee92b6622..6a7e95f6765 100644 --- a/src/shared/secure-path-windows-acl.ts +++ b/src/shared/secure-path-windows-acl.ts @@ -14,9 +14,10 @@ const BUILTIN_ADMINISTRATORS_SID = 'S-1-5-32-544' const WINDOWS_SID_PATTERN = /^S-1-\d+(?:-\d+)+$/ -export type SecurePathHardeningFailure = { +export type SecurePathHardeningReport = { targetPath: string - stage: 'sid-lookup' | 'reset' | 'grant' | 'verify' + /** `throttled` and `recovered` mark entering and leaving the rate-limited degraded state. */ + stage: 'sid-lookup' | 'reset' | 'grant' | 'verify' | 'throttled' | 'recovered' detail: string } @@ -149,20 +150,37 @@ function toIcaclsPath(targetPath: string): string { * installs a reporter that routes into the diagnostic trace; the console default keeps dev runs * and the CLI readable. */ -let reportFailure: (failure: SecurePathHardeningFailure) => void = (failure) => { - console.warn('[secure-path.windows-acl] failed to restrict path', failure) +const consoleReporter = (entry: SecurePathHardeningReport): void => { + if (entry.stage === 'recovered') { + console.info('[secure-path.windows-acl] path hardening recovered', entry) + return + } + console.warn('[secure-path.windows-acl] failed to restrict path', entry) } -export function setSecurePathHardeningFailureReporter( - reporter: ((failure: SecurePathHardeningFailure) => void) | null +let reportEntry: (entry: SecurePathHardeningReport) => void = consoleReporter + +export function setSecurePathHardeningReporter( + reporter: ((entry: SecurePathHardeningReport) => void) | null ): void { - reportFailure = reporter ?? ((failure) => { - console.warn('[secure-path.windows-acl] failed to restrict path', failure) - }) + reportEntry = reporter ?? consoleReporter } -function report(targetPath: string, stage: SecurePathHardeningFailure['stage'], detail: string): void { - reportFailure({ targetPath, stage, detail: detail.trim().slice(0, 500) }) +/** Exported so the caller owning the retry budget reports degradation and recovery on this lane. */ +export function reportSecurePathHardening( + targetPath: string, + stage: SecurePathHardeningReport['stage'], + detail: string +): void { + reportEntry({ targetPath, stage, detail: detail.trim().slice(0, 500) }) +} + +function report( + targetPath: string, + stage: SecurePathHardeningReport['stage'], + detail: string +): void { + reportSecurePathHardening(targetPath, stage, detail) } /** diff --git a/src/shared/secure-path-windows-acl.win32.test.ts b/src/shared/secure-path-windows-acl.win32.test.ts index abce510c727..b188195ff1b 100644 --- a/src/shared/secure-path-windows-acl.win32.test.ts +++ b/src/shared/secure-path-windows-acl.win32.test.ts @@ -191,6 +191,45 @@ describeOnWindows('restrictWindowsPathSync against a real filesystem', () => { expect(readAclEntries(file)).toEqual(first) }) + /** + * Verification writes a temp SDDL file. If it cannot, the ACL may well have been applied — but + * it cannot be *proved*, so hardening must report failure rather than assume success. Fail + * closed, and say so: a silently-unverifiable control is the shape of the original bug. + */ + it('reports failure, loudly, when verification cannot write its descriptor', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const file = join(root, 'unverifiable.json') + writeFileSync(file, '{}') + const realTemp = process.env.TEMP + const realTmp = process.env.TMP + // Point the descriptor save at a directory that cannot exist. + process.env.TEMP = join(root, 'no-such-dir', 'nested') + process.env.TMP = process.env.TEMP + + try { + expect(restrictWindowsPathSync(file, false)).toBe(false) + expect(warn).toHaveBeenCalledWith( + '[secure-path.windows-acl] failed to restrict path', + expect.objectContaining({ stage: 'verify' }) + ) + } finally { + if (realTemp === undefined) { + delete process.env.TEMP + } else { + process.env.TEMP = realTemp + } + if (realTmp === undefined) { + delete process.env.TMP + } else { + process.env.TMP = realTmp + } + warn.mockRestore() + } + + // And the ACL itself was still applied, so the failure is a loss of proof, not of protection. + 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)