diff --git a/internal/dockerclient/dockerclient.go b/internal/dockerclient/dockerclient.go index 43409bc..2e4a997 100644 --- a/internal/dockerclient/dockerclient.go +++ b/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 diff --git a/internal/dockerclient/dockerclient_test.go b/internal/dockerclient/dockerclient_test.go index b9f1f06..2268713 100644 --- a/internal/dockerclient/dockerclient_test.go +++ b/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) diff --git a/internal/dockerdaemon/dockerdaemon.go b/internal/dockerdaemon/dockerdaemon.go index ddd9496..c5471a4 100644 --- a/internal/dockerdaemon/dockerdaemon.go +++ b/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 { diff --git a/internal/dockerdaemon/dockerdaemon_test.go b/internal/dockerdaemon/dockerdaemon_test.go index 4f5d631..94af2ba 100644 --- a/internal/dockerdaemon/dockerdaemon_test.go +++ b/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) diff --git a/internal/gnome/gnome.go b/internal/gnome/gnome.go index 085c424..297faa8 100644 --- a/internal/gnome/gnome.go +++ b/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 { diff --git a/internal/gnome/gnome_test.go b/internal/gnome/gnome_test.go index 526b2e7..8638e2f 100644 --- a/internal/gnome/gnome_test.go +++ b/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) diff --git a/internal/target/diagnostic.go b/internal/target/diagnostic.go index 9c39b06..4f8e622 100644 --- a/internal/target/diagnostic.go +++ b/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 } diff --git a/internal/target/diagnostic_test.go b/internal/target/diagnostic_test.go new file mode 100644 index 0000000..4a93cd5 --- /dev/null +++ b/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) + } + }) + } +}