uutils/shadow
0
0
Fork 0
mirror of https://github.com/uutils/shadow.git synced 2026-06-10 16:14:57 -07:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #158 from uutils/fix/157-mkdir-mode

Browse Source 
useradd, skel: set mkdir mode atomically to close world-writable window (#157)
This commit is contained in:
Pierre Warnier
2026-04-23 09:40:07 +02:00
committed by GitHub
parent d341138086 8b08f73000
commit befcac05fc
3 changed files with 124 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/shadow-core/src/atomic.rs
+9 -5
View File
@@ -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<std::rc::Rc<()>>);
/// single-threaded, so this is not an issue in practice. The embedded
/// `PhantomData<Rc<()>>` makes the guard `!Send`, preventing accidental
/// movement across threads.
pub struct UmaskGuard(rustix::fs::Mode, std::marker::PhantomData<std::rc::Rc<()>>);
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,
src/shadow-core/src/skel.rs
+38 -6
View File
@@ -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
src/uu/useradd/src/useradd.rs
+77 -12
View File
@@ -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() {