Merge pull request #46 from shadow-utils-rs/fix/43-45-openbsd-hardening

security: core dump suppression, RLIMIT_FSIZE, zero-length guard

Cargo.toml

@@ -38,7 +38,7 @@ uucore = "0.7"
 thiserror = "2"
 
 # Unix/Linux
-nix = { version = "0.29", features = ["user", "fs", "process", "signal", "term"] }
+nix = { version = "0.29", features = ["user", "fs", "process", "signal", "term", "resource"] }
 libc = "0.2"
 
 # Security

src/shadow-core/src/atomic.rs

@@ -20,6 +20,26 @@ use std::sync::atomic::{AtomicU64, Ordering};
 
 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.
+struct UmaskGuard(nix::sys::stat::Mode);
+
+impl UmaskGuard {
+    /// Set umask to zero and return a guard that restores the original.
+    fn zero() -> Self {
+        Self(nix::sys::stat::umask(nix::sys::stat::Mode::empty()))
+    }
+}
+
+impl Drop for UmaskGuard {
+    fn drop(&mut self) {
+        nix::sys::stat::umask(self.0);
+    }
+}
+
 /// Drop guard that auto-deletes a temporary file unless explicitly committed.
 ///
 /// Ensures the tmp file is cleaned up on any error path, including panics.
@@ -76,6 +96,11 @@ where
 
     let mut guard = TmpGuard::new(tmp_path.clone());
 
+    // Save and reset umask to ensure mode parameter is applied exactly.
+    // A caller could set a restrictive umask before invoking setuid passwd.
+    // The guard restores the original umask on any exit path.
+    let _umask = UmaskGuard::zero();
+
     let mut tmp_file = std::fs::OpenOptions::new()
         .write(true)
         .create_new(true)
@@ -85,6 +110,18 @@ where
 
     f(&mut tmp_file)?;
 
+    // Zero-length output guard: a zero-length shadow file locks out all users.
+    // OpenBSD checks this in pw_mkdb before replacing the original.
+    let written = tmp_file
+        .metadata()
+        .map_err(|e| ShadowError::IoPath(e, tmp_path.clone()))?
+        .len();
+    if written == 0 {
+        return Err(ShadowError::Other(
+            "refusing to write zero-length file".into(),
+        ));
+    }
+
     // Flush and fsync.
     tmp_file
         .flush()

src/shadow-core/src/lock.rs

@@ -268,6 +268,27 @@ mod tests {
         lock.release().unwrap();
     }
 
+    #[test]
+    fn test_lock_file_has_cloexec() {
+        use std::os::unix::io::AsRawFd;
+
+        // Rust's stdlib sets O_CLOEXEC by default on Linux.
+        // Verify the lock file FD won't leak to child processes.
+        let dir = tempfile::tempdir().expect("tempdir creation failed");
+        let file = dir.path().join("test_file");
+        fs::write(&file, "data").expect("failed to write test file");
+
+        let lock = FileLock::acquire(&file).expect("failed to acquire lock");
+
+        let f = fs::File::open(&lock.lock_path).expect("failed to open lock file");
+        let fd = f.as_raw_fd();
+        let flags =
+            nix::fcntl::fcntl(fd, nix::fcntl::FcntlArg::F_GETFD).expect("fcntl F_GETFD failed");
+        assert!(flags & libc::FD_CLOEXEC != 0, "FD should have CLOEXEC set");
+
+        lock.release().expect("failed to release lock");
+    }
+
     #[test]
     fn test_lock_file_contains_pid() {
         let dir = tempfile::tempdir().unwrap();

src/uu/passwd/src/passwd.rs

@@ -108,6 +108,41 @@ impl UError for PasswdError {
     }
 }
 
+// ---------------------------------------------------------------------------
+// Security hardening — process hardening
+// ---------------------------------------------------------------------------
+
+/// Suppress core dumps and prevent ptrace attachment.
+///
+/// OpenBSD's `pw_init()` sets `RLIMIT_CORE=0`. A core dump from a setuid
+/// passwd process could expose password hashes and plaintext passwords.
+fn suppress_core_dumps() {
+    // RLIMIT_CORE = 0: no core dumps. Best-effort — ignore errors.
+    let _ = nix::sys::resource::setrlimit(nix::sys::resource::Resource::RLIMIT_CORE, 0, 0);
+    // PR_SET_DUMPABLE = 0: prevent ptrace attachment and /proc/pid/mem reads.
+    // Linux-specific — silently skipped on other platforms.
+    #[cfg(target_os = "linux")]
+    {
+        // SAFETY: prctl with PR_SET_DUMPABLE is a simple flag set, no pointers.
+        unsafe {
+            libc::prctl(libc::PR_SET_DUMPABLE, 0);
+        }
+    }
+}
+
+/// Raise `RLIMIT_FSIZE` to prevent truncated file writes.
+///
+/// OpenBSD raises `RLIMIT_FSIZE` to infinity before file operations. A
+/// malicious caller could `ulimit -f 1` before invoking setuid passwd,
+/// causing /etc/shadow to be truncated mid-write.
+fn raise_file_size_limit() {
+    let _ = nix::sys::resource::setrlimit(
+        nix::sys::resource::Resource::RLIMIT_FSIZE,
+        nix::sys::resource::RLIM_INFINITY,
+        nix::sys::resource::RLIM_INFINITY,
+    );
+}
+
 // ---------------------------------------------------------------------------
 // Security hardening — environment sanitization
 // ---------------------------------------------------------------------------
@@ -171,6 +206,8 @@ fn apply_landlock_inner(_root: &SysRoot) {
 #[uucore::main]
 #[allow(clippy::too_many_lines)]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
+    suppress_core_dumps();
+    raise_file_size_limit();
     sanitize_env();
 
     let matches = match uu_app().try_get_matches_from(args) {
@@ -293,6 +330,18 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         });
     }
 
+    // Prevent non-root from targeting other users (avoids timing-based
+    // user enumeration through PAM auth failure timing).
+    if !caller_is_root() {
+        let current = get_current_username()?;
+        if current != target_user {
+            return Err(PasswdError::PermissionDenied(
+                "You may not view or modify password information for another user.".into(),
+            )
+            .into());
+        }
+    }
+
     // Default: password change via PAM.
     cmd_pam_change(&matches, &target_user)
 }
@@ -520,10 +569,39 @@ impl Drop for PrivDrop {
     }
 }
 
+/// Install a signal handler for SIGINT that prints "Password unchanged."
+/// and exits cleanly. This matches OpenBSD's kbintr pattern.
+///
+/// Only call this during interactive password input. The handler uses
+/// async-signal-safe functions only (_exit, write).
+fn install_interrupt_handler() {
+    // SAFETY: The handler uses only async-signal-safe operations
+    // (write to fd 2, _exit). No heap allocation or mutex locking.
+    unsafe {
+        let action = nix::sys::signal::SigAction::new(
+            nix::sys::signal::SigHandler::Handler(handle_interrupt),
+            nix::sys::signal::SaFlags::empty(),
+            nix::sys::signal::SigSet::empty(),
+        );
+        let _ = nix::sys::signal::sigaction(nix::sys::signal::Signal::SIGINT, &action);
+    }
+}
+
+extern "C" fn handle_interrupt(_sig: libc::c_int) {
+    // Async-signal-safe: only write() and _exit().
+    // SAFETY: Writing to stderr fd and exiting — both are signal-safe.
+    unsafe {
+        libc::write(2, b"\nPassword unchanged.\n".as_ptr().cast(), 21);
+        libc::_exit(0);
+    }
+}
+
 /// Default operation: change password via PAM.
 ///
 /// Feature-gated on `pam`. When PAM is not compiled in, prints an error.
 fn cmd_pam_change(matches: &clap::ArgMatches, _target_user: &str) -> UResult<()> {
+    install_interrupt_handler();
+
     let _keep_tokens = matches.get_flag(options::KEEP_TOKENS);
     let _use_stdin = matches.get_flag(options::STDIN);
     let _repository = matches.get_one::<String>(options::REPOSITORY);
@@ -753,6 +831,12 @@ fn mutate_shadow<F>(
 where
     F: FnOnce(&mut ShadowEntry) -> Result<(), String>,
 {
+    // Consolidate real + effective UID to root for file operations.
+    // Some filesystem configurations check real UID.
+    if nix::unistd::geteuid().is_root() {
+        let _ = nix::unistd::setuid(nix::unistd::Uid::from_raw(0));
+    }
+
     // Block signals for the entire critical section (lock → write → unlock).
     // The RAII guard restores the original signal mask when this function returns.
     let _signals = SignalBlocker::block_critical()?;
@@ -1477,6 +1561,50 @@ mod tests {
         );
     }
 
+    // -------------------------------------------------------------------
+    // OpenBSD hardening tests
+    // -------------------------------------------------------------------
+
+    #[test]
+    fn test_core_dump_suppression() {
+        // After calling suppress_core_dumps(), RLIMIT_CORE should be 0.
+        suppress_core_dumps();
+        let (soft, _hard) =
+            nix::sys::resource::getrlimit(nix::sys::resource::Resource::RLIMIT_CORE).unwrap();
+        assert_eq!(soft, 0, "RLIMIT_CORE should be 0 after suppression");
+    }
+
+    #[test]
+    fn test_raise_file_size_limit() {
+        raise_file_size_limit();
+        let (soft, _hard) =
+            nix::sys::resource::getrlimit(nix::sys::resource::Resource::RLIMIT_FSIZE).unwrap();
+        // In environments where the hard limit is already restricted (containers,
+        // CI), we may not reach RLIM_INFINITY. Verify it's at least very large.
+        assert!(
+            soft >= 1024 * 1024 * 1024 || soft == nix::sys::resource::RLIM_INFINITY,
+            "RLIMIT_FSIZE should be raised (got {soft})"
+        );
+    }
+
+    #[test]
+    fn test_zero_length_write_rejected() {
+        // atomic_write should refuse to replace a file with zero-length output.
+        let dir = tempfile::tempdir().unwrap();
+        let target = dir.path().join("shadow");
+        std::fs::write(&target, "original content\n").unwrap();
+
+        let result = shadow_core::atomic::atomic_write(&target, |_file| {
+            // Write nothing — zero-length output.
+            Ok(())
+        });
+
+        assert!(result.is_err(), "zero-length write should be rejected");
+        // Original file should be untouched.
+        let content = std::fs::read_to_string(&target).unwrap();
+        assert_eq!(content, "original content\n");
+    }
+
     #[test]
     fn test_mutation_with_aging_combined() {
         if skip_unless_root() {