fix: audit MEDIUM fixes — D-Bus race, TOCTOU, FP reset, entry clear (v0.6.11)
- fingerprint: split cleanup_dbus into a sync take_cleanup_proxy() + async perform_dbus_cleanup(). resume_async now awaits VerifyStop+Release before re-claiming, so fprintd cannot reject the Claim on a slow bus. stop() still spawns the cleanup fire-and-forget. - fingerprint: remove failed_attempts = 0 from resume_async. An attacker with sensor control could otherwise cycle verify-match → account-fail → resume and never trip the 10-attempt cap. - lockscreen: open the wallpaper with O_NOFOLLOW and build the texture from bytes, closing the TOCTOU between the symlink check and Texture:: from_file. - lockscreen: clear password_entry immediately after extracting the Zeroizing<String>, shortening the window the GLib GString copy stays in libc-malloc'd memory.
This commit is contained in:
parent
39d9cbb624
commit
9dfd1829e9
2
Cargo.lock
generated
2
Cargo.lock
generated
@ -575,7 +575,7 @@ dependencies = [
|
||||
|
||||
[[package]]
|
||||
name = "moonlock"
|
||||
version = "0.6.9"
|
||||
version = "0.6.11"
|
||||
dependencies = [
|
||||
"gdk-pixbuf",
|
||||
"gdk4",
|
||||
|
||||
@ -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"
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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<RefCell<FingerprintListener>>,
|
||||
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<gio::DBusProxy> {
|
||||
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));
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -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<gdk::Texture> {
|
||||
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
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user