From 3c745a8228901a039d18a30fbecf5a409499ba24 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Wed, 29 Jul 2026 18:00:04 +0200 Subject: [PATCH] Fail closed when a local group name cannot be resolved to a SID --- client/ssh/server/privileges_windows.go | 47 +++++++++++++++++--- client/ssh/server/privileges_windows_test.go | 38 ++++++++++++++++ client/ssh/server/user_utils.go | 12 ++--- 3 files changed, 81 insertions(+), 16 deletions(-) diff --git a/client/ssh/server/privileges_windows.go b/client/ssh/server/privileges_windows.go index 82da45231..981e2f073 100644 --- a/client/ssh/server/privileges_windows.go +++ b/client/ssh/server/privileges_windows.go @@ -4,6 +4,7 @@ package server import ( "fmt" + "strings" "unsafe" log "github.com/sirupsen/logrus" @@ -13,6 +14,12 @@ import ( var ( netapi32 = windows.NewLazySystemDLL("netapi32.dll") procNetUserGetLocalGroups = netapi32.NewProc("NetUserGetLocalGroups") + + // lookupGroupSID resolves a group name to its SID, indirected for testing. + lookupGroupSID = func(name string) (*windows.SID, error) { + sid, _, _, err := windows.LookupSID("", name) + return sid, err + } ) const ( @@ -136,23 +143,42 @@ func s4uTokenIsMember(account, domain string, sid *windows.SID) (bool, error) { } // localGroupsContainSID enumerates the local groups the account belongs to -// (including membership through global groups) and compares each group's SID -// against the wanted SID. Group names are resolved back to SIDs so the -// comparison is not sensitive to localization. +// (including membership through global groups) and reports whether the wanted +// SID is among them. Group names are resolved to SIDs so the comparison is not +// sensitive to localization. +// +// A group whose name does not resolve is compared against the wanted SID's own +// account name rather than skipped: skipping it would under-report membership, +// which for a privilege check means reporting a privileged account as +// unprivileged. If neither comparison is possible the error is returned so the +// caller can fail closed. func localGroupsContainSID(username string, want *windows.SID) (bool, error) { groups, err := netUserGetLocalGroups(username) if err != nil { return false, err } + + wantName, _, _, wantNameErr := want.LookupAccount("") + for _, group := range groups { - groupSid, _, _, err := windows.LookupSID("", group) - if err != nil { - log.Debugf("resolve local group %q to SID: %v", group, err) + groupSid, err := lookupGroupSID(group) + if err == nil { + if groupSid.Equals(want) { + return true, nil + } continue } - if groupSid.Equals(want) { + + if wantNameErr != nil { + return false, fmt.Errorf("resolve group %q: %w (and resolve wanted SID to a name: %w)", group, err, wantNameErr) + } + + // Local group names are unique per machine, so a name mismatch is + // conclusive even though the SID is unavailable. + if strings.EqualFold(group, wantName) { return true, nil } + log.Debugf("local group %q does not resolve to a SID and does not match %q: %v", group, wantName, err) } return false, nil } @@ -189,6 +215,13 @@ func netUserGetLocalGroups(username string) ([]string, error) { } }() + // MAX_PREFERRED_LENGTH makes the API allocate as much as it needs, so a + // short read is not expected. Report it rather than silently returning a + // subset of the account's groups. + if entriesRead != totalEntries { + return nil, fmt.Errorf("NetUserGetLocalGroups for %q returned %d of %d groups", username, entriesRead, totalEntries) + } + entries := unsafe.Slice((*localGroupUsersInfo0)(unsafe.Pointer(buf)), entriesRead) groups := make([]string, 0, entriesRead) for _, entry := range entries { diff --git a/client/ssh/server/privileges_windows_test.go b/client/ssh/server/privileges_windows_test.go index 45831dbe7..e9bc37720 100644 --- a/client/ssh/server/privileges_windows_test.go +++ b/client/ssh/server/privileges_windows_test.go @@ -3,6 +3,7 @@ package server import ( + "errors" "os/user" "testing" "unsafe" @@ -166,6 +167,43 @@ func TestS4UMembershipAgreesWithLocalGroups(t *testing.T) { t.Logf("checked %d local accounts via S4U", checked) } +// TestLocalGroupsContainSID_UnresolvableGroupMatchesByName covers the case +// where a group name cannot be resolved to a SID: membership must still be +// found by name instead of the group being skipped, which would report a +// privileged account as unprivileged. +func TestLocalGroupsContainSID_UnresolvableGroupMatchesByName(t *testing.T) { + adminSid, err := windows.CreateWellKnownSid(windows.WinBuiltinAdministratorsSid) + require.NoError(t, err, "create Administrators SID") + + original := lookupGroupSID + t.Cleanup(func() { lookupGroupSID = original }) + lookupGroupSID = func(string) (*windows.SID, error) { + return nil, errors.New("simulated SID resolution failure") + } + + // The built-in Administrator is always a member of Administrators. + member, err := localGroupsContainSID("Administrator", adminSid) + require.NoError(t, err, "enumerate local groups for Administrator") + assert.True(t, member, "unresolvable group must be matched by name, not skipped") +} + +// TestLocalGroupsContainSID_UnresolvableFailsClosed covers the case where +// neither the group SID nor the wanted SID's name can be resolved: the error +// must surface so the caller treats the account as privileged. +func TestLocalGroupsContainSID_UnresolvableFailsClosed(t *testing.T) { + // A SID that resolves to no account, so LookupAccount fails. + unknown := mustParseSID(t, "S-1-5-21-1111111111-2222222222-3333333333-4444") + + original := lookupGroupSID + t.Cleanup(func() { lookupGroupSID = original }) + lookupGroupSID = func(string) (*windows.SID, error) { + return nil, errors.New("simulated SID resolution failure") + } + + _, err := localGroupsContainSID("Administrator", unknown) + require.Error(t, err, "must report an error when membership cannot be determined either way") +} + func TestLocalGroupsContainSID_Guest(t *testing.T) { guestsSid, err := windows.CreateWellKnownSid(windows.WinBuiltinGuestsSid) require.NoError(t, err, "create Guests SID") diff --git a/client/ssh/server/user_utils.go b/client/ssh/server/user_utils.go index 5ffa3c8a1..93a86fd18 100644 --- a/client/ssh/server/user_utils.go +++ b/client/ssh/server/user_utils.go @@ -189,16 +189,10 @@ func (s *Server) SetAllowRootLogin(allow bool) { // userNameLookup performs user lookup with root login permission check func (s *Server) userNameLookup(username string) (*user.User, error) { - result := s.CheckPrivileges(PrivilegeCheckRequest{ - RequestedUsername: username, - FeatureSupportsUserSwitch: true, - FeatureName: FeatureSSHLogin, - }) - - if !result.Allowed { - return nil, result.Error + result, err := s.userPrivilegeCheck(username) + if err != nil { + return nil, err } - return result.User, nil }