From a91fe94ada3edfc4b9c6939b13fc084a0847669d Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Fri, 2 Oct 2026 12:32:27 +0200 Subject: [PATCH] Read user certificates only from the session of the active profile's owner --- client/internal/certproof/README.md | 17 +-- client/internal/certproof/collect_darwin.go | 10 +- client/internal/certproof/collect_windows.go | 8 +- .../internal/certproof/consoleuser_darwin.go | 8 ++ .../certproof/consoleuser_darwin_test.go | 10 ++ .../internal/certproof/desktopuser_windows.go | 111 ++++++++++++++---- .../certproof/desktopuser_windows_test.go | 35 ++++++ client/internal/certproof/store.go | 12 +- client/internal/connect.go | 9 ++ client/server/server.go | 16 ++- 10 files changed, 196 insertions(+), 40 deletions(-) create mode 100644 client/internal/certproof/desktopuser_windows_test.go diff --git a/client/internal/certproof/README.md b/client/internal/certproof/README.md index 8a62849b5..726bc213b 100644 --- a/client/internal/certproof/README.md +++ b/client/internal/certproof/README.md @@ -88,8 +88,11 @@ cmd.SysProcAttr = &syscall.SysProcAttr{Token: syscall.Token(token), CreationFlag `CREATE_NO_WINDOW` matters: without it a console window flashes on the user's desktop on every sync. -Session selection prefers the physical console, then falls back to any active session, -so remote desktop and VDI hosts work. `WTSQueryUserToken` needs `SE_TCB_NAME`, which +The session asked is one belonging to the account that owns the active profile, matched +by SID: the console first, then active remote sessions, then disconnected ones, whose +user is still signed in. Session 0 hosts services and is never asked. A profile without +an owner asks the console user only. Nobody else who happens to be signed in to a +terminal server or VDI host can therefore decide the result. `WTSQueryUserToken` needs `SE_TCB_NAME`, which LocalSystem holds and an ordinary process does not, so a user-run `netbird up` skips the helper and reads the machine store alone. @@ -221,11 +224,11 @@ currently open**. Consequences worth designing around: - **Signing out changes the answer.** Posture can flip between compliant and non-compliant across a sign-out, so management should treat "no proof" as its own state rather than as a failed check, or users get disconnected at the sign-in screen. -- **One session is asked, not all of them.** macOS asks the console user, so other - fast-user-switched accounts are skipped even though their keychains are unlocked. - Windows prefers the console and otherwise takes the first active session. If you ever - need every signed-in user, both platforms would have to enumerate sessions and ask - each one. +- **Only the profile owner is asked.** The user certificate belongs to whoever owns the + active NetBird profile. macOS asks the console user only when that user owns the + profile, so a fast-user-switched account never answers for someone else. Windows asks + a session of the owner wherever it is, console or remote, and with no owner recorded + only the console user. - **A locked keychain still blocks signing.** A user can be logged in with their keychain locked (locked on sleep, or manually). The helper then needs an unlock prompt and may block, which is why the spawn has a 30s timeout and a failure is reported as diff --git a/client/internal/certproof/collect_darwin.go b/client/internal/certproof/collect_darwin.go index de3a9fb12..99019ce5b 100644 --- a/client/internal/certproof/collect_darwin.go +++ b/client/internal/certproof/collect_darwin.go @@ -23,7 +23,7 @@ const helperTimeout = 30 * time.Second // installs device identities, and reaches the console user's login keychain only by // launching a helper into that user's session. A Mac sitting at the login window // therefore yields device proofs alone. -func CollectProofs(ctx context.Context, checks []*proto.Checks, peerKey []byte, _ Config) []certposture.Proof { +func CollectProofs(ctx context.Context, checks []*proto.Checks, peerKey []byte, cfg Config) []certposture.Proof { challenges := certificateChallenges(checks) if len(challenges) == 0 { logNoChallenges(checks) @@ -38,7 +38,7 @@ func CollectProofs(ctx context.Context, checks []*proto.Checks, peerKey []byte, proofs := CollectChallenges(ctx, DefaultStore(), challenges, peerKey) - userProofs, err := collectAsConsoleUser(ctx, challenges, peerKey) + userProofs, err := collectAsConsoleUser(ctx, cfg.ProfileOwner, challenges, peerKey) if err != nil { log.Debugf("certificate posture: console user keychain unavailable: %v", err) } @@ -49,11 +49,15 @@ func CollectProofs(ctx context.Context, checks []*proto.Checks, peerKey []byte, // user. Dropping to their uid is not enough: keychain access is an XPC call to a // per-session securityd, so the helper has to enter their Mach bootstrap namespace, // which is what launchctl asuser does. -func collectAsConsoleUser(ctx context.Context, challenges []*proto.CertificateChallenge, peerKey []byte) ([]certposture.Proof, error) { +func collectAsConsoleUser(ctx context.Context, owner string, challenges []*proto.CertificateChallenge, peerKey []byte) ([]certposture.Proof, error) { user, ok := CurrentConsoleUser() if !ok { return nil, nil } + if !user.isOwner(owner) { + log.Debugf("certificate posture: console user %s does not own the active profile, no user keychain is asked", user.Name) + return nil, nil + } binary, err := os.Executable() if err != nil { diff --git a/client/internal/certproof/collect_windows.go b/client/internal/certproof/collect_windows.go index 2202e0aa6..d3e005276 100644 --- a/client/internal/certproof/collect_windows.go +++ b/client/internal/certproof/collect_windows.go @@ -22,7 +22,7 @@ const helperTimeout = 30 * time.Second // Intune enrol device certificates, and reaches the signed-in user's store by launching // a helper with that session's token. A machine at the sign-in screen therefore proves // device certificates alone. -func CollectProofs(ctx context.Context, checks []*proto.Checks, peerKey []byte, _ Config) []certposture.Proof { +func CollectProofs(ctx context.Context, checks []*proto.Checks, peerKey []byte, cfg Config) []certposture.Proof { challenges := certificateChallenges(checks) if len(challenges) == 0 { logNoChallenges(checks) @@ -37,7 +37,7 @@ func CollectProofs(ctx context.Context, checks []*proto.Checks, peerKey []byte, return proofs } - userProofs, err := collectAsDesktopUser(ctx, challenges, peerKey) + userProofs, err := collectAsDesktopUser(ctx, cfg.ProfileOwner, challenges, peerKey) if err != nil { log.Debugf("certificate posture: user certificate store unavailable: %v", err) } @@ -53,8 +53,8 @@ func helperStore() Store { // collectAsDesktopUser runs the helper inside the interactive session of the signed-in // user. Unlike a keychain on macOS, a Windows service can assume a user identity // directly, so the session token goes straight into the child process. -func collectAsDesktopUser(ctx context.Context, challenges []*proto.CertificateChallenge, peerKey []byte) ([]certposture.Proof, error) { - user, ok := CurrentDesktopUser() +func collectAsDesktopUser(ctx context.Context, owner string, challenges []*proto.CertificateChallenge, peerKey []byte) ([]certposture.Proof, error) { + user, ok := CurrentDesktopUser(owner) if !ok { return nil, nil } diff --git a/client/internal/certproof/consoleuser_darwin.go b/client/internal/certproof/consoleuser_darwin.go index 5f41bf7e1..a5182c7fd 100644 --- a/client/internal/certproof/consoleuser_darwin.go +++ b/client/internal/certproof/consoleuser_darwin.go @@ -5,6 +5,7 @@ package certproof import ( "bytes" "fmt" + "strconv" "sync" "github.com/ebitengine/purego" @@ -69,6 +70,13 @@ func (u ConsoleUser) hasDesktop() bool { return u.UID != 0 } +// isOwner reports whether the console user is owner, the account of the active profile, +// which is recorded as a short user name or, for an account without one, a numeric uid. +// With no owner the console user counts, as macOS has a single console user. +func (u ConsoleUser) isOwner(owner string) bool { + return owner == "" || owner == u.Name || owner == strconv.FormatUint(uint64(u.UID), 10) +} + func cfString(str uintptr) string { buf := make([]byte, consoleNameBufSize) if !cfStringGetCString(str, &buf[0], len(buf), encodingUTF8) { diff --git a/client/internal/certproof/consoleuser_darwin_test.go b/client/internal/certproof/consoleuser_darwin_test.go index d1881986f..9c1aaa22c 100644 --- a/client/internal/certproof/consoleuser_darwin_test.go +++ b/client/internal/certproof/consoleuser_darwin_test.go @@ -39,3 +39,13 @@ func TestCurrentConsoleUser_AgreesWithItself(t *testing.T) { assert.NotZero(t, user.UID, "a desktop session never belongs to uid 0") assert.True(t, user.hasDesktop(), "a reported console user must be a desktop session") } + +func TestConsoleUser_IsOwner(t *testing.T) { + user := ConsoleUser{Name: "maycon", UID: 501, GID: 20} + + assert.True(t, user.isOwner(""), "a profile without owner accepts the single console user") + assert.True(t, user.isOwner("maycon"), "the owner by short name") + assert.True(t, user.isOwner("501"), "the owner recorded as a numeric uid") + assert.False(t, user.isOwner("viktor"), "another account's profile must not read this user's keychain") + assert.False(t, user.isOwner("502"), "another uid is another account") +} diff --git a/client/internal/certproof/desktopuser_windows.go b/client/internal/certproof/desktopuser_windows.go index da7168d73..a07449186 100644 --- a/client/internal/certproof/desktopuser_windows.go +++ b/client/internal/certproof/desktopuser_windows.go @@ -2,6 +2,7 @@ package certproof import ( "fmt" + "strings" "unsafe" log "github.com/sirupsen/logrus" @@ -10,11 +11,14 @@ import ( const ( noActiveSession = 0xFFFFFFFF + servicesSession = 0 - // wtsCurrentServer is WTS_CURRENT_SERVER_HANDLE and wtsActive is WTSActive of - // WTS_CONNECTSTATE_CLASS. Neither is exported by x/sys/windows. + // wtsCurrentServer is WTS_CURRENT_SERVER_HANDLE; wtsActive and wtsDisconnected are + // WTSActive and WTSDisconnected of WTS_CONNECTSTATE_CLASS. None is exported by + // x/sys/windows. wtsCurrentServer = windows.Handle(0) wtsActive = 0 + wtsDisconnected = 4 ) // DesktopUser is an interactive session and the account signed into it. The user's @@ -33,37 +37,91 @@ func (u DesktopUser) Close() { } } -// CurrentDesktopUser returns a token for the interactive user whose certificate store -// should be asked. The physical console comes first, and an active remote desktop -// session is used when nobody is at the console, which is how servers and VDI hosts are -// normally reached. The second return is false at the sign-in screen, where no -// interactive session exists and only machine certificates can be proven. +// CurrentDesktopUser returns a token for the session whose certificate store should be +// asked: a session of owner, the account the active profile belongs to, with the +// physical console preferred over remote sessions. With no owner only the console user +// counts. Picking any signed-in user instead would let whoever else is logged in to a +// terminal server or VDI host decide the result. The second return is false when no +// such session exists, and only machine certificates can then be proven. // // Obtaining the token needs SE_TCB_NAME, which the LocalSystem service has and an // ordinary process does not. -func CurrentDesktopUser() (DesktopUser, bool) { - if session := windows.WTSGetActiveConsoleSessionId(); session != noActiveSession { - if user, ok := desktopUser(session); ok { - return user, true - } - log.Debugf("console session %d has nobody signed in, looking for an active remote session", session) +func CurrentDesktopUser(owner string) (DesktopUser, bool) { + console := windows.WTSGetActiveConsoleSessionId() + if owner == "" { + return consoleUser(console) } - sessions, err := activeSessions() + match, err := ownerMatcher(owner) + if err != nil { + log.Debugf("certificate posture: %v", err) + return DesktopUser{}, false + } + + sessions, err := userSessions(console) if err != nil { log.Debugf("cannot enumerate terminal sessions: %v", err) return DesktopUser{}, false } for _, session := range sessions { - if user, ok := desktopUser(session); ok { + user, ok := desktopUser(session) + if !ok { + continue + } + if match(user) { return user, true } + user.Close() } - log.Debug("no interactive session is signed in, no user certificate store is reachable") + log.Debugf("certificate posture: profile owner %s has no signed-in session, no user certificate store is reachable", owner) return DesktopUser{}, false } +func consoleUser(console uint32) (DesktopUser, bool) { + if console == noActiveSession || console == servicesSession { + log.Debug("no console session, no user certificate store is reachable") + return DesktopUser{}, false + } + user, ok := desktopUser(console) + if !ok { + log.Debugf("console session %d has nobody signed in, no user certificate store is reachable", console) + } + return user, ok +} + +// ownerMatcher reports whether a session user is owner. Accounts are compared by SID, +// which is what identifies a Windows account; the name comparison is a fallback for an +// owner name that no longer resolves, and is case-insensitive like Windows account names. +func ownerMatcher(owner string) (func(DesktopUser) bool, error) { + ownerSID, _, _, err := windows.LookupSID("", owner) + if err != nil { + log.Debugf("certificate posture: resolving profile owner %s: %v, matching by name", owner, err) + return func(user DesktopUser) bool { return sameAccountName(user.Name, owner) }, nil + } + return func(user DesktopUser) bool { + tokenUser, err := user.Token.GetTokenUser() + if err != nil { + log.Debugf("failed reading token user of session %d: %v", user.Session, err) + return false + } + return tokenUser.User.Sid.Equals(ownerSID) + }, nil +} + +// sameAccountName compares DOMAIN\account names case-insensitively, and an owner given +// without a domain against the account part alone. +func sameAccountName(sessionName, owner string) bool { + if strings.EqualFold(sessionName, owner) { + return true + } + if strings.Contains(owner, `\`) { + return false + } + _, account, found := strings.Cut(sessionName, `\`) + return found && strings.EqualFold(account, owner) +} + func desktopUser(session uint32) (DesktopUser, bool) { var token windows.Token if err := windows.WTSQueryUserToken(session, &token); err != nil { @@ -97,7 +155,10 @@ func tokenAccount(token windows.Token) (string, error) { return domain + `\` + account, nil } -func activeSessions() ([]uint32, error) { +// userSessions lists the sessions a user can be signed in to, console first, then +// active remote sessions, then disconnected ones, whose user is still signed in. Session +// 0 is skipped: it hosts services and never belongs to an interactive user. +func userSessions(console uint32) ([]uint32, error) { var info *windows.WTS_SESSION_INFO var count uint32 if err := windows.WTSEnumerateSessions(wtsCurrentServer, 0, 1, &info, &count); err != nil { @@ -105,13 +166,19 @@ func activeSessions() ([]uint32, error) { } defer windows.WTSFreeMemory(uintptr(unsafe.Pointer(info))) - var active []uint32 - for _, session := range unsafe.Slice(info, count) { - if session.State == wtsActive { - active = append(active, session.SessionID) + var sessions []uint32 + if console != noActiveSession && console != servicesSession { + sessions = append(sessions, console) + } + for _, state := range []uint32{wtsActive, wtsDisconnected} { + for _, session := range unsafe.Slice(info, count) { + if session.SessionID == servicesSession || session.SessionID == console || session.State != state { + continue + } + sessions = append(sessions, session.SessionID) } } - return active, nil + return sessions, nil } // runningAsLocalSystem reports whether this process is the service. The helper runs as diff --git a/client/internal/certproof/desktopuser_windows_test.go b/client/internal/certproof/desktopuser_windows_test.go new file mode 100644 index 000000000..0b89562c4 --- /dev/null +++ b/client/internal/certproof/desktopuser_windows_test.go @@ -0,0 +1,35 @@ +package certproof + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSameAccountName(t *testing.T) { + tests := []struct { + session, owner string + want bool + }{ + {`CORP\alice`, `CORP\alice`, true}, + {`CORP\alice`, `corp\ALICE`, true}, + {`CORP\alice`, `alice`, true}, + {`CORP\alice`, `OTHER\alice`, false}, + {`CORP\alice`, `bob`, false}, + {`alice`, `alice`, true}, + {`CORP\alice`, `CORP\alic`, false}, + } + for _, tt := range tests { + assert.Equal(t, tt.want, sameAccountName(tt.session, tt.owner), "session %q, owner %q", tt.session, tt.owner) + } +} + +// CurrentDesktopUser against the real session manager: an owner no session belongs to +// must never yield a session, whoever else is signed in. +func TestCurrentDesktopUser_UnknownOwnerHasNoSession(t *testing.T) { + user, ok := CurrentDesktopUser(`NO-SUCH-DOMAIN\no-such-user-netbird`) + if ok { + user.Close() + } + assert.False(t, ok, "a profile owner without a session must not borrow another user's store") +} diff --git a/client/internal/certproof/store.go b/client/internal/certproof/store.go index 11b758cd4..8c2996905 100644 --- a/client/internal/certproof/store.go +++ b/client/internal/certproof/store.go @@ -47,12 +47,18 @@ type Store interface { Candidates(ctx context.Context) ([]Candidate, error) } -// Config selects where the Linux daemon looks for certificates: Dir is the PEM directory, +// Config selects where the daemon looks for certificates. Dir is the Linux PEM directory, // empty for NB_CERT_STORE_DIR or /etc/netbird/certs, and PKCS11 names a token whose keys // sign for certificates on the token or in that directory. +// +// ProfileOwner is the OS account the active profile belongs to. On macOS and Windows only +// that account's certificate store is consulted for user certificates, so on a machine +// with several people signed in the result does not depend on who else is logged in. +// Empty means the profile has no owner, and only the user at the physical console counts. type Config struct { - Dir string - PKCS11 PKCS11Config + Dir string + PKCS11 PKCS11Config + ProfileOwner string } func (c Config) dir() string { diff --git a/client/internal/connect.go b/client/internal/connect.go index a8485a994..e2ec230cf 100644 --- a/client/internal/connect.go +++ b/client/internal/connect.go @@ -76,6 +76,8 @@ type ConnectClient struct { // netMgr gates every reconnection loop on OS-reported network // availability and sweeps connections on network change. netMgr *netevents.Manager + + profileOwner string } // ConnectClientOption configures optional ConnectClient behavior. @@ -86,6 +88,12 @@ func WithNetEvents(events *netevents.Manager) ConnectClientOption { return func(c *ConnectClient) { c.netMgr = events } } +// WithProfileOwner names the OS account the active profile belongs to, whose own +// certificate store answers user certificate posture checks. +func WithProfileOwner(username string) ConnectClientOption { + return func(c *ConnectClient) { c.profileOwner = username } +} + func NewConnectClient( ctx context.Context, config *profilemanager.Config, @@ -417,6 +425,7 @@ func (c *ConnectClient) run(mobileDependency MobileDependency, runningChan chan return wrapErr(err) } engineConfig.TempDir = mobileDependency.TempDir + engineConfig.CertStore.ProfileOwner = c.profileOwner // Leave StateDir empty when there is no state path so a disk-backed // syncstore falls back to os.TempDir() instead of filepath.Dir("") == ".". if path != "" { diff --git a/client/server/server.go b/client/server/server.go index 108aa8a41..5cff2df74 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -2515,9 +2515,23 @@ func (s *Server) checkDisableAdvancedView() *bool { return nil } +// profileOwnerOption passes the OS account of the active profile to the connect client, +// which reads that account's certificate store for user certificate posture checks. +func (s *Server) profileOwnerOption() []internal.ConnectClientOption { + if s.profileManager == nil { + return nil + } + activeProf, err := s.profileManager.GetActiveProfileState() + if err != nil { + log.Debugf("no active profile owner for certificate posture: %v", err) + return nil + } + return []internal.ConnectClientOption{internal.WithProfileOwner(activeProf.Username)} +} + func (s *Server) connect(ctx context.Context, config *profilemanager.Config, statusRecorder *peer.Status, runningChan chan struct{}) error { log.Tracef("running client connection") - client := internal.NewConnectClient(ctx, config, statusRecorder) + client := internal.NewConnectClient(ctx, config, statusRecorder, s.profileOwnerOption()...) client.SetUpdateManager(s.updateManager) client.SetSyncResponsePersistence(s.persistSyncResponse)