From 9242fe49937241713d5ffc3ea438f1956ab53378 Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Tue, 24 Mar 2026 11:58:48 +0100 Subject: [PATCH] security: fix 5 Gemini round 3 findings Fixes #73 Critical: useradd lock-then-read (hold lock across all file ops). Medium: skel only copies regular files (skip FIFOs/devices). Medium: lock PID file uses create_new (O_EXCL, no symlink follow). Medium: usermod validates group names before mutation. Medium: userdel canonicalizes paths before protected-dir check. 436 tests, zero clippy warnings. --- src/shadow-core/src/lock.rs | 37 +++++++++++++----- src/shadow-core/src/skel.rs | 7 +++- src/uu/useradd/src/useradd.rs | 71 +++++++++++++++++++++-------------- src/uu/userdel/src/userdel.rs | 8 ++-- src/uu/usermod/src/usermod.rs | 11 ++++++ 5 files changed, 93 insertions(+), 41 deletions(-) diff --git a/src/shadow-core/src/lock.rs b/src/shadow-core/src/lock.rs index bccf677..97ea724 100644 --- a/src/shadow-core/src/lock.rs +++ b/src/shadow-core/src/lock.rs @@ -157,16 +157,35 @@ fn tmp_lock_path(lock_path: &Path) -> PathBuf { } /// Write our PID to a temp file for later hard-linking. +/// +/// Uses `O_CREAT | O_EXCL` (`create_new`) to prevent symlink attacks: if +/// an attacker plants a symlink at `tmp_path`, `open` will fail instead of +/// following it. If the file already exists from a previous crashed run, +/// we remove it first and retry once. fn write_pid_file(tmp_path: &Path) -> Result<(), ShadowError> { - let mut file = std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .mode(0o600) - .open(tmp_path) - .map_err(|e| { - ShadowError::Lock(format!("cannot create {}: {e}", tmp_path.display()).into()) - })?; + let open_exclusive = || { + std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(tmp_path) + }; + + let mut file = match open_exclusive() { + Ok(f) => f, + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + // Stale temp file from a previous crashed run — remove and retry. + let _ = fs::remove_file(tmp_path); + open_exclusive().map_err(|e2| { + ShadowError::Lock(format!("cannot create {}: {e2}", tmp_path.display()).into()) + })? + } + Err(e) => { + return Err(ShadowError::Lock( + format!("cannot create {}: {e}", tmp_path.display()).into(), + )); + } + }; let pid = unistd::getpid(); write!(file, "{pid}").map_err(|e| { diff --git a/src/shadow-core/src/skel.rs b/src/shadow-core/src/skel.rs index f1847fc..5059d01 100644 --- a/src/shadow-core/src/skel.rs +++ b/src/shadow-core/src/skel.rs @@ -50,10 +50,15 @@ fn copy_dir_recursive(src: &Path, dst: &Path, uid: u32, gid: u32) -> Result<(), .map_err(|e| ShadowError::IoPath(e, src_path.clone()))?; std::os::unix::fs::symlink(&target, &dst_path) .map_err(|e| ShadowError::IoPath(e, dst_path.clone()))?; - } else { + } else if file_type.is_file() { std::fs::copy(&src_path, &dst_path) .map_err(|e| ShadowError::IoPath(e, dst_path.clone()))?; } + // Silently skip FIFOs, sockets, and device nodes — copying them + // would block indefinitely (FIFOs) or create security issues (devices). + else { + continue; + } // Set ownership (lchown for symlinks — doesn't follow the target). std::os::unix::fs::lchown(&dst_path, Some(uid), Some(gid)) diff --git a/src/uu/useradd/src/useradd.rs b/src/uu/useradd/src/useradd.rs index 9031dff..8c26703 100644 --- a/src/uu/useradd/src/useradd.rs +++ b/src/uu/useradd/src/useradd.rs @@ -456,33 +456,44 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { validate::validate_username(&opts.login) .map_err(|e| UseraddError::BadArgument(format!("{e}")))?; - // Step 2: Check username not already in use. + // Step 2: Acquire locks BEFORE reading so concurrent useradd cannot + // silently overwrite entries added between our read and write. let passwd_path = opts.root.passwd_path(); + let passwd_lock = FileLock::acquire(&passwd_path) + .map_err(|e| UseraddError::CannotUpdatePasswd(format!("cannot lock passwd: {e}")))?; + + let group_path = opts.root.group_path(); + let group_lock = FileLock::acquire(&group_path) + .map_err(|e| UseraddError::CannotUpdateGroup(format!("cannot lock group: {e}")))?; + + // Step 3: Read passwd under lock and check username not already in use. let passwd_entries = passwd::read_passwd_file(&passwd_path) .map_err(|e| UseraddError::CannotUpdatePasswd(format!("{e}")))?; if passwd_entries.iter().any(|e| e.name == opts.login) { + drop(group_lock); + drop(passwd_lock); return Err( UseraddError::UsernameInUse(format!("user '{}' already exists", opts.login)).into(), ); } - // Step 3: Load login.defs for UID/GID ranges. + // Step 4: Load login.defs for UID/GID ranges. let defs = LoginDefs::load(&opts.root.login_defs_path()) .map_err(|e| UseraddError::CannotUpdatePasswd(format!("{e}")))?; - // Step 4: Determine UID. + // Step 5: Determine UID. let uid = determine_uid(opts, &passwd_entries, &defs)?; - // Step 5: Read group entries (needed for GID resolution and user group creation). - let group_path = opts.root.group_path(); + // Step 6: Read group entries under lock (needed for GID resolution and + // user group creation). let mut group_entries = group::read_group_file(&group_path) .map_err(|e| UseraddError::CannotUpdateGroup(format!("{e}")))?; - // Step 6: Determine primary GID. + // Step 7: Determine primary GID. let (gid, new_group) = determine_gid(opts, uid, &group_entries, &defs)?; - // Step 7: Read gshadow entries. + // Step 8: Read gshadow entries. let gshadow_path = opts.root.gshadow_path(); let mut gshadow_entries = if gshadow_path.exists() { gshadow::read_gshadow_file(&gshadow_path) @@ -491,16 +502,18 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { Vec::new() }; - // Step 8: Validate supplementary groups exist. + // Step 9: Validate supplementary groups exist. for grp_name in &opts.groups { if !group_entries.iter().any(|g| g.name == *grp_name) { + drop(group_lock); + drop(passwd_lock); return Err( UseraddError::GroupNotExist(format!("group '{grp_name}' does not exist")).into(), ); } } - // Step 9: Determine home directory path. + // Step 10: Determine home directory path. let home_dir = opts.home_dir.clone().unwrap_or_else(|| { let home_base = defs.get("HOME").unwrap_or("/home"); format!("{home_base}/{}", opts.login) @@ -508,10 +521,10 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { // ------------------------------------------------------------------- // Begin mutations. From here, partial state is left on failure - // (matching GNU behavior). + // (matching GNU behavior). Locks are held throughout. // ------------------------------------------------------------------- - // Step 10: Create user group if needed. + // Step 11: Create user group if needed (lock already held). if let Some(ref new_grp) = new_group { write_new_group(&group_path, &mut group_entries, new_grp)?; if gshadow_path.exists() { @@ -519,7 +532,7 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { } } - // Step 11: Write /etc/passwd entry. + // Step 12: Write /etc/passwd entry (lock already held). let passwd_entry = PasswdEntry { name: opts.login.clone(), passwd: "x".to_string(), @@ -531,7 +544,11 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { }; write_passwd_entry(&passwd_path, &passwd_entries, &passwd_entry)?; - // Step 12: Write /etc/shadow entry. + // Release locks now that passwd and group writes are complete. + drop(group_lock); + drop(passwd_lock); + + // Step 13: Write /etc/shadow entry. let shadow_path = opts.root.shadow_path(); let shadow_entry = ShadowEntry { name: opts.login.clone(), @@ -546,17 +563,17 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { }; write_shadow_entry(&shadow_path, &shadow_entry)?; - // Step 13: Add to supplementary groups. + // Step 14: Add to supplementary groups. if !opts.groups.is_empty() { add_to_supplementary_groups(opts, &group_path, &gshadow_path)?; } - // Step 14: Create home directory and copy skel. + // Step 15: Create home directory and copy skel. if opts.create_home { create_home_directory(&home_dir, &opts.skel_dir, uid, gid)?; } - // Step 15: Invalidate nscd caches. + // Step 16: Invalidate nscd caches. nscd::invalidate_cache("passwd"); nscd::invalidate_cache("group"); @@ -665,15 +682,14 @@ fn resolve_group(gid_arg: &str, group_entries: &[GroupEntry]) -> Result, new_group: &GroupEntry, ) -> UResult<()> { - let _lock = FileLock::acquire(group_path) - .map_err(|e| UseraddError::CannotUpdateGroup(format!("{e}")))?; - group_entries.push(new_group.clone()); atomic::atomic_write(group_path, |f| group::write_group(group_entries, f)) @@ -682,15 +698,15 @@ fn write_new_group( Ok(()) } -/// Append a new gshadow entry to `/etc/gshadow` with proper locking. +/// Append a new gshadow entry to `/etc/gshadow`. +/// +/// Caller must hold the gshadow file lock (or the group file lock +/// if gshadow is protected by the same lock scheme). fn write_new_gshadow( gshadow_path: &Path, gshadow_entries: &mut Vec, new_group: &GroupEntry, ) -> UResult<()> { - let _lock = FileLock::acquire(gshadow_path) - .map_err(|e| UseraddError::CannotUpdateGroup(format!("{e}")))?; - gshadow_entries.push(GshadowEntry { name: new_group.name.clone(), passwd: "!".to_string(), @@ -704,15 +720,14 @@ fn write_new_gshadow( Ok(()) } -/// Append a new passwd entry to `/etc/passwd` with proper locking. +/// Append a new passwd entry to `/etc/passwd`. +/// +/// Caller must hold the passwd file lock. fn write_passwd_entry( passwd_path: &Path, existing: &[PasswdEntry], new_entry: &PasswdEntry, ) -> UResult<()> { - let _lock = FileLock::acquire(passwd_path) - .map_err(|e| UseraddError::CannotUpdatePasswd(format!("{e}")))?; - let mut entries: Vec = existing.to_vec(); entries.push(new_entry.clone()); diff --git a/src/uu/userdel/src/userdel.rs b/src/uu/userdel/src/userdel.rs index 64a4d91..9bd94ad 100644 --- a/src/uu/userdel/src/userdel.rs +++ b/src/uu/userdel/src/userdel.rs @@ -211,10 +211,12 @@ fn safe_remove_home(home: &Path) -> Result<(), UserdelError> { return Ok(()); } - // Check against protected paths. - let canonical = home.to_str().unwrap_or(""); + // Resolve symlinks and relative components so tricks like + // `/home/../etc` cannot bypass the protected-directory check. + let canonical = std::fs::canonicalize(home).unwrap_or_else(|_| home.to_owned()); + let canonical_str = canonical.to_string_lossy(); for &protected in PROTECTED_DIRS { - if canonical == protected { + if canonical_str == protected { return Err(UserdelError::CantRemoveHome(format!( "refusing to remove protected directory '{}'", home.display() diff --git a/src/uu/usermod/src/usermod.rs b/src/uu/usermod/src/usermod.rs index fc932b0..137886d 100644 --- a/src/uu/usermod/src/usermod.rs +++ b/src/uu/usermod/src/usermod.rs @@ -200,6 +200,17 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let mut ge = group::read_group_file(&group_path) .map_err(|e| UsermodError::CantUpdate(format!("{e}")))?; + // Validate all requested groups exist before mutating anything. + for gname in &new_groups { + if !ge.iter().any(|g| g.name == *gname) { + drop(glock); + return Err(UsermodError::CantUpdate(format!( + "group '{gname}' does not exist" + )) + .into()); + } + } + if !append { for g in &mut ge { g.members.retain(|m| m != login);