From 1d8921abeef22869788af41b2865c428b468749b Mon Sep 17 00:00:00 2001 From: nevaforget Date: Mon, 30 Mar 2026 13:09:02 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20audit=20fixes=20=E2=80=94=20blur=20offse?= =?UTF-8?q?t,=20lock-before-IO,=20FP=20signal=20lifecycle,=20TOCTOU=20(v0.?= =?UTF-8?q?6.6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third triple audit (quality, performance, security). Key fixes: - Blur padding offset: texture at (-pad,-pad) prevents edge darkening on all sides - Wallpaper loads after lock.lock() — disk I/O no longer delays lock acquisition - begin_verification disconnects old signal handler before registering new one - resume_async resets failed_attempts to prevent premature exhaustion - Unknown VerifyStatus with done=true triggers restart instead of hanging - symlink_metadata() replaces separate is_file()+is_symlink() (TOCTOU) - faillock_warning dead code removed, blur sigma clamped to [0,100] - Redundant Zeroizing> removed, on_verify_status restricted to pub(crate) - Warn logging for non-UTF-8 GECOS and avatar path errors - Default impl for FingerprintListener, 3 new tests (47 total) --- CLAUDE.md | 13 +++++++------ Cargo.toml | 2 +- DECISIONS.md | 7 +++++++ src/auth.rs | 17 ++++++++++++++--- src/config.rs | 8 ++++++-- src/fingerprint.rs | 31 ++++++++++++++++++++++++------- src/i18n.rs | 23 ++++++++++++++++++++--- src/lockscreen.rs | 15 ++++++++++++--- src/main.rs | 19 +++++++++++-------- src/users.rs | 18 ++++++++++++++---- 10 files changed, 116 insertions(+), 37 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9837a8f..eb9d512 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -37,14 +37,14 @@ LD_PRELOAD=/usr/lib/libgtk4-layer-shell.so ./target/release/moonlock ## Architektur -- `auth.rs` — PAM-Authentifizierung via Raw FFI (unsafe extern "C" conv callback, msg_style-aware, Zeroizing> + Zeroizing), check_account() für pam_acct_mgmt-Only-Checks nach Fingerprint-Unlock -- `fingerprint.rs` — fprintd D-Bus Listener, async init/claim/verify via gio futures, sender-validated signal handler, cleanup_dbus() für sauberen D-Bus-Lifecycle, running_flag für Race-Safety in async restarts, on_exhausted callback after MAX_FP_ATTEMPTS, resume_async() für Neustart nach transientem Fehler +- `auth.rs` — PAM-Authentifizierung via Raw FFI (unsafe extern "C" conv callback, msg_style-aware, Zeroizing), check_account() für pam_acct_mgmt-Only-Checks nach Fingerprint-Unlock +- `fingerprint.rs` — fprintd D-Bus Listener, async init/claim/verify via gio futures, sender-validated signal handler, cleanup_dbus() für sauberen D-Bus-Lifecycle, running_flag für Race-Safety in async restarts, on_exhausted callback after MAX_FP_ATTEMPTS, resume_async() für Neustart nach transientem Fehler (mit failed_attempts-Reset und Signal-Handler-Cleanup) - `users.rs` — Aktuellen User via nix getuid, Avatar-Loading mit Symlink-Rejection - `power.rs` — Reboot/Shutdown via /usr/bin/systemctl - `i18n.rs` — Locale-Erkennung (OnceLock-cached) und String-Tabellen (DE/EN), faillock_warning mit konfigurierbarem max_attempts -- `config.rs` — TOML-Config (background_path, background_blur, fingerprint_enabled als Option) + Wallpaper-Fallback + Symlink-Rejection für background_path + Parse-Error-Logging +- `config.rs` — TOML-Config (background_path, background_blur clamped [0,100], fingerprint_enabled als Option) + Wallpaper-Fallback + Symlink-Rejection via symlink_metadata + Parse-Error-Logging - `lockscreen.rs` — GTK4 UI via LockscreenHandles, PAM-Auth via gio::spawn_blocking mit 30s Timeout und Generation Counter, FP-Label/Start separat verdrahtet mit pam_acct_mgmt-Check und auto-resume, Zeroizing für Passwort, Power-Confirm, GPU-Blur via GskBlurNode (Downscale auf max 1920px), Blur/Avatar-Cache für Multi-Monitor -- `main.rs` — Entry Point, Panic-Hook (vor Logging), Root-Check, ext-session-lock-v1 (Pflicht in Release), Multi-Monitor mit shared Blur/Avatar-Caches, systemd-Journal-Logging, Debug-Level per `MOONLOCK_DEBUG` Env-Var, async fprintd-Init nach window.present() +- `main.rs` — Entry Point, Panic-Hook (vor Logging), Root-Check, ext-session-lock-v1 (Pflicht in Release), Multi-Monitor mit shared Blur/Avatar-Caches, systemd-Journal-Logging, Debug-Level per `MOONLOCK_DEBUG` Env-Var, async fprintd-Init nach window.present(), Wallpaper-Laden nach lock() ## Sicherheit @@ -53,8 +53,9 @@ LD_PRELOAD=/usr/lib/libgtk4-layer-shell.so ./target/release/moonlock - Panic-Hook: Bei Crash wird geloggt, aber NIEMALS unlock() aufgerufen — Screen bleibt schwarz. Hook wird vor Logging installiert. - PAM-Callback: msg_style-aware (Passwort nur bei PAM_PROMPT_ECHO_OFF), strdup-OOM-sicher, num_msg-Guard gegen negative Werte - fprintd: D-Bus Signal-Sender wird gegen fprintd's unique bus name validiert (Anti-Spoofing) -- Passwort: Zeroizing ab GTK-Entry-Extraktion, Zeroizing> + Zeroizing im PAM-FFI-Layer (bekannte Einschränkung: GLib-GString und strdup-Kopie in PAM werden nicht gezeroized — inhärente GTK/libc-Limitierung) -- Fingerprint-Unlock: pam_acct_mgmt-Check nach verify-match erzwingt Account-Policies (Lockout, Ablauf), resume_async() startet FP bei transientem Fehler neu +- Passwort: Zeroizing ab GTK-Entry-Extraktion, Zeroizing im PAM-FFI-Layer (bekannte Einschränkung: GLib-GString und strdup-Kopie in PAM werden nicht gezeroized — inhärente GTK/libc-Limitierung) +- Fingerprint-Unlock: pam_acct_mgmt-Check nach verify-match erzwingt Account-Policies (Lockout, Ablauf), resume_async() startet FP bei transientem Fehler neu (mit failed_attempts-Reset und Signal-Handler-Cleanup) +- Wallpaper wird nach lock() geladen — Disk-I/O verzögert nicht die Lock-Akquisition - PAM-Timeout: 30s Timeout verhindert permanentes Aussperren bei hängenden PAM-Modulen, Generation Counter verhindert Interferenz paralleler Auth-Versuche - Root-Check: Exit mit Fehler wenn als root gestartet - Faillock: UI-Warnung nach 3 Fehlversuchen, aber PAM entscheidet über Lockout (Entry bleibt aktiv) diff --git a/Cargo.toml b/Cargo.toml index 26639d4..6b70fee 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "moonlock" -version = "0.6.5" +version = "0.6.6" edition = "2024" description = "A secure Wayland lockscreen with GTK4, PAM and fingerprint support" license = "MIT" diff --git a/DECISIONS.md b/DECISIONS.md index 4317514..67873d0 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -2,6 +2,13 @@ Architectural and design decisions for Moonlock, in reverse chronological order. +## 2026-03-30 – Third audit: blur offset, lock-before-IO, FP signal lifecycle, TOCTOU + +- **Who**: Nyx, Dom +- **Why**: Third triple audit (quality, performance, security) found: blur padding offset rendering texture at (0,0) instead of (-pad,-pad) causing edge darkening on left/top (BUG), wallpaper disk I/O blocking before lock() extending the unsecured window (PERF/SEC), signal handler duplication on resume_async (SEC), failed_attempts not reset on FP resume (SEC), unknown VerifyStatus with done=false hanging FP listener (SEC), TOCTOU in is_file+is_symlink checks (SEC), dead code in faillock_warning (QUALITY), unbounded blur sigma (SEC). +- **Tradeoffs**: Wallpaper loads after lock() — screen briefly shows without wallpaper until texture is ready. Acceptable: security > aesthetics. Blur sigma clamped to [0.0, 100.0] — arbitrary upper bound but prevents GPU memory exhaustion. +- **How**: (1) Texture offset to (-pad, -pad) in render_blurred_texture. (2) lock.lock() before resolve_background_path. (3) begin_verification disconnects old signal_id before registering new. (4) resume_async resets failed_attempts. (5) Unknown VerifyStatus with done=true triggers restart. (6) symlink_metadata() for atomic file+symlink check. (7) faillock_warning dead code removed, saturating_sub. (8) background_blur clamped. (9) Redundant Zeroizing> removed. (10) Default impl for FingerprintListener. (11) on_verify_status restricted to pub(crate). (12) Warn logging for non-UTF-8 GECOS and avatar paths. + ## 2026-03-30 – Second audit: zeroize CString, FP account check, PAM timeout, blur downscale - **Who**: Nyx, Dom diff --git a/src/auth.rs b/src/auth.rs index b9c25b3..3dc078f 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -149,9 +149,8 @@ unsafe extern "C" fn pam_conv_callback( /// Returns true on success, false on authentication failure. /// The password is wiped from memory after use via zeroize. pub fn authenticate(username: &str, password: &str) -> bool { - // Use Zeroizing to ensure password bytes are wiped on drop - let password_bytes = Zeroizing::new(password.as_bytes().to_vec()); - let password_cstr = match CString::new(password_bytes.as_slice()) { + // CString::new takes ownership of the Vec — Zeroizing wipes on drop + let password_cstr = match CString::new(password.as_bytes().to_vec()) { Ok(c) => Zeroizing::new(c), Err(_) => return false, // Password contains null byte }; @@ -286,4 +285,16 @@ mod tests { let result = authenticate("", "password"); assert!(!result); } + + #[test] + fn check_account_empty_username_fails() { + let result = check_account(""); + assert!(!result); + } + + #[test] + fn check_account_null_byte_username_fails() { + let result = check_account("user\0name"); + assert!(!result); + } } diff --git a/src/config.rs b/src/config.rs index c190b1e..b3aefe3 100644 --- a/src/config.rs +++ b/src/config.rs @@ -51,7 +51,9 @@ pub fn load_config(config_paths: Option<&[PathBuf]>) -> Config { match toml::from_str::(&content) { Ok(parsed) => { if parsed.background_path.is_some() { merged.background_path = parsed.background_path; } - if parsed.background_blur.is_some() { merged.background_blur = parsed.background_blur; } + if let Some(blur) = parsed.background_blur { + merged.background_blur = Some(blur.clamp(0.0, 100.0)); + } if let Some(fp) = parsed.fingerprint_enabled { merged.fingerprint_enabled = fp; } } Err(e) => { @@ -70,7 +72,9 @@ pub fn resolve_background_path(config: &Config) -> Option { pub fn resolve_background_path_with(config: &Config, moonarch_wallpaper: &Path) -> Option { if let Some(ref bg) = config.background_path { let path = PathBuf::from(bg); - if path.is_file() && !path.is_symlink() { return Some(path); } + if let Ok(meta) = path.symlink_metadata() { + if meta.is_file() && !meta.file_type().is_symlink() { return Some(path); } + } } if moonarch_wallpaper.is_file() { return Some(moonarch_wallpaper.to_path_buf()); } None diff --git a/src/fingerprint.rs b/src/fingerprint.rs index 96ed0da..cf80f77 100644 --- a/src/fingerprint.rs +++ b/src/fingerprint.rs @@ -35,10 +35,8 @@ pub struct FingerprintListener { on_exhausted: Option>, } -impl FingerprintListener { - /// Create a lightweight FingerprintListener without any D-Bus calls. - /// Call `init_async().await` afterwards to connect to fprintd. - pub fn new() -> Self { +impl Default for FingerprintListener { + fn default() -> Self { FingerprintListener { device_proxy: None, signal_id: None, @@ -50,6 +48,14 @@ impl FingerprintListener { on_exhausted: None, } } +} + +impl FingerprintListener { + /// Create a lightweight FingerprintListener without any D-Bus calls. + /// Call `init_async().await` afterwards to connect to fprintd. + pub fn new() -> Self { + Self::default() + } /// Connect to fprintd and get the default device asynchronously. pub async fn init_async(&mut self) { @@ -171,6 +177,7 @@ impl FingerprintListener { listener: &Rc>, username: &str, ) { + listener.borrow_mut().failed_attempts = 0; Self::begin_verification(listener, username).await; } @@ -181,11 +188,16 @@ impl FingerprintListener { username: &str, ) { let proxy = { - let inner = listener.borrow(); - match inner.device_proxy.clone() { + let mut inner = listener.borrow_mut(); + let proxy = match inner.device_proxy.clone() { Some(p) => p, None => return, + }; + // Disconnect any previous signal handler to prevent duplicates on resume + if let Some(old_id) = inner.signal_id.take() { + proxy.disconnect(old_id); } + proxy }; // Claim the device @@ -265,7 +277,7 @@ impl FingerprintListener { } /// Process a VerifyStatus signal from fprintd. - pub fn on_verify_status(&mut self, status: &str, done: bool) { + pub(crate) fn on_verify_status(&mut self, status: &str, done: bool) { if !self.running { return; } @@ -305,6 +317,9 @@ impl FingerprintListener { } log::debug!("Unhandled fprintd status: {status}"); + if done { + self.restart_verify_async(); + } } /// Restart fingerprint verification asynchronously after a completed attempt. @@ -403,11 +418,13 @@ mod tests { let called_clone = called.clone(); let mut listener = FingerprintListener::new(); listener.running = true; + listener.running_flag.set(true); listener.on_failure = Some(Box::new(move || { called_clone.set(true); })); listener.on_verify_status("verify-no-match", false); assert!(called.get()); assert!(listener.running); + assert!(listener.running_flag.get()); assert_eq!(listener.failed_attempts, 1); } diff --git a/src/i18n.rs b/src/i18n.rs index b143d0b..77284bf 100644 --- a/src/i18n.rs +++ b/src/i18n.rs @@ -100,9 +100,10 @@ pub fn load_strings(locale: Option<&str>) -> &'static Strings { match locale { "de" => &STRINGS_DE, _ => &STRINGS_EN } } +/// Returns a warning when the user is one attempt away from lockout. +/// Caller is responsible for handling the locked state (count >= max_attempts). pub fn faillock_warning(attempt_count: u32, max_attempts: u32, strings: &Strings) -> Option { - if attempt_count >= max_attempts { return Some(strings.faillock_locked.to_string()); } - let remaining = max_attempts - attempt_count; + let remaining = max_attempts.saturating_sub(attempt_count); if remaining == 1 { return Some(strings.faillock_attempts_remaining.replace("{n}", &remaining.to_string())); } None } @@ -139,5 +140,21 @@ mod tests { #[test] fn faillock_zero() { assert!(faillock_warning(0, 3, load_strings(Some("en"))).is_none()); } #[test] fn faillock_one() { assert!(faillock_warning(1, 3, load_strings(Some("en"))).is_none()); } #[test] fn faillock_two() { assert!(faillock_warning(2, 3, load_strings(Some("en"))).is_some()); } - #[test] fn faillock_three() { assert_eq!(faillock_warning(3, 3, load_strings(Some("en"))).unwrap(), "Account may be locked"); } + #[test] fn faillock_three() { assert!(faillock_warning(3, 3, load_strings(Some("en"))).is_none()); } + + #[test] + fn faillock_caller_contract() { + // Mirrors the lockscreen.rs usage: caller handles count >= max separately, + // faillock_warning is only called when count < max. + let max = 3u32; + let strings = load_strings(Some("en")); + for count in 0..max { + let result = faillock_warning(count, max, strings); + if max - count == 1 { + assert!(result.is_some(), "should warn at count={count}"); + } else { + assert!(result.is_none(), "should not warn at count={count}"); + } + } + } } diff --git a/src/lockscreen.rs b/src/lockscreen.rs index 20eb3f4..1ad04db 100644 --- a/src/lockscreen.rs +++ b/src/lockscreen.rs @@ -609,7 +609,7 @@ fn render_blurred_texture( snapshot.push_clip(&graphene::Rect::new(pad, pad, w, h)); snapshot.push_blur(scaled_sigma as f64); // Render texture with padding on all sides (edges repeat via oversized bounds) - snapshot.append_texture(texture, &graphene::Rect::new(0.0, 0.0, w + 2.0 * pad, h + 2.0 * pad)); + snapshot.append_texture(texture, &graphene::Rect::new(-pad, -pad, w + 2.0 * pad, h + 2.0 * pad)); snapshot.pop(); // blur snapshot.pop(); // clip @@ -624,13 +624,22 @@ fn set_avatar_from_file( path: &Path, cache: &Rc>>, ) { - match Pixbuf::from_file_at_scale(path.to_str().unwrap_or(""), AVATAR_SIZE, AVATAR_SIZE, true) { + let path_str = match path.to_str() { + Some(s) => s, + None => { + log::warn!("Avatar path is not valid UTF-8: {:?}", path); + image.set_icon_name(Some("avatar-default-symbolic")); + return; + } + }; + match Pixbuf::from_file_at_scale(path_str, AVATAR_SIZE, AVATAR_SIZE, true) { Ok(pixbuf) => { let texture = gdk::Texture::for_pixbuf(&pixbuf); image.set_paintable(Some(&texture)); *cache.borrow_mut() = Some(texture); } - Err(_) => { + Err(e) => { + log::warn!("Failed to load avatar from {:?}: {e}", path); image.set_icon_name(Some("avatar-default-symbolic")); } } diff --git a/src/main.rs b/src/main.rs index c9419e8..f50dade 100644 --- a/src/main.rs +++ b/src/main.rs @@ -40,16 +40,14 @@ fn activate(app: >k::Application) { load_css(&display); let config = config::load_config(None); - let bg_texture = config::resolve_background_path(&config) - .and_then(|path| lockscreen::load_background_texture(&path)); if gtk4_session_lock::is_supported() { - activate_with_session_lock(app, &display, bg_texture.as_ref(), &config); + activate_with_session_lock(app, &display, &config); } else { #[cfg(debug_assertions)] { log::warn!("ext-session-lock-v1 not supported — running in development mode"); - activate_without_lock(app, bg_texture.as_ref(), &config); + activate_without_lock(app, &config); } #[cfg(not(debug_assertions))] { @@ -62,12 +60,15 @@ fn activate(app: >k::Application) { fn activate_with_session_lock( app: >k::Application, display: &gdk::Display, - bg_texture: Option<&gdk::Texture>, config: &config::Config, ) { let lock = gtk4_session_lock::Instance::new(); lock.lock(); + // Load wallpaper AFTER lock — disk I/O must not delay the lock acquisition + let bg_texture = config::resolve_background_path(config) + .and_then(|path| lockscreen::load_background_texture(&path)); + let monitors = display.monitors(); // Shared unlock callback — unlocks session and quits. @@ -99,7 +100,7 @@ fn activate_with_session_lock( .and_then(|obj| obj.downcast::().ok()) { let handles = lockscreen::create_lockscreen_window( - bg_texture, + bg_texture.as_ref(), config, app, unlock_callback.clone(), @@ -158,9 +159,11 @@ fn init_fingerprint_async(all_handles: Vec) { #[cfg(debug_assertions)] fn activate_without_lock( app: >k::Application, - bg_texture: Option<&gdk::Texture>, config: &config::Config, ) { + let bg_texture = config::resolve_background_path(config) + .and_then(|path| lockscreen::load_background_texture(&path)); + let app_clone = app.clone(); let unlock_callback: Rc = Rc::new(move || { app_clone.quit(); @@ -169,7 +172,7 @@ fn activate_without_lock( let blur_cache = Rc::new(RefCell::new(None)); let avatar_cache = Rc::new(RefCell::new(None)); let handles = lockscreen::create_lockscreen_window( - bg_texture, + bg_texture.as_ref(), config, app, unlock_callback, diff --git a/src/users.rs b/src/users.rs index 17b4a7b..39a2532 100644 --- a/src/users.rs +++ b/src/users.rs @@ -18,7 +18,13 @@ pub struct User { pub fn get_current_user() -> Option { let uid = getuid(); let nix_user = NixUser::from_uid(uid).ok()??; - let gecos = nix_user.gecos.to_str().unwrap_or("").to_string(); + let gecos = match nix_user.gecos.to_str() { + Ok(s) => s.to_string(), + Err(_) => { + log::warn!("GECOS field is not valid UTF-8, falling back to username"); + String::new() + } + }; let display_name = if !gecos.is_empty() { let first = gecos.split(',').next().unwrap_or(""); if first.is_empty() { nix_user.name.clone() } else { first.to_string() } @@ -31,13 +37,17 @@ pub fn get_avatar_path(home: &Path, username: &str) -> Option { } pub fn get_avatar_path_with(home: &Path, username: &str, accountsservice_dir: &Path) -> Option { - // ~/.face takes priority + // ~/.face takes priority — single stat via symlink_metadata to avoid TOCTOU let face = home.join(".face"); - if face.exists() && !face.is_symlink() { return Some(face); } + if let Ok(meta) = face.symlink_metadata() { + if meta.is_file() && !meta.file_type().is_symlink() { return Some(face); } + } // AccountsService icon if accountsservice_dir.exists() { let icon = accountsservice_dir.join(username); - if icon.exists() && !icon.is_symlink() { return Some(icon); } + if let Ok(meta) = icon.symlink_metadata() { + if meta.is_file() && !meta.file_type().is_symlink() { return Some(icon); } + } } None }