diff --git a/cmd/setup/setup.go b/cmd/setup/setup.go index 3b5af04..d677ae6 100644 --- a/cmd/setup/setup.go +++ b/cmd/setup/setup.go @@ -60,7 +60,7 @@ func install(system bool) error { if setupGeteuid() == 0 { fmt.Printf("%s %s\n", ui.Colors.Yellow("⚠"), - "Running as root without --system configures only root's home. Use `pmg setup install --system` so all users are covered.") + "Running as root without --system does not protect other users. Use `pmg setup install --system` so all users are covered.") } if err := config.WriteTemplateConfig(); err != nil { diff --git a/internal/audit/cloud_sink.go b/internal/audit/cloud_sink.go index 32f3c77..44565d0 100644 --- a/internal/audit/cloud_sink.go +++ b/internal/audit/cloud_sink.go @@ -129,6 +129,9 @@ func invokingUser() *user.User { if u, err := user.Lookup(name); err == nil { return u } + // No passwd entry for the sudo user (minimal containers): keep + // the attribution sudo recorded rather than reporting root. + return &user.User{Username: name, Uid: os.Getenv("SUDO_UID")} } } u, err := user.Current() diff --git a/internal/audit/cloud_sink_test.go b/internal/audit/cloud_sink_test.go index abc2f81..ffc0bcc 100644 --- a/internal/audit/cloud_sink_test.go +++ b/internal/audit/cloud_sink_test.go @@ -180,3 +180,17 @@ func TestInvokingUserIgnoresSudoUserWhenNotElevated(t *testing.T) { require.NotNil(t, got) assert.Equal(t, current.Username, got.Username) } + +func TestInvokingUserKeepsSudoAttributionWithoutPasswdEntry(t *testing.T) { + orig := auditGeteuid + t.Cleanup(func() { auditGeteuid = orig }) + + auditGeteuid = func() int { return 0 } + t.Setenv("SUDO_USER", "no-such-user-xyz") + t.Setenv("SUDO_UID", "4242") + + got := invokingUser() + require.NotNil(t, got) + assert.Equal(t, "no-such-user-xyz", got.Username) + assert.Equal(t, "4242", got.Uid) +} diff --git a/internal/shim/shim.go b/internal/shim/shim.go index 69743f5..e385328 100644 --- a/internal/shim/shim.go +++ b/internal/shim/shim.go @@ -76,6 +76,14 @@ func (m *ShimManager) Install() error { return fmt.Errorf("failed to create shim directory %s: %w", m.config.BinDir, err) } + if m.config.ManageProfile { + for _, dir := range []string{filepath.Dir(m.config.BinDir), m.config.BinDir} { + if err := secureSystemDir(dir); err != nil { + return err + } + } + } + for _, pm := range m.config.PackageManagers { if err := m.writeShimScript(pm); err != nil { return fmt.Errorf("failed to write shim for %s: %w", pm, err) diff --git a/internal/shim/system.go b/internal/shim/system.go index 431c760..f8a8a1c 100644 --- a/internal/shim/system.go +++ b/internal/shim/system.go @@ -207,6 +207,24 @@ func ValidateSystemBinary(path string) error { return validateSystemExecutable(path) } +// secureSystemDir forces root ownership and 0755 on a directory pmg manages +// system-wide. MkdirAll leaves pre-existing directories untouched, so a dir +// pre-created with weaker ownership (possible under Debian's group-writable +// /usr/local/lib) would let a non-root user replace shims; this closes that +// hole. No-op when not running as root (unit tests, dry contexts). +func secureSystemDir(path string) error { + if os.Geteuid() != 0 { + return nil + } + if err := os.Chown(path, 0, 0); err != nil { + return fmt.Errorf("failed to set root ownership on %s: %w", path, err) + } + if err := os.Chmod(path, 0o755); err != nil { + return fmt.Errorf("failed to set permissions on %s: %w", path, err) + } + return nil +} + func shimsPresent(dir string) bool { _, ok := firstShimContent(dir) return ok @@ -246,6 +264,9 @@ func writeSystemProfile(binDir string) error { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return fmt.Errorf("failed to create profile.d directory: %w", err) } + if err := secureSystemDir(filepath.Dir(path)); err != nil { + return err + } content := fmt.Sprintf(`# %s - managed by pmg setup install --system # remove by running: pmg setup remove --system diff --git a/internal/shim/system_test.go b/internal/shim/system_test.go index 7c5b62b..d8d6b74 100644 --- a/internal/shim/system_test.go +++ b/internal/shim/system_test.go @@ -3,6 +3,7 @@ package shim import ( "os" "path/filepath" + "runtime" "testing" "github.com/stretchr/testify/assert" @@ -151,6 +152,9 @@ func TestValidateSystemExecutableRejectsGroupWritable(t *testing.T) { } func TestValidateSystemExecutableRejectsNonRootOwner(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("file ownership is not resolvable on Windows") + } if os.Geteuid() == 0 { t.Skip("running as root: temp file is root-owned, so the owner check passes") }