From 9e3faa966c8bc46575174dd18617e34d46f52fbf Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Tue, 24 Mar 2026 14:43:38 +0100 Subject: [PATCH] address Copilot review: fix docs, feature-gate crypt, MSRV 1.94, CI msrv job --- .clippy.toml | 2 +- .github/workflows/ci.yml | 10 +++++++ Cargo.toml | 2 +- src/shadow-core/Cargo.toml | 1 + src/shadow-core/src/hardening.rs | 18 +++++------- src/shadow-core/src/lib.rs | 1 + src/uu/newgrp/Cargo.toml | 2 +- src/uu/passwd/src/passwd.rs | 9 +++--- src/uu/pwck/src/pwck.rs | 16 ++++------- src/uu/userdel/src/userdel.rs | 47 +++++++++++++++----------------- src/uu/usermod/src/usermod.rs | 8 +++--- 11 files changed, 59 insertions(+), 57 deletions(-) diff --git a/.clippy.toml b/.clippy.toml index 94c835e..5f62e25 100644 --- a/.clippy.toml +++ b/.clippy.toml @@ -1,4 +1,4 @@ -msrv = "1.85.0" +msrv = "1.94.0" cognitive-complexity-threshold = 30 check-private-items = true avoid-breaking-exported-api = false diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 363fa8f..e101bb6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,6 +41,16 @@ jobs: - run: sudo apt-get update && sudo apt-get install -y libpam0g-dev libselinux1-dev libaudit-dev pkg-config libcrypt-dev - run: cargo test --workspace + msrv: + name: MSRV (1.94.0) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@1.94.0 + - uses: Swatinem/rust-cache@v2 + - run: sudo apt-get update && sudo apt-get install -y libpam0g-dev libselinux1-dev libaudit-dev pkg-config libcrypt-dev + - run: cargo check --workspace + deny: name: Cargo Deny runs-on: ubuntu-latest diff --git a/Cargo.toml b/Cargo.toml index 04b50b4..8cace05 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,7 +33,7 @@ members = [ [workspace.package] version = "0.0.1" edition = "2024" -rust-version = "1.85.0" +rust-version = "1.94.0" license = "MIT" authors = ["shadow-rs contributors"] repository = "https://github.com/shadow-utils-rs/shadow-rs" diff --git a/src/shadow-core/Cargo.toml b/src/shadow-core/Cargo.toml index 02f8170..c7a729a 100644 --- a/src/shadow-core/Cargo.toml +++ b/src/shadow-core/Cargo.toml @@ -22,6 +22,7 @@ tempfile = { workspace = true } [features] default = [] +crypt = [] pam = [] selinux = [] shadow = [] diff --git a/src/shadow-core/src/hardening.rs b/src/shadow-core/src/hardening.rs index 818c8dd..1a9c86f 100644 --- a/src/shadow-core/src/hardening.rs +++ b/src/shadow-core/src/hardening.rs @@ -9,11 +9,10 @@ //! hostile callers. These functions implement the standard hardening //! steps that all tools share. -/// Suppress core dumps (`RLIMIT_CORE=0`) and prevent ptrace attachment. +/// Suppress core dumps via `RLIMIT_CORE=0`. /// /// A core dump from a setuid-root process could expose password hashes -/// and plaintext passwords. `PR_SET_DUMPABLE=0` also prevents -/// `/proc/pid/mem` reads by other processes. +/// and plaintext passwords. pub fn suppress_core_dumps() { let _ = nix::sys::resource::setrlimit(nix::sys::resource::Resource::RLIMIT_CORE, 0, 0); // PR_SET_DUMPABLE via nix::sys::prctl (no raw unsafe needed). @@ -38,15 +37,12 @@ pub fn raise_file_size_limit() { /// Clears all environment variables except essential ones (`TERM`, `LANG`, /// `LC_*`) and sets `PATH` to a safe default. Prevents environment variable /// injection attacks (`LD_PRELOAD`, `IFS`, `CDPATH`, etc.). -/// Sanitize the environment by re-execing ourselves with a clean env. +/// Build a sanitized environment for child process spawning. /// -/// Instead of using `set_var`/`remove_var` (unsafe in edition 2024), -/// we record the sanitized environment and let the caller pass it -/// to any child processes via `Command::env_clear().envs(...)`. -/// -/// Returns the sanitized environment as key-value pairs. The current -/// process environment is NOT modified (that would require unsafe). -/// Tools should use the returned env when spawning subprocesses. +/// Returns safe key-value pairs (PATH + TERM/LANG/LC_*). The current +/// process environment is NOT modified (`set_var` is unsafe in edition +/// 2024). Pass the returned Vec to `Command::env_clear().envs(...)` +/// when spawning subprocesses. pub fn sanitized_env() -> Vec<(String, String)> { let mut env = Vec::new(); env.push(( diff --git a/src/shadow-core/src/lib.rs b/src/shadow-core/src/lib.rs index 87d9d41..9fc8cd0 100644 --- a/src/shadow-core/src/lib.rs +++ b/src/shadow-core/src/lib.rs @@ -33,6 +33,7 @@ pub mod subid; #[allow(unsafe_code)] pub mod pam; +#[cfg(feature = "crypt")] #[allow(unsafe_code)] pub mod crypt; diff --git a/src/uu/newgrp/Cargo.toml b/src/uu/newgrp/Cargo.toml index a736c5f..5ae6715 100644 --- a/src/uu/newgrp/Cargo.toml +++ b/src/uu/newgrp/Cargo.toml @@ -17,7 +17,7 @@ path = "src/main.rs" [dependencies] clap = { workspace = true } nix = { workspace = true } -shadow-core = { workspace = true, features = ["group", "gshadow"] } +shadow-core = { workspace = true, features = ["group", "gshadow", "crypt"] } uucore = { workspace = true } [dev-dependencies] diff --git a/src/uu/passwd/src/passwd.rs b/src/uu/passwd/src/passwd.rs index 1ef937d..ce446e1 100644 --- a/src/uu/passwd/src/passwd.rs +++ b/src/uu/passwd/src/passwd.rs @@ -504,10 +504,11 @@ impl Drop for PrivDrop { } } -// Note: signal handler removed — it required unsafe (sigaction + libc::write -// + libc::_exit). The EchoGuard RAII drop already restores terminal echo on -// any exit path including Ctrl+C (Rust runs destructors on panic). The -// "Password unchanged." message was cosmetic, not security-critical. +// Note: custom SIGINT handler removed — it required unsafe (sigaction + +// libc::write + libc::_exit). SIGINT terminates without unwinding, so +// EchoGuard::drop won't run. Terminal echo restoration after Ctrl+C relies +// on the terminal driver resetting on process exit (standard behavior). +// The "Password unchanged." message was cosmetic, not security-critical. /// Default operation: change password via PAM. /// diff --git a/src/uu/pwck/src/pwck.rs b/src/uu/pwck/src/pwck.rs index 01cc21f..6b31f62 100644 --- a/src/uu/pwck/src/pwck.rs +++ b/src/uu/pwck/src/pwck.rs @@ -506,16 +506,12 @@ fn check_shadow_entries( } // Check 11: Shadow last_change is not in the future (warning). - if !quiet { - if let Some(last_change) = entry.last_change { - if last_change > today_days { - uucore::show_warning!( - "user '{}': last password change in the future", - entry.name - ); - warnings += 1; - } - } + if !quiet + && let Some(last_change) = entry.last_change + && last_change > today_days + { + uucore::show_warning!("user '{}': last password change in the future", entry.name); + warnings += 1; } } diff --git a/src/uu/userdel/src/userdel.rs b/src/uu/userdel/src/userdel.rs index c83aee4..6285b3d 100644 --- a/src/uu/userdel/src/userdel.rs +++ b/src/uu/userdel/src/userdel.rs @@ -134,11 +134,11 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // 5. Optionally remove home directory (using the path saved from passwd). if remove_home { - if let Some(ref home_dir) = saved_home { - if !home_dir.is_empty() { - let home = root.resolve(home_dir); - safe_remove_home(&home)?; - } + if let Some(ref home_dir) = saved_home + && !home_dir.is_empty() + { + let home = root.resolve(home_dir); + safe_remove_home(&home)?; } // Remove mail spool. @@ -240,21 +240,18 @@ fn safe_remove_home(home: &Path) -> Result<(), UserdelError> { } // Refuse to remove a mount point (device ID differs from parent). - if let Some(parent) = home.parent() { - if parent.exists() { - use std::os::unix::fs::MetadataExt; - let parent_meta = std::fs::metadata(parent).map_err(|e| { - UserdelError::CantRemoveHome(format!( - "cannot stat parent of '{}': {e}", - home.display() - )) - })?; - if meta.dev() != parent_meta.dev() { - return Err(UserdelError::CantRemoveHome(format!( - "refusing to remove mount point at '{}'", - home.display() - ))); - } + if let Some(parent) = home.parent() + && parent.exists() + { + use std::os::unix::fs::MetadataExt; + let parent_meta = std::fs::metadata(parent).map_err(|e| { + UserdelError::CantRemoveHome(format!("cannot stat parent of '{}': {e}", home.display())) + })?; + if meta.dev() != parent_meta.dev() { + return Err(UserdelError::CantRemoveHome(format!( + "refusing to remove mount point at '{}'", + home.display() + ))); } } @@ -306,11 +303,11 @@ where continue; } - if let Ok(entry) = line.parse::() { - if entry.name() == login { - found = true; - continue; // skip this entry - } + if let Ok(entry) = line.parse::() + && entry.name() == login + { + found = true; + continue; // skip this entry } kept_lines.push(line.to_string()); } diff --git a/src/uu/usermod/src/usermod.rs b/src/uu/usermod/src/usermod.rs index 417e11c..f17c3b4 100644 --- a/src/uu/usermod/src/usermod.rs +++ b/src/uu/usermod/src/usermod.rs @@ -263,10 +263,10 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } } for gname in &new_groups { - if let Some(g) = ge.iter_mut().find(|g| g.name == *gname) { - if !g.members.iter().any(|m| m == login) { - g.members.push(login.clone()); - } + if let Some(g) = ge.iter_mut().find(|g| g.name == *gname) + && !g.members.iter().any(|m| m == login) + { + g.members.push(login.clone()); } }