fix: anchor profile directory traversal

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
 	}

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) {

internal/profile/store.go

@@ -18,9 +18,10 @@ import (
 )
 
 type FileStore struct {
-	path       string
+	path        string
-	owner      *UserIdentity
+	owner       *UserIdentity
-	operations fileOperations
+	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
+	Path        string
-	Owner UserIdentity
+	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"),
+		Path:        filepath.Join(configDir, "proxyctl", "profiles.json"),
-		Owner: owner,
+		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) {
+func (s *FileStore) openConfigurationDirectory(create bool) (*os.File, error) {
-	directoryPath := filepath.Dir(s.path)
+	directoryPath := filepath.Clean(filepath.Dir(s.path))
-	if err := os.MkdirAll(directoryPath, 0o700); err != nil {
+	if !filepath.IsAbs(directoryPath) {
-		return nil, fmt.Errorf("create Profile configuration directory: %w", err)
+		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 fmt.Errorf("set created Profile configuration directory %q owner: %w", path, err)
-			return nil, fmt.Errorf("set Profile configuration directory owner: %w", err)
 		}
 	}
 	if err := directory.Chmod(0o700); err != nil {
-		directory.Close()
+		return fmt.Errorf("secure created Profile configuration directory %q: %w", path, err)
-		return nil, fmt.Errorf("secure Profile configuration directory: %w", 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) {

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")}