From 4de39a80f8a8632c02916c9fe82cb479f5e14d0d Mon Sep 17 00:00:00 2001 From: "Theodor S. Midtlien" Date: Thu, 23 Jul 2026 18:11:51 +0200 Subject: [PATCH] Clean up windows impersonation/SDDL and some comments --- client/cmd/root.go | 2 -- client/cmd/service_controller.go | 14 ++++---- client/cmd/service_pipe_windows.go | 20 +++++------ client/cmd/service_socket.go | 5 ++- client/internal/ipcauth/creds_windows.go | 46 ++++++++++-------------- client/server/server.go | 6 ++-- client/server/ssh_gate.go | 16 +++------ 7 files changed, 43 insertions(+), 66 deletions(-) diff --git a/client/cmd/root.go b/client/cmd/root.go index 24ee8a6de..47b06077d 100644 --- a/client/cmd/root.go +++ b/client/cmd/root.go @@ -144,8 +144,6 @@ func init() { defaultDaemonAddr := "unix:///var/run/netbird.sock" if runtime.GOOS == "windows" { - // Named pipe (not loopback TCP): the pipe client token carries the - // caller's SID so the daemon can authorize each RPC by caller identity. defaultDaemonAddr = "npipe://netbird" } diff --git a/client/cmd/service_controller.go b/client/cmd/service_controller.go index d56aa9457..10a39d23d 100644 --- a/client/cmd/service_controller.go +++ b/client/cmd/service_controller.go @@ -22,19 +22,19 @@ import ( ) // daemonServerOptions installs peer-identity transport credentials and the -// authorization interceptor on the daemon control channel. Identity is only -// available over a Unix socket (SO_PEERCRED) or a Windows named pipe (client -// token); over TCP, or on platforms without a peer-credential primitive, the -// daemon runs without per-caller authorization and warns (no interceptor, so it -// does not deny everyone). +// authorization interceptor on the daemon ipc. Identity is only available +// over a Unix socket (SO_PEERCRED) or a Windows named pipe (client token). +// Over TCP, or on platforms without a peer-credential primitive, the daemon +// runs without per-caller authorization and warns (no interceptor, so it does +// not deny everyone). func daemonServerOptions(network string, interceptor *ipcauth.Interceptor) []grpc.ServerOption { creds := ipcauth.NewTransportCredentials() if creds == nil { - log.Warnf("daemon control channel has no peer-identity primitive on %s; per-caller authorization is disabled", runtime.GOOS) + log.Warnf("daemon ipc has no peer-identity primitive on %s, per-caller authorization is disabled", runtime.GOOS) return nil } if network == "tcp" { - log.Warnf("daemon is listening on TCP (%s); peer identity cannot be authenticated over TCP, per-caller authorization is disabled", daemonAddr) + log.Warnf("daemon is listening on TCP (%s), peer identity cannot be authenticated over TCP, per-caller authorization is disabled", daemonAddr) return nil } return []grpc.ServerOption{ diff --git a/client/cmd/service_pipe_windows.go b/client/cmd/service_pipe_windows.go index 18ca0fbd5..76bbd7f47 100644 --- a/client/cmd/service_pipe_windows.go +++ b/client/cmd/service_pipe_windows.go @@ -12,24 +12,22 @@ import ( "github.com/netbirdio/netbird/client/internal/ipcauth" ) -// listenNamedPipe creates the daemon control named pipe with a tight SDDL -// (SYSTEM + Administrators + interactive users). ListenPipe fails if the pipe -// already exists (first-instance semantics), which prevents a squatting process -// from pre-creating it — we surface that error loudly rather than falling back. +// listenNamedPipe creates the daemon control named pipe with a permissive, +// local-only SDDL. Any local caller may connect, on par with the Unix +// socket's 0666, and the per-RPC interceptor authorizes. ListenPipe fails +// if the pipe already exists (first-instance semantics), which prevents a +// squatting process from pre-creating it. func listenNamedPipe(path string) (net.Listener, error) { return winio.ListenPipe(path, &winio.PipeConfig{ SecurityDescriptor: ipcauth.DefaultPipeSDDL(), }) } -// dialNamedPipe connects to the daemon control named pipe at SECURITY_IDENTIFICATION. -// -// winio's plain DialPipe connects at SECURITY_ANONYMOUS, under which the daemon -// cannot read the caller's token (ImpersonateNamedPipeClient fails / yields an -// anonymous token and the handshake is dropped). Identification lets the daemon -// *identify* the caller (read its SID/groups) without granting it the ability to -// act as the caller — the least privilege the daemon needs for authorization. +// dialNamedPipe connects to the daemon ipc named pipe at SECURITY_IDENTIFICATION. func dialNamedPipe(ctx context.Context, path string) (net.Conn, error) { access := uint32(windows.GENERIC_READ | windows.GENERIC_WRITE) + // winio's plain DialPipe connects at SECURITY_ANONYMOUS, under which the + // daemon cannot read the caller's token. Identification lets the daemon + // read its SID/groups without granting it the ability to act as the caller. return winio.DialPipeAccessImpLevel(ctx, path, access, winio.PipeImpLevelIdentification) } diff --git a/client/cmd/service_socket.go b/client/cmd/service_socket.go index 67031203b..511b1364b 100644 --- a/client/cmd/service_socket.go +++ b/client/cmd/service_socket.go @@ -61,9 +61,8 @@ func parseListenAddress(addr string) (string, string, error) { } } -// pipePath maps a daemon-addr npipe name (e.g. "netbird" from "npipe://netbird") -// to a Windows named-pipe path (\\.\pipe\netbird). A full \\.\pipe\ path is -// returned unchanged. +// pipePath maps a daemon-addr npipe name ("npipe://netbird") to a Windows +// named-pipe path (\\.\pipe\netbird). func pipePath(name string) string { if strings.HasPrefix(name, `\\`) { return name diff --git a/client/internal/ipcauth/creds_windows.go b/client/internal/ipcauth/creds_windows.go index 490116aee..3ef43da75 100644 --- a/client/internal/ipcauth/creds_windows.go +++ b/client/internal/ipcauth/creds_windows.go @@ -15,23 +15,17 @@ import ( var ( modadvapi32 = windows.NewLazySystemDLL("advapi32.dll") procImpersonateNamedPipeClient = modadvapi32.NewProc("ImpersonateNamedPipeClient") - procRevertToSelf = modadvapi32.NewProc("RevertToSelf") ) -// Windows group-SID attribute flags (winnt.h): a group only counts toward -// membership when it is enabled and not marked use-for-deny-only. -const ( - seGroupEnabled = 0x00000004 - seGroupUseForDenyOnly = 0x00000010 -) - -// DefaultPipeSDDL restricts the daemon control pipe to LocalSystem (SY), the -// Administrators group (BA), and interactive logon users (IU). It deliberately -// excludes Authenticated Users / Everyone so remote or arbitrary service -// principals cannot connect. This is the connection gate; the interceptor still -// does per-RPC authorization by caller identity. +// DefaultPipeSDDL keeps the daemon control pipe open to any LOCAL caller, on par +// with the Unix socket's 0666 mode. +// +// D:P protected DACL, no inheritance +// (D;;GA;;;NU) deny GENERIC_ALL to NETWORK (remote/SMB) +// (A;;GA;;;SY) allow GENERIC_ALL to LocalSystem (the daemon itself) +// (A;;GA;;;WD) allow GENERIC_ALL to Everyone (local, per-RPC ACL gates) func DefaultPipeSDDL() string { - return "D:P(A;;GA;;;SY)(A;;GA;;;BA)(A;;GA;;;IU)" + return "D:P(D;;GA;;;NU)(A;;GA;;;SY)(A;;GA;;;WD)" } // NewTransportCredentials returns gRPC transport credentials that derive the @@ -85,19 +79,25 @@ func (winpipeCreds) OverrideServerName(string) error { return nil } // and elevation by impersonating the pipe client on this thread and reading the // impersonation token. Impersonation is connection-bound (no PID race) and the // window is kept minimal and pinned to the OS thread (impersonation is thread-local). -func pipeClientIdentity(handle windows.Handle) (Identity, error) { +func pipeClientIdentity(handle windows.Handle) (id Identity, err error) { runtime.LockOSThread() defer runtime.UnlockOSThread() - if err := impersonateNamedPipeClient(handle); err != nil { + if err = impersonateNamedPipeClient(handle); err != nil { return Identity{}, fmt.Errorf("impersonate named pipe client: %w", err) } - defer func() { _ = revertToSelf() }() + defer func() { + // Surface revert error if there are no other errors. + revErr := windows.RevertToSelf() + if err == nil { + err = revErr + } + }() // openAsSelf=true: the token is opened using the daemon's process context // (LocalSystem), not the impersonated client's, so the open always succeeds. var token windows.Token - if err := windows.OpenThreadToken(windows.CurrentThread(), windows.TOKEN_QUERY, true, &token); err != nil { + if err = windows.OpenThreadToken(windows.CurrentThread(), windows.TOKEN_QUERY, true, &token); err != nil { return Identity{}, fmt.Errorf("open thread token: %w", err) } defer token.Close() @@ -113,7 +113,7 @@ func pipeClientIdentity(handle windows.Handle) (Identity, error) { } var groups []string for _, g := range tg.AllGroups() { - if g.Attributes&seGroupEnabled == 0 || g.Attributes&seGroupUseForDenyOnly != 0 { + if g.Attributes&windows.SE_GROUP_ENABLED == 0 || g.Attributes&windows.SE_GROUP_USE_FOR_DENY_ONLY != 0 { continue } groups = append(groups, g.Sid.String()) @@ -134,11 +134,3 @@ func impersonateNamedPipeClient(h windows.Handle) error { } return nil } - -func revertToSelf() error { - r, _, e := procRevertToSelf.Call() - if r == 0 { - return e - } - return nil -} diff --git a/client/server/server.go b/client/server/server.go index 8a071d079..ebe96c2c2 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -403,8 +403,7 @@ func (s *Server) SetConfig(callerCtx context.Context, msg *proto.SetConfigReques } } - // M-SSHGATE: turning on SSH root login / disabling SSH auth requires a - // privileged caller (root or elevated admin). + // SSH root login / disabling SSH auth requires a privileged caller. if err := requirePrivilegedForDangerousSSH(callerCtx, msg.EnableSSHRoot, msg.DisableSSHAuth); err != nil { return nil, err } @@ -544,8 +543,7 @@ func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*pro } } - // M-SSHGATE: turning on SSH root login / disabling SSH auth requires a - // privileged caller (root or elevated admin). + // SSH root login / disabling SSH auth requires a privileged caller. if err := requirePrivilegedForDangerousSSH(callerCtx, msg.EnableSSHRoot, msg.DisableSSHAuth); err != nil { return nil, err } diff --git a/client/server/ssh_gate.go b/client/server/ssh_gate.go index 0365c7c9f..91bc1e27f 100644 --- a/client/server/ssh_gate.go +++ b/client/server/ssh_gate.go @@ -10,19 +10,11 @@ import ( "github.com/netbirdio/netbird/client/internal/ipcauth" ) -// requirePrivilegedForDangerousSSH enforces M-SSHGATE: enabling SSH root login -// or disabling SSH authentication turns the root/LocalSystem daemon's SSH server -// into an unauthenticated root shell (local-to-remote-root escalation), so only -// a privileged caller (Unix root, or Windows elevated-admin/LocalSystem) may set +// requirePrivilegedForDangerousSSH enforces admin permissions for SSH config. +// Enabling SSH root login or disabling SSH authentication turns the +// root/LocalSystem daemon's SSH server into an unauthenticated root shell +// (local-to-remote-root escalation), so only a privileged caller may set // these flags to true over the local IPC. -// -// It gates the request fields, not the resulting config: a value already -// persisted (e.g. set previously by root, or by MDM/managed config) is untouched; -// only a new attempt to turn them on via SetConfig/Login is checked. -// -// When the caller identity cannot be verified (e.g. the daemon is on a TCP -// control channel with no peer credentials) it fails closed — refusing rather -// than letting an unauthenticated local caller flip these flags. func requirePrivilegedForDangerousSSH(ctx context.Context, enableSSHRoot, disableSSHAuth *bool) error { dangerous := (enableSSHRoot != nil && *enableSSHRoot) || (disableSSHAuth != nil && *disableSSHAuth) if !dangerous {