diff --git a/Cargo.lock b/Cargo.lock index 0e9d0c8..1c9e10d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -575,7 +575,7 @@ dependencies = [ [[package]] name = "moonlock" -version = "0.6.9" +version = "0.6.11" dependencies = [ "gdk-pixbuf", "gdk4", diff --git a/Cargo.toml b/Cargo.toml index ef29185..738124f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "moonlock" -version = "0.6.10" +version = "0.6.11" edition = "2024" description = "A secure Wayland lockscreen with GTK4, PAM and fingerprint support" license = "MIT" diff --git a/DECISIONS.md b/DECISIONS.md index 0d1d0c6..c782a7a 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -2,6 +2,13 @@ Architectural and design decisions for Moonlock, in reverse chronological order. +## 2026-04-24 – Audit MEDIUM fixes: D-Bus cleanup race, TOCTOU open, FP reset, GTK entry clear (v0.6.11) + +- **Who**: ClaudeCode, Dom +- **Why**: Second round after the HIGH fixes, addressing the four MEDIUM findings. (1) `cleanup_dbus` spawned VerifyStop + Release as fire-and-forget, then `resume_async` called Claim after only a 2 s timeout — shorter than the 3 s D-Bus timeout, so on a slow bus the Claim could race the Release and fprintd would reject it, leaving the FP listener permanently dead. (2) `load_background_texture` relied on the caller's `symlink_metadata` check, re-opening the path via `gdk::Texture::from_file` — a classic TOCTOU window. (3) `resume_async` unconditionally reset `failed_attempts`, allowing an attacker with sensor control to evade the 10-attempt cap by cycling verify-match → `check_account` fail → resume. (4) The GTK `PasswordEntry` buffer was only cleared on timeout or auth failure, leaving the password in GLib malloc'd memory longer than necessary. +- **Tradeoffs**: The D-Bus cleanup is now split into a synchronous helper (`take_cleanup_proxy` — signal disconnect + flag clear) and an async helper (`perform_dbus_cleanup` — VerifyStop + Release), so `resume_async` can await the release while `stop()` stays fire-and-forget. Dropping the `failed_attempts` reset means a flaky sensor could reach 10 failures faster, but the correct remedy is a new lock session (construction) rather than a reset that also helps attackers. +- **How**: (1) Split `cleanup_dbus` into `take_cleanup_proxy()` (sync) + `perform_dbus_cleanup(proxy)` (async). `resume_async` now awaits `perform_dbus_cleanup` before `begin_verification`. `stop()` still spawns the cleanup fire-and-forget. (2) `load_background_texture` opens with `O_NOFOLLOW` via `std::fs::OpenOptions::custom_flags`, reads to bytes, and builds the texture via `gdk::Texture::from_bytes`. (3) Removed `listener.borrow_mut().failed_attempts = 0` from `resume_async`. (4) `password_entry.set_text("")` now fires right after the `Zeroizing::new(entry.text().to_string())` extraction, shortening the GTK-side window. + ## 2026-04-24 – Audit fixes: RefCell borrow across await, async avatar decode - **Who**: ClaudeCode, Dom diff --git a/src/fingerprint.rs b/src/fingerprint.rs index 080c3cb..5bed210 100644 --- a/src/fingerprint.rs +++ b/src/fingerprint.rs @@ -177,12 +177,24 @@ impl FingerprintListener { /// Resume fingerprint verification after a transient interruption (e.g. failed /// PAM account check). Reuses previously stored callbacks. Re-claims the device - /// and restarts verification from scratch. + /// and restarts verification from scratch. Awaits any in-flight VerifyStop + + /// Release before re-claiming the device so fprintd does not reject the Claim + /// while the previous session is still being torn down. pub async fn resume_async( listener: &Rc>, username: &str, ) { - listener.borrow_mut().failed_attempts = 0; + // Drain in-flight cleanup so the device is actually released before Claim. + // Without this, a fast resume after on_verify_status's fire-and-forget + // cleanup races the Release call and fprintd returns "already claimed". + let proxy = listener.borrow_mut().take_cleanup_proxy(); + if let Some(proxy) = proxy { + Self::perform_dbus_cleanup(proxy).await; + } + // Deliberately do NOT reset failed_attempts here. An attacker with sensor + // control could otherwise cycle verify-match → check_account fail → resume, + // and the 10-attempt cap would never trigger. The counter decays only via + // a fresh lock session (listener construction). Self::begin_verification(listener, username).await; } @@ -352,26 +364,37 @@ impl FingerprintListener { } } - /// Disconnect the signal handler and send VerifyStop + Release to fprintd. - /// Signal disconnect is synchronous to prevent further callbacks. - /// D-Bus cleanup is fire-and-forget to avoid blocking the UI. - fn cleanup_dbus(&mut self) { + /// Disconnect the signal handler and clear running flags. Returns the proxy + /// the caller should use for the async D-Bus cleanup (VerifyStop + Release). + /// + /// Split into a sync part (signal disconnect, flags) and an async part + /// (`perform_dbus_cleanup`) so callers can either spawn the async work + /// fire-and-forget (via `cleanup_dbus`) or await it to serialize with a + /// subsequent Claim (via `resume_async`). + fn take_cleanup_proxy(&mut self) -> Option { self.running = false; self.running_flag.set(false); - if let Some(ref proxy) = self.device_proxy { - if let Some(id) = self.signal_id.take() { - proxy.disconnect(id); - } - let proxy = proxy.clone(); - glib::spawn_future_local(async move { - let _ = proxy - .call_future("VerifyStop", None, gio::DBusCallFlags::NONE, DBUS_TIMEOUT_MS) - .await; - let _ = proxy - .call_future("Release", None, gio::DBusCallFlags::NONE, DBUS_TIMEOUT_MS) - .await; - }); + let proxy = self.device_proxy.clone()?; + if let Some(id) = self.signal_id.take() { + proxy.disconnect(id); + } + Some(proxy) + } + + async fn perform_dbus_cleanup(proxy: gio::DBusProxy) { + let _ = proxy + .call_future("VerifyStop", None, gio::DBusCallFlags::NONE, DBUS_TIMEOUT_MS) + .await; + let _ = proxy + .call_future("Release", None, gio::DBusCallFlags::NONE, DBUS_TIMEOUT_MS) + .await; + } + + /// Fire-and-forget cleanup for code paths that cannot await (e.g. drop, stop). + fn cleanup_dbus(&mut self) { + if let Some(proxy) = self.take_cleanup_proxy() { + glib::spawn_future_local(Self::perform_dbus_cleanup(proxy)); } } diff --git a/src/lockscreen.rs b/src/lockscreen.rs index e000075..3199ab8 100644 --- a/src/lockscreen.rs +++ b/src/lockscreen.rs @@ -244,6 +244,10 @@ pub fn create_lockscreen_window( if password.is_empty() { return; } + // Clear the GTK entry's internal buffer as early as possible. GTK allocates + // the backing GString via libc malloc, which zeroize cannot reach — the + // best we can do is shorten the window during which it resides in memory. + entry.set_text(""); entry.set_sensitive(false); let username = username.clone(); @@ -518,12 +522,35 @@ pub fn start_fingerprint( /// Load the wallpaper as a texture once, for sharing across all windows. /// Returns None if no wallpaper path is provided or the file cannot be loaded. /// Blur is applied at render time via GPU (GskBlurNode), not here. +/// +/// Opens the file with O_NOFOLLOW to close the TOCTOU window between the +/// symlink check in `resolve_background_path_with` and this read. If the path +/// was swapped for a symlink after the check, `open` fails with ELOOP. pub fn load_background_texture(bg_path: &Path) -> Option { - let file = gio::File::for_path(bg_path); - match gdk::Texture::from_file(&file) { + use std::io::Read; + use std::os::unix::fs::OpenOptionsExt; + + let mut file = match std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW) + .open(bg_path) + { + Ok(f) => f, + Err(e) => { + log::warn!("Failed to open wallpaper {}: {e}", bg_path.display()); + return None; + } + }; + let mut bytes = Vec::new(); + if let Err(e) = file.read_to_end(&mut bytes) { + log::warn!("Failed to read wallpaper {}: {e}", bg_path.display()); + return None; + } + let glib_bytes = glib::Bytes::from_owned(bytes); + match gdk::Texture::from_bytes(&glib_bytes) { Ok(texture) => Some(texture), Err(e) => { - log::warn!("Failed to load wallpaper {}: {e}", bg_path.display()); + log::warn!("Failed to decode wallpaper {}: {e}", bg_path.display()); None } }