mirror of
https://github.com/openclaw/openclaw.git
synced 2026-03-12 07:20:45 +00:00
fix(security): use icacls /sid for locale-independent Windows ACL audit (#38900)
* 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 <vincentkoc@ieee.org>
This commit is contained in:
Byungsker
@@ -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 () => {
|
||||
|
||||
@@ -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<string> {
|
||||
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<string | null> {
|
||||
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<WindowsAclSummary> {
|
||||
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 {
|
||||
|
||||
Reference in New Issue
Block a user