diff --git a/Cargo.toml b/Cargo.toml index 9cd0b68..a366691 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -64,6 +64,7 @@ libc = "0.2" # Security zeroize = "1" +subtle = "2" landlock = "0.4" # Testing diff --git a/src/shadow-core/Cargo.toml b/src/shadow-core/Cargo.toml index d577069..b602c0c 100644 --- a/src/shadow-core/Cargo.toml +++ b/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] diff --git a/src/shadow-core/src/crypt.rs b/src/shadow-core/src/crypt.rs index 4053136..8e0bedf 100644 --- a/src/shadow-core/src/crypt.rs +++ b/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 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()) } diff --git a/src/shadow-core/src/sysroot.rs b/src/shadow-core/src/sysroot.rs index f8619c8..9e8134a 100644 --- a/src/shadow-core/src/sysroot.rs +++ b/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 { + 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")) + ); + } } diff --git a/src/uu/grpck/src/grpck.rs b/src/uu/grpck/src/grpck.rs index 78ce51d..650e755 100644 --- a/src/uu/grpck/src/grpck.rs +++ b/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) diff --git a/src/uu/newgrp/Cargo.toml b/src/uu/newgrp/Cargo.toml index 5d4828e..a6fa58e 100644 --- a/src/uu/newgrp/Cargo.toml +++ b/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 } diff --git a/src/uu/newgrp/src/newgrp.rs b/src/uu/newgrp/src/newgrp.rs index 2c112de..0f76157 100644 --- a/src/uu/newgrp/src/newgrp.rs +++ b/src/uu/newgrp/src/newgrp.rs @@ -100,13 +100,23 @@ fn get_current_gid() -> Result { } } -/// 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 { +/// +/// The returned password is wrapped in `Zeroizing` to ensure it is +/// scrubbed from memory when dropped. +fn read_password(prompt: &str) -> Result, NewgrpError> { use std::io::{BufRead, Write}; let tty = std::fs::File::options() @@ -196,7 +209,7 @@ fn read_password(prompt: &str) -> Result { // 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 { 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()); } } diff --git a/src/uu/pwck/src/pwck.rs b/src/uu/pwck/src/pwck.rs index 6b31f62..5cc8942 100644 --- a/src/uu/pwck/src/pwck.rs +++ b/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) diff --git a/src/uu/useradd/src/useradd.rs b/src/uu/useradd/src/useradd.rs index 8cf9544..c3ef975 100644 --- a/src/uu/useradd/src/useradd.rs +++ b/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, diff --git a/src/uu/userdel/src/userdel.rs b/src/uu/userdel/src/userdel.rs index a8ddd71..239f050 100644 --- a/src/uu/userdel/src/userdel.rs +++ b/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::(options::LOGIN) - .expect("LOGIN is required"); + let Some(login) = matches.get_one::(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::(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(()) diff --git a/src/uu/usermod/src/usermod.rs b/src/uu/usermod/src/usermod.rs index a58bdc1..a4be874 100644 --- a/src/uu/usermod/src/usermod.rs +++ b/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::(options::USER) - .expect("USER is required"); + let Some(login) = matches.get_one::(options::USER) else { + return Err(UsermodError::AlreadyPrinted(2).into()); + }; let prefix = matches .get_one::(options::PREFIX) .or_else(|| matches.get_one::(options::ROOT))