diff --git a/docs/METRICS_HISTORY.md b/docs/METRICS_HISTORY.md index c36d5e5f2..883bf2db5 100644 --- a/docs/METRICS_HISTORY.md +++ b/docs/METRICS_HISTORY.md @@ -56,6 +56,33 @@ only the metrics SQLite database without moving secrets or general config: PULSE_METRICS_DB_PATH=/dev/shm/pulse/metrics.db ``` +Use a dedicated directory owned by the account running Pulse. Do not place the +database directly in a shared parent such as `/tmp/metrics.db`: Pulse secures +the database directory to mode `0700` and will reject a directory owned by +another user. On systemd installs, `PrivateTmp=true` also means a path below +`/tmp` is private to the service and will not appear in the host's `/tmp`. + +The default systemd sandbox permits `/dev/shm/pulse` and Pulse can create that +subdirectory itself. A dedicated mount elsewhere, such as `/mnt/ramdisk`, also +needs an explicit writable-path grant: + +```bash +sudo install -d -o pulse -g pulse -m 0700 /mnt/ramdisk/pulse +sudo systemctl edit pulse +``` + +Add this drop-in (replace `pulse` with the installed unit name if different): + +```ini +[Service] +ReadWritePaths=/mnt/ramdisk/pulse +``` + +Then set `PULSE_METRICS_DB_PATH=/mnt/ramdisk/pulse/metrics.db` in +`/etc/pulse/.env` and restart Pulse. `ProtectSystem=strict` deliberately keeps +paths outside the unit's writable allowlist read-only even when Unix ownership +would otherwise permit writes. + For Docker, mount a tmpfs at the selected directory and keep `/data` on a persistent volume: diff --git a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md index 18d92c672..55b3e2e74 100644 --- a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md +++ b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md @@ -2091,6 +2091,11 @@ store must create or re-harden the selected database directory as owner-only and must reject symlink or non-regular database targets before opening SQLite; the `.db`, `.db-wal`, and `.db-shm` artifacts must be chmodded owner-only even under a permissive process umask. +That hardening must never chmod a filesystem root, sticky shared directory, or +directory owned by another account. Unsafe parents such as direct +`/tmp/metrics.db` placement must fail with dedicated-subdirectory guidance; +external systemd mounts must remain explicit `ReadWritePaths` grants rather +than weakening the default service sandbox. That same metrics hot path must also keep startup maintenance off the constructor critical path. `NewStore` may initialize schema and return a usable store, but restart-time retention cleanup and one-time auto-vacuum migration diff --git a/frontend-modern/public/docs/METRICS_HISTORY.md b/frontend-modern/public/docs/METRICS_HISTORY.md index c36d5e5f2..883bf2db5 100644 --- a/frontend-modern/public/docs/METRICS_HISTORY.md +++ b/frontend-modern/public/docs/METRICS_HISTORY.md @@ -56,6 +56,33 @@ only the metrics SQLite database without moving secrets or general config: PULSE_METRICS_DB_PATH=/dev/shm/pulse/metrics.db ``` +Use a dedicated directory owned by the account running Pulse. Do not place the +database directly in a shared parent such as `/tmp/metrics.db`: Pulse secures +the database directory to mode `0700` and will reject a directory owned by +another user. On systemd installs, `PrivateTmp=true` also means a path below +`/tmp` is private to the service and will not appear in the host's `/tmp`. + +The default systemd sandbox permits `/dev/shm/pulse` and Pulse can create that +subdirectory itself. A dedicated mount elsewhere, such as `/mnt/ramdisk`, also +needs an explicit writable-path grant: + +```bash +sudo install -d -o pulse -g pulse -m 0700 /mnt/ramdisk/pulse +sudo systemctl edit pulse +``` + +Add this drop-in (replace `pulse` with the installed unit name if different): + +```ini +[Service] +ReadWritePaths=/mnt/ramdisk/pulse +``` + +Then set `PULSE_METRICS_DB_PATH=/mnt/ramdisk/pulse/metrics.db` in +`/etc/pulse/.env` and restart Pulse. `ProtectSystem=strict` deliberately keeps +paths outside the unit's writable allowlist read-only even when Unix ownership +would otherwise permit writes. + For Docker, mount a tmpfs at the selected directory and keep `/data` on a persistent volume: diff --git a/pkg/metrics/directory_owner_other.go b/pkg/metrics/directory_owner_other.go new file mode 100644 index 000000000..0ce844aa6 --- /dev/null +++ b/pkg/metrics/directory_owner_other.go @@ -0,0 +1,11 @@ +//go:build !unix + +package metrics + +import "os" + +// Platforms without Unix ownership metadata retain the existing chmod-based +// hardening. Filesystem-root and symlink targets are still rejected first. +func directoryOwnedByCurrentUser(_ os.FileInfo) bool { + return true +} diff --git a/pkg/metrics/directory_owner_unix.go b/pkg/metrics/directory_owner_unix.go new file mode 100644 index 000000000..12c4ca510 --- /dev/null +++ b/pkg/metrics/directory_owner_unix.go @@ -0,0 +1,13 @@ +//go:build unix + +package metrics + +import ( + "os" + "syscall" +) + +func directoryOwnedByCurrentUser(info os.FileInfo) bool { + stat, ok := info.Sys().(*syscall.Stat_t) + return ok && uint64(stat.Uid) == uint64(os.Geteuid()) +} diff --git a/pkg/metrics/store.go b/pkg/metrics/store.go index 57ae21f7e..3265d12b2 100644 --- a/pkg/metrics/store.go +++ b/pkg/metrics/store.go @@ -251,7 +251,7 @@ func NewStore(config StoreConfig) (*Store, error) { dir := filepath.Dir(config.DBPath) if err := ensureOwnerOnlyDir(dir); err != nil { - return nil, fmt.Errorf("failed to create metrics directory: %w", err) + return nil, fmt.Errorf("failed to prepare metrics directory: %w", err) } if err := rejectSymlinkOrNonRegular(config.DBPath); err != nil && !errors.Is(err, os.ErrNotExist) { @@ -2256,10 +2256,41 @@ func (s *Store) Close() error { } func ensureOwnerOnlyDir(dir string) error { - if err := os.MkdirAll(dir, privateDirPerm); err != nil { + cleanedDir := filepath.Clean(dir) + if filepath.IsAbs(cleanedDir) && filepath.Dir(cleanedDir) == cleanedDir { + return fmt.Errorf("metrics database must use a dedicated subdirectory, not filesystem root %q", cleanedDir) + } + + info, err := os.Lstat(cleanedDir) + if errors.Is(err, os.ErrNotExist) { + if err := os.MkdirAll(cleanedDir, privateDirPerm); err != nil { + return err + } + info, err = os.Lstat(cleanedDir) + } + if err != nil { return err } - return os.Chmod(dir, privateDirPerm) + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("unsafe metrics directory %q: symlink is not allowed", cleanedDir) + } + if !info.IsDir() { + return fmt.Errorf("unsafe metrics directory %q: not a directory", cleanedDir) + } + if info.Mode()&os.ModeSticky != 0 && info.Mode().Perm()&0o022 != 0 { + return fmt.Errorf("metrics directory %q is shared; choose a dedicated Pulse-owned subdirectory", cleanedDir) + } + // Do not try to harden shared parents such as /tmp or /dev/shm. Apart from + // usually failing for the unprivileged service user, a root-run container + // could otherwise make the shared directory inaccessible to the host. A + // dedicated child remains safe and lets Pulse enforce owner-only access. + if !directoryOwnedByCurrentUser(info) { + return fmt.Errorf("metrics directory %q is not owned by the current user; choose a dedicated Pulse-owned subdirectory", cleanedDir) + } + if err := os.Chmod(cleanedDir, privateDirPerm); err != nil { + return fmt.Errorf("secure metrics directory %q to %04o (ensure it is writable in the service sandbox): %w", cleanedDir, privateDirPerm, err) + } + return nil } func rejectSymlinkOrNonRegular(path string) error { diff --git a/pkg/metrics/store_additional_test.go b/pkg/metrics/store_additional_test.go index 4c5c8f870..293c6cca4 100644 --- a/pkg/metrics/store_additional_test.go +++ b/pkg/metrics/store_additional_test.go @@ -326,6 +326,110 @@ func TestNewStoreCanonicalizesDBPath(t *testing.T) { } } +func TestEnsureOwnerOnlyDirHardensOwnedDirectory(t *testing.T) { + dir := filepath.Join(t.TempDir(), "metrics") + if err := os.Mkdir(dir, 0o755); err != nil { + t.Fatalf("Mkdir: %v", err) + } + + if err := ensureOwnerOnlyDir(dir); err != nil { + t.Fatalf("ensureOwnerOnlyDir: %v", err) + } + + info, err := os.Stat(dir) + if err != nil { + t.Fatalf("Stat: %v", err) + } + if got := info.Mode().Perm(); got != privateDirPerm { + t.Fatalf("directory permissions = %#o, want %#o", got, privateDirPerm) + } +} + +func TestEnsureOwnerOnlyDirRejectsFilesystemRoot(t *testing.T) { + root := string(filepath.Separator) + before, err := os.Stat(root) + if err != nil { + t.Fatalf("Stat root: %v", err) + } + + err = ensureOwnerOnlyDir(root) + if err == nil || !strings.Contains(err.Error(), "dedicated subdirectory") { + t.Fatalf("ensureOwnerOnlyDir(root) error = %v, want dedicated-subdirectory guidance", err) + } + + after, err := os.Stat(root) + if err != nil { + t.Fatalf("Stat root after rejection: %v", err) + } + if after.Mode().Perm() != before.Mode().Perm() { + t.Fatalf("root permissions changed from %#o to %#o", before.Mode().Perm(), after.Mode().Perm()) + } +} + +func TestEnsureOwnerOnlyDirRejectsSystemTemporaryDirectory(t *testing.T) { + dir := os.TempDir() + before, err := os.Stat(dir) + if err != nil { + t.Skipf("temporary directory is unavailable: %v", err) + } + if directoryOwnedByCurrentUser(before) { + t.Skip("temporary directory is owned by the test user on this platform") + } + + err = ensureOwnerOnlyDir(dir) + if err == nil || (!strings.Contains(err.Error(), "is shared") && !strings.Contains(err.Error(), "not owned by the current user")) { + t.Fatalf("ensureOwnerOnlyDir(%q) error = %v, want shared-directory or ownership guidance", dir, err) + } + + after, err := os.Stat(dir) + if err != nil { + t.Fatalf("Stat shared directory after rejection: %v", err) + } + if after.Mode().Perm() != before.Mode().Perm() { + t.Fatalf("shared directory permissions changed from %#o to %#o", before.Mode().Perm(), after.Mode().Perm()) + } +} + +func TestEnsureOwnerOnlyDirRejectsOwnedStickySharedDirectory(t *testing.T) { + dir := filepath.Join(t.TempDir(), "shared") + if err := os.Mkdir(dir, 0o777); err != nil { + t.Fatalf("Mkdir: %v", err) + } + if err := os.Chmod(dir, os.ModeSticky|0o777); err != nil { + t.Skipf("sticky directories are unavailable: %v", err) + } + + err := ensureOwnerOnlyDir(dir) + if err == nil || !strings.Contains(err.Error(), "is shared") { + t.Fatalf("ensureOwnerOnlyDir(sticky shared directory) error = %v, want shared-directory guidance", err) + } + + after, err := os.Stat(dir) + if err != nil { + t.Fatalf("Stat shared directory after rejection: %v", err) + } + if after.Mode().Perm() != 0o777 || after.Mode()&os.ModeSticky == 0 { + t.Fatalf("shared directory mode changed to %v", after.Mode()) + } +} + +func TestEnsureOwnerOnlyDirRejectsSymlink(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "target") + link := filepath.Join(root, "metrics") + if err := os.Mkdir(target, privateDirPerm); err != nil { + t.Fatalf("Mkdir target: %v", err) + } + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks are unavailable: %v", err) + } + + err := ensureOwnerOnlyDir(link) + if err == nil || !strings.Contains(err.Error(), "symlink is not allowed") { + t.Fatalf("ensureOwnerOnlyDir(symlink) error = %v, want symlink rejection", err) + } +} + func TestStoreFilesOwnerOnlyUnderPermissiveUmask(t *testing.T) { oldUmask := syscall.Umask(0o022) defer syscall.Umask(oldUmask)