fix: reject profile diagnostic symlink ancestors

2026-07-17 11:41:55 +08:00
parent 6cb4943a54
commit e9ff24e04a
4 changed files with 166 additions and 20 deletions
@@ -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
@@ -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 {
@@ -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}
+}
@@ -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