From df6989c178238f11f4c405e3d87f4026058cb05b Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Fri, 7 Aug 2026 01:36:58 +0900 Subject: [PATCH] fix(windows): grant secret ACLs to effective token SID Resolve the current token SID instead of trusting USERDOMAIN and USERNAME on workgroup hosts. Keep identity lookup failures separate from icacls timeouts.\n\nRefs #1149 --- src/lib/windows-secret-acl.ts | 49 +++--- src/lib/windows-user-principal.ts | 215 +++++++++++++++++++++++++++ tests/windows-secret-acl.test.ts | 75 ++++++++++ tests/windows-user-principal.test.ts | 131 ++++++++++++++++ 4 files changed, 448 insertions(+), 22 deletions(-) create mode 100644 src/lib/windows-user-principal.ts create mode 100644 tests/windows-user-principal.test.ts diff --git a/src/lib/windows-secret-acl.ts b/src/lib/windows-secret-acl.ts index f1b72bce8..ff994bc2e 100644 --- a/src/lib/windows-secret-acl.ts +++ b/src/lib/windows-secret-acl.ts @@ -31,6 +31,10 @@ import { existsSync, statSync } from "node:fs"; import { env, platform } from "node:process"; +import { + resolveCurrentWindowsPrincipal, + resolveCurrentWindowsPrincipalAsync, +} from "./windows-user-principal"; const hardenedDirectories = new Map(); const hardenedPaths = new Map(); @@ -393,18 +397,23 @@ function icaclsError(step: string, result: IcaclsResult): NodeJS.ErrnoException return err; } -/** - * Return the current Windows username from the environment. - * Falls back to USERDOMAIN\USERNAME if USERNAME alone is ambiguous. - * The value is used directly in icacls arguments, so it must be present. - */ -function currentWindowsUser(): string | undefined { - const username = env["USERNAME"]; - const domain = env["USERDOMAIN"]; - if (!username) return undefined; - // USERDOMAIN is the machine/domain name; USERNAME is the account name. - // icacls accepts "DOMAIN\User" or just "User" for local accounts. - return domain ? `${domain}\\${username}` : username; +// POSIX CI deliberately drives the Windows ACL branch through platformOverride. +// The synthetic SID exists only for that test seam; production Windows always +// resolves the effective token and never falls back to USERDOMAIN/USERNAME. +const FORCED_NON_WINDOWS_TEST_PRINCIPAL = "*S-1-5-21-1-2-3-1001"; + +function currentWindowsPrincipal(deadline: number): string { + if (platformOverride === "win32" && platform !== "win32") { + return FORCED_NON_WINDOWS_TEST_PRINCIPAL; + } + return resolveCurrentWindowsPrincipal(deadline - nowFn()); +} + +async function currentWindowsPrincipalAsync(deadline: number): Promise { + if (platformOverride === "win32" && platform !== "win32") { + return FORCED_NON_WINDOWS_TEST_PRINCIPAL; + } + return resolveCurrentWindowsPrincipalAsync(deadline - nowFn()); } /** @@ -424,10 +433,7 @@ function grantAce(user: string, directory: boolean): string { } function runIcacls(targetPath: string, directory: boolean, deadline: number): void { - const user = currentWindowsUser(); - if (!user) { - throw new Error("Cannot determine current Windows user for ACL hardening"); - } + const principal = currentWindowsPrincipal(deadline); // The deadline is owned by hardenEntry (total budget incl. retry + verification). const run = (step: string, args: string[]): IcaclsResult => { @@ -444,7 +450,7 @@ function runIcacls(targetPath: string, directory: boolean, deadline: number): vo // Step 1: grant current user full control BEFORE any destructive ACL change. // If this fails, inheritance is untouched and the writer keeps inherited access. - runOrThrow("/grant:r", [targetPath, "/grant:r", grantAce(user, directory)]); + runOrThrow("/grant:r", [targetPath, "/grant:r", grantAce(principal, directory)]); // Step 2: disable inheritance and remove inherited ACEs. The explicit owner ACE // from step 1 survives this transition, so a later failure still leaves cleanup access. @@ -473,10 +479,7 @@ function runIcacls(targetPath: string, directory: boolean, deadline: number): vo /** Async counterpart of runIcacls — same step order and timeout/error classification (#612). */ async function runIcaclsAsync(targetPath: string, directory: boolean, deadline: number): Promise { - const user = currentWindowsUser(); - if (!user) { - throw new Error("Cannot determine current Windows user for ACL hardening"); - } + const principal = await currentWindowsPrincipalAsync(deadline); const run = async (step: string, args: string[]): Promise => { const remaining = deadline - nowFn(); @@ -490,7 +493,7 @@ async function runIcaclsAsync(targetPath: string, directory: boolean, deadline: if (!result.success) throw icaclsError(step, result); }; - await runOrThrow("/grant:r", [targetPath, "/grant:r", grantAce(user, directory)]); + await runOrThrow("/grant:r", [targetPath, "/grant:r", grantAce(principal, directory)]); await runOrThrow("/inheritance:r", [targetPath, "/inheritance:r"]); const removal = await run("/remove:g", [targetPath, "/remove:g", ...BROAD_SIDS]); @@ -524,6 +527,8 @@ function sanitizeDiagnostics(error: unknown): string { return `ACL hardening failed (${code}) — permission denied running icacls`; case "EICACLS": return "ACL hardening failed (EICACLS) — icacls command error; filesystem may not support per-user NTFS ACLs"; + case "EACLIDENTITY": + return "ACL hardening failed (EACLIDENTITY) — the effective Windows account SID could not be resolved"; default: return `ACL hardening failed${code ? ` (${code})` : ""} — filesystem may not support per-user NTFS ACLs`; } diff --git a/src/lib/windows-user-principal.ts b/src/lib/windows-user-principal.ts new file mode 100644 index 000000000..e94c45f7d --- /dev/null +++ b/src/lib/windows-user-principal.ts @@ -0,0 +1,215 @@ +/** + * Resolve the effective Windows token to the locale-independent SID form that + * icacls accepts ("*S-1-..."). Environment values such as USERDOMAIN are not + * an authority for the current token: on workgroup machines USERDOMAIN may be + * the literal WORKGROUP even though the account belongs to the local computer. + */ + +import { resolveTrustedWindowsPowerShellExe } from "./windows-elevation"; + +const SID_PATTERN = /^S-1-(?:\d+-)+\d+$/i; +const SID_EXPRESSION = + "[System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value"; + +export interface WindowsPrincipalLookupResult { + success: boolean; + exitCode: number | null; + timedOut: boolean; + stdout: string; +} + +export type WindowsPrincipalRunner = ( + timeoutMs: number, +) => WindowsPrincipalLookupResult; + +export type AsyncWindowsPrincipalRunner = ( + timeoutMs: number, +) => Promise; + +const POWERSHELL_ARGS = [ + "-NoLogo", + "-NoProfile", + "-NonInteractive", + "-WindowStyle", + "Hidden", + "-Command", + SID_EXPRESSION, +] as const; + +function windowsPrincipalPowerShellCommand(): string[] { + return [resolveTrustedWindowsPowerShellExe(), ...POWERSHELL_ARGS]; +} + +/** Test-only readback of the exact trusted executable and static arguments. */ +export function windowsPrincipalPowerShellCommandForTests(): string[] { + return windowsPrincipalPowerShellCommand(); +} + +function defaultWindowsPrincipalRunner(timeoutMs: number): WindowsPrincipalLookupResult { + const result = Bun.spawnSync(windowsPrincipalPowerShellCommand(), { + stdin: "ignore", + stdout: "pipe", + stderr: "ignore", + timeout: Math.max(1, timeoutMs), + windowsHide: true, + }); + return { + success: result.success, + exitCode: result.exitCode, + timedOut: result.exitedDueToTimeout ?? false, + stdout: result.stdout ? result.stdout.toString() : "", + }; +} + +async function defaultAsyncWindowsPrincipalRunner( + timeoutMs: number, +): Promise { + const proc = Bun.spawn(windowsPrincipalPowerShellCommand(), { + stdin: "ignore", + stdout: "pipe", + stderr: "ignore", + windowsHide: true, + }); + let timedOut = false; + const timer = setTimeout(() => { + timedOut = true; + try { proc.kill(); } catch { /* already exited */ } + }, Math.max(1, timeoutMs)); + let exitCode: number | null = null; + try { + exitCode = await proc.exited; + } finally { + clearTimeout(timer); + } + const stdout = proc.stdout + ? await new Response(proc.stdout).text().catch(() => "") + : ""; + return { + success: !timedOut && exitCode === 0, + exitCode: timedOut ? null : exitCode, + timedOut, + stdout, + }; +} + +let principalRunner: WindowsPrincipalRunner = defaultWindowsPrincipalRunner; +let asyncPrincipalRunner: AsyncWindowsPrincipalRunner = defaultAsyncWindowsPrincipalRunner; +let cachedPrincipal: string | null = null; +let asyncLookupInFlight: Promise | null = null; + +function identityError(reason: string): NodeJS.ErrnoException { + const error = new Error(`Windows effective-account SID lookup ${reason}`) as NodeJS.ErrnoException; + // Keep identity lookup failures distinct from icacls timeouts. In particular, + // they must not populate windows-secret-acl's destination timeout memo. + error.code = "EACLIDENTITY"; + return error; +} + +function principalFromResult(result: WindowsPrincipalLookupResult): string { + if (!result.success) { + throw identityError(result.timedOut + ? "timed out" + : `exited ${result.exitCode ?? "null"}`); + } + const sid = result.stdout.trim(); + if (!SID_PATTERN.test(sid)) { + throw identityError(sid ? "returned an invalid SID" : "returned an empty SID"); + } + return `*${sid.toUpperCase()}`; +} + +/** Resolve and process-cache the effective token SID for synchronous ACL paths. */ +export function resolveCurrentWindowsPrincipal(timeoutMs: number): string { + if (cachedPrincipal) return cachedPrincipal; + if (timeoutMs <= 0) throw identityError("had no remaining deadline"); + let result: WindowsPrincipalLookupResult; + try { + result = principalRunner(timeoutMs); + } catch { + throw identityError("could not start"); + } + const principal = principalFromResult(result); + cachedPrincipal = principal; + return principal; +} + +async function waitForExistingLookup( + lookup: Promise, + timeoutMs: number, +): Promise { + if (timeoutMs <= 0) throw identityError("had no remaining deadline"); + let timer: ReturnType | undefined; + try { + return await Promise.race([ + lookup, + new Promise((_, reject) => { + timer = setTimeout( + () => reject(identityError("timed out while awaiting the shared lookup")), + Math.max(1, timeoutMs), + ); + }), + ]); + } finally { + if (timer) clearTimeout(timer); + } +} + +/** + * Async counterpart. Concurrent callers share one owned child lookup; a later + * caller may exhaust its own budget without cancelling the lookup owned by the + * first caller. The first caller owns that child and its process timeout; a + * later, longer budget deliberately does not extend an already-running child. + */ +export async function resolveCurrentWindowsPrincipalAsync(timeoutMs: number): Promise { + if (cachedPrincipal) return cachedPrincipal; + if (asyncLookupInFlight) return waitForExistingLookup(asyncLookupInFlight, timeoutMs); + if (timeoutMs <= 0) throw identityError("had no remaining deadline"); + + const lookup = (async (): Promise => { + let result: WindowsPrincipalLookupResult; + try { + result = await asyncPrincipalRunner(timeoutMs); + } catch { + throw identityError("could not start"); + } + const principal = principalFromResult(result); + cachedPrincipal = principal; + return principal; + })(); + asyncLookupInFlight = lookup; + try { + return await lookup; + } finally { + if (asyncLookupInFlight === lookup) asyncLookupInFlight = null; + } +} + +/** Test seam: replace the sync resolver process and clear its successful cache. */ +export function setWindowsPrincipalRunnerForTests( + runner: WindowsPrincipalRunner | null, +): void { + if (asyncLookupInFlight) { + throw new Error("Cannot replace the Windows principal runner while a lookup is in flight."); + } + principalRunner = runner ?? defaultWindowsPrincipalRunner; + cachedPrincipal = null; +} + +/** Test seam: replace the async resolver process and clear its successful cache. */ +export function setAsyncWindowsPrincipalRunnerForTests( + runner: AsyncWindowsPrincipalRunner | null, +): void { + if (asyncLookupInFlight) { + throw new Error("Cannot replace the Windows principal runner while a lookup is in flight."); + } + asyncPrincipalRunner = runner ?? defaultAsyncWindowsPrincipalRunner; + cachedPrincipal = null; +} + +/** Test seam: clear only process-local principal state. */ +export function resetWindowsPrincipalForTests(): void { + if (asyncLookupInFlight) { + throw new Error("Cannot reset the Windows principal while a lookup is in flight."); + } + cachedPrincipal = null; +} diff --git a/tests/windows-secret-acl.test.ts b/tests/windows-secret-acl.test.ts index acf3cb343..430fe0d75 100644 --- a/tests/windows-secret-acl.test.ts +++ b/tests/windows-secret-acl.test.ts @@ -36,6 +36,10 @@ import { atomicWriteFile } from "../src/config"; import { hardenStableLockFile } from "../src/codex/native-main-lock-file"; import { nativeMainClaimPath, withNativeMainSharedClaim } from "../src/codex/native-main-claim"; import { NATIVE_MAIN_OWNER_DB, retainNativeMainOwner } from "../src/codex/native-main-owner"; +import { + resetWindowsPrincipalForTests, + setWindowsPrincipalRunnerForTests, +} from "../src/lib/windows-user-principal"; let testDir = ""; @@ -116,6 +120,77 @@ describe("hardenSecretPath – required mode (required: true)", () => { }); }); +describe("effective Windows principal integration", () => { + test("the owner grant uses a numeric SID even when USERDOMAIN says WORKGROUP", () => { + const filePath = join(testDir, "workgroup-secret.json"); + writeFileSync(filePath, "data", "utf-8"); + const oldDomain = process.env.USERDOMAIN; + const oldUser = process.env.USERNAME; + process.env.USERDOMAIN = "WORKGROUP"; + process.env.USERNAME = "not-authoritative"; + resetHardenedStateForTests(); + setPlatformForTests("win32"); + const seen: string[][] = []; + setIcaclsRunnerForTests(args => { + seen.push(args); + return { success: true, exitCode: 0, timedOut: false, stdout: "" }; + }); + try { + expect(hardenSecretPath(filePath, { required: true })).toEqual({ ok: true }); + const grant = seen.find(args => args.includes("/grant:r")); + expect(grant).toBeDefined(); + expect(grant![2]).toMatch(/^\*S-1-(?:\d+-)+\d+:\(F\)$/i); + expect(grant![2]).not.toContain("WORKGROUP"); + } finally { + setIcaclsRunnerForTests(null); + setPlatformForTests(null); + resetHardenedStateForTests(); + if (oldDomain === undefined) delete process.env.USERDOMAIN; + else process.env.USERDOMAIN = oldDomain; + if (oldUser === undefined) delete process.env.USERNAME; + else process.env.USERNAME = oldUser; + } + }); + + test("identity lookup failure is fail-closed but never memoized as an icacls timeout", () => { + if (process.platform !== "win32") return; + const requiredPath = join(testDir, "identity-required.json"); + const optionalPath = join(testDir, "identity-optional.json"); + writeFileSync(requiredPath, "required", "utf-8"); + writeFileSync(optionalPath, "optional", "utf-8"); + let identityCalls = 0; + let icaclsCalls = 0; + setWindowsPrincipalRunnerForTests(() => { + identityCalls += 1; + return { success: false, exitCode: null, timedOut: true, stdout: "" }; + }); + setIcaclsRunnerForTests(() => { + icaclsCalls += 1; + return { success: true, exitCode: 0, timedOut: false, stdout: "" }; + }); + resetHardenedStateForTests(); + setPlatformForTests("win32"); + try { + expect(() => hardenSecretPath(requiredPath, { required: true })) + .toThrow(/EACLIDENTITY/); + expect(hardenSecretPath(optionalPath, { required: false })).toEqual({ + ok: false, + diagnostics: + "ACL hardening failed (EACLIDENTITY) — the effective Windows account SID could not be resolved", + }); + expect(identityCalls).toBe(2); + expect(icaclsCalls).toBe(0); + expect(timedOutSecretPathCountForTests()).toBe(0); + } finally { + setIcaclsRunnerForTests(null); + setPlatformForTests(null); + resetHardenedStateForTests(); + setWindowsPrincipalRunnerForTests(null); + resetWindowsPrincipalForTests(); + } + }); +}); + describe("ephemeral harden success memo lifecycle", () => { test("forgetHardenedSecretPath releases only the actual temp and a second temp hardens again", () => { // Earlier cases in this file harden real paths under the win32 override and diff --git a/tests/windows-user-principal.test.ts b/tests/windows-user-principal.test.ts new file mode 100644 index 000000000..c0d01cc57 --- /dev/null +++ b/tests/windows-user-principal.test.ts @@ -0,0 +1,131 @@ +import { afterEach, describe, expect, test } from "bun:test"; + +import { + resetWindowsPrincipalForTests, + resolveCurrentWindowsPrincipal, + resolveCurrentWindowsPrincipalAsync, + setAsyncWindowsPrincipalRunnerForTests, + setWindowsPrincipalRunnerForTests, + windowsPrincipalPowerShellCommandForTests, +} from "../src/lib/windows-user-principal"; +import { setTrustedWindowsElevationExecutablesForTests } from "../src/lib/windows-elevation"; + +const ok = (stdout = "S-1-5-21-111-222-333-1001\r\n") => ({ + success: true, + exitCode: 0, + timedOut: false, + stdout, +}); + +afterEach(() => { + setWindowsPrincipalRunnerForTests(null); + setAsyncWindowsPrincipalRunnerForTests(null); + setTrustedWindowsElevationExecutablesForTests(null); + resetWindowsPrincipalForTests(); +}); + +describe("Windows effective ACL principal", () => { + test("builds a hidden non-interactive command from the trusted PowerShell path", () => { + const trusted = "C:\\trusted-system32\\WindowsPowerShell\\v1.0\\powershell.exe"; + setTrustedWindowsElevationExecutablesForTests({ powershell: trusted }); + expect(windowsPrincipalPowerShellCommandForTests()).toEqual([ + trusted, + "-NoLogo", + "-NoProfile", + "-NonInteractive", + "-WindowStyle", + "Hidden", + "-Command", + "[System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value", + ]); + }); + + test("the default trusted runner resolves the real token on Windows", () => { + if (process.platform !== "win32") return; + expect(resolveCurrentWindowsPrincipal(5_000)).toMatch(/^\*S-1-(?:\d+-)+\d+$/i); + }); + + test("the default trusted async runner settles and resolves the real token on Windows", async () => { + if (process.platform !== "win32") return; + expect(await resolveCurrentWindowsPrincipalAsync(5_000)) + .toMatch(/^\*S-1-(?:\d+-)+\d+$/i); + }); + + test("uses the token SID and normalizes it for icacls, independent of WORKGROUP env", () => { + const oldDomain = process.env.USERDOMAIN; + const oldUser = process.env.USERNAME; + process.env.USERDOMAIN = "WORKGROUP"; + process.env.USERNAME = "not-the-token-authority"; + setWindowsPrincipalRunnerForTests(() => ok()); + try { + expect(resolveCurrentWindowsPrincipal(1_000)).toBe("*S-1-5-21-111-222-333-1001"); + } finally { + if (oldDomain === undefined) delete process.env.USERDOMAIN; + else process.env.USERDOMAIN = oldDomain; + if (oldUser === undefined) delete process.env.USERNAME; + else process.env.USERNAME = oldUser; + } + }); + + test("caches only a successful lookup", () => { + let calls = 0; + setWindowsPrincipalRunnerForTests(() => { + calls += 1; + return ok(); + }); + expect(resolveCurrentWindowsPrincipal(1_000)).toMatch(/^\*S-1-/); + expect(resolveCurrentWindowsPrincipal(1_000)).toMatch(/^\*S-1-/); + expect(calls).toBe(1); + }); + + test("invalid output fails closed and is retried rather than cached", () => { + let calls = 0; + setWindowsPrincipalRunnerForTests(() => { + calls += 1; + return ok("WORKGROUP\\user\n"); + }); + for (let attempt = 0; attempt < 2; attempt += 1) { + try { + resolveCurrentWindowsPrincipal(1_000); + throw new Error("expected identity refusal"); + } catch (error) { + expect((error as NodeJS.ErrnoException).code).toBe("EACLIDENTITY"); + } + } + expect(calls).toBe(2); + }); + + test("a resolver timeout stays EACLIDENTITY rather than entering the icacls timeout class", () => { + setWindowsPrincipalRunnerForTests(() => ({ + success: false, + exitCode: null, + timedOut: true, + stdout: "", + })); + try { + resolveCurrentWindowsPrincipal(1_000); + throw new Error("expected identity refusal"); + } catch (error) { + expect((error as NodeJS.ErrnoException).code).toBe("EACLIDENTITY"); + } + }); + + test("concurrent async callers share one owned lookup", async () => { + let calls = 0; + let release!: () => void; + const gate = new Promise(resolve => { release = resolve; }); + setAsyncWindowsPrincipalRunnerForTests(async () => { + calls += 1; + await gate; + return ok(); + }); + + const first = resolveCurrentWindowsPrincipalAsync(2_000); + const second = resolveCurrentWindowsPrincipalAsync(2_000); + await Bun.sleep(0); + expect(calls).toBe(1); + release(); + await expect(first).resolves.toBe("*S-1-5-21-111-222-333-1001"); + await expect(second).resolves.toBe("*S-1-5-21-111-222-333-1001"); + }); +});