diff --git a/src/codex/desktop-compatibility/windows-key-protection.ts b/src/codex/desktop-compatibility/windows-key-protection.ts index a40c64d408e..fc1a11b54f6 100644 --- a/src/codex/desktop-compatibility/windows-key-protection.ts +++ b/src/codex/desktop-compatibility/windows-key-protection.ts @@ -1,12 +1,14 @@ import { execFile } from "node:child_process"; -import { resolveTrustedWindowsPowerShellExe } from "../../lib/windows-elevation"; +import { join } from "node:path"; +import { dlopen, ptr, toArrayBuffer, type Pointer } from "bun:ffi"; +import { resolveTrustedWindowsPowerShellExe, resolveTrustedWindowsSystemDirectory } from "../../lib/windows-elevation"; const MAX_INPUT = 65_536; const MAX_OUTPUT = 131_072; const PURPOSE = "opencodex/codex-desktop-compatibility/authority/v1"; -// A cold Windows PowerShell that must also load System.Security has been measured past 10s on a -// loaded machine (Windows CI run 38591626343). The call is async and off the request path, so a -// wider bound costs nothing while healthy and turns a slow start into success instead of a refusal. +const ENTROPY = new TextEncoder().encode(PURPOSE); +// Only the PowerShell fallback is bounded by a clock. A cold Windows PowerShell that must also load +// System.Security has been measured past 10s on a loaded machine (Windows CI run 38591626343). const DPAPI_TIMEOUT_MS = 20_000; const SCRIPT = [ "$ErrorActionPreference='Stop'", @@ -24,18 +26,94 @@ const SCRIPT = [ "[Console]::Out.Write([Convert]::ToBase64String($out))", ].join("\n"); +type Operation = "protect" | "unprotect"; + export interface AuthorityKeyProtection { protect(cleartext: Uint8Array): Promise; unprotect(ciphertext: Uint8Array): Promise; } +// CRYPTPROTECT_UI_FORBIDDEN and no CRYPTPROTECT_LOCAL_MACHINE: the CurrentUser scope that +// ProtectedData.Protect(..., CurrentUser) requests, so blobs written by either path read in the other. +const CRYPTPROTECT_UI_FORBIDDEN = 0x1; +// DATA_BLOB on x64: DWORD cbData, four bytes of padding, BYTE* pbData. +const DATA_BLOB_BYTES = 16; +const DPAPI_SIGNATURE = { args: ["ptr", "ptr", "ptr", "ptr", "ptr", "u32", "ptr"], returns: "i32" } as const; + +type NativeDpapi = (operation: Operation, data: Uint8Array) => Uint8Array; +let nativeDpapi: NativeDpapi | undefined; + +function dataBlob(bytes: Uint8Array): Buffer { + const blob = Buffer.alloc(DATA_BLOB_BYTES); + blob.writeUInt32LE(bytes.byteLength, 0); + blob.writeBigUInt64LE(BigInt(ptr(bytes)), 8); + return blob; +} + +/** + * In-process CryptProtectData/CryptUnprotectData. Spawning Windows PowerShell for every call cost + * seconds of cold start (past 20s on a loaded CI runner) for a call that takes milliseconds. + * crypt32 is loaded from the GetSystemDirectoryW path so the DLL search order cannot substitute it. + * Returns null where Bun's FFI is unavailable (Windows ARM64), leaving the PowerShell helper. + */ +function loadNativeDpapi(): NativeDpapi | null { + if (nativeDpapi) return nativeDpapi; + if (process.platform !== "win32" || process.arch !== "x64") return null; + let crypt32; + let kernel32; + try { + const systemDirectory = resolveTrustedWindowsSystemDirectory(); + crypt32 = dlopen(join(systemDirectory, "crypt32.dll"), { + CryptProtectData: DPAPI_SIGNATURE, + CryptUnprotectData: DPAPI_SIGNATURE, + }).symbols; + kernel32 = dlopen(join(systemDirectory, "kernel32.dll"), { + LocalFree: { args: ["ptr"], returns: "ptr" }, + // Wipes by address, so clearing never depends on a JS view having been created. + RtlZeroMemory: { args: ["ptr", "u64"], returns: "void" }, + }).symbols; + } catch { + return null; + } + const { CryptProtectData, CryptUnprotectData } = crypt32; + const { LocalFree, RtlZeroMemory } = kernel32; + nativeDpapi = (operation, data) => { + const input = dataBlob(data); + const entropy = dataBlob(ENTROPY); + const output = Buffer.alloc(DATA_BLOB_BYTES); + const call = operation === "protect" ? CryptProtectData : CryptUnprotectData; + // Description, reserved and prompt are null; Windows reports no detail worth propagating. + if (!call(ptr(input), null, ptr(entropy), null, null, CRYPTPROTECT_UI_FORBIDDEN, ptr(output))) { + throw new Error("desktop_compatibility_key_protection_failed"); + } + const length = output.readUInt32LE(0); + const address = Number(output.readBigUInt64LE(8)) as Pointer; + // Every step after a successful call sits inside try so the native buffer is always + // released, even if creating the view over it throws. + let view: Uint8Array | null = null; + try { + view = length > 0 ? new Uint8Array(toArrayBuffer(address, 0, length)) : null; + if (length === 0 || length > MAX_INPUT) throw new Error("desktop_compatibility_key_size"); + return Uint8Array.from(view!); + } finally { + // The unprotected buffer is cleartext key material; wipe it natively before LocalFree + // releases it, including when the size check or view creation fails. LocalFree still + // runs if wiping throws. + try { + if (operation === "unprotect" && length > 0) RtlZeroMemory(address, length); + } finally { + LocalFree(address); + } + } + }; + return nativeDpapi; +} + /** The payload travels on stdin, never in process arguments, environment or logs. */ -function runDpapi(operation: "protect" | "unprotect", data: Uint8Array): Promise { - if (process.platform !== "win32") return Promise.reject(new Error("desktop_compatibility_windows_required")); - if (data.byteLength === 0 || data.byteLength > MAX_INPUT) return Promise.reject(new Error("desktop_compatibility_key_size")); +function runPowerShellDpapi(operation: Operation, data: Uint8Array, timeoutMs = DPAPI_TIMEOUT_MS): Promise { return new Promise((resolve, reject) => { - const child = execFile(resolveTrustedWindowsPowerShellExe(), ["-NoProfile", "-NonInteractive", "-Command", SCRIPT], { - timeout: DPAPI_TIMEOUT_MS, maxBuffer: MAX_OUTPUT, windowsHide: true, encoding: "utf8", + const child = execFile(resolveTrustedWindowsPowerShellExe(), ["-NoLogo", "-NoProfile", "-NonInteractive", "-Command", SCRIPT], { + timeout: timeoutMs, maxBuffer: MAX_OUTPUT, windowsHide: true, encoding: "utf8", }, (error, stdout) => { // Do not include PowerShell stderr, command output or the input in a propagated error. if (error || !/^[A-Za-z0-9+/]+={0,2}$/.test(stdout.trim())) { @@ -52,8 +130,36 @@ function runDpapi(operation: "protect" | "unprotect", data: Uint8Array): Promise }); } +function validated(data: Uint8Array): Uint8Array { + if (process.platform !== "win32") throw new Error("desktop_compatibility_windows_required"); + if (data.byteLength === 0 || data.byteLength > MAX_INPUT) throw new Error("desktop_compatibility_key_size"); + return data; +} + +async function runDpapi(operation: Operation, data: Uint8Array): Promise { + validated(data); + const native = loadNativeDpapi(); + return native ? native(operation, data) : runPowerShellDpapi(operation, data); +} + /** CurrentUser protects against other OS identities, not other processes of the same user. */ export const windowsAuthorityKeyProtection: AuthorityKeyProtection = { protect: bytes => runDpapi("protect", bytes), unprotect: bytes => runDpapi("unprotect", bytes), }; + +/** + * Test-only: the PowerShell helper (Windows ARM64, and every key stored before the native path). + * The caller owns the bound because a compatibility check must not depend on cold-start latency. + */ +export function powerShellAuthorityKeyProtectionForTests(timeoutMs: number): AuthorityKeyProtection { + return { + protect: async bytes => runPowerShellDpapi("protect", validated(bytes), timeoutMs), + unprotect: async bytes => runPowerShellDpapi("unprotect", validated(bytes), timeoutMs), + }; +} + +/** Test-only: whether this process protects keys in-process rather than through PowerShell. */ +export function usesNativeDpapiForTests(): boolean { + return process.platform === "win32" && loadNativeDpapi() !== null; +} diff --git a/structure/clients/codex-desktop.md b/structure/clients/codex-desktop.md index 0312fb9cf71..886205dd02c 100644 --- a/structure/clients/codex-desktop.md +++ b/structure/clients/codex-desktop.md @@ -52,11 +52,16 @@ Failure before publication preserves the original removable identity; staging is An older authority without the server-only EKU reports `renewal-required`: serving and trust registration refuse it, while exact trust removal and deliberate renewal remain available. -`src/codex/desktop-compatibility/windows-key-protection.ts` uses trusted PowerShell -and bounded stdin/stdout, never command-line secrets or plaintext fallback. CurrentUser -protects against other OS identities, not another process using the same credentials. +`src/codex/desktop-compatibility/windows-key-protection.ts` calls CurrentUser +`CryptProtectData`/`CryptUnprotectData` in-process on Windows x64, loading `crypt32.dll` +from the `GetSystemDirectoryW` path and clearing unprotected output before `LocalFree`. +Windows ARM64, where Bun FFI is unavailable, keeps the trusted PowerShell helper with +bounded stdin/stdout. Both write the same blob format; neither puts secrets on a command +line or falls back to plaintext. CurrentUser protects against other OS identities, not +another process using the same credentials. `tests/clients/desktop-compatibility-authority.test.ts` covers persistence, refusal, -and a real Windows DPAPI round trip with synthetic data. +a real Windows DPAPI round trip with synthetic data, and a PowerShell-written key opening +in-process. ## Certificate setup API diff --git a/tests/clients/desktop-compatibility-authority.test.ts b/tests/clients/desktop-compatibility-authority.test.ts index 8b3c142693c..ea8582a6451 100644 --- a/tests/clients/desktop-compatibility-authority.test.ts +++ b/tests/clients/desktop-compatibility-authority.test.ts @@ -5,7 +5,9 @@ import { existsSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSy import { tmpdir } from "node:os"; import { join } from "node:path"; import { ensureDesktopCompatibilityAuthority, inspectDesktopCompatibilityAuthority, loadDesktopCompatibilityAuthority, renewDesktopCompatibilityAuthority } from "../../src/codex/desktop-compatibility/certificate-store"; -import { windowsAuthorityKeyProtection, type AuthorityKeyProtection } from "../../src/codex/desktop-compatibility/windows-key-protection"; +import { + powerShellAuthorityKeyProtectionForTests, usesNativeDpapiForTests, windowsAuthorityKeyProtection, type AuthorityKeyProtection, +} from "../../src/codex/desktop-compatibility/windows-key-protection"; import { setAsyncIcaclsRunnerForTests, setIcaclsRunnerForTests } from "../../src/lib/windows-secret-acl"; const roots: string[] = []; @@ -164,11 +166,20 @@ describe("Codex Desktop compatibility authority lifecycle", () => { }); test.skipIf(process.platform !== "win32")("real CurrentUser DPAPI survives separate helper invocations and rejects corruption", async () => { + // x64 protects in-process; spawning PowerShell per call is what exceeded its bound on CI. + expect(usesNativeDpapiForTests()).toBe(process.arch === "x64"); const original = Buffer.from("synthetic authority secret; no real credentials"); const encrypted = await windowsAuthorityKeyProtection.protect(original); expect(Buffer.from(encrypted).includes(original)).toBe(false); expect(Buffer.from(await windowsAuthorityKeyProtection.unprotect(encrypted))).toEqual(original); const corrupted = Uint8Array.from(encrypted); corrupted[corrupted.length - 1] ^= 1; await expect(windowsAuthorityKeyProtection.unprotect(corrupted)).rejects.toThrow("desktop_compatibility_key_protection_failed"); - }, 65_000); // three PowerShell calls, each bounded at 20s, plus margin + }, 65_000); // ARM64 still takes the PowerShell path: three calls, each bounded at 20s + + test.skipIf(process.platform !== "win32" || process.arch !== "x64")("a key protected by the PowerShell helper still opens in-process", async () => { + // Every key stored before the native path was written by ProtectedData through PowerShell. + const original = Buffer.from("synthetic legacy authority secret; no real credentials"); + const legacy = await powerShellAuthorityKeyProtectionForTests(50_000).protect(original); + expect(Buffer.from(await windowsAuthorityKeyProtection.unprotect(legacy))).toEqual(original); + }, 60_000); // one cold PowerShell; the bound is the test's, not the production path's });