Harden the elevation trust checks and narrow what one authorization applies

client/ui/services/guarded.go

@@ -16,11 +16,9 @@ import (
 )
 
 // The command line of the one-shot mode this binary runs itself in, elevated, to
-// apply a setting the daemon restricts to root/administrator. Named here, next to
-// the code that builds the arguments; parsed by runPrivilegedSettings in the main
-// package. The setting flags deliberately spell the same words as `netbird up`,
-// so what the user is shown as a command and what runs behind the prompt read
-// alike.
+// apply a setting the daemon restricts to root/administrator. The setting flags
+// spell the same words as `netbird up`, so the command a user is shown and what
+// runs behind the prompt read alike. Parsed in oneshot.go.
 const (
 	FlagApplyPrivilegedSettings = "apply-privileged-settings"
 	FlagDaemonAddr              = "daemon-addr"
@@ -39,17 +37,14 @@ const (
 	CodeElevationFailed      = "elevation_failed"
 )
 
-// elevationTimeout bounds the wait for a prompt and the change behind it, so an
-// authentication dialog nobody ever answers does not leave the control it belongs
-// to disabled for the rest of the session. Long enough to find a password manager,
-// and no shorter than the platforms' own prompt timeouts (Windows gives up on its
-// consent dialog after two minutes by itself).
+// elevationTimeout bounds the wait for a prompt and the change behind it, so a
+// dialog nobody answers does not leave its control disabled for the session. Long
+// enough to find a password manager, and no shorter than the platforms' own prompt
+// timeouts: Windows gives up on its consent dialog after two minutes by itself.
 //
-// How much it can actually interrupt differs. On Linux the prompt is a child
-// process and is killed with the context; on Windows the wait for it is
-// interruptible. On macOS the dialog belongs to Security.framework, which offers no
-// way to withdraw the request, so there the timeout only stops us waiting — the
-// system's own dialog timeout is what ends it.
+// It always ends our waiting, and not always the prompt: Security.framework offers
+// no way to withdraw a request, so on macOS the system's own timeout is what closes
+// the dialog.
 const elevationTimeout = 5 * time.Minute
 
 // elevator raises the platform's privilege prompt and runs the change behind it.
@@ -141,11 +136,9 @@ func (s *Settings) SetGuardedSettings(ctx context.Context, p GuardedSettings) (S
 	ctx, cancel := context.WithTimeout(ctx, elevationTimeout)
 	defer cancel()
 
-	// Both ends of it: when the prompt went up, and what came of it. These are
-	// changes that hand out shells on this host, so the log should say who was
-	// asked and when, and it is also the only account of a prompt that was slow to
-	// appear or never answered. The daemon records the change itself, against the
-	// identity it authorized.
+	// These changes hand out shells on this host, so both ends are logged: when the
+	// prompt went up, and what came of it. It is also the only account of a prompt
+	// that was slow to appear or never answered.
 	log.Infof("asking for privileges to apply %s", guardedSummary(p))
 
 	if err := s.elevator.Run(ctx, args...); err != nil {

client/ui/services/guarded_test.go

@@ -23,6 +23,10 @@ import (
 // elevation is worth offering at all: see Settings.canElevate.
 const testDaemonAddr = "unix:///var/run/netbird.sock"
 
+// storedManagementURL is what the stub daemon already holds, so that a request
+// naming a different one is a change: see Settings.guardedChanges.
+const storedManagementURL = "https://stored.example.com"
+
 // stubElevator stands in for the platform's prompt: it records what would have run
 // and answers with a fixed outcome.
 type stubElevator struct {
@@ -38,12 +42,15 @@ func (e *stubElevator) Run(_ context.Context, args ...string) error {
 
 func (e *stubElevator) Available() bool { return e.available }
 
-// stubDaemon implements only the RPC under test. The embedded interface is nil, so
+// stubDaemon implements only the RPCs under test. The embedded interface is nil, so
 // any other call panics rather than passing quietly.
 type stubDaemon struct {
 	proto.DaemonServiceClient
 	setConfig func(*proto.SetConfigRequest) error
-	requests  []*proto.SetConfigRequest
+	// stored is what GetConfig reports, which is what a refused request's guarded
+	// settings are compared against.
+	stored   *proto.GetConfigResponse
+	requests []*proto.SetConfigRequest
 }
 
 func (d *stubDaemon) SetConfig(_ context.Context, in *proto.SetConfigRequest, _ ...grpc.CallOption) (*proto.SetConfigResponse, error) {
@@ -54,6 +61,10 @@ func (d *stubDaemon) SetConfig(_ context.Context, in *proto.SetConfigRequest, _
 	return &proto.SetConfigResponse{}, nil
 }
 
+func (d *stubDaemon) GetConfig(_ context.Context, _ *proto.GetConfigRequest, _ ...grpc.CallOption) (*proto.GetConfigResponse, error) {
+	return d.stored, nil
+}
+
 type stubConn struct{ client proto.DaemonServiceClient }
 
 func (c stubConn) Client() (proto.DaemonServiceClient, error) { return c.client, nil }
@@ -84,12 +95,14 @@ func settingsWithElevation(t *testing.T, outcome error) (*Settings, *stubElevato
 }
 
 // settingsRefusingOnce returns a Settings whose daemon refuses the first SetConfig
-// for want of privileges and accepts anything after it.
+// for want of privileges and accepts anything after it. Its stored config holds
+// another management server and no SSH grants, so a request naming either is a
+// change rather than a restatement.
 func settingsRefusingOnce(t *testing.T, elev *stubElevator) (*Settings, *stubDaemon) {
 	t.Helper()
 
 	refusal := privilegeRefusal(t)
-	daemon := &stubDaemon{}
+	daemon := &stubDaemon{stored: &proto.GetConfigResponse{ManagementUrl: storedManagementURL}}
 	daemon.setConfig = func(*proto.SetConfigRequest) error {
 		if len(daemon.requests) == 1 {
 			return refusal
@@ -275,6 +288,52 @@ func TestSetConfigReportsTheRefusalWhenItCannotElevate(t *testing.T) {
 	assert.Empty(t, elev.calls, "no prompt where there is none to raise")
 }
 
+// One authorization must buy only the change the user made. A settings form
+// submits every field it holds, so most of a refused request restates what the
+// daemon already has, and elevating those too would apply a guarded setting the
+// user never touched — a value gone stale since the form loaded above all.
+func TestSetConfigElevatesOnlyTheGuardedSettingsThatChange(t *testing.T) {
+	elev := &stubElevator{available: true}
+	s, _ := settingsRefusingOnce(t, elev)
+
+	on, off := true, false
+	_, err := s.SetConfig(context.Background(), SetConfigParams{
+		ProfileName:      "default",
+		ManagementURL:    storedManagementURL,
+		ServerSSHAllowed: &off,
+		EnableSSHRoot:    &off,
+		DisableSSHAuth:   &on,
+	})
+	require.NoError(t, err)
+
+	require.Len(t, elev.calls, 1, "one prompt")
+	args := elev.calls[0]
+	assert.Contains(t, args, "--"+FlagDisableSSHAuth+"=true", "the setting that changes")
+	assert.NotContains(t, args, "--"+FlagManagementURL+"="+storedManagementURL,
+		"a management URL the daemon already holds")
+	assert.NotContains(t, args, "--"+FlagAllowServerSSH+"=false", "a setting already off")
+	assert.NotContains(t, args, "--"+FlagEnableSSHRoot+"=false", "a setting already off")
+}
+
+// A request that changes no guarded setting has nothing an elevated run could
+// apply, so the refusal must have come from somewhere a prompt cannot reach.
+func TestSetConfigDoesNotElevateWhenNoGuardedSettingChanges(t *testing.T) {
+	elev := &stubElevator{available: true}
+	s, _ := settingsRefusingOnce(t, elev)
+
+	off := false
+	_, err := s.SetConfig(context.Background(), SetConfigParams{
+		ProfileName:      "default",
+		ManagementURL:    storedManagementURL,
+		ServerSSHAllowed: &off,
+	})
+
+	var clientErr *ClientError
+	require.ErrorAs(t, err, &clientErr)
+	assert.Equal(t, "privilege_required", clientErr.Code, "error code")
+	assert.Empty(t, elev.calls, "no prompt for a change nobody made")
+}
+
 // A refusal with nothing in the request the one-shot could apply: the daemon
 // cannot see who is calling, and being root would not help either.
 func TestSetConfigReportsARefusalWithNothingToElevate(t *testing.T) {

client/ui/services/oneshot.go

@@ -14,6 +14,7 @@ import (
 	gstatus "google.golang.org/grpc/status"
 
 	"github.com/netbirdio/netbird/client/internal/elevate"
+	"github.com/netbirdio/netbird/client/internal/profilemanager"
 	"github.com/netbirdio/netbird/client/proto"
 	"github.com/netbirdio/netbird/util"
 )
@@ -64,6 +65,11 @@ var guardedFields = []guardedField{
 		usage: "Management server the profile registers with.",
 		read:  func(p GuardedSettings) (string, bool) { return p.ManagementURL, p.ManagementURL != "" },
 		write: func(req *proto.SetConfigRequest, value string) error {
+			// Parsed with the config layer's own parser, so what the elevated run
+			// accepts cannot drift from what the daemon would store.
+			if _, err := profilemanager.ParseServiceURL("Management URL", value); err != nil {
+				return err
+			}
 			req.ManagementUrl = value
 			return nil
 		},

client/ui/services/oneshot_test.go

@@ -3,6 +3,7 @@
 package services
 
 import (
+	"flag"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -123,44 +124,28 @@ func TestPrivilegedRequestRejectsAnUnparseableValue(t *testing.T) {
 }
 
 // parseRendered puts the settings through both ends: rendered as the arguments the
-// elevated process is given, then parsed as that process parses them.
+// elevated process is given, then parsed by a flag set registered from the same
+// table, which is what the one-shot itself parses them with. Anything hand-rolled
+// here would pin down a parser nothing uses.
 func parseRendered(t *testing.T, p GuardedSettings) *proto.SetConfigRequest {
 	t.Helper()
 
 	rendered := guardedSettings(p)
 	require.NotEmpty(t, rendered, "nothing rendered for %+v", p)
 
-	values := make([]fieldValue, len(guardedFields))
+	args := make([]string, 0, len(rendered))
 	for _, setting := range rendered {
-		flag, value, found := splitFlag(setting.arg)
-		require.True(t, found, "rendered %q without a value", setting.arg)
-
-		matched := false
-		for i, field := range guardedFields {
-			if field.flag != flag {
-				continue
-			}
-			require.NoError(t, values[i].Set(value))
-			matched = true
-		}
-		require.True(t, matched, "rendered %q, which no field claims", setting.arg)
+		args = append(args, setting.arg)
 	}
 
+	fs := flag.NewFlagSet(t.Name(), flag.ContinueOnError)
+	values := make([]fieldValue, len(guardedFields))
+	for i, field := range guardedFields {
+		fs.Var(&values[i], field.flag, field.usage)
+	}
+	require.NoError(t, fs.Parse(args), "the one-shot's own flag set must accept %v", args)
+
 	req, err := privilegedRequest(p.ProfileName, p.Username, values)
 	require.NoError(t, err)
 	return req
 }
-
-// splitFlag takes "--name=value" apart the way the flag package does.
-func splitFlag(arg string) (name, value string, found bool) {
-	trimmed := arg
-	for len(trimmed) > 0 && trimmed[0] == '-' {
-		trimmed = trimmed[1:]
-	}
-	for i := 0; i < len(trimmed); i++ {
-		if trimmed[i] == '=' {
-			return trimmed[:i], trimmed[i+1:], true
-		}
-	}
-	return trimmed, "", false
-}

client/ui/services/settings.go

@@ -252,13 +252,10 @@ func (s *Settings) setConfigElevated(ctx context.Context, p SetConfigParams, req
 		return SaveOutcome{}, s.classifier.classify(refusal)
 	}
 
-	guarded := GuardedSettings{
-		ProfileName:      p.ProfileName,
-		Username:         p.Username,
-		ManagementURL:    p.ManagementURL,
-		ServerSSHAllowed: p.ServerSSHAllowed,
-		EnableSSHRoot:    p.EnableSSHRoot,
-		DisableSSHAuth:   p.DisableSSHAuth,
+	guarded, err := s.guardedChanges(ctx, p)
+	if err != nil {
+		log.Warnf("cannot tell which guarded settings this request changes: %v", err)
+		return SaveOutcome{}, s.classifier.classify(refusal)
 	}
 	if len(guardedSettings(guarded)) == 0 {
 		// Refused over something no prompt can settle, such as a control channel
@@ -281,6 +278,33 @@ func (s *Settings) setConfigElevated(ctx context.Context, p SetConfigParams, req
 	return SaveOutcome{}, nil
 }
 
+// guardedChanges is the guarded part of a request, reduced to what it actually
+// changes.
+//
+// A settings form submits every field it holds, so a request restates values the
+// daemon already has. Carrying those into the elevated run would spend one
+// authorization on more than the user asked for, and a value that has gone stale
+// since the form was loaded would spend it on something they never asked about.
+func (s *Settings) guardedChanges(ctx context.Context, p SetConfigParams) (GuardedSettings, error) {
+	stored, err := s.GetConfig(ctx, ConfigParams{ProfileName: p.ProfileName, Username: p.Username})
+	if err != nil {
+		return GuardedSettings{}, fmt.Errorf("read the stored config: %w", err)
+	}
+
+	guarded := GuardedSettings{
+		ProfileName:      p.ProfileName,
+		Username:         p.Username,
+		ServerSSHAllowed: changedFlag(p.ServerSSHAllowed, stored.ServerSSHAllowed),
+		EnableSSHRoot:    changedFlag(p.EnableSSHRoot, stored.EnableSSHRoot),
+		DisableSSHAuth:   changedFlag(p.DisableSSHAuth, stored.DisableSSHAuth),
+	}
+	// An empty URL leaves the setting alone, which is the daemon's rule too.
+	if p.ManagementURL != "" && p.ManagementURL != stored.ManagementURL {
+		guarded.ManagementURL = p.ManagementURL
+	}
+	return guarded, nil
+}
+
 // Privilege reports whether this UI process could carry out the changes the
 // daemon restricts to root/administrator, whether it can instead ask the
 // operating system for the privileges when the user wants one of them, and the
@@ -363,6 +387,15 @@ func (s *Settings) GetRestrictions(ctx context.Context) (Restrictions, error) {
 	return r, nil
 }
 
+// changedFlag returns requested only when it differs from what is stored, so a
+// setting the request merely restates is left out of the elevated run.
+func changedFlag(requested *bool, stored bool) *bool {
+	if requested == nil || *requested == stored {
+		return nil
+	}
+	return requested
+}
+
 func applyMDMRestrictions(mdm *MDMFields, cfgResp *proto.GetConfigResponse) {
 	managed := cfgResp.GetMDMManagedFields()
 	if len(managed) == 0 {