From 0ad043d14a92720829296251344064158118686f Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Sat, 4 Apr 2026 06:59:39 +0200 Subject: [PATCH] review: address all 5 Copilot review comments - userdel: scope SignalBlocker to file mutations only, drop before home directory removal so long-running deletions stay interruptible - usermod: drop SignalBlocker before recursive_chown so large home trees remain interruptible - useradd: drop SignalBlocker after lock release, before subid/home creation steps that may be long-running - atomic: use PhantomData> instead of PhantomData<*const ()> for clearer !Send/!Sync opt-out on UmaskGuard - newgrp: replace harden_process() with suppress_core_dumps() + sanitized_env() to avoid leaking RLIMIT_FSIZE into exec'd shell --- src/shadow-core/src/atomic.rs | 5 ++++- src/uu/newgrp/src/newgrp.rs | 5 ++++- src/uu/useradd/src/useradd.rs | 12 +++++++----- src/uu/userdel/src/userdel.rs | 9 ++++++--- src/uu/usermod/src/usermod.rs | 9 ++++++--- 5 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/shadow-core/src/atomic.rs b/src/shadow-core/src/atomic.rs index 4827ab2..9460b9a 100644 --- a/src/shadow-core/src/atomic.rs +++ b/src/shadow-core/src/atomic.rs @@ -30,7 +30,10 @@ use crate::error::ShadowError; /// `umask(2)` is a process-wide operation. This guard is NOT safe to use /// from multiple threads concurrently. All shadow-rs tools are /// single-threaded, so this is not an issue in practice. -struct UmaskGuard(nix::sys::stat::Mode, std::marker::PhantomData<*const ()>); +struct UmaskGuard( + nix::sys::stat::Mode, + std::marker::PhantomData>, +); impl UmaskGuard { /// Set umask to zero and return a guard that restores the original. diff --git a/src/uu/newgrp/src/newgrp.rs b/src/uu/newgrp/src/newgrp.rs index 44fb24a..67a9787 100644 --- a/src/uu/newgrp/src/newgrp.rs +++ b/src/uu/newgrp/src/newgrp.rs @@ -221,7 +221,10 @@ fn verify_password(password: &str, hash: &str) -> Result { #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { - let _clean_env = shadow_core::hardening::harden_process(); + // newgrp execs a shell, so only suppress core dumps — do NOT raise + // RLIMIT_FSIZE as that would leak into the user's interactive session. + shadow_core::hardening::suppress_core_dumps(); + let _clean_env = shadow_core::hardening::sanitized_env(); let matches = match uu_app().try_get_matches_from(args) { Ok(m) => m, diff --git a/src/uu/useradd/src/useradd.rs b/src/uu/useradd/src/useradd.rs index 79970e1..d766bb7 100644 --- a/src/uu/useradd/src/useradd.rs +++ b/src/uu/useradd/src/useradd.rs @@ -475,10 +475,9 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { validate::validate_username(&opts.login) .map_err(|e| UseraddError::BadArgument(format!("{e}")))?; - // Step 2: Block signals for the duration of the critical section so a - // SIGINT between lock acquisition and atomic_write cannot leave stale - // lock files on disk. - let _signals = shadow_core::hardening::SignalBlocker::block_critical() + // Step 2: Block signals for the lock→write critical section only. + // Dropped after file writes complete so home creation remains interruptible. + let signals = shadow_core::hardening::SignalBlocker::block_critical() .map_err(|e| UseraddError::CannotUpdatePasswd(format!("cannot block signals: {e}")))?; // Acquire locks BEFORE reading so concurrent useradd cannot @@ -588,9 +587,12 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { }; write_shadow_entry(&shadow_path, &shadow_entry)?; - // Release locks now that passwd, group, and shadow writes are complete. + // Release locks and signal blocker now that passwd, group, and shadow writes are complete. + // Subsequent steps (subid, supplementary groups, home creation) are individually + // crash-safe and may be long-running, so signals should be interruptible. drop(group_lock); drop(passwd_lock); + drop(signals); // Step 14: Allocate subordinate UID/GID ranges for rootless containers. // Only done when the relevant file exists (matching GNU shadow-utils behavior). diff --git a/src/uu/userdel/src/userdel.rs b/src/uu/userdel/src/userdel.rs index d78ab88..6d4362e 100644 --- a/src/uu/userdel/src/userdel.rs +++ b/src/uu/userdel/src/userdel.rs @@ -118,9 +118,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { None }; - // Block signals for the duration of the critical section so a SIGINT - // between lock acquisition and atomic_write cannot leave stale lock files. - let _signals = shadow_core::hardening::SignalBlocker::block_critical() + // Block signals for the file-modification critical section only. + // Dropped before home removal so long-running deletions remain interruptible. + let signals = shadow_core::hardening::SignalBlocker::block_critical() .map_err(|e| UserdelError::CantUpdatePasswd(format!("cannot block signals: {e}")))?; // 1. Remove from /etc/passwd @@ -145,6 +145,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let _ = remove_from_gshadow_members(&gshadow_path, login); } + // Restore signals before potentially long-running home removal. + drop(signals); + // 5. Optionally remove home directory (using the path saved from passwd). if remove_home { if let Some(ref home_dir) = saved_home diff --git a/src/uu/usermod/src/usermod.rs b/src/uu/usermod/src/usermod.rs index b924445..7bab695 100644 --- a/src/uu/usermod/src/usermod.rs +++ b/src/uu/usermod/src/usermod.rs @@ -102,9 +102,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { return Err(UsermodError::CantUpdate("Permission denied.".into()).into()); } - // Block signals for the duration of the critical section so a SIGINT - // between lock acquisition and atomic_write cannot leave stale lock files. - let _signals = shadow_core::hardening::SignalBlocker::block_critical() + // Block signals for the passwd lock→write critical section only. + // Dropped before recursive_chown so long-running operations remain interruptible. + let signals = shadow_core::hardening::SignalBlocker::block_critical() .map_err(|e| UsermodError::CantUpdate(format!("cannot block signals: {e}")))?; // Modify /etc/passwd. @@ -159,6 +159,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { .map_err(|e| UsermodError::CantUpdate(format!("{e}")))?; drop(lock); + // Restore signals before potentially long-running recursive chown. + drop(signals); + // If the UID changed and the home directory was not explicitly moved, // recursively chown the existing home directory to the new UID. // Only files owned by old_uid are touched (files owned by other users