[client] Only treat LocalSystem as a privileged identity by SID on Windows (#7889) (#7904)

(cherry picked from commit 6425b04200)

Co-authored-by: Viktor Liu <17948409+lixmal@users.noreply.github.com>

client/internal/ipcauth/forward_test.go

@@ -38,7 +38,7 @@ func asDaemon(t *testing.T, id Identity) {
 	prevID, prevKnown, prevDelegate := selfIdentity, selfKnown, selfMayDelegate
 	t.Cleanup(func() { selfIdentity, selfKnown, selfMayDelegate = prevID, prevKnown, prevDelegate })
 	selfIdentity, selfKnown = id, true
-	selfMayDelegate = !id.IsPrivileged()
+	selfMayDelegate = mayDelegate(id)
 }
 
 func TestCallerIdentity_DirectConnections(t *testing.T) {

client/internal/ipcauth/identity.go

@@ -18,7 +18,8 @@ import (
 	"google.golang.org/grpc/peer"
 )
 
-// Well-known Windows SIDs that identify a fully privileged principal.
+// Well-known Windows SIDs. Only LocalSystem and BUILTIN\Administrators identify a
+// privileged principal; the service accounts are shared by unrelated services.
 const (
 	sidLocalSystem    = "S-1-5-18"     // NT AUTHORITY\SYSTEM
 	sidLocalService   = "S-1-5-19"     // NT AUTHORITY\LOCAL SERVICE
@@ -67,9 +68,9 @@ func (i Identity) IsWindows() bool {
 // user-to-root boundary.
 //
 // On Windows the decision comes from the caller's token rather than from
-// account names or group RIDs: an elevated token, one of the service accounts
+// account names or group RIDs: an elevated token, the LocalSystem SID, or a
-// the daemon itself may run as, or a token with BUILTIN\Administrators
+// token with BUILTIN\Administrators enabled. LocalService and NetworkService
-// enabled. A UAC-filtered administrator has that group marked deny-only, and
+// are not privileged by SID. A UAC-filtered administrator has that group marked deny-only, and
 // deny-only groups are dropped when the identity is captured, so such a
 // caller is correctly reported as unprivileged. Domain group memberships
 // (Domain Admins and friends) are deliberately not consulted: they say
@@ -83,8 +84,7 @@ func (i Identity) IsPrivileged() bool {
 		return true
 	}
 
-	switch i.SID {
+	if i.SID == sidLocalSystem {
-	case sidLocalSystem, sidLocalService, sidNetworkService:
 		return true
 	}
 

client/internal/ipcauth/identity_test.go

@@ -64,3 +64,58 @@ func TestIdentitySameUser(t *testing.T) {
 		})
 	}
 }
+
+func TestIdentityIsPrivileged(t *testing.T) {
+	tests := []struct {
+		name string
+		id   Identity
+		want bool
+	}{
+		{
+			name: "Root",
+			id:   Identity{UID: 0, GID: 0},
+			want: true,
+		},
+		{
+			name: "Non-root",
+			id:   Identity{UID: 1000, GID: 1000},
+			want: false,
+		},
+		{
+			name: "Local system windows",
+			id:   Identity{SID: sidLocalSystem},
+			want: true,
+		},
+		{
+			name: "Windows elevated",
+			id:   Identity{SID: "S-1-5-21-1927267129-3959769253-3036563910-1001", Elevated: true},
+			want: true,
+		},
+		{
+			name: "Admin group windows",
+			id:   Identity{SID: "S-1-5-21-1927267129-3959769253-3036563910-1001", Groups: []string{sidAdministrators}},
+			want: true,
+		},
+		{
+			name: "Regular user windows",
+			id:   Identity{SID: "S-1-5-21-1927267129-3959769253-3036563910-1001"},
+			want: false,
+		},
+		{
+			name: "Network service windows",
+			id:   Identity{SID: sidNetworkService},
+			want: false,
+		},
+		{
+			name: "Local service windows",
+			id:   Identity{SID: sidLocalService},
+			want: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, tt.id.IsPrivileged())
+		})
+	}
+}

client/internal/ipcauth/privileged.go

@@ -45,7 +45,15 @@ func init() {
 	// matching there would let a non-elevated shell of an administrator account
 	// act as an administrator, which is the boundary the token check exists to
 	// keep.
-	selfMayDelegate = !id.IsPrivileged()
+	selfMayDelegate = mayDelegate(id)
+}
+
+// mayDelegate reports whether a daemon running as id may extend its authority to
+// callers sharing its identity. The shared service accounts are excluded: their
+// SID is held by unrelated services, so matching on it would grant them the
+// daemon's authority.
+func mayDelegate(id Identity) bool {
+	return !id.IsPrivileged() && id.SID != sidLocalService && id.SID != sidNetworkService
 }
 
 // IsDaemonSelf reports whether an identity is this very process. The JSON gateway

client/internal/ipcauth/privileged_test.go

@@ -98,7 +98,7 @@ func TestIsPrivilegedCaller_SelfRule(t *testing.T) {
 			t.Cleanup(func() { selfIdentity, selfKnown, selfMayDelegate = prevID, prevKnown, prevDelegate })
 
 			selfIdentity, selfKnown = tt.self, tt.selfKnown
-			selfMayDelegate = tt.selfKnown && !tt.self.IsPrivileged()
+			selfMayDelegate = tt.selfKnown && mayDelegate(tt.self)
 
 			if got := IsPrivilegedCaller(tt.caller); got != tt.want {
 				t.Fatalf("IsPrivilegedCaller(%v) with daemon %v = %t, want %t",
@@ -132,3 +132,28 @@ func TestIsPrivilegedCaller_ThisProcess(t *testing.T) {
 		t.Errorf("an unrelated identity %v was treated as privileged", other)
 	}
 }
+
+// The shared service accounts are held by unrelated services, so a daemon running
+// as one of them must not extend its authority to every process with that SID.
+func TestMayDelegate(t *testing.T) {
+	tests := []struct {
+		name string
+		self Identity
+		want bool
+	}{
+		{name: "unprivileged unix user", self: Identity{UID: 1000}, want: true},
+		{name: "root", self: Identity{UID: 0}, want: false},
+		{name: "unprivileged windows user", self: Identity{SID: "S-1-5-21-1-2-3-1001"}, want: true},
+		{name: "elevated windows user", self: Identity{SID: "S-1-5-21-1-2-3-1001", Elevated: true}, want: false},
+		{name: "local system", self: Identity{SID: sidLocalSystem}, want: false},
+		{name: "local service", self: Identity{SID: sidLocalService}, want: false},
+		{name: "network service", self: Identity{SID: sidNetworkService}, want: false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := mayDelegate(tt.self); got != tt.want {
+				t.Errorf("mayDelegate(%+v) = %v, want %v", tt.self, got, tt.want)
+			}
+		})
+	}
+}