From 0a33f2186da12f8d86127c031df532eb5a7b1acc Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Fri, 3 Apr 2026 15:27:58 +0200 Subject: [PATCH] review: address 5 Copilot review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix function name: sanitize_env() → sanitized_env() / harden_process() - Fix SIGINT handler: was removed, not implemented - Fix user enumeration: early permission check, not constant-time PAM - Fix Rust module paths: shadow-core → shadow_core (crate vs module) --- CONTRIBUTING.md | 2 +- docs/OPENBSD-REFERENCE.md | 6 +++--- docs/SECURITY-HARDENING.md | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6b0bb86..bb2367e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -92,7 +92,7 @@ Utilities must be embeddable. Return `UResult<()>` from `uumain`. The ### `unsafe` Denied at the workspace level (`unsafe_code = "deny"`). Only two FFI boundary -modules are exempted: `shadow-core::pam` (PAM C library) and `shadow-core::crypt` +modules are exempted: `shadow_core::pam` (PAM C library) and `shadow_core::crypt` (POSIX crypt(3)). Every `unsafe` block must have a `// SAFETY:` comment. ### `str`, `OsStr` & `Path` diff --git a/docs/OPENBSD-REFERENCE.md b/docs/OPENBSD-REFERENCE.md index 9c9916a..0b22c9f 100644 --- a/docs/OPENBSD-REFERENCE.md +++ b/docs/OPENBSD-REFERENCE.md @@ -11,7 +11,7 @@ Source: `cvsweb.openbsd.org/src/usr.bin/passwd/` and `src/lib/libutil/passwd.c`. |---|---------|--------| | Signal blocking during file writes | #38 — `SignalBlocker` RAII | | Privilege drop during PAM conversation | #39 — `PrivDrop` RAII | -| Environment sanitization | #40 — `sanitize_env()` | +| Environment sanitization | #40 — `sanitized_env()` / `harden_process()` | | Landlock filesystem restriction | #41 — `apply_landlock()` in passwd | | Absolute paths for subprocesses | #20 — `/usr/sbin/nscd` | | Password zeroing | #7 — `zeroize` crate | @@ -21,8 +21,8 @@ Source: `cvsweb.openbsd.org/src/usr.bin/passwd/` and `src/lib/libutil/passwd.c`. | Resource limit hardening | #44 — `raise_file_size_limit()` in hardening.rs | | Zero-length output guard | #45 — in `atomic_write` | | setuid(0) consolidation | #47 — before file operations in passwd | -| User enumeration prevention | #49 — constant-time PAM in passwd | -| SIGINT handler during password input | #48 — "Password unchanged" + terminal restore | +| User enumeration prevention | #49 — early permission check in passwd | +| Password input interrupt handling | #48 — custom SIGINT handler removed; signal blocking covers critical sections | | Umask reset | #51 — `UmaskGuard` RAII in lock/tmp creation | ### Not Yet Implemented diff --git a/docs/SECURITY-HARDENING.md b/docs/SECURITY-HARDENING.md index c40fd77..b4042de 100644 --- a/docs/SECURITY-HARDENING.md +++ b/docs/SECURITY-HARDENING.md @@ -14,14 +14,14 @@ Techniques adopted from OpenBSD and best practices for setuid-root tools. - [x] PAM delegation (no custom password hashing) - [x] `TmpGuard` drop pattern (no leaked temp files) - [x] Signal blocking during file writes (#38 — `SignalBlocker` RAII) -- [x] Environment sanitization (#40 — `sanitize_env()`) +- [x] Environment sanitization (#40 — `sanitized_env()` / `harden_process()`) - [x] Privilege drop during PAM conversation (#39 — `PrivDrop` RAII) - [x] Core dump suppression (#43 — `suppress_core_dumps()`) - [x] Resource limit hardening (#44 — `raise_file_size_limit()`) - [x] Zero-length output guard (#45 — in `atomic_write`) - [x] setuid(0) consolidation (#47 — before file operations) -- [x] SIGINT handler during password input (#48 — "Password unchanged") -- [x] User enumeration prevention (#49 — constant-time PAM) +- [x] Password input interrupt handling (#48 — custom SIGINT handler removed; signal blocking during file writes covers critical sections) +- [x] User enumeration prevention (#49 — early permission check for non-root callers) - [x] O_CLOEXEC on file descriptors (#50) - [x] Umask reset (#51 — `UmaskGuard` RAII) - [x] Landlock filesystem restriction (#41 — `apply_landlock()` in passwd)