diff --git a/src/shadow-core/src/atomic.rs b/src/shadow-core/src/atomic.rs index 888eb56..0fddcc9 100644 --- a/src/shadow-core/src/atomic.rs +++ b/src/shadow-core/src/atomic.rs @@ -22,19 +22,23 @@ use crate::error::ShadowError; /// RAII guard that saves and restores the process umask. /// /// 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. +/// `OpenOptions::mode()` (or `DirBuilder::mode()`) are applied exactly. +/// The original umask is restored when the guard is dropped, even on +/// error or panic paths. /// /// # 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(rustix::fs::Mode, std::marker::PhantomData>); +/// single-threaded, so this is not an issue in practice. The embedded +/// `PhantomData>` makes the guard `!Send`, preventing accidental +/// movement across threads. +pub struct UmaskGuard(rustix::fs::Mode, std::marker::PhantomData>); impl UmaskGuard { /// Set umask to zero and return a guard that restores the original. - fn zero() -> Self { + #[must_use = "the umask is restored when the guard is dropped; binding to `_` drops it immediately"] + pub fn zero() -> Self { Self( rustix::process::umask(rustix::fs::Mode::empty()), std::marker::PhantomData, diff --git a/src/shadow-core/src/skel.rs b/src/shadow-core/src/skel.rs index 0d01a92..9380825 100644 --- a/src/shadow-core/src/skel.rs +++ b/src/shadow-core/src/skel.rs @@ -8,8 +8,10 @@ //! When `useradd -m` creates a home directory, it populates it with //! files from the skeleton directory (typically `/etc/skel`). +use std::os::unix::fs::{DirBuilderExt, OpenOptionsExt, PermissionsExt}; use std::path::Path; +use crate::atomic::UmaskGuard; use crate::error::ShadowError; /// Recursively copy the skeleton directory into the target home directory. @@ -27,6 +29,12 @@ pub fn copy_skel(skel_dir: &Path, home_dir: &Path, uid: u32, gid: u32) -> Result return Ok(()); } + // Reset umask so the modes passed to mkdir/open are honored exactly. + // Without this, an inherited umask would leave copied files and + // subdirectories more permissive than the source until set_permissions + // is called — a window during which they are owned by root. + let _umask = UmaskGuard::zero(); + copy_dir_recursive(skel_dir, home_dir, uid, gid) } @@ -42,12 +50,18 @@ 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(&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) + // Preserve the source directory's permissions atomically by + // passing the mode to mkdir(2) via DirBuilder. Mask to permission + // and special bits only — `mode()` returns raw st_mode which + // includes file-type bits we must not pass to mkdir. + let src_mode = std::fs::metadata(&src_path) .map_err(|e| ShadowError::IoPath(e, src_path.clone()))? - .permissions(); - std::fs::set_permissions(&dst_path, src_perms) + .permissions() + .mode() + & 0o7777; + std::fs::DirBuilder::new() + .mode(src_mode) + .create(&dst_path) .map_err(|e| ShadowError::IoPath(e, dst_path.clone()))?; copy_dir_recursive(&src_path, &dst_path, uid, gid)?; } else if file_type.is_symlink() { @@ -56,7 +70,25 @@ fn copy_dir_recursive(src: &Path, dst: &Path, uid: u32, gid: u32) -> Result<(), std::os::unix::fs::symlink(&target, &dst_path) .map_err(|e| ShadowError::IoPath(e, dst_path.clone()))?; } else if file_type.is_file() { - std::fs::copy(&src_path, &dst_path) + // std::fs::copy uses File::create internally, which applies the + // umask and leaves a window where the file is more permissive + // than the source. Open the destination with the source's mode + // baked into the open(2) call instead. Mask raw st_mode to + // permission and special bits only. + let src_mode = std::fs::metadata(&src_path) + .map_err(|e| ShadowError::IoPath(e, src_path.clone()))? + .permissions() + .mode() + & 0o7777; + let mut src_file = std::fs::File::open(&src_path) + .map_err(|e| ShadowError::IoPath(e, src_path.clone()))?; + let mut dst_file = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(src_mode) + .open(&dst_path) + .map_err(|e| ShadowError::IoPath(e, dst_path.clone()))?; + std::io::copy(&mut src_file, &mut dst_file) .map_err(|e| ShadowError::IoPath(e, dst_path.clone()))?; } // Silently skip FIFOs, sockets, and device nodes — copying them diff --git a/src/uu/useradd/src/useradd.rs b/src/uu/useradd/src/useradd.rs index c555d92..577647c 100644 --- a/src/uu/useradd/src/useradd.rs +++ b/src/uu/useradd/src/useradd.rs @@ -15,7 +15,7 @@ use std::fmt; use std::io::Write as _; -use std::os::unix::fs::PermissionsExt; +use std::os::unix::fs::DirBuilderExt; use std::path::Path; use clap::{Arg, ArgAction, Command}; @@ -916,8 +916,23 @@ fn append_subid_entry(path: &Path, name: &str, count: u64) -> UResult<()> { /// /// Paths must already be resolved through `SysRoot` by the caller. fn create_home_directory(home_path: &Path, skel_path: &Path, uid: u32, gid: u32) -> UResult<()> { - // Use create_dir (not create_dir_all) to avoid TOCTOU between exists() and mkdir(). - match std::fs::create_dir(home_path) { + // The kernel does not reset umask across setuid, so a caller-controlled + // inherited umask may still be in effect in our process. A non-zero umask + // can mask off requested permission bits, so even mkdir(0o700) is not + // guaranteed to result in 0o700 unless we clear it first (e.g., umask + // 0o700 would mask the user RWX bits and leave the dir at 0o000). + // Forcing umask to 0 makes the requested mode exact, regardless of caller + // environment; umask can only make the result less permissive than the + // mode we requested, never more. Scoped to the mkdir call only — chown + // doesn't need it, and copy_skel manages its own umask internally. + let mkdir_result = { + let _umask = shadow_core::atomic::UmaskGuard::zero(); + std::fs::DirBuilder::new().mode(0o700).create(home_path) + }; + + // Use DirBuilder::mode() so mkdir(2) is called with 0o700 atomically. + // Use create (not recursive) to avoid TOCTOU between exists() and mkdir(). + match mkdir_result { Ok(()) => {} Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { uucore::show_warning!( @@ -935,15 +950,8 @@ fn create_home_directory(home_path: &Path, skel_path: &Path, uid: u32, gid: u32) } } - // Set permissions to 0700 (home directories should be private by default). - std::fs::set_permissions(home_path, std::fs::Permissions::from_mode(0o700)).map_err(|e| { - UseraddError::CannotCreateHome(format!( - "cannot set permissions on '{}': {e}", - home_path.display() - )) - })?; - - // Set ownership. + // The directory is 0o700 owned by root at this point — only root can + // traverse it, so the chown that follows transfers a private dir. std::os::unix::fs::chown(home_path, Some(uid), Some(gid)).map_err(|e| { UseraddError::CannotCreateHome(format!( "cannot set ownership on '{}': {e}", @@ -1119,6 +1127,7 @@ pub fn uu_app() -> Command { mod tests { use super::*; use std::fs; + use std::os::unix::fs::PermissionsExt; // ----------------------------------------------------------------------- // Clap validation tests @@ -1776,6 +1785,62 @@ mod tests { assert_eq!(meta.permissions().mode() & 0o777, 0o700); } + /// Local RAII guard used by the umask regression test. + struct UmaskRestore(rustix::fs::Mode); + impl Drop for UmaskRestore { + fn drop(&mut self) { + rustix::process::umask(self.0); + } + } + + /// Serialize tests that mutate process-global umask. Without this, parallel + /// cargo test runs can leak umask=0 to unrelated tests creating files. + static UMASK_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + #[test] + fn test_integration_home_directory_ignores_inherited_umask() { + // Regression test for #157: an inherited umask must not weaken the + // home directory's final mode. Note that this test asserts the *final* + // mode after `create_home_directory` returns; it does not directly + // observe the mkdir(2) syscall, so a pre-fix implementation that did + // `mkdir(0o777)` followed by `chmod(0o700)` would also pass. The + // atomicity guarantee — mode set in the mkdir syscall itself, not + // after — is enforced by inspection of `DirBuilder::mode(0o700)` in + // the implementation. Catching the window directly would require + // strace / ptrace / eBPF, which is out of scope for a unit test. + if skip_unless_root() { + return; + } + + // Hold the umask lock for the duration of the test so parallel + // cargo test runs don't observe our umask=0 in unrelated file ops. + let _serialize = UMASK_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + + let dir = tempfile::tempdir().expect("tempdir"); + let home = dir.path().join("home/testuser"); + let skel = dir.path().join("skel"); + fs::create_dir_all(dir.path().join("home")).expect("create home parent"); + fs::create_dir_all(&skel).expect("create skel"); + + // Set process umask to zero (most permissive setting), then create + // the home dir. The guard inside create_home_directory also sets + // umask to zero for the duration of the mkdir; this test asserts the + // final directory mode. `_restore` puts back the previously active + // umask when this scope exits. + let _restore = UmaskRestore(rustix::process::umask(rustix::fs::Mode::empty())); + + create_home_directory(&home, &skel, 1000, 1000).expect("create home"); + + let meta = fs::metadata(&home).expect("metadata"); + assert_eq!( + meta.permissions().mode() & 0o777, + 0o700, + "home directory must be 0o700 even with permissive umask", + ); + } + #[test] fn test_integration_home_already_exists() { if skip_unless_root() {