From 17f8930ff744f61dd6c10cabcbf1ed3edc166f81 Mon Sep 17 00:00:00 2001 From: nevaforget Date: Sat, 28 Mar 2026 00:06:27 +0100 Subject: [PATCH] fix: security and correctness audit fixes (v0.4.1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PAM conv callback: check msg_style (password only for ECHO_OFF), handle strdup OOM with proper cleanup, null-check PAM handle. Fingerprint: self-wire D-Bus g-signal in start() via Rc> and connect_local — VerifyStatus signals are now actually dispatched. VerifyStop before VerifyStart in restart_verify. Lockscreen: password entry stays active after faillock threshold (PAM decides lockout, not UI), use Zeroizing from GTK entry. Release builds exit(1) without ext-session-lock-v1 support. Config: fingerprint_enabled as Option so empty user config does not override system config. Dead code: remove unused i18n strings and fingerprint accessors, parameterize faillock_warning max_attempts. --- CLAUDE.md | 17 +++++---- Cargo.toml | 2 +- src/auth.rs | 57 ++++++++++++++++++++++++++--- src/config.rs | 36 +++++++++++++++---- src/fingerprint.rs | 90 ++++++++++++++++++++++++++++++++-------------- src/i18n.rs | 21 ++++------- src/lockscreen.rs | 35 +++++++++++++----- src/main.rs | 13 +++++-- 8 files changed, 201 insertions(+), 70 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 97f725f..6cff80b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -37,20 +37,23 @@ LD_PRELOAD=/usr/lib/libgtk4-layer-shell.so ./target/release/moonlock ## Architektur -- `auth.rs` — PAM-Authentifizierung via Raw FFI (unsafe extern "C" conv callback, Zeroizing>) -- `fingerprint.rs` — fprintd D-Bus Listener (gio::DBusProxy, VerifyStatus Signal-Handling) +- `auth.rs` — PAM-Authentifizierung via Raw FFI (unsafe extern "C" conv callback, msg_style-aware, Zeroizing>) +- `fingerprint.rs` — fprintd D-Bus Listener (Rc>, self-wiring g-signal via connect_local) - `users.rs` — Aktuellen User via nix getuid, Avatar-Loading mit Symlink-Rejection - `power.rs` — Reboot/Shutdown via /usr/bin/loginctl -- `i18n.rs` — Locale-Erkennung und String-Tabellen (DE/EN) -- `config.rs` — TOML-Config (background_path, fingerprint_enabled) + Wallpaper-Fallback -- `lockscreen.rs` — GTK4 UI, PAM-Auth via gio::spawn_blocking, Fingerprint-Indikator, Power-Confirm -- `main.rs` — Entry Point, Panic-Hook, Root-Check, ext-session-lock-v1, Multi-Monitor +- `i18n.rs` — Locale-Erkennung und String-Tabellen (DE/EN), faillock_warning mit konfigurierbarem max_attempts +- `config.rs` — TOML-Config (background_path, fingerprint_enabled als Option) + Wallpaper-Fallback +- `lockscreen.rs` — GTK4 UI, PAM-Auth via gio::spawn_blocking, Fingerprint-Indikator, Zeroizing für Passwort, Power-Confirm +- `main.rs` — Entry Point, Panic-Hook, Root-Check, ext-session-lock-v1 (Pflicht in Release), Multi-Monitor ## Sicherheit - ext-session-lock-v1 garantiert: Compositor sperrt alle Surfaces bei lock() +- Release-Build: Ohne ext-session-lock-v1 wird `exit(1)` aufgerufen — kein Fenster-Fallback - Panic-Hook: Bei Crash wird geloggt, aber NIEMALS unlock() aufgerufen — Screen bleibt schwarz -- Passwort: Zeroizing> für sicheres Wiping nach PAM-Callback +- PAM-Callback: msg_style-aware (Passwort nur bei PAM_PROMPT_ECHO_OFF), strdup-OOM-sicher +- Passwort: Zeroizing ab GTK-Entry-Extraktion, Zeroizing> im PAM-FFI-Layer - Root-Check: Exit mit Fehler wenn als root gestartet +- Faillock: UI-Warnung nach 3 Fehlversuchen, aber PAM entscheidet über Lockout (Entry bleibt aktiv) - Kein Schließen per Escape/Alt-F4 — nur durch erfolgreiche PAM-Auth oder Fingerprint - GResource-Bundle: CSS/Assets in der Binary kompiliert diff --git a/Cargo.toml b/Cargo.toml index 019d92f..be9134e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "moonlock" -version = "0.4.0" +version = "0.4.1" edition = "2024" description = "A secure Wayland lockscreen with GTK4, PAM and fingerprint support" license = "MIT" diff --git a/src/auth.rs b/src/auth.rs index df9e2af..a3e927e 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -7,6 +7,11 @@ use zeroize::Zeroizing; // PAM return codes const PAM_SUCCESS: i32 = 0; +const PAM_BUF_ERR: i32 = 5; + +// PAM message styles +const PAM_PROMPT_ECHO_OFF: libc::c_int = 1; +const PAM_PROMPT_ECHO_ON: libc::c_int = 2; /// PAM message structure (pam_message). #[repr(C)] @@ -60,7 +65,7 @@ unsafe extern "C" { /// which PAM will free. The appdata_ptr must point to a valid CString (the password). unsafe extern "C" fn pam_conv_callback( num_msg: libc::c_int, - _msg: *mut *const PamMessage, + msg: *mut *const PamMessage, resp: *mut *mut PamResponse, appdata_ptr: *mut libc::c_void, ) -> libc::c_int { @@ -83,10 +88,48 @@ unsafe extern "C" fn pam_conv_callback( } for i in 0..num_msg as isize { - // Safety: strdup allocates with malloc — PAM will free() the resp strings. - // We dereference password which is valid for the lifetime of authenticate(). let resp_ptr = resp_array.offset(i); - (*resp_ptr).resp = libc::strdup((*password).as_ptr()); + // Safety: msg is an array of pointers provided by PAM + let pam_msg = *msg.offset(i); + let msg_style = (*pam_msg).msg_style; + + match msg_style { + PAM_PROMPT_ECHO_OFF => { + // Password prompt — provide the password via strdup + let dup = libc::strdup((*password).as_ptr()); + if dup.is_null() { + // strdup failed (OOM) — free all previously allocated strings + for j in 0..i { + let prev = resp_array.offset(j); + if !(*prev).resp.is_null() { + libc::free((*prev).resp as *mut libc::c_void); + } + } + libc::free(resp_array as *mut libc::c_void); + return PAM_BUF_ERR; + } + (*resp_ptr).resp = dup; + } + PAM_PROMPT_ECHO_ON => { + // Visible prompt — provide empty string, never the password + let empty = libc::strdup(b"\0".as_ptr() as *const libc::c_char); + if empty.is_null() { + for j in 0..i { + let prev = resp_array.offset(j); + if !(*prev).resp.is_null() { + libc::free((*prev).resp as *mut libc::c_void); + } + } + libc::free(resp_array as *mut libc::c_void); + return PAM_BUF_ERR; + } + (*resp_ptr).resp = empty; + } + _ => { + // PAM_ERROR_MSG, PAM_TEXT_INFO, or unknown — no response expected + (*resp_ptr).resp = ptr::null_mut(); + } + } (*resp_ptr).resp_retcode = 0; } @@ -140,7 +183,11 @@ pub fn authenticate(username: &str, password: &str) -> bool { return false; } - // Safety: handle is valid after successful pam_start + if handle.is_null() { + return false; + } + + // Safety: handle is valid and non-null after successful pam_start let auth_ret = unsafe { pam_authenticate(handle, 0) }; let acct_ret = if auth_ret == PAM_SUCCESS { // Safety: handle is valid, check account restrictions diff --git a/src/config.rs b/src/config.rs index fb11c91..0ff77d4 100644 --- a/src/config.rs +++ b/src/config.rs @@ -16,24 +16,39 @@ fn default_config_paths() -> Vec { paths } +/// Raw deserialization struct — fingerprint_enabled is optional so that +/// an empty user config does not override the system config's value. #[derive(Debug, Clone, Default, Deserialize)] +struct RawConfig { + pub background_path: Option, + pub fingerprint_enabled: Option, +} + +/// Resolved configuration with concrete values. +#[derive(Debug, Clone)] pub struct Config { pub background_path: Option, - #[serde(default = "default_fingerprint")] pub fingerprint_enabled: bool, } -fn default_fingerprint() -> bool { true } +impl Default for Config { + fn default() -> Self { + Config { + background_path: None, + fingerprint_enabled: true, + } + } +} pub fn load_config(config_paths: Option<&[PathBuf]>) -> Config { let default_paths = default_config_paths(); let paths = config_paths.unwrap_or(&default_paths); - let mut merged = Config { fingerprint_enabled: true, ..Config::default() }; + let mut merged = Config::default(); for path in paths { if let Ok(content) = fs::read_to_string(path) { - if let Ok(parsed) = toml::from_str::(&content) { + if let Ok(parsed) = toml::from_str::(&content) { if parsed.background_path.is_some() { merged.background_path = parsed.background_path; } - merged.fingerprint_enabled = parsed.fingerprint_enabled; + if let Some(fp) = parsed.fingerprint_enabled { merged.fingerprint_enabled = fp; } } } } @@ -57,7 +72,7 @@ pub fn resolve_background_path_with(config: &Config, moonarch_wallpaper: &Path) mod tests { use super::*; - #[test] fn default_config() { let c = Config::default(); assert!(c.background_path.is_none()); assert!(!c.fingerprint_enabled); } + #[test] fn default_config() { let c = Config::default(); assert!(c.background_path.is_none()); assert!(c.fingerprint_enabled); } #[test] fn load_default_fingerprint_true() { let dir = tempfile::tempdir().unwrap(); let conf = dir.path().join("moonlock.toml"); @@ -79,6 +94,15 @@ mod tests { let c = Config { background_path: Some(wp.to_str().unwrap().to_string()), fingerprint_enabled: true }; assert_eq!(resolve_background_path_with(&c, Path::new("/nonexistent")), wp); } + #[test] fn empty_user_config_preserves_system_fingerprint() { + let dir = tempfile::tempdir().unwrap(); + let sys_conf = dir.path().join("system.toml"); + let usr_conf = dir.path().join("user.toml"); + fs::write(&sys_conf, "fingerprint_enabled = false\n").unwrap(); + fs::write(&usr_conf, "").unwrap(); + let c = load_config(Some(&[sys_conf, usr_conf])); + assert!(!c.fingerprint_enabled); + } #[test] fn resolve_gresource_fallback() { let c = Config::default(); let r = resolve_background_path_with(&c, Path::new("/nonexistent")); diff --git a/src/fingerprint.rs b/src/fingerprint.rs index 224c053..1e3e12f 100644 --- a/src/fingerprint.rs +++ b/src/fingerprint.rs @@ -3,6 +3,8 @@ use gio::prelude::*; use gtk4::gio; +use std::cell::RefCell; +use std::rc::Rc; const FPRINTD_BUS_NAME: &str = "net.reactivated.Fprint"; const FPRINTD_MANAGER_PATH: &str = "/net/reactivated/Fprint/Manager"; @@ -126,24 +128,31 @@ impl FingerprintListener { } } - /// Whether the listener is currently running. - pub fn is_running(&self) -> bool { - self.running - } - /// Start listening for fingerprint verification. - pub fn start(&mut self, username: &str, on_success: F, on_failure: G) - where + /// Connects the D-Bus g-signal handler internally. The `listener` parameter + /// must be the same `Rc>` that owns `self`. + pub fn start( + listener: &Rc>, + username: &str, + on_success: F, + on_failure: G, + ) where F: Fn() + 'static, G: Fn() + 'static, { - let proxy = match &self.device_proxy { - Some(p) => p, - None => return, + let proxy = { + let inner = listener.borrow(); + match inner.device_proxy.clone() { + Some(p) => p, + None => return, + } }; - self.on_success = Some(Box::new(on_success)); - self.on_failure = Some(Box::new(on_failure)); + { + let mut inner = listener.borrow_mut(); + inner.on_success = Some(Box::new(on_success)); + inner.on_failure = Some(Box::new(on_failure)); + } // Claim the device let args = glib::Variant::from((&username,)); @@ -178,12 +187,42 @@ impl FingerprintListener { return; } - self.running = true; + // Connect the g-signal handler on the proxy to dispatch VerifyStatus + let listener_weak = Rc::downgrade(listener); + let signal_id = proxy.connect_local("g-signal", false, move |values| { + // g-signal arguments: (proxy, sender_name, signal_name, parameters) + let signal_name: String = match values[2].get() { + Ok(v) => v, + Err(_) => return None, + }; + if signal_name.as_str() != "VerifyStatus" { + return None; + } - // Note: Signal handling is set up by the caller via connect_g_signal() - // because FingerprintListener is not an Rc and the g-signal callback - // needs to reference mutable state. The caller (lockscreen.rs) will - // connect the proxy's "g-signal" and call on_verify_status(). + let params = match values[3].get::() { + Ok(v) => v, + Err(_) => return None, + }; + + let status = params + .child_value(0) + .get::() + .unwrap_or_default(); + let done = params + .child_value(1) + .get::() + .unwrap_or(false); + + if let Some(listener_rc) = listener_weak.upgrade() { + listener_rc.borrow_mut().on_verify_status(&status, done); + } + + None + }); + + let mut inner = listener.borrow_mut(); + inner.signal_id = Some(signal_id); + inner.running = true; } /// Process a VerifyStatus signal from fprintd. @@ -228,6 +267,14 @@ impl FingerprintListener { /// Restart fingerprint verification after a completed attempt. fn restart_verify(&self) { if let Some(ref proxy) = self.device_proxy { + // VerifyStop before VerifyStart to avoid D-Bus errors + let _ = proxy.call_sync( + "VerifyStop", + None, + gio::DBusCallFlags::NONE, + -1, + gio::Cancellable::NONE, + ); let args = glib::Variant::from((&"any",)); if let Err(e) = proxy.call_sync( "VerifyStart", @@ -269,15 +316,6 @@ impl FingerprintListener { } } - /// Get a reference to the device proxy for signal connection. - pub fn device_proxy(&self) -> Option<&gio::DBusProxy> { - self.device_proxy.as_ref() - } - - /// Store the signal handler ID for cleanup. - pub fn set_signal_id(&mut self, id: glib::SignalHandlerId) { - self.signal_id = Some(id); - } } #[cfg(test)] diff --git a/src/i18n.rs b/src/i18n.rs index 340f220..e931395 100644 --- a/src/i18n.rs +++ b/src/i18n.rs @@ -10,13 +10,11 @@ const DEFAULT_LOCALE_CONF: &str = "/etc/locale.conf"; #[derive(Debug, Clone)] pub struct Strings { pub password_placeholder: &'static str, - pub unlock_button: &'static str, pub reboot_tooltip: &'static str, pub shutdown_tooltip: &'static str, pub fingerprint_prompt: &'static str, pub fingerprint_success: &'static str, pub fingerprint_failed: &'static str, - pub auth_failed: &'static str, pub wrong_password: &'static str, pub reboot_failed: &'static str, pub shutdown_failed: &'static str, @@ -30,13 +28,11 @@ pub struct Strings { const STRINGS_DE: Strings = Strings { password_placeholder: "Passwort", - unlock_button: "Entsperren", reboot_tooltip: "Neustart", shutdown_tooltip: "Herunterfahren", fingerprint_prompt: "Fingerabdruck auflegen zum Entsperren", fingerprint_success: "Fingerabdruck erkannt", fingerprint_failed: "Fingerabdruck nicht erkannt", - auth_failed: "Authentifizierung fehlgeschlagen", wrong_password: "Falsches Passwort", reboot_failed: "Neustart fehlgeschlagen", shutdown_failed: "Herunterfahren fehlgeschlagen", @@ -50,13 +46,11 @@ const STRINGS_DE: Strings = Strings { const STRINGS_EN: Strings = Strings { password_placeholder: "Password", - unlock_button: "Unlock", reboot_tooltip: "Reboot", shutdown_tooltip: "Shut down", fingerprint_prompt: "Place finger on reader to unlock", fingerprint_success: "Fingerprint recognized", fingerprint_failed: "Fingerprint not recognized", - auth_failed: "Authentication failed", wrong_password: "Wrong password", reboot_failed: "Reboot failed", shutdown_failed: "Shutdown failed", @@ -96,10 +90,9 @@ pub fn load_strings(locale: Option<&str>) -> &'static Strings { match locale.as_str() { "de" => &STRINGS_DE, _ => &STRINGS_EN } } -pub fn faillock_warning(attempt_count: u32, strings: &Strings) -> Option { - const MAX: u32 = 3; - if attempt_count >= MAX { return Some(strings.faillock_locked.to_string()); } - let remaining = MAX - attempt_count; +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; if remaining == 1 { return Some(strings.faillock_attempts_remaining.replace("{n}", &remaining.to_string())); } None } @@ -133,8 +126,8 @@ mod tests { assert!(!s.fingerprint_failed.is_empty()); } - #[test] fn faillock_zero() { assert!(faillock_warning(0, load_strings(Some("en"))).is_none()); } - #[test] fn faillock_one() { assert!(faillock_warning(1, load_strings(Some("en"))).is_none()); } - #[test] fn faillock_two() { assert!(faillock_warning(2, load_strings(Some("en"))).is_some()); } - #[test] fn faillock_three() { assert_eq!(faillock_warning(3, load_strings(Some("en"))).unwrap(), "Account may be locked"); } + #[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"); } } diff --git a/src/lockscreen.rs b/src/lockscreen.rs index 16727f4..696c219 100644 --- a/src/lockscreen.rs +++ b/src/lockscreen.rs @@ -10,6 +10,8 @@ use std::cell::RefCell; use std::path::Path; use std::rc::Rc; +use zeroize::Zeroizing; + use crate::auth; use crate::config::Config; use crate::fingerprint::FingerprintListener; @@ -24,6 +26,7 @@ const FAILLOCK_MAX_ATTEMPTS: u32 = 3; struct LockscreenState { failed_attempts: u32, fp_listener: FingerprintListener, + fp_listener_rc: Option>>, } /// Create a lockscreen window for a single monitor. @@ -54,6 +57,7 @@ pub fn create_lockscreen_window( let state = Rc::new(RefCell::new(LockscreenState { failed_attempts: 0, fp_listener, + fp_listener_rc: None, })); // Root overlay for background + centered content @@ -205,7 +209,7 @@ pub fn create_lockscreen_window( #[weak] password_entry, move |entry| { - let password = entry.text().to_string(); + let password = Zeroizing::new(entry.text().to_string()); if password.is_empty() { return; } @@ -223,14 +227,18 @@ pub fn create_lockscreen_window( password_entry, async move { let user = username.clone(); - let pass = password.clone(); + let pass = Zeroizing::new((*password).clone()); let result = gio::spawn_blocking(move || { auth::authenticate(&user, &pass) }).await; match result { Ok(true) => { - state.borrow_mut().fp_listener.stop(); + let s = state.borrow(); + if let Some(ref fp_rc) = s.fp_listener_rc { + fp_rc.borrow_mut().stop(); + } + drop(s); unlock_cb(); } _ => { @@ -241,13 +249,15 @@ pub fn create_lockscreen_window( password_entry.set_text(""); if count >= FAILLOCK_MAX_ATTEMPTS { + // Show warning but keep entry active — PAM decides lockout error_label.set_text(strings.faillock_locked); error_label.set_visible(true); - password_entry.set_sensitive(false); + password_entry.set_sensitive(true); + password_entry.grab_focus(); } else { password_entry.set_sensitive(true); password_entry.grab_focus(); - if let Some(warning) = faillock_warning(count, strings) { + if let Some(warning) = faillock_warning(count, FAILLOCK_MAX_ATTEMPTS, strings) { error_label.set_text(&warning); } else { error_label.set_text(strings.wrong_password); @@ -325,10 +335,17 @@ pub fn create_lockscreen_window( )); }; - state - .borrow_mut() - .fp_listener - .start(&user.username, on_success, on_failure); + // Extract the fp_listener into its own Rc> for signal self-wiring + let fp_rc = { + let mut s = state.borrow_mut(); + let listener = std::mem::replace(&mut s.fp_listener, FingerprintListener::new()); + Rc::new(RefCell::new(listener)) + }; + + FingerprintListener::start(&fp_rc, &user.username, on_success, on_failure); + + // Store back the Rc reference for stop() on unlock + state.borrow_mut().fp_listener_rc = Some(fp_rc); } // Fade-in on map diff --git a/src/main.rs b/src/main.rs index f0989ea..5fb89e2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -43,8 +43,16 @@ fn activate(app: >k::Application) { if gtk4_session_lock::is_supported() { activate_with_session_lock(app, &display, &bg_path, &config); } else { - log::warn!("ext-session-lock-v1 not supported — running in development mode"); - activate_without_lock(app, &bg_path, &config); + #[cfg(debug_assertions)] + { + log::warn!("ext-session-lock-v1 not supported — running in development mode"); + activate_without_lock(app, &bg_path, &config); + } + #[cfg(not(debug_assertions))] + { + log::error!("ext-session-lock-v1 not supported — refusing to run without session lock"); + std::process::exit(1); + } } } @@ -90,6 +98,7 @@ fn activate_with_session_lock( } } +#[cfg(debug_assertions)] fn activate_without_lock( app: >k::Application, bg_path: &PathBuf,