From 3012fe98b2c747cb97e89f3fc1fffb4ab706a6cc Mon Sep 17 00:00:00 2001 From: Haozhe Wu Date: Sat, 15 Aug 2026 03:34:54 +0800 Subject: [PATCH] fix(desktop): harden dormant update safety boundary Signed-off-by: Haozhe Wu --- desktop/electron/README.md | 7 +- desktop/electron/THREAT_MODEL.md | 21 +++- desktop/electron/UPDATE_SAFETY.md | 33 ++++- .../electron/scripts/test-update-safety.mjs | 114 +++++++++++++++++- desktop/electron/src/backend-manager.ts | 87 ++++++++----- desktop/electron/src/update-recovery.ts | 39 +++--- desktop/electron/src/update-verification.ts | 86 +++++++++++-- 7 files changed, 317 insertions(+), 70 deletions(-) diff --git a/desktop/electron/README.md b/desktop/electron/README.md index 6b2d59f7..fe4cb71f 100644 --- a/desktop/electron/README.md +++ b/desktop/electron/README.md @@ -29,7 +29,9 @@ download an artifact, launch an installer, or enable updates. See - Requests authenticated graceful shutdown before asking the watchdog to terminate the owned Windows process tree as a fallback. - Exposes a strict update-handoff shutdown call that succeeds only after the - owned backend PID, watchdog PID, and loopback listener are all gone. + owned backend PID, watchdog PID, and loopback listener are all gone. A failed + handoff retains those identifiers for retry, and a TCP probe keeps a + listening-but-unresponsive port fail-closed. - Terminates the Python process tree if the Electron main process is killed without running JavaScript shutdown handlers. - Localizes desktop-owned loading, status, error, dialog, and menu text in @@ -71,7 +73,8 @@ the parent-death path by force-terminating only the Electron main PID before checking that the Python process and loopback listener are gone. Both graceful and parent-death tests keep a separate Python sentinel alive throughout the owned-process cleanup. The update-safety test exercises the rejection matrix -and every journal recovery phase without enabling an updater. +and every journal recovery phase without enabling an updater, including +artifact mutation, concurrent journal creation, and failed-shutdown retry. Backend resolution is intentionally narrow. It checks an explicit `VIBE_TRADING_EXECUTABLE` override first, then exact application/resource diff --git a/desktop/electron/THREAT_MODEL.md b/desktop/electron/THREAT_MODEL.md index 5457f08b..a1a42d23 100644 --- a/desktop/electron/THREAT_MODEL.md +++ b/desktop/electron/THREAT_MODEL.md @@ -121,7 +121,9 @@ therefore security-significant. then independently verifies that both the Python PID and loopback listener are gone. - Graceful update handoff returns structured evidence for the exact owned - backend PID, watchdog PID, and listener. It fails closed if any remains. + backend PID, watchdog PID, and listener. The listener check probes TCP rather + than HTTP responsiveness. It fails closed if any remains and retains the + ownership state for a later cleanup retry. - Graceful and parent-death tests each start an unrelated Python sentinel and verify that it remains alive; termination commands are always PID-scoped and never use an image name such as `python.exe`. @@ -132,22 +134,31 @@ therefore security-significant. and the candidate must be strictly newer. - The artifact SHA-256 must match caller-supplied release metadata before its signature is considered. +- The production Authenticode adapter holds a read-only Windows file lock that + denies writers and deletion while it hashes and inspects the same artifact. + Its locked digest and a post-inspection digest must both match release + metadata. - Windows Authenticode status must be `Valid`; unsigned and invalid signatures are rejected with stable codes. - Publisher identity uses an explicit allowlist of SHA-256 hashes of signer certificate raw bytes. Subject display names alone are not trusted. - No publisher is currently configured. An empty allowlist is itself a policy error, so these primitives cannot silently trust the first certificate seen. -- A pending-attempt journal is written through a same-directory temporary file - and rename. It records only a simple staged `.exe` file name, not an - arbitrary deletion path. +- A pending-attempt journal is fsynced to a same-directory temporary file. Its + first publication uses an atomic hard-link create-if-absent operation, so two + concurrent begins cannot replace one another; later phase changes use the + existing same-directory rename path. It records only a simple staged `.exe` + file name, not an arbitrary deletion path. - On the next launch, an attempt that still reports the old version discards the staged candidate and requires a fresh download and verification. A successful target version clears the journal. Any third version or malformed journal is left untouched for explicit operator intervention. - The allowed phase order is `verified` -> `backend-stopped` -> `installer-launched`. A future updater must call the strict backend shutdown - path before recording `backend-stopped`. + path before recording `backend-stopped`, and must re-hash the verified + artifact immediately before installer launch. +- PowerShell host discovery and Authenticode inspection have hard timeouts; a + hung host rejects the candidate rather than blocking Electron indefinitely. ### Credential storage diff --git a/desktop/electron/UPDATE_SAFETY.md b/desktop/electron/UPDATE_SAFETY.md index 73c603e8..a4a05830 100644 --- a/desktop/electron/UPDATE_SAFETY.md +++ b/desktop/electron/UPDATE_SAFETY.md @@ -16,7 +16,10 @@ This code makes two parts of that future review executable now: `BackendManager.stopForUpdate()` is the required handoff boundary. It returns only after the exact owned backend PID, watchdog PID, and loopback listener are -gone. It never terminates `python.exe` by image name. +gone. Listener evidence uses a TCP connection probe rather than HTTP health, so +a process that accepts connections but stops answering HTTP remains open. A +failed handoff retains the exact identifiers and log stream for a later cleanup +retry. It never terminates `python.exe` by image name. ## Verification order @@ -29,8 +32,18 @@ Checks run in this order: 1. policy and release metadata are well formed; 2. the candidate semantic version is strictly newer; 3. the local file digest matches the expected SHA-256; -4. Windows reports a `Valid` Authenticode signature; -5. the SHA-256 hash of the signer certificate's raw bytes is allowlisted. +4. PowerShell opens the artifact with a lock that permits readers but denies + writers and deletion, hashes those locked bytes, and inspects Authenticode; +5. the locked digest matches the initial and expected digests; +6. Windows reports a `Valid` Authenticode signature; +7. the SHA-256 hash of the signer certificate's raw bytes is allowlisted; +8. the file digest is confirmed again after the locked inspection. + +The future launch path must call +`assertVerifiedUpdateArtifactUnchanged()` immediately before starting the +installer. Verification and PowerShell discovery are both timeout-bounded. The +pre-launch assertion is mandatory even though the artifact was already checked: +no result object makes a mutable path permanently trustworthy. The expected digest and publisher allowlist must eventually come from a trusted release configuration owned by the maintainers. That source is deliberately @@ -48,6 +61,7 @@ not invented here. | Missing publisher policy | Certificate allowlist is empty | `invalid-verification-policy` | Keep the updater disabled | | Malformed version or digest | Release metadata is not valid | `invalid-release-metadata` | Reject before signature inspection | | File or Authenticode inspection failure | Artifact cannot be read or Windows inspection fails | `artifact-inspection-failed` | Reject without launching | +| Artifact replaced during inspection | Locked Authenticode digest or post-inspection digest differs | `artifact-changed-during-verification` | Reject and require a fresh staged artifact | The unsigned automated test uses injected Authenticode observations so every branch is deterministic. The later signed end-to-end run must repeat the first @@ -78,8 +92,11 @@ application version: | Malformed journal | Unknown | Leave it untouched; require manual intervention | The journal stores a simple `.exe` file name inside its dedicated staging -directory. Malformed or traversal paths are never followed for deletion. Writes -use a temporary file and same-directory rename. +directory. Malformed or traversal paths are never followed for deletion. A +begin operation fsyncs a complete same-directory temporary file and publishes +it with an atomic hard-link create-if-absent operation. Concurrent begins can +therefore never overwrite one another. Later phase writes retain the +same-directory rename path. This recovery path restores the application to a clean retry state when the old or new version can launch. It does **not** claim rollback of a partially @@ -99,7 +116,11 @@ npm run smoke:parent-death The lifecycle tests start an unrelated Python sentinel before stopping the desktop-owned backend. The sentinel must still be alive after graceful and forced-parent-death cleanup, while the owned Python PID and listener must be -gone. +gone. The update-safety test also holds open a TCP listener that never answers +HTTP, proves shutdown fails closed while retaining the exact identifiers, then +closes the listener and proves a second cleanup attempt succeeds. It races two +journal begins and requires exactly one durable winner, and mutates an artifact +during and after verification to exercise both integrity gates. ## Remaining enablement gates diff --git a/desktop/electron/scripts/test-update-safety.mjs b/desktop/electron/scripts/test-update-safety.mjs index 40f43049..48e50ee4 100644 --- a/desktop/electron/scripts/test-update-safety.mjs +++ b/desktop/electron/scripts/test-update-safety.mjs @@ -1,11 +1,16 @@ import assert from "node:assert/strict"; import { createHash } from "node:crypto"; +import { writeFileSync } from "node:fs"; import { access, mkdtemp, readFile, writeFile } from "node:fs/promises"; +import { createServer } from "node:net"; import os from "node:os"; import path from "node:path"; +import { BackendManager } from "../dist/backend-manager.js"; +import { getDesktopMessages } from "../dist/locales.js"; import { UpdateRecoveryJournal } from "../dist/update-recovery.js"; import { + assertVerifiedUpdateArtifactUnchanged, compareSemanticVersions, inspectAuthenticodeSignature, verifyWindowsUpdateCandidate, @@ -26,12 +31,14 @@ const policy = { }; const validSignature = () => ({ status: "Valid", + artifactSha256: fixtureHash, signerSubject: "CN=Vibe-Trading Test Publisher", certificateSha256: publisher, }); const accepted = await verifyWindowsUpdateCandidate(candidate, policy, validSignature); assert.equal(accepted.accepted, true); +await assertVerifiedUpdateArtifactUnchanged(artifactPath, accepted.sha256); await expectRejection( { ...candidate, expectedSha256: "00".repeat(32) }, @@ -76,6 +83,31 @@ await expectRejection( validSignature, "artifact-inspection-failed", ); +await expectRejection( + candidate, + policy, + () => ({ ...validSignature(), artifactSha256: otherPublisher }), + "artifact-changed-during-verification", +); + +const mutationArtifactPath = path.join(testRoot, "mutation.exe"); +await writeFile(mutationArtifactPath, fixture); +await expectRejection( + { ...candidate, artifactPath: mutationArtifactPath }, + policy, + (inspectedPath) => { + writeFileSync(inspectedPath, Buffer.from("changed-during-signature-inspection", "utf8")); + return validSignature(); + }, + "artifact-changed-during-verification", +); + +await writeFile(artifactPath, Buffer.from("changed-after-verification", "utf8")); +await assert.rejects( + () => assertVerifiedUpdateArtifactUnchanged(artifactPath, accepted.sha256), + /changed before installer launch/u, +); +await writeFile(artifactPath, fixture); assert.equal(compareSemanticVersions("0.3.1", "0.3.0"), 1); assert.equal(compareSemanticVersions("0.3.1-beta.2", "0.3.1-beta.1"), 1); @@ -140,6 +172,21 @@ assert.equal( assert.equal(await exists(mismatchArtifact), true); assert.equal(await exists(mismatchJournal.journalPath), true); +const concurrentDirectory = await mkdtemp(path.join(testRoot, "concurrent-")); +const concurrentJournal = new UpdateRecoveryJournal(concurrentDirectory); +const concurrentResults = await Promise.allSettled([ + concurrentJournal.begin(newAttempt("first.exe")), + concurrentJournal.begin(newAttempt("second.exe")), +]); +const concurrentWinners = concurrentResults.filter((result) => result.status === "fulfilled"); +const concurrentLosers = concurrentResults.filter((result) => result.status === "rejected"); +assert.equal(concurrentWinners.length, 1); +assert.equal(concurrentLosers.length, 1); +assert.match(String(concurrentLosers[0].reason), /already exists/u); +const concurrentRecord = JSON.parse(await readFile(concurrentJournal.journalPath, "utf8")); +assert.equal(concurrentRecord.attemptId, concurrentWinners[0].value.attemptId); +assert.equal(concurrentRecord.artifactFileName, concurrentWinners[0].value.artifactFileName); + const traversalDirectory = await mkdtemp(path.join(testRoot, "traversal-")); const traversalJournal = new UpdateRecoveryJournal(traversalDirectory); const protectedFile = path.join(testRoot, "protected.exe"); @@ -158,9 +205,56 @@ assert.equal( ); assert.equal((await readFile(protectedFile)).equals(fixture), true); +const hungListener = createServer(() => { + // Accept TCP and deliberately never speak HTTP. Shutdown evidence must still + // classify this port as open. +}); +await listen(hungListener); +const hungAddress = hungListener.address(); +if (!hungAddress || typeof hungAddress === "string") throw new Error("Expected a TCP listener address"); +const hungBaseUrl = `http://127.0.0.1:${hungAddress.port}/`; +const retainedBackendPid = 2_147_483_646; +const retainedWatchdogPid = 2_147_483_647; +const retainedManager = new BackendManager({ + appPath: testRoot, + resourcesPath: testRoot, + allowSourceDiscovery: false, + logDirectory: testRoot, + apiAuthKey: "test-auth-key", + messages: getDesktopMessages("en"), + shutdownEvidenceTimeoutMilliseconds: 25, + onStatus: () => {}, + onUnexpectedExit: () => {}, +}); +// TypeScript `private` fields compile to ordinary properties. This test seam +// models an already-exited watchdog whose exact identifiers must be retained +// while a stubborn listener survives. +retainedManager.watchdog = { + pid: retainedWatchdogPid, + exitCode: 0, + signalCode: null, +}; +retainedManager.backendPid = retainedBackendPid; +retainedManager.baseUrl = hungBaseUrl; +await assert.rejects(() => retainedManager.stopForUpdate(), /could not be verified/u); +assert.equal(retainedManager.url, hungBaseUrl); +assert.equal(retainedManager.processId, retainedBackendPid); +await close(hungListener); +const retriedEvidence = await retainedManager.stopForUpdate(); +assert.deepEqual(retriedEvidence, { + backendPid: retainedBackendPid, + watchdogPid: retainedWatchdogPid, + backendExited: true, + watchdogExited: true, + listenerClosed: true, +}); +assert.equal(retainedManager.url, undefined); +assert.equal(retainedManager.processId, undefined); + console.log(JSON.stringify({ - verificationCases: ["accepted", "tampered", "unsigned", "invalid-signature", "wrong-publisher", "downgraded"], - recoveryPhases: ["verified", "backend-stopped", "installer-launched", "completed", "version-mismatch", "path-traversal"], + verificationCases: ["accepted", "tampered", "unsigned", "invalid-signature", "wrong-publisher", "downgraded", "mutation-during-verification", "mutation-before-launch"], + recoveryPhases: ["verified", "backend-stopped", "installer-launched", "completed", "version-mismatch", "path-traversal", "concurrent-begin"], + shutdownFailureRetryVerified: true, authenticodeAdapterVerified, updaterEnabled: false, }, null, 2)); @@ -189,3 +283,19 @@ async function exists(file) { return false; } } + +function listen(server) { + return new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(0, "127.0.0.1", () => { + server.off("error", reject); + resolve(); + }); + }); +} + +function close(server) { + return new Promise((resolve, reject) => { + server.close((error) => error ? reject(error) : resolve()); + }); +} diff --git a/desktop/electron/src/backend-manager.ts b/desktop/electron/src/backend-manager.ts index 0def9db6..1be77fa1 100644 --- a/desktop/electron/src/backend-manager.ts +++ b/desktop/electron/src/backend-manager.ts @@ -6,7 +6,7 @@ import { readFileSync, WriteStream, } from "node:fs"; -import { createServer } from "node:net"; +import { connect, createServer } from "node:net"; import path from "node:path"; import { DesktopMessages, @@ -21,6 +21,7 @@ type BackendManagerOptions = { apiAuthKey: string; messages: DesktopMessages; credentialEnvironment?: NodeJS.ProcessEnv; + shutdownEvidenceTimeoutMilliseconds?: number; onStatus: (message: string) => void; onUnexpectedExit: (message: string) => void; }; @@ -39,6 +40,11 @@ export type BackendShutdownEvidence = { listenerClosed: boolean; }; +type BackendShutdownTarget = Pick< + BackendShutdownEvidence, + "backendPid" | "watchdogPid" +> & { baseUrl?: string }; + export type BackendResolutionOptions = { appPath: string; resourcesPath: string; @@ -62,6 +68,7 @@ export class BackendManager { private watchdogError: Error | undefined; private backendPid: number | undefined; private baseUrl: string | undefined; + private shutdownTarget: BackendShutdownTarget | undefined; private stopping = false; private logStream: WriteStream | undefined; private readonly recentOutput: string[] = []; @@ -138,7 +145,6 @@ export class BackendManager { }); watchdog.once("exit", (code, signal) => { this.writeLog(`[WATCHDOG] exited code=${String(code)} signal=${String(signal)}`); - this.backendPid = undefined; if (!this.stopping) { this.options.onUnexpectedExit(formatDesktopMessage( this.options.messages.backendUnexpectedExit, @@ -155,16 +161,16 @@ export class BackendManager { async stop(): Promise { const watchdog = this.watchdog; - const backendPid = this.backendPid; - const watchdogPid = watchdog?.pid; - const baseUrl = this.baseUrl; - if (!watchdog) { - return shutdownEvidence(backendPid, watchdogPid, baseUrl); - } + const target = this.shutdownTarget ?? { + backendPid: this.backendPid, + watchdogPid: watchdog?.pid, + baseUrl: this.baseUrl, + }; + this.shutdownTarget = target; this.stopping = true; - if (isRunning(watchdog) && this.baseUrl) { + if (watchdog && isRunning(watchdog) && target.baseUrl) { try { - await fetch(new URL("system/shutdown", this.baseUrl), { + await fetch(new URL("system/shutdown", target.baseUrl), { method: "POST", headers: { Authorization: `Bearer ${this.options.apiAuthKey}` }, signal: AbortSignal.timeout(2_000), @@ -174,11 +180,11 @@ export class BackendManager { } } - if (isRunning(watchdog)) { + if (watchdog && isRunning(watchdog)) { await Promise.race([waitForExit(watchdog), delay(3_000)]); } - if (isRunning(watchdog)) { - this.writeLog(`[SHUTDOWN] asking watchdog to terminate backend pid=${String(this.backendPid)}`); + if (watchdog && isRunning(watchdog)) { + this.writeLog(`[SHUTDOWN] asking watchdog to terminate backend pid=${String(target.backendPid)}`); try { watchdog.send({ type: "terminate-backend" }); } catch (error) { @@ -186,21 +192,23 @@ export class BackendManager { } await Promise.race([waitForExit(watchdog), delay(5_000)]); } - if (isRunning(watchdog) && watchdog.pid) { - this.writeLog(`[SHUTDOWN] terminating watchdog tree pid=${watchdog.pid}`); - await runTaskkill(watchdog.pid); + if (watchdog && isRunning(watchdog) && target.watchdogPid) { + this.writeLog(`[SHUTDOWN] terminating watchdog tree pid=${target.watchdogPid}`); + await runTaskkill(target.watchdogPid); } const evidence = await waitForShutdownEvidence( - backendPid, - watchdogPid, - baseUrl, - 5_000, + target.backendPid, + target.watchdogPid, + target.baseUrl, + this.options.shutdownEvidenceTimeoutMilliseconds ?? 5_000, ); - if (!evidence.backendExited || !evidence.watchdogExited || !evidence.listenerClosed) { + if (!isCompleteShutdownEvidence(evidence)) { this.writeLog(`[SHUTDOWN] incomplete evidence ${JSON.stringify(evidence)}`); + return evidence; } + this.shutdownTarget = undefined; this.watchdog = undefined; this.watchdogError = undefined; this.backendPid = undefined; @@ -212,7 +220,7 @@ export class BackendManager { async stopForUpdate(): Promise { const evidence = await this.stop(); - if (!evidence.backendExited || !evidence.watchdogExited || !evidence.listenerClosed) { + if (!isCompleteShutdownEvidence(evidence)) { throw new Error(`Owned backend shutdown could not be verified: ${JSON.stringify(evidence)}`); } return evidence; @@ -443,7 +451,7 @@ async function waitForShutdownEvidence( let evidence = await shutdownEvidence(backendPid, watchdogPid, baseUrl); while ( Date.now() < deadline && - (!evidence.backendExited || !evidence.watchdogExited || !evidence.listenerClosed) + !isCompleteShutdownEvidence(evidence) ) { await delay(100); evidence = await shutdownEvidence(backendPid, watchdogPid, baseUrl); @@ -461,10 +469,14 @@ async function shutdownEvidence( watchdogPid, backendExited: !backendPid || !isProcessAlive(backendPid), watchdogExited: !watchdogPid || !isProcessAlive(watchdogPid), - listenerClosed: !baseUrl || !(await listenerAcceptsConnections(baseUrl)), + listenerClosed: !baseUrl || await listenerIsClosed(baseUrl), }; } +function isCompleteShutdownEvidence(evidence: BackendShutdownEvidence): boolean { + return evidence.backendExited && evidence.watchdogExited && evidence.listenerClosed; +} + function isProcessAlive(pid: number): boolean { try { process.kill(pid, 0); @@ -474,15 +486,32 @@ function isProcessAlive(pid: number): boolean { } } -async function listenerAcceptsConnections(baseUrl: string): Promise { +async function listenerIsClosed(baseUrl: string): Promise { + let endpoint: URL; try { - await fetch(new URL("health", baseUrl), { - signal: AbortSignal.timeout(500), - }); - return true; + endpoint = new URL(baseUrl); } catch { return false; } + const port = Number(endpoint.port); + if (!Number.isSafeInteger(port) || port <= 0 || port > 65_535) return false; + return new Promise((resolve) => { + const socket = connect({ host: endpoint.hostname, port }); + let settled = false; + const finish = (closed: boolean): void => { + if (settled) return; + settled = true; + socket.destroy(); + resolve(closed); + }; + socket.setTimeout(500, () => finish(false)); + socket.once("connect", () => finish(false)); + socket.once("error", (error: NodeJS.ErrnoException) => { + // Only an explicit loopback refusal proves that no listener accepted the + // connection. Timeouts and other socket failures stay fail-closed. + finish(error.code === "ECONNREFUSED"); + }); + }); } function runTaskkill(pid: number): Promise { diff --git a/desktop/electron/src/update-recovery.ts b/desktop/electron/src/update-recovery.ts index 4d8ca7a5..97a3762c 100644 --- a/desktop/electron/src/update-recovery.ts +++ b/desktop/electron/src/update-recovery.ts @@ -1,5 +1,5 @@ import { randomBytes } from "node:crypto"; -import { mkdir, readFile, rename, rm, unlink, writeFile } from "node:fs/promises"; +import { link, mkdir, open, readFile, rename, rm, unlink } from "node:fs/promises"; import path from "node:path"; import { compareSemanticVersions } from "./update-verification"; @@ -53,12 +53,6 @@ export class UpdateRecoveryJournal { throw new Error("An update attempt must target a newer semantic version"); } await mkdir(this.stagingDirectory, { recursive: true }); - try { - await readFile(this.journalPath, "utf8"); - throw new Error("A pending update attempt already exists"); - } catch (error) { - if (errorCode(error) !== "ENOENT") throw error; - } const now = new Date().toISOString(); const record: UpdateAttempt = { ...attempt, @@ -68,7 +62,7 @@ export class UpdateRecoveryJournal { createdAt: now, updatedAt: now, }; - await this.write(record); + await this.write(record, true); return record; } @@ -135,17 +129,34 @@ export class UpdateRecoveryJournal { return parsed; } - private async write(attempt: UpdateAttempt): Promise { + private async write(attempt: UpdateAttempt, exclusive = false): Promise { const temporaryPath = path.join( this.stagingDirectory, `${journalFileName}.${process.pid}.${randomBytes(6).toString("hex")}.tmp`, ); try { - await writeFile(temporaryPath, `${JSON.stringify(attempt, null, 2)}\n`, { - encoding: "utf8", - flag: "wx", - }); - await rename(temporaryPath, this.journalPath); + const handle = await open(temporaryPath, "wx"); + try { + await handle.writeFile(`${JSON.stringify(attempt, null, 2)}\n`, "utf8"); + await handle.sync(); + } finally { + await handle.close(); + } + if (exclusive) { + try { + // Linking a complete same-directory temp file is an atomic + // create-if-absent operation on Windows and POSIX. Unlike rename, it + // can never replace a concurrent pending attempt. + await link(temporaryPath, this.journalPath); + } catch (error) { + if (errorCode(error) === "EEXIST") { + throw new Error("A pending update attempt already exists"); + } + throw error; + } + } else { + await rename(temporaryPath, this.journalPath); + } } finally { await rm(temporaryPath, { force: true }); } diff --git a/desktop/electron/src/update-verification.ts b/desktop/electron/src/update-verification.ts index 27dbf8a4..a1d6734e 100644 --- a/desktop/electron/src/update-verification.ts +++ b/desktop/electron/src/update-verification.ts @@ -5,6 +5,7 @@ import path from "node:path"; export type AuthenticodeInspection = { status: string; + artifactSha256?: string; signerSubject?: string; certificateSha256?: string; }; @@ -26,6 +27,7 @@ export type UpdateRejectionCode = | "version-not-newer" | "artifact-inspection-failed" | "artifact-hash-mismatch" + | "artifact-changed-during-verification" | "artifact-unsigned" | "artifact-signature-invalid" | "publisher-not-allowed"; @@ -113,15 +115,56 @@ export async function verifyWindowsUpdateCandidate( ); } + const inspectedArtifactSha256 = normalizeSha256(signature.artifactSha256); + if (!inspectedArtifactSha256 || inspectedArtifactSha256 !== actualHash) { + return reject( + "artifact-changed-during-verification", + "Authenticode inspection did not cover the same artifact bytes as the release digest.", + ); + } + + let confirmedHash: string; + try { + confirmedHash = await sha256(candidate.artifactPath); + } catch (error) { + return reject( + "artifact-inspection-failed", + `The artifact could not be confirmed after signature inspection: ${errorText(error)}`, + ); + } + if (confirmedHash !== actualHash) { + return reject( + "artifact-changed-during-verification", + "The update artifact changed while its signature was being verified.", + ); + } + return { accepted: true, version: candidate.version, - sha256: actualHash, + sha256: confirmedHash, signerSubject: signature.signerSubject, certificateSha256, }; } +export async function assertVerifiedUpdateArtifactUnchanged( + artifactPath: string, + expectedSha256: string, +): Promise { + const expectedHash = normalizeSha256(expectedSha256); + if (!expectedHash) throw new Error("The verified artifact SHA-256 is malformed"); + let actualHash: string; + try { + actualHash = await sha256(artifactPath); + } catch (error) { + throw new Error(`The verified update artifact could not be read: ${errorText(error)}`); + } + if (actualHash !== expectedHash) { + throw new Error("The verified update artifact changed before installer launch"); + } +} + export function inspectAuthenticodeSignature(artifactPath: string): AuthenticodeInspection { if (process.platform !== "win32") { return { status: "UnsupportedPlatform" }; @@ -133,22 +176,33 @@ export function inspectAuthenticodeSignature(artifactPath: string): Authenticode "-NonInteractive", "-Command", [ - "$signature = Get-AuthenticodeSignature -LiteralPath $env:VIBE_UPDATE_ARTIFACT_PATH", - "$fingerprint = $null", - "if ($signature.SignerCertificate) {", - " $sha256 = [System.Security.Cryptography.SHA256]::Create()", + "$artifactPath = [System.IO.Path]::GetFullPath($env:VIBE_UPDATE_ARTIFACT_PATH)", + "$stream = [System.IO.File]::Open($artifactPath, [System.IO.FileMode]::Open, [System.IO.FileAccess]::Read, [System.IO.FileShare]::Read)", + "try {", + " $artifactHasher = [System.Security.Cryptography.SHA256]::Create()", " try {", - " $bytes = $sha256.ComputeHash($signature.SignerCertificate.RawData)", - " $fingerprint = ([System.BitConverter]::ToString($bytes)).Replace('-', '').ToLowerInvariant()", - " } finally { $sha256.Dispose() }", - "}", - "@{ status = $signature.Status.ToString(); signerSubject = $signature.SignerCertificate.Subject; certificateSha256 = $fingerprint } | ConvertTo-Json -Compress", - ].join("; "), + " $artifactBytes = $artifactHasher.ComputeHash($stream)", + " $artifactFingerprint = ([System.BitConverter]::ToString($artifactBytes)).Replace('-', '').ToLowerInvariant()", + " } finally { $artifactHasher.Dispose() }", + " $signature = Get-AuthenticodeSignature -LiteralPath $artifactPath", + " $certificateFingerprint = $null", + " if ($signature.SignerCertificate) {", + " $certificateHasher = [System.Security.Cryptography.SHA256]::Create()", + " try {", + " $certificateBytes = $certificateHasher.ComputeHash($signature.SignerCertificate.RawData)", + " $certificateFingerprint = ([System.BitConverter]::ToString($certificateBytes)).Replace('-', '').ToLowerInvariant()", + " } finally { $certificateHasher.Dispose() }", + " }", + " @{ status = $signature.Status.ToString(); artifactSha256 = $artifactFingerprint; signerSubject = $signature.SignerCertificate.Subject; certificateSha256 = $certificateFingerprint } | ConvertTo-Json -Compress", + "} finally { $stream.Dispose() }", + ].join("\n"), ], { env: { ...process.env, VIBE_UPDATE_ARTIFACT_PATH: path.resolve(artifactPath) }, encoding: "utf8", windowsHide: true, + timeout: 15_000, + maxBuffer: 1_048_576, }, ); if (result.error) throw result.error; @@ -162,6 +216,9 @@ export function inspectAuthenticodeSignature(artifactPath: string): Authenticode const payload = parsed as Record; return { status: payload.status as string, + artifactSha256: typeof payload.artifactSha256 === "string" + ? payload.artifactSha256 + : undefined, signerSubject: typeof payload.signerSubject === "string" ? payload.signerSubject : undefined, certificateSha256: typeof payload.certificateSha256 === "string" ? payload.certificateSha256 @@ -231,7 +288,12 @@ function resolveSignaturePowerShell(): string { const probe = spawnSync( candidate, ["-NoProfile", "-NonInteractive", "-Command", "Get-Command Get-AuthenticodeSignature -ErrorAction Stop | Out-Null"], - { encoding: "utf8", windowsHide: true }, + { + encoding: "utf8", + windowsHide: true, + timeout: 5_000, + maxBuffer: 262_144, + }, ); if (!probe.error && probe.status === 0) return candidate; }