Skip to content

Restrict Windows named-pipe DACL to creating user#5214

Merged
samuv merged 1 commit intomainfrom
windows-pipe-dacl
May 7, 2026
Merged

Restrict Windows named-pipe DACL to creating user#5214
samuv merged 1 commit intomainfrom
windows-pipe-dacl

Conversation

@samuv
Copy link
Copy Markdown
Contributor

@samuv samuv commented May 7, 2026

Summary

Follow-up to #5201 (review thread). Stacked on socker-windows; will rebase onto main once #5201 merges.

By default winio.ListenPipe inherits CreateNamedPipeW's default DACL, which on shared / RDP / Citrix hosts permits any other interactive user to open the pipe and exercise the entire REST API (OIDC is opt-in via WithOIDCConfig, not always present). This PR sets an explicit SDDL on the pipe and adds defense-in-depth around connection lifecycle:

  • Pipe DACL (pkg/api/socket_windows.go) — D:P(A;;GA;;;OW)(A;;GA;;;SY) grants GenericAll to the creating user (OW) and SYSTEM (SY) only, with the Protected flag (P) blocking any inherited ACEs that could widen the descriptor.
  • Doc-comment alignment — soften the "winio's default restricts access to the creating user" claim to match what's actually enforced now (the explicit SDDL above).
  • AF_UNIX fallback marked best-effort — the Windows AF_UNIX path still inherits the parent directory's NTFS ACL since os.Chmod is meaningless on Windows; the named-pipe path is the recommended transport.
  • http.Server.IdleTimeout (pkg/api/server.go) — winio's default MaxInstances is 255, a hard concurrency ceiling unlike POSIX, so a slow client cannot be allowed to hold an instance idle forever and starve new connections. Applies cross-platform.

Addresses inline review comments 3201085355 (pipe DACL) and 3201085359 (AF_UNIX fallback ACL), plus the IdleTimeout note in 3201085433.

Type of change

  • Security improvement / hardening

Test plan

  • Unit tests (go test ./pkg/api/...) — green on macOS.
  • GOOS=windows go vet ./pkg/api/... and GOOS=windows go test -c -o /dev/null ./pkg/api/ — both clean.
  • Manual verification on a Windows host that a second-user CreateFile(\\.\pipe\thv-api) is rejected with ERROR_ACCESS_DENIED — pending; deferred to a reviewer with a Windows host.

Does this introduce a user-facing change?

Yes (Windows only). Other local users on the same Windows host can no longer dial \\.\pipe\thv-api; only the user that started thv serve and SYSTEM can. macOS / Linux behaviour is unchanged.

@samuv samuv requested review from JAORMX and amirejaz as code owners May 7, 2026 12:52
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels May 7, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.78%. Comparing base (d5337ec) to head (2176f4b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5214      +/-   ##
==========================================
- Coverage   67.79%   67.78%   -0.02%     
==========================================
  Files         608      610       +2     
  Lines       62234    62266      +32     
==========================================
+ Hits        42191    42204      +13     
- Misses      16869    16889      +20     
+ Partials     3174     3173       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

By default winio.ListenPipe inherits the kernel default DACL, which
on shared, RDP, or Citrix hosts permits any other interactive user
to open the pipe and exercise the entire REST API; OIDC is opt-in
via WithOIDCConfig and not always present.

Set an explicit SDDL granting GenericAll only to the creating user
(OW) and SYSTEM (SY), with the Protected flag (P) so no inherited
ACEs widen the descriptor. Soften the surrounding doc comment to
match what the listener now enforces, and mark the Windows
AF_UNIX fallback path as best-effort since the .sock file inherits
the parent directory's NTFS ACL.

Add http.Server.IdleTimeout as defense in depth across platforms;
winio.MaxInstances defaults to 255, so a slow client must not be
allowed to keep an instance idle indefinitely and starve new
connections.

Co-authored-by: Cursor <cursoragent@cursor.com>
@samuv samuv force-pushed the windows-pipe-dacl branch from a6799aa to 2176f4b Compare May 7, 2026 13:45
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels May 7, 2026
@samuv samuv requested a review from aponcedeleonch May 7, 2026 13:46
@samuv samuv merged commit ef5e8f7 into main May 7, 2026
43 of 44 checks passed
@samuv samuv deleted the windows-pipe-dacl branch May 7, 2026 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants