Skip to content

fix(security): classify broad Windows SIDs as world principals#74383

Open
dwc1997 wants to merge 1 commit intoopenclaw:mainfrom
dwc1997:fix/windows-acl-world-sids-74350
Open

fix(security): classify broad Windows SIDs as world principals#74383
dwc1997 wants to merge 1 commit intoopenclaw:mainfrom
dwc1997:fix/windows-acl-world-sids-74350

Conversation

@dwc1997
Copy link
Copy Markdown

@dwc1997 dwc1997 commented Apr 29, 2026

Made-with: Cursor

Summary

  • Problem: Windows ACL audit currently classifies several broad/unauthenticated principals (for example Anonymous Logon, BUILTIN\Guests) as group instead of world.
  • Why it matters: This can downgrade filesystem findings from worldWritable (critical) to groupWritable (warn), reducing severity for effectively world-accessible paths.
  • What changed: Expanded Windows world-equivalent SID/principal lists and added regression tests at both classifier and filesystem-audit levels.
  • What did NOT change (scope boundary): No POSIX permission logic changes, no ACL parsing format changes, no unrelated security policy changes.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

Root Cause (if applicable)

  • Root cause: classifyPrincipal only treated a narrow world-equivalent set as world (Everyone, Authenticated Users, BUILTIN\Users), so other broad-access principals fell through to group.
  • Missing detection / guardrail: Tests did not cover classification for S-1-5-7, S-1-5-32-546, S-1-5-4, S-1-2-0, S-1-5-2, or audit severity impact from those entries.
  • Contributing context (if known): Audit findings differentiate worldWritable (critical) and groupWritable (warn), so misclassification directly affects security signal strength.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file:
    • src/security/windows-acl.test.ts
    • src/security/audit-filesystem-windows.test.ts
  • Scenario the test should lock in:
    • Added SIDs are classified into untrustedWorld (not untrustedGroup).
    • A Windows state-dir ACL using *S-1-5-7:(F) triggers fs.state_dir.perms_world_writable with critical severity.
  • Why this is the smallest reliable guardrail:
    • The bug is in ACL principal classification and filesystem finding mapping; these tests hit both directly without requiring host-specific E2E setup.
  • Existing test that already covers this (if any):
    • Existing SID tests covered only a narrower world SID set.
  • If no new test is added, why not:
    • N/A (new tests added).

User-visible / Behavior Changes

  • On Windows, ACL entries for broad principals such as Anonymous Logon, Guests, Interactive, Local, and Network are now treated as world-equivalent in security audit classification.
  • Security audit can now correctly elevate affected state-dir findings to worldWritable critical severity.

Diagram (if applicable)

Before:
[state dir ACL includes *S-1-5-7:(F)] -> [classified as group] -> [groupWritable warn]

After:
[state dir ACL includes *S-1-5-7:(F)] -> [classified as world] -> [worldWritable critical]

@dwc1997 dwc1997 requested a review from a team as a code owner April 29, 2026 14:15
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 29, 2026 •
edited
Loading

Greptile Summary

This PR correctly expands the Windows ACL classifier to treat Anonymous Logon, Guests, Interactive, Network, and Local principals (and their SID equivalents) as world-equivalent, preventing them from being downgraded to the lower-severity groupWritable bucket. The buildTrustedPrincipals guard already references WORLD_SIDS directly, so the new SIDs are automatically excluded from the trusted set. Both classifier unit tests and an integration-level filesystem-audit test are added to lock in the behavior.

Confidence Score: 4/5

Safe to merge — the change is security-conservative and well-tested, with only a stale inline comment as a minor concern.

All logic changes are correct and covered by new tests. The only finding is a P2 stale comment in buildTrustedPrincipals that could mislead a future reader but does not affect runtime behavior.

No files require special attention beyond the stale comment on line 131 of windows-acl.ts.

Comments Outside Diff (1)

  1. src/security/windows-acl.ts, line 131-132 (link)

    P2 Stale guard comment after WORLD_SIDS expansion

    The inline comment still names only the original three SIDs (Everyone, Authenticated Users, BUILTIN\\Users), but the guard itself now protects against five additional world-equivalent SIDs (S-1-5-7, S-1-5-32-546, S-1-5-4, S-1-2-0, S-1-5-2). A future reader could mistake the comment as an exhaustive list and add a targeted SID bypass that the guard already covers.

    Prompt To Fix With AI 
    This is a comment left during a code review.
Path: src/security/windows-acl.ts
Line: 131-132

Comment:
**Stale guard comment after `WORLD_SIDS` expansion**

The inline comment still names only the original three SIDs (`Everyone`, `Authenticated Users`, `BUILTIN\\Users`), but the guard itself now protects against five additional world-equivalent SIDs (`S-1-5-7`, `S-1-5-32-546`, `S-1-5-4`, `S-1-2-0`, `S-1-5-2`). A future reader could mistake the comment as an exhaustive list and add a targeted SID bypass that the guard already covers.



How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI 
This is a comment left during a code review.
Path: src/security/windows-acl.ts
Line: 131-132

Comment:
**Stale guard comment after `WORLD_SIDS` expansion**

The inline comment still names only the original three SIDs (`Everyone`, `Authenticated Users`, `BUILTIN\\Users`), but the guard itself now protects against five additional world-equivalent SIDs (`S-1-5-7`, `S-1-5-32-546`, `S-1-5-4`, `S-1-2-0`, `S-1-5-2`). A future reader could mistake the comment as an exhaustive list and add a targeted SID bypass that the guard already covers.

```suggestion
    // Guard: never add world-equivalent SIDs to the trusted set, even if USERSID is set
    // to one of them by a malicious process. See WORLD_SIDS for the full list.
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix(security): classify broad Windows SI..." | Re-trigger Greptile

@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Apr 29, 2026 •
edited
Loading

Codex review: needs changes before merge.

Summary
The PR expands Windows ACL world-equivalent principal/SID lists and adds unit plus filesystem-audit regression tests so broad Windows principals classify as world.

Reproducibility: yes. Source inspection on current main shows a SID such as *S-1-5-7 is absent from WORLD_SIDS, falls through to group, and can therefore produce the warn-level group-writable path instead of the critical world-writable finding.

Next step before merge
A repair worker can safely make the two narrow hygiene fixes; final merge still needs normal security-owner review.

Security
Cleared: The diff is limited to Windows ACL classification and focused tests, with no dependency, workflow, package-script, secret, publishing, or external code-execution surface change.

Review findings

  • [P2] Add the required changelog entry — src/security/windows-acl.ts:43-51
  • [P3] Refresh the trusted-SID guard comment — src/security/windows-acl.ts:85-94
Review details

Best possible solution:

Land a focused src/security fix that keeps Windows ACL classification authoritative for broad well-known principals, preserves trusted SYSTEM/current-user handling, documents the user-visible audit change, and keeps the classifier plus filesystem-audit regression coverage.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection on current main shows a SID such as *S-1-5-7 is absent from WORLD_SIDS, falls through to group, and can therefore produce the warn-level group-writable path instead of the critical world-writable finding.

Is this the best way to solve the issue?

Mostly yes. Expanding the classifier-owned SID/principal sets with focused unit and filesystem-audit tests is the narrow maintainable fix; the safer merge path is to add the changelog entry, refresh the stale comment, then get security-owner review.

Full review comments:

  • [P2] Add the required changelog entry — src/security/windows-acl.ts:43-51
    This is a user-visible security-audit fix, but the PR changes only src/security files and tests. Repo policy requires fix/security behavior changes to be listed under the active CHANGELOG.md Fixes section with appropriate human contributor credit before merge.
    Confidence: 0.9
  • [P3] Refresh the trusted-SID guard comment — src/security/windows-acl.ts:85-94
    After this change WORLD_SIDS includes broad SIDs beyond Everyone, Authenticated Users, and BUILTIN\Users, but the later guard comment still names only the original three. Point it at WORLD_SIDS or make the list explicitly non-exhaustive so future edits do not misread the security guard.
    Confidence: 0.82

Overall correctness: patch is incorrect
Overall confidence: 0.88

Acceptance criteria:

  • git diff --check
  • pnpm exec oxfmt --check --threads=1 CHANGELOG.md src/security/windows-acl.ts
  • pnpm test src/security/windows-acl.test.ts src/security/audit-filesystem-windows.test.ts
  • pnpm check:changed in Testbox before merge

What I checked:

Likely related people:

  • @openclaw/openclaw-secops: CODEOWNERS explicitly owns /src/security/, and this PR changes Windows security-audit classification and severity behavior. (role: required security reviewer; confidence: high; files: .github/CODEOWNERS, src/security/windows-acl.ts, src/security/audit.ts)
  • @vincentkoc: Available local history and blame show the current Windows ACL/audit files and tests entering this checkout in commit d122a3492d17103c6585621c50a1f69967984b28. (role: recent maintainer of current security audit files; confidence: medium; commits: d122a3492d17; files: src/security/windows-acl.ts, src/security/audit.ts, src/security/audit-fs.ts)

Remaining risk / open question:

  • Final merge still needs @openclaw/openclaw-secops review because the patch changes security-sensitive classification under /src/security/.
  • No tests were executed in this read-only review; targeted tests and the changed gate should run after the mechanical PR repairs.

Codex review notes: model gpt-5.5, reasoning high; reviewed against d583662fd905.

This was referenced Apr 29, 2026
Open
Open
@BradGroux BradGroux mentioned this pull request May 5, 2026
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

size: S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Security] Windows ACL audit bypass: Anonymous and Guest SIDs are misclassified as "group" instead of "world"

1 participant

@dwc1997