diff --git a/src/lib/windows-secret-acl.ts b/src/lib/windows-secret-acl.ts index f8cc2ec90b8..cc3b701bcb2 100644 --- a/src/lib/windows-secret-acl.ts +++ b/src/lib/windows-secret-acl.ts @@ -51,13 +51,15 @@ type HardenedIdentity = string; /** * What a stat can tell us about WHICH OBJECT is at a path. * - * Two fields, deliberately separated, because conflating them shipped a bug: + * Three fields, deliberately separated, because conflating them shipped a bug: * * - `object` — `dev:ino`. Answers "is this the same file". Survives an ACL or * permission change, which is exactly what we need across an icacls call. * - `freshness` — `ctimeNs`. Answers "has this file's metadata moved since". It * distinguishes an unlink/recreate that ext4 gave the same inode back for, and * it MOVES when permissions change. + * - `generation` — `birthtimeNs`. Distinguishes same-`dev:ino` replacement + * across icacls while remaining stable when only the ACL changes. * * The first version used `dev:ino:ctimeNs` for both jobs. Since chmod bumps ctime * — probed, `{ctimeChangedByChmod: true}` — and icacls is a permission change, the @@ -69,6 +71,8 @@ type HardenedIdentity = string; interface PathObservation { readonly object: string; readonly freshness: string; + /** Creation time is stable across ACL edits but changes on unlink/recreate. */ + readonly generation: string | null; } /** @@ -92,25 +96,40 @@ interface PathObservation { * Darwin nor Linux CI can confirm it and no pinned-Bun Windows probe has run. * It is defensive code, not a demonstrated platform fact. */ -type StatReader = (path: string) => { dev: bigint; ino: bigint; ctimeNs: bigint }; +type StatReader = (path: string) => { + dev: bigint; + ino: bigint; + ctimeNs: bigint; + birthtimeNs?: bigint; +}; const defaultStatReader: StatReader = path => { const s = statSync(path, { bigint: true }); - return { dev: s.dev, ino: s.ino, ctimeNs: s.ctimeNs }; + return { dev: s.dev, ino: s.ino, ctimeNs: s.ctimeNs, birthtimeNs: s.birthtimeNs }; }; let statReader: StatReader = defaultStatReader; /** Test seam: drive dev / ino / ctime independently. */ export function setStatForTests(reader: StatReader | null): void { - statReader = reader ?? defaultStatReader; + // Older tests predate the creation-generation check. Give observations that + // omit it one stable, nonzero generation; race tests can vary it explicitly. + statReader = reader === null + ? defaultStatReader + : path => ({ birthtimeNs: 1n, ...reader(path) }); } function observe(targetPath: string): PathObservation | null { try { const s = statReader(targetPath); if (s.ino === 0n) return null; - return { object: `${s.dev}:${s.ino}`, freshness: `${s.ctimeNs}` }; + return { + object: `${s.dev}:${s.ino}`, + freshness: `${s.ctimeNs}`, + generation: s.birthtimeNs === undefined || s.birthtimeNs === 0n + ? null + : `${s.birthtimeNs}`, + }; } catch { return null; } @@ -162,12 +181,10 @@ function memoSatisfied(cache: Map, targetPath: string) * {identityChangedDuringHarden: true, callsForOriginal: 3, totalCalls: 3, * replacementWasHardened: false} * - * So the OBJECT is captured before the sequence and compared after it. Only the - * object — `dev:ino` — because icacls changes permissions, and `ctimeNs` moves - * when permissions change (probed: `{ctimeChangedByChmod: true}`). Comparing the - * full identity across the call would have rejected every successful harden and - * failed closed on the first harden on Windows: the check would have been - * demanding that an operation not do the thing it exists to do. + * So the object and its creation generation are captured before the sequence + * and compared after it. `ctimeNs` cannot be used for that comparison because + * icacls itself changes it. Creation time, unlike ctime, survives an ACL edit; + * it closes the same-dev:ino reuse gap without rejecting successful hardening. * * The memo then stores the object plus the freshness read AFTER hardening, which * is the state a later lookup should match. @@ -185,7 +202,12 @@ function recordHarden( before: PathObservation | null, ): boolean { const after = observe(targetPath); - if (before === null || after === null || before.object !== after.object) { + if ( + before === null + || after === null + || before.object !== after.object + || before.generation !== after.generation + ) { // Never leave a memo behind for a file we cannot vouch for, including one // written by an earlier successful harden of a now-replaced file. cache.delete(targetPath); diff --git a/tests/windows-secret-acl.test.ts b/tests/windows-secret-acl.test.ts index 26e07df0f95..c4f30a5f94a 100644 --- a/tests/windows-secret-acl.test.ts +++ b/tests/windows-secret-acl.test.ts @@ -813,7 +813,7 @@ for (const { label, harden, create } of ENTRY_POINTS) { // an unlinked inode immediately (100/100 cycles) while APFS recycled none // in 200, so a real-file version of this test asserts different things on // different platforms — it passed on macOS with a fix broken on Linux. - let current = { dev: 1n, ino: 10n, ctimeNs: 100n }; + let current = { dev: 1n, ino: 10n, ctimeNs: 100n, birthtimeNs: 50n }; setStatForTests(() => current); expect(await harden(stable, { required: true })).toEqual({ ok: true }); @@ -843,7 +843,9 @@ for (const { label, harden, create } of ENTRY_POINTS) { runner(args => { if (args.includes("/grant:r")) grants += 1; // Another process replaces the file before the sequence returns. - if (args.includes("/remove:g")) current = { dev: 1n, ino: 99n, ctimeNs: 500n }; + if (args.includes("/remove:g")) { + current = { dev: 1n, ino: 10n, ctimeNs: 500n, birthtimeNs: 400n }; + } }); await expect(harden(stable, { required: true })).rejects.toThrow( @@ -856,7 +858,7 @@ for (const { label, harden, create } of ENTRY_POINTS) { // An optional caller hitting the same race soft-fails with the same // honest diagnostic. The runner keeps swapping the file, so this second // attempt races too rather than settling on the replacement. - current = { dev: 1n, ino: 10n, ctimeNs: 100n }; + current = { dev: 1n, ino: 10n, ctimeNs: 100n, birthtimeNs: 50n }; const optional = await harden(stable, { required: false }); expect(optional.ok).toBe(false); expect(optional.diagnostics).toMatch(/changed during hardening/); @@ -879,7 +881,7 @@ for (const { label, harden, create } of ENTRY_POINTS) { await withWin32(async () => { let grants = 0; let ctime = 100n; - setStatForTests(() => ({ dev: 1n, ino: 10n, ctimeNs: ctime })); + setStatForTests(() => ({ dev: 1n, ino: 10n, ctimeNs: ctime, birthtimeNs: 50n })); runner(args => { if (args.includes("/grant:r")) grants += 1; ctime += 1n; // editing the DACL moves ctime; same file throughout