From ac87703657a18087ad7080b47a4528550d52d34f Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 11:21:56 +0900 Subject: [PATCH 1/3] fix(service): stage elevated Task Scheduler XML instead of inlining it (#4692) When "ocx service repair" re-registered the task through the elevated fallback, the spawn failed before UAC ever appeared: WindowsElevationError: ENAMETOOLONG: name too long, uv_spawn at startPowerShellCommand (src/lib/windows-elevation.ts:560) at runWindowsElevatedScheduledTaskRegistration (.../windows-elevation.ts:704) runWindowsElevatedScheduledTaskRegistration embedded the new task XML and the expected-existing snapshot as base64(utf16le) inside an inner PowerShell script, which was then base64(utf16le)-encoded again into -EncodedCommand. Two base64 layers over UTF-16 cost roughly 14.2 command-line characters per XML character, and a replacement carries two payloads, so a ~2 KB definition put the outer command past the Windows limit. On a host where Task Scheduler exports the trigger scope as an account name the re-register path runs on every repair, so repair could never exit 0. Both payloads are now staged to files and the command carries two paths and two 64-character digests, so its length no longer depends on the size of the XML at all. A file an administrator process will read is itself a privilege-escalation surface, so three properties hold together and none is sufficient alone: - Access. The staging directory is created fresh by mkdtemp and ACL-hardened through the existing hardenSecretDir/hardenSecretPath before anything is written into it, so the payload is private from the moment it exists. - No redirection. Each artifact is inspected with lstat and rejected unless it is what it claims to be. Exclusive "wx" creation inside a directory that did not exist a moment ago is the atomic step; the explicit check keeps that guarantee from resting on a reading of O_EXCL semantics. - Tamper evidence. The digest covers the exact bytes written, and the elevated script reads the file once, hashes what it read, and refuses before decoding. An ACL cannot cover this: a process running as the same user has the same SID and can rewrite the file, so the digest is what makes a swap during the UAC prompt fail closed instead of registering a different definition. Cleanup runs on every exit -- success, UAC cancellation, a synchronous spawn failure, a failed digest check, and a partial staging failure -- and a cleanup error is aggregated with the registration error rather than replacing it. The original "immutable bytes, never a caller-writable pathname" goal is kept by different means rather than abandoned, and the replacement precondition is untouched: the elevated process still re-queries the live registration and compares it to the verified predecessor before passing -Force. Payloads are UTF-16LE with no BOM and are decoded straight into Register-ScheduledTask, so what is hashed is exactly what is registered, with no trimming step the two sides could disagree about. Closes #4692 --- src/lib/windows-elevation.ts | 71 +++++-- src/service.ts | 2 +- src/service/windows-ops.ts | 199 ++++++++++++++++-- tests/service/service.test.ts | 114 ++++++++++ tests/windows/windows-elevation-spawn.test.ts | 62 +++++- 5 files changed, 410 insertions(+), 38 deletions(-) diff --git a/src/lib/windows-elevation.ts b/src/lib/windows-elevation.ts index b2d02b8123..aa728ab159 100644 --- a/src/lib/windows-elevation.ts +++ b/src/lib/windows-elevation.ts @@ -645,36 +645,79 @@ export function runWindowsElevated(file: string, args: string[]): Promise { - if (replace && !expectedExistingXml?.trim()) { + if (replace && !expectedExisting) { throw new Error("Elevated Task Scheduler replacement requires a captured existing definition."); } - const xmlBase64 = Buffer.from(xml, "utf16le").toString("base64"); - const expectedExistingBase64 = expectedExistingXml === undefined - ? null - : Buffer.from(expectedExistingXml, "utf16le").toString("base64"); const powerShellPath = windowsPowerShell(); const powerShellDirectory = powerShellPath.replace(/[\\/][^\\/]+$/, ""); const scheduledTasksModule = `${powerShellDirectory}\\Modules\\ScheduledTasks\\ScheduledTasks.psd1`; const inner = [ `$taskName = ${psSingleQuote(taskName)}`, - `$xmlBase64 = ${psSingleQuote(xmlBase64)}`, - "$xml = [Text.Encoding]::Unicode.GetString([Convert]::FromBase64String($xmlBase64))", + READ_STAGED_TASK_XML, + `$xml = Read-OcxStagedTaskXml ${psSingleQuote(xml.path)} ${psSingleQuote(xml.sha256)}`, `$module = Microsoft.PowerShell.Core\\Import-Module -Name ${psSingleQuote(scheduledTasksModule)} -PassThru -Force -ErrorAction Stop`, "$registerTask = $module.ExportedCommands['Register-ScheduledTask']", "if ($null -eq $registerTask) { throw 'Trusted ScheduledTasks module does not export Register-ScheduledTask.' }", ...(replace ? [ - `$expectedBase64 = ${psSingleQuote(expectedExistingBase64!)}`, - "$expectedXml = [Text.Encoding]::Unicode.GetString([Convert]::FromBase64String($expectedBase64))", + `$expectedXml = Read-OcxStagedTaskXml ${psSingleQuote(expectedExisting!.path)} ${psSingleQuote(expectedExisting!.sha256)}`, `$schtasks = ${psSingleQuote(resolveTrustedWindowsSchtasksExe())}`, "$currentXml = & $schtasks /query /tn $taskName /xml 2>$null | Out-String", "if ($LASTEXITCODE -ne 0) { throw 'Task Scheduler replacement precondition could not be read.' }", diff --git a/src/service.ts b/src/service.ts index 92cedd7e22..f6a571b574 100644 --- a/src/service.ts +++ b/src/service.ts @@ -19,7 +19,7 @@ export { decodeSchtasksOutput, setQuerySchtasksForTests, formatWindowsSchedulerS export type { WindowsSchedulerXmlState } from "./service/windows-taskxml"; export { buildWindowsServiceScript, buildWindowsSchtasksCreateArgs, buildWindowsSchtasksCreateArgsForXml, buildWindowsLauncherVbs, buildWindowsTaskXml, buildWindowsTaskXmlDocument, windowsTaskRegistrationOwnedByAttempt, windowsTaskRegistrationHealthy, readWindowsSchedulerXmlState } from "./service/windows-taskxml"; export type { WindowsSchedulerRegistrationStageDeps, FreshWindowsSchedulerRegistrationDeps, RemoveNativeWindowsServiceDeps } from "./service/windows-ops"; -export { windowsListenPort, winswListenPort, writeServiceDefinitionFile, definitionCarriesCredential, stageWindowsSchedulerRegistrationXml, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, assertWindowsNativeServiceAccountSupported, isWindowsSchedulerEndBenign, stopWindows, stopWindowsChecked, classifyWindowsServiceStop } from "./service/windows-ops"; +export { windowsListenPort, winswListenPort, writeServiceDefinitionFile, definitionCarriesCredential, stageWindowsSchedulerRegistrationXml, stageElevatedSchedulerRegistration, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, assertWindowsNativeServiceAccountSupported, isWindowsSchedulerEndBenign, stopWindows, stopWindowsChecked, classifyWindowsServiceStop } from "./service/windows-ops"; export type { ServiceRepairVerb, RepairServiceDeps } from "./service/repair"; export { repairService } from "./service/repair"; export type { ServiceInstallPreparationDeps, FreshWindowsSchedulerInstallDeps, ServiceStopOutcome, ServiceUninstallOutcome } from "./service/orchestration"; diff --git a/src/service/windows-ops.ts b/src/service/windows-ops.ts index 64e3d48373..75321fb9fc 100644 --- a/src/service/windows-ops.ts +++ b/src/service/windows-ops.ts @@ -1,4 +1,5 @@ -import { chmodSync, readFileSync, writeFileSync } from "node:fs"; +import { chmodSync, lstatSync, readFileSync, writeFileSync } from "node:fs"; +import { createHash } from "node:crypto"; import { win32 } from "node:path"; import { winswXmlPath } from "../lib/winsw"; import { hardenSecretPath } from "../lib/windows-secret-acl"; @@ -9,7 +10,7 @@ import { existsSync, mkdirSync, mkdtempSync, rmdirSync, unlinkSync } from "node: import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { getConfigDir } from "../config"; -import { runWindowsElevatedScheduledTaskRegistration, WindowsSchtasksError } from "../lib/windows-elevation"; +import { runWindowsElevatedScheduledTaskRegistration, WindowsSchtasksError, type StagedWindowsTaskXml } from "../lib/windows-elevation"; import { defaultWinswEntry, installWinswService, statusWinswRaw, uninstallWinswService, WINSW_SERVICE_ID, type WinswStatus } from "../lib/winsw"; import { forgetEphemeralSecretDir, forgetEphemeralSecretPath, hardenSecretDir } from "../lib/windows-secret-acl"; import { recordOwnedConfigPath } from "../lib/config-ownership"; @@ -165,6 +166,170 @@ function cleanupWindowsSchedulerStage( if (cleanupError) throw cleanupError; } +/** A staged payload set for one elevated registration, plus the way to remove it. */ +export interface StagedElevatedSchedulerRegistration { + readonly xml: StagedWindowsTaskXml; + readonly expectedExisting?: StagedWindowsTaskXml; + /** Remove every staged artifact. Idempotent, so a second call after success is a no-op. */ + cleanup(): void; +} + +export interface ElevatedSchedulerStagingDeps { + createStageDir?: () => string; + hardenDir?: (path: string) => void; + writePayload?: (path: string, bytes: Buffer) => void; + hardenPath?: (path: string) => void; + inspect?: (path: string) => { isSymbolicLink(): boolean; isFile(): boolean; isDirectory(): boolean }; + removeStageDir?: (path: string) => void; +} + +/** + * Stage the captured definitions an elevated registration needs, as files rather than + * as command-line payloads (#4692). + * + * A file that an administrator process will read is itself a privilege-escalation + * surface, so three properties have to hold together and none of them is sufficient + * alone: + * + * - **Access.** The directory is created fresh by `mkdtemp`, then ACL-hardened before + * anything is written into it, so another local account cannot read or replace the + * payload while the UAC prompt is open. Hardening the directory first is what makes + * the file private from the moment it exists. + * - **No reparse point.** Each artifact is inspected with `lstat` and rejected unless it + * is what it claims to be. `wx` already refuses to create over an existing name, which + * is the atomic step here — there is no replace path to race, because every path is + * inside a directory that did not exist a moment ago. The explicit check is what keeps + * that guarantee from depending on a reading of `O_EXCL` semantics. + * - **Tamper evidence.** The digest is taken over the exact bytes written, and the + * elevated script recomputes it over the bytes it reads. An ACL cannot cover this: + * a process running as the same user has the same SID and can rewrite the file, so + * the digest is the only thing that makes such a swap fail closed rather than + * silently register a different task definition. + * + * Payloads are UTF-16LE with no BOM, and the elevated process decodes them straight into + * `Register-ScheduledTask`. What is hashed is therefore exactly what is registered, with + * no trimming step in between that the two sides could disagree about. + */ +export function stageElevatedSchedulerRegistration( + xml: string, + expectedExistingXml?: string, + deps: ElevatedSchedulerStagingDeps = {}, +): StagedElevatedSchedulerRegistration { + const createStageDir = deps.createStageDir + ?? (() => mkdtempSync(join(tmpdir(), WINDOWS_SCHEDULER_STAGE_PREFIX))); + const hardenDir = deps.hardenDir ?? ((path: string) => { hardenSecretDir(path, { required: true }); }); + const writePayload = deps.writePayload ?? ((path: string, bytes: Buffer) => { + writeFileSync(path, bytes, { flag: "wx", mode: 0o600 }); + }); + const hardenPath = deps.hardenPath ?? ((path: string) => { hardenSecretPath(path, { required: true }); }); + const inspect = deps.inspect ?? ((path: string) => lstatSync(path)); + const removeStageDir = deps.removeStageDir ?? ((path: string) => { rmdirSync(path); }); + + const stageDir = createStageDir(); + const files: string[] = []; + const cleanup = (): void => { + let failure: unknown; + for (const file of files.splice(0)) { + try { + unlinkSync(file); + forgetEphemeralSecretPath(file); + } catch (error) { + if ((error as NodeJS.ErrnoException | undefined)?.code === "ENOENT") forgetEphemeralSecretPath(file); + else failure ??= error; + } + } + try { + removeStageDir(stageDir); + forgetEphemeralSecretDir(stageDir); + } catch (error) { + if ((error as NodeJS.ErrnoException | undefined)?.code === "ENOENT") forgetEphemeralSecretDir(stageDir); + else if (failure) throw new AggregateError([failure, error], "Elevated Task Scheduler staging cleanup failed."); + else failure = error; + } + if (failure) throw failure; + }; + + try { + try { chmodSync(stageDir, 0o700); } catch { /* required Windows ACL is authoritative */ } + const dirStats = inspect(stageDir); + if (dirStats.isSymbolicLink() || !dirStats.isDirectory()) { + throw new Error(`Refusing to stage an elevated Task Scheduler payload under a redirected path: ${stageDir}`); + } + hardenDir(stageDir); + const stage = (name: string, value: string): StagedWindowsTaskXml => { + const path = join(stageDir, name); + const bytes = Buffer.from(value, "utf16le"); + writePayload(path, bytes); + files.push(path); + const stats = inspect(path); + if (stats.isSymbolicLink() || !stats.isFile()) { + throw new Error(`Refusing to stage an elevated Task Scheduler payload through a redirected path: ${path}`); + } + hardenPath(path); + return { path, sha256: createHash("sha256").update(bytes).digest("hex") }; + }; + return { + xml: stage("register.xml", xml), + ...(expectedExistingXml === undefined + ? {} + : { expectedExisting: stage("expected.xml", expectedExistingXml) }), + cleanup, + }; + } catch (error) { + try { + cleanup(); + } catch (cleanupError) { + throw new AggregateError( + [error, cleanupError], + "Elevated Task Scheduler staging failed and could not be cleaned up.", + ); + } + throw error; + } +} + +/** + * Stage, elevate, and clean up — on every exit, including UAC cancellation and a + * synchronous spawn failure. + * + * A cleanup failure never replaces the registration failure it followed: an operator + * told only that a temp directory could not be removed would have no idea the task was + * never registered. + */ +async function runStagedElevatedSchedulerRegistration( + taskName: string, + xml: string, + replace: boolean, + expectedExistingXml: string | undefined, + failureLabel: string, +): Promise { + const staged = stageElevatedSchedulerRegistration(xml, expectedExistingXml); + let failure: unknown; + try { + const exitCode = await runWindowsElevatedScheduledTaskRegistration( + taskName, + staged.xml, + replace, + staged.expectedExisting, + ); + if (exitCode !== 0) failure = new Error(`${failureLabel} with exit code ${exitCode}.`); + } catch (error) { + failure = error; + } + try { + staged.cleanup(); + } catch (cleanupError) { + if (failure) { + throw new AggregateError( + [failure, cleanupError], + "Elevated Task Scheduler registration failed and its staging could not be cleaned up.", + ); + } + throw cleanupError; + } + if (failure) throw failure; +} + export function stageWindowsSchedulerRegistrationXml( attemptNonce: string, deps: WindowsSchedulerRegistrationStageDeps = {}, @@ -294,7 +459,8 @@ export async function registerFreshWindowsSchedulerTask( throw error; } // Register from the captured XML string inside the elevated process. Another - // same-user process can mutate its own temp files, but cannot change this command. + // same-user process can mutate its own temp files, so the captured bytes are staged + // privately and the elevated script verifies their digest before registering them. // UAC can remain open for an arbitrary amount of time. Recheck the captured predecessor // before launch; the elevated helper repeats the same check after consent and before Force. assertReplacementPrecondition(); @@ -303,15 +469,13 @@ export async function registerFreshWindowsSchedulerTask( xml: string, replaceCurrent: boolean, previousXml?: string, - ) => { - const exitCode = await runWindowsElevatedScheduledTaskRegistration( - taskName, - xml, - replaceCurrent, - previousXml, - ); - if (exitCode !== 0) throw new Error(`Background service install failed with exit code ${exitCode}.`); - }); + ) => runStagedElevatedSchedulerRegistration( + taskName, + xml, + replaceCurrent, + previousXml, + "Background service install failed", + )); await elevate(TASK, expectedXml, replace, expectedExistingXml); } @@ -501,10 +665,13 @@ export async function restoreWindowsSchedulerTaskIfAbsent(registeredXml: string) ) { throw error; } - const exitCode = await runWindowsElevatedScheduledTaskRegistration(TASK, registeredXml, false); - if (exitCode !== 0) { - throw new Error(`Task Scheduler rollback failed with exit code ${exitCode}.`); - } + await runStagedElevatedSchedulerRegistration( + TASK, + registeredXml, + false, + undefined, + "Task Scheduler rollback failed", + ); } const recoveredXml = statusWindowsXml(); if (!windowsSchedulerRegistrationMatchesSnapshot(recoveredXml, registeredXml)) { diff --git a/tests/service/service.test.ts b/tests/service/service.test.ts index 5daaa02ec9..3b17cd6e93 100644 --- a/tests/service/service.test.ts +++ b/tests/service/service.test.ts @@ -1,6 +1,7 @@ import { afterAll, afterEach, describe, expect, spyOn, test } from "bun:test"; import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, statSync, writeFileSync } from "node:fs"; import { execFileSync } from "node:child_process"; +import { createHash } from "node:crypto"; import { tmpdir } from "node:os"; import { delimiter, isAbsolute, join, posix, win32 } from "node:path"; import { pathToFileURL } from "node:url"; @@ -2078,6 +2079,119 @@ describe("service lifecycle cleanup ordering", () => { } }); + /** + * #4692: a file an administrator process will read is itself a privilege-escalation + * surface, so access, redirection and tamper-evidence each have to hold. + */ + test("elevated staging hardens before writing, digests the exact bytes, and cleans up", () => { + const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-")); + const stageDir = join(parent, "private-stage"); + const calls: string[] = []; + try { + const staged = serviceModule.stageElevatedSchedulerRegistration( + "new", + "previous", + { + createStageDir: () => { + mkdirSync(stageDir, { mode: 0o700 }); + calls.push("create-stage-dir"); + return stageDir; + }, + hardenDir: () => { calls.push("harden-dir"); }, + writePayload: (path, bytes) => { + calls.push("write:" + path.slice(stageDir.length + 1)); + writeFileSync(path, bytes, { flag: "wx" }); + }, + hardenPath: path => { calls.push("harden:" + path.slice(stageDir.length + 1)); }, + }, + ); + + // The directory is private before anything is written into it; hardening after the + // write would leave a window where the payload is readable by another account. + expect(calls).toEqual([ + "create-stage-dir", + "harden-dir", + "write:register.xml", + "harden:register.xml", + "write:expected.xml", + "harden:expected.xml", + ]); + + // The digest covers exactly the bytes on disk, and those bytes are UTF-16LE with no + // BOM: the elevated process decodes them straight into Register-ScheduledTask, so + // what is hashed here is what gets registered, with no trimming step in between. + for (const [payload, value] of [ + [staged.xml, "new"], + [staged.expectedExisting!, "previous"], + ] as const) { + const onDisk = readFileSync(payload.path); + expect(onDisk.equals(Buffer.from(value, "utf16le"))).toBe(true); + expect(onDisk[0]).not.toBe(0xff); + expect(payload.sha256).toBe(createHash("sha256").update(onDisk).digest("hex")); + expect(payload.sha256).toMatch(/^[0-9a-f]{64}$/); + } + expect(staged.xml.sha256).not.toBe(staged.expectedExisting!.sha256); + + staged.cleanup(); + expect(existsSync(stageDir)).toBe(false); + // Idempotent: the success path calls it once, but a failure path may race it. + expect(() => staged.cleanup()).not.toThrow(); + } finally { + removeTreeWithRetry(parent); + } + }); + + test("elevated staging refuses a redirected path and leaves nothing behind", () => { + const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-reparse-")); + const stageDir = join(parent, "private-stage"); + try { + // A staged payload reached through a reparse point is a payload somebody else chose + // the destination for. Exclusive creation already refuses an existing name, so this + // is the check that keeps the guarantee from resting on a reading of O_EXCL. + expect(() => serviceModule.stageElevatedSchedulerRegistration("", undefined, { + createStageDir: () => { + mkdirSync(stageDir, { mode: 0o700 }); + return stageDir; + }, + hardenDir: () => {}, + writePayload: (path, bytes) => { writeFileSync(path, bytes, { flag: "wx" }); }, + hardenPath: () => { throw new Error("must not harden a redirected payload"); }, + inspect: path => ({ + isSymbolicLink: () => path !== stageDir, + isFile: () => true, + isDirectory: () => path === stageDir, + }), + })).toThrow("redirected path"); + expect(existsSync(stageDir)).toBe(false); + } finally { + removeTreeWithRetry(parent); + } + }); + + test("elevated staging cleans up when a payload write fails partway", () => { + const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-partial-")); + const stageDir = join(parent, "private-stage"); + try { + // The predecessor is the second payload, so this leaves a real file behind unless + // cleanup walks everything it created rather than only the one that failed. + expect(() => serviceModule.stageElevatedSchedulerRegistration("", "", { + createStageDir: () => { + mkdirSync(stageDir, { mode: 0o700 }); + return stageDir; + }, + hardenDir: () => {}, + writePayload: (path, bytes) => { + if (path.endsWith("expected.xml")) throw new Error("synthetic predecessor write failure"); + writeFileSync(path, bytes, { flag: "wx" }); + }, + hardenPath: () => {}, + })).toThrow("synthetic predecessor write failure"); + expect(existsSync(stageDir)).toBe(false); + } finally { + removeTreeWithRetry(parent); + } + }); + test("UAC cancellation removes only staged XML and never enters cleanup or asset publication", async () => { const calls: string[] = []; mkdirSync(TEST_DIR, { recursive: true }); diff --git a/tests/windows/windows-elevation-spawn.test.ts b/tests/windows/windows-elevation-spawn.test.ts index 3eaaec1258..fa0058f51d 100644 --- a/tests/windows/windows-elevation-spawn.test.ts +++ b/tests/windows/windows-elevation-spawn.test.ts @@ -153,7 +153,7 @@ describe("runWindowsElevated spawn contract", () => { await expect(runWindowsElevatedScheduledTaskRegistration( "opencodex-proxy", - "", + { path: "C:\\Temp\\opencodex-service-stage-aaaaaa\\register.xml", sha256: "0".repeat(64) }, )).resolves.toBe(0); const startProcessIndex = commandScript.indexOf("Start-Process"); @@ -174,7 +174,7 @@ describe("runWindowsElevated spawn contract", () => { expect(commandScript).not.toMatch(/-ArgumentList\s+'[^']*';\s+-Verb RunAs/); }); - test("scheduled-task registration embeds immutable XML bytes instead of a file path", async () => { + test("scheduled-task registration passes staged paths and digests, never inline payloads", async () => { let commandScript = ""; setWindowsElevationSpawnForTests((( _cmd: string, @@ -196,7 +196,9 @@ describe("runWindowsElevated spawn contract", () => { }) as never); const xml = "fixed-definition"; - await expect(runWindowsElevatedScheduledTaskRegistration("opencodex-proxy", xml)).resolves.toBe(0); + const stageDir = "C:\\Temp\\opencodex-service-stage-aaaaaa"; + const staged = { path: stageDir + "\\register.xml", sha256: "a".repeat(64) }; + await expect(runWindowsElevatedScheduledTaskRegistration("opencodex-proxy", staged)).resolves.toBe(0); const match = /-EncodedCommand ([A-Za-z0-9+/=]+)/.exec(commandScript); expect(match).not.toBeNull(); const elevatedScript = Buffer.from(match![1]!, "base64").toString("utf16le"); @@ -211,21 +213,67 @@ describe("runWindowsElevated spawn contract", () => { expect(elevatedScript).toContain("& $registerTask -TaskName $taskName -Xml $xml -ErrorAction Stop"); expect(elevatedScript).not.toContain("-Xml $xml -Force"); expect(elevatedScript.match(/\bRegister-ScheduledTask\b/g)).toHaveLength(2); - expect(elevatedScript).toContain(Buffer.from(xml, "utf16le").toString("base64")); + + // #4692: the definition now travels as a path plus a digest. A pathname on its own + // would be a promise about content, so the elevated side has to check it: read the + // bytes once, hash exactly those bytes, and refuse BEFORE decoding them. Hashing and + // then rereading would leave the swap window this check exists to close. + expect(elevatedScript).toContain(staged.path); + expect(elevatedScript).toContain(staged.sha256); + expect(elevatedScript).toContain("[IO.File]::ReadAllBytes($path)"); + expect(elevatedScript).toContain("$sha.ComputeHash($bytes)"); + expect(elevatedScript).toContain("Task Scheduler staged payload failed its integrity check."); + expect(elevatedScript.indexOf("-cne $expectedHash")) + .toBeLessThan(elevatedScript.indexOf("[Text.Encoding]::Unicode.GetString($bytes)")); + // No payload rides the command line any more, in either encoding layer. + expect(elevatedScript).not.toContain(Buffer.from(xml, "utf16le").toString("base64")); + expect(elevatedScript).not.toContain("FromBase64String"); expect(commandScript).not.toContain("/xml"); - expect(commandScript).not.toContain("task.xml"); + + // The regression itself. The old form embedded base64(utf16le) of the XML inside a + // script that was base64(utf16le)-encoded again — about 14.2 command-line characters + // per XML character, twice over for a replacement — so a ~2 KB definition pushed the + // spawn past the Windows command-line limit and failed with ENAMETOOLONG. What is + // pinned here is independence, not one lucky measurement: the same staging shape must + // produce the same command length no matter how large the definition behind it is. + const smallLength = commandScript.length; + const largeStaged = { path: stageDir + "\\register.xml", sha256: "b".repeat(64) }; + await expect(runWindowsElevatedScheduledTaskRegistration("opencodex-proxy", largeStaged)).resolves.toBe(0); + expect(commandScript.length).toBe(smallLength); + expect(commandScript.length).toBeLessThan(8192); const predecessor = "captured-predecessor"; + const stagedPredecessor = { path: stageDir + "\\expected.xml", sha256: "c".repeat(64) }; await expect( - runWindowsElevatedScheduledTaskRegistration("opencodex-proxy", xml, true, predecessor), + runWindowsElevatedScheduledTaskRegistration("opencodex-proxy", staged, true, stagedPredecessor), ).resolves.toBe(0); const replaceMatch = /-EncodedCommand ([A-Za-z0-9+/=]+)/.exec(commandScript); expect(replaceMatch).not.toBeNull(); const replaceScript = Buffer.from(replaceMatch![1]!, "base64").toString("utf16le"); expect(replaceScript).toContain("& $registerTask -TaskName $taskName -Xml $xml -Force"); - expect(replaceScript).toContain(Buffer.from(predecessor, "utf16le").toString("base64")); + expect(replaceScript).toContain(stagedPredecessor.path); + expect(replaceScript).toContain(stagedPredecessor.sha256); + expect(replaceScript).not.toContain(Buffer.from(predecessor, "utf16le").toString("base64")); + // The predecessor is verified the same way before it is used as a precondition: two + // call sites, both digest-checked. The helper is declared as + // "Read-OcxStagedTaskXml([string]$path", so the trailing space matches calls only. + expect(replaceScript.match(/Read-OcxStagedTaskXml /g)).toHaveLength(2); + expect(elevatedScript.match(/Read-OcxStagedTaskXml /g)).toHaveLength(1); expect(replaceScript).toContain("$currentXml = & $schtasks /query /tn $taskName /xml"); expect(replaceScript).toContain("Task Scheduler replacement precondition changed."); + // A replacement used to carry TWO payloads, which is what made this the reported + // failure. It stays bounded now. + expect(commandScript.length).toBeLessThan(8192); + }); + + test("an elevated replacement still refuses without a captured predecessor", () => { + // The post-UAC compare-before-Force is the only thing standing between a repair and + // overwriting a registration somebody else changed while the prompt was open. + expect(() => runWindowsElevatedScheduledTaskRegistration( + "opencodex-proxy", + { path: "C:\\Temp\\opencodex-service-stage-aaaaaa\\register.xml", sha256: "a".repeat(64) }, + true, + )).toThrow("requires a captured existing definition"); }); test("maps exit 1223 to cancelled", async () => { From f88191d5316ef252f6e1df7e1d9a2089e9aa7b65 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 11:37:16 +0900 Subject: [PATCH 2/3] fix(service): report an unreadable staged payload with its cause (#4692) Staging the elevated Task Scheduler XML introduces exactly one new failure of its own: hardenSecretPath grants the staging account and strips inheritance, so a split-token elevation of the same user reads the file while an elevation answered with a DIFFERENT administrator's credentials does not. The inline form had no such dependency. The elevated process runs hidden, so nothing it writes survives and only the exit code crosses back. That made the failure an unexplained non-zero status -- the same undiagnosable shape as the ENAMETOOLONG this change set removes. The read failure now has its own protocol code, and the parent turns it into a message that names both the cause and the way out: approve the prompt as the signed-in user, or run again from a session already elevated as that user. The code sits outside OCX_ELEVATED_PROTOCOL_CODES, which is the create-and-run transaction's alphabet, and cannot collide with UAC cancellation. Whether to widen the ACL to SYSTEM and Administrators is left as a separate security decision rather than bundled here, because it changes a security-sensitive module. --- src/lib/windows-elevation.ts | 21 ++++++++++++- src/service.ts | 2 +- src/service/windows-ops.ts | 31 +++++++++++++++++-- tests/service/service.test.ts | 25 +++++++++++++++ tests/windows/windows-elevation-spawn.test.ts | 10 ++++++ 5 files changed, 85 insertions(+), 4 deletions(-) diff --git a/src/lib/windows-elevation.ts b/src/lib/windows-elevation.ts index aa728ab159..171545276c 100644 --- a/src/lib/windows-elevation.ts +++ b/src/lib/windows-elevation.ts @@ -250,6 +250,21 @@ export const OCX_ELEVATED_PROTOCOL_FAILED = 13; /** Windows ERROR_CANCELLED — reserved for UAC denial; never emitted by the elevated script. */ export const OCX_ELEVATED_UAC_CANCELLED = 1223; +/** + * The elevated process could not read a staged payload (#4692). + * + * `hardenSecretPath` grants the staging account and strips inheritance, so a split-token + * elevation of the same user reads the file and an elevation answered with a DIFFERENT + * administrator's credentials does not. The elevated side cannot explain that itself: it + * runs hidden, so its stderr goes nowhere and only the exit code survives the boundary. + * Without a code of its own the operator would be told "exit code 1" for a cause that + * names its own remedy — the same undiagnosable failure this change set exists to remove. + * + * Deliberately outside OCX_ELEVATED_PROTOCOL_CODES: that list is the create-and-run + * transaction's alphabet, and this code belongs to the registration path. + */ +export const OCX_ELEVATED_STAGING_UNREADABLE = 14; + export const OCX_ELEVATED_PROTOCOL_CODES = [ OCX_ELEVATED_SUCCESS, OCX_ELEVATED_CREATE_FAILED, @@ -667,7 +682,11 @@ export interface StagedWindowsTaskXml { * exists to close. */ const READ_STAGED_TASK_XML = "function Read-OcxStagedTaskXml([string]$path, [string]$expectedHash) {" - + " $bytes = [IO.File]::ReadAllBytes($path);" + // An unreadable payload is a diagnosable condition, not a generic throw: a hidden + // elevated process has nowhere to print, so the cause has to ride the exit code. + + " try { $bytes = [IO.File]::ReadAllBytes($path) }" + + " catch [System.UnauthorizedAccessException] { exit " + OCX_ELEVATED_STAGING_UNREADABLE + " }" + + " catch [System.Security.SecurityException] { exit " + OCX_ELEVATED_STAGING_UNREADABLE + " };" + " $sha = [Security.Cryptography.SHA256]::Create();" + " try { $actual = [BitConverter]::ToString($sha.ComputeHash($bytes)).Replace('-', '').ToLowerInvariant() } finally { $sha.Dispose() };" + " if ($actual -cne $expectedHash) { throw 'Task Scheduler staged payload failed its integrity check.' };" diff --git a/src/service.ts b/src/service.ts index f6a571b574..149b1ae02c 100644 --- a/src/service.ts +++ b/src/service.ts @@ -19,7 +19,7 @@ export { decodeSchtasksOutput, setQuerySchtasksForTests, formatWindowsSchedulerS export type { WindowsSchedulerXmlState } from "./service/windows-taskxml"; export { buildWindowsServiceScript, buildWindowsSchtasksCreateArgs, buildWindowsSchtasksCreateArgsForXml, buildWindowsLauncherVbs, buildWindowsTaskXml, buildWindowsTaskXmlDocument, windowsTaskRegistrationOwnedByAttempt, windowsTaskRegistrationHealthy, readWindowsSchedulerXmlState } from "./service/windows-taskxml"; export type { WindowsSchedulerRegistrationStageDeps, FreshWindowsSchedulerRegistrationDeps, RemoveNativeWindowsServiceDeps } from "./service/windows-ops"; -export { windowsListenPort, winswListenPort, writeServiceDefinitionFile, definitionCarriesCredential, stageWindowsSchedulerRegistrationXml, stageElevatedSchedulerRegistration, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, assertWindowsNativeServiceAccountSupported, isWindowsSchedulerEndBenign, stopWindows, stopWindowsChecked, classifyWindowsServiceStop } from "./service/windows-ops"; +export { windowsListenPort, winswListenPort, writeServiceDefinitionFile, definitionCarriesCredential, stageWindowsSchedulerRegistrationXml, stageElevatedSchedulerRegistration, describeElevatedRegistrationFailure, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, assertWindowsNativeServiceAccountSupported, isWindowsSchedulerEndBenign, stopWindows, stopWindowsChecked, classifyWindowsServiceStop } from "./service/windows-ops"; export type { ServiceRepairVerb, RepairServiceDeps } from "./service/repair"; export { repairService } from "./service/repair"; export type { ServiceInstallPreparationDeps, FreshWindowsSchedulerInstallDeps, ServiceStopOutcome, ServiceUninstallOutcome } from "./service/orchestration"; diff --git a/src/service/windows-ops.ts b/src/service/windows-ops.ts index 75321fb9fc..9206d6a8f9 100644 --- a/src/service/windows-ops.ts +++ b/src/service/windows-ops.ts @@ -10,7 +10,7 @@ import { existsSync, mkdirSync, mkdtempSync, rmdirSync, unlinkSync } from "node: import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { getConfigDir } from "../config"; -import { runWindowsElevatedScheduledTaskRegistration, WindowsSchtasksError, type StagedWindowsTaskXml } from "../lib/windows-elevation"; +import { OCX_ELEVATED_STAGING_UNREADABLE, runWindowsElevatedScheduledTaskRegistration, WindowsSchtasksError, type StagedWindowsTaskXml } from "../lib/windows-elevation"; import { defaultWinswEntry, installWinswService, statusWinswRaw, uninstallWinswService, WINSW_SERVICE_ID, type WinswStatus } from "../lib/winsw"; import { forgetEphemeralSecretDir, forgetEphemeralSecretPath, hardenSecretDir } from "../lib/windows-secret-acl"; import { recordOwnedConfigPath } from "../lib/config-ownership"; @@ -288,6 +288,31 @@ export function stageElevatedSchedulerRegistration( } } +/** + * Turn an elevated registration exit code into something an operator can act on. + * + * The elevated process runs hidden, so nothing it writes survives; only the exit code + * crosses back. That makes an unexplained code the whole user-facing error, which is + * exactly what made the ENAMETOOLONG in #4692 expensive to diagnose. Staging introduces + * one new failure of its own — the payload is readable only by the account that created + * it, so an elevation answered with a different administrator's credentials cannot open + * it — and that one gets named along with its remedy rather than surfacing as a number. + */ +export function describeElevatedRegistrationFailure( + failureLabel: string, + exitCode: number, + stageDir: string, +): string { + if (exitCode === OCX_ELEVATED_STAGING_UNREADABLE) { + return `${failureLabel}: the elevated process could not read the staged task definition in ` + + `${stageDir}. That directory is readable only by the account that staged it, so this ` + + "happens when the UAC prompt was answered with a different administrator account. " + + "Approve the prompt as the signed-in user, or run the command again from a session " + + "already elevated as that user."; + } + return `${failureLabel} with exit code ${exitCode}.`; +} + /** * Stage, elevate, and clean up — on every exit, including UAC cancellation and a * synchronous spawn failure. @@ -312,7 +337,9 @@ async function runStagedElevatedSchedulerRegistration( replace, staged.expectedExisting, ); - if (exitCode !== 0) failure = new Error(`${failureLabel} with exit code ${exitCode}.`); + if (exitCode !== 0) { + failure = new Error(describeElevatedRegistrationFailure(failureLabel, exitCode, dirname(staged.xml.path))); + } } catch (error) { failure = error; } diff --git a/tests/service/service.test.ts b/tests/service/service.test.ts index 3b17cd6e93..e3870a5939 100644 --- a/tests/service/service.test.ts +++ b/tests/service/service.test.ts @@ -15,6 +15,7 @@ import { buildWinswXml } from "../../src/lib/winsw"; import { CONFIG_OWNER_FILE, CONFIG_UNINSTALL_MANIFEST, recordOwnedConfigPath, removeOwnedConfigState } from "../../src/lib/config-ownership"; import { serviceApiTokenFilePath } from "../../src/lib/service-secrets"; import { WindowsSchtasksError } from "../../src/lib/windows-elevation"; +import { OCX_ELEVATED_STAGING_UNREADABLE } from "../../src/lib/windows-elevation"; import { resolveCurrentWindowsPrincipal, setWindowsPrincipalRunnerForTests } from "../../src/lib/windows-user-principal"; import { setAsyncIcaclsRunnerForTests, setIcaclsRunnerForTests } from "../../src/lib/windows-secret-acl"; import type { OcxConfig } from "../../src/types"; @@ -2192,6 +2193,30 @@ describe("service lifecycle cleanup ordering", () => { } }); + test("an unreadable staged payload is reported with its cause and its remedy", () => { + // The elevated process runs hidden, so nothing it writes survives and the exit code is + // the entire user-facing error. Staging adds exactly one new failure -- the payload is + // readable only by the account that created it, so an elevation answered with another + // administrator's credentials cannot open it -- and reporting that as a bare number + // would reproduce what made #4692 expensive to diagnose in the first place. + const message = serviceModule.describeElevatedRegistrationFailure( + "Background service install failed", + OCX_ELEVATED_STAGING_UNREADABLE, + "C:\\Temp\\opencodex-service-stage-aaaaaa", + ); + expect(message).toContain("could not read the staged task definition"); + expect(message).toContain("C:\\Temp\\opencodex-service-stage-aaaaaa"); + expect(message).toContain("different administrator account"); + expect(message).toContain("Approve the prompt as the signed-in user"); + expect(message).not.toMatch(/exit code \d+/); + + // Every other code keeps the plain form; this is a named cause, not a catch-all. + for (const code of [1, 10, 13, 1223]) { + expect(serviceModule.describeElevatedRegistrationFailure("Task Scheduler rollback failed", code, "C:\\Temp\\x")) + .toBe("Task Scheduler rollback failed with exit code " + code + "."); + } + }); + test("UAC cancellation removes only staged XML and never enters cleanup or asset publication", async () => { const calls: string[] = []; mkdirSync(TEST_DIR, { recursive: true }); diff --git a/tests/windows/windows-elevation-spawn.test.ts b/tests/windows/windows-elevation-spawn.test.ts index fa0058f51d..4c905a49af 100644 --- a/tests/windows/windows-elevation-spawn.test.ts +++ b/tests/windows/windows-elevation-spawn.test.ts @@ -6,6 +6,7 @@ import { OCX_ELEVATED_PROTOCOL_FAILED, OCX_ELEVATED_RUN_FAILED_ROLLBACK_FAILED, OCX_ELEVATED_RUN_FAILED_ROLLED_BACK, + OCX_ELEVATED_STAGING_UNREADABLE, OCX_ELEVATED_SUCCESS, OCX_ELEVATED_UAC_CANCELLED, WindowsElevationError, @@ -223,6 +224,15 @@ describe("runWindowsElevated spawn contract", () => { expect(elevatedScript).toContain("[IO.File]::ReadAllBytes($path)"); expect(elevatedScript).toContain("$sha.ComputeHash($bytes)"); expect(elevatedScript).toContain("Task Scheduler staged payload failed its integrity check."); + // #4692 follow-up: the one failure this staging design introduces has to be readable. + // A hidden elevated process has nowhere to print, so an unreadable payload rides its + // own exit code instead of collapsing into a generic non-zero status. + expect(elevatedScript).toContain("catch [System.UnauthorizedAccessException] { exit " + OCX_ELEVATED_STAGING_UNREADABLE + " }"); + expect(elevatedScript).toContain("catch [System.Security.SecurityException] { exit " + OCX_ELEVATED_STAGING_UNREADABLE + " }"); + // It is not part of the create-and-run transaction's alphabet, and cannot be mistaken + // for UAC denial. + expect(OCX_ELEVATED_PROTOCOL_CODES).not.toContain(OCX_ELEVATED_STAGING_UNREADABLE); + expect(OCX_ELEVATED_STAGING_UNREADABLE).not.toBe(OCX_ELEVATED_UAC_CANCELLED); expect(elevatedScript.indexOf("-cne $expectedHash")) .toBeLessThan(elevatedScript.indexOf("[Text.Encoding]::Unicode.GetString($bytes)")); // No payload rides the command line any more, in either encoding layer. From 6a2b148f5ab4cc762573317c18b15d924273e6d3 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 12:14:45 +0900 Subject: [PATCH 3/3] test(service): move elevated-staging cover out of the ratcheted service suite (#4692) The file-size ratchet failed: tests/service/service.test.ts has a committed cap of 4106 lines and the new staging cover pushed it to 4245. The ratchet only ever lowers baselines, so growing past a cap is the thing it exists to refuse, not something to re-baseline around. The cover moves to tests/windows/windows-elevation-spawn.test.ts, which is the better home anyway: its subject is the elevated registration payload, which is exactly what these tests exercise. That file has no cap and stays well under the 2000-line threshold, and the service suite returns to its baseline unchanged, so no new test file and no test-layout registration are needed. Also replaces a logical-assignment shorthand in the staging cleanup with the explicit form the surrounding code already uses. No behaviour change; folded in here rather than spending a separate CI cycle on it. --- src/service/windows-ops.ts | 2 +- tests/service/service.test.ts | 139 ----------------- tests/windows/windows-elevation-spawn.test.ts | 146 ++++++++++++++++++ 3 files changed, 147 insertions(+), 140 deletions(-) diff --git a/src/service/windows-ops.ts b/src/service/windows-ops.ts index 9206d6a8f9..703e942061 100644 --- a/src/service/windows-ops.ts +++ b/src/service/windows-ops.ts @@ -235,7 +235,7 @@ export function stageElevatedSchedulerRegistration( forgetEphemeralSecretPath(file); } catch (error) { if ((error as NodeJS.ErrnoException | undefined)?.code === "ENOENT") forgetEphemeralSecretPath(file); - else failure ??= error; + else if (failure === undefined) failure = error; } } try { diff --git a/tests/service/service.test.ts b/tests/service/service.test.ts index e3870a5939..5daaa02ec9 100644 --- a/tests/service/service.test.ts +++ b/tests/service/service.test.ts @@ -1,7 +1,6 @@ import { afterAll, afterEach, describe, expect, spyOn, test } from "bun:test"; import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, statSync, writeFileSync } from "node:fs"; import { execFileSync } from "node:child_process"; -import { createHash } from "node:crypto"; import { tmpdir } from "node:os"; import { delimiter, isAbsolute, join, posix, win32 } from "node:path"; import { pathToFileURL } from "node:url"; @@ -15,7 +14,6 @@ import { buildWinswXml } from "../../src/lib/winsw"; import { CONFIG_OWNER_FILE, CONFIG_UNINSTALL_MANIFEST, recordOwnedConfigPath, removeOwnedConfigState } from "../../src/lib/config-ownership"; import { serviceApiTokenFilePath } from "../../src/lib/service-secrets"; import { WindowsSchtasksError } from "../../src/lib/windows-elevation"; -import { OCX_ELEVATED_STAGING_UNREADABLE } from "../../src/lib/windows-elevation"; import { resolveCurrentWindowsPrincipal, setWindowsPrincipalRunnerForTests } from "../../src/lib/windows-user-principal"; import { setAsyncIcaclsRunnerForTests, setIcaclsRunnerForTests } from "../../src/lib/windows-secret-acl"; import type { OcxConfig } from "../../src/types"; @@ -2080,143 +2078,6 @@ describe("service lifecycle cleanup ordering", () => { } }); - /** - * #4692: a file an administrator process will read is itself a privilege-escalation - * surface, so access, redirection and tamper-evidence each have to hold. - */ - test("elevated staging hardens before writing, digests the exact bytes, and cleans up", () => { - const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-")); - const stageDir = join(parent, "private-stage"); - const calls: string[] = []; - try { - const staged = serviceModule.stageElevatedSchedulerRegistration( - "new", - "previous", - { - createStageDir: () => { - mkdirSync(stageDir, { mode: 0o700 }); - calls.push("create-stage-dir"); - return stageDir; - }, - hardenDir: () => { calls.push("harden-dir"); }, - writePayload: (path, bytes) => { - calls.push("write:" + path.slice(stageDir.length + 1)); - writeFileSync(path, bytes, { flag: "wx" }); - }, - hardenPath: path => { calls.push("harden:" + path.slice(stageDir.length + 1)); }, - }, - ); - - // The directory is private before anything is written into it; hardening after the - // write would leave a window where the payload is readable by another account. - expect(calls).toEqual([ - "create-stage-dir", - "harden-dir", - "write:register.xml", - "harden:register.xml", - "write:expected.xml", - "harden:expected.xml", - ]); - - // The digest covers exactly the bytes on disk, and those bytes are UTF-16LE with no - // BOM: the elevated process decodes them straight into Register-ScheduledTask, so - // what is hashed here is what gets registered, with no trimming step in between. - for (const [payload, value] of [ - [staged.xml, "new"], - [staged.expectedExisting!, "previous"], - ] as const) { - const onDisk = readFileSync(payload.path); - expect(onDisk.equals(Buffer.from(value, "utf16le"))).toBe(true); - expect(onDisk[0]).not.toBe(0xff); - expect(payload.sha256).toBe(createHash("sha256").update(onDisk).digest("hex")); - expect(payload.sha256).toMatch(/^[0-9a-f]{64}$/); - } - expect(staged.xml.sha256).not.toBe(staged.expectedExisting!.sha256); - - staged.cleanup(); - expect(existsSync(stageDir)).toBe(false); - // Idempotent: the success path calls it once, but a failure path may race it. - expect(() => staged.cleanup()).not.toThrow(); - } finally { - removeTreeWithRetry(parent); - } - }); - - test("elevated staging refuses a redirected path and leaves nothing behind", () => { - const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-reparse-")); - const stageDir = join(parent, "private-stage"); - try { - // A staged payload reached through a reparse point is a payload somebody else chose - // the destination for. Exclusive creation already refuses an existing name, so this - // is the check that keeps the guarantee from resting on a reading of O_EXCL. - expect(() => serviceModule.stageElevatedSchedulerRegistration("", undefined, { - createStageDir: () => { - mkdirSync(stageDir, { mode: 0o700 }); - return stageDir; - }, - hardenDir: () => {}, - writePayload: (path, bytes) => { writeFileSync(path, bytes, { flag: "wx" }); }, - hardenPath: () => { throw new Error("must not harden a redirected payload"); }, - inspect: path => ({ - isSymbolicLink: () => path !== stageDir, - isFile: () => true, - isDirectory: () => path === stageDir, - }), - })).toThrow("redirected path"); - expect(existsSync(stageDir)).toBe(false); - } finally { - removeTreeWithRetry(parent); - } - }); - - test("elevated staging cleans up when a payload write fails partway", () => { - const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-partial-")); - const stageDir = join(parent, "private-stage"); - try { - // The predecessor is the second payload, so this leaves a real file behind unless - // cleanup walks everything it created rather than only the one that failed. - expect(() => serviceModule.stageElevatedSchedulerRegistration("", "", { - createStageDir: () => { - mkdirSync(stageDir, { mode: 0o700 }); - return stageDir; - }, - hardenDir: () => {}, - writePayload: (path, bytes) => { - if (path.endsWith("expected.xml")) throw new Error("synthetic predecessor write failure"); - writeFileSync(path, bytes, { flag: "wx" }); - }, - hardenPath: () => {}, - })).toThrow("synthetic predecessor write failure"); - expect(existsSync(stageDir)).toBe(false); - } finally { - removeTreeWithRetry(parent); - } - }); - - test("an unreadable staged payload is reported with its cause and its remedy", () => { - // The elevated process runs hidden, so nothing it writes survives and the exit code is - // the entire user-facing error. Staging adds exactly one new failure -- the payload is - // readable only by the account that created it, so an elevation answered with another - // administrator's credentials cannot open it -- and reporting that as a bare number - // would reproduce what made #4692 expensive to diagnose in the first place. - const message = serviceModule.describeElevatedRegistrationFailure( - "Background service install failed", - OCX_ELEVATED_STAGING_UNREADABLE, - "C:\\Temp\\opencodex-service-stage-aaaaaa", - ); - expect(message).toContain("could not read the staged task definition"); - expect(message).toContain("C:\\Temp\\opencodex-service-stage-aaaaaa"); - expect(message).toContain("different administrator account"); - expect(message).toContain("Approve the prompt as the signed-in user"); - expect(message).not.toMatch(/exit code \d+/); - - // Every other code keeps the plain form; this is a named cause, not a catch-all. - for (const code of [1, 10, 13, 1223]) { - expect(serviceModule.describeElevatedRegistrationFailure("Task Scheduler rollback failed", code, "C:\\Temp\\x")) - .toBe("Task Scheduler rollback failed with exit code " + code + "."); - } - }); - test("UAC cancellation removes only staged XML and never enters cleanup or asset publication", async () => { const calls: string[] = []; mkdirSync(TEST_DIR, { recursive: true }); diff --git a/tests/windows/windows-elevation-spawn.test.ts b/tests/windows/windows-elevation-spawn.test.ts index 4c905a49af..d2550a44bc 100644 --- a/tests/windows/windows-elevation-spawn.test.ts +++ b/tests/windows/windows-elevation-spawn.test.ts @@ -1,5 +1,9 @@ import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; import { EventEmitter } from "node:events"; +import { createHash } from "node:crypto"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { OCX_ELEVATED_CREATE_FAILED, OCX_ELEVATED_PROTOCOL_CODES, @@ -26,8 +30,150 @@ import { finalizeWindowsSchedulerServiceRegistration, schedulerVerificationMaySettle, setFinalizeWindowsSchedulerHooksForTests, + stageElevatedSchedulerRegistration, + describeElevatedRegistrationFailure, } from "../../src/service"; import type { WindowsSchedulerInstallVerification } from "../../src/service"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; + +/** + * #4692: a file an administrator process will read is itself a privilege-escalation + * surface, so access, redirection and tamper-evidence each have to hold. + */ +describe("elevated Task Scheduler payload staging", () => { + test("hardens before writing, digests the exact bytes, and cleans up", () => { + const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-")); + const stageDir = join(parent, "private-stage"); + const calls: string[] = []; + try { + const staged = stageElevatedSchedulerRegistration( + "new", + "previous", + { + createStageDir: () => { + mkdirSync(stageDir, { mode: 0o700 }); + calls.push("create-stage-dir"); + return stageDir; + }, + hardenDir: () => { calls.push("harden-dir"); }, + writePayload: (path, bytes) => { + calls.push("write:" + path.slice(stageDir.length + 1)); + writeFileSync(path, bytes, { flag: "wx" }); + }, + hardenPath: path => { calls.push("harden:" + path.slice(stageDir.length + 1)); }, + }, + ); + + // The directory is private before anything is written into it; hardening after the + // write would leave a window where the payload is readable by another account. + expect(calls).toEqual([ + "create-stage-dir", + "harden-dir", + "write:register.xml", + "harden:register.xml", + "write:expected.xml", + "harden:expected.xml", + ]); + + // The digest covers exactly the bytes on disk, and those bytes are UTF-16LE with no + // BOM: the elevated process decodes them straight into Register-ScheduledTask, so + // what is hashed here is what gets registered, with no trimming step in between. + for (const [payload, value] of [ + [staged.xml, "new"], + [staged.expectedExisting!, "previous"], + ] as const) { + const onDisk = readFileSync(payload.path); + expect(onDisk.equals(Buffer.from(value, "utf16le"))).toBe(true); + expect(onDisk[0]).not.toBe(0xff); + expect(payload.sha256).toBe(createHash("sha256").update(onDisk).digest("hex")); + expect(payload.sha256).toMatch(/^[0-9a-f]{64}$/); + } + expect(staged.xml.sha256).not.toBe(staged.expectedExisting!.sha256); + + staged.cleanup(); + expect(existsSync(stageDir)).toBe(false); + // Idempotent: the success path calls it once, but a failure path may race it. + expect(() => staged.cleanup()).not.toThrow(); + } finally { + removeTreeWithRetry(parent); + } + }); + + test("refuses a redirected path and leaves nothing behind", () => { + const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-reparse-")); + const stageDir = join(parent, "private-stage"); + try { + // A staged payload reached through a reparse point is a payload somebody else chose + // the destination for. Exclusive creation already refuses an existing name, so this + // is the check that keeps the guarantee from resting on a reading of O_EXCL. + expect(() => stageElevatedSchedulerRegistration("", undefined, { + createStageDir: () => { + mkdirSync(stageDir, { mode: 0o700 }); + return stageDir; + }, + hardenDir: () => {}, + writePayload: (path, bytes) => { writeFileSync(path, bytes, { flag: "wx" }); }, + hardenPath: () => { throw new Error("must not harden a redirected payload"); }, + inspect: path => ({ + isSymbolicLink: () => path !== stageDir, + isFile: () => true, + isDirectory: () => path === stageDir, + }), + })).toThrow("redirected path"); + expect(existsSync(stageDir)).toBe(false); + } finally { + removeTreeWithRetry(parent); + } + }); + + test("cleans up when a payload write fails partway", () => { + const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-partial-")); + const stageDir = join(parent, "private-stage"); + try { + // The predecessor is the second payload, so this leaves a real file behind unless + // cleanup walks everything it created rather than only the one that failed. + expect(() => stageElevatedSchedulerRegistration("", "", { + createStageDir: () => { + mkdirSync(stageDir, { mode: 0o700 }); + return stageDir; + }, + hardenDir: () => {}, + writePayload: (path, bytes) => { + if (path.endsWith("expected.xml")) throw new Error("synthetic predecessor write failure"); + writeFileSync(path, bytes, { flag: "wx" }); + }, + hardenPath: () => {}, + })).toThrow("synthetic predecessor write failure"); + expect(existsSync(stageDir)).toBe(false); + } finally { + removeTreeWithRetry(parent); + } + }); + + test("an unreadable staged payload is reported with its cause and its remedy", () => { + // The elevated process runs hidden, so nothing it writes survives and the exit code is + // the entire user-facing error. Staging adds exactly one new failure -- the payload is + // readable only by the account that created it, so an elevation answered with another + // administrator's credentials cannot open it -- and reporting that as a bare number + // would reproduce what made #4692 expensive to diagnose in the first place. + const message = describeElevatedRegistrationFailure( + "Background service install failed", + OCX_ELEVATED_STAGING_UNREADABLE, + "C:\\Temp\\opencodex-service-stage-aaaaaa", + ); + expect(message).toContain("could not read the staged task definition"); + expect(message).toContain("C:\\Temp\\opencodex-service-stage-aaaaaa"); + expect(message).toContain("different administrator account"); + expect(message).toContain("Approve the prompt as the signed-in user"); + expect(message).not.toMatch(/exit code \d+/); + + // Every other code keeps the plain form; this is a named cause, not a catch-all. + for (const code of [1, 10, 13, 1223]) { + expect(describeElevatedRegistrationFailure("Task Scheduler rollback failed", code, "C:\\Temp\\x")) + .toBe("Task Scheduler rollback failed with exit code " + code + "."); + } + }); +}); /** Linux CI fakes win32 without a real System32; keep elevation paths production-shaped. */ const FAKE_TRUSTED_ELEVATION_EXES = {