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<Rc<()>> 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
This commit is contained in:
Pierre Warnier
2026-04-04 06:59:39 +02:00
parent 92783375aa
commit 0ad043d14a
5 changed files with 27 additions and 13 deletions
+4 -1
View File
@@ -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<std::rc::Rc<()>>,
);
impl UmaskGuard {
/// Set umask to zero and return a guard that restores the original.
+4 -1
View File
@@ -221,7 +221,10 @@ fn verify_password(password: &str, hash: &str) -> Result<bool, NewgrpError> {
#[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,
+7 -5
View File
@@ -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).
+6 -3
View File
@@ -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
+6 -3
View File
@@ -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