diff --git a/src/main/startup/gpu-lifecycle-install-dir-acl-guard.test.ts b/src/main/startup/gpu-lifecycle-install-dir-acl-guard.test.ts index e35fe401cdc..7c6e7dcaaec 100644 --- a/src/main/startup/gpu-lifecycle-install-dir-acl-guard.test.ts +++ b/src/main/startup/gpu-lifecycle-install-dir-acl-guard.test.ts @@ -84,7 +84,7 @@ function reportProbePoisoned(): { finishRepair: () => Promise } { release = resolve }) startWindowsInstallDirAclRepairIfPoisoned( - { status: 'ok', matchesPoisonSignature: true, wellKnownNameCheckReliable: true }, + { status: 'ok', matchesPoisonSignature: true }, { ...recoveryOptions(), runProcessFn: (async () => { @@ -115,7 +115,7 @@ async function reportProbePoisonedWithSettledRepair( userDataPath?: string ): Promise { startWindowsInstallDirAclRepairIfPoisoned( - { status: 'ok', matchesPoisonSignature: true, wellKnownNameCheckReliable: true }, + { status: 'ok', matchesPoisonSignature: true }, { ...recoveryOptions(userDataPath), runProcessFn: (async () => ({ @@ -268,7 +268,7 @@ describe('handleGpuChildCrash vs the install-dir ACL verdict', () => { // The reading lands poisoned: the claim was false, and engagement stays withheld. startWindowsInstallDirAclRepairIfPoisoned( - { status: 'ok', matchesPoisonSignature: true, wellKnownNameCheckReliable: true }, + { status: 'ok', matchesPoisonSignature: true }, recoveryOptions(userData.path) ) await decisive diff --git a/src/main/startup/windows-install-dir-acl-probe.test.ts b/src/main/startup/windows-install-dir-acl-probe.test.ts index da63e35cd77..5e1d67b12d5 100644 --- a/src/main/startup/windows-install-dir-acl-probe.test.ts +++ b/src/main/startup/windows-install-dir-acl-probe.test.ts @@ -1,4 +1,7 @@ +import { existsSync } from 'node:fs' +import { tmpdir } from 'node:os' import { join } from 'node:path' +import type { CrashReportBreadcrumbData } from '../../shared/crash-reporting' import { describe, expect, it, beforeEach, vi } from 'vitest' import { probeWindowsInstallDirAcl, @@ -6,13 +9,10 @@ import { WINDOWS_INSTALL_DIR_ACL_BREADCRUMB, type WindowsInstallDirAclProbeOptions } from './windows-install-dir-acl-probe' +import { isInstallDirAclPoisonVerdict } from './windows-install-dir-package-acl-repair' import { ALL_PACKAGES_ACE, - ENGLISH_BASELINE_ACES, fakeIcaclsSpawn, - FRENCH_BASELINE_ACES, - FRENCH_RESTRICTED_PACKAGES_ACE, - icaclsDacl, ORPHAN_PACKAGE_ACE, RESTRICTED_PACKAGES_ACE } from './windows-install-dir-acl.test-fixture' @@ -21,10 +21,6 @@ const INSTALL_DIR = 'C:\\Users\\neil\\AppData\\Local\\Programs\\orca' const ORPHAN = ORPHAN_PACKAGE_ACE const RESTRICTED_GRANT = RESTRICTED_PACKAGES_ACE -function dacl(target: string, firstAce: string, ...rest: string[]): string { - return icaclsDacl(target, [firstAce, ...rest]) -} - const fakeSpawn = fakeIcaclsSpawn function probe(options: WindowsInstallDirAclProbeOptions): Promise> { @@ -44,7 +40,7 @@ function probe(options: WindowsInstallDirAclProbeOptions): Promise> { - return probe({ spawnFn: fakeSpawn((target) => dacl(target, aces[0], ...aces.slice(1))).spawnFn }) + return probe({ spawnFn: fakeSpawn(() => aces).spawnFn }) } describe('probeWindowsInstallDirAcl', () => { @@ -54,7 +50,7 @@ describe('probeWindowsInstallDirAcl', () => { it('reports a clean DACL as unpoisoned', async () => { const data = await probe({ - spawnFn: fakeSpawn((target) => icaclsDacl(target, [], ENGLISH_BASELINE_ACES)).spawnFn + spawnFn: fakeSpawn(() => []).spawnFn }) expect(data.name).toBe(WINDOWS_INSTALL_DIR_ACL_BREADCRUMB) expect(data.status).toBe('ok') @@ -82,8 +78,8 @@ describe('probeWindowsInstallDirAcl', () => { // orphan alongside it launched clean on win32 10.0.26200 / Electron 43.4.1 — so // it is not the reproduced state, however useless -1 is to an LPAC token. it.each([ - ['the localized-safe name form', ALL_PACKAGES_ACE], - ['the raw SID form', 'S-1-15-2-1:(OI)(CI)(RX)'] + ['the AC alias', ALL_PACKAGES_ACE], + ['the raw SID form', '(A;OICI;0x1200a9;;;S-1-15-2-1)'] ])('clears the signature when only ALL APPLICATION PACKAGES grants (%s)', async (_l, ace) => { const data = await probeWith(ace, ORPHAN) expect(data.hasWellKnownPackageGrant).toBe(true) @@ -94,10 +90,10 @@ describe('probeWindowsInstallDirAcl', () => { // The reproduced remedy was an additive *grant*; an ACE that grants nothing on // the object cannot satisfy the orphan, so it must not clear the signature. it.each([ - ['deny', 'APPLICATION PACKAGE AUTHORITY\\ALL APPLICATION PACKAGES:(DENY)(OI)(CI)(F)'], - ['inherit-only', 'APPLICATION PACKAGE AUTHORITY\\ALL APPLICATION PACKAGES:(OI)(CI)(IO)(GR,GE)'], - ['raw-sid deny', 'S-1-15-2-2:(DENY)(F)'], - ['raw-sid inherit-only', 'S-1-15-2-1:(OI)(CI)(IO)(GR,GE)'] + ['deny', '(D;OICI;FA;;;AC)'], + ['inherit-only', '(A;OICIIO;GRGX;;;AC)'], + ['restricted deny', '(D;;FA;;;S-1-15-2-2)'], + ['restricted inherit-only', '(A;CIOIIO;GRGX;;;S-1-15-2-2)'] ])('does not let a %s well-known ACE satisfy an orphan', async (_label, ace) => { const data = await probeWith(ace, ORPHAN) expect(data.hasWellKnownPackageGrant).toBe(false) @@ -108,67 +104,131 @@ describe('probeWindowsInstallDirAcl', () => { it('does not let a grant on one target mask its absence on another', async () => { const data = await probe({ spawnFn: fakeSpawn((target) => - target.endsWith('ffmpeg.dll') - ? dacl(target, ORPHAN) - : dacl(target, RESTRICTED_GRANT, ORPHAN) + target.endsWith('ffmpeg.dll') ? [ORPHAN] : [RESTRICTED_GRANT, ORPHAN] ).spawnFn }) expect(data.hasWellKnownPackageGrant).toBe(true) expect(data.matchesPoisonSignature).toBe(true) }) - it('reports whether the well-known name check could be trusted', async () => { - const english = await probeWith(ORPHAN) - expect(english.wellKnownNameCheckReliable).toBe(true) - const localized = await new Promise>((resolve) => { - resetWindowsInstallDirAclProbeForTest() - probeWindowsInstallDirAcl({ - platform: 'win32', - installDir: INSTALL_DIR, - fileExists: () => false, - // fr-FR install that already carries the restricted grant: the name check - // cannot see it, so the signature is a false positive the flag must expose. - spawnFn: fakeSpawn((target) => - icaclsDacl(target, [ORPHAN, FRENCH_RESTRICTED_PACKAGES_ACE], FRENCH_BASELINE_ACES) - ).spawnFn, - recordBreadcrumb: (_name, d) => { - resolve(d as Record) - return undefined - } - }) + // zh-CN/ja-JP/ko-KR icacls keeps "NT AUTHORITY" English but translates the package + // names and summary. A tree the repair just fixed read as poisoned AND reliable there, + // re-arming the pre-window repair and blaming the install on every launch. + it('reads a repaired tree as clean when icacls translates package names', async () => { + const zhDisplay = (target: string): string => + [ + `${target} S-1-15-2-999-999-999:(OI)(CI)(RX)`, + ' APPLICATION PACKAGE AUTHORITY\\所有受限制的应用程序包:(OI)(CI)(RX)', + ' NT AUTHORITY\\SYSTEM:(I)(OI)(CI)(F)', + ' BUILTIN\\Administrators:(I)(OI)(CI)(F)', + '', + '已成功处理 1 个文件; 处理 0 个文件时失败' + ].join('\r\n') + let verdict: CrashReportBreadcrumbData = {} + const data = await probe({ + spawnFn: fakeSpawn(() => [ORPHAN, RESTRICTED_GRANT], zhDisplay).spawnFn, + onDone: (done) => (verdict = done) }) - expect(localized.matchesPoisonSignature).toBe(true) - expect(localized.wellKnownNameCheckReliable).toBe(false) + expect(isInstallDirAclPoisonVerdict(verdict)).toBe(false) + expect(data.status).toBe('ok') + expect(data.orphanPackageSidCount).toBe(1) + expect(data.hasRestrictedPackageGrant).toBe(true) }) it('matches the well-known SIDs exactly, not by prefix', async () => { - const data = await probeWith('S-1-15-2-1234567890:(OI)(CI)(RX)') + const data = await probeWith('(A;OICI;0x1200a9;;;S-1-15-2-1234567890)') expect(data.orphanPackageSidCount).toBe(1) expect(data.hasWellKnownPackageGrant).toBe(false) expect(data.matchesPoisonSignature).toBe(true) }) it('ignores capability SIDs, which are a different family and harmless', async () => { - const data = await probeWith('S-1-15-3-65536-599108337-2355189375-1353122160:(S,X)') + const data = await probeWith('(A;;0x100020;;;S-1-15-3-65536-599108337-2355189375-1353122160)') expect(data.orphanPackageSidCount).toBe(0) expect(data.matchesPoisonSignature).toBe(false) }) it('probes a content file, not just the directory object', async () => { - const fake = fakeSpawn((target) => dacl(target, ORPHAN)) + const fake = fakeSpawn(() => [ORPHAN]) await probe({ spawnFn: fake.spawnFn }) expect(fake.calls.map((c) => c.args[0])).toEqual([INSTALL_DIR, join(INSTALL_DIR, 'ffmpeg.dll')]) }) - it('never passes a recursive or write flag', async () => { - const fake = fakeSpawn((target) => dacl(target, ORPHAN)) + it('only saves the DACL to a temp file: no recursive or ACL-writing flag', async () => { + const fake = fakeSpawn(() => [ORPHAN]) await probe({ spawnFn: fake.spawnFn }) for (const call of fake.calls) { - expect(call.args).toHaveLength(1) + expect(call.args).toHaveLength(3) expect(call.args[0]).not.toMatch(/^\//) + expect(call.args[1]).toBe('/save') + expect(call.args[2].startsWith(tmpdir())).toBe(true) + expect(existsSync(call.args[2])).toBe(false) } }) + // Verbatim `icacls /save` / `icacls /save` output from win32 10.0.26200. + it.each([ + [ + 'a repaired module file', + 'ffmpeg.dll\r\nD:AI(A;;0x1200a9;;;S-1-15-2-2)(A;;0x1200a9;;;S-1-15-2-999-999-999)' + + '(A;ID;0x1200a9;;;S-1-15-2-999-999-999)(A;ID;FA;;;SY)(A;ID;FA;;;BA)' + + '(A;ID;FA;;;S-1-5-21-432636774-4279371817-3971399515-1001)\r\n', + false + ], + [ + 'a poisoned dir whose AC ACE denies', + 'scan21acl\r\nD:AI(D;;0x100116;;;AC)(A;OICI;0x1200a9;;;S-1-15-2-999-999-999)' + + '(A;OICIID;FA;;;SY)(A;OICIID;FA;;;BA)' + + '(A;OICIID;FA;;;S-1-5-21-432636774-4279371817-3971399515-1001)\r\n', + true + ] + ])('reads a real icacls /save capture: %s', async (_label, saved, poisoned) => { + const data = await probe({ + fileExists: () => false, + spawnFn: fakeSpawn(() => Buffer.from(saved, 'utf16le')).spawnFn + }) + expect(data.orphanPackageSids).toBe('S-1-15-2-999-999-999') + expect(data.matchesPoisonSignature).toBe(poisoned) + }) + + it('keeps a conditional DACL unreadable instead of hiding a later package grant', async () => { + const saved = + 'orca\r\nD:AI(A;OICI;0x1200a9;;;S-1-15-2-999-999-999)' + + '(XA;OICI;FA;;;WD;(@User.Department == "Finance"))' + + '(A;OICIID;0x1200a9;;;S-1-15-2-2)\r\n' + let verdict: CrashReportBreadcrumbData = {} + const data = await probe({ + fileExists: () => false, + spawnFn: fakeSpawn(() => Buffer.from(saved, 'utf16le')).spawnFn, + onDone: (done) => (verdict = done) + }) + expect(data).toMatchObject({ status: 'failed', reason: 'all-targets-unreadable' }) + expect(data.matchesPoisonSignature).toBeUndefined() + expect(isInstallDirAclPoisonVerdict(verdict)).toBe(false) + }) + + it.each([1, null])('ignores a saved DACL when icacls exits with %s', async (exitCode) => { + const fake = fakeSpawn(() => [ORPHAN], undefined, exitCode) + let verdict: CrashReportBreadcrumbData = {} + const data = await probe({ + fileExists: () => false, + spawnFn: fake.spawnFn, + onDone: (done) => (verdict = done) + }) + expect(data).toMatchObject({ status: 'failed', reason: 'all-targets-unreadable' }) + expect(isInstallDirAclPoisonVerdict(verdict)).toBe(false) + expect(fake.calls.every((call) => !existsSync(call.args[2]))).toBe(true) + }) + + it('does not report malformed saved output as a clean DACL', async () => { + const data = await probe({ + fileExists: () => false, + spawnFn: fakeSpawn(() => Buffer.from('orca\r\n', 'utf16le')).spawnFn + }) + expect(data).toMatchObject({ status: 'failed', reason: 'all-targets-unreadable' }) + expect(data.matchesPoisonSignature).toBeUndefined() + }) + it('records a failure instead of throwing when icacls cannot be read', async () => { const data = await probe({ spawnFn: fakeSpawn(() => null).spawnFn }) expect(data.status).toBe('failed') @@ -180,7 +240,7 @@ describe('probeWindowsInstallDirAcl', () => { ['darwin', { platform: 'darwin' as NodeJS.Platform }], ['serve mode', { platform: 'win32' as NodeJS.Platform, isServeMode: true }] ])('does no work on %s', async (_label, options) => { - const fake = fakeSpawn((target) => dacl(target, ORPHAN)) + const fake = fakeSpawn(() => [ORPHAN]) const record = vi.fn() const fileExists = vi.fn(() => true) probeWindowsInstallDirAcl({ @@ -197,7 +257,7 @@ describe('probeWindowsInstallDirAcl', () => { }) it('runs once per process', async () => { - const fake = fakeSpawn((target) => dacl(target, ORPHAN)) + const fake = fakeSpawn(() => [ORPHAN]) await probe({ spawnFn: fake.spawnFn }) const before = fake.calls.length probeWindowsInstallDirAcl({ platform: 'win32', installDir: INSTALL_DIR, spawnFn: fake.spawnFn }) diff --git a/src/main/startup/windows-install-dir-acl-probe.ts b/src/main/startup/windows-install-dir-acl-probe.ts index 25d1e581c9b..a717c87c241 100644 --- a/src/main/startup/windows-install-dir-acl-probe.ts +++ b/src/main/startup/windows-install-dir-acl-probe.ts @@ -1,5 +1,7 @@ import { spawn } from 'node:child_process' -import { existsSync } from 'node:fs' +import { randomUUID } from 'node:crypto' +import { existsSync, readFileSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' import { sanitizeCrashReportString, @@ -36,23 +38,16 @@ const MODULE_SHORTLIST = ['ffmpeg.dll', 'libGLESv2.dll', 'libEGL.dll', 'icudtl.d const PROBE_BUDGET_MS = 5_000 -/** Raw S-1-15-2-* means icacls could not resolve it — locale-independent. */ -const RAW_PACKAGE_SID = /\bS-1-15-2-[0-9-]+\b/i +// Why SDDL (`icacls /save`) rather than icacls's display: the display localizes +// the well-known package names — and on zh/ja/ko even keeps "NT AUTHORITY" English +// while doing so — so a repaired tree read as poisoned. SDDL prints SIDs on every locale. +const PACKAGE_SID = /^S-1-15-2-[0-9-]+$/i /** ALL RESTRICTED APPLICATION PACKAGES: the grant the reproduced remedy added. */ -const RESTRICTED_PACKAGES_SID = 's-1-15-2-2' -const WELL_KNOWN_PACKAGE_SIDS = new Set(['s-1-15-2-1', RESTRICTED_PACKAGES_SID]) -// icacls localizes these; the raw SID form is never printed for them. Orphan -// detection stays SID-form and locale-independent, but this check is not — so we -// report whether the output looked English at all, making a locale-induced -// false positive recognizable instead of silent. -const WELL_KNOWN_PACKAGE_NAMES = /ALL (RESTRICTED )?APPLICATION PACKAGES/i -const RESTRICTED_PACKAGE_NAME = /ALL RESTRICTED APPLICATION PACKAGES/i -// Not BUILTIN: fr-FR and es-ES print it verbatim while localizing the package -// names, so it is evidence of nothing. SYSTEM's ACE is on every install tree. -const ENGLISH_PRINCIPAL = /\b(NT AUTHORITY|APPLICATION PACKAGE AUTHORITY)\b/i -// Why: an ACE that denies, or only propagates to children, grants nothing on this -// object — so it cannot satisfy an orphan the way the reproduced fix did. -const NON_GRANTING_FLAGS = /\((?:DENY|IO)\)/i +const RESTRICTED_PACKAGES_SID = 'S-1-15-2-2' +/** `AC` is SDDL's alias for ALL APPLICATION PACKAGES (S-1-15-2-1). */ +const WELL_KNOWN_PACKAGE_SIDS = new Set(['AC', 'S-1-15-2-1', RESTRICTED_PACKAGES_SID]) +/** (type;flags;rights;object;inheritedObject;sid[;condition]) */ +const SDDL_ACE = /\(([A-Z]+);([A-Z]*);[^;()]*;[^;()]*;[^;()]*;([^;()]+)[;)]/gi export type WindowsInstallDirAclProbeOptions = { platform?: NodeJS.Platform @@ -69,80 +64,80 @@ type AclFacts = { orphanPackageSids: string[] hasWellKnownPackageGrant: boolean hasRestrictedPackageGrant: boolean - sawEnglishPrincipal: boolean } -function readDacl(spawnFn: typeof spawn, target: string, deadlineMs: number): Promise { - return new Promise((resolve) => { - // No flags: icacls with a bare path only reads. Never /T — a recursive walk on - // a real profile measured 62s and timed out (see windows-user-data-acl.ts). - const child = spawnFn(getIcaclsExePath(), [target], { - stdio: ['ignore', 'pipe', 'ignore'], +function readSavedDacl(spawnFn: typeof spawn, target: string, deadlineMs: number): Promise { + const saveFile = join(tmpdir(), `orca-install-acl-${randomUUID()}.txt`) + return new Promise((resolve) => { + // /save only reads the target (it writes the temp file). Never /T — a recursive + // walk on a real profile measured 62s and timed out (see windows-user-data-acl.ts). + const child = spawnFn(getIcaclsExePath(), [target, '/save', saveFile], { + stdio: ['ignore', 'ignore', 'ignore'], windowsHide: true }) - let out = '' let settled = false - const settle = (value: string): void => { + const settle = (read: boolean): void => { if (settled) { return } settled = true clearTimeout(timer) - resolve(value) + let out = '' + try { + // An empty read parses as a clean DACL, so a failed /save must stay ''. + out = read ? readFileSync(saveFile, 'utf16le') : '' + } catch { + out = '' + } + try { + // A killed icacls may still hold the file; a leftover temp file is harmless. + rmSync(saveFile, { force: true }) + } catch { + // Nothing to do. + } + resolve(out) } const timer = setTimeout(() => { child.kill() - settle('') + settle(false) }, deadlineMs) timer.unref?.() - child.stdout?.on('data', (chunk: Buffer) => { - out += chunk.toString('utf-8') - }) - child.on('error', () => settle('')) - // 'close' not 'exit': exit can fire before stdout drains, and an empty read - // would parse as a clean DACL — a false negative in the only case we care about. - child.on('close', () => settle(out)) + child.on('error', () => settle(false)) + child.on('close', (code) => settle(code === 0)) }) } -function collectAclFacts(daclOutput: string): AclFacts { +function collectAclFacts(savedDacl: string): AclFacts | null { const orphanPackageSids: string[] = [] let hasWellKnownPackageGrant = false let hasRestrictedPackageGrant = false - let sawEnglishPrincipal = false - for (const line of daclOutput.split(/\r?\n/)) { - if (ENGLISH_PRINCIPAL.test(line)) { - sawEnglishPrincipal = true - } - // icacls glues the echoed path onto the first principal with no separator, so - // match the principal:(flags) tail rather than trying to split the line. - const ace = /([^\s:][^:]*):(\([^\s]*\))\s*$/.exec(line.trim()) - if (!ace) { + // A partial DACL can hide a later grant; unsupported ACEs must stay unreadable. + const dacl = /^D:[A-Z]*((?:\([^()]*\))*)(?:S:.*)?$/im.exec(savedDacl) + if (!dacl) { + return null + } + for (const [, type, flags, rawSid] of dacl[1].matchAll(SDDL_ACE)) { + const sid = rawSid.toUpperCase() + if (!WELL_KNOWN_PACKAGE_SIDS.has(sid)) { + if (PACKAGE_SID.test(sid)) { + orphanPackageSids.push(rawSid) + } continue } - const [, principal, flags] = ace - const rawSid = RAW_PACKAGE_SID.exec(principal)?.[0] - const sid = rawSid?.toLowerCase() - if (rawSid && !WELL_KNOWN_PACKAGE_SIDS.has(rawSid.toLowerCase())) { - orphanPackageSids.push(rawSid) - continue - } - const isWellKnown = sid !== undefined || WELL_KNOWN_PACKAGE_NAMES.test(principal) - if (!isWellKnown || NON_GRANTING_FLAGS.test(flags)) { + // Why: an ACE that denies, or only propagates to children (IO), grants nothing + // on this object — so it cannot satisfy an orphan the way the reproduced fix did. + const flagTokens: string[] = flags.toUpperCase().match(/../g) ?? [] + const inheritOnly = flagTokens.includes('IO') + if (type.toUpperCase() !== 'A' || inheritOnly) { continue } hasWellKnownPackageGrant = true // Reported, never the verdict: narrows which grant is present for triage. - if (sid === RESTRICTED_PACKAGES_SID || (!sid && RESTRICTED_PACKAGE_NAME.test(principal))) { + if (sid === RESTRICTED_PACKAGES_SID) { hasRestrictedPackageGrant = true } } - return { - orphanPackageSids, - hasWellKnownPackageGrant, - hasRestrictedPackageGrant, - sawEnglishPrincipal - } + return { orphanPackageSids, hasWellKnownPackageGrant, hasRestrictedPackageGrant } } function resolveTargets(installDir: string, fileExists: (path: string) => boolean): string[] { @@ -162,9 +157,9 @@ async function runProbe(options: WindowsInstallDirAclProbeOptions): Promise 0 ? await readDacl(spawnFn, target, remaining) : '') + outputs.push(remaining > 0 ? await readSavedDacl(spawnFn, target, remaining) : '') } - const facts = outputs.map(collectAclFacts) + const facts = outputs.map(collectAclFacts).filter((fact) => fact !== null) const orphans = [...new Set(facts.flatMap((f) => f.orphanPackageSids))] const hasWellKnownPackageGrant = facts.some((f) => f.hasWellKnownPackageGrant) const hasRestrictedPackageGrant = facts.some((f) => f.hasRestrictedPackageGrant) @@ -174,24 +169,22 @@ async function runProbe(options: WindowsInstallDirAclProbeOptions): Promise f.orphanPackageSids.length > 0 && !f.hasWellKnownPackageGrant ) - data = outputs.every((out) => out === '') - ? { status: 'failed', reason: 'all-targets-unreadable' } - : { - status: 'ok', - probedTargetCount: targets.length, - orphanPackageSidCount: orphans.length, - // Capped: correlating the same orphan across reports is what would - // identify the tool that left it, which is the point of recording it. - orphanPackageSids: sanitizeCrashReportString(orphans.slice(0, 3).join(','), 200), - // The verdict rides on this one: either well-known grant satisfies the orphan. - hasWellKnownPackageGrant, - // Diagnostic only — the -1-only shape launches clean on real hardware. - hasRestrictedPackageGrant, - // False positives are possible on a non-English Windows, where the - // well-known ACE resolves to a localized name this cannot match. - wellKnownNameCheckReliable: facts.some((f) => f.sawEnglishPrincipal), - matchesPoisonSignature: poisoned - } + data = + facts.length === 0 + ? { status: 'failed', reason: 'all-targets-unreadable' } + : { + status: 'ok', + probedTargetCount: targets.length, + orphanPackageSidCount: orphans.length, + // Capped: correlating the same orphan across reports is what would + // identify the tool that left it, which is the point of recording it. + orphanPackageSids: sanitizeCrashReportString(orphans.slice(0, 3).join(','), 200), + // The verdict rides on this one: either well-known grant satisfies the orphan. + hasWellKnownPackageGrant, + // Diagnostic only — the -1-only shape launches clean on real hardware. + hasRestrictedPackageGrant, + matchesPoisonSignature: poisoned + } } catch (error) { data = { status: 'failed', reason: sanitizeCrashReportString(`probe: ${String(error)}`, 200) } } diff --git a/src/main/startup/windows-install-dir-acl-recovery.test.ts b/src/main/startup/windows-install-dir-acl-recovery.test.ts index 7641cdc0700..23ca9de5062 100644 --- a/src/main/startup/windows-install-dir-acl-recovery.test.ts +++ b/src/main/startup/windows-install-dir-acl-recovery.test.ts @@ -32,9 +32,6 @@ import { import { ALL_PACKAGES_ACE, fakeIcaclsSpawn, - FRENCH_BASELINE_ACES, - FRENCH_RESTRICTED_PACKAGES_ACE, - icaclsDacl, ORPHAN_PACKAGE_ACE, RESTRICTED_PACKAGES_ACE } from './windows-install-dir-acl.test-fixture' @@ -49,7 +46,7 @@ type Runner = (spec: ProcessSpec) => Promise * which decides whether icacls ever runs. Only the two process seams are faked. */ function probeThenRecover( - dacl: (target: string) => string, + savedAces: (target: string) => string[], options: { failRepair?: boolean } = {} ): Promise { const specs: ProcessSpec[] = [] @@ -68,7 +65,7 @@ function probeThenRecover( platform: 'win32', installDir: INSTALL_DIR, fileExists: (path) => path.endsWith('ffmpeg.dll'), - spawnFn: fakeIcaclsSpawn(dacl).spawnFn, + spawnFn: fakeIcaclsSpawn(savedAces).spawnFn, recordBreadcrumb: () => undefined, onDone: (data) => { startWindowsInstallDirAclRepairIfPoisoned(data, { @@ -95,7 +92,7 @@ describe('startWindowsInstallDirAclRepairIfPoisoned', () => { }) it('repairs when the probe sees an orphan package ACE and no well-known grant', async () => { - const specs = await probeThenRecover((target) => icaclsDacl(target, [ORPHAN_PACKAGE_ACE])) + const specs = await probeThenRecover(() => [ORPHAN_PACKAGE_ACE]) expect(specs.map((spec) => spec.args?.[2])).toEqual([ '*S-1-15-2-2:(OI)(CI)(RX)', '*S-1-15-2-2:(RX)' @@ -106,27 +103,13 @@ describe('startWindowsInstallDirAclRepairIfPoisoned', () => { // win32 10.0.26200 / Electron 43.4.1, so it earns neither an ACL write nor the // accusing dialog copy. it('leaves an install whose package grant is ALL APPLICATION PACKAGES alone', async () => { - const specs = await probeThenRecover((target) => - icaclsDacl(target, [ORPHAN_PACKAGE_ACE, ALL_PACKAGES_ACE]) - ) + const specs = await probeThenRecover(() => [ORPHAN_PACKAGE_ACE, ALL_PACKAGES_ACE]) expect(specs).toHaveLength(0) expect(describeInstallDirAclPoison()).toBeNull() }) it('does not touch an install that already carries the restricted grant', async () => { - const specs = await probeThenRecover((target) => - icaclsDacl(target, [ORPHAN_PACKAGE_ACE, RESTRICTED_PACKAGES_ACE]) - ) - expect(specs).toHaveLength(0) - expect(describeInstallDirAclPoison()).toBeNull() - }) - - // A localized icacls prints the grant under a name the probe cannot match, so - // the signature is unproven: neither icacls nor the accusing dialog copy. - it('does not act on a signature from a non-English icacls', async () => { - const specs = await probeThenRecover((target) => - icaclsDacl(target, [ORPHAN_PACKAGE_ACE, FRENCH_RESTRICTED_PACKAGES_ACE], FRENCH_BASELINE_ACES) - ) + const specs = await probeThenRecover(() => [ORPHAN_PACKAGE_ACE, RESTRICTED_PACKAGES_ACE]) expect(specs).toHaveLength(0) expect(describeInstallDirAclPoison()).toBeNull() }) @@ -169,7 +152,7 @@ describe('describeInstallDirAclPoison', () => { }) it('offers the copyable commands, and drops them once the repair lands', async () => { - await probeThenRecover((target) => icaclsDacl(target, [ORPHAN_PACKAGE_ACE])) + await probeThenRecover(() => [ORPHAN_PACKAGE_ACE]) const repaired = describeInstallDirAclPoison() expect(repaired?.detail).toContain('Orca repaired the permissions') expect(repaired?.detail).not.toContain('Administrator Command Prompt') @@ -180,7 +163,7 @@ describe('describeInstallDirAclPoison', () => { }) it('walks a standard user through icacls when the repair could not write', async () => { - await probeThenRecover((target) => icaclsDacl(target, [ORPHAN_PACKAGE_ACE]), { + await probeThenRecover(() => [ORPHAN_PACKAGE_ACE], { failRepair: true }) const failed = describeInstallDirAclPoison() @@ -190,7 +173,7 @@ describe('describeInstallDirAclPoison', () => { it('reports the repair as in flight before icacls has answered', () => { startWindowsInstallDirAclRepairIfPoisoned( - { status: 'ok', matchesPoisonSignature: true, wellKnownNameCheckReliable: true }, + { status: 'ok', matchesPoisonSignature: true }, { platform: 'win32', installDir: INSTALL_DIR, @@ -206,8 +189,7 @@ describe('describeInstallDirAclPoison', () => { const POISON_VERDICT: CrashReportBreadcrumbData = { status: 'ok', - matchesPoisonSignature: true, - wellKnownNameCheckReliable: true + matchesPoisonSignature: true } const GPU_ENV = { appVersion: APP_VERSION, electronVersion: '43.4.1', platform: 'win32' } as const @@ -897,7 +879,7 @@ describe('the probe-pending grace window', () => { platform: 'win32' as const, installDir: INSTALL_DIR, fileExists: () => false, - spawnFn: fakeIcaclsSpawn((target) => icaclsDacl(target, [RESTRICTED_PACKAGES_ACE])).spawnFn, + spawnFn: fakeIcaclsSpawn(() => [RESTRICTED_PACKAGES_ACE]).spawnFn, recordBreadcrumb: () => undefined } let settleVerdict: () => void = () => undefined diff --git a/src/main/startup/windows-install-dir-acl.test-fixture.ts b/src/main/startup/windows-install-dir-acl.test-fixture.ts index ecbfec808f0..f28fe9c3d67 100644 --- a/src/main/startup/windows-install-dir-acl.test-fixture.ts +++ b/src/main/startup/windows-install-dir-acl.test-fixture.ts @@ -1,64 +1,69 @@ import { EventEmitter } from 'node:events' +import { writeFileSync } from 'node:fs' +import { basename } from 'node:path' import type { spawn } from 'node:child_process' -/** Shared icacls doubles for the install-dir DACL probe, repair, and recovery tests. */ +/** + * Shared icacls doubles for the install-dir DACL probe, repair, and recovery tests. + * ACEs are SDDL, the shape `icacls /save ` writes (captured on + * win32 10.0.26200: the leaf name, CRLF, then `D:AI(...)(...)`, UTF-16LE). + */ -export const ORPHAN_PACKAGE_ACE = 'S-1-15-2-999-999-999:(OI)(CI)(RX)' -export const RESTRICTED_PACKAGES_ACE = - 'APPLICATION PACKAGE AUTHORITY\\ALL RESTRICTED APPLICATION PACKAGES:(OI)(CI)(RX)' -/** The Program Files default: present on healthy installs, which launch clean. */ -export const ALL_PACKAGES_ACE = 'APPLICATION PACKAGE AUTHORITY\\ALL APPLICATION PACKAGES:(RX)' +export const ORPHAN_PACKAGE_ACE = '(A;OICI;0x1200a9;;;S-1-15-2-999-999-999)' +/** ALL RESTRICTED APPLICATION PACKAGES; SDDL has no alias for it. */ +export const RESTRICTED_PACKAGES_ACE = '(A;OICI;0x1200a9;;;S-1-15-2-2)' +/** ALL APPLICATION PACKAGES (`AC`), the Program Files default: healthy installs launch clean. */ +export const ALL_PACKAGES_ACE = '(A;;0x1200a9;;;AC)' -export const ENGLISH_BASELINE_ACES = [ - 'NT AUTHORITY\\SYSTEM:(I)(OI)(CI)(F)', - 'BUILTIN\\Administrators:(I)(OI)(CI)(F)', - 'awin\\neil:(I)(OI)(CI)(F)' +export const BASELINE_ACES = [ + '(A;OICIID;FA;;;SY)', + '(A;OICIID;FA;;;BA)', + '(A;OICIID;FA;;;S-1-5-21-432636774-4279371817-3971399515-1001)' ] -/** fr-FR icacls: no principal the English name check can recognize. */ -export const FRENCH_BASELINE_ACES = [ - 'AUTORITE NT\\Systeme:(I)(OI)(CI)(F)', - 'BUILTIN\\Administrateurs:(I)(OI)(CI)(F)' -] -export const FRENCH_RESTRICTED_PACKAGES_ACE = - "AUTORITE DE PACKAGE D'APPLICATION\\TOUS LES PACKAGES D'APPLICATION RESTREINTS:(RX)" -/** Real icacls shape: the echoed path is glued onto the first principal. */ -export function icaclsDacl( - target: string, - aces: string[], - baseline: string[] = ENGLISH_BASELINE_ACES -): string { - const [first, ...rest] = [...aces, ...baseline] - return [ - `${target} ${first}`, - ...rest.map((ace) => ` ${ace}`), - '', - 'Successfully processed 1 files' - ].join('\r\n') +/** The file `icacls /save` writes for a target carrying these ACEs. */ +export function icaclsSavedAcl(target: string, aces: string[]): Buffer { + const leaf = basename(target.replaceAll('\\', '/')) + return Buffer.from(`${leaf}\r\nD:AI${[...aces, ...BASELINE_ACES].join('')}\r\n`, 'utf16le') } -/** `null` output makes the spawn fail, as an unreadable target does. */ -export function fakeIcaclsSpawn(output: (target: string) => string | null): { +/** + * `saved` returns the target's extra ACEs, or the raw saved file; `null` makes the + * spawn fail, as an unreadable target does. `display` is what a bare + * `icacls ` prints. + */ +export function fakeIcaclsSpawn( + saved: (target: string) => string[] | Buffer | null, + display: (target: string) => string = () => '', + exitCode: number | null = 0 +): { spawnFn: typeof spawn calls: { file: string; args: string[] }[] } { const calls: { file: string; args: string[] }[] = [] + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: test double; the probe only touches kill/stdout/on of the child. const spawnFn = ((file: string, args: string[]) => { calls.push({ file, args }) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: both fields are assigned on the next two lines. const child = new EventEmitter() as EventEmitter & { stdout: EventEmitter kill: () => void } child.stdout = new EventEmitter() child.kill = () => undefined - const out = output(args[0]) + const [target, verb, saveFile] = args + const aces = saved(target) setImmediate(() => { - if (out === null) { + if (aces === null) { child.emit('error', new Error('ENOENT')) return } - child.stdout.emit('data', Buffer.from(out, 'utf-8')) - child.emit('close', 0) + if (verb === '/save') { + writeFileSync(saveFile, Buffer.isBuffer(aces) ? aces : icaclsSavedAcl(target, aces)) + } else { + child.stdout.emit('data', Buffer.from(display(target), 'utf-8')) + } + child.emit('close', exitCode) }) return child }) as unknown as typeof spawn diff --git a/src/main/startup/windows-install-dir-package-acl-repair.test.ts b/src/main/startup/windows-install-dir-package-acl-repair.test.ts index cc41388b811..e7222b016c0 100644 --- a/src/main/startup/windows-install-dir-package-acl-repair.test.ts +++ b/src/main/startup/windows-install-dir-package-acl-repair.test.ts @@ -312,8 +312,7 @@ describe('isInstallDirAclPoisonVerdict', () => { expect( isInstallDirAclPoisonVerdict({ status: 'ok', - matchesPoisonSignature: true, - wellKnownNameCheckReliable: true + matchesPoisonSignature: true }) ).toBe(true) expect( @@ -324,16 +323,4 @@ describe('isInstallDirAclPoisonVerdict', () => { ).toBe(false) expect(isInstallDirAclPoisonVerdict({ status: 'failed', reason: 'unreadable' })).toBe(false) }) - - // A localized icacls hides the well-known grants behind translated names, so the - // signature there is unproven: repairing and blaming the install would be wrong. - it('refuses a signature the probe could not name-check', () => { - expect( - isInstallDirAclPoisonVerdict({ - status: 'ok', - matchesPoisonSignature: true, - wellKnownNameCheckReliable: false - }) - ).toBe(false) - }) }) diff --git a/src/main/startup/windows-install-dir-package-acl-repair.ts b/src/main/startup/windows-install-dir-package-acl-repair.ts index 6bd6505a2cb..878f7c47411 100644 --- a/src/main/startup/windows-install-dir-package-acl-repair.ts +++ b/src/main/startup/windows-install-dir-package-acl-repair.ts @@ -103,13 +103,11 @@ const MAX_REPAIR_ATTEMPTS = 3 /** * The probe's verdict is the only trigger: an orphan package ACE with no - * well-known package grant to satisfy it. A localized icacls prints those grants - * under translated names the probe cannot match, so an unreliable name check is - * not evidence of poison — acting on it would spawn icacls and tell a user with a - * healthy install that their permissions are broken. + * well-known package grant to satisfy it. The probe reads SDDL, so the verdict + * holds on every Windows display language. */ export function isInstallDirAclPoisonVerdict(data: CrashReportBreadcrumbData): boolean { - return data.matchesPoisonSignature === true && data.wellKnownNameCheckReliable !== false + return data.matchesPoisonSignature === true } /** The commands to hand a user whose account cannot write the install ACL. */