diff --git a/.github/workflows/pmg-e2e.yml b/.github/workflows/pmg-e2e.yml index 4424a09..4f6da54 100644 --- a/.github/workflows/pmg-e2e.yml +++ b/.github/workflows/pmg-e2e.yml @@ -916,6 +916,17 @@ jobs: fi echo "SUCCESS: private binary rejected" + - name: Reject user-owned PMG binary for system install + run: | + mkdir -p "$HOME/pmg-user-writable" + cp bin/pmg "$HOME/pmg-user-writable/pmg" + chmod 755 "$HOME/pmg-user-writable/pmg" + if sudo "$HOME/pmg-user-writable/pmg" setup install --system; then + echo "ERROR: system install accepted a user-owned binary" + exit 1 + fi + echo "SUCCESS: user-owned binary rejected" + - name: Install PMG system-wide run: | sudo install -m 755 bin/pmg /usr/local/bin/pmg @@ -956,7 +967,7 @@ jobs: out=$(pmg setup doctor 2>&1 || true) echo "$out" echo "$out" | grep -q 'No aliases (system install)' - echo "$out" | grep -q 'System shim directory is in PATH' + echo "$out" | grep -Eq 'npm resolves to system shim|System shim directory is in PATH' - name: Non-root user interception via system shims run: | diff --git a/cmd/setup/doctor.go b/cmd/setup/doctor.go index 3dc1845..cb12122 100644 --- a/cmd/setup/doctor.go +++ b/cmd/setup/doctor.go @@ -3,7 +3,9 @@ package setup import ( "fmt" "os" + "os/exec" "path/filepath" + "strings" "github.com/safedep/pmg/config" "github.com/safedep/pmg/internal/alias" @@ -139,8 +141,9 @@ func runCoreChecks(cfg *config.RuntimeConfig) []doctor.CheckResult { } if installed { return doctor.CheckResult{ - Status: doctor.StatusPass, - Message: aliasesInstalledMessage, + Status: doctor.StatusPass, + Message: aliasesInstalledMessage, + ImpliesInterception: true, } } if shim.SystemShimsInstalled() { @@ -189,32 +192,7 @@ func runCoreChecks(cfg *config.RuntimeConfig) []doctor.CheckResult { { Name: checkShimInPath, Category: "Shell Integration", - Run: func() doctor.CheckResult { - pathEntries := filepath.SplitList(os.Getenv("PATH")) - systemDir := shim.SystemBinDir() - if shim.SystemShimsInstalled() && pathContainsDir(pathEntries, systemDir) { - return doctor.CheckResult{ - Status: doctor.StatusPass, - Message: "System shim directory is in PATH", - } - } - if userDir, err := shim.UserBinDir(); err == nil && pathContainsDir(pathEntries, userDir) { - return doctor.CheckResult{ - Status: doctor.StatusPass, - Message: "Shim directory is in PATH", - } - } - if shim.SystemShimsInstalled() { - return doctor.CheckResult{ - Status: doctor.StatusFail, - Message: "System shim directory not in PATH", - } - } - return doctor.CheckResult{ - Status: doctor.StatusFail, - Message: "Shim directory not in PATH", - } - }, + Run: checkShimInPathResult, }, { Name: checkProxyMode, @@ -313,6 +291,80 @@ func pathContainsDir(pathEntries []string, dir string) bool { return false } +func pathIsUnderDir(path, dir string) bool { + if path == "" || dir == "" { + return false + } + cleanPath := filepath.Clean(path) + cleanDir := filepath.Clean(dir) + if cleanPath == cleanDir { + return true + } + prefix := cleanDir + string(os.PathSeparator) + return strings.HasPrefix(cleanPath, prefix) +} + +func checkShimInPathResult() doctor.CheckResult { + pathEntries := filepath.SplitList(os.Getenv("PATH")) + systemDir := shim.SystemBinDir() + userDir, userDirErr := shim.UserBinDir() + resolved, lookErr := exec.LookPath("npm") + + if lookErr == nil { + if shim.SystemShimsInstalled() && pathIsUnderDir(resolved, systemDir) { + return doctor.CheckResult{ + Status: doctor.StatusPass, + Message: "npm resolves to system shim", + ImpliesInterception: true, + } + } + if userDirErr == nil && pathIsUnderDir(resolved, userDir) { + return doctor.CheckResult{ + Status: doctor.StatusPass, + Message: "npm resolves to PMG shim", + ImpliesInterception: true, + } + } + } + + if shim.SystemShimsInstalled() && pathContainsDir(pathEntries, systemDir) { + if lookErr == nil { + return doctor.CheckResult{ + Status: doctor.StatusWarn, + Message: fmt.Sprintf("System shim directory is in PATH, but npm resolves to %s", resolved), + } + } + return doctor.CheckResult{ + Status: doctor.StatusPass, + Message: "System shim directory is in PATH", + ImpliesInterception: true, + } + } + if userDirErr == nil && pathContainsDir(pathEntries, userDir) { + if lookErr == nil { + return doctor.CheckResult{ + Status: doctor.StatusWarn, + Message: fmt.Sprintf("Shim directory is in PATH, but npm resolves to %s", resolved), + } + } + return doctor.CheckResult{ + Status: doctor.StatusPass, + Message: "Shim directory is in PATH", + ImpliesInterception: true, + } + } + if shim.SystemShimsInstalled() { + return doctor.CheckResult{ + Status: doctor.StatusFail, + Message: "System shim directory not in PATH", + } + } + return doctor.CheckResult{ + Status: doctor.StatusFail, + Message: "Shim directory not in PATH", + } +} + func runProtectionChecks(coreResults []doctor.CheckResult) []doctor.CheckResult { if !isInterceptionActive(coreResults) { var results []doctor.CheckResult @@ -344,10 +396,7 @@ func runProtectionChecks(coreResults []doctor.CheckResult) []doctor.CheckResult func isInterceptionActive(coreResults []doctor.CheckResult) bool { for _, r := range coreResults { - if r.Name == checkShimInPath && r.Status == doctor.StatusPass { - return true - } - if r.Name == checkShellAliases && r.Status == doctor.StatusPass && r.Message == aliasesInstalledMessage { + if r.ImpliesInterception { return true } } diff --git a/cmd/setup/doctor_system_test.go b/cmd/setup/doctor_system_test.go index 5522bf2..15d010f 100644 --- a/cmd/setup/doctor_system_test.go +++ b/cmd/setup/doctor_system_test.go @@ -13,6 +13,12 @@ func TestPathContainsDir(t *testing.T) { assert.False(t, pathContainsDir([]string{"/usr/bin"}, "")) } +func TestPathIsUnderDir(t *testing.T) { + assert.True(t, pathIsUnderDir("/usr/local/lib/pmg/bin/npm", "/usr/local/lib/pmg/bin")) + assert.False(t, pathIsUnderDir("/usr/local/bin/npm", "/usr/local/lib/pmg/bin")) + assert.False(t, pathIsUnderDir("/usr/local/lib/pmg/bin-extra/npm", "/usr/local/lib/pmg/bin")) +} + func TestSystemInstallAliasesPassDoesNotActivateInterception(t *testing.T) { results := []doctor.CheckResult{ {Name: checkShellAliases, Status: doctor.StatusPass, Message: "No aliases (system install)"}, @@ -24,9 +30,27 @@ func TestSystemInstallAliasesPassDoesNotActivateInterception(t *testing.T) { func TestAliasesInstalledActivatesInterception(t *testing.T) { results := []doctor.CheckResult{ - {Name: checkShellAliases, Status: doctor.StatusPass, Message: aliasesInstalledMessage}, + { + Name: checkShellAliases, + Status: doctor.StatusPass, + Message: aliasesInstalledMessage, + ImpliesInterception: true, + }, {Name: checkShimInPath, Status: doctor.StatusFail}, } assert.True(t, isInterceptionActive(results)) } + +func TestShimInPathImpliesInterception(t *testing.T) { + results := []doctor.CheckResult{ + { + Name: checkShimInPath, + Status: doctor.StatusPass, + Message: "npm resolves to system shim", + ImpliesInterception: true, + }, + } + + assert.True(t, isInterceptionActive(results)) +} diff --git a/cmd/setup/setup.go b/cmd/setup/setup.go index 9e818cf..08133e2 100644 --- a/cmd/setup/setup.go +++ b/cmd/setup/setup.go @@ -105,7 +105,7 @@ func install(system bool) error { } func installSystem() error { - if err := errIfSystemInstallAllowed(); err != nil { + if err := requireSystemInstallSupported(); err != nil { return err } @@ -190,11 +190,11 @@ func remove(system, removeConfig bool) error { } func removeSystem(removeConfig bool) error { - if err := errIfSystemInstallAllowed(); err != nil { + if err := requireSystemInstallSupported(); err != nil { return err } - shimMgr, err := shim.NewSystemShimManager() + shimMgr, err := shim.NewSystemShimManagerForRemove() if err != nil { return fmt.Errorf("failed to create system shim manager: %w", err) } @@ -212,7 +212,7 @@ func removeSystem(removeConfig bool) error { return nil } -func errIfSystemInstallAllowed() error { +func requireSystemInstallSupported() error { if runtime.GOOS != "linux" { return usefulerror.NewUsefulError(). WithCode(errcodes.UnsupportedPlatform). diff --git a/cmd/setup/setup_test.go b/cmd/setup/setup_test.go index a1f1447..a881b77 100644 --- a/cmd/setup/setup_test.go +++ b/cmd/setup/setup_test.go @@ -15,7 +15,7 @@ func TestErrIfSystemInstallAllowed(t *testing.T) { t.Cleanup(func() { setupGeteuid = orig }) setupGeteuid = func() int { return 0 } - err := errIfSystemInstallAllowed() + err := requireSystemInstallSupported() if runtime.GOOS == "linux" { assert.NoError(t, err) } else { @@ -26,7 +26,7 @@ func TestErrIfSystemInstallAllowed(t *testing.T) { } setupGeteuid = func() int { return 1000 } - err = errIfSystemInstallAllowed() + err = requireSystemInstallSupported() require.Error(t, err) usefulErr, ok := usefulerror.AsUsefulError(err) require.True(t, ok) diff --git a/docs/system-install.md b/docs/system-install.md index ddc1662..068d6a2 100644 --- a/docs/system-install.md +++ b/docs/system-install.md @@ -90,6 +90,7 @@ Optional lockdown (`global_lockdown: true`) is documented in [config.md](./confi ## Limitations - **Virtualenv.** After `source .venv/bin/activate`, bare `pip` uses the venv binary and skips PMG shims. Call `pmg pip …` explicitly. +- **Version managers.** Tools like nvm, pyenv, volta, and asdf often prepend their own bin directories from shell rc files that run after `/etc/profile.d`. That can put real `npm`/`pip` ahead of PMG shims even when the shim directory is on `PATH`. Prefer putting `/usr/local/lib/pmg/bin` first in a durable `ENV PATH` / login PATH, or call `pmg npm` / `pmg pip` explicitly. `pmg setup doctor` warns when `npm` resolves outside the shim directory. - **No shell aliases.** System install only installs PATH shims. There is no `~/.pmg.rc` alias layer. - **Config changes.** `pmg config set` and `pmg config edit` are unavailable while the system config is active. Edit `/etc/safedep/pmg/config.yml` as root, or redeploy the file. - **Custom sandbox `policy_templates`.** Relative paths in the system config resolve under each user's config directory, not `/etc/safedep/pmg`. Prefer absolute paths. diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 82b009b..23e4c04 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -9,10 +9,11 @@ const ( ) type CheckResult struct { - Name string - Category string - Status CheckStatus - Message string + Name string + Category string + Status CheckStatus + Message string + ImpliesInterception bool } type Check struct { diff --git a/internal/shim/file_owner_unix.go b/internal/shim/file_owner_unix.go new file mode 100644 index 0000000..d5ceeb8 --- /dev/null +++ b/internal/shim/file_owner_unix.go @@ -0,0 +1,16 @@ +//go:build unix + +package shim + +import ( + "os" + "syscall" +) + +func fileOwnerUID(info os.FileInfo) (uint32, bool) { + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return 0, false + } + return uint32(stat.Uid), true +} diff --git a/internal/shim/file_owner_windows.go b/internal/shim/file_owner_windows.go new file mode 100644 index 0000000..f79d04c --- /dev/null +++ b/internal/shim/file_owner_windows.go @@ -0,0 +1,9 @@ +//go:build windows + +package shim + +import "os" + +func fileOwnerUID(info os.FileInfo) (uint32, bool) { + return 0, false +} diff --git a/internal/shim/shim.go b/internal/shim/shim.go index f8bcaf3..5b71f7a 100644 --- a/internal/shim/shim.go +++ b/internal/shim/shim.go @@ -82,7 +82,7 @@ func (m *ShimManager) Install() error { } if m.config.ManageProfile { - if err := writeSystemProfile(); err != nil { + if err := writeSystemProfile(m.config.BinDir); err != nil { return fmt.Errorf("failed to write system profile: %w", err) } } diff --git a/internal/shim/system.go b/internal/shim/system.go index b74fb21..cf2173d 100644 --- a/internal/shim/system.go +++ b/internal/shim/system.go @@ -20,6 +20,9 @@ const ( var ( systemBinDirOverride string systemProfilePathOverride string + // systemExecutableOwnershipCheck requires root ownership of the binary and + // its parent directories. Disabled in tests that cannot create root-owned files. + systemExecutableOwnershipCheck = true ) // SystemBinDir returns the directory for system-wide PMG shims. @@ -40,15 +43,29 @@ func SystemProfilePath() string { // NewSystemShimManager creates a shim manager for system-wide install: shims // under SystemBinDir, no per-user rc edits, and /etc/profile.d management. +// The current executable is validated for multi-user use. func NewSystemShimManager() (*ShimManager, error) { + return newSystemShimManager(true) +} + +// NewSystemShimManagerForRemove creates a system shim manager without +// validating the current executable. Uninstall must work even when the binary +// that originally installed the shims is no longer suitable for install. +func NewSystemShimManagerForRemove() (*ShimManager, error) { + return newSystemShimManager(false) +} + +func newSystemShimManager(validateExecutable bool) (*ShimManager, error) { aliasCfg := alias.DefaultConfig() pmgBin, err := currentExecutable() if err != nil { return nil, err } - if err := validateSystemExecutable(pmgBin); err != nil { - return nil, err + if validateExecutable { + if err := validateSystemExecutable(pmgBin); err != nil { + return nil, err + } } return &ShimManager{ @@ -62,19 +79,70 @@ func NewSystemShimManager() (*ShimManager, error) { }, nil } -// validateSystemExecutable rejects binaries other users cannot execute. System shims hard-code this path. +// validateSystemExecutable rejects binaries unsafe for system-wide shims. +// System shims hard-code this path, so it must be world-executable, not +// group/other-writable, and (when ownership checks are enabled) root-owned +// under a root-owned, non-group/other-writable directory chain. func validateSystemExecutable(path string) error { info, err := os.Stat(path) if err != nil { return fmt.Errorf("failed to inspect pmg executable %s: %w", path, err) } - if info.Mode().Perm()&0o001 == 0 { + perm := info.Mode().Perm() + if perm&0o001 == 0 { return fmt.Errorf("pmg executable %s is not executable by all users", path) } + if perm&0o022 != 0 { + return fmt.Errorf("pmg executable %s is writable by group or others", path) + } + + if systemExecutableOwnershipCheck { + if err := requireRootOwnedPath(path, info); err != nil { + return err + } + if err := requireSafeAncestorDirs(filepath.Dir(path)); err != nil { + return err + } + } return nil } +func requireRootOwnedPath(path string, info os.FileInfo) error { + uid, ok := fileOwnerUID(info) + if !ok { + return fmt.Errorf("cannot determine owner of %s", path) + } + if uid != 0 { + return fmt.Errorf("pmg executable %s must be owned by root", path) + } + return nil +} + +func requireSafeAncestorDirs(dir string) error { + for { + info, err := os.Stat(dir) + if err != nil { + return fmt.Errorf("failed to inspect directory %s: %w", dir, err) + } + if info.Mode().Perm()&0o022 != 0 { + return fmt.Errorf("directory %s on pmg executable path is writable by group or others", dir) + } + uid, ok := fileOwnerUID(info) + if !ok { + return fmt.Errorf("cannot determine owner of directory %s", dir) + } + if uid != 0 { + return fmt.Errorf("directory %s on pmg executable path must be owned by root", dir) + } + parent := filepath.Dir(dir) + if parent == dir { + return nil + } + dir = parent + } +} + // SystemShimsInstalled reports whether the system shim directory contains at // least one shim script. func SystemShimsInstalled() bool { @@ -108,8 +176,7 @@ func SystemProfileInstalled() bool { return strings.Contains(string(data), systemProfileMarker) } -func writeSystemProfile() error { - binDir := SystemBinDir() +func writeSystemProfile(binDir string) error { path := SystemProfilePath() if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { diff --git a/internal/shim/system_test.go b/internal/shim/system_test.go index e3f707c..6f577d4 100644 --- a/internal/shim/system_test.go +++ b/internal/shim/system_test.go @@ -13,9 +13,11 @@ func useSystemPaths(t *testing.T, dir string) { t.Helper() systemBinDirOverride = filepath.Join(dir, "bin") systemProfilePathOverride = filepath.Join(dir, "profile.d", "pmg.sh") + systemExecutableOwnershipCheck = false t.Cleanup(func() { systemBinDirOverride = "" systemProfilePathOverride = "" + systemExecutableOwnershipCheck = true }) } @@ -100,15 +102,20 @@ func TestWriteSystemProfileRepairsStalePath(t *testing.T) { 0o644, )) - require.NoError(t, writeSystemProfile()) + binDir := filepath.Join(root, "custom-bin") + require.NoError(t, writeSystemProfile(binDir)) content, err := os.ReadFile(SystemProfilePath()) require.NoError(t, err) - assert.Contains(t, string(content), SystemBinDir()) + assert.Contains(t, string(content), binDir) assert.NotContains(t, string(content), "/stale/path") + assert.NotContains(t, string(content), SystemBinDir()) } func TestValidateSystemExecutableRejectsPrivateBinary(t *testing.T) { + systemExecutableOwnershipCheck = false + t.Cleanup(func() { systemExecutableOwnershipCheck = true }) + privateDir := t.TempDir() privateExecutable := filepath.Join(privateDir, "pmg") require.NoError(t, os.WriteFile(privateExecutable, []byte("binary"), 0o700)) @@ -118,3 +125,40 @@ func TestValidateSystemExecutableRejectsPrivateBinary(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "not executable by all users") } + +func TestValidateSystemExecutableRejectsGroupWritable(t *testing.T) { + systemExecutableOwnershipCheck = false + t.Cleanup(func() { systemExecutableOwnershipCheck = true }) + + dir := t.TempDir() + path := filepath.Join(dir, "pmg") + require.NoError(t, os.WriteFile(path, []byte("binary"), 0o755)) + require.NoError(t, os.Chmod(path, 0o775)) + + err := validateSystemExecutable(path) + + require.Error(t, err) + assert.Contains(t, err.Error(), "writable by group or others") +} + +func TestValidateSystemExecutableRejectsNonRootOwner(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "pmg") + require.NoError(t, os.WriteFile(path, []byte("binary"), 0o755)) + + err := validateSystemExecutable(path) + + require.Error(t, err) + assert.Contains(t, err.Error(), "must be owned by root") +} + +func TestNewSystemShimManagerForRemoveSkipsValidation(t *testing.T) { + root := t.TempDir() + useSystemPaths(t, root) + systemExecutableOwnershipCheck = true + + mgr, err := NewSystemShimManagerForRemove() + require.NoError(t, err) + require.NoError(t, mgr.Install()) + require.NoError(t, mgr.Remove()) +} diff --git a/internal/ui/info.go b/internal/ui/info.go index d69b6f1..dfa181a 100644 --- a/internal/ui/info.go +++ b/internal/ui/info.go @@ -38,6 +38,7 @@ func PrintSetupSystemInstallCmdInfo(shimBinDir, configDir, profilePath string) { fmt.Printf(" %s\n", Colors.Dim(fmt.Sprintf("Shims: %s", shimBinDir))) fmt.Printf(" %s\n", Colors.Dim(fmt.Sprintf("Config: %s", configDir))) fmt.Printf(" %s\n", Colors.Dim(fmt.Sprintf("Profile: %s", profilePath))) + fmt.Printf(" %s\n", Colors.Dim(fmt.Sprintf("Per-user config files are now ignored; edit %s/config.yml as root.", configDir))) fmt.Printf("\n%s For Docker builds (RUN does not source profile.d), add:\n", Colors.Dim("ℹ")) fmt.Printf(" %s\n", Colors.Bold(fmt.Sprintf(`ENV PATH="%s:$PATH"`, shimBinDir))) fmt.Printf("%s Login shells pick up PATH from profile.d. After venv activate, use `pmg pip`.\n", Colors.Dim("ℹ")) diff --git a/main.go b/main.go index b20be6e..3d42efb 100644 --- a/main.go +++ b/main.go @@ -103,10 +103,7 @@ func main() { } if eventlogErr != nil { - log.Warnf("failed to initialize event logging: %v", eventlogErr) - // Soft-fail: unusable HOME must not block package-manager commands. - ui.Infof("%s [pmg] Event logging unavailable (%v)", - ui.Colors.Yellow("⚠"), eventlogErr) + ui.Fatalf("failed to initialize event logging: %v", eventlogErr) } if err := audit.Initialize(config.Get()); err != nil {