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

security: fix TOCTOU, temp permissions, PATH injection; improve Rust idioms

Browse Source 
Fixes found by Gemini code review:

Fixes #18 — lock: TOCTOU race eliminated via lock-via-link pattern.
  Write PID to temp file, hard_link() to lock path (atomic POSIX).
  Two processes detecting stale lock simultaneously can't both win.

Fixes #19 — atomic: temp file created with 0o600 from the start.
  Uses OpenOptions::create_new().mode() instead of File::create().
  No window where shadow hashes are world-readable.

Fixes #20 — nscd: absolute paths (/usr/sbin/nscd, /usr/sbin/sss_cache).
  Prevents PATH injection in setuid-root context.

Fixes #21 — error: thiserror derive replaces manual Display/Error impls.
  Less code, less room for mistakes. #[from] for io::Error, #[source]
  for IoPath.

Fixes #22 — parsers: splitn() iterator replaces Vec allocation.
  Zero heap allocation for field splitting on every line.

Fixes #23 — deny.toml: license allowlist + advisory scanning.
  cargo deny check passes (advisories ok, bans ok, licenses ok, sources ok).

142 tests passing on Debian/Alpine/Fedora, zero clippy warnings.
This commit is contained in:
Pierre Warnier
2026-03-23 14:25:19 +01:00
parent 3811dbb6d5
commit 42b501cf2a
7 changed files with 201 additions and 106 deletions
Download Patch File Download Diff File Expand all files Collapse all files
deny.toml
+27
View File
@@ -0,0 +1,27 @@
[advisories]
version = 2
yanked = "warn"
[licenses]
version = 2
allow = [
"MIT",
"Apache-2.0",
"ISC",
"BSD-2-Clause",
"BSD-3-Clause",
"CC0-1.0",
"Unicode-3.0",
"Zlib",
"MPL-2.0",
]
confidence-threshold = 0.8
[bans]
multiple-versions = "warn"
wildcards = "allow"
[sources]
unknown-registry = "warn"
unknown-git = "warn"
allow-registry = ["https://github.com/rust-lang/crates.io-index"]
src/shadow-core/src/atomic.rs
+12 -9
View File
@@ -13,6 +13,7 @@
use std::fs::{self, File};
use std::io::{self, Write};
use std::os::unix::fs::OpenOptionsExt;
use std::os::unix::io::AsRawFd;
use std::path::{Path, PathBuf};
@@ -37,16 +38,18 @@ where
let tmp_path = tmp_path_for(target);
let mut tmp_file =
File::create(&tmp_path).map_err(|e| ShadowError::IoPath(e, tmp_path.clone()))?;
// Determine permissions: preserve original if target exists, otherwise 0600.
// Set mode at creation time to avoid any window where the file is world-readable.
let mode = fs::metadata(target)
.map(|m| std::os::unix::fs::PermissionsExt::mode(&m.permissions()))
.unwrap_or(0o600);
// Preserve original file permissions if the target exists.
if let Ok(meta) = fs::metadata(target) {
let permissions = meta.permissions();
tmp_file
.set_permissions(permissions)
.map_err(|e| ShadowError::IoPath(e, tmp_path.clone()))?;
}
let mut tmp_file = std::fs::OpenOptions::new()
.write(true)
.create_new(true)
.mode(mode)
.open(&tmp_path)
.map_err(|e| ShadowError::IoPath(e, tmp_path.clone()))?;
let result = f(&mut tmp_file);
src/shadow-core/src/error.rs
+15 -39
View File
@@ -5,67 +5,43 @@
//! Unified error types for shadow-rs utilities.
use std::fmt;
use std::io;
use std::path::PathBuf;
use thiserror::Error;
/// Result type alias used across all shadow-rs utilities.
pub type ShadowResult<T> = Result<T, ShadowError>;
/// Top-level error type for shadow-rs operations.
#[derive(Debug)]
#[derive(Debug, Error)]
pub enum ShadowError {
/// I/O error with optional context.
Io(io::Error),
/// I/O error.
#[error("{0}")]
Io(#[from] io::Error),
/// I/O error with path context.
IoPath(io::Error, std::path::PathBuf),
#[error("{path}: {source}", path = .1.display(), source = .0)]
IoPath(#[source] io::Error, PathBuf),
/// File format parse error.
#[error("parse error: {0}")]
Parse(String),
/// Lock acquisition failed.
#[error("lock error: {0}")]
Lock(String),
/// Validation error (invalid username, UID range, etc.).
#[error("{0}")]
Validation(String),
/// Authentication error (PAM failure, wrong password, etc.).
#[error("authentication error: {0}")]
Auth(String),
/// Permission denied.
#[error("permission denied: {0}")]
Permission(String),
/// Generic error with message.
#[error("{0}")]
Other(String),
}
impl fmt::Display for ShadowError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::Io(e) => write!(f, "{e}"),
Self::IoPath(e, path) => write!(f, "{}: {e}", path.display()),
Self::Parse(msg) => write!(f, "parse error: {msg}"),
Self::Lock(msg) => write!(f, "lock error: {msg}"),
Self::Auth(msg) => write!(f, "authentication error: {msg}"),
Self::Permission(msg) => write!(f, "permission denied: {msg}"),
Self::Validation(msg) | Self::Other(msg) => write!(f, "{msg}"),
}
}
}
impl std::error::Error for ShadowError {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
match self {
Self::Io(e) | Self::IoPath(e, _) => Some(e),
Self::Parse(_)
| Self::Lock(_)
| Self::Validation(_)
| Self::Auth(_)
| Self::Permission(_)
| Self::Other(_) => None,
}
}
}
impl From<io::Error> for ShadowError {
fn from(e: io::Error) -> Self {
Self::Io(e)
}
}
/// Print an error message prefixed with the utility name to stderr.
#[macro_export]
macro_rules! show_error {
src/shadow-core/src/lock.rs
+63 -25
View File
@@ -14,12 +14,11 @@
use std::fs;
use std::io::Write;
use std::os::unix::fs::OpenOptionsExt;
use std::path::{Path, PathBuf};
use std::thread;
use std::time::{Duration, Instant};
use nix::fcntl::{self, OFlag};
use nix::sys::stat::Mode;
use nix::unistd;
use crate::error::ShadowError;
@@ -52,32 +51,65 @@ impl FileLock {
/// Acquire a lock with a custom timeout.
///
/// Uses the classic lock-via-link pattern to avoid TOCTOU races:
/// 1. Write our PID to a unique temp file
/// 2. Try to `hard_link` it to the lock path (atomic on POSIX)
/// 3. If link fails (lock exists), check for staleness and retry
///
/// Even if two processes both detect a stale lock and both remove it,
/// only one will succeed at the subsequent `hard_link`, so mutual
/// exclusion is never violated.
///
/// # Errors
///
/// Returns `ShadowError::Lock` if the lock cannot be acquired within the timeout.
pub fn acquire_with_timeout(file_path: &Path, timeout: Duration) -> Result<Self, ShadowError> {
let lock_path = lock_path_for(file_path);
let deadline = Instant::now() + timeout;
let tmp_path = tmp_lock_path(&lock_path);
// Write our PID to the temp file once, then try to link it in a loop.
write_pid_file(&tmp_path)?;
let result = Self::acquire_loop(&lock_path, &tmp_path, deadline);
// Always clean up our temp file, regardless of success or failure.
let _ = fs::remove_file(&tmp_path);
result
}
/// Inner acquisition loop. Separated so the caller can guarantee temp file cleanup.
fn acquire_loop(
lock_path: &Path,
tmp_path: &Path,
deadline: Instant,
) -> Result<Self, ShadowError> {
loop {
if try_create_lock(&lock_path).is_ok() {
// Attempt to hard-link our temp file to the lock path. hard_link is
// atomic: it either creates the destination or fails, so two
// processes can never both succeed for the same lock_path.
if fs::hard_link(tmp_path, lock_path).is_ok() {
return Ok(Self {
lock_path,
lock_path: lock_path.to_owned(),
released: false,
});
}
// Lock file exists — check if it's stale.
if is_stale_lock(&lock_path) {
// Remove stale lock and retry immediately.
let _ = fs::remove_file(&lock_path);
// Link failed — lock file exists. Check if it's stale.
if is_stale_lock(lock_path) {
// Remove the stale lock. If another process already removed it
// and re-acquired, our remove may fail or remove the wrong file,
// but the subsequent hard_link attempt is the real arbiter:
// it will fail atomically if someone else got there first.
let _ = fs::remove_file(lock_path);
continue;
}
if Instant::now() >= deadline {
return Err(ShadowError::Lock(format!(
"cannot acquire lock {}: timed out after {timeout:?}",
lock_path.display()
"cannot acquire lock {}: timed out",
lock_path.display(),
)));
}
@@ -116,21 +148,29 @@ fn lock_path_for(file_path: &Path) -> PathBuf {
PathBuf::from(lock)
}
/// Try to atomically create the lock file. Write our PID into it.
fn try_create_lock(lock_path: &Path) -> Result<(), ShadowError> {
// O_CREAT | O_EXCL ensures atomic creation — fails if the file exists.
let fd = fcntl::open(
lock_path,
OFlag::O_CREAT | OFlag::O_EXCL | OFlag::O_WRONLY,
Mode::from_bits_truncate(0o600),
)
.map_err(|e| ShadowError::Lock(format!("cannot create {}: {e}", lock_path.display())))?;
/// Compute a unique temp file path for the lock-via-link pattern.
///
/// Uses PID to avoid collisions between concurrent processes.
fn tmp_lock_path(lock_path: &Path) -> PathBuf {
let pid = std::process::id();
let mut tmp = lock_path.as_os_str().to_owned();
tmp.push(format!(".{pid}.tmp"));
PathBuf::from(tmp)
}
/// Write our PID to a temp file for later hard-linking.
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())))?;
// Write our PID for stale detection.
// SAFETY: fd is a valid file descriptor we just created.
let mut file = unsafe { std::fs::File::from_raw_fd(fd) };
let pid = unistd::getpid();
let _ = write!(file, "{pid}");
write!(file, "{pid}")
.map_err(|e| ShadowError::Lock(format!("cannot write {}: {e}", tmp_path.display())))?;
Ok(())
}
@@ -155,8 +195,6 @@ fn is_stale_lock(lock_path: &Path) -> bool {
nix::sys::signal::kill(pid, None).is_err()
}
use std::os::unix::io::FromRawFd;
#[cfg(test)]
mod tests {
use super::*;
src/shadow-core/src/nscd.rs
+6 -3
View File
@@ -19,8 +19,11 @@ use std::process::Command;
/// Silently succeeds if `nscd`/`sssd` is not installed or not running —
/// this matches GNU shadow-utils behavior.
pub fn invalidate_cache(database: &str) {
// nscd -i <database>
let _ = Command::new("nscd").arg("-i").arg(database).status();
// Use absolute paths to avoid PATH-based lookups in setuid context.
let _ = Command::new("/usr/sbin/nscd")
.arg("-i")
.arg(database)
.status();
// sssd: sss_cache with the appropriate flag
let flag = match database {
@@ -28,5 +31,5 @@ pub fn invalidate_cache(database: &str) {
"group" => "-G",
_ => return,
};
let _ = Command::new("sss_cache").arg(flag).status();
let _ = Command::new("/usr/sbin/sss_cache").arg(flag).status();
}
src/shadow-core/src/passwd.rs
+36 -15
View File
@@ -54,29 +54,50 @@ impl FromStr for PasswdEntry {
type Err = ShadowError;
fn from_str(line: &str) -> Result<Self, Self::Err> {
let fields: Vec<&str> = line.split(':').collect();
if fields.len() != 7 {
return Err(ShadowError::Parse(format!(
"expected 7 colon-separated fields, got {}",
fields.len()
)));
// Use splitn(8) to detect extra fields without allocating a Vec.
let mut fields = line.splitn(8, ':');
let name = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing name".into()))?;
let passwd = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing passwd".into()))?;
let uid_str = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing uid".into()))?;
let gid_str = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing gid".into()))?;
let gecos = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing gecos".into()))?;
let home = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing home".into()))?;
let shell = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing shell".into()))?;
if fields.next().is_some() {
return Err(ShadowError::Parse("too many fields".into()));
}
let uid = fields[2]
let uid = uid_str
.parse::<u32>()
.map_err(|e| ShadowError::Parse(format!("invalid UID '{}': {e}", fields[2])))?;
let gid = fields[3]
.map_err(|e| ShadowError::Parse(format!("invalid UID '{uid_str}': {e}")))?;
let gid = gid_str
.parse::<u32>()
.map_err(|e| ShadowError::Parse(format!("invalid GID '{}': {e}", fields[3])))?;
.map_err(|e| ShadowError::Parse(format!("invalid GID '{gid_str}': {e}")))?;
Ok(Self {
name: fields[0].to_string(),
passwd: fields[1].to_string(),
name: name.to_string(),
passwd: passwd.to_string(),
uid,
gid,
gecos: fields[4].to_string(),
home: fields[5].to_string(),
shell: fields[6].to_string(),
gecos: gecos.to_string(),
home: home.to_string(),
shell: shell.to_string(),
})
}
}
src/shadow-core/src/shadow.rs
+42 -15
View File
@@ -148,24 +148,51 @@ impl FromStr for ShadowEntry {
type Err = ShadowError;
fn from_str(line: &str) -> Result<Self, Self::Err> {
let fields: Vec<&str> = line.split(':').collect();
if fields.len() != 9 {
return Err(ShadowError::Parse(format!(
"expected 9 colon-separated fields, got {}",
fields.len()
)));
// Use splitn(10) to detect extra fields without allocating a Vec.
let mut fields = line.splitn(10, ':');
let name = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing name".into()))?;
let passwd = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing passwd".into()))?;
let last_change_str = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing last_change".into()))?;
let min_age_str = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing min_age".into()))?;
let max_age_str = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing max_age".into()))?;
let warn_days_str = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing warn_days".into()))?;
let inactive_days_str = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing inactive_days".into()))?;
let expire_date_str = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing expire_date".into()))?;
let reserved = fields
.next()
.ok_or_else(|| ShadowError::Parse("missing reserved".into()))?;
if fields.next().is_some() {
return Err(ShadowError::Parse("too many fields".into()));
}
Ok(Self {
name: fields[0].to_string(),
passwd: fields[1].to_string(),
last_change: parse_optional_field(fields[2])?,
min_age: parse_optional_field(fields[3])?,
max_age: parse_optional_field(fields[4])?,
warn_days: parse_optional_field(fields[5])?,
inactive_days: parse_optional_field(fields[6])?,
expire_date: parse_optional_field(fields[7])?,
reserved: fields[8].to_string(),
name: name.to_string(),
passwd: passwd.to_string(),
last_change: parse_optional_field(last_change_str)?,
min_age: parse_optional_field(min_age_str)?,
max_age: parse_optional_field(max_age_str)?,
warn_days: parse_optional_field(warn_days_str)?,
inactive_days: parse_optional_field(inactive_days_str)?,
expire_date: parse_optional_field(expire_date_str)?,
reserved: reserved.to_string(),
})
}
}