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.
This commit is contained in:
Sahilb315
2026-07-14 14:58:13 +05:30
parent f251a073e3
commit 4d65a2c5f0
2 changed files with 28 additions and 16 deletions
+2
View File
@@ -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 <user>` 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
+26 -16
View File
@@ -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
}