fix: harden read-only target diagnostics

internal/dockerclient/dockerclient.go

@@ -188,9 +188,12 @@ func (a *Adapter) DiagnosticChecks(_ context.Context, _ *profile.Profile) []targ
 	} else if proxies, _, err := childObject(config, "proxies"); err != nil {
 		schemaCheck.OK = false
 		schemaCheck.Evidence = profile.RedactText(err.Error())
-	} else if _, _, err := childObject(proxies, "default"); err != nil {
+	} else if defaults, _, err := childObject(proxies, "default"); err != nil {
 		schemaCheck.OK = false
 		schemaCheck.Evidence = profile.RedactText(err.Error())
+	} else if err := validateManagedProxyStrings(defaults); err != nil {
+		schemaCheck.OK = false
+		schemaCheck.Evidence = err.Error()
 	} else if _, _, err := a.readRecovery(); err != nil && !errors.Is(err, fs.ErrNotExist) {
 		schemaCheck.OK = false
 		schemaCheck.Evidence = profile.RedactText("Docker client recovery metadata is invalid or unreadable: " + err.Error())
@@ -305,6 +308,9 @@ func (a *Adapter) enable(active *profile.Profile) error {
 	if err != nil {
 		return err
 	}
+	if err := validateManagedProxyStrings(defaults); err != nil {
+		return err
+	}
 	if !managed {
 		metadata = recovery{ConfigPresent: configPresent, ProxiesPresent: proxiesPresent, DefaultPresent: defaultPresent, Original: make(map[string]priorValue)}
 		for _, key := range managedKeys {
@@ -463,6 +469,20 @@ func childObject(parent map[string]json.RawMessage, key string) (map[string]json
 	object, err := decodeObject(raw, "Docker client "+key+" setting")
 	return object, true, err
 }
+func validateManagedProxyStrings(defaults map[string]json.RawMessage) error {
+	for _, key := range managedKeys {
+		raw, present := defaults[key]
+		if !present {
+			continue
+		}
+		var value string
+		if err := json.Unmarshal(raw, &value); err != nil {
+			return fmt.Errorf("Docker client proxies.default.%s must be a JSON string", key)
+		}
+	}
+	return nil
+}
+
 func setChildObject(parent map[string]json.RawMessage, key string, child map[string]json.RawMessage) {
 	data, _ := json.Marshal(child)
 	parent[key] = data

internal/dockerclient/dockerclient_test.go

@@ -67,6 +67,43 @@ func TestDiagnosticChecksCoverSchemaAndAuthorityWithoutMutation(t *testing.T) {
 	}
 }
 
+func TestDiagnosticsAndEnableRejectMalformedManagedProxyTypesWithoutMutation(t *testing.T) {
+	for _, document := range []string{
+		`{"proxies":[]}`,
+		`{"proxies":{"default":[]}}`,
+		`{"proxies":{"default":{"httpProxy":42}}}`,
+		`{"proxies":{"default":{"httpsProxy":false}}}`,
+		`{"proxies":{"default":{"allProxy":{}}}}`,
+		`{"proxies":{"default":{"noProxy":[]}}}`,
+	} {
+		t.Run(document, func(t *testing.T) {
+			files := &countingFS{FileSystem: dockerclient.OSFileSystem{}}
+			a := newAdapterWithFS(t, files)
+			writeConfig(t, a.ConfigPath(), document)
+			before, err := os.ReadFile(a.ConfigPath())
+			if err != nil {
+				t.Fatal(err)
+			}
+			checks := a.DiagnosticChecks(context.Background(), activeProfile())
+			if checks[1].OK {
+				t.Fatalf("malformed schema passed diagnostics: %#v", checks)
+			}
+			writesBefore := files.writes
+			result := a.SetDesired(context.Background(), true, activeProfile())
+			if result.Err == nil {
+				t.Fatalf("enable accepted malformed schema: %#v", result)
+			}
+			after, err := os.ReadFile(a.ConfigPath())
+			if err != nil {
+				t.Fatal(err)
+			}
+			if string(after) != string(before) || files.writes != writesBefore {
+				t.Fatalf("malformed config was modified: writes %d -> %d", writesBefore, files.writes)
+			}
+		})
+	}
+}
+
 func TestDockerClientAdapterRunsReusableTargetContract(t *testing.T) {
 	targettest.Run(t, func(t *testing.T) targettest.Fixture {
 		a := newAdapter(t, nil)

internal/dockerdaemon/dockerdaemon.go

@@ -68,15 +68,24 @@ func (ExecRunner) Run(ctx context.Context, name string, args ...string) ([]byte,
 	return output, nil
 }
 
+type ProcessAuthority interface {
+	EffectiveUID() int
+}
+
+type processAuthority struct{}
+
+func (processAuthority) EffectiveUID() int { return os.Geteuid() }
+
 type Adapter struct {
 	configuration Configuration
 	files         FileSystem
 	runner        CommandRunner
+	authority     ProcessAuthority
 }
 
 var _ target.Diagnosable = (*Adapter)(nil)
 
-func New(configuration Configuration, files FileSystem, runner CommandRunner) *Adapter {
+func New(configuration Configuration, files FileSystem, runner CommandRunner, authorities ...ProcessAuthority) *Adapter {
 	if configuration.DaemonConfigPath == "" {
 		configuration.DaemonConfigPath = "/etc/docker/daemon.json"
 	}
@@ -90,7 +99,11 @@ func New(configuration Configuration, files FileSystem, runner CommandRunner) *A
 	if runner == nil {
 		runner = ExecRunner{}
 	}
-	return &Adapter{configuration: configuration, files: files, runner: runner}
+	authority := ProcessAuthority(processAuthority{})
+	if len(authorities) != 0 && authorities[0] != nil {
+		authority = authorities[0]
+	}
+	return &Adapter{configuration: configuration, files: files, runner: runner, authority: authority}
 }
 
 func (a *Adapter) ID() string { return "docker-daemon" }
@@ -119,9 +132,9 @@ func (a *Adapter) DiagnosticChecks(ctx context.Context, _ *profile.Profile) []ta
 	}
 	schemaCheck := target.Check{Name: "configuration schema", OK: true, Evidence: "Docker daemon configuration and recovery metadata are absent or valid JSON"}
 	if data, err := a.files.ReadFile(a.configuration.DaemonConfigPath); err == nil {
-		if _, schemaErr := mergeConfig(data, profile.Profile{}); schemaErr != nil {
+		if schemaErr := validateConfigSchema(data); schemaErr != nil {
 			schemaCheck.OK = false
-			schemaCheck.Evidence = profile.RedactText("Docker daemon configuration cannot be safely merged: " + schemaErr.Error())
+			schemaCheck.Evidence = profile.RedactText("Docker daemon configuration schema is invalid: " + schemaErr.Error())
 		}
 	} else if !errors.Is(err, fs.ErrNotExist) {
 		schemaCheck.OK = false
@@ -133,10 +146,17 @@ func (a *Adapter) DiagnosticChecks(ctx context.Context, _ *profile.Profile) []ta
 			schemaCheck.Evidence = profile.RedactText("Docker daemon recovery metadata is invalid or unreadable: " + err.Error())
 		}
 	}
+	serviceAuthority := target.Check{Name: "service control authority", OK: a.authority.EffectiveUID() == 0}
+	if serviceAuthority.OK {
+		serviceAuthority.Evidence = "effective process authority can control the system Docker service"
+	} else {
+		serviceAuthority.Evidence = "effective process authority cannot reload or restart the system Docker service; run proxyctl explicitly with sufficient authority"
+	}
 	return []target.Check{
 		commandCheck,
 		schemaCheck,
 		target.ReadOnlyWriteAuthority(a.configuration.DaemonConfigPath, a.configuration.RecoveryPath),
+		serviceAuthority,
 	}
 }
 
@@ -366,16 +386,45 @@ func (a *Adapter) writeRecovery(value recovery) error {
 	return a.files.AtomicWrite(a.configuration.RecoveryPath, append(data, '\n'), 0o600)
 }
 
-func mergeConfig(data []byte, p profile.Profile) ([]byte, error) {
+func validateConfigSchema(data []byte) error {
+	if len(strings.TrimSpace(string(data))) == 0 {
+		return errors.New("configuration must be a JSON object")
+	}
 	var document map[string]any
+	if err := json.Unmarshal(data, &document); err != nil {
+		return err
+	}
+	if document == nil {
+		return errors.New("configuration must be a JSON object")
+	}
+	existing, present := document["proxies"]
+	if !present {
+		return nil
+	}
+	proxies, ok := existing.(map[string]any)
+	if !ok {
+		return errors.New("the existing proxies field is not an object")
+	}
+	for _, key := range []string{"http-proxy", "https-proxy", "no-proxy"} {
+		if value, present := proxies[key]; present {
+			if _, ok := value.(string); !ok {
+				return fmt.Errorf("the existing proxies.%s field is not a string", key)
+			}
+		}
+	}
+	return nil
+}
+
+func mergeConfig(data []byte, p profile.Profile) ([]byte, error) {
 	if len(strings.TrimSpace(string(data))) == 0 {
 		data = []byte("{}")
 	}
-	if err := json.Unmarshal(data, &document); err != nil {
+	if err := validateConfigSchema(data); err != nil {
 		return nil, err
 	}
-	if document == nil {
-		document = map[string]any{}
+	var document map[string]any
+	if err := json.Unmarshal(data, &document); err != nil {
+		return nil, err
 	}
 	proxies := map[string]any{}
 	if existing, ok := document["proxies"].(map[string]any); ok {

internal/dockerdaemon/dockerdaemon_test.go

@@ -102,10 +102,15 @@ func (f *faultFS) AtomicWrite(path string, data []byte, mode os.FileMode) error
 	return f.recordingFS.AtomicWrite(path, data, mode)
 }
 
+type fixedAuthority int
+
+func (a fixedAuthority) EffectiveUID() int { return int(a) }
+
 func TestDiagnosticChecksCoverSystemdSchemaAndAuthorityWithoutMutation(t *testing.T) {
-	a, record, daemonPath, recoveryPath, _ := fixture(t)
+	a, record, daemonPath, recoveryPath, runner := fixture(t)
+	a = dockerdaemon.New(dockerdaemon.Configuration{DaemonConfigPath: daemonPath, RecoveryPath: recoveryPath, TrustedRoot: filepath.Dir(filepath.Dir(daemonPath))}, recordingFS{recorder: record}, runner, fixedAuthority(0))
 	checks := a.DiagnosticChecks(context.Background(), activeProfile())
-	for _, name := range []string{"required command and service", "configuration schema", "write authority"} {
+	for _, name := range []string{"required command and service", "configuration schema", "write authority", "service control authority"} {
 		found := false
 		for _, check := range checks {
 			found = found || check.Name == name
@@ -126,6 +131,75 @@ func TestDiagnosticChecksCoverSystemdSchemaAndAuthorityWithoutMutation(t *testin
 	}
 }
 
+func TestDiagnosticServiceControlAuthorityUsesInjectedEUIDWithoutMutation(t *testing.T) {
+	for _, test := range []struct {
+		name string
+		uid  int
+		ok   bool
+	}{
+		{name: "unprivileged", uid: 1000, ok: false},
+		{name: "root", uid: 0, ok: true},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			_, record, daemonPath, recoveryPath, runner := fixture(t)
+			a := dockerdaemon.New(dockerdaemon.Configuration{DaemonConfigPath: daemonPath, RecoveryPath: recoveryPath}, recordingFS{recorder: record}, runner, fixedAuthority(test.uid))
+			checks := a.DiagnosticChecks(context.Background(), activeProfile())
+			var found *target.Check
+			for i := range checks {
+				if checks[i].Name == "service control authority" {
+					found = &checks[i]
+				}
+			}
+			if found == nil || found.OK != test.ok {
+				t.Fatalf("service authority check = %#v", found)
+			}
+			for _, event := range record.events {
+				if strings.Contains(event, "restart") || strings.Contains(event, "daemon-reload") {
+					t.Fatalf("authority diagnosis mutated service: %#v", record.events)
+				}
+			}
+		})
+	}
+}
+
+func TestDiagnosticsAndEnableRejectMalformedDaemonProxyTypesWithoutMutation(t *testing.T) {
+	for _, document := range []string{
+		`[]`,
+		`{"proxies":[]}`,
+		`{"proxies":{"http-proxy":42}}`,
+		`{"proxies":{"https-proxy":false}}`,
+		`{"proxies":{"no-proxy":[]}}`,
+	} {
+		t.Run(document, func(t *testing.T) {
+			a, record, daemonPath, _, _ := fixture(t)
+			if err := os.MkdirAll(filepath.Dir(daemonPath), 0o700); err != nil {
+				t.Fatal(err)
+			}
+			if err := os.WriteFile(daemonPath, []byte(document), 0o600); err != nil {
+				t.Fatal(err)
+			}
+			checks := a.DiagnosticChecks(context.Background(), activeProfile())
+			if checks[1].OK {
+				t.Fatalf("malformed schema passed diagnostics: %#v", checks)
+			}
+			record.events = nil
+			result := a.SetDesired(context.Background(), true, activeProfile())
+			if result.Err == nil {
+				t.Fatalf("enable accepted malformed schema: %#v", result)
+			}
+			for _, event := range record.events {
+				if strings.HasPrefix(event, "write ") || strings.Contains(event, "restart") || strings.Contains(event, "daemon-reload") {
+					t.Fatalf("malformed schema caused mutation: %#v", record.events)
+				}
+			}
+			got, err := os.ReadFile(daemonPath)
+			if err != nil || string(got) != document {
+				t.Fatalf("daemon config changed: %q, %v", got, err)
+			}
+		})
+	}
+}
+
 func TestDockerDaemonRunsReusableTargetContract(t *testing.T) {
 	targettest.Run(t, func(t *testing.T) targettest.Fixture {
 		a, _, daemonPath, _, _ := fixture(t)

internal/gnome/gnome.go

@@ -535,7 +535,7 @@ func (a *Adapter) readSettings(ctx context.Context) (settings, error) {
 	if result.HTTPAuthUser, err = a.getString(ctx, httpSchema, "authentication-user"); err != nil {
 		return settings{}, err
 	}
-	if result.HTTPAuthPassword, err = a.getString(ctx, httpSchema, "authentication-password"); err != nil {
+	if result.HTTPAuthPassword, err = a.getSensitiveString(ctx, httpSchema, "authentication-password"); err != nil {
 		return settings{}, err
 	}
 	if result.HTTPSHost, err = a.getString(ctx, httpsSchema, "host"); err != nil {
@@ -560,6 +560,22 @@ func (a *Adapter) getString(ctx context.Context, schema, key string) (string, er
 	}
 	return parseString(raw)
 }
+
+// getSensitiveString deliberately suppresses both command error detail and raw
+// output. A password value is secret even when malformed, and command runners
+// are not trusted to keep that value out of their errors.
+func (a *Adapter) getSensitiveString(ctx context.Context, schema, key string) (string, error) {
+	raw, err := a.run(ctx, "get", schema, key)
+	if err != nil {
+		return "", safeRedaction(fmt.Sprintf("get %s %s failed; sensitive output suppressed", schema, key), err)
+	}
+	value, err := parseString(strings.TrimSpace(raw))
+	if err != nil {
+		return "", errors.New("parse " + schema + " " + key + " failed; sensitive output suppressed")
+	}
+	return value, nil
+}
+
 func (a *Adapter) getInt(ctx context.Context, schema, key string) (int, error) {
 	raw, err := a.get(ctx, schema, key)
 	if err != nil {

internal/gnome/gnome_test.go

@@ -108,6 +108,49 @@ func active() *profile.Profile {
 	return &profile.Profile{Name: "office", Mode: profile.AdvancedMode, HTTPURL: "http://proxy.example:8080", HTTPSURL: "http://secure.example:8443", AllURL: "socks5://socks.example:1080", Bypass: []string{"localhost", ".example.com", "10.0.0.0/8", "2001:db8::1"}}
 }
 
+func TestDiagnosticsSuppressSensitiveGSettingsOutputAndRunnerErrors(t *testing.T) {
+	const secret = "GNOME-DIAGNOSTIC-SECRET-7f3a"
+	for _, test := range []struct {
+		name  string
+		value string
+		fail  bool
+	}{
+		{name: "malformed value", value: "unterminated-" + secret},
+		{name: "runner error", value: secret, fail: true},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			r := newRunner()
+			key := "org.gnome.system.proxy.http authentication-password"
+			r.values[key] = test.value
+			if test.fail {
+				r.fail = func(c gnome.Command) error {
+					if reflect.DeepEqual(c.Args, []string{"get", "org.gnome.system.proxy.http", "authentication-password"}) {
+						return errors.New("runner exposed " + secret)
+					}
+					return nil
+				}
+			}
+			a := adapter(r, &memoryState{})
+			checks := a.DiagnosticChecks(context.Background(), active())
+			for _, check := range checks {
+				if strings.Contains(check.Evidence, secret) {
+					t.Fatalf("diagnostic evidence exposed password: %#v", checks)
+				}
+			}
+			result := a.SetDesired(context.Background(), true, active())
+			operationError := ""
+			if result.Err != nil {
+				operationError = result.Err.Error()
+			}
+			for _, evidence := range []string{a.Availability(context.Background()).Evidence, a.Inspect(context.Background(), active()).Evidence, operationError} {
+				if strings.Contains(evidence, secret) {
+					t.Fatalf("Target evidence or loggable operation error exposed password: %q", evidence)
+				}
+			}
+		})
+	}
+}
+
 func TestDiagnosticChecksCoverCommandSchemasSessionAndAuthorityWithoutMutation(t *testing.T) {
 	r, state := newRunner(), &memoryState{}
 	a := adapter(r, state)

internal/target/diagnostic.go

@@ -3,7 +3,6 @@ package target
 import (
 	"errors"
 	"io/fs"
-	"os"
 	"path/filepath"
 	"strings"
 
@@ -26,26 +25,66 @@ func ReadOnlyWriteAuthority(paths ...string) Check {
 	return check
 }
 
+// readOnlyWritablePath anchors traversal at an open root descriptor and refuses
+// symbolic links in every component. The authority observation therefore
+// cannot be redirected to a victim between path lookup steps, and it never
+// opens victim content or mutates filesystem metadata.
 func readOnlyWritablePath(path string) (bool, string) {
-	candidate := filepath.Clean(path)
-	for {
-		info, err := os.Stat(candidate)
-		if err == nil {
-			if !info.IsDir() {
-				candidate = filepath.Dir(candidate)
-			}
-			if err := unix.Access(candidate, unix.W_OK); err != nil {
-				return false, candidate + " is not writable: " + err.Error()
-			}
-			return true, candidate + " is writable"
-		}
-		if !errors.Is(err, fs.ErrNotExist) {
-			return false, candidate + " cannot be inspected: " + err.Error()
-		}
-		parent := filepath.Dir(candidate)
-		if parent == candidate {
-			return false, path + " has no writable existing ancestor"
-		}
-		candidate = parent
+	absolute, err := filepath.Abs(path)
+	if err != nil {
+		return false, path + " cannot be inspected: " + err.Error()
 	}
+	absolute = filepath.Clean(absolute)
+	components := strings.Split(strings.TrimPrefix(absolute, string(filepath.Separator)), string(filepath.Separator))
+	if len(components) == 1 && components[0] == "" {
+		components = nil
+	}
+
+	current, err := unix.Open(string(filepath.Separator), unix.O_PATH|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
+	if err != nil {
+		return false, absolute + " cannot be inspected: " + err.Error()
+	}
+	defer func() { _ = unix.Close(current) }()
+	currentPath := string(filepath.Separator)
+
+	for index, component := range components {
+		child, openErr := unix.Openat(current, component, unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
+		if errors.Is(openErr, fs.ErrNotExist) {
+			return writableDirectory(current, currentPath, absolute)
+		}
+		if openErr != nil {
+			return false, filepath.Join(currentPath, component) + " cannot be inspected without following links: " + openErr.Error()
+		}
+		var stat unix.Stat_t
+		statErr := unix.Fstat(child, &stat)
+		if statErr != nil {
+			_ = unix.Close(child)
+			return false, filepath.Join(currentPath, component) + " cannot be inspected: " + statErr.Error()
+		}
+		kind := stat.Mode & unix.S_IFMT
+		componentPath := filepath.Join(currentPath, component)
+		if kind == unix.S_IFLNK {
+			_ = unix.Close(child)
+			return false, componentPath + " is a symbolic link and is unavailable for safe write observation"
+		}
+		if index == len(components)-1 {
+			_ = unix.Close(child)
+			return writableDirectory(current, currentPath, absolute)
+		}
+		if kind != unix.S_IFDIR {
+			_ = unix.Close(child)
+			return false, componentPath + " is not a directory"
+		}
+		_ = unix.Close(current)
+		current = child
+		currentPath = componentPath
+	}
+	return writableDirectory(current, currentPath, absolute)
+}
+
+func writableDirectory(descriptor int, directory, targetPath string) (bool, string) {
+	if err := unix.Faccessat(descriptor, ".", unix.W_OK|unix.X_OK, unix.AT_EACCESS); err != nil {
+		return false, directory + " is not writable for " + targetPath + ": " + err.Error()
+	}
+	return true, directory + " is writable for " + targetPath
 }

internal/target/diagnostic_test.go

@@ -0,0 +1,74 @@
+package target_test
+
+import (
+	"os"
+	"path/filepath"
+	"reflect"
+	"strings"
+	"testing"
+
+	"proxyctl/internal/target"
+)
+
+func TestReadOnlyWriteAuthorityRejectsSymlinkedFinalAndAncestorWithoutTouchingVictim(t *testing.T) {
+	for _, test := range []struct {
+		name string
+		path func(root, victim string) string
+	}{
+		{
+			name: "final path",
+			path: func(root, victim string) string {
+				link := filepath.Join(root, "managed.json")
+				if err := os.Symlink(victim, link); err != nil {
+					t.Fatal(err)
+				}
+				return link
+			},
+		},
+		{
+			name: "ancestor path",
+			path: func(root, victim string) string {
+				victimDir := filepath.Dir(victim)
+				link := filepath.Join(root, "managed")
+				if err := os.Symlink(victimDir, link); err != nil {
+					t.Fatal(err)
+				}
+				return filepath.Join(link, filepath.Base(victim))
+			},
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			root := t.TempDir()
+			victimDir := t.TempDir()
+			victim := filepath.Join(victimDir, "victim.json")
+			const content = "VICTIM-CONTENT-MUST-STAY"
+			if err := os.WriteFile(victim, []byte(content), 0o640); err != nil {
+				t.Fatal(err)
+			}
+			beforeContent, err := os.ReadFile(victim)
+			if err != nil {
+				t.Fatal(err)
+			}
+			before, err := os.Stat(victim)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			check := target.ReadOnlyWriteAuthority(test.path(root, victim))
+			if check.OK || (!strings.Contains(check.Evidence, "symbolic link") && !strings.Contains(check.Evidence, "symlink")) {
+				t.Fatalf("symlink path reported writable: %#v", check)
+			}
+			after, err := os.Stat(victim)
+			if err != nil {
+				t.Fatal(err)
+			}
+			data, err := os.ReadFile(victim)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if string(beforeContent) != content || string(data) != content || !reflect.DeepEqual(before.Sys(), after.Sys()) {
+				t.Fatalf("victim changed: before=%#v after=%#v content=%q", before, after, data)
+			}
+		})
+	}
+}