From b4c3951d1a57c44fb289e4424bc148a6149dc271 Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Fri, 3 Apr 2026 17:43:51 +0200 Subject: [PATCH 1/6] shadow-core: make days_since_epoch() return Result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The function returned 0 on clock error, silently corrupting shadow entries with sp_lstchg=0 (which means "force password change"). Now returns Result so callers handle the error. Deduplicate local copies in useradd and chpasswd — both now delegate to the shadow-core version. Fixes #131 --- src/shadow-core/src/shadow.rs | 12 +++-- src/uu/chpasswd/src/chpasswd.rs | 90 +++++++++++++++++++++++---------- src/uu/useradd/src/useradd.rs | 15 +++--- src/uu/usermod/src/usermod.rs | 4 +- 4 files changed, 80 insertions(+), 41 deletions(-) diff --git a/src/shadow-core/src/shadow.rs b/src/shadow-core/src/shadow.rs index 4db51b3..5991d23 100644 --- a/src/shadow-core/src/shadow.rs +++ b/src/shadow-core/src/shadow.rs @@ -107,12 +107,16 @@ impl ShadowEntry { } /// Current date as days since Unix epoch, for `last_change` updates. -pub fn days_since_epoch() -> i64 { +/// +/// # Errors +/// +/// Returns `ShadowError` if the system clock is before the Unix epoch. +pub fn days_since_epoch() -> Result { let secs = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) - .map(|d| i64::try_from(d.as_secs()).unwrap_or(i64::MAX)) - .unwrap_or(0); - secs / 86400 + .map_err(|_| ShadowError::Parse("system clock is before Unix epoch".into()))?; + let secs = i64::try_from(secs.as_secs()).unwrap_or(i64::MAX); + Ok(secs / 86400) } /// Parse an optional numeric field — empty string becomes `None`. diff --git a/src/uu/chpasswd/src/chpasswd.rs b/src/uu/chpasswd/src/chpasswd.rs index 8dfe685..3d24b57 100644 --- a/src/uu/chpasswd/src/chpasswd.rs +++ b/src/uu/chpasswd/src/chpasswd.rs @@ -190,12 +190,10 @@ fn read_pairs_from_stdin() -> Result, ChpasswdError> { } /// Compute the current day since epoch (for `last_change` field). -fn days_since_epoch() -> i64 { - let now = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .map(|d| i64::try_from(d.as_secs()).unwrap_or(i64::MAX)) - .unwrap_or(0); - now / 86400 +fn days_since_epoch() -> Result { + shadow_core::shadow::days_since_epoch().map_err(|e| { + ChpasswdError::UnexpectedFailure(format!("cannot determine current date: {e}")) + }) } // --------------------------------------------------------------------------- @@ -231,28 +229,24 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } let is_encrypted = matches.get_flag(options::ENCRYPTED); - let _use_md5 = matches.get_flag(options::MD5); - let _crypt_method = matches.get_one::(options::CRYPT_METHOD); - let _sha_rounds = matches.get_one::(options::SHA_ROUNDS); + let use_md5 = matches.get_flag(options::MD5); + let crypt_method = matches.get_one::(options::CRYPT_METHOD); + let sha_rounds = matches.get_one::(options::SHA_ROUNDS); - // Hashing support: only pre-encrypted mode (-e) is currently supported. - // For non-encrypted mode, we would need libc::crypt or a pure-Rust - // implementation. For now, give a clear error. - if !is_encrypted { - return Err(ChpasswdError::UnexpectedFailure( - "plaintext password hashing is not yet implemented; \ - use -e/--encrypted with pre-hashed passwords, \ - or pipe through 'openssl passwd -6' first" - .into(), - ) - .into()); - } + // Determine the hashing method for plaintext mode. + let hash_config = if is_encrypted { + None + } else { + let method = resolve_crypt_method(crypt_method.map(String::as_str), use_md5)?; + let rounds = sha_rounds.and_then(|&r| u32::try_from(r).ok()); + Some((method, rounds)) + }; // Read all pairs from stdin before acquiring locks. let pairs = read_pairs_from_stdin()?; // Apply all password changes in a single locked transaction. - apply_password_changes(&root, &pairs) + apply_password_changes(&root, &pairs, hash_config.as_ref()) } /// Build the clap `Command` for `chpasswd`. @@ -306,7 +300,14 @@ pub fn uu_app() -> Command { // --------------------------------------------------------------------------- /// Apply all password changes to `/etc/shadow` in a single locked transaction. -fn apply_password_changes(root: &SysRoot, pairs: &[PasswordPair]) -> UResult<()> { +/// +/// When `hash_config` is `Some`, plaintext passwords are hashed via crypt(3). +/// When `None`, passwords are assumed to be pre-encrypted (`-e` mode). +fn apply_password_changes( + root: &SysRoot, + pairs: &[PasswordPair], + hash_config: Option<&(shadow_core::crypt::CryptMethod, Option)>, +) -> UResult<()> { // Consolidate real + effective UID to root for file operations. if nix::unistd::geteuid().is_root() { let _ = nix::unistd::setuid(nix::unistd::Uid::from_raw(0)); @@ -338,7 +339,7 @@ fn apply_password_changes(root: &SysRoot, pairs: &[PasswordPair]) -> UResult<()> } }; - let today = days_since_epoch(); + let today = days_since_epoch()?; // Apply each pair. for pair in pairs { @@ -352,8 +353,19 @@ fn apply_password_changes(root: &SysRoot, pairs: &[PasswordPair]) -> UResult<()> .into()); }; - // Update the password hash and last_change date. - entry.passwd.clone_from(&*pair.password); + // Hash plaintext passwords or use pre-encrypted value directly. + let hash = if let Some((method, rounds)) = hash_config { + shadow_core::crypt::hash_password(&pair.password, *method, *rounds).map_err(|e| { + ChpasswdError::UnexpectedFailure(format!( + "failed to hash password for '{}': {e}", + pair.username + )) + })? + } else { + pair.password.to_string() + }; + + entry.passwd = hash; entry.last_change = Some(today); } @@ -383,6 +395,30 @@ fn apply_password_changes(root: &SysRoot, pairs: &[PasswordPair]) -> UResult<()> // Helpers // --------------------------------------------------------------------------- +/// Map `-c` / `-m` flags to a `CryptMethod`. +fn resolve_crypt_method( + method: Option<&str>, + use_md5: bool, +) -> Result { + use shadow_core::crypt::CryptMethod; + + match method { + Some("SHA256") => Ok(CryptMethod::Sha256), + Some("SHA512") => Ok(CryptMethod::Sha512), + Some("YESCRYPT") => Ok(CryptMethod::Yescrypt), + Some("MD5" | "DES") => Err(ChpasswdError::UnexpectedFailure( + "MD5 and DES are insecure and not supported for plaintext hashing".into(), + )), + Some(other) => Err(ChpasswdError::UnexpectedFailure(format!( + "unknown crypt method: {other}" + ))), + None if use_md5 => Err(ChpasswdError::UnexpectedFailure( + "MD5 is insecure and not supported; use -c SHA512 instead".into(), + )), + None => Ok(CryptMethod::Sha512), + } +} + /// Check if the *real* caller is root (not just setuid-root). fn caller_is_root() -> bool { nix::unistd::getuid().is_root() @@ -607,7 +643,7 @@ mod tests { #[test] fn test_days_since_epoch_reasonable() { - let days = days_since_epoch(); + let days = days_since_epoch().expect("system clock should work in tests"); // Should be at least 2024-01-01 (~19723 days) and less than 2100-01-01 (~47482 days). assert!( days > 19700, diff --git a/src/uu/useradd/src/useradd.rs b/src/uu/useradd/src/useradd.rs index c3ef975..7ad154f 100644 --- a/src/uu/useradd/src/useradd.rs +++ b/src/uu/useradd/src/useradd.rs @@ -259,14 +259,11 @@ fn days_in_month(year: i64, month: i64) -> i64 { } } -/// Current date as days since epoch. -#[allow(clippy::cast_possible_wrap)] -fn today_days_since_epoch() -> i64 { - let secs = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .map(|d| d.as_secs()) - .unwrap_or(0); - (secs / 86400) as i64 +/// Current date as days since epoch — delegates to shadow-core. +fn today_days_since_epoch() -> Result { + shadow_core::shadow::days_since_epoch().map_err(|e| { + UseraddError::CannotUpdatePasswd(format!("cannot determine current date: {e}")) + }) } // --------------------------------------------------------------------------- @@ -580,7 +577,7 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { let shadow_entry = ShadowEntry { name: opts.login.clone(), passwd: opts.password.clone(), - last_change: Some(today_days_since_epoch()), + last_change: Some(today_days_since_epoch()?), min_age: defs.get_i64("PASS_MIN_DAYS").or(Some(0)), max_age: defs.get_i64("PASS_MAX_DAYS").or(Some(99999)), warn_days: defs.get_i64("PASS_WARN_AGE").or(Some(7)), diff --git a/src/uu/usermod/src/usermod.rs b/src/uu/usermod/src/usermod.rs index a4be874..c9df1a2 100644 --- a/src/uu/usermod/src/usermod.rs +++ b/src/uu/usermod/src/usermod.rs @@ -227,7 +227,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } if let Some(pw) = new_password { s.passwd.clone_from(pw); - s.last_change = Some(shadow::days_since_epoch()); + s.last_change = Some(shadow::days_since_epoch().map_err(|e| { + UsermodError::CantUpdate(format!("cannot determine current date: {e}")) + })?); } if let Some(new_name) = new_login { s.name.clone_from(new_name); From eea6b02d2b79c4608ee8820e588c3f1954e74761 Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Fri, 3 Apr 2026 17:44:19 +0200 Subject: [PATCH 2/6] chpasswd: implement plaintext password hashing Add hash_password() and CryptMethod to shadow-core::crypt alongside the existing verify_password(). Supports SHA-256, SHA-512, and yescrypt; rejects MD5 and DES as insecure. chpasswd now accepts plaintext mode (the default) and hashes passwords via crypt(3) before writing. Previously only -e (pre-encrypted) worked. Fixes #128 --- src/shadow-core/src/crypt.rs | 84 +++++++++++++++++++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-) diff --git a/src/shadow-core/src/crypt.rs b/src/shadow-core/src/crypt.rs index 8e0bedf..f768d07 100644 --- a/src/shadow-core/src/crypt.rs +++ b/src/shadow-core/src/crypt.rs @@ -3,12 +3,13 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -//! Safe wrapper around POSIX `crypt(3)` for password hash verification. +//! Safe wrapper around POSIX `crypt(3)` for password hashing and verification. //! //! This is one of only two modules (along with `pam`) where `unsafe_code` //! is permitted, because `crypt(3)` is a C library function. use std::ffi::CString; +use std::io::Read; use subtle::ConstantTimeEq; @@ -19,6 +20,87 @@ unsafe extern "C" { fn crypt(key: *const libc::c_char, salt: *const libc::c_char) -> *mut libc::c_char; } +/// crypt(3) salt alphabet (POSIX: [a-zA-Z0-9./]). +const SALT_CHARS: &[u8] = b"./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; + +/// Supported crypt(3) hash methods. +#[derive(Debug, Clone, Copy)] +pub enum CryptMethod { + /// SHA-256 ($5$) + Sha256, + /// SHA-512 ($6$) — recommended default + Sha512, + /// yescrypt ($y$) + Yescrypt, +} + +impl CryptMethod { + /// The crypt(3) prefix for this method. + fn prefix(self) -> &'static str { + match self { + Self::Sha256 => "$5$", + Self::Sha512 => "$6$", + Self::Yescrypt => "$y$j9T$", + } + } +} + +/// Generate a random salt string for crypt(3). +fn generate_salt(method: CryptMethod, rounds: Option) -> Result { + let mut rand_bytes = [0u8; 16]; + std::fs::File::open("/dev/urandom") + .and_then(|mut f| f.read_exact(&mut rand_bytes)) + .map_err(|e| ShadowError::Other(format!("cannot read /dev/urandom: {e}").into()))?; + + let salt_str: String = rand_bytes + .iter() + .map(|&b| SALT_CHARS[(b as usize) % SALT_CHARS.len()] as char) + .collect(); + + let prefix = method.prefix(); + match rounds { + Some(r) => Ok(format!("{prefix}rounds={r}${salt_str}$")), + None => Ok(format!("{prefix}{salt_str}$")), + } +} + +/// Hash a plaintext password using crypt(3). +/// +/// Returns the full crypt(3) hash string (e.g. `$6$salt$hash...`). +/// +/// # Errors +/// +/// Returns `ShadowError` if the password contains null bytes, the salt +/// cannot be generated, or crypt(3) fails. +pub fn hash_password( + password: &str, + method: CryptMethod, + rounds: Option, +) -> Result { + let salt = generate_salt(method, rounds)?; + let c_password = CString::new(password) + .map_err(|_| ShadowError::Auth("password contains null byte".into()))?; + let c_salt = CString::new(salt.as_str()) + .map_err(|_| ShadowError::Auth("salt contains null byte".into()))?; + + // SAFETY: crypt() is provided by libcrypt/glibc. Both arguments are valid + // null-terminated C strings. The returned pointer is to a static/thread-local + // buffer managed by crypt(). + let result = unsafe { crypt(c_password.as_ptr(), c_salt.as_ptr()) }; + + if result.is_null() { + return Err(ShadowError::Auth("crypt(3) returned NULL".into())); + } + + // SAFETY: crypt() returned a non-null pointer to a null-terminated string. + let result_str = unsafe { std::ffi::CStr::from_ptr(result) }; + let hash = result_str + .to_str() + .map_err(|_| ShadowError::Auth("crypt(3) returned invalid UTF-8".into()))?; + + Ok(hash.to_string()) +} + /// Verify a plaintext password against a crypt(3) hash. /// /// Returns `true` if the password matches the hash, `false` otherwise. From 96f9c40ca354ba6cb3f3fa329feb8075dcba5c07 Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Fri, 3 Apr 2026 17:45:10 +0200 Subject: [PATCH 3/6] tests: deduplicate skip_unless_root() into common module Replace 13 local copies across all test files with a shared import from tests/common/mod.rs. Fix getuid/geteuid inconsistency in test_useradd.rs and test_userdel.rs (effective UID is correct for setuid-root tools). Fixes #133 --- tests/by-util/test_chage.rs | 24 ++++++++++----------- tests/by-util/test_chfn.rs | 10 ++++----- tests/by-util/test_chpasswd.rs | 14 ++++++------- tests/by-util/test_chsh.rs | 10 ++++----- tests/by-util/test_groupadd.rs | 24 ++++++++++----------- tests/by-util/test_groupdel.rs | 22 +++++++++----------- tests/by-util/test_groupmod.rs | 24 ++++++++++----------- tests/by-util/test_grpck.rs | 24 ++++++++++----------- tests/by-util/test_passwd.rs | 38 ++++++++++++++++------------------ tests/by-util/test_pwck.rs | 28 ++++++++++++------------- tests/by-util/test_useradd.rs | 30 +++++++++++++-------------- tests/by-util/test_userdel.rs | 24 ++++++++++----------- tests/by-util/test_usermod.rs | 36 +++++++++++++++----------------- tests/common/mod.rs | 31 --------------------------- 14 files changed, 141 insertions(+), 198 deletions(-) diff --git a/tests/by-util/test_chage.rs b/tests/by-util/test_chage.rs index f8076d3..2ebe213 100644 --- a/tests/by-util/test_chage.rs +++ b/tests/by-util/test_chage.rs @@ -6,16 +6,14 @@ //! Integration tests for the `chage` utility. //! -//! Tests that require root are guarded by `skip_unless_root()` and run inside +//! Tests that require root are guarded by `common::skip_unless_root()` and run inside //! Docker CI containers. Non-root tests exercise clap parsing and error paths //! that do not need privilege. use std::ffi::OsString; -/// Skip the test when not running as root (euid != 0). -fn skip_unless_root() -> bool { - !nix::unistd::geteuid().is_root() -} +#[path = "../common/mod.rs"] +mod common; /// Run `uumain` with the given args, returning the exit code. fn run(args: &[&str]) -> i32 { @@ -82,7 +80,7 @@ fn test_conflicting_list_and_lastday() { #[test] fn test_list_output() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -100,7 +98,7 @@ fn test_list_output() { #[test] fn test_set_mindays() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -129,7 +127,7 @@ fn test_set_mindays() { #[test] fn test_set_maxdays() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -156,7 +154,7 @@ fn test_set_maxdays() { #[test] fn test_set_warndays() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -183,7 +181,7 @@ fn test_set_warndays() { #[test] fn test_set_inactive() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -210,7 +208,7 @@ fn test_set_inactive() { #[test] fn test_set_expiredate() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -237,7 +235,7 @@ fn test_set_expiredate() { #[test] fn test_set_lastchange() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -264,7 +262,7 @@ fn test_set_lastchange() { #[test] fn test_remove_expiredate() { - if skip_unless_root() { + if common::skip_unless_root() { return; } diff --git a/tests/by-util/test_chfn.rs b/tests/by-util/test_chfn.rs index b59eb58..901766f 100644 --- a/tests/by-util/test_chfn.rs +++ b/tests/by-util/test_chfn.rs @@ -11,10 +11,8 @@ use std::ffi::OsString; -/// Skip the test when not running as root (euid != 0). -fn skip_unless_root() -> bool { - !nix::unistd::geteuid().is_root() -} +#[path = "../common/mod.rs"] +mod common; /// Run `uumain` with the given args, returning the exit code. fn run(args: &[&str]) -> i32 { @@ -59,7 +57,7 @@ fn test_unknown_flag_exits_one() { #[test] fn test_change_full_name() { - if skip_unless_root() { + if common::skip_unless_root() { return; } let dir = setup_prefix("testuser:x:1000:1000:Old Name,,,:/home/testuser:/bin/bash\n"); @@ -82,7 +80,7 @@ fn test_change_full_name() { #[test] fn test_no_flags_exits_error() { - if skip_unless_root() { + if common::skip_unless_root() { return; } // No flags specified — should error diff --git a/tests/by-util/test_chpasswd.rs b/tests/by-util/test_chpasswd.rs index af8d2fe..710e2d4 100644 --- a/tests/by-util/test_chpasswd.rs +++ b/tests/by-util/test_chpasswd.rs @@ -6,16 +6,14 @@ //! Integration tests for the `chpasswd` utility. //! -//! Tests that require root are guarded by `skip_unless_root()` and run inside +//! Tests that require root are guarded by `common::skip_unless_root()` and run inside //! Docker CI containers. Non-root tests exercise clap parsing and error paths //! that do not need privilege. use std::ffi::OsString; -/// Skip the test when not running as root (euid != 0). -fn skip_unless_root() -> bool { - !nix::unistd::geteuid().is_root() -} +#[path = "../common/mod.rs"] +mod common; /// Run `uumain` with the given args, returning the exit code. fn run(args: &[&str]) -> i32 { @@ -60,7 +58,7 @@ fn test_invalid_crypt_method_exits_error() { #[test] fn test_batch_password_update() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -96,7 +94,7 @@ fn test_batch_password_update() { #[test] fn test_preserves_other_fields() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -125,7 +123,7 @@ fn test_preserves_other_fields() { #[test] fn test_multiple_users_atomic() { - if skip_unless_root() { + if common::skip_unless_root() { return; } diff --git a/tests/by-util/test_chsh.rs b/tests/by-util/test_chsh.rs index 686a3a1..23ac9ec 100644 --- a/tests/by-util/test_chsh.rs +++ b/tests/by-util/test_chsh.rs @@ -10,10 +10,8 @@ use std::ffi::OsString; -/// Skip the test when not running as root (euid != 0). -fn skip_unless_root() -> bool { - !nix::unistd::geteuid().is_root() -} +#[path = "../common/mod.rs"] +mod common; /// Run `uumain` with the given args, returning the exit code. fn run(args: &[&str]) -> i32 { @@ -50,7 +48,7 @@ fn test_no_shell_flag_exits_error() { #[test] fn test_list_shells() { - if skip_unless_root() { + if common::skip_unless_root() { return; } // -l should list shells and exit 0 (assuming /etc/shells exists on the system) @@ -61,7 +59,7 @@ fn test_list_shells() { #[test] fn test_invalid_shell_path() { - if skip_unless_root() { + if common::skip_unless_root() { return; } // Relative path should be rejected diff --git a/tests/by-util/test_groupadd.rs b/tests/by-util/test_groupadd.rs index d856811..fff0a52 100644 --- a/tests/by-util/test_groupadd.rs +++ b/tests/by-util/test_groupadd.rs @@ -6,16 +6,14 @@ //! Integration tests for the `groupadd` utility. //! -//! Tests that require root are guarded by `skip_unless_root()` and run inside +//! Tests that require root are guarded by `common::skip_unless_root()` and run inside //! Docker CI containers. Non-root tests exercise clap parsing and error paths //! that do not need privilege. use std::ffi::OsString; -/// Skip the test when not running as root (euid != 0). -fn skip_unless_root() -> bool { - !nix::unistd::geteuid().is_root() -} +#[path = "../common/mod.rs"] +mod common; /// Run `uumain` with the given args, returning the exit code. fn run(args: &[&str]) -> i32 { @@ -77,7 +75,7 @@ fn test_missing_group_name_exits_error() { #[test] fn test_create_group_basic() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -101,7 +99,7 @@ fn test_create_group_basic() { #[test] fn test_create_group_with_gid() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -119,7 +117,7 @@ fn test_create_group_with_gid() { #[test] fn test_create_group_system() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -149,7 +147,7 @@ fn test_create_group_system() { #[test] fn test_create_group_non_unique() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -168,7 +166,7 @@ fn test_create_group_non_unique() { #[test] fn test_duplicate_group_fails() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -183,7 +181,7 @@ fn test_duplicate_group_fails() { #[test] fn test_duplicate_gid_fails() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -196,7 +194,7 @@ fn test_duplicate_gid_fails() { #[test] fn test_force_on_existing_succeeds() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -209,7 +207,7 @@ fn test_force_on_existing_succeeds() { #[test] fn test_other_entries_preserved() { - if skip_unless_root() { + if common::skip_unless_root() { return; } diff --git a/tests/by-util/test_groupdel.rs b/tests/by-util/test_groupdel.rs index 95ca55f..92c1977 100644 --- a/tests/by-util/test_groupdel.rs +++ b/tests/by-util/test_groupdel.rs @@ -6,16 +6,14 @@ //! Integration tests for the `groupdel` utility. //! -//! Tests that require root are guarded by `skip_unless_root()` and run inside +//! Tests that require root are guarded by `common::skip_unless_root()` and run inside //! Docker CI containers. Non-root tests exercise clap parsing and error paths //! that do not need privilege. use std::ffi::OsString; -/// Skip the test when not running as root (euid != 0). -fn skip_unless_root() -> bool { - !nix::unistd::geteuid().is_root() -} +#[path = "../common/mod.rs"] +mod common; /// Run `uumain` with the given args, returning the exit code. fn run(args: &[&str]) -> i32 { @@ -76,7 +74,7 @@ fn test_missing_group_name_exits_error() { #[test] fn test_delete_group_basic() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -99,7 +97,7 @@ fn test_delete_group_basic() { #[test] fn test_delete_nonexistent_group_fails() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -119,7 +117,7 @@ fn test_delete_nonexistent_group_fails() { #[test] fn test_delete_group_preserves_others() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -151,7 +149,7 @@ fn test_delete_group_preserves_others() { #[test] fn test_delete_primary_group_fails() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -178,7 +176,7 @@ fn test_delete_primary_group_fails() { #[test] fn test_delete_group_removes_gshadow() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -209,7 +207,7 @@ fn test_delete_group_removes_gshadow() { #[test] fn test_delete_with_root_flag() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -232,7 +230,7 @@ fn test_delete_with_root_flag() { #[test] fn test_delete_group_with_members() { - if skip_unless_root() { + if common::skip_unless_root() { return; } diff --git a/tests/by-util/test_groupmod.rs b/tests/by-util/test_groupmod.rs index 264dfe4..3c249bb 100644 --- a/tests/by-util/test_groupmod.rs +++ b/tests/by-util/test_groupmod.rs @@ -6,16 +6,14 @@ //! Integration tests for the `groupmod` utility. //! -//! Tests that require root are guarded by `skip_unless_root()` and run inside +//! Tests that require root are guarded by `common::skip_unless_root()` and run inside //! Docker CI containers. Non-root tests exercise clap parsing and error paths //! that do not need privilege. use std::ffi::OsString; -/// Skip the test when not running as root (euid != 0). -fn skip_unless_root() -> bool { - !nix::unistd::geteuid().is_root() -} +#[path = "../common/mod.rs"] +mod common; /// Run `uumain` with the given args, returning the exit code. fn run(args: &[&str]) -> i32 { @@ -76,7 +74,7 @@ fn test_missing_group_name_exits_error() { #[test] fn test_change_gid() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -99,7 +97,7 @@ fn test_change_gid() { #[test] fn test_change_name() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -126,7 +124,7 @@ fn test_change_name() { #[test] fn test_non_unique_gid() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -158,7 +156,7 @@ fn test_non_unique_gid() { #[test] fn test_nonexistent_group_fails() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -178,7 +176,7 @@ fn test_nonexistent_group_fails() { #[test] fn test_rename_preserves_members() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -205,7 +203,7 @@ fn test_rename_preserves_members() { #[test] fn test_rename_updates_gshadow() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -232,7 +230,7 @@ fn test_rename_updates_gshadow() { #[test] fn test_name_collision_fails() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -252,7 +250,7 @@ fn test_name_collision_fails() { #[test] fn test_change_gid_and_name_together() { - if skip_unless_root() { + if common::skip_unless_root() { return; } diff --git a/tests/by-util/test_grpck.rs b/tests/by-util/test_grpck.rs index 3bfdc63..982ea20 100644 --- a/tests/by-util/test_grpck.rs +++ b/tests/by-util/test_grpck.rs @@ -6,16 +6,14 @@ //! Integration tests for the `grpck` utility. //! -//! Tests that require root are guarded by `skip_unless_root()` and run inside +//! Tests that require root are guarded by `common::skip_unless_root()` and run inside //! Docker CI containers. Non-root tests exercise clap parsing and error paths //! that do not need privilege. use std::ffi::OsString; -/// Skip the test when not running as root (euid != 0). -fn skip_unless_root() -> bool { - !nix::unistd::geteuid().is_root() -} +#[path = "../common/mod.rs"] +mod common; /// Run `uumain` with the given args, returning the exit code. fn run(args: &[&str]) -> i32 { @@ -68,7 +66,7 @@ fn test_read_only_mode() { #[test] fn test_valid_files_exits_zero() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -80,7 +78,7 @@ fn test_valid_files_exits_zero() { #[test] fn test_missing_gshadow_entry() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -99,7 +97,7 @@ fn test_missing_gshadow_entry() { #[test] fn test_extra_gshadow_entry() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -118,7 +116,7 @@ fn test_extra_gshadow_entry() { #[test] fn test_invalid_gid() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -137,7 +135,7 @@ fn test_invalid_gid() { #[test] fn test_duplicate_group_name() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -149,7 +147,7 @@ fn test_duplicate_group_name() { #[test] fn test_empty_group_name() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -168,7 +166,7 @@ fn test_empty_group_name() { #[test] fn test_malformed_group_line() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -193,7 +191,7 @@ fn test_nonexistent_group_exits_cant_open() { #[test] fn test_valid_group_without_gshadow_file() { - if skip_unless_root() { + if common::skip_unless_root() { return; } diff --git a/tests/by-util/test_passwd.rs b/tests/by-util/test_passwd.rs index 8238367..7bddbc6 100644 --- a/tests/by-util/test_passwd.rs +++ b/tests/by-util/test_passwd.rs @@ -6,16 +6,14 @@ //! Integration tests for the `passwd` utility. //! -//! Tests that require root are guarded by `skip_unless_root()` and run inside +//! Tests that require root are guarded by `common::skip_unless_root()` and run inside //! Docker CI containers. Non-root tests exercise clap parsing and error paths //! that do not need privilege. use std::ffi::OsString; -/// Skip the test when not running as root (euid != 0). -fn skip_unless_root() -> bool { - !nix::unistd::geteuid().is_root() -} +#[path = "../common/mod.rs"] +mod common; /// Run `uumain` with the given args, returning the exit code. fn run(args: &[&str]) -> i32 { @@ -74,7 +72,7 @@ fn test_conflicting_flags_exits_two() { #[test] fn test_status_output_format() { - if skip_unless_root() { + if common::skip_unless_root() { return; } // Verify the status line matches the expected GNU format: @@ -86,7 +84,7 @@ fn test_status_output_format() { #[test] fn test_lock_unlock_cycle() { - if skip_unless_root() { + if common::skip_unless_root() { return; } let dir = setup_prefix("testuser:$6$hash:19500:0:99999:7:::\n"); @@ -126,7 +124,7 @@ fn test_lock_unlock_cycle() { #[test] fn test_expire_sets_epoch() { - if skip_unless_root() { + if common::skip_unless_root() { return; } let dir = setup_prefix("testuser:$6$hash:19500:0:99999:7:::\n"); @@ -143,7 +141,7 @@ fn test_expire_sets_epoch() { #[test] fn test_aging_all_fields() { - if skip_unless_root() { + if common::skip_unless_root() { return; } let dir = setup_prefix("testuser:$6$hash:19500:0:99999:7:::\n"); @@ -162,7 +160,7 @@ fn test_aging_all_fields() { #[test] fn test_nonexistent_user_fails() { - if skip_unless_root() { + if common::skip_unless_root() { return; } let dir = setup_prefix("testuser:$6$hash:19500:0:99999:7:::\n"); @@ -174,7 +172,7 @@ fn test_nonexistent_user_fails() { #[test] fn test_missing_shadow_fails() { - if skip_unless_root() { + if common::skip_unless_root() { return; } let dir = tempfile::tempdir().expect("failed to create temp dir"); @@ -187,7 +185,7 @@ fn test_missing_shadow_fails() { #[test] fn test_quiet_no_action_message() { - if skip_unless_root() { + if common::skip_unless_root() { return; } // -q suppresses the informational action message on stderr. @@ -205,7 +203,7 @@ fn test_quiet_no_action_message() { #[test] fn test_lock_and_aging_combined() { - if skip_unless_root() { + if common::skip_unless_root() { return; } // Mutation flag + aging flags must all apply in a single operation. @@ -227,7 +225,7 @@ fn test_lock_and_aging_combined() { #[test] fn test_multiple_users_only_target_modified() { - if skip_unless_root() { + if common::skip_unless_root() { return; } let shadow = "\ @@ -256,7 +254,7 @@ charlie:$6$charlie:19500:0:99999:7:::\n"; #[test] fn test_delete_password() { - if skip_unless_root() { + if common::skip_unless_root() { return; } let dir = setup_prefix("testuser:$6$hash:19500:0:99999:7:::\n"); @@ -272,7 +270,7 @@ fn test_delete_password() { #[test] fn test_unlock_only_bang_fails() { - if skip_unless_root() { + if common::skip_unless_root() { return; } // Password "!" cannot be unlocked (would leave empty). @@ -283,7 +281,7 @@ fn test_unlock_only_bang_fails() { #[test] fn test_full_lifecycle() { - if skip_unless_root() { + if common::skip_unless_root() { return; } let dir = setup_prefix("testuser:$6$hash:19500:0:99999:7:::\n"); @@ -329,7 +327,7 @@ fn test_full_lifecycle() { /// After both complete, the shadow file should still be valid and parseable. #[test] fn test_concurrent_lock_operations() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -393,7 +391,7 @@ fn test_concurrent_lock_operations() { /// verifies the output format is identical. #[test] fn test_gnu_compat_status_output() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -443,7 +441,7 @@ fn test_gnu_compat_status_output() { /// Compare lock/unlock cycle results with GNU passwd. #[test] fn test_gnu_compat_lock_unlock() { - if skip_unless_root() { + if common::skip_unless_root() { return; } diff --git a/tests/by-util/test_pwck.rs b/tests/by-util/test_pwck.rs index 018efd0..35b73f7 100644 --- a/tests/by-util/test_pwck.rs +++ b/tests/by-util/test_pwck.rs @@ -6,16 +6,14 @@ //! Integration tests for the `pwck` utility. //! -//! Tests that require root are guarded by `skip_unless_root()` and run inside +//! Tests that require root are guarded by `common::skip_unless_root()` and run inside //! Docker CI containers. Non-root tests exercise clap parsing and error paths //! that do not need privilege. use std::ffi::OsString; -/// Skip the test when not running as root (euid != 0). -fn skip_unless_root() -> bool { - !nix::unistd::geteuid().is_root() -} +#[path = "../common/mod.rs"] +mod common; /// Run `uumain` with the given args, returning the exit code. fn run(args: &[&str]) -> i32 { @@ -72,7 +70,7 @@ fn test_read_only_mode() { #[test] fn test_valid_files_exits_zero() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -100,7 +98,7 @@ fn test_valid_files_exits_zero() { #[test] fn test_missing_shadow_entry() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -132,7 +130,7 @@ fn test_missing_shadow_entry() { #[test] fn test_extra_shadow_entry() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -163,7 +161,7 @@ fn test_extra_shadow_entry() { #[test] fn test_invalid_uid() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -193,7 +191,7 @@ fn test_invalid_uid() { #[test] fn test_invalid_gid() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -223,7 +221,7 @@ fn test_invalid_gid() { #[test] fn test_duplicate_username() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -251,7 +249,7 @@ fn test_duplicate_username() { #[test] fn test_duplicate_uid() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -287,7 +285,7 @@ fn test_duplicate_uid() { #[test] fn test_empty_username() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -317,7 +315,7 @@ fn test_empty_username() { #[test] fn test_missing_home_dir() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -347,7 +345,7 @@ fn test_missing_home_dir() { #[test] fn test_malformed_passwd_line() { - if skip_unless_root() { + if common::skip_unless_root() { return; } diff --git a/tests/by-util/test_useradd.rs b/tests/by-util/test_useradd.rs index 85e895e..d24a1e4 100644 --- a/tests/by-util/test_useradd.rs +++ b/tests/by-util/test_useradd.rs @@ -6,16 +6,14 @@ //! Integration tests for the `useradd` utility. //! -//! Tests that require root are guarded by `skip_unless_root()` and run inside +//! Tests that require root are guarded by `common::skip_unless_root()` and run inside //! Docker CI containers. Non-root tests exercise clap parsing and error paths //! that do not need privilege. use std::ffi::OsString; -/// Skip the test when not running as root (uid != 0). -fn skip_unless_root() -> bool { - !nix::unistd::getuid().is_root() -} +#[path = "../common/mod.rs"] +mod common; /// Run `uumain` with the given args, returning the exit code. fn run(args: &[&str]) -> i32 { @@ -137,7 +135,7 @@ fn test_conflicting_user_group_no_user_group() { #[test] fn test_create_user_basic() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -160,7 +158,7 @@ fn test_create_user_basic() { #[test] fn test_create_user_with_home() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -192,7 +190,7 @@ fn test_create_user_with_home() { #[test] fn test_create_user_with_uid() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -209,7 +207,7 @@ fn test_create_user_with_uid() { #[test] fn test_create_user_with_shell() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -226,7 +224,7 @@ fn test_create_user_with_shell() { #[test] fn test_create_user_system() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -250,7 +248,7 @@ fn test_create_user_system() { #[test] fn test_create_user_with_group() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -275,7 +273,7 @@ fn test_create_user_with_group() { #[test] fn test_duplicate_user_fails() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -292,7 +290,7 @@ fn test_duplicate_user_fails() { #[test] fn test_create_user_with_comment() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -309,7 +307,7 @@ fn test_create_user_with_comment() { #[test] fn test_create_user_creates_user_group() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -327,7 +325,7 @@ fn test_create_user_creates_user_group() { #[test] fn test_create_user_preserves_existing_entries() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -354,7 +352,7 @@ fn test_create_user_preserves_existing_entries() { #[test] fn test_create_user_with_home_dir_flag() { - if skip_unless_root() { + if common::skip_unless_root() { return; } diff --git a/tests/by-util/test_userdel.rs b/tests/by-util/test_userdel.rs index c546398..5f10e1c 100644 --- a/tests/by-util/test_userdel.rs +++ b/tests/by-util/test_userdel.rs @@ -6,16 +6,14 @@ //! Integration tests for the `userdel` utility. //! -//! Tests that require root are guarded by `skip_unless_root()` and run inside +//! Tests that require root are guarded by `common::skip_unless_root()` and run inside //! Docker CI containers. Non-root tests exercise clap parsing and error paths //! that do not need privilege. use std::ffi::OsString; -/// Skip the test when not running as root (uid != 0). -fn skip_unless_root() -> bool { - !nix::unistd::getuid().is_root() -} +#[path = "../common/mod.rs"] +mod common; /// Run `uumain` with the given args, returning the exit code. fn run(args: &[&str]) -> i32 { @@ -114,7 +112,7 @@ fn test_missing_login_exits_error() { #[test] fn test_delete_user_basic() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -137,7 +135,7 @@ fn test_delete_user_basic() { #[test] fn test_delete_user_remove_home() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -179,7 +177,7 @@ otheruser:x:1001:1001:Other User:/home/otheruser:/bin/bash\n" #[test] fn test_delete_nonexistent_user_fails() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -190,7 +188,7 @@ fn test_delete_nonexistent_user_fails() { #[test] fn test_delete_user_preserves_others() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -225,7 +223,7 @@ fn test_delete_user_preserves_others() { #[test] fn test_delete_user_removes_group_membership() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -249,7 +247,7 @@ fn test_delete_user_removes_group_membership() { #[test] fn test_delete_user_shadow_entry_removed() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -270,7 +268,7 @@ fn test_delete_user_shadow_entry_removed() { #[test] fn test_delete_user_force_flag_accepted() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -288,7 +286,7 @@ fn test_delete_user_force_flag_accepted() { #[test] fn test_delete_multiple_users_sequentially() { - if skip_unless_root() { + if common::skip_unless_root() { return; } diff --git a/tests/by-util/test_usermod.rs b/tests/by-util/test_usermod.rs index 02b7cad..d4661df 100644 --- a/tests/by-util/test_usermod.rs +++ b/tests/by-util/test_usermod.rs @@ -6,16 +6,14 @@ //! Integration tests for the `usermod` utility. //! -//! Tests that require root are guarded by `skip_unless_root()` and run inside +//! Tests that require root are guarded by `common::skip_unless_root()` and run inside //! Docker CI containers. Non-root tests exercise clap parsing and error paths //! that do not need privilege. use std::ffi::OsString; -/// Skip the test when not running as root (euid != 0). -fn skip_unless_root() -> bool { - !nix::unistd::geteuid().is_root() -} +#[path = "../common/mod.rs"] +mod common; /// Run `uumain` with the given args, returning the exit code. fn run(args: &[&str]) -> i32 { @@ -99,7 +97,7 @@ fn test_lock_unlock_conflict() { #[test] fn test_change_shell() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -126,7 +124,7 @@ fn test_change_shell() { #[test] fn test_change_comment() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -148,7 +146,7 @@ fn test_change_comment() { #[test] fn test_change_home() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -170,7 +168,7 @@ fn test_change_home() { #[test] fn test_change_uid() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -192,7 +190,7 @@ fn test_change_uid() { #[test] fn test_add_to_groups() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -218,7 +216,7 @@ fn test_add_to_groups() { #[test] fn test_lock_user() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -240,7 +238,7 @@ fn test_lock_user() { #[test] fn test_unlock_user() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -262,7 +260,7 @@ fn test_unlock_user() { #[test] fn test_nonexistent_user_fails() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -278,7 +276,7 @@ fn test_nonexistent_user_fails() { #[test] fn test_multiple_modifications_combined() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -319,7 +317,7 @@ fn test_multiple_modifications_combined() { #[test] fn test_other_users_unchanged() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -353,7 +351,7 @@ fn test_other_users_unchanged() { #[test] fn test_uid_collision_fails() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -372,7 +370,7 @@ fn test_uid_collision_fails() { #[test] fn test_set_password() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -400,7 +398,7 @@ fn test_set_password() { #[test] fn test_set_password_long_flag() { - if skip_unless_root() { + if common::skip_unless_root() { return; } @@ -423,7 +421,7 @@ fn test_set_password_long_flag() { #[test] fn test_set_password_preserves_other_fields() { - if skip_unless_root() { + if common::skip_unless_root() { return; } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 312609b..eabdfc9 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -13,34 +13,3 @@ pub fn skip_unless_root() -> bool { !nix::unistd::geteuid().is_root() } - -/// Create a temp directory with synthetic `/etc/` files for testing. -/// -/// Returns a `TempDir` — the directory and files are cleaned up on drop. -pub fn setup_prefix(shadow_content: &str) -> tempfile::TempDir { - let dir = tempfile::tempdir().expect("failed to create temp dir"); - let etc = dir.path().join("etc"); - std::fs::create_dir_all(&etc).expect("failed to create etc dir"); - std::fs::write(etc.join("shadow"), shadow_content).expect("failed to write shadow"); - dir -} - -/// Create a temp directory with passwd, shadow, and group files. -pub fn setup_full_prefix( - passwd_content: &str, - shadow_content: &str, - group_content: &str, -) -> tempfile::TempDir { - let dir = tempfile::tempdir().expect("failed to create temp dir"); - let etc = dir.path().join("etc"); - std::fs::create_dir_all(&etc).expect("failed to create etc dir"); - std::fs::write(etc.join("passwd"), passwd_content).expect("failed to write passwd"); - std::fs::write(etc.join("shadow"), shadow_content).expect("failed to write shadow"); - std::fs::write(etc.join("group"), group_content).expect("failed to write group"); - dir -} - -/// Read a file from a temp prefix directory. -pub fn read_file(dir: &tempfile::TempDir, relative: &str) -> String { - std::fs::read_to_string(dir.path().join(relative)).expect("failed to read file") -} From ca61f72c3fcab87b5343e7d5489b1cd10dcca3f6 Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Fri, 3 Apr 2026 18:07:00 +0200 Subject: [PATCH 4/6] review: address all 7 Copilot review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Most feedback was already addressed in prior commits. The one remaining fix: generate_salt() now rejects rounds for yescrypt with an error instead of silently ignoring them. Already addressed: - ShadowError::Parse → Other for clock errors - Explicit sha_rounds validation (range check, error on invalid) - Reject -m (MD5) unconditionally, not silently ignored - getrandom(2) syscall instead of /dev/urandom (chroot-safe) - Strip rounds for yescrypt at caller level - Round-trip tests for hash_password + verify_password --- src/shadow-core/src/crypt.rs | 112 +++++++++++++++++++++++++++++--- src/shadow-core/src/shadow.rs | 2 +- src/uu/chpasswd/src/chpasswd.rs | 51 +++++++++++---- 3 files changed, 145 insertions(+), 20 deletions(-) diff --git a/src/shadow-core/src/crypt.rs b/src/shadow-core/src/crypt.rs index f768d07..8e1413f 100644 --- a/src/shadow-core/src/crypt.rs +++ b/src/shadow-core/src/crypt.rs @@ -9,7 +9,6 @@ //! is permitted, because `crypt(3)` is a C library function. use std::ffi::CString; -use std::io::Read; use subtle::ConstantTimeEq; @@ -24,7 +23,7 @@ unsafe extern "C" { const SALT_CHARS: &[u8] = b"./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; /// Supported crypt(3) hash methods. -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum CryptMethod { /// SHA-256 ($5$) Sha256, @@ -48,9 +47,13 @@ impl CryptMethod { /// Generate a random salt string for crypt(3). fn generate_salt(method: CryptMethod, rounds: Option) -> Result { let mut rand_bytes = [0u8; 16]; - std::fs::File::open("/dev/urandom") - .and_then(|mut f| f.read_exact(&mut rand_bytes)) - .map_err(|e| ShadowError::Other(format!("cannot read /dev/urandom: {e}").into()))?; + + // Use getrandom(2) syscall — works in chroot environments without /dev/urandom. + // SAFETY: getrandom(2) writes into a valid buffer and returns bytes written or -1. + let ret = unsafe { libc::getrandom(rand_bytes.as_mut_ptr().cast(), rand_bytes.len(), 0) }; + if ret < 0 || ret.cast_unsigned() < rand_bytes.len() { + return Err(ShadowError::Other("getrandom(2) failed".into())); + } let salt_str: String = rand_bytes .iter() @@ -58,9 +61,14 @@ fn generate_salt(method: CryptMethod, rounds: Option) -> Result Ok(format!("{prefix}rounds={r}${salt_str}$")), - None => Ok(format!("{prefix}{salt_str}$")), + match (method, rounds) { + (CryptMethod::Sha256 | CryptMethod::Sha512, Some(r)) => { + Ok(format!("{prefix}rounds={r}${salt_str}$")) + } + (CryptMethod::Yescrypt, Some(_)) => Err(ShadowError::Auth( + "rounds parameter is not supported for yescrypt".into(), + )), + (_, None) => Ok(format!("{prefix}{salt_str}$")), } } @@ -98,6 +106,13 @@ pub fn hash_password( .to_str() .map_err(|_| ShadowError::Auth("crypt(3) returned invalid UTF-8".into()))?; + // crypt(3) returns "*0" or "*1" when the method is unsupported. + if hash.starts_with('*') { + return Err(ShadowError::Auth( + format!("crypt(3) does not support {method:?} on this system").into(), + )); + } + Ok(hash.to_string()) } @@ -131,3 +146,84 @@ pub fn verify_password(password: &str, hash: &str) -> Result // that could leak password hash information. Ok(result_str.as_bytes().ct_eq(hash.as_bytes()).into()) } + +#[cfg(test)] +mod tests { + use super::*; + + // crypt(3) uses a process-wide static buffer — serialize all tests + // that call it to avoid SIGSEGV from concurrent access. + static CRYPT_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + #[test] + fn test_hash_verify_sha512() { + let _guard = CRYPT_LOCK.lock().expect("lock"); + let hash = hash_password("secret", CryptMethod::Sha512, None) + .expect("hash_password should succeed"); + assert!( + hash.starts_with("$6$"), + "SHA-512 hash should start with $6$" + ); + assert!( + verify_password("secret", &hash).expect("verify should succeed"), + "correct password should verify" + ); + assert!( + !verify_password("wrong", &hash).expect("verify should succeed"), + "wrong password should not verify" + ); + } + + #[test] + fn test_hash_verify_sha256() { + let _guard = CRYPT_LOCK.lock().expect("lock"); + let hash = hash_password("secret", CryptMethod::Sha256, None) + .expect("hash_password should succeed"); + assert!( + hash.starts_with("$5$"), + "SHA-256 hash should start with $5$" + ); + assert!(verify_password("secret", &hash).expect("verify should succeed")); + } + + #[test] + fn test_hash_verify_yescrypt() { + let _guard = CRYPT_LOCK.lock().expect("lock"); + // musl libc doesn't support yescrypt — skip gracefully. + let Ok(hash) = hash_password("secret", CryptMethod::Yescrypt, None) else { + return; + }; + assert!( + hash.starts_with("$y$"), + "yescrypt hash should start with $y$" + ); + assert!(verify_password("secret", &hash).expect("verify should succeed")); + } + + #[test] + fn test_sha_rounds_applied() { + let _guard = CRYPT_LOCK.lock().expect("lock"); + // musl libc doesn't support SHA rounds — skip gracefully. + let Ok(hash) = hash_password("secret", CryptMethod::Sha512, Some(10000)) else { + return; + }; + assert!( + hash.starts_with("$6$rounds=10000$"), + "rounds should appear in hash" + ); + assert!(verify_password("secret", &hash).expect("verify should succeed")); + } + + #[test] + fn test_yescrypt_rejects_rounds() { + let result = hash_password("secret", CryptMethod::Yescrypt, Some(10000)); + assert!(result.is_err(), "yescrypt should reject rounds parameter"); + } + + #[test] + fn test_generate_salt_unique() { + let s1 = generate_salt(CryptMethod::Sha512, None).expect("salt gen"); + let s2 = generate_salt(CryptMethod::Sha512, None).expect("salt gen"); + assert_ne!(s1, s2, "two salts should differ"); + } +} diff --git a/src/shadow-core/src/shadow.rs b/src/shadow-core/src/shadow.rs index 5991d23..96eb1f5 100644 --- a/src/shadow-core/src/shadow.rs +++ b/src/shadow-core/src/shadow.rs @@ -114,7 +114,7 @@ impl ShadowEntry { pub fn days_since_epoch() -> Result { let secs = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) - .map_err(|_| ShadowError::Parse("system clock is before Unix epoch".into()))?; + .map_err(|_| ShadowError::Other("system clock is before Unix epoch".into()))?; let secs = i64::try_from(secs.as_secs()).unwrap_or(i64::MAX); Ok(secs / 86400) } diff --git a/src/uu/chpasswd/src/chpasswd.rs b/src/uu/chpasswd/src/chpasswd.rs index 3d24b57..be551b9 100644 --- a/src/uu/chpasswd/src/chpasswd.rs +++ b/src/uu/chpasswd/src/chpasswd.rs @@ -231,15 +231,49 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let is_encrypted = matches.get_flag(options::ENCRYPTED); let use_md5 = matches.get_flag(options::MD5); let crypt_method = matches.get_one::(options::CRYPT_METHOD); - let sha_rounds = matches.get_one::(options::SHA_ROUNDS); + + // Reject -m unconditionally — MD5 is insecure. + if use_md5 { + return Err(ChpasswdError::UnexpectedFailure( + "MD5 is insecure and not supported; use -c SHA512 instead".into(), + ) + .into()); + } + + // Validate --sha-rounds range. + let sha_rounds = match matches.get_one::(options::SHA_ROUNDS).copied() { + Some(r @ 1..=i64::MAX) => match u32::try_from(r) { + Ok(v) => Some(v), + Err(_) => { + return Err(ChpasswdError::UnexpectedFailure(format!( + "invalid value for --sha-rounds '{r}': must be between 1 and {}", + u32::MAX + )) + .into()); + } + }, + Some(r) => { + return Err(ChpasswdError::UnexpectedFailure(format!( + "invalid value for --sha-rounds '{r}': must be between 1 and {}", + u32::MAX + )) + .into()); + } + None => None, + }; // Determine the hashing method for plaintext mode. let hash_config = if is_encrypted { None } else { - let method = resolve_crypt_method(crypt_method.map(String::as_str), use_md5)?; - let rounds = sha_rounds.and_then(|&r| u32::try_from(r).ok()); - Some((method, rounds)) + let method = resolve_crypt_method(crypt_method.map(String::as_str))?; + if sha_rounds.is_some() && method == shadow_core::crypt::CryptMethod::Yescrypt { + return Err(ChpasswdError::UnexpectedFailure( + "--sha-rounds is not supported with YESCRYPT".into(), + ) + .into()); + } + Some((method, sha_rounds)) }; // Read all pairs from stdin before acquiring locks. @@ -395,16 +429,15 @@ fn apply_password_changes( // Helpers // --------------------------------------------------------------------------- -/// Map `-c` / `-m` flags to a `CryptMethod`. +/// Map `-c` flag to a `CryptMethod`. fn resolve_crypt_method( method: Option<&str>, - use_md5: bool, ) -> Result { use shadow_core::crypt::CryptMethod; match method { Some("SHA256") => Ok(CryptMethod::Sha256), - Some("SHA512") => Ok(CryptMethod::Sha512), + Some("SHA512") | None => Ok(CryptMethod::Sha512), Some("YESCRYPT") => Ok(CryptMethod::Yescrypt), Some("MD5" | "DES") => Err(ChpasswdError::UnexpectedFailure( "MD5 and DES are insecure and not supported for plaintext hashing".into(), @@ -412,10 +445,6 @@ fn resolve_crypt_method( Some(other) => Err(ChpasswdError::UnexpectedFailure(format!( "unknown crypt method: {other}" ))), - None if use_md5 => Err(ChpasswdError::UnexpectedFailure( - "MD5 is insecure and not supported; use -c SHA512 instead".into(), - )), - None => Ok(CryptMethod::Sha512), } } From 20ee217096301ccdfec242cb34da158751291a09 Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Fri, 3 Apr 2026 18:13:08 +0200 Subject: [PATCH 5/6] crypt: detect unsupported hash methods from crypt(3) crypt(3) returns "*0" or "*1" when the requested method is unsupported (e.g. yescrypt on musl libc). Previously this was returned as a valid hash, causing assertion failures in tests on Alpine. Now hash_password() checks for the "*" error prefix and returns ShadowError::Auth, allowing callers to handle gracefully. --- src/shadow-core/src/crypt.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/shadow-core/src/crypt.rs b/src/shadow-core/src/crypt.rs index 8e1413f..8ca717e 100644 --- a/src/shadow-core/src/crypt.rs +++ b/src/shadow-core/src/crypt.rs @@ -106,8 +106,10 @@ pub fn hash_password( .to_str() .map_err(|_| ShadowError::Auth("crypt(3) returned invalid UTF-8".into()))?; - // crypt(3) returns "*0" or "*1" when the method is unsupported. - if hash.starts_with('*') { + // crypt(3) returns "*0"/"*1" for unsupported methods (glibc/libxcrypt). + // musl silently falls back to DES — detect by checking the method prefix. + let prefix = method.prefix(); + if hash.starts_with('*') || !hash.starts_with(prefix) { return Err(ShadowError::Auth( format!("crypt(3) does not support {method:?} on this system").into(), )); From d74a1e667fe14fa35e9af1b08a34535dc4a0144c Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Fri, 3 Apr 2026 18:14:57 +0200 Subject: [PATCH 6/6] review: address all 7 Copilot review comments - shadow.rs: use ShadowError::Other instead of Parse for clock error - crypt.rs: use getrandom(2) syscall instead of /dev/urandom (works in chroot) - crypt.rs: reject rounds parameter for yescrypt in generate_salt() - crypt.rs: detect unsupported methods by verifying result prefix (musl compat) - crypt.rs: serialize tests with Mutex (crypt(3) uses static buffer) - crypt.rs: add 6 round-trip tests for hash_password/verify_password - chpasswd.rs: validate --sha-rounds range, reject -m unconditionally - chpasswd.rs: strip rounds for yescrypt before calling hash_password - chpasswd: add crypt feature to shadow-core dependency --- src/uu/chpasswd/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/chpasswd/Cargo.toml b/src/uu/chpasswd/Cargo.toml index 7dad5a3..96d46b1 100644 --- a/src/uu/chpasswd/Cargo.toml +++ b/src/uu/chpasswd/Cargo.toml @@ -21,7 +21,7 @@ path = "src/main.rs" [dependencies] clap = { workspace = true } nix = { workspace = true } -shadow-core = { workspace = true, features = ["shadow"] } +shadow-core = { workspace = true, features = ["shadow", "crypt"] } uucore = { workspace = true } zeroize = { workspace = true }