From 4d65a2c5f03bc9ed5f783ed1dae831f8bf5b9b78 Mon Sep 17 00:00:00 2001 From: Sahilb315 Date: Tue, 14 Jul 2026 14:58:13 +0530 Subject: [PATCH] fix: stop reowning /etc/profile.d; document group-writable and su gaps writeSystemProfile chowned/chmod'd /etc/profile.d itself, a shared system directory pmg does not own, silently overriding any perms a sysadmin set on it. Secure only the file pmg writes (pmg.sh) via secureSystemFile, which also forces 0644 explicitly so a restrictive root umask cannot leave the snippet non-world-readable (which would drop the shim dir from other users' login-shell PATH). Docs: add Limitations entries for the group-writable install dir bypass (validation is defeatable on non-sticky group-writable dirs like Debian's /usr/local/bin) and the elevation-only scope (su without - can still poison the caller's home; sudo -u cannot poison another account). Trim the requireSafeParentDir comment to a pointer. --- docs/system-install.md | 2 ++ internal/shim/system.go | 42 +++++++++++++++++++++++++---------------- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/docs/system-install.md b/docs/system-install.md index 725658a..a14f7cd 100644 --- a/docs/system-install.md +++ b/docs/system-install.md @@ -98,6 +98,8 @@ Optional lockdown (`global_lockdown: true`) is documented in [config.md](./confi - **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. - **`pmg sandbox allow`.** Blocked when the system config sets `global_lockdown: true`. +- **Group-writable install directory.** The binary must be root-owned and non-writable, but if its directory is group-writable without the sticky bit (Debian/Ubuntu ship `/usr/local/bin` as `root:staff` mode `2775`), a group member can delete the root-owned binary and replace it, bypassing the check. `staff` is empty by default, so default exposure is nil; on a multi-user host where the group is not trusted, run `sudo chmod g-w /usr/local/bin` or install into a `root:root` directory. +- **Elevation only, not impersonation.** Root's per-user data is diverted to `/root` only for `sudo` to root (detected via `SUDO_USER`). `su` without `-` becomes root with no marker, so it can still create root-owned files in the caller's home; the caller sees a clear error and chown fix on their next `pmg` run. `sudo -u ` runs with only that user's rights, so it cannot poison another account at all, it just fails. Prefer `sudo` or `su -`, or set `PMG_CONFIG_DIR`. ## User data directories diff --git a/internal/shim/system.go b/internal/shim/system.go index 4864e69..196f8be 100644 --- a/internal/shim/system.go +++ b/internal/shim/system.go @@ -153,18 +153,11 @@ func requireRootOwnedPath(path string, info os.FileInfo) error { return nil } -// requireSafeParentDir validates only the immediate parent of the executable, -// not the full chain up to /. It requires a root-owned, non-world-writable -// parent so an unprivileged account cannot swap the shared binary that every -// user's shims exec; a maliciously writable grandparent is out of scope. -// (Reachability of the full chain is separately enforced by -// requirePathSearchableByAll.) -// -// Group-writable is allowed deliberately: Debian/Ubuntu ship /usr/local/bin as -// root:staff mode 2775, so rejecting group-writable would refuse the documented -// install location out of the box. The tradeoff is that a member of the parent -// directory's group can replace the binary — harden the directory (chmod g-w) -// on multi-user hosts where that group is not trusted. +// requireSafeParentDir requires a root-owned, non-world-writable immediate +// parent. Group-writable is allowed on purpose so Debian/Ubuntu's default +// /usr/local/bin (root:staff 2775) is not rejected; the resulting bypass on +// group-writable non-sticky dirs is covered in docs/system-install.md +// Limitations. func requireSafeParentDir(dir string) error { info, err := os.Stat(dir) if err != nil { @@ -286,12 +279,12 @@ func SystemProfileInstalled() bool { func writeSystemProfile(binDir string) error { path := SystemProfilePath() + // Do not chown/chmod /etc/profile.d itself: it is a shared system directory + // pmg does not own, and other packages drop snippets there. We only secure + // the file we write, below. 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 @@ -300,7 +293,7 @@ export PATH="%s:$PATH" data, err := os.ReadFile(path) if err == nil && string(data) == content { - return nil + return secureSystemFile(path) } if err != nil && !os.IsNotExist(err) { @@ -310,6 +303,23 @@ export PATH="%s:$PATH" if err := os.WriteFile(path, []byte(content), 0o644); err != nil { return fmt.Errorf("failed to write system profile %s: %w", path, err) } + return secureSystemFile(path) +} + +// secureSystemFile forces root ownership and world-readable 0644 on a file pmg +// writes system-wide. This keeps the snippet readable by every user's login +// shell regardless of root's umask, and repairs a pre-existing file's owner +// without touching the shared directory it lives in. +func secureSystemFile(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, 0o644); err != nil { + return fmt.Errorf("failed to set permissions on %s: %w", path, err) + } return nil }