diff --git a/cmd/proxyctl/main.go b/cmd/proxyctl/main.go index 3e17e3c..9f1b517 100644 --- a/cmd/proxyctl/main.go +++ b/cmd/proxyctl/main.go @@ -27,7 +27,7 @@ func run(args []string, input io.Reader, output io.Writer) error { if err != nil { return err } - application, err := app.New(profile.NewOwnedFileStore(configuration.Path, configuration.Owner)) + application, err := app.New(profile.NewOwnedFileStoreAt(configuration.Path, configuration.Owner, configuration.TrustedRoot)) if err != nil { return err } diff --git a/internal/app/storage_test.go b/internal/app/storage_test.go index 6857d45..246102d 100644 --- a/internal/app/storage_test.go +++ b/internal/app/storage_test.go @@ -168,6 +168,9 @@ func TestLoadSecuresExistingSecretBearingConfiguration(t *testing.T) { t.Fatalf("load existing configuration: %v", err) } assertMode(t, path, 0o600) + if _, err := os.Lstat(path + ".lock"); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("view-only Load unexpectedly created a lock: %v", err) + } } func assertMode(t *testing.T, path string, want os.FileMode) { diff --git a/internal/profile/store.go b/internal/profile/store.go index 0db3f3d..578624e 100644 --- a/internal/profile/store.go +++ b/internal/profile/store.go @@ -18,9 +18,10 @@ import ( ) type FileStore struct { - path string - owner *UserIdentity - operations fileOperations + path string + owner *UserIdentity + trustedRoot string + operations fileOperations } type fileOperations interface { @@ -43,11 +44,19 @@ func NewFileStore(path string) *FileStore { } func NewOwnedFileStore(path string, owner UserIdentity) *FileStore { - return &FileStore{path: path, owner: &owner, operations: systemFileOperations{}} + return NewOwnedFileStoreAt(path, owner, owner.HomeDir) +} + +func NewOwnedFileStoreAt(path string, owner UserIdentity, trustedRoot string) *FileStore { + return &FileStore{path: path, owner: &owner, trustedRoot: trustedRoot, operations: systemFileOperations{}} } func newFileStore(path string, owner *UserIdentity, operations fileOperations) *FileStore { - return &FileStore{path: path, owner: owner, operations: operations} + trustedRoot := "" + if owner != nil { + trustedRoot = owner.HomeDir + } + return &FileStore{path: path, owner: owner, trustedRoot: trustedRoot, operations: operations} } type UserIdentity struct { @@ -58,8 +67,9 @@ type UserIdentity struct { } type Configuration struct { - Path string - Owner UserIdentity + Path string + Owner UserIdentity + TrustedRoot string } type IdentityEnvironment interface { @@ -132,17 +142,22 @@ func ResolveConfiguration(environment IdentityEnvironment) (Configuration, error } configDir := "" + trustedRoot := owner.HomeDir if value, ok := environment.LookupEnv("XDG_CONFIG_HOME"); ok && filepath.IsAbs(value) { if !invokingUser || pathWithin(value, owner.HomeDir) { configDir = value + if !pathWithin(value, owner.HomeDir) { + trustedRoot = value + } } } if configDir == "" { configDir = filepath.Join(owner.HomeDir, ".config") } return Configuration{ - Path: filepath.Join(configDir, "proxyctl", "profiles.json"), - Owner: owner, + Path: filepath.Join(configDir, "proxyctl", "profiles.json"), + Owner: owner, + TrustedRoot: trustedRoot, }, nil } @@ -171,7 +186,7 @@ func (s *FileStore) Load() (State, error) { } func (s *FileStore) Mutate(change func(State) (State, error)) (State, error) { - directory, err := s.secureDirectory() + directory, err := s.openConfigurationDirectory(true) if err != nil { return State{}, err } @@ -221,14 +236,17 @@ func openAt(directory *os.File, name string, flags int, mode uint32) (*os.File, } func (s *FileStore) load() (State, error) { - directory, err := openNoFollow(filepath.Dir(s.path), unix.O_RDONLY|unix.O_DIRECTORY, 0) + directory, err := s.openConfigurationDirectory(false) if errors.Is(err, fs.ErrNotExist) { return State{}, nil } if err != nil { - return State{}, fmt.Errorf("open Profile configuration directory without following links: %w", err) + return State{}, err } defer directory.Close() + if err := s.secureExistingLock(directory); err != nil { + return State{}, err + } return s.loadFrom(directory) } @@ -276,26 +294,111 @@ func (s *FileStore) secureRegularFile(file *os.File, description string) error { return nil } -func (s *FileStore) secureDirectory() (*os.File, error) { - directoryPath := filepath.Dir(s.path) - if err := os.MkdirAll(directoryPath, 0o700); err != nil { - return nil, fmt.Errorf("create Profile configuration directory: %w", err) +func (s *FileStore) openConfigurationDirectory(create bool) (*os.File, error) { + directoryPath := filepath.Clean(filepath.Dir(s.path)) + if !filepath.IsAbs(directoryPath) { + return nil, errors.New("Profile configuration path must be absolute") } - directory, err := openNoFollow(directoryPath, unix.O_RDONLY|unix.O_DIRECTORY, 0) + if directoryPath == string(filepath.Separator) { + return nil, errors.New("Profile configuration directory cannot be the filesystem root") + } + creationRoot := directoryPath + if s.owner != nil { + creationRoot = filepath.Clean(s.trustedRoot) + if !filepath.IsAbs(creationRoot) || !pathWithin(directoryPath, creationRoot) || directoryPath == creationRoot { + return nil, errors.New("Profile configuration directory escapes its trusted root") + } + } + + current, err := openNoFollow(string(filepath.Separator), unix.O_RDONLY|unix.O_DIRECTORY, 0) if err != nil { - return nil, fmt.Errorf("open Profile configuration directory without following links: %w", err) + return nil, fmt.Errorf("open filesystem root: %w", err) } + currentPath := string(filepath.Separator) + for _, component := range strings.Split(strings.TrimPrefix(directoryPath, string(filepath.Separator)), string(filepath.Separator)) { + if component == "" { + continue + } + nextPath := filepath.Join(currentPath, component) + next, openErr := openAt(current, component, unix.O_RDONLY|unix.O_DIRECTORY, 0) + if errors.Is(openErr, fs.ErrNotExist) && create && s.mayCreateDirectory(nextPath, directoryPath, creationRoot) { + created := false + if mkdirErr := unix.Mkdirat(int(current.Fd()), component, 0o700); mkdirErr != nil { + if !errors.Is(mkdirErr, fs.ErrExist) { + current.Close() + return nil, fmt.Errorf("create Profile configuration directory component %q: %w", nextPath, mkdirErr) + } + } else { + created = true + } + next, openErr = openAt(current, component, unix.O_RDONLY|unix.O_DIRECTORY, 0) + if openErr == nil && created { + if err := s.secureCreatedDirectory(next, nextPath); err != nil { + next.Close() + _ = unix.Unlinkat(int(current.Fd()), component, unix.AT_REMOVEDIR) + current.Close() + return nil, err + } + } + } + current.Close() + if openErr != nil { + if errors.Is(openErr, fs.ErrNotExist) { + return nil, openErr + } + return nil, fmt.Errorf("open Profile configuration ancestor %q without following links: %w", nextPath, openErr) + } + current = next + currentPath = nextPath + } + if err := s.secureConfigurationDirectory(current); err != nil { + current.Close() + return nil, err + } + return current, nil +} + +func (s *FileStore) mayCreateDirectory(path, directoryPath, creationRoot string) bool { + if s.owner == nil { + return path == directoryPath + } + return path != creationRoot && pathWithin(path, creationRoot) +} + +func (s *FileStore) secureCreatedDirectory(directory *os.File, path string) error { if s.owner != nil { if err := s.operations.SetOwner(directory, s.owner.UID, s.owner.GID); err != nil { - directory.Close() - return nil, fmt.Errorf("set Profile configuration directory owner: %w", err) + return fmt.Errorf("set created Profile configuration directory %q owner: %w", path, err) } } if err := directory.Chmod(0o700); err != nil { - directory.Close() - return nil, fmt.Errorf("secure Profile configuration directory: %w", err) + return fmt.Errorf("secure created Profile configuration directory %q: %w", path, err) } - return directory, nil + return nil +} + +func (s *FileStore) secureConfigurationDirectory(directory *os.File) error { + if s.owner != nil { + if err := s.operations.SetOwner(directory, s.owner.UID, s.owner.GID); err != nil { + return fmt.Errorf("set Profile configuration directory owner: %w", err) + } + } + if err := directory.Chmod(0o700); err != nil { + return fmt.Errorf("secure Profile configuration directory: %w", err) + } + return nil +} + +func (s *FileStore) secureExistingLock(directory *os.File) error { + lock, err := openAt(directory, filepath.Base(s.path)+".lock", unix.O_RDWR, 0) + if errors.Is(err, fs.ErrNotExist) { + return nil + } + if err != nil { + return fmt.Errorf("open existing Profile configuration lock without following links: %w", err) + } + defer lock.Close() + return s.secureRegularFile(lock, "Profile configuration lock") } func (s *FileStore) openLockAt(directory *os.File) (*os.File, error) { diff --git a/internal/profile/store_test.go b/internal/profile/store_test.go index 81fefad..0335c71 100644 --- a/internal/profile/store_test.go +++ b/internal/profile/store_test.go @@ -111,7 +111,7 @@ func TestFileStoreLockSerializesAcrossProcesses(t *testing.T) { func TestOwnedStoreAssignsInvokingUserToDirectoryLockAndReplacement(t *testing.T) { path := filepath.Join(t.TempDir(), "proxyctl", "profiles.json") - owner := UserIdentity{Username: "alice", UID: 1001, GID: 1002} + owner := UserIdentity{Username: "alice", HomeDir: filepath.Dir(filepath.Dir(path)), UID: 1001, GID: 1002} operations := &fakeFileOperations{} store := newFileStore(path, &owner, operations) if _, err := store.Mutate(func(state State) (State, error) { @@ -146,7 +146,7 @@ func TestOwnedStoreReportsOwnershipFailuresBeforeCommit(t *testing.T) { for _, failure := range []string{"proxyctl", "profiles.json.lock", ".profiles-"} { t.Run(failure, func(t *testing.T) { path := filepath.Join(t.TempDir(), "proxyctl", "profiles.json") - owner := UserIdentity{Username: "alice", UID: 1001, GID: 1002} + owner := UserIdentity{Username: "alice", HomeDir: filepath.Dir(filepath.Dir(path)), UID: 1001, GID: 1002} operations := &fakeFileOperations{failContains: failure} store := newFileStore(path, &owner, operations) _, err := store.Mutate(func(state State) (State, error) { @@ -171,7 +171,7 @@ func TestOwnedLoadRejectsConfigurationOwnershipFailure(t *testing.T) { if err := os.WriteFile(path, []byte(`{"profiles":[]}`), 0o600); err != nil { t.Fatal(err) } - owner := UserIdentity{Username: "alice", UID: 1001, GID: 1002} + owner := UserIdentity{Username: "alice", HomeDir: filepath.Dir(filepath.Dir(path)), UID: 1001, GID: 1002} operations := &fakeFileOperations{failContains: "profiles.json"} _, err := newFileStore(path, &owner, operations).Load() if err == nil || !strings.Contains(err.Error(), "set Profile configuration owner") { @@ -202,7 +202,7 @@ func TestLoadRejectsProfileConfigurationSymlinkWithoutChangingTarget(t *testing. } } -func TestMutationRejectsLockSymlinkWithoutChangingTarget(t *testing.T) { +func TestLoadRejectsLockSymlinkWithoutChangingTarget(t *testing.T) { directory := filepath.Join(t.TempDir(), "proxyctl") if err := os.MkdirAll(directory, 0o700); err != nil { t.Fatal(err) @@ -215,7 +215,7 @@ func TestMutationRejectsLockSymlinkWithoutChangingTarget(t *testing.T) { if err := os.Symlink(target, path+".lock"); err != nil { t.Fatal(err) } - _, err := NewFileStore(path).Mutate(func(state State) (State, error) { return state, nil }) + _, err := NewFileStore(path).Load() if err == nil || !strings.Contains(err.Error(), "without following links") { t.Fatalf("lock symlink error = %v", err) } @@ -235,6 +235,136 @@ func TestMutationRejectsLockSymlinkWithoutChangingTarget(t *testing.T) { } } +func TestLoadRepairsExistingDirectoryConfigurationAndLockOwnership(t *testing.T) { + home := t.TempDir() + directory := filepath.Join(home, ".config", "proxyctl") + if err := os.MkdirAll(directory, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Chmod(directory, 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(directory, "profiles.json") + if err := os.WriteFile(path, []byte(`{"profiles":[]}`), 0o644); err != nil { + t.Fatal(err) + } + lockPath := path + ".lock" + if err := os.WriteFile(lockPath, nil, 0o644); err != nil { + t.Fatal(err) + } + owner := UserIdentity{Username: "alice", HomeDir: home, UID: 1001, GID: 1002} + operations := &fakeFileOperations{} + _, err := newFileStore(path, &owner, operations).Load() + if err != nil { + t.Fatal(err) + } + + for target, mode := range map[string]os.FileMode{directory: 0o700, path: 0o600, lockPath: 0o600} { + info, statErr := os.Stat(target) + if statErr != nil { + t.Fatal(statErr) + } + if info.Mode().Perm() != mode { + t.Errorf("mode for %s = %#o, want %#o", target, info.Mode().Perm(), mode) + } + } + for _, target := range []string{directory, path, lockPath} { + found := false + for _, call := range operations.calls { + if call.path == target && call.uid == 1001 && call.gid == 1002 { + found = true + } + } + if !found { + t.Errorf("no invoking-user ownership repair for %s; calls=%#v", target, operations.calls) + } + } +} + +func TestLoadRejectsConfigAncestorSymlinkWithoutChangingVictim(t *testing.T) { + home := t.TempDir() + victim := t.TempDir() + victimFile := filepath.Join(victim, "unchanged") + if err := os.WriteFile(victimFile, []byte("victim"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(victim, filepath.Join(home, ".config")); err != nil { + t.Fatal(err) + } + path := filepath.Join(home, ".config", "proxyctl", "profiles.json") + owner := UserIdentity{Username: "alice", HomeDir: home, UID: 1001, GID: 1002} + _, err := newFileStore(path, &owner, &fakeFileOperations{}).Load() + if err == nil || !strings.Contains(err.Error(), "without following links") { + t.Fatalf("ancestor symlink error = %v", err) + } + contents, readErr := os.ReadFile(victimFile) + if readErr != nil || string(contents) != "victim" { + t.Fatalf("victim changed: contents=%q error=%v", contents, readErr) + } + if _, statErr := os.Stat(filepath.Join(victim, "proxyctl")); !errors.Is(statErr, os.ErrNotExist) { + t.Fatalf("created through ancestor symlink: %v", statErr) + } +} + +func TestMutationRejectsIntermediateXDGAncestorSymlink(t *testing.T) { + home := t.TempDir() + xdg := filepath.Join(home, "xdg") + if err := os.Mkdir(xdg, 0o700); err != nil { + t.Fatal(err) + } + victim := t.TempDir() + if err := os.Symlink(victim, filepath.Join(xdg, "intermediate")); err != nil { + t.Fatal(err) + } + path := filepath.Join(xdg, "intermediate", "proxyctl", "profiles.json") + owner := UserIdentity{Username: "alice", HomeDir: home, UID: 1001, GID: 1002} + store := &FileStore{path: path, owner: &owner, trustedRoot: xdg, operations: &fakeFileOperations{}} + _, err := store.Mutate(func(state State) (State, error) { return state, nil }) + if err == nil || !strings.Contains(err.Error(), "without following links") { + t.Fatalf("intermediate symlink error = %v", err) + } + entries, readErr := os.ReadDir(victim) + if readErr != nil { + t.Fatal(readErr) + } + if len(entries) != 0 { + t.Fatalf("victim directory changed through symlink: %#v", entries) + } +} + +func TestOwnedMutationCreatesMissingDirectoriesAnchoredWithOwnerAndMode(t *testing.T) { + home := t.TempDir() + path := filepath.Join(home, ".config", "nested", "proxyctl", "profiles.json") + owner := UserIdentity{Username: "alice", HomeDir: home, UID: 1001, GID: 1002} + operations := &fakeFileOperations{} + store := newFileStore(path, &owner, operations) + if _, err := store.Mutate(func(state State) (State, error) { return state, nil }); err != nil { + t.Fatal(err) + } + for _, directory := range []string{ + filepath.Join(home, ".config"), + filepath.Join(home, ".config", "nested"), + filepath.Join(home, ".config", "nested", "proxyctl"), + } { + info, err := os.Stat(directory) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm() != 0o700 { + t.Errorf("created directory %s mode = %#o, want 0700", directory, info.Mode().Perm()) + } + found := false + for _, call := range operations.calls { + if call.path == directory && call.uid == 1001 && call.gid == 1002 { + found = true + } + } + if !found { + t.Errorf("created directory %s was not assigned invoking-user ownership", directory) + } + } +} + func TestDirectorySyncFailureReturnsCommittedStateAndRecognizableError(t *testing.T) { path := filepath.Join(t.TempDir(), "proxyctl", "profiles.json") operations := &fakeFileOperations{syncErr: errors.New("injected sync failure")}