From 7735a0b85c72a39b4cb88793e5f46e0f73538b87 Mon Sep 17 00:00:00 2001 From: Byungsker <72309817+byungsker@users.noreply.github.com> Date: Sun, 8 Mar 2026 02:49:33 +0900 Subject: [PATCH] fix(security): use icacls /sid for locale-independent Windows ACL audit (#38900) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(security): use icacls /sid for locale-independent Windows ACL audit On non-English Windows editions (Russian, Chinese, etc.) icacls prints account names in the system locale. When Node.js reads the output in a different code page the strings are garbled (e.g. "NT AUTHORITY\???????" for "NT AUTHORITY\СИСТЕМА"), causing summarizeWindowsAcl to classify SYSTEM and Administrators as untrusted and flag the config files as "others writable" — a false-positive security alert. Fix: 1. Pass /sid to icacls so it outputs security identifiers (*S-1-5-X-...) instead of locale-dependent account names. 2. Extend SID_RE to accept the leading * that icacls prepends to SIDs in /sid mode: /^\*?s-\d+-\d+(-\d+)+$/i 3. Strip the * before looking up the bare SID in TRUSTED_SIDS / the per-user USERSID set so *S-1-5-18 is correctly classified as SYSTEM (trusted) and *S-1-5-32-544 as Administrators (trusted). Tests: - Update the inspectWindowsAcl "returns parsed ACL entries" assertion to expect the /sid flag in the icacls call. - Add "classifies *S-1-5-18 (icacls /sid prefix form of SYSTEM) as trusted" SID classification test. - Add "classifies *S-1-5-32-544 (icacls /sid Administrators) as trusted". - Add inspectWindowsAcl end-to-end test with /sid-format mock output (*S-1-5-18, *S-1-5-32-544, user SID) — all three classified as trusted. Fixes #35834 * fix(security): classify world-equivalent SIDs as 'world' when using icacls /sid When icacls is invoked with /sid, world-equivalent principals like Everyone, Authenticated Users, and BUILTIN\Users are emitted as raw SIDs (*S-1-1-0, *S-1-5-11, *S-1-5-32-545). classifyPrincipal() had no SID-based mapping for these, so they fell through to the generic 'group' category instead of 'world', silently downgrading security findings that should trigger world-write/world-readable alerts. Fix: add a WORLD_SIDS constant and check it before falling back to 'group'. Add three regression tests to lock in the behaviour. * Security: resolve owner SID fallback for Windows ACL audit --------- Co-authored-by: Vincent Koc --- src/security/windows-acl.test.ts | 126 ++++++++++++++++++++++++++++++- src/security/windows-acl.ts | 62 +++++++++++++-- 2 files changed, 182 insertions(+), 6 deletions(-) diff --git a/src/security/windows-acl.test.ts b/src/security/windows-acl.test.ts index 5f7b86da8f5..f9cb67fa4e5 100644 --- a/src/security/windows-acl.test.ts +++ b/src/security/windows-acl.test.ts @@ -244,6 +244,20 @@ Successfully processed 1 files`; expectTrustedOnly([aclEntry({ principal: "S-1-5-18" })]); }); + it("classifies *S-1-5-18 (icacls /sid prefix form of SYSTEM) as trusted (refs #35834)", () => { + // icacls /sid output prefixes SIDs with *, e.g. *S-1-5-18 instead of + // S-1-5-18. Without this fix the asterisk caused SID_RE to not match + // and the SYSTEM entry was misclassified as "group" (untrusted). + expectTrustedOnly([aclEntry({ principal: "*S-1-5-18" })]); + }); + + it("classifies *S-1-5-32-544 (icacls /sid Administrators) as trusted", () => { + const entries: WindowsAclEntry[] = [aclEntry({ principal: "*S-1-5-32-544" })]; + const summary = summarizeWindowsAcl(entries); + expect(summary.trusted).toHaveLength(1); + expect(summary.untrustedGroup).toHaveLength(0); + }); + it("classifies BUILTIN\\Administrators SID (S-1-5-32-544) as trusted", () => { const entries: WindowsAclEntry[] = [aclEntry({ principal: "S-1-5-32-544" })]; const summary = summarizeWindowsAcl(entries); @@ -265,6 +279,21 @@ Successfully processed 1 files`; ); }); + it("does not trust *-prefixed Everyone via USERSID", () => { + const entries: WindowsAclEntry[] = [ + { + principal: "*S-1-1-0", + rights: ["R"], + rawRights: "(R)", + canRead: true, + canWrite: false, + }, + ]; + const summary = summarizeWindowsAcl(entries, { USERSID: "*S-1-1-0" }); + expect(summary.untrustedWorld).toHaveLength(1); + expect(summary.trusted).toHaveLength(0); + }); + it("classifies unknown SID as group (not world)", () => { const entries: WindowsAclEntry[] = [ { @@ -281,6 +310,53 @@ Successfully processed 1 files`; expect(summary.trusted).toHaveLength(0); }); + it("classifies Everyone SID (S-1-1-0) as world, not group", () => { + // When icacls is run with /sid, "Everyone" becomes *S-1-1-0. + // It must be classified as "world" to preserve security-audit severity. + const entries: WindowsAclEntry[] = [ + { + principal: "*S-1-1-0", + rights: ["R"], + rawRights: "(R)", + canRead: true, + canWrite: false, + }, + ]; + const summary = summarizeWindowsAcl(entries); + expect(summary.untrustedWorld).toHaveLength(1); + expect(summary.untrustedGroup).toHaveLength(0); + }); + + it("classifies Authenticated Users SID (S-1-5-11) as world, not group", () => { + const entries: WindowsAclEntry[] = [ + { + principal: "*S-1-5-11", + rights: ["R"], + rawRights: "(R)", + canRead: true, + canWrite: false, + }, + ]; + const summary = summarizeWindowsAcl(entries); + expect(summary.untrustedWorld).toHaveLength(1); + expect(summary.untrustedGroup).toHaveLength(0); + }); + + it("classifies BUILTIN\\Users SID (S-1-5-32-545) as world, not group", () => { + const entries: WindowsAclEntry[] = [ + { + principal: "*S-1-5-32-545", + rights: ["R"], + rawRights: "(R)", + canRead: true, + canWrite: false, + }, + ]; + const summary = summarizeWindowsAcl(entries); + expect(summary.untrustedWorld).toHaveLength(1); + expect(summary.untrustedGroup).toHaveLength(0); + }); + it("full scenario: SYSTEM SID + owner SID only → no findings", () => { const ownerSid = "S-1-5-21-1824257776-4070701511-781240313-1001"; const entries: WindowsAclEntry[] = [ @@ -319,7 +395,55 @@ Successfully processed 1 files`; exec: mockExec, }); expectInspectSuccess(result, 2); - expect(mockExec).toHaveBeenCalledWith("icacls", ["C:\\test\\file.txt"]); + // /sid is passed so that account names are printed as SIDs, making the + // audit locale-independent (fixes #35834). + expect(mockExec).toHaveBeenCalledWith("icacls", ["C:\\test\\file.txt", "/sid"]); + }); + + it("classifies *S-1-5-18 (SID form of SYSTEM from /sid) as trusted", async () => { + // When icacls is called with /sid it outputs *S-X-X-X instead of + // locale-dependent names like "NT AUTHORITY\\SYSTEM" or the Russian + // garbled equivalent. + const mockExec = vi.fn().mockResolvedValue({ + stdout: + "C:\\test\\file.txt *S-1-5-21-111-222-333-1001:(F)\n *S-1-5-18:(F)\n *S-1-5-32-544:(F)", + stderr: "", + }); + + const result = await inspectWindowsAcl("C:\\test\\file.txt", { + exec: mockExec, + env: { USERSID: "S-1-5-21-111-222-333-1001" }, + }); + expectInspectSuccess(result, 3); + // All three entries (current user, SYSTEM, Administrators) must be trusted. + expect(result.trusted).toHaveLength(3); + expect(result.untrustedGroup).toHaveLength(0); + expect(result.untrustedWorld).toHaveLength(0); + }); + + it("resolves current user SID via whoami when USERSID is missing", async () => { + const mockExec = vi + .fn() + .mockResolvedValueOnce({ + stdout: + "C:\\test\\file.txt *S-1-5-21-111-222-333-1001:(F)\n *S-1-5-18:(F)", + stderr: "", + }) + .mockResolvedValueOnce({ + stdout: '"mock-host\\\\MockUser","S-1-5-21-111-222-333-1001"\r\n', + stderr: "", + }); + + const result = await inspectWindowsAcl("C:\\test\\file.txt", { + exec: mockExec, + env: { USERNAME: "MockUser", USERDOMAIN: "mock-host" }, + }); + + expectInspectSuccess(result, 2); + expect(result.trusted).toHaveLength(2); + expect(result.untrustedGroup).toHaveLength(0); + expect(mockExec).toHaveBeenNthCalledWith(1, "icacls", ["C:\\test\\file.txt", "/sid"]); + expect(mockExec).toHaveBeenNthCalledWith(2, "whoami", ["/user", "/fo", "csv", "/nh"]); }); it("returns error state on exec failure", async () => { diff --git a/src/security/windows-acl.ts b/src/security/windows-acl.ts index 64e415cca32..c7580bbc42c 100644 --- a/src/security/windows-acl.ts +++ b/src/security/windows-acl.ts @@ -42,12 +42,20 @@ const TRUSTED_BASE = new Set([ const WORLD_SUFFIXES = ["\\users", "\\authenticated users"]; const TRUSTED_SUFFIXES = ["\\administrators", "\\system", "\\système"]; -const SID_RE = /^s-\d+-\d+(-\d+)+$/i; +// Accept an optional leading * which icacls prefixes to SIDs when invoked with /sid +// (e.g. *S-1-5-18 instead of S-1-5-18). +const SID_RE = /^\*?s-\d+-\d+(-\d+)+$/i; const TRUSTED_SIDS = new Set([ "s-1-5-18", "s-1-5-32-544", "s-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464", ]); +// SIDs for world-equivalent principals that icacls /sid emits as raw SIDs. +// Without this list these would be classified as "group" instead of "world". +// S-1-1-0 Everyone +// S-1-5-11 Authenticated Users +// S-1-5-32-545 BUILTIN\Users +const WORLD_SIDS = new Set(["s-1-1-0", "s-1-5-11", "s-1-5-32-545"]); const STATUS_PREFIXES = [ "successfully processed", "processed", @@ -57,6 +65,11 @@ const STATUS_PREFIXES = [ const normalize = (value: string) => value.trim().toLowerCase(); +function normalizeSid(value: string): string { + const normalized = normalize(value); + return normalized.startsWith("*") ? normalized.slice(1) : normalized; +} + export function resolveWindowsUserPrincipal(env?: NodeJS.ProcessEnv): string | null { const username = env?.USERNAME?.trim() || os.userInfo().username?.trim(); if (!username) { @@ -77,7 +90,7 @@ function buildTrustedPrincipals(env?: NodeJS.ProcessEnv): Set { trusted.add(normalize(userOnly)); } } - const userSid = normalize(env?.USERSID ?? ""); + const userSid = normalizeSid(env?.USERSID ?? ""); if (userSid && SID_RE.test(userSid)) { trusted.add(userSid); } @@ -91,7 +104,18 @@ function classifyPrincipal( const normalized = normalize(principal); if (SID_RE.test(normalized)) { - return TRUSTED_SIDS.has(normalized) || trustedPrincipals.has(normalized) ? "trusted" : "group"; + // Strip the leading * that icacls /sid prefixes to SIDs before lookup. + const sid = normalizeSid(normalized); + // World-equivalent SIDs must be classified as "world", not "group", so + // that callers applying world-write policies catch everyone/authenticated- + // users entries the same way they would catch the human-readable names. + if (WORLD_SIDS.has(sid)) { + return "world"; + } + if (TRUSTED_SIDS.has(sid) || trustedPrincipals.has(sid)) { + return "trusted"; + } + return "group"; } if ( @@ -243,16 +267,44 @@ export function summarizeWindowsAcl( return { trusted, untrustedWorld, untrustedGroup }; } +async function resolveCurrentUserSid(exec: ExecFn): Promise { + try { + const { stdout, stderr } = await exec("whoami", ["/user", "/fo", "csv", "/nh"]); + const match = `${stdout}\n${stderr}`.match(/\*?S-\d+-\d+(?:-\d+)+/i); + return match ? normalizeSid(match[0]) : null; + } catch { + return null; + } +} + export async function inspectWindowsAcl( targetPath: string, opts?: { env?: NodeJS.ProcessEnv; exec?: ExecFn }, ): Promise { const exec = opts?.exec ?? runExec; try { - const { stdout, stderr } = await exec("icacls", [targetPath]); + // /sid outputs security identifiers (e.g. *S-1-5-18) instead of locale- + // dependent account names so the audit works correctly on non-English + // Windows (Russian, Chinese, etc.) where icacls prints Cyrillic / CJK + // characters that may be garbled when Node reads them in the wrong code + // page. Fixes #35834. + const { stdout, stderr } = await exec("icacls", [targetPath, "/sid"]); const output = `${stdout}\n${stderr}`.trim(); const entries = parseIcaclsOutput(output, targetPath); - const { trusted, untrustedWorld, untrustedGroup } = summarizeWindowsAcl(entries, opts?.env); + let effectiveEnv = opts?.env; + let { trusted, untrustedWorld, untrustedGroup } = summarizeWindowsAcl(entries, effectiveEnv); + + const needsUserSidResolution = + !effectiveEnv?.USERSID && + untrustedGroup.some((entry) => SID_RE.test(normalize(entry.principal))); + if (needsUserSidResolution) { + const currentUserSid = await resolveCurrentUserSid(exec); + if (currentUserSid) { + effectiveEnv = { ...effectiveEnv, USERSID: currentUserSid }; + ({ trusted, untrustedWorld, untrustedGroup } = summarizeWindowsAcl(entries, effectiveEnv)); + } + } + return { ok: true, entries, trusted, untrustedWorld, untrustedGroup }; } catch (err) { return {