mirror of
https://github.com/safedep/pmg.git
synced 2026-08-03 07:24:09 +02:00
fix: harden system dirs at install; keep sudo attribution without passwd
Address remaining review comments: - shim: force root:root 0755 on the managed system dirs (shim tree and profile.d) after MkdirAll. A pre-created dir with weaker ownership, possible under Debian's group-writable /usr/local/lib, would let a non-root user replace the shims every account executes. - audit: when SUDO_USER has no passwd entry (minimal containers), attribute cloud events from sudo's recorded SUDO_USER/SUDO_UID env instead of falling back to root. Still gated on euid 0. - setup: reword the root-without---system warning; alias/shim install follows HOME, so claiming it configures only root's home was wrong. - shim: skip the non-root-owner validation test on Windows, where file ownership is not resolvable.
This commit is contained in:
+1
-1
@@ -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 {
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user