From 92783375aa65e91f77fefe90df91ae5f99f0bb52 Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Fri, 3 Apr 2026 19:27:25 +0200 Subject: [PATCH] review: fix all findings from comprehensive code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security fixes: - newgrp: add initgroups() before execv to reset supplementary groups (H-3) - pam: use Zeroizing for password buffers on all exit paths (M-5) - atomic: retry-once on stale tmp file from crashed run (M-3) - atomic: UmaskGuard opts out of Send/Sync via PhantomData (L-2) - lock: flush + fsync in write_pid_file before hard_link (L-7) Deduplication (~300 lines removed): - Centralize caller_is_root(), current_username(), SignalBlocker in shadow_core::hardening — remove 10+5+5 private copies across tools - Add SignalBlocker to all 6 remaining mutating tools (M-4) - Remove dead CantSort variant from grpck, fix pwck allow (M-8) - Remove dead read_passwd from test_chfn (M-8) Binary cleanup: - shadow-rs multicall: use ExitCode instead of process::exit (M-6) - completions: use ExitCode + Result pattern, remove unwrap/exit (H-3) Documentation: - crypt: thread-safety warning on public API (H-1) - shadow: clarify unlock() behavior for * password (H-2) - validate: document $ rejection deviation from GNU (L-4) - selinux/audit: note shell-out implementation status (L-6) - chpasswd/chage tests: TODO for --prefix integration (M-8) - userdel: document inline skip_unless_root reason (L-9) Testing: - Add fuzz targets for group and gshadow parsers (M-7) - skel: use create_dir for tighter error detection (L-8) - crypt: compile-time modulo bias assertion (L-1) --- fuzz/Cargo.toml | 12 +++- fuzz/fuzz_targets/fuzz_group_parse.rs | 18 +++++ fuzz/fuzz_targets/fuzz_gshadow_parse.rs | 18 +++++ src/bin/completions.rs | 58 +++++++++------- src/bin/shadow-rs.rs | 19 ++++-- src/shadow-core/src/atomic.rs | 29 +++++++- src/shadow-core/src/audit.rs | 4 ++ src/shadow-core/src/crypt.rs | 16 +++++ src/shadow-core/src/hardening.rs | 71 ++++++++++++++++++++ src/shadow-core/src/lock.rs | 7 ++ src/shadow-core/src/pam.rs | 17 ++--- src/shadow-core/src/selinux.rs | 4 ++ src/shadow-core/src/shadow.rs | 9 ++- src/shadow-core/src/skel.rs | 3 +- src/shadow-core/src/validate.rs | 4 ++ src/uu/chage/src/chage.rs | 78 ++-------------------- src/uu/chfn/src/chfn.rs | 69 +++---------------- src/uu/chpasswd/src/chpasswd.rs | 51 ++------------ src/uu/chsh/src/chsh.rs | 66 +++---------------- src/uu/groupadd/src/groupadd.rs | 12 ++-- src/uu/groupdel/src/groupdel.rs | 11 ++-- src/uu/groupmod/src/groupmod.rs | 11 ++-- src/uu/grpck/src/grpck.rs | 26 ++++---- src/uu/newgrp/src/newgrp.rs | 38 ++++------- src/uu/passwd/src/passwd.rs | 88 ++++--------------------- src/uu/pwck/src/pwck.rs | 2 +- src/uu/useradd/src/useradd.rs | 15 +++-- src/uu/userdel/src/userdel.rs | 7 ++ src/uu/usermod/src/usermod.rs | 5 ++ tests/by-util/test_chage.rs | 13 ++-- tests/by-util/test_chfn.rs | 6 -- tests/by-util/test_chpasswd.rs | 8 +++ 32 files changed, 363 insertions(+), 432 deletions(-) create mode 100644 fuzz/fuzz_targets/fuzz_group_parse.rs create mode 100644 fuzz/fuzz_targets/fuzz_gshadow_parse.rs diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index c07c608..614702c 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -9,7 +9,7 @@ cargo-fuzz = true [dependencies] libfuzzer-sys = "0.4" -shadow-core = { path = "../src/shadow-core", features = ["shadow", "login-defs"] } +shadow-core = { path = "../src/shadow-core", features = ["shadow", "login-defs", "group", "gshadow"] } tempfile = "3" [workspace] @@ -34,3 +34,13 @@ doc = false name = "fuzz_validate_username" path = "fuzz_targets/fuzz_validate_username.rs" doc = false + +[[bin]] +name = "fuzz_group_parse" +path = "fuzz_targets/fuzz_group_parse.rs" +doc = false + +[[bin]] +name = "fuzz_gshadow_parse" +path = "fuzz_targets/fuzz_gshadow_parse.rs" +doc = false diff --git a/fuzz/fuzz_targets/fuzz_group_parse.rs b/fuzz/fuzz_targets/fuzz_group_parse.rs new file mode 100644 index 0000000..9c58f5a --- /dev/null +++ b/fuzz/fuzz_targets/fuzz_group_parse.rs @@ -0,0 +1,18 @@ +// This file is part of the shadow-rs package. +// +// For the full copyright and license information, please view the LICENSE +// file that was distributed with this source code. + +//! Fuzz target for `/etc/group` line parsing. +//! +//! Ensures `GroupEntry::from_str` never panics on arbitrary input. + +#![no_main] +use libfuzzer_sys::fuzz_target; + +fuzz_target!(|data: &[u8]| { + if let Ok(s) = std::str::from_utf8(data) { + // Must not panic on any input — errors are fine. + let _ = s.parse::(); + } +}); diff --git a/fuzz/fuzz_targets/fuzz_gshadow_parse.rs b/fuzz/fuzz_targets/fuzz_gshadow_parse.rs new file mode 100644 index 0000000..d1d3d6f --- /dev/null +++ b/fuzz/fuzz_targets/fuzz_gshadow_parse.rs @@ -0,0 +1,18 @@ +// This file is part of the shadow-rs package. +// +// For the full copyright and license information, please view the LICENSE +// file that was distributed with this source code. + +//! Fuzz target for `/etc/gshadow` line parsing. +//! +//! Ensures `GshadowEntry::from_str` never panics on arbitrary input. + +#![no_main] +use libfuzzer_sys::fuzz_target; + +fuzz_target!(|data: &[u8]| { + if let Ok(s) = std::str::from_utf8(data) { + // Must not panic on any input — errors are fine. + let _ = s.parse::(); + } +}); diff --git a/src/bin/completions.rs b/src/bin/completions.rs index 7d35924..98ff357 100644 --- a/src/bin/completions.rs +++ b/src/bin/completions.rs @@ -117,13 +117,15 @@ fn cli() -> Command { ) } -fn generate_for_tool(name: &str, shell: Shell, out: &mut dyn io::Write) { +fn generate_for_tool(name: &str, shell: Shell, out: &mut dyn io::Write) -> Result<(), String> { if let Some(mut cmd) = get_tool_app(name) { generate(shell, &mut cmd, name, out); + Ok(()) } else { - eprintln!("unknown tool: {name}"); - eprintln!("available: {}", all_tool_names().join(", ")); - std::process::exit(1); + Err(format!( + "unknown tool: {name}\navailable: {}", + all_tool_names().join(", ") + )) } } @@ -138,32 +140,37 @@ fn shell_extension(shell: Shell) -> &'static str { } } -fn main() { +fn main() -> std::process::ExitCode { + match run() { + Ok(()) => std::process::ExitCode::SUCCESS, + Err(msg) => { + eprintln!("{msg}"); + std::process::ExitCode::FAILURE + } + } +} + +fn run() -> Result<(), String> { let matches = cli().get_matches(); - let shell: Shell = matches + + // clap enforces `--shell` is required, so the value is always present. + let shell_str = matches .get_one::("shell") - .expect("shell is required") - .parse() - .unwrap_or_else(|e| { - eprintln!("invalid shell: {e}"); - eprintln!("supported: bash, zsh, fish, elvish, powershell"); - std::process::exit(1); - }); + .expect("clap guarantees --shell is present"); + let shell: Shell = shell_str.parse().map_err(|e| { + format!("invalid shell: {e}\nsupported: bash, zsh, fish, elvish, powershell") + })?; if matches.get_flag("all") { let tools = all_tool_names(); if let Some(dir) = matches.get_one::("dir") { - std::fs::create_dir_all(dir).unwrap_or_else(|e| { - eprintln!("cannot create directory '{dir}': {e}"); - std::process::exit(1); - }); + std::fs::create_dir_all(dir) + .map_err(|e| format!("cannot create directory '{dir}': {e}"))?; let ext = shell_extension(shell); for name in &tools { let path = format!("{dir}/{name}.{ext}"); - let mut file = std::fs::File::create(&path).unwrap_or_else(|e| { - eprintln!("cannot create '{path}': {e}"); - std::process::exit(1); - }); + let mut file = std::fs::File::create(&path) + .map_err(|e| format!("cannot create '{path}': {e}"))?; if let Some(mut cmd) = get_tool_app(name) { generate(shell, &mut cmd, *name, &mut file); } @@ -179,9 +186,14 @@ fn main() { } } } else { - let tool = matches.get_one::("tool").expect("tool is required"); + // clap enforces `tool` is required when `--all` is absent. + let tool = matches + .get_one::("tool") + .expect("clap guarantees tool is present when --all is not set"); let stdout = io::stdout(); let mut out = stdout.lock(); - generate_for_tool(tool, shell, &mut out); + generate_for_tool(tool, shell, &mut out)?; } + + Ok(()) } diff --git a/src/bin/shadow-rs.rs b/src/bin/shadow-rs.rs index e1f4d1d..e538800 100644 --- a/src/bin/shadow-rs.rs +++ b/src/bin/shadow-rs.rs @@ -9,8 +9,15 @@ //! When invoked as `shadow-rs `, uses the first argument instead. use std::path::Path; +use std::process::ExitCode; -fn main() { +/// Convert a tool's `i32` exit code to `ExitCode`. +#[allow(clippy::cast_sign_loss)] // clamp(0, 255) guarantees non-negative +fn to_exit_code(code: i32) -> ExitCode { + ExitCode::from(code.clamp(0, 255) as u8) +} + +fn main() -> ExitCode { let args: Vec = std::env::args_os().collect(); let binary_name = args @@ -24,7 +31,7 @@ fn main() { // Direct invocation via symlink (e.g., argv[0] = "passwd") if let Some(code) = dispatch(&binary_name, &args) { - std::process::exit(code); + return to_exit_code(code); } // Multicall: `shadow-rs [args...]` @@ -33,21 +40,21 @@ fn main() { if util_name == "--list" { print_available_utils(); - std::process::exit(0); + return ExitCode::SUCCESS; } if let Some(code) = dispatch(&util_name, &args[1..]) { - std::process::exit(code); + return to_exit_code(code); } eprintln!("shadow-rs: unknown utility '{util_name}'"); eprintln!("Run 'shadow-rs --list' for available utilities."); - std::process::exit(1); + return ExitCode::FAILURE; } eprintln!("Usage: shadow-rs [arguments...]"); eprintln!("Run 'shadow-rs --list' for available utilities."); - std::process::exit(1); + ExitCode::FAILURE } fn dispatch(name: &str, args: &[std::ffi::OsString]) -> Option { diff --git a/src/shadow-core/src/atomic.rs b/src/shadow-core/src/atomic.rs index 221e3ad..4827ab2 100644 --- a/src/shadow-core/src/atomic.rs +++ b/src/shadow-core/src/atomic.rs @@ -24,12 +24,21 @@ use crate::error::ShadowError; /// On creation, sets the umask to zero so that file mode bits passed to /// `OpenOptions::mode()` are applied exactly. The original umask is restored /// when the guard is dropped, even on error or panic paths. -struct UmaskGuard(nix::sys::stat::Mode); +/// +/// # Thread safety +/// +/// `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 ()>); impl UmaskGuard { /// Set umask to zero and return a guard that restores the original. fn zero() -> Self { - Self(nix::sys::stat::umask(nix::sys::stat::Mode::empty())) + Self( + nix::sys::stat::umask(nix::sys::stat::Mode::empty()), + std::marker::PhantomData, + ) } } @@ -105,7 +114,21 @@ where .create_new(true) .mode(mode) .open(&tmp_path) - .map_err(|e| ShadowError::IoPath(e, tmp_path.clone()))?; + .or_else(|e| { + if e.kind() == io::ErrorKind::AlreadyExists { + // Stale tmp file from a crashed run — remove and retry once. + fs::remove_file(&tmp_path) + .map_err(|re| ShadowError::IoPath(re, tmp_path.clone()))?; + std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(mode) + .open(&tmp_path) + .map_err(|e2| ShadowError::IoPath(e2, tmp_path.clone())) + } else { + Err(ShadowError::IoPath(e, tmp_path.clone())) + } + })?; f(&mut tmp_file)?; diff --git a/src/shadow-core/src/audit.rs b/src/shadow-core/src/audit.rs index eed474e..13beb9c 100644 --- a/src/shadow-core/src/audit.rs +++ b/src/shadow-core/src/audit.rs @@ -11,6 +11,10 @@ //! //! This module provides a best-effort logging interface that silently //! succeeds when audit is not available. +//! +//! **Implementation note**: This module currently shells out to `logger` +//! rather than using native `libaudit` bindings. Native integration is +//! planned for a future release. /// Log a user account event to the audit subsystem. /// diff --git a/src/shadow-core/src/crypt.rs b/src/shadow-core/src/crypt.rs index 8ca717e..2e24147 100644 --- a/src/shadow-core/src/crypt.rs +++ b/src/shadow-core/src/crypt.rs @@ -22,6 +22,10 @@ unsafe extern "C" { /// crypt(3) salt alphabet (POSIX: [a-zA-Z0-9./]). const SALT_CHARS: &[u8] = b"./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; +// Modulo bias check: random u8 values (0-255) mapped via `% len` have zero +// bias only when `len` divides 256 evenly. Assert this at compile time. +const _: () = assert!(256 % SALT_CHARS.len() == 0); + /// Supported crypt(3) hash methods. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum CryptMethod { @@ -80,6 +84,12 @@ fn generate_salt(method: CryptMethod, rounds: Option) -> Result Result { let c_password = CString::new(password) .map_err(|_| ShadowError::Auth("password contains null byte".into()))?; diff --git a/src/shadow-core/src/hardening.rs b/src/shadow-core/src/hardening.rs index eaf7636..2dde351 100644 --- a/src/shadow-core/src/hardening.rs +++ b/src/shadow-core/src/hardening.rs @@ -110,3 +110,74 @@ pub fn harden_process() -> Vec<(String, String)> { raise_file_size_limit(); sanitized_env() } + +// --------------------------------------------------------------------------- +// Identity helpers +// --------------------------------------------------------------------------- + +/// Check whether the *real* caller is root (not just setuid-root). +/// +/// Uses `getuid()` (real UID). When a tool is installed setuid-root, +/// `geteuid()` is 0 for all callers, but the real UID identifies who +/// actually invoked the program. +pub fn caller_is_root() -> bool { + nix::unistd::getuid().is_root() +} + +/// Return the current user's username from the real UID. +pub fn current_username() -> Result { + let uid = nix::unistd::getuid(); + match nix::unistd::User::from_uid(uid) { + Ok(Some(user)) => Ok(user.name), + Ok(None) => Err(crate::error::ShadowError::Other( + format!("cannot determine username for uid {uid}").into(), + )), + Err(e) => Err(crate::error::ShadowError::Other( + format!("cannot determine username: {e}").into(), + )), + } +} + +// --------------------------------------------------------------------------- +// Signal blocking +// --------------------------------------------------------------------------- + +/// RAII guard that blocks critical signals during file modifications. +/// +/// Prevents `SIGINT`/`SIGTERM`/`SIGHUP` from interrupting a +/// lock-modify-write sequence, which could leave password files in an +/// inconsistent state or holding a stale lock. The original signal mask +/// is restored when the guard is dropped. +pub struct SignalBlocker { + old_mask: nix::sys::signal::SigSet, +} + +impl SignalBlocker { + /// Block `SIGINT`, `SIGTERM`, `SIGHUP` to prevent partial file writes. + pub fn block_critical() -> Result { + use nix::sys::signal::{SigSet, SigmaskHow, Signal}; + + let mut block_set = SigSet::empty(); + block_set.add(Signal::SIGINT); + block_set.add(Signal::SIGTERM); + block_set.add(Signal::SIGHUP); + + let mut old_mask = SigSet::empty(); + nix::sys::signal::sigprocmask(SigmaskHow::SIG_BLOCK, Some(&block_set), Some(&mut old_mask)) + .map_err(|e| { + crate::error::ShadowError::Other(format!("cannot block signals: {e}").into()) + })?; + + Ok(Self { old_mask }) + } +} + +impl Drop for SignalBlocker { + fn drop(&mut self) { + let _ = nix::sys::signal::sigprocmask( + nix::sys::signal::SigmaskHow::SIG_SETMASK, + Some(&self.old_mask), + None, + ); + } +} diff --git a/src/shadow-core/src/lock.rs b/src/shadow-core/src/lock.rs index b9f4f45..0140d30 100644 --- a/src/shadow-core/src/lock.rs +++ b/src/shadow-core/src/lock.rs @@ -192,6 +192,13 @@ fn write_pid_file(tmp_path: &Path) -> Result<(), ShadowError> { ShadowError::Lock(format!("cannot write {}: {e}", tmp_path.display()).into()) })?; + file.flush().map_err(|e| { + ShadowError::Lock(format!("cannot flush {}: {e}", tmp_path.display()).into()) + })?; + nix::unistd::fsync(&file).map_err(|e| { + ShadowError::Lock(format!("cannot fsync {}: {e}", tmp_path.display()).into()) + })?; + Ok(()) } diff --git a/src/shadow-core/src/pam.rs b/src/shadow-core/src/pam.rs index b0d21ee..e5eb255 100644 --- a/src/shadow-core/src/pam.rs +++ b/src/shadow-core/src/pam.rs @@ -371,7 +371,7 @@ fn prompt_for_input( /// to the real terminal even if stdin has been redirected. Uses /// `nix::sys::termios` to disable `ECHO` for password prompts and restores /// the original settings afterward (including on error, via a drop guard). -fn read_from_tty(message: &PamMessage, echo: bool) -> io::Result { +fn read_from_tty(message: &PamMessage, echo: bool) -> io::Result> { let mut tty = File::options().read(true).write(true).open("/dev/tty")?; // Show prompt. @@ -391,7 +391,7 @@ fn read_from_tty(message: &PamMessage, echo: bool) -> io::Result { // Read one line from the tty. let mut reader = io::BufReader::new(tty.try_clone()?); - let mut line = String::new(); + let mut line = zeroize::Zeroizing::new(String::new()); reader.read_line(&mut line)?; // Print a newline after hidden input so the cursor moves down. @@ -412,9 +412,9 @@ fn read_from_tty(message: &PamMessage, echo: bool) -> io::Result { } /// Read a line from stdin without prompting. -fn read_from_stdin() -> io::Result { +fn read_from_stdin() -> io::Result> { let stdin = io::stdin(); - let mut line = String::new(); + let mut line = zeroize::Zeroizing::new(String::new()); stdin.lock().read_line(&mut line)?; if line.ends_with('\n') { @@ -725,12 +725,13 @@ impl Drop for PamContext { } } -// `PamContext` holds a raw pointer but is safe to send between threads — the -// PAM handle is only accessed through `&mut self` methods, and the pointer is -// not shared. -// // SAFETY: The raw pointer `handle` is exclusively owned by `PamContext`. No // concurrent access is possible because all mutating methods require `&mut self`. +// +// Note: While sending the handle between threads is memory-safe, PAM module +// implementations are not guaranteed to be thread-safe. In practice, all +// shadow-rs tools are single-threaded, so this is not an issue. Do not use +// `PamContext` across threads without verifying the PAM modules in use. unsafe impl Send for PamContext {} // --------------------------------------------------------------------------- diff --git a/src/shadow-core/src/selinux.rs b/src/shadow-core/src/selinux.rs index 13dd66b..3d03ad2 100644 --- a/src/shadow-core/src/selinux.rs +++ b/src/shadow-core/src/selinux.rs @@ -10,6 +10,10 @@ //! contexts during atomic file replacement. //! //! Feature-gated behind `selinux`. When disabled, all operations are no-ops. +//! +//! **Implementation note**: This module currently shells out to `getfattr`, +//! `chcon`, and `restorecon` rather than using native `libselinux` bindings. +//! Native integration is planned for a future release. use std::path::Path; diff --git a/src/shadow-core/src/shadow.rs b/src/shadow-core/src/shadow.rs index 96eb1f5..afa75c0 100644 --- a/src/shadow-core/src/shadow.rs +++ b/src/shadow-core/src/shadow.rs @@ -68,8 +68,13 @@ impl ShadowEntry { /// Unlock the password by removing the leading `!`. /// - /// Returns `false` if the password is not locked or would become empty - /// after unlocking (GNU passwd refuses this — use `delete` instead). + /// Returns `false` if the password is not locked, would become empty + /// after unlocking, or is the system account marker `*` (which + /// cannot be unlocked — use `delete_password` + rehash instead). + /// + /// Note: `is_locked()` returns `true` for `*`, but `unlock()` returns + /// `false` because removing one character from `*` yields an empty + /// password, which GNU passwd refuses. pub fn unlock(&mut self) -> bool { if !self.is_locked() { return false; diff --git a/src/shadow-core/src/skel.rs b/src/shadow-core/src/skel.rs index c1ab57e..7239032 100644 --- a/src/shadow-core/src/skel.rs +++ b/src/shadow-core/src/skel.rs @@ -42,8 +42,7 @@ fn copy_dir_recursive(src: &Path, dst: &Path, uid: u32, gid: u32) -> Result<(), .map_err(|e| ShadowError::IoPath(e, src_path.clone()))?; if file_type.is_dir() { - std::fs::create_dir_all(&dst_path) - .map_err(|e| ShadowError::IoPath(e, dst_path.clone()))?; + std::fs::create_dir(&dst_path).map_err(|e| ShadowError::IoPath(e, dst_path.clone()))?; // Preserve the source directory's permissions. let src_perms = std::fs::metadata(&src_path) .map_err(|e| ShadowError::IoPath(e, src_path.clone()))? diff --git a/src/shadow-core/src/validate.rs b/src/shadow-core/src/validate.rs index f137098..05f7244 100644 --- a/src/shadow-core/src/validate.rs +++ b/src/shadow-core/src/validate.rs @@ -23,6 +23,10 @@ const MAX_USERNAME_LEN: usize = 32; /// - Must not end with a period (historically problematic) /// - Must not consist of only dots /// +/// **Deviation from GNU shadow-utils**: GNU allows `$` as the final +/// character (used by Samba machine accounts, e.g., `MACHINE$`). This +/// implementation intentionally rejects `$` for stricter validation. +/// /// # Errors /// /// Returns `ShadowError::Validation` if the username violates any rule. diff --git a/src/uu/chage/src/chage.rs b/src/uu/chage/src/chage.rs index b10df9a..635e96c 100644 --- a/src/uu/chage/src/chage.rs +++ b/src/uu/chage/src/chage.rs @@ -93,49 +93,6 @@ impl UError for ChageError { } } -// Hardening functions are now centralized in shadow_core::hardening. - -// --------------------------------------------------------------------------- -// Signal blocking during critical sections -// --------------------------------------------------------------------------- - -/// RAII guard that blocks signals during critical sections and restores on drop. -/// -/// Prevents SIGINT/SIGTERM/SIGHUP from interrupting a lock-modify-write -/// sequence, which could leave the shadow file in an inconsistent state -/// or holding a stale lock. -struct SignalBlocker { - old_mask: nix::sys::signal::SigSet, -} - -impl SignalBlocker { - /// Block `SIGINT`, `SIGTERM`, `SIGHUP` to prevent partial file writes. - fn block_critical() -> Result { - use nix::sys::signal::{SigSet, SigmaskHow, Signal}; - - let mut block_set = SigSet::empty(); - block_set.add(Signal::SIGINT); - block_set.add(Signal::SIGTERM); - block_set.add(Signal::SIGHUP); - - let mut old_mask = SigSet::empty(); - nix::sys::signal::sigprocmask(SigmaskHow::SIG_BLOCK, Some(&block_set), Some(&mut old_mask)) - .map_err(|e| ChageError::UnexpectedFailure(format!("cannot block signals: {e}")))?; - - Ok(Self { old_mask }) - } -} - -impl Drop for SignalBlocker { - fn drop(&mut self) { - let _ = nix::sys::signal::sigprocmask( - nix::sys::signal::SigmaskHow::SIG_SETMASK, - Some(&self.old_mask), - None, - ); - } -} - // --------------------------------------------------------------------------- // Date parsing // --------------------------------------------------------------------------- @@ -312,8 +269,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { if is_list { // -l mode: non-root can view own aging info. - if !caller_is_root() { - let current_user = get_current_username()?; + if !shadow_core::hardening::caller_is_root() { + let current_user = shadow_core::hardening::current_username() + .map_err(|e| ChageError::UnexpectedFailure(e.to_string()))?; if current_user != *login { return Err(ChageError::PermissionDenied("Permission denied.".into()).into()); } @@ -322,7 +280,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } // All modification flags require root. - if !caller_is_root() { + if !shadow_core::hardening::caller_is_root() { return Err(ChageError::PermissionDenied("Permission denied.".into()).into()); } @@ -537,35 +495,12 @@ fn compute_inactive_display( // Helpers // --------------------------------------------------------------------------- -/// Check if the *real* caller is root (not just setuid-root). -/// -/// Uses `getuid()` (real UID). When chage is installed setuid-root, -/// euid is 0 for all callers, but real UID identifies who actually -/// invoked the program. -fn caller_is_root() -> bool { - nix::unistd::getuid().is_root() -} - -/// Return the current user's username (from real UID). -fn get_current_username() -> Result { - let uid = nix::unistd::getuid(); - match nix::unistd::User::from_uid(uid) { - Ok(Some(user)) => Ok(user.name), - Ok(None) => Err(ChageError::UnexpectedFailure(format!( - "cannot determine current username for uid {uid}" - ))), - Err(e) => Err(ChageError::UnexpectedFailure(format!( - "cannot determine current username: {e}" - ))), - } -} - /// Perform `chroot(2)` into the specified directory. /// /// Must be root to call `chroot`. After `chroot`, chdir to `/` so the /// working directory is valid inside the new root. fn do_chroot(dir: &str) -> Result<(), ChageError> { - if !caller_is_root() { + if !shadow_core::hardening::caller_is_root() { return Err(ChageError::PermissionDenied( "only root may use --root".into(), )); @@ -595,7 +530,8 @@ where } // Block signals for the entire critical section (lock -> write -> unlock). - let _signals = SignalBlocker::block_critical()?; + let _signals = shadow_core::hardening::SignalBlocker::block_critical() + .map_err(|e| ChageError::UnexpectedFailure(e.to_string()))?; let shadow_path = root.shadow_path(); diff --git a/src/uu/chfn/src/chfn.rs b/src/uu/chfn/src/chfn.rs index 6e2054a..93429f3 100644 --- a/src/uu/chfn/src/chfn.rs +++ b/src/uu/chfn/src/chfn.rs @@ -104,71 +104,16 @@ impl Gecos { // Hardening functions are now centralized in shadow_core::hardening. -// --------------------------------------------------------------------------- -// Signal blocking -// --------------------------------------------------------------------------- - -/// RAII guard that blocks signals during critical sections. -struct SignalBlocker { - old_mask: nix::sys::signal::SigSet, -} - -impl SignalBlocker { - fn block_critical() -> Result { - use nix::sys::signal::{SigSet, SigmaskHow, Signal}; - - let mut block_set = SigSet::empty(); - block_set.add(Signal::SIGINT); - block_set.add(Signal::SIGTERM); - block_set.add(Signal::SIGHUP); - - let mut old_mask = SigSet::empty(); - nix::sys::signal::sigprocmask(SigmaskHow::SIG_BLOCK, Some(&block_set), Some(&mut old_mask)) - .map_err(|e| ChfnError::Error(format!("cannot block signals: {e}")))?; - - Ok(Self { old_mask }) - } -} - -impl Drop for SignalBlocker { - fn drop(&mut self) { - let _ = nix::sys::signal::sigprocmask( - nix::sys::signal::SigmaskHow::SIG_SETMASK, - Some(&self.old_mask), - None, - ); - } -} - // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- -/// Check if the *real* caller is root. -fn caller_is_root() -> bool { - nix::unistd::getuid().is_root() -} - -/// Return the current user's username (from real UID). -fn get_current_username() -> Result { - let uid = nix::unistd::getuid(); - match nix::unistd::User::from_uid(uid) { - Ok(Some(user)) => Ok(user.name), - Ok(None) => Err(ChfnError::Error(format!( - "cannot determine current username for uid {uid}" - ))), - Err(e) => Err(ChfnError::Error(format!( - "cannot determine current username: {e}" - ))), - } -} - /// Resolve the target username from args or current user. fn resolve_target_user(matches: &clap::ArgMatches) -> Result { if let Some(user) = matches.get_one::(options::USER) { return Ok(user.clone()); } - get_current_username() + shadow_core::hardening::current_username().map_err(|e| ChfnError::Error(e.to_string())) } /// Validate that a GECOS sub-field does not contain illegal characters. @@ -190,7 +135,7 @@ fn validate_gecos_field(value: &str, field_name: &str, allow_comma: bool) -> Res /// Perform `chroot(2)` into the specified directory. fn do_chroot(dir: &str) -> Result<(), ChfnError> { - if !caller_is_root() { + if !shadow_core::hardening::caller_is_root() { return Err(ChfnError::Error("only root may use --root".into())); } @@ -219,7 +164,8 @@ where let _ = nix::unistd::setuid(nix::unistd::Uid::from_raw(0)); } - let _signals = SignalBlocker::block_critical()?; + let _signals = shadow_core::hardening::SignalBlocker::block_critical() + .map_err(|e| ChfnError::Error(e.to_string()))?; let passwd_path = root.passwd_path(); @@ -301,8 +247,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let target_user = resolve_target_user(&matches)?; // Non-root users can only change their own info. - if !caller_is_root() { - let current_user = get_current_username()?; + if !shadow_core::hardening::caller_is_root() { + let current_user = shadow_core::hardening::current_username() + .map_err(|e| ChfnError::Error(e.to_string()))?; if current_user != target_user { return Err( ChfnError::Error("you may only change your own finger information".into()).into(), @@ -349,7 +296,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } // Non-root users may not set the "other" field (matches GNU behavior). - if !caller_is_root() && new_other.is_some() { + if !shadow_core::hardening::caller_is_root() && new_other.is_some() { return Err(ChfnError::Error("only root may change the 'other' field".into()).into()); } diff --git a/src/uu/chpasswd/src/chpasswd.rs b/src/uu/chpasswd/src/chpasswd.rs index be551b9..c8c0d37 100644 --- a/src/uu/chpasswd/src/chpasswd.rs +++ b/src/uu/chpasswd/src/chpasswd.rs @@ -77,45 +77,6 @@ impl UError for ChpasswdError { } } -// Hardening functions are now centralized in shadow_core::hardening. - -// --------------------------------------------------------------------------- -// Signal blocking during critical sections -// --------------------------------------------------------------------------- - -/// RAII guard that blocks signals during critical sections and restores on drop. -struct SignalBlocker { - old_mask: nix::sys::signal::SigSet, -} - -impl SignalBlocker { - /// Block `SIGINT`, `SIGTERM`, `SIGHUP` to prevent partial file writes. - fn block_critical() -> Result { - use nix::sys::signal::{SigSet, SigmaskHow, Signal}; - - let mut block_set = SigSet::empty(); - block_set.add(Signal::SIGINT); - block_set.add(Signal::SIGTERM); - block_set.add(Signal::SIGHUP); - - let mut old_mask = SigSet::empty(); - nix::sys::signal::sigprocmask(SigmaskHow::SIG_BLOCK, Some(&block_set), Some(&mut old_mask)) - .map_err(|e| ChpasswdError::UnexpectedFailure(format!("cannot block signals: {e}")))?; - - Ok(Self { old_mask }) - } -} - -impl Drop for SignalBlocker { - fn drop(&mut self) { - let _ = nix::sys::signal::sigprocmask( - nix::sys::signal::SigmaskHow::SIG_SETMASK, - Some(&self.old_mask), - None, - ); - } -} - // --------------------------------------------------------------------------- // Input parsing // --------------------------------------------------------------------------- @@ -224,7 +185,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let root = SysRoot::default(); // chpasswd always requires root. - if !caller_is_root() { + if !shadow_core::hardening::caller_is_root() { return Err(ChpasswdError::PermissionDenied("Permission denied.".into()).into()); } @@ -348,7 +309,8 @@ fn apply_password_changes( } // Block signals for the entire critical section. - let _signals = SignalBlocker::block_critical()?; + let _signals = shadow_core::hardening::SignalBlocker::block_critical() + .map_err(|e| ChpasswdError::UnexpectedFailure(e.to_string()))?; let shadow_path = root.shadow_path(); @@ -448,14 +410,9 @@ fn resolve_crypt_method( } } -/// Check if the *real* caller is root (not just setuid-root). -fn caller_is_root() -> bool { - nix::unistd::getuid().is_root() -} - /// Perform `chroot(2)` into the specified directory. fn do_chroot(dir: &str) -> Result<(), ChpasswdError> { - if !caller_is_root() { + if !shadow_core::hardening::caller_is_root() { return Err(ChpasswdError::PermissionDenied( "only root may use --root".into(), )); diff --git a/src/uu/chsh/src/chsh.rs b/src/uu/chsh/src/chsh.rs index 269c107..f03be1c 100644 --- a/src/uu/chsh/src/chsh.rs +++ b/src/uu/chsh/src/chsh.rs @@ -63,71 +63,19 @@ impl UError for ChshError { // Hardening functions are now centralized in shadow_core::hardening. -// --------------------------------------------------------------------------- -// Signal blocking -// --------------------------------------------------------------------------- - -struct SignalBlocker { - old_mask: nix::sys::signal::SigSet, -} - -impl SignalBlocker { - fn block_critical() -> Result { - use nix::sys::signal::{SigSet, SigmaskHow, Signal}; - - let mut block_set = SigSet::empty(); - block_set.add(Signal::SIGINT); - block_set.add(Signal::SIGTERM); - block_set.add(Signal::SIGHUP); - - let mut old_mask = SigSet::empty(); - nix::sys::signal::sigprocmask(SigmaskHow::SIG_BLOCK, Some(&block_set), Some(&mut old_mask)) - .map_err(|e| ChshError::Error(format!("cannot block signals: {e}")))?; - - Ok(Self { old_mask }) - } -} - -impl Drop for SignalBlocker { - fn drop(&mut self) { - let _ = nix::sys::signal::sigprocmask( - nix::sys::signal::SigmaskHow::SIG_SETMASK, - Some(&self.old_mask), - None, - ); - } -} - // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- -fn caller_is_root() -> bool { - nix::unistd::getuid().is_root() -} - -fn get_current_username() -> Result { - let uid = nix::unistd::getuid(); - match nix::unistd::User::from_uid(uid) { - Ok(Some(user)) => Ok(user.name), - Ok(None) => Err(ChshError::Error(format!( - "cannot determine current username for uid {uid}" - ))), - Err(e) => Err(ChshError::Error(format!( - "cannot determine current username: {e}" - ))), - } -} - fn resolve_target_user(matches: &clap::ArgMatches) -> Result { if let Some(user) = matches.get_one::(options::USER) { return Ok(user.clone()); } - get_current_username() + shadow_core::hardening::current_username().map_err(|e| ChshError::Error(e.to_string())) } fn do_chroot(dir: &str) -> Result<(), ChshError> { - if !caller_is_root() { + if !shadow_core::hardening::caller_is_root() { return Err(ChshError::Error("only root may use --root".into())); } @@ -191,7 +139,7 @@ fn validate_shell(shell: &str, shells_path: &Path) -> Result<(), ChshError> { } // Root can set any existing shell, bypassing the /etc/shells check. - if caller_is_root() { + if shadow_core::hardening::caller_is_root() { return Ok(()); } @@ -230,7 +178,8 @@ where let _ = nix::unistd::setuid(nix::unistd::Uid::from_raw(0)); } - let _signals = SignalBlocker::block_critical()?; + let _signals = shadow_core::hardening::SignalBlocker::block_critical() + .map_err(|e| ChshError::Error(e.to_string()))?; let passwd_path = root.passwd_path(); @@ -325,8 +274,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let target_user = resolve_target_user(&matches)?; // Non-root users can only change their own shell. - if !caller_is_root() { - let current_user = get_current_username()?; + if !shadow_core::hardening::caller_is_root() { + let current_user = shadow_core::hardening::current_username() + .map_err(|e| ChshError::Error(e.to_string()))?; if current_user != target_user { return Err(ChshError::Error("you may only change your own login shell".into()).into()); } diff --git a/src/uu/groupadd/src/groupadd.rs b/src/uu/groupadd/src/groupadd.rs index 8255b7e..d809fcf 100644 --- a/src/uu/groupadd/src/groupadd.rs +++ b/src/uu/groupadd/src/groupadd.rs @@ -89,11 +89,6 @@ impl UError for GroupaddError { // Hardening functions are now centralized in shadow_core::hardening. -/// Check whether the real UID is root. -fn caller_is_root() -> bool { - nix::unistd::getuid().is_root() -} - // --------------------------------------------------------------------------- // Entry point // --------------------------------------------------------------------------- @@ -113,7 +108,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } }; - if !caller_is_root() { + if !shadow_core::hardening::caller_is_root() { uucore::show_error!("Permission denied."); return Err(GroupaddError::AlreadyPrinted(1).into()); } @@ -188,6 +183,11 @@ fn do_groupadd(matches: &clap::ArgMatches) -> UResult<()> { &login_defs_overrides, )?; + // 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() + .map_err(|e| GroupaddError::CantUpdate(format!("cannot block signals: {e}")))?; + // Write to /etc/group. write_group_entry(&group_path, &group_name, gid)?; diff --git a/src/uu/groupdel/src/groupdel.rs b/src/uu/groupdel/src/groupdel.rs index dbe8c9b..be5e737 100644 --- a/src/uu/groupdel/src/groupdel.rs +++ b/src/uu/groupdel/src/groupdel.rs @@ -77,10 +77,6 @@ impl UError for GroupdelError { // Hardening functions are now centralized in shadow_core::hardening. -fn caller_is_root() -> bool { - nix::unistd::getuid().is_root() -} - // --------------------------------------------------------------------------- // Entry point // --------------------------------------------------------------------------- @@ -100,7 +96,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } }; - if !caller_is_root() { + if !shadow_core::hardening::caller_is_root() { uucore::show_error!("Permission denied."); return Err(GroupdelError::AlreadyPrinted(1).into()); } @@ -113,6 +109,11 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let root_dir = matches.get_one::(options::ROOT).map(Path::new); let root = SysRoot::new(prefix.or(root_dir)); + // 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() + .map_err(|e| GroupdelError::CantUpdate(format!("cannot block signals: {e}")))?; + // Read existing groups to find the target. let group_path = root.group_path(); let group_lock = FileLock::acquire(&group_path).map_err(|e| { diff --git a/src/uu/groupmod/src/groupmod.rs b/src/uu/groupmod/src/groupmod.rs index 08cb640..82b4f29 100644 --- a/src/uu/groupmod/src/groupmod.rs +++ b/src/uu/groupmod/src/groupmod.rs @@ -88,10 +88,6 @@ impl UError for GroupmodError { // Hardening functions are now centralized in shadow_core::hardening. -fn caller_is_root() -> bool { - nix::unistd::getuid().is_root() -} - // --------------------------------------------------------------------------- // Entry point // --------------------------------------------------------------------------- @@ -112,7 +108,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } }; - if !caller_is_root() { + if !shadow_core::hardening::caller_is_root() { uucore::show_error!("Permission denied."); return Err(GroupmodError::AlreadyPrinted(1).into()); } @@ -142,6 +138,11 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { }) .transpose()?; + // 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() + .map_err(|e| GroupmodError::CantUpdate(format!("cannot block signals: {e}")))?; + // Lock and read /etc/group. let group_path = root.group_path(); let group_lock = FileLock::acquire(&group_path).map_err(|e| { diff --git a/src/uu/grpck/src/grpck.rs b/src/uu/grpck/src/grpck.rs index 650e755..a9d0ad4 100644 --- a/src/uu/grpck/src/grpck.rs +++ b/src/uu/grpck/src/grpck.rs @@ -47,9 +47,6 @@ mod exit_codes { pub const CANT_LOCK: i32 = 4; /// Cannot update files. pub const CANT_UPDATE: i32 = 5; - /// Cannot sort files. - #[allow(dead_code)] - pub const CANT_SORT: i32 = 6; } #[derive(Debug)] @@ -58,8 +55,6 @@ enum GrpckError { CantOpen(String), CantLock(String), CantUpdate(String), - #[allow(dead_code)] - CantSort(String), } impl fmt::Display for GrpckError { @@ -68,8 +63,7 @@ impl fmt::Display for GrpckError { Self::BadEntry(msg) | Self::CantOpen(msg) | Self::CantLock(msg) - | Self::CantUpdate(msg) - | Self::CantSort(msg) => f.write_str(msg), + | Self::CantUpdate(msg) => f.write_str(msg), } } } @@ -83,7 +77,6 @@ impl UError for GrpckError { Self::CantOpen(_) => exit_codes::CANT_OPEN, Self::CantLock(_) => exit_codes::CANT_LOCK, Self::CantUpdate(_) => exit_codes::CANT_UPDATE, - Self::CantSort(_) => exit_codes::CANT_SORT, } } } @@ -344,15 +337,18 @@ fn sort_gshadow_by_group( gshadow_entries: &[GshadowEntry], ) -> Vec { let mut result = Vec::with_capacity(gshadow_entries.len()); - let gs_by_name: std::collections::HashMap<&str, &GshadowEntry> = gshadow_entries - .iter() - .map(|gs| (gs.name.as_str(), gs)) - .collect(); + let mut gs_by_name: std::collections::HashMap<&str, Vec<&GshadowEntry>> = + std::collections::HashMap::new(); + for gs in gshadow_entries { + gs_by_name.entry(gs.name.as_str()).or_default().push(gs); + } - // First, add entries in group-sorted order. + // Add entries in group-sorted order, preserving duplicates. for g in sorted_groups { - if let Some(&gs) = gs_by_name.get(g.name.as_str()) { - result.push(gs.clone()); + if let Some(entries) = gs_by_name.get(g.name.as_str()) { + for gs in entries { + result.push((*gs).clone()); + } } } diff --git a/src/uu/newgrp/src/newgrp.rs b/src/uu/newgrp/src/newgrp.rs index 0f76157..44fb24a 100644 --- a/src/uu/newgrp/src/newgrp.rs +++ b/src/uu/newgrp/src/newgrp.rs @@ -68,24 +68,6 @@ impl UError for NewgrpError { // Helpers // --------------------------------------------------------------------------- -fn caller_is_root() -> bool { - nix::unistd::getuid().is_root() -} - -/// Get the current user's username from the real UID. -fn get_current_username() -> Result { - let uid = nix::unistd::getuid(); - match nix::unistd::User::from_uid(uid) { - Ok(Some(user)) => Ok(user.name), - Ok(None) => Err(NewgrpError::Error(format!( - "cannot determine current username for uid {uid}" - ))), - Err(e) => Err(NewgrpError::Error(format!( - "cannot determine current username: {e}" - ))), - } -} - /// Get the current user's primary GID from the real UID. fn get_current_gid() -> Result { let uid = nix::unistd::getuid(); @@ -239,11 +221,7 @@ fn verify_password(password: &str, hash: &str) -> Result { #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { - shadow_core::hardening::suppress_core_dumps(); - // sanitized_env() returns a clean env without mutating the process. - // newgrp preserves the current process env because it needs $SHELL - // and $HOME for the new shell session. - let _clean_env = shadow_core::hardening::sanitized_env(); + let _clean_env = shadow_core::hardening::harden_process(); let matches = match uu_app().try_get_matches_from(args) { Ok(m) => m, @@ -257,7 +235,8 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { }; let root = SysRoot::default(); - let username = get_current_username()?; + let username = shadow_core::hardening::current_username() + .map_err(|e| NewgrpError::Error(e.to_string()))?; let user_gid = get_current_gid()?; let group_name = matches.get_one::(options::GROUP); @@ -278,7 +257,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // Check membership: if the user is not a member, they need the // group password. Root always gets in. - if !caller_is_root() && !is_member(&username, user_gid, gid, &group_entry.members) { + if !shadow_core::hardening::caller_is_root() + && !is_member(&username, user_gid, gid, &group_entry.members) + { // Check if the group has a password in /etc/gshadow. let gshadow_path = root.gshadow_path(); match group_has_password(&gshadow_path, gname) { @@ -308,6 +289,13 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { nix::unistd::setgid(gid) .map_err(|e| NewgrpError::Error(format!("cannot set group ID to {target_gid}: {e}")))?; + // Reset supplementary groups. POSIX requires newgrp to reinitialize + // the group list. Without this, the new shell inherits stale groups. + let username_cstr = std::ffi::CString::new(username.as_str()) + .map_err(|_| NewgrpError::Error("invalid username".into()))?; + nix::unistd::initgroups(&username_cstr, gid) + .map_err(|e| NewgrpError::Error(format!("cannot initialize groups: {e}")))?; + // Drop back to the real UID (in case we are setuid-root). let real_uid = nix::unistd::getuid(); if nix::unistd::geteuid() != real_uid { diff --git a/src/uu/passwd/src/passwd.rs b/src/uu/passwd/src/passwd.rs index 735d300..27b8b0f 100644 --- a/src/uu/passwd/src/passwd.rs +++ b/src/uu/passwd/src/passwd.rs @@ -185,11 +185,12 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let show_all = matches.get_flag(options::ALL); // Non-root users can only view their own status. - if !caller_is_root() { + if !shadow_core::hardening::caller_is_root() { if show_all { return Err(PasswdError::PermissionDenied("Permission denied.".into()).into()); } - let current_user = get_current_username()?; + let current_user = shadow_core::hardening::current_username() + .map_err(|e| PasswdError::UnexpectedFailure(e.to_string()))?; if current_user != target_user { return Err(PasswdError::PermissionDenied("Permission denied.".into()).into()); } @@ -215,7 +216,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // Admin operations (lock/unlock/delete/expire/aging) require the real // caller to be root. Non-root users can only change their own password // (the default PAM path below). - if (has_mutation || has_aging) && !caller_is_root() { + if (has_mutation || has_aging) && !shadow_core::hardening::caller_is_root() { return Err(PasswdError::PermissionDenied("Permission denied.".into()).into()); } @@ -268,8 +269,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // Prevent non-root from targeting other users (avoids timing-based // user enumeration through PAM auth failure timing). - if !caller_is_root() { - let current = get_current_username()?; + if !shadow_core::hardening::caller_is_root() { + let current = shadow_core::hardening::current_username() + .map_err(|e| PasswdError::UnexpectedFailure(e.to_string()))?; if current != target_user { return Err(PasswdError::PermissionDenied( "You may not view or modify password information for another user.".into(), @@ -541,7 +543,7 @@ fn cmd_pam_change(matches: &clap::ArgMatches, _target_user: &str) -> UResult<()> let _priv_drop = PrivDrop::drop_to(nix::unistd::getuid())?; // Non-root users changing their own password must authenticate first. - if !caller_is_root() { + if !shadow_core::hardening::caller_is_root() { if let Err(e) = pam.authenticate(0) { return Err(PasswdError::PamError(e.to_string()).into()); } @@ -605,36 +607,12 @@ fn resolve_target_user(matches: &clap::ArgMatches) -> Result bool { - nix::unistd::getuid().is_root() -} - -/// Return the current user's username (from real UID). -fn get_current_username() -> Result { - let uid = nix::unistd::getuid(); - match nix::unistd::User::from_uid(uid) { - Ok(Some(user)) => Ok(user.name), - Ok(None) => Err(PasswdError::UnexpectedFailure(format!( - "cannot determine current username for uid {uid}" - ))), - Err(e) => Err(PasswdError::UnexpectedFailure(format!( - "cannot determine current username: {e}" - ))), - } -} - /// Perform `chroot(2)` into the specified directory. /// /// Must be root to call `chroot`. After `chroot`, chdir to `/` so the /// working directory is valid inside the new root. fn do_chroot(dir: &str) -> Result<(), PasswdError> { - if !caller_is_root() { + if !shadow_core::hardening::caller_is_root() { return Err(PasswdError::PermissionDenied( "only root may use --root".into(), )); @@ -698,47 +676,6 @@ fn format_days_since_epoch(days: i64) -> String { format!("{y:04}-{m:02}-{d:02}") } -// --------------------------------------------------------------------------- -// Security hardening — signal blocking during critical file writes -// --------------------------------------------------------------------------- - -/// RAII guard that blocks signals during critical sections and restores on drop. -/// -/// Prevents SIGINT/SIGTERM/SIGHUP from interrupting a lock-modify-write -/// sequence, which could leave the shadow file in an inconsistent state -/// or holding a stale lock. -struct SignalBlocker { - old_mask: nix::sys::signal::SigSet, -} - -impl SignalBlocker { - /// Block `SIGINT`, `SIGTERM`, `SIGHUP` to prevent partial file writes. - fn block_critical() -> Result { - use nix::sys::signal::{SigSet, SigmaskHow, Signal}; - - let mut block_set = SigSet::empty(); - block_set.add(Signal::SIGINT); - block_set.add(Signal::SIGTERM); - block_set.add(Signal::SIGHUP); - - let mut old_mask = SigSet::empty(); - nix::sys::signal::sigprocmask(SigmaskHow::SIG_BLOCK, Some(&block_set), Some(&mut old_mask)) - .map_err(|e| PasswdError::UnexpectedFailure(format!("cannot block signals: {e}")))?; - - Ok(Self { old_mask }) - } -} - -impl Drop for SignalBlocker { - fn drop(&mut self) { - let _ = nix::sys::signal::sigprocmask( - nix::sys::signal::SigmaskHow::SIG_SETMASK, - Some(&self.old_mask), - None, - ); - } -} - /// Lock the shadow file, read entries, apply a mutation to one user's entry, /// write back atomically, invalidate nscd cache. fn mutate_shadow( @@ -759,7 +696,8 @@ where // Block signals for the entire critical section (lock → write → unlock). // The RAII guard restores the original signal mask when this function returns. - let _signals = SignalBlocker::block_critical()?; + let _signals = shadow_core::hardening::SignalBlocker::block_critical() + .map_err(|e| PasswdError::UnexpectedFailure(e.to_string()))?; let shadow_path = root.shadow_path(); @@ -1555,9 +1493,9 @@ mod tests { #[test] fn test_status_permission_denied_code_path() { // Verify the permission-denied code path is reachable by checking - // that the get_current_username helper works (it will return a + // that the current_username helper works (it will return a // username for the current uid). - let username = get_current_username(); + let username = shadow_core::hardening::current_username(); assert!(username.is_ok(), "should resolve current username"); } } diff --git a/src/uu/pwck/src/pwck.rs b/src/uu/pwck/src/pwck.rs index 5cc8942..40e5394 100644 --- a/src/uu/pwck/src/pwck.rs +++ b/src/uu/pwck/src/pwck.rs @@ -70,7 +70,7 @@ enum PwckError { /// Exit 5 -- cannot update files. CantUpdate(String), /// Exit 6 -- cannot sort files (sort logic errors only). - #[allow(dead_code)] + #[cfg_attr(not(test), allow(dead_code))] CantSort(String), } diff --git a/src/uu/useradd/src/useradd.rs b/src/uu/useradd/src/useradd.rs index 7ad154f..79970e1 100644 --- a/src/uu/useradd/src/useradd.rs +++ b/src/uu/useradd/src/useradd.rs @@ -170,11 +170,6 @@ struct UseraddOptions { // Hardening functions are now centralized in shadow_core::hardening. -/// Check whether the real UID is root. -fn caller_is_root() -> bool { - nix::unistd::getuid().is_root() -} - // --------------------------------------------------------------------------- // Date parsing // --------------------------------------------------------------------------- @@ -293,7 +288,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { }; // Only root can add users. - if !caller_is_root() { + if !shadow_core::hardening::caller_is_root() { uucore::show_error!("Permission denied."); return Err(UseraddError::AlreadyPrinted(1).into()); } @@ -480,7 +475,13 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { validate::validate_username(&opts.login) .map_err(|e| UseraddError::BadArgument(format!("{e}")))?; - // Step 2: Acquire locks BEFORE reading so concurrent useradd cannot + // 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() + .map_err(|e| UseraddError::CannotUpdatePasswd(format!("cannot block signals: {e}")))?; + + // Acquire locks BEFORE reading so concurrent useradd cannot // silently overwrite entries added between our read and write. let passwd_path = opts.root.passwd_path(); let passwd_lock = FileLock::acquire(&passwd_path) diff --git a/src/uu/userdel/src/userdel.rs b/src/uu/userdel/src/userdel.rs index 239f050..d78ab88 100644 --- a/src/uu/userdel/src/userdel.rs +++ b/src/uu/userdel/src/userdel.rs @@ -118,6 +118,11 @@ 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() + .map_err(|e| UserdelError::CantUpdatePasswd(format!("cannot block signals: {e}")))?; + // 1. Remove from /etc/passwd remove_entry_from_file::(&passwd_path, login, "passwd") .map_err(UserdelError::CantUpdatePasswd)?; @@ -423,6 +428,8 @@ mod tests { assert!(m.get_flag(options::FORCE)); } + // Duplicated from tests/common/mod.rs — unit tests inside the crate + // cannot import from the workspace-level tests directory. fn skip_unless_root() -> bool { !nix::unistd::geteuid().is_root() } diff --git a/src/uu/usermod/src/usermod.rs b/src/uu/usermod/src/usermod.rs index c9df1a2..b924445 100644 --- a/src/uu/usermod/src/usermod.rs +++ b/src/uu/usermod/src/usermod.rs @@ -102,6 +102,11 @@ 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() + .map_err(|e| UsermodError::CantUpdate(format!("cannot block signals: {e}")))?; + // Modify /etc/passwd. let passwd_path = root.passwd_path(); let lock = FileLock::acquire(&passwd_path) diff --git a/tests/by-util/test_chage.rs b/tests/by-util/test_chage.rs index 2ebe213..d1662ab 100644 --- a/tests/by-util/test_chage.rs +++ b/tests/by-util/test_chage.rs @@ -72,11 +72,14 @@ fn test_conflicting_list_and_lastday() { // --------------------------------------------------------------------------- // Root-only tests — exercise real operations via SysRoot prefix // --------------------------------------------------------------------------- - -// Note: these tests use SysRoot via the --root flag. Since chage uses -// SysRoot::default() (no --prefix support like passwd), we test by -// constructing the shadow file and calling chage's internal functions. -// For full integration, we'd need to chroot, which requires root. +// +// TODO(#integration): These tests directly manipulate shadow-core data +// structures instead of calling chage::uumain(). Full end-to-end integration +// via uumain() is not yet feasible because chage only supports --root (which +// performs a real chroot(2) and requires root), not --prefix (path-prefix +// without chroot). Once chage gains a --prefix flag, replace these tests with +// uumain() calls using run(&["chage", "--prefix", ..., "-m", "10", "testuser"]) +// with synthetic files. #[test] fn test_list_output() { diff --git a/tests/by-util/test_chfn.rs b/tests/by-util/test_chfn.rs index 901766f..65e52e5 100644 --- a/tests/by-util/test_chfn.rs +++ b/tests/by-util/test_chfn.rs @@ -29,12 +29,6 @@ fn setup_prefix(passwd_content: &str) -> tempfile::TempDir { dir } -/// Read the passwd file content back from a prefix dir. -#[allow(dead_code)] -fn read_passwd(dir: &tempfile::TempDir) -> String { - std::fs::read_to_string(dir.path().join("etc/passwd")).expect("failed to read passwd file") -} - // --------------------------------------------------------------------------- // Non-root tests // --------------------------------------------------------------------------- diff --git a/tests/by-util/test_chpasswd.rs b/tests/by-util/test_chpasswd.rs index 710e2d4..eb342aa 100644 --- a/tests/by-util/test_chpasswd.rs +++ b/tests/by-util/test_chpasswd.rs @@ -55,6 +55,14 @@ fn test_invalid_crypt_method_exits_error() { // --------------------------------------------------------------------------- // Root-only tests — exercise real operations via shadow-core // --------------------------------------------------------------------------- +// +// TODO(#integration): These tests directly manipulate shadow-core data +// structures instead of calling chpasswd::uumain(). Full end-to-end +// integration via uumain() is not yet feasible because chpasswd only supports +// --root (which performs a real chroot(2) and requires root), not --prefix +// (path-prefix without chroot). Once chpasswd gains a --prefix flag, replace +// these tests with uumain() calls using run(&["chpasswd", "--prefix", ..., +// "-e", ...]) with synthetic files. #[test] fn test_batch_password_update() {