Commit Graph

7 Commits

Author SHA1 Message Date
Byungsker
7735a0b85c 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>
2026-03-07 12:49:33 -05:00
Peter Steinberger
57e1534df8 refactor(tests): consolidate repeated setup helpers 2026-03-03 01:06:00 +00:00
StingNing
944abe0a6c fix(security): recognize localized Windows SYSTEM account in ACL audit (#29698)
* fix(security): recognize localized Windows SYSTEM account in ACL audit

On non-English Windows (e.g. French "AUTORITE NT\Système"), the security
audit falsely reports fs.config.perms_writable because the localized
SYSTEM account name is not recognized as trusted.

Changes:
- Add common localized SYSTEM principal names (French, German, Spanish,
  Portuguese) to TRUSTED_BASE
- Add diacritics-stripping fallback in classifyPrincipal for unhandled
  locales
- Use well-known SID *S-1-5-18 in icacls reset commands instead of
  hardcoded "SYSTEM" string for locale independence

Fixes #29681

* style: format windows acl files

---------

Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com>
2026-03-02 08:38:56 -06:00
Peter Steinberger
bd4f670544 refactor: simplify windows ACL parsing and expand coverage 2026-02-22 10:43:03 +01:00
SK Akram
85a3c0c818 fix: use SID-based ACL classification for non-English Windows 2026-02-22 10:37:34 +01:00
Peter Steinberger
d6cde28c8e fix: stabilize windows acl tests and command auth registry (#9335) (thanks @M00N7682) 2026-02-05 00:38:35 -08:00
M00N7682
f26cc60872 Tests: add test coverage for security/windows-acl.ts
Adds comprehensive unit tests for Windows ACL inspection utilities:
- resolveWindowsUserPrincipal: username resolution with fallback
- parseIcaclsOutput: icacls output parsing
- summarizeWindowsAcl: ACL entry classification (trusted/world/group)
- inspectWindowsAcl: async ACL inspection with mocked exec
- formatWindowsAclSummary: summary string formatting
- formatIcaclsResetCommand: reset command string generation
- createIcaclsResetCommand: structured reset command generation

All 26 tests passing.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2026-02-05 00:35:29 -08:00