From 2176f4bbd25cf1adb53a45c9e7af2566f43992be Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Thu, 7 May 2026 14:51:21 +0200 Subject: [PATCH] Restrict Windows named-pipe DACL to creating user 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 --- pkg/api/server.go | 7 +++++++ pkg/api/socket_windows.go | 33 +++++++++++++++++++++++---------- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/pkg/api/server.go b/pkg/api/server.go index 31d873ac07..faea31edc1 100644 --- a/pkg/api/server.go +++ b/pkg/api/server.go @@ -61,6 +61,7 @@ import ( const ( middlewareTimeout = 60 * time.Second readHeaderTimeout = 10 * time.Second + idleTimeout = 120 * time.Second shutdownTimeout = 30 * time.Second nonceBytes = 16 socketPermissions = 0660 // Socket file permissions (owner/group read-write) @@ -559,6 +560,12 @@ func NewServer(ctx context.Context, builder *ServerBuilder) (*Server, error) { Addr: builder.address, Handler: handler, ReadHeaderTimeout: readHeaderTimeout, + // IdleTimeout caps how long a keep-alive connection can sit idle. + // On Windows named pipes winio.MaxInstances defaults to 255, so a + // slow client cannot hold an instance forever and starve new + // connections; on POSIX it bounds keep-alive resource use the same + // way the http stdlib defaults would for a tcp listener. + IdleTimeout: idleTimeout, } return &Server{ diff --git a/pkg/api/socket_windows.go b/pkg/api/socket_windows.go index 494a82a550..95197d1e0e 100644 --- a/pkg/api/socket_windows.go +++ b/pkg/api/socket_windows.go @@ -22,6 +22,15 @@ import ( // (Docker, containerd, Podman) and is well above any single HTTP header chunk. const namedPipeBufferSize = 64 * 1024 +// namedPipeSDDL restricts the named-pipe DACL to the creating user (OW) and +// SYSTEM (SY), each granted GenericAll. The Protected flag (P) blocks ACE +// inheritance from any container object. Without an explicit DACL, winio +// inherits CreateNamedPipeW's default which permits other interactive users +// to open the pipe and exercise the entire REST API; on shared, RDP, or +// Citrix hosts that is reachable by anyone with a desktop session, since +// OIDC is opt-in (WithOIDCConfig) rather than always-on. +const namedPipeSDDL = `D:P(A;;GA;;;OW)(A;;GA;;;SY)` + // supportsNamedPipe reports whether the current build target can host a // Windows named-pipe listener. Used by createListener to choose between the // pipe and AF_UNIX paths without dragging the runtime package into server.go. @@ -30,22 +39,26 @@ func supportsNamedPipe() bool { return true } // setupUnixSocket creates either a Windows named-pipe listener (when address // has the \\.\pipe\ prefix) or an AF_UNIX listener at a filesystem path. // -// Named pipes are kernel objects rather than files, so the os.Stat / os.Remove -// precheck, os.MkdirAll, and os.Chmod steps are skipped: the pipe namespace -// has no parent directory, and access control is governed by the security -// descriptor on the listener (winio's default restricts access to the -// creating user, which matches the toolhive-studio same-user use case). +// Named pipes are kernel objects rather than files, so the os.Remove, +// os.MkdirAll, and os.Chmod steps are skipped: the pipe namespace has no +// parent directory, and access control is governed by the SDDL passed to +// winio.ListenPipe (see namedPipeSDDL). // // AF_UNIX is supported on Windows 10 1803+. The chmod step is dropped on this -// path because POSIX file modes do not apply on Windows. +// path because POSIX file modes do not apply on Windows; the resulting +// .sock file inherits the parent directory's NTFS ACL, which is best-effort. +// Prefer the named-pipe path when listener access control matters. func setupUnixSocket(address string) (net.Listener, error) { if isNamedPipeAddress(address) { // MessageMode is left at false (byte stream) explicitly because HTTP - // requires byte-oriented framing. + // requires byte-oriented framing. SecurityDescriptor restricts the + // pipe DACL to the creating user and SYSTEM; without it the winio + // default permits other interactive users to dial the pipe. listener, err := winio.ListenPipe(address, &winio.PipeConfig{ - MessageMode: false, - InputBufferSize: namedPipeBufferSize, - OutputBufferSize: namedPipeBufferSize, + MessageMode: false, + InputBufferSize: namedPipeBufferSize, + OutputBufferSize: namedPipeBufferSize, + SecurityDescriptor: namedPipeSDDL, }) if err != nil { return nil, fmt.Errorf("failed to create named pipe listener: %w", err)