diff --git a/cmd/proxyctl/maintenance_test.go b/cmd/proxyctl/maintenance_test.go index 9ccf39a..020abdc 100644 --- a/cmd/proxyctl/maintenance_test.go +++ b/cmd/proxyctl/maintenance_test.go @@ -105,6 +105,36 @@ func TestDoctorReturnsFailureWhenAnyCheckFails(t *testing.T) { } } +func TestDoctorProfileValidityRejectsSymlinkWithoutValidatingTarget(t *testing.T) { + victim := filepath.Join(t.TempDir(), "victim.json") + const content = "target content that is deliberately not JSON" + if err := os.WriteFile(victim, []byte(content), 0o640); err != nil { + t.Fatal(err) + } + directory := t.TempDir() + path := filepath.Join(directory, "profiles.json") + if err := os.Symlink(victim, path); err != nil { + t.Fatal(err) + } + + var output bytes.Buffer + err := runMaintenanceCommand([]string{"doctor"}, strings.NewReader(""), &output, profile.NewFileStore(path), nil, nil, nil, nil) + text := output.String() + if err == nil || !strings.Contains(text, "[FAIL] Profile validity") { + t.Fatalf("doctor symlink result err=%v output=%q", err, text) + } + if strings.Contains(text, "parse Profile configuration") || strings.Contains(text, content) { + t.Fatalf("doctor read or validated symlink target: %q", text) + } + got, readErr := os.ReadFile(victim) + if readErr != nil { + t.Fatal(readErr) + } + if string(got) != content { + t.Fatalf("doctor changed victim content to %q", got) + } +} + func TestDoctorStorageAuthorityRejectsSymlinkedPathsWithoutTouchingVictim(t *testing.T) { for _, test := range []struct { name string diff --git a/internal/profile/diagnostic.go b/internal/profile/diagnostic.go index b49e51a..db2b9de 100644 --- a/internal/profile/diagnostic.go +++ b/internal/profile/diagnostic.go @@ -4,34 +4,18 @@ import ( "encoding/json" "errors" "fmt" - "io" "io/fs" - "os" - - "golang.org/x/sys/unix" ) // ReadOnlyLoad reads and validates Profile configuration without creating, // chmodding, chowning, locking, or replacing any filesystem object. func (s *FileStore) ReadOnlyLoad() (State, error) { - file, err := os.OpenFile(s.path, unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_NONBLOCK, 0) + data, err := s.secureStore().ReadFile(s.path) if errors.Is(err, fs.ErrNotExist) { return State{}, nil } if err != nil { - return State{}, fmt.Errorf("open Profile configuration read-only: %w", err) - } - defer file.Close() - info, err := file.Stat() - if err != nil { - return State{}, fmt.Errorf("inspect Profile configuration: %w", err) - } - if !info.Mode().IsRegular() { - return State{}, errors.New("Profile configuration is not a regular file") - } - data, err := io.ReadAll(file) - if err != nil { - return State{}, fmt.Errorf("read Profile configuration: %w", err) + return State{}, fmt.Errorf("open Profile configuration read-only without following links: %w", err) } var state State if err := json.Unmarshal(data, &state); err != nil { diff --git a/internal/profile/diagnostic_test.go b/internal/profile/diagnostic_test.go index 6aae118..7503584 100644 --- a/internal/profile/diagnostic_test.go +++ b/internal/profile/diagnostic_test.go @@ -3,6 +3,8 @@ package profile_test import ( "os" "path/filepath" + "strings" + "syscall" "testing" "proxyctl/internal/profile" @@ -33,3 +35,125 @@ func TestReadOnlyLoadValidatesWithoutRepairingOrCreatingStorage(t *testing.T) { t.Fatalf("read-only load created lock: %v", err) } } + +func TestReadOnlyLoadPreservesMissingAndMalformedBehavior(t *testing.T) { + t.Run("missing", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "missing", "profiles.json") + state, err := profile.NewFileStore(path).ReadOnlyLoad() + if err != nil || len(state.Profiles) != 0 || state.ActiveProfile != "" { + t.Fatalf("missing read-only load state=%#v err=%v", state, err) + } + if _, statErr := os.Stat(filepath.Dir(path)); !os.IsNotExist(statErr) { + t.Fatalf("missing read-only load created storage: %v", statErr) + } + }) + + t.Run("malformed", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "profiles.json") + if err := os.WriteFile(path, []byte("not JSON"), 0o640); err != nil { + t.Fatal(err) + } + _, err := profile.NewFileStore(path).ReadOnlyLoad() + if err == nil || !strings.Contains(err.Error(), "parse Profile configuration") { + t.Fatalf("malformed read-only load error = %v", err) + } + }) +} + +func TestReadOnlyLoadUsesConfiguredTrustedRoot(t *testing.T) { + trustedRoot := t.TempDir() + outside := filepath.Join(t.TempDir(), "profiles.json") + if err := os.WriteFile(outside, []byte(`{"profiles":[]}`), 0o600); err != nil { + t.Fatal(err) + } + owner := profile.UserIdentity{HomeDir: trustedRoot, UID: os.Getuid(), GID: os.Getgid()} + _, err := profile.NewOwnedFileStoreAt(outside, owner, trustedRoot).ReadOnlyLoad() + if err == nil || !strings.Contains(err.Error(), "escapes its trusted root") { + t.Fatalf("configured trusted root was not enforced: %v", err) + } +} + +func TestReadOnlyLoadRejectsSymlinksWithoutReadingOrChangingVictim(t *testing.T) { + for _, test := range []struct { + name string + path func(t *testing.T, root, victim string) string + }{ + { + name: "final symlink", + path: func(t *testing.T, root, victim string) string { + t.Helper() + directory := filepath.Join(root, "proxyctl") + if err := os.MkdirAll(directory, 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(directory, "profiles.json") + if err := os.Symlink(victim, path); err != nil { + t.Fatal(err) + } + return path + }, + }, + { + name: "ancestor symlink", + path: func(t *testing.T, root, victim string) string { + t.Helper() + link := filepath.Join(root, "proxyctl") + if err := os.Symlink(filepath.Dir(victim), link); err != nil { + t.Fatal(err) + } + return filepath.Join(link, filepath.Base(victim)) + }, + }, + } { + t.Run(test.name, func(t *testing.T) { + victim := filepath.Join(t.TempDir(), "profiles.json") + const content = "victim content that is deliberately not JSON" + if err := os.WriteFile(victim, []byte(content), 0o640); err != nil { + t.Fatal(err) + } + before := fileIdentity(t, victim) + path := test.path(t, t.TempDir(), victim) + + _, err := profile.NewFileStore(path).ReadOnlyLoad() + if err == nil { + t.Fatal("ReadOnlyLoad accepted a symlinked Profile configuration") + } + if strings.Contains(err.Error(), "parse Profile configuration") { + t.Fatalf("ReadOnlyLoad followed and parsed the victim: %v", err) + } + got, readErr := os.ReadFile(victim) + if readErr != nil { + t.Fatal(readErr) + } + if string(got) != content { + t.Fatalf("victim content changed to %q", got) + } + after := fileIdentity(t, victim) + if before != after { + t.Fatalf("victim metadata changed: before=%+v after=%+v", before, after) + } + if _, statErr := os.Lstat(path + ".lock"); !os.IsNotExist(statErr) { + t.Fatalf("ReadOnlyLoad created a lock: %v", statErr) + } + }) + } +} + +type diagnosticFileIdentity struct { + mode os.FileMode + uid uint32 + gid uint32 +} + +func fileIdentity(t *testing.T, path string) diagnosticFileIdentity { + t.Helper() + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + t.Fatalf("stat data has type %T", info.Sys()) + } + return diagnosticFileIdentity{mode: info.Mode(), uid: stat.Uid, gid: stat.Gid} +} diff --git a/internal/profile/store.go b/internal/profile/store.go index a7035e8..e0245bf 100644 --- a/internal/profile/store.go +++ b/internal/profile/store.go @@ -40,7 +40,7 @@ func (systemFileOperations) SyncDirectory(directory *os.File) error { } func NewFileStore(path string) *FileStore { - return &FileStore{path: path, operations: systemFileOperations{}} + return &FileStore{path: path, trustedRoot: securefs.ExistingTrustedRoot(path), operations: systemFileOperations{}} } func NewOwnedFileStore(path string, owner UserIdentity) *FileStore { @@ -52,13 +52,21 @@ func NewOwnedFileStoreAt(path string, owner UserIdentity, trustedRoot string) *F } func newFileStore(path string, owner *UserIdentity, operations fileOperations) *FileStore { - trustedRoot := "" + trustedRoot := securefs.ExistingTrustedRoot(path) if owner != nil { trustedRoot = owner.HomeDir } return &FileStore{path: path, owner: owner, trustedRoot: trustedRoot, operations: operations} } +func (s *FileStore) secureStore() securefs.Store { + store := securefs.Store{TrustedRoot: s.trustedRoot} + if s.owner != nil { + store.Owner = &securefs.Owner{UID: s.owner.UID, GID: s.owner.GID} + } + return store +} + type UserIdentity struct { Username string HomeDir string