Fail closed when a local group name cannot be resolved to a SID

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 {

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")

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
 }