Merge pull request #134 from shadow-utils-rs/fix/119-133-review-findings

security: fix 10 review findings (2 critical, 5 high, 1 medium)

Cargo.toml

@@ -64,6 +64,7 @@ libc = "0.2"
 
 # Security
 zeroize = "1"
+subtle = "2"
 landlock = "0.4"
 
 # Testing

src/shadow-core/Cargo.toml

@@ -19,6 +19,7 @@ libc = { workspace = true }
 nix = { workspace = true }
 thiserror = { workspace = true }
 zeroize = { workspace = true }
+subtle = { workspace = true }
 landlock = { workspace = true, optional = true }
 
 [dev-dependencies]

src/shadow-core/src/crypt.rs

@@ -10,6 +10,8 @@
 
 use std::ffi::CString;
 
+use subtle::ConstantTimeEq;
+
 use crate::error::ShadowError;
 
 #[link(name = "crypt")]
@@ -43,5 +45,7 @@ pub fn verify_password(password: &str, hash: &str) -> Result<bool, ShadowError>
     let result_str = unsafe { std::ffi::CStr::from_ptr(result) };
     let result_str = result_str.to_str().unwrap_or("");
 
-    Ok(result_str == hash)
+    // Constant-time comparison prevents timing side-channel attacks
+    // that could leak password hash information.
+    Ok(result_str.as_bytes().ct_eq(hash.as_bytes()).into())
 }

src/shadow-core/src/sysroot.rs

@@ -9,7 +9,7 @@
 //! `--prefix DIR` prepends DIR to all file paths (no `chroot` syscall).
 //! `--root DIR` does an actual `chroot()` — paths are then relative to `/`.
 
-use std::path::{Path, PathBuf};
+use std::path::{Component, Path, PathBuf};
 
 /// Resolves file paths relative to an optional prefix directory.
 #[derive(Debug, Clone)]
@@ -31,10 +31,28 @@ impl SysRoot {
     /// Resolve a path relative to the prefix.
     ///
     /// Strips leading `/` from `relative` before joining with the prefix.
+    /// Returns `None` if the path contains `..` components (path traversal).
+    pub fn try_resolve(&self, relative: &str) -> Option<PathBuf> {
+        let stripped = relative.strip_prefix('/').unwrap_or(relative);
+        let joined = self.prefix.join(stripped);
+        // Reject path traversal: ".." components could escape the prefix.
+        for component in joined.components() {
+            if matches!(component, Component::ParentDir) {
+                return None;
+            }
+        }
+        Some(joined)
+    }
+
+    /// Resolve a path relative to the prefix.
+    ///
+    /// Strips leading `/` from `relative` before joining with the prefix.
+    /// Only for hardcoded paths — use [`try_resolve`] for user-controlled input.
     #[must_use]
     pub fn resolve(&self, relative: &str) -> PathBuf {
-        let stripped = relative.strip_prefix('/').unwrap_or(relative);
-        self.prefix.join(stripped)
+        // All callers pass hardcoded paths like "/etc/passwd" — never ".."
+        self.try_resolve(relative)
+            .unwrap_or_else(|| unreachable!("resolve() called with path traversal: {relative:?}"))
     }
 
     /// Path to `/etc/passwd`.
@@ -129,4 +147,21 @@ mod tests {
         );
         assert_eq!(root.resolve("etc/shadow"), PathBuf::from("/mnt/etc/shadow"));
     }
+
+    #[test]
+    fn test_try_resolve_rejects_path_traversal() {
+        let root = SysRoot::new(Some(Path::new("/mnt/chroot")));
+        // Attempting to escape the prefix via ".." returns None.
+        assert_eq!(root.try_resolve("/../etc/shadow"), None);
+        assert_eq!(root.try_resolve("/home/../../etc/shadow"), None);
+    }
+
+    #[test]
+    fn test_try_resolve_accepts_valid_paths() {
+        let root = SysRoot::new(Some(Path::new("/mnt/chroot")));
+        assert_eq!(
+            root.try_resolve("/etc/shadow"),
+            Some(PathBuf::from("/mnt/chroot/etc/shadow"))
+        );
+    }
 }

src/uu/grpck/src/grpck.rs

@@ -127,6 +127,8 @@ impl GrpckOptions {
 
 #[uucore::main]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
+    let _ = shadow_core::hardening::harden_process();
+
     let matches = uu_app().try_get_matches_from(args)?;
     let opts = GrpckOptions::from_matches(&matches);
     run_checks(&opts)

src/uu/newgrp/Cargo.toml

@@ -21,6 +21,7 @@ path = "src/main.rs"
 [dependencies]
 clap = { workspace = true }
 nix = { workspace = true }
+zeroize = { workspace = true }
 shadow-core = { workspace = true, features = ["group", "gshadow", "crypt"] }
 uucore = { workspace = true }
 

src/uu/newgrp/src/newgrp.rs

@@ -100,13 +100,23 @@ fn get_current_gid() -> Result<u32, NewgrpError> {
     }
 }
 
-/// Determine the shell to exec. Uses `$SHELL` if set and non-empty,
-/// otherwise falls back to `/bin/sh`.
-fn get_shell() -> String {
-    std::env::var("SHELL")
-        .ok()
-        .filter(|s| !s.is_empty())
-        .unwrap_or_else(|| "/bin/sh".to_string())
+/// Determine the shell to exec from the user's passwd entry.
+///
+/// Reads the shell field from `/etc/passwd` for the given UID rather
+/// than trusting `$SHELL`, which is attacker-controlled in a
+/// setuid-root context.
+fn get_shell(uid: nix::unistd::Uid) -> String {
+    match nix::unistd::User::from_uid(uid) {
+        Ok(Some(user)) => {
+            let shell = user.shell.to_string_lossy().to_string();
+            if shell.is_empty() {
+                "/bin/sh".to_string()
+            } else {
+                shell
+            }
+        }
+        _ => "/bin/sh".to_string(),
+    }
 }
 
 /// Check if the user is a member of the group (either as primary GID
@@ -171,7 +181,10 @@ impl Drop for EchoGuard {
 }
 
 /// Read a password from `/dev/tty` with echo disabled.
-fn read_password(prompt: &str) -> Result<String, NewgrpError> {
+///
+/// The returned password is wrapped in `Zeroizing` to ensure it is
+/// scrubbed from memory when dropped.
+fn read_password(prompt: &str) -> Result<zeroize::Zeroizing<String>, NewgrpError> {
     use std::io::{BufRead, Write};
 
     let tty = std::fs::File::options()
@@ -196,7 +209,7 @@ fn read_password(prompt: &str) -> Result<String, NewgrpError> {
     // Disable echo; restored automatically on drop.
     let guard = EchoGuard::disable(tty_for_guard)?;
 
-    let mut buf = String::new();
+    let mut buf = zeroize::Zeroizing::new(String::new());
     let mut reader = std::io::BufReader::new(&tty);
     reader
         .read_line(&mut buf)
@@ -206,7 +219,9 @@ fn read_password(prompt: &str) -> Result<String, NewgrpError> {
     drop(guard);
     let _ = (&tty).write_all(b"\n");
 
-    Ok(buf.trim_end_matches('\n').to_string())
+    Ok(zeroize::Zeroizing::new(
+        buf.trim_end_matches('\n').to_string(),
+    ))
 }
 
 /// Verify a password against a crypt(3) hash.
@@ -300,8 +315,8 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
             .map_err(|e| NewgrpError::Error(format!("cannot drop privileges: {e}")))?;
     }
 
-    // Exec the user's shell.
-    let shell = get_shell();
+    // Exec the user's shell (from passwd entry, not $SHELL).
+    let shell = get_shell(real_uid);
     let shell_cstr = CString::new(shell.as_str())
         .map_err(|_| NewgrpError::Error("invalid shell path".into()))?;
 
@@ -455,7 +470,8 @@ mod tests {
     #[test]
     fn test_get_shell_default() {
         // This test is environment-dependent but should at least not panic.
-        let shell = get_shell();
+        let uid = nix::unistd::getuid();
+        let shell = get_shell(uid);
         assert!(!shell.is_empty());
     }
 }

src/uu/pwck/src/pwck.rs

@@ -141,6 +141,8 @@ impl PwckOptions {
 
 #[uucore::main]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
+    let _ = shadow_core::hardening::harden_process();
+
     let matches = uu_app().try_get_matches_from(args)?;
     let opts = PwckOptions::from_matches(&matches);
     run_checks(&opts)

src/uu/useradd/src/useradd.rs

@@ -575,11 +575,7 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> {
     };
     write_passwd_entry(&passwd_path, &passwd_entries, &passwd_entry)?;
 
-    // Release locks now that passwd and group writes are complete.
-    drop(group_lock);
-    drop(passwd_lock);
-
-    // Step 13: Write /etc/shadow entry.
+    // Step 13: Write /etc/shadow entry (passwd+group locks still held).
     let shadow_path = opts.root.shadow_path();
     let shadow_entry = ShadowEntry {
         name: opts.login.clone(),
@@ -594,27 +590,21 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> {
     };
     write_shadow_entry(&shadow_path, &shadow_entry)?;
 
+    // Release locks now that passwd, group, and shadow writes are complete.
+    drop(group_lock);
+    drop(passwd_lock);
+
     // Step 14: Allocate subordinate UID/GID ranges for rootless containers.
     // Only done when the relevant file exists (matching GNU shadow-utils behavior).
     let subuid_path = opts.root.subuid_path();
     if subuid_path.exists()
-        && let Err(e) = append_subid_entry(
-            &subuid_path,
-            &opts.login,
-            100_000 + u64::from(uid) * 65_536,
-            65_536,
-        )
+        && let Err(e) = append_subid_entry(&subuid_path, &opts.login, 65_536)
     {
         uucore::show_error!("warning: failed to add subordinate UID range: {e}");
     }
     let subgid_path = opts.root.subgid_path();
     if subgid_path.exists()
-        && let Err(e) = append_subid_entry(
-            &subgid_path,
-            &opts.login,
-            100_000 + u64::from(gid) * 65_536,
-            65_536,
-        )
+        && let Err(e) = append_subid_entry(&subgid_path, &opts.login, 65_536)
     {
         uucore::show_error!("warning: failed to add subordinate GID range: {e}");
     }
@@ -869,7 +859,7 @@ fn add_to_supplementary_groups(
 ///
 /// Skips the write if the user already has an entry in the file.
 /// Uses file locking and atomic writes for crash safety.
-fn append_subid_entry(path: &Path, name: &str, start: u64, count: u64) -> UResult<()> {
+fn append_subid_entry(path: &Path, name: &str, count: u64) -> UResult<()> {
     use shadow_core::subid::{self, SubIdEntry};
 
     let lock = FileLock::acquire(path).map_err(|e| {
@@ -894,6 +884,15 @@ fn append_subid_entry(path: &Path, name: &str, start: u64, count: u64) -> UResul
         return Ok(());
     }
 
+    // Find next available range by starting after the highest existing end.
+    // Clamp to at least 100_000 even if existing entries are below that threshold.
+    let start = entries
+        .iter()
+        .map(|e| e.start.saturating_add(e.count))
+        .max()
+        .unwrap_or(100_000)
+        .max(100_000);
+
     entries.push(SubIdEntry {
         name: name.to_string(),
         start,

src/uu/userdel/src/userdel.rs

@@ -72,6 +72,8 @@ impl UError for UserdelError {
 
 #[uucore::main]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
+    let _ = shadow_core::hardening::harden_process();
+
     let matches = match uu_app().try_get_matches_from(args) {
         Ok(m) => m,
         Err(e) => {
@@ -83,9 +85,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         }
     };
 
-    let login = matches
-        .get_one::<String>(options::LOGIN)
-        .expect("LOGIN is required");
+    let Some(login) = matches.get_one::<String>(options::LOGIN) else {
+        return Err(UserdelError::AlreadyPrinted(exit_codes::INVALID_SYNTAX).into());
+    };
     let remove_home = matches.get_flag(options::REMOVE);
     let prefix = matches
         .get_one::<String>(options::PREFIX)
@@ -263,8 +265,8 @@ fn safe_remove_home(home: &Path) -> Result<(), UserdelError> {
         }
     }
 
-    std::fs::remove_dir_all(home).map_err(|e| {
-        UserdelError::CantRemoveHome(format!("cannot remove '{}': {e}", home.display()))
+    std::fs::remove_dir_all(&canonical).map_err(|e| {
+        UserdelError::CantRemoveHome(format!("cannot remove '{}': {e}", canonical.display()))
     })?;
 
     Ok(())

src/uu/usermod/src/usermod.rs

@@ -76,6 +76,8 @@ impl UError for UsermodError {
 #[uucore::main]
 #[allow(clippy::too_many_lines)]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
+    let _ = shadow_core::hardening::harden_process();
+
     let matches = match uu_app().try_get_matches_from(args) {
         Ok(m) => m,
         Err(e) => {
@@ -87,9 +89,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         }
     };
 
-    let login = matches
-        .get_one::<String>(options::USER)
-        .expect("USER is required");
+    let Some(login) = matches.get_one::<String>(options::USER) else {
+        return Err(UsermodError::AlreadyPrinted(2).into());
+    };
     let prefix = matches
         .get_one::<String>(options::PREFIX)
         .or_else(|| matches.get_one::<String>(options::ROOT))