fix(security): use icacls /sid for locale-independent Windows ACL audit by byungsker · Pull Request #38900 · openclaw/openclaw

byungsker · 2026-03-07T13:49:44Z

Problem

On Windows machines with non-English locales (Russian, German, French, Spanish, CJK), icacls prints account names in the system locale (e.g. СИСТЕМА on Russian Windows). When Node.js reads the output, the multi-byte characters may be garbled, causing the auditor to misclassify SYSTEM as an unknown/untrusted principal and raise false-positive security alerts.

Fix

Pass /sid to icacls so it emits canonical ASCII SIDs (*S-1-5-18) instead of locale-specific names. Update:

SID_RE to accept the optional leading * that /sid prepends
classifyPrincipal to strip the * prefix before SID lookup, add WORLD_SIDS constant so world-equivalent SIDs (S-1-1-0 Everyone, S-1-5-11 Authenticated Users, S-1-5-32-545 BUILTIN\Users) are classified as "world" instead of falling through to "group"
formatIcaclsResetCommand to use *S-1-5-18 (SYSTEM SID) instead of the localized name in the reset command

Tests added

*S-1-5-18 (SID form of SYSTEM from /sid) → trusted
*S-1-5-32-544 (BUILTIN\Administrators SID from /sid) → trusted
Full end-to-end mock with /sid output format
Updated icacls call assertion to verify /sid is passed
*S-1-1-0 (Everyone SID) → world (not group)
*S-1-5-11 (Authenticated Users SID) → world (not group)
*S-1-5-32-545 (BUILTIN\Users SID) → world (not group)

CI: 29/29 checks pass ✅

Fixes #35834

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 openclaw#35834

…cacls /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.

greptile-apps · 2026-03-07T13:53:07Z

Greptile Summary

This PR fixes a real locale-dependence bug in the Windows ACL auditor by switching icacls to use the /sid flag, which outputs canonical ASCII SIDs (e.g. *S-1-5-18) instead of locale-dependent account names that could be garbled on non-English Windows. The code changes are well-targeted: SID_RE is widened to accept the * prefix that /sid adds, a new WORLD_SIDS set correctly maps world-equivalent SIDs to "world" classification, and formatIcaclsResetCommand/createIcaclsResetCommand are updated to use the portable SID form. Tests are comprehensive.

Notable issue:

The widened SID_RE (which now accepts a leading *) is also used in buildTrustedPrincipals to validate USERSID from the environment. If USERSID is ever set to a *-prefixed world SID such as *S-1-1-0, that value gets stored verbatim in trustedPrincipals. When classifyPrincipal later encounters *S-1-1-0 from icacls output, the trustedPrincipals.has(normalized) branch matches before the WORLD_SIDS check is reached, silently promoting Everyone to "trusted" and hiding a world-write ACL finding. Stripping any * prefix from USERSID before storing it (see inline comment) closes this gap.

Confidence Score: 4/5

Safe to merge after addressing the USERSID validation regression; the core /sid fix is correct and well-tested.
The fix is logically sound and the test suite is thorough. The single issue — SID_RE now accepting * for USERSID validation, which can bypass the WORLD_SIDS check — is a real but low-likelihood security concern (requires an unusual *-prefixed USERSID env value). No regressions observed in existing behavior.
src/security/windows-acl.ts lines 88–91 (buildTrustedPrincipals): the SID_RE change inadvertently widens USERSID validation to accept *-prefixed SIDs.

Comments Outside Diff (1)

src/security/windows-acl.ts, line 88-91 (link)

SID_RE now accepts *-prefixed values for USERSID validation

SID_RE was widened to accept an optional leading * (needed for icacls /sid output), but that same regex is used here to validate the USERSID environment variable. If USERSID is set to a *-prefixed world-equivalent SID — for example *S-1-1-0 — then *s-1-1-0 gets added to trustedPrincipals. When classifyPrincipal later sees *S-1-1-0 from icacls output it performs:
```
sid          = "s-1-1-0"   // asterisk stripped
trustedPrincipals.has(sid)        → false  (stored as *s-1-1-0)
trustedPrincipals.has(normalized) → true   (stored as *s-1-1-0 ← exact match)
```
The WORLD_SIDS check is never reached, so Everyone is silently promoted to "trusted" rather than "world", hiding a world-write ACL finding.

A minimal fix is to strip any leading * from USERSID before storing it, or to use a tighter regex that does not accept * for this validation path:

_{Last reviewed commit: cd05349}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd05349815

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-03-07T13:55:06Z

src/security/windows-acl.ts

+    // 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"]);


Keep caller ACE trusted when enabling icacls SID mode

Switching inspectWindowsAcl to icacls ... /sid makes the caller's ACE arrive as a SID, but the trust set only includes a caller SID when env.USERSID is populated; in normal audit execution we pass process.env and do not populate USERSID, so the owner entry is reclassified as untrustedGroup and secure paths can be falsely flagged as group-readable/writable. Please add a non-USERSID fallback for resolving the current user SID (or a non-SID fallback path) before forcing SID-only output.

Useful? React with 👍 / 👎.

Jimmy-xuzimo · 2026-03-07T13:56:59Z

📝 PR Review Fix - Windows ACL SID Mode

Hi @byungsker! Excellent work on fixing the locale-independent Windows ACL audit using icacls /sid. I've reviewed the PR and identified one critical issue that should be fixed before merging:

🔴 Critical Issue

Caller ACE Classification Fails Without USERSID

Location: src/security/windows-acl.ts lines 77-92 (buildTrustedPrincipals) and line 276 (inspectWindowsAcl)

Problem:
When icacls /sid is used, the caller's ACE arrives as a SID (e.g., *S-1-5-21-...-1001). However, the buildTrustedPrincipals function only adds the user's SID to the trusted set if env.USERSID is explicitly populated:

function buildTrustedPrincipals(env?: NodeJS.ProcessEnv): Set<string> {
  const trusted = new Set<string>(TRUSTED_BASE);
  const principal = resolveWindowsUserPrincipal(env);
  if (principal) {
    trusted.add(normalize(principal));
    // ... adds username variants
  }
  const userSid = normalize(env?.USERSID ?? "");
  if (userSid && SID_RE.test(userSid)) {
    trusted.add(userSid);  // ❌ Only trusts SID if USERSID env var exists
  }
  return trusted;
}

Impact:

In normal audit execution, inspectWindowsAcl is called with just { env: process.env }
process.env.USERSID is typically not set in normal environments
The caller's SID from icacls /sid output won't match any trusted principal
Result: caller's ACE gets classified as "untrustedGroup" instead of "trusted"
Secure paths can be falsely flagged as group-readable/writable

Example scenario:

// Normal call - USERSID not set
const result = await inspectWindowsAcl("C:\\secret", { env: process.env });

// icacls /sid output includes:
// *S-1-5-21-...-1001:(OI)(CI)(F)  // Caller's SID

// But buildTrustedPrincipals only trusts:
// - TRUSTED_BASE (SYSTEM, Administrators)
// - Username variants (DOMAIN\user, user)
// - NOT the caller's SID (because USERSID env var is missing)

// Result: caller's ACE classified as "untrustedGroup" ❌

✅ Suggested Fix

Add a fallback mechanism to resolve the current user's SID when USERSID env var is not available. Here are two approaches:

Option 1: Use `whoami /user` to get current user SID (Recommended)

export async function resolveCurrentUserSid(): Promise<string | null> {
  try {
    const { stdout } = await exec("whoami", ["/user"]);
    // Output format: username sid
    // Example: DESKTOP-ABC\user *S-1-5-21-...-1001
    const lines = stdout.trim().split(/\r?\n/);
    if (lines.length >= 2) {
      const parts = lines[1].trim().split(/\s+/);
      const sid = parts.at(-1);
      if (sid && SID_RE.test(sid)) {
        return sid;
      }
    }
  } catch {
    // Ignore errors - this is a best-effort fallback
  }
  return null;
}

async function buildTrustedPrincipals(env?: NodeJS.ProcessEnv): Promise<Set<string>> {
  const trusted = new Set<string>(TRUSTED_BASE);
  const principal = resolveWindowsUserPrincipal(env);
  if (principal) {
    trusted.add(normalize(principal));
    const parts = principal.split("\\");
    const userOnly = parts.at(-1);
    if (userOnly) {
      trusted.add(normalize(userOnly));
    }
  }
  
  // Try explicit USERSID first
  let userSid = normalize(env?.USERSID ?? "");
  
  // Fallback: resolve current user SID dynamically
  if (!userSid || !SID_RE.test(userSid)) {
    const resolvedSid = await resolveCurrentUserSid();
    if (resolvedSid) {
      userSid = resolvedSid;
    }
  }
  
  if (userSid && SID_RE.test(userSid)) {
    trusted.add(userSid);
  }
  return trusted;
}

Option 2: Add non-SID fallback for caller ACE (Simpler)

If you prefer to avoid async changes, you can document that callers should ensure USERSID is set when using SID mode, or add a comment warning about this limitation:

async function inspectWindowsAcl(
  targetPath: string,
  opts?: { env?: NodeJS.ProcessEnv; exec?: ExecFn },
): Promise<WindowsAclSummary> {
  const exec = opts?.exec ?? runExec;
  try {
    // /sid outputs security identifiers...
    const { stdout, stderr } = await exec("icacls", [targetPath, "/sid"]);
    const output = `${stdout}\n${stderr}`.trim();
    const entries = parseIcaclsOutput(output, targetPath);
    
    // Ensure USERSID is set when using /sid mode, or caller ACE may be misclassified
    const envWithSid = opts?.env ?? process.env;
    if (!envWithSid.USERSID) {
      // TODO: resolve current user SID via whoami /user
      // For now, rely on username matching (less reliable with /sid output)
    }
    
    const { trusted, untrustedWorld, untrustedGroup } = summarizeWindowsAcl(entries, envWithSid);
    return { ok: true, entries, trusted, untrustedWorld, untrustedGroup };
  } catch (err) {
    // ...
  }
}

📋 Recommendation

I recommend Option 1 because:

✅ Properly resolves the caller's SID regardless of environment
✅ Maintains the security guarantee of SID-based classification
✅ Works transparently without requiring callers to set USERSID
✅ Follows the existing pattern of using exec for Windows commands

The fix ensures that when /sid mode is enabled, the caller's ACE is correctly classified as trusted, preventing false-positive security alerts.

🧪 Test Cases to Add

After implementing the fix, consider adding these test cases:

test('classifies caller SID as trusted when USERSID is not set but whoami /user succeeds', async () => {
  const mockExec = vi.fn()
    .mockResolvedValueOnce({ stdout: 'username sid\\nDESKTOP\\user *S-1-5-21-...-1001\\n' }) // whoami /user
    .mockResolvedValueOnce({ stdout: '...icacls output with *S-1-5-21-...-1001...' });
  
  const result = await inspectWindowsAcl('C:\\test', { env: {}, exec: mockExec });
  
  expect(result.trusted.some(entry => entry.principal.includes('*S-1-5-21'))).toBe(true);
});

Let me know if you need help implementing this fix! 🦞

mdlmarkham

Review: Locale-Independent Windows ACL Audit ✅

Verdict: Critical fix for non-English Windows. Ready to merge.

The Bug

On non-English Windows (Russian, German, CJK), icacls outputs localized account names:

СИСТЕМА instead of SYSTEM
ВСЕ instead of Everyone

Node.js may garble multi-byte characters, causing the security auditor to misclassify SYSTEM as unknown/untrusted → false-positive alerts.

The Fix

Pass /sid to icacls to output canonical ASCII SIDs:

*S-1-5-18 instead of СИСТЕМА (SYSTEM)
*S-1-1-0 instead of Все (Everyone)

Then classify by SID, not by localized name.

Key Changes

SID_RE — Accept optional leading * from /sid output
classifyPrincipal() — Strip * prefix, lookup SID in known tables
WORLD_SIDS — New constant for world-equivalent SIDs:
- S-1-1-0 (Everyone)
- S-1-5-11 (Authenticated Users)
- S-1-5-32-545 (BUILTIN\Users)
formatIcaclsResetCommand() — Use *S-1-5-18 instead of localized name

Test Coverage

Comprehensive:

✅ *S-1-5-18 (SYSTEM SID) → trusted
✅ *S-1-5-32-544 (Administrators SID) → trusted
✅ *S-1-1-0 (Everyone) → world (not group)
✅ *S-1-5-11 (Authenticated Users) → world
✅ *S-1-5-32-545 (BUILTIN\Users) → world
✅ Full mock with /sid output
✅ CI: 29/29 checks pass

Security Impact

Before: Non-English Windows → false positives → operators learn to ignore security alerts
After: Correct classification → real issues get attention

This is a security reliability fix — reduces alert fatigue.

Edge Cases

Mixed output (some names, some SIDs): icacls /sid outputs all SIDs ✅
Third-party SIDs not in table: Fall through to group classification ✅
Domain SIDs: Not in table, classified by prefix matching

Recommendation: Approve. Critical for international users, well-tested, follows SID best practices.

aisle-research-bot · 2026-03-07T17:48:44Z

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

#	Severity	Title
1	🟠 High	Uncontrolled search path execution of `whoami` allows ACL audit bypass (and potential code execution)

1. 🟠 Uncontrolled search path execution of `whoami` allows ACL audit bypass (and potential code execution)

Property	Value
Severity	High
CWE	CWE-427
Location	`src/security/windows-acl.ts:270-304`

Description

The new fallback in inspectWindowsAcl() resolves the current user SID by executing whoami by name when USERSID is missing.

runExec("whoami", ...) relies on Windows executable resolution (current directory/PATH search).
If an attacker can influence the working directory or PATH (e.g., running the tool inside an untrusted repo directory containing a malicious whoami.exe), the process may execute the attacker-controlled binary.
The resulting (spoofed) SID is then trusted as USERSID, which can downgrade findings by reclassifying matching SID ACL entries from untrustedGroup to trusted.

Vulnerable code:

const { stdout, stderr } = await exec("whoami", ["/user", "/fo", "csv", "/nh"]);
const match = `${stdout}\n${stderr}`.match(/\*?S-\d+-\d+(?:-\d+)+/i);
...
effectiveEnv = { ...effectiveEnv, USERSID: currentUserSid };

Recommendation

Avoid PATH/current-directory resolution for OS binaries and tighten SID extraction.

Execute whoami.exe via an absolute, trusted path (e.g., System32) rather than relying on search-path resolution.
Strictly parse the expected CSV output and validate:
- exactly 2 columns
- the SID column matches SID_RE (anchored)
- optionally the username column matches the expected current user principal

Example:

import path from "node:path";

function system32Bin(name: string): string {
  // Prefer a constant base; env vars can be influenced.
  const windir = process.env.WINDIR || "C:\\Windows";
  return path.join(windir, "System32", `${name}.exe`);
}

async function resolveCurrentUserSid(exec: ExecFn): Promise<string | null> {
  const whoamiPath = system32Bin("whoami");
  const { stdout } = await exec(whoamiPath, ["/user", "/fo", "csv", "/nh"]);
  const line = stdout.trim().split(/\r?\n/)[0] ?? "";
  // Minimal CSV handling for two quoted fields: "user","SID"
  const m = line.match(/^\s*"[^"]+"\s*,\s*"(\*?s-\d+-\d+(?:-\d+)+)"\s*$/i);
  return m ? normalizeSid(m[1]) : null;
}

This prevents binary-hijacking and reduces the chance of trusting an unintended SID.

Analyzed PR: #38900 at commit 378ff5b

_{Last updated on: 2026-03-07T17:59:51Z}

vincentkoc · 2026-03-07T17:49:19Z

Pushed a follow-up fix on top of this branch.

The SID-based approach is still the right canonical path for #35834, but it needed one more piece: when icacls /sid emits the current user as a SID and the environment does not already provide USERSID, the audit could still misclassify the owner ACE as group. This update resolves that by falling back to whoami /user only when unresolved SID principals remain, then re-running classification with the resolved caller SID.

It also keeps well-known world SIDs (Everyone, Authenticated Users, BUILTIN\\Users) pinned to world even if a malformed USERSID value is present.

Local verification:

pnpm test src/security/windows-acl.test.ts
pnpm check src/security/windows-acl.ts src/security/windows-acl.test.ts

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 378ff5b1f6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-03-07T17:54:06Z

src/security/windows-acl.ts

+    if (TRUSTED_SIDS.has(sid) || trustedPrincipals.has(sid)) {
+      return "trusted";
+    }
+    return "group";


Trust Creator Owner SID in SID-based ACL classification

After switching inspectWindowsAcl to icacls /sid, ACL entries like CREATOR OWNER are emitted as *S-1-3-0, but the SID path in classifyPrincipal only trusts TRUSTED_SIDS and caller SID, so S-1-3-0 now falls through to group. On default Windows directory ACLs that include CREATOR OWNER:(OI)(CI)(IO)(F), this produces false untrusted group-writable findings even though this principal was previously trusted by name.

Useful? React with 👍 / 👎.

@onstash

* origin/main: (37 commits) chore: land #39056 Node version hint sync (thanks @onstash) fix(security): use icacls /sid for locale-independent Windows ACL audit (#38900) fix(ci): refresh detect-secrets baseline offsets fix(security): strip custom auth headers on cross-origin redirects fix(ci): harden diffs viewer request guard and secret scan baseline Secrets: harden SecretRef-safe models.json persistence (#38955) docs(changelog): credit allowlist scoping report refactor(commands): dedupe onboard search perplexity test setup refactor(commands): dedupe message command secret-config tests refactor(cli): dedupe restart health probe setup tests refactor(cron): dedupe interim retry fallback assertions refactor(commands): dedupe model probe target test fixtures refactor(discord): dedupe message preflight test runners refactor(discord): share message handler test scaffolding refactor(discord): dedupe native command ACP routing test setup refactor(slack): dedupe app mention in-flight race setup refactor(slack): reuse shared prepare test scaffolding refactor(plugin-sdk): extract shared channel prelude exports refactor(slack): dedupe app mention race test setup refactor(line): dedupe replay webhook test fixtures ... # Conflicts: # .secrets.baseline # apps/ios/fastlane/Fastfile # src/commands/message.test.ts

…it (openclaw#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 openclaw#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 <[email protected]>

…it (openclaw#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 openclaw#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 <[email protected]> (cherry picked from commit 7735a0b)

lbo728 added 2 commits March 6, 2026 22:26

openclaw-barnacle bot added the size: S label Mar 7, 2026

chatgpt-codex-connector bot reviewed Mar 7, 2026

View reviewed changes

mdlmarkham reviewed Mar 7, 2026

View reviewed changes

Security: resolve owner SID fallback for Windows ACL audit

378ff5b

vincentkoc self-assigned this Mar 7, 2026

This was referenced Mar 7, 2026

fix(security): add Russian Cyrillic SYSTEM locale for Windows ACL audit #35852

Closed

fix(security): avoid localized NT AUTHORITY ACL false positives on Windows #35874

Closed

vincentkoc merged commit 7735a0b into openclaw:main Mar 7, 2026
28 checks passed

chatgpt-codex-connector bot reviewed Mar 7, 2026

View reviewed changes

ademczuk mentioned this pull request Mar 7, 2026

fix(daemon): use locale-invariant result code for schtasks status detection #39076

Closed

5 tasks

github-actions bot mentioned this pull request Mar 8, 2026

上游更新: v2026.3.7 — 19 P0 + 122 P1 待合并 jiulingyun/openclaw-cn#486

Open

alexey-pelykh mentioned this pull request Mar 10, 2026

Cherry-pick: upstream security and infra updates (11 commits) remoteclaw/remoteclaw#854

Closed

4 tasks

alexey-pelykh mentioned this pull request Mar 21, 2026

Cherry-pick upstream security and infra hardening (#854) remoteclaw/remoteclaw#1739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(security): use icacls /sid for locale-independent Windows ACL audit#38900

fix(security): use icacls /sid for locale-independent Windows ACL audit#38900
vincentkoc merged 3 commits intoopenclaw:mainfrom
byungsker:fix/windows-acl-use-sid-for-locale-independence

byungsker commented Mar 7, 2026

Uh oh!

greptile-apps bot commented Mar 7, 2026 •

edited

Loading

Comments Outside Diff (1)

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Mar 7, 2026

Uh oh!

Jimmy-xuzimo commented Mar 7, 2026

Uh oh!

mdlmarkham left a comment

Uh oh!

aisle-research-bot bot commented Mar 7, 2026 •

edited

Loading

Uh oh!

vincentkoc commented Mar 7, 2026

Uh oh!

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

byungsker commented Mar 7, 2026

Problem

Fix

Tests added

Uh oh!

greptile-apps bot commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Comments Outside Diff (1)

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Jimmy-xuzimo commented Mar 7, 2026

📝 PR Review Fix - Windows ACL SID Mode

🔴 Critical Issue

Caller ACE Classification Fails Without USERSID

✅ Suggested Fix

Option 1: Use whoami /user to get current user SID (Recommended)

Option 2: Add non-SID fallback for caller ACE (Simpler)

📋 Recommendation

🧪 Test Cases to Add

Uh oh!

mdlmarkham left a comment

Choose a reason for hiding this comment

Review: Locale-Independent Windows ACL Audit ✅

The Bug

The Fix

Key Changes

Test Coverage

Security Impact

Edge Cases

Uh oh!

aisle-research-bot bot commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔒 Aisle Security Analysis

1. 🟠 Uncontrolled search path execution of whoami allows ACL audit bypass (and potential code execution)

Description

Recommendation

Uh oh!

vincentkoc commented Mar 7, 2026

Uh oh!

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

greptile-apps bot commented Mar 7, 2026 •

edited

Loading

Option 1: Use `whoami /user` to get current user SID (Recommended)

aisle-research-bot bot commented Mar 7, 2026 •

edited

Loading

1. 🟠 Uncontrolled search path execution of `whoami` allows ACL audit bypass (and potential code execution)