From 73c59e54c12fdee42ff58116ceebe80fe533b1b4 Mon Sep 17 00:00:00 2001 From: nevaforget Date: Mon, 4 May 2026 09:28:11 +0200 Subject: [PATCH] fix: drop pam_acct_mgmt from password and FP paths (v0.6.13) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PAM stack only ever had `auth include login` — no account module. auth.rs nevertheless called pam_acct_mgmt after pam_authenticate, which fell back to /etc/pam.d/other (pam_deny) and rejected every password. On the FP side, the same call was wrapped in a spawn_blocking + 2s resume_async retry path that triggered a use-after-free in gtk_window_destroy (20+ SIGSEGVs in 6 days). - auth.rs: remove pam_acct_mgmt extern + call; return pam_authenticate result directly. Lockout still works via pam_faillock in the auth stack. - auth.rs: drop check_account() and its tests (FP path no longer needs it). - lockscreen.rs::start_fingerprint: on success go straight to label.set_text + fp.stop() + cb(); no PAM acct check, no resume retry. - fingerprint.rs: remove resume_async() — no caller left. - config/moonlock-pam: keep single `auth include login` line, matching swaylock/gtklock pattern. - CLAUDE.md, DECISIONS.md updated. --- CLAUDE.md | 7 ++-- Cargo.lock | 2 +- Cargo.toml | 2 +- DECISIONS.md | 7 ++++ config/moonlock-pam | 4 +-- src/auth.rs | 79 ++++----------------------------------------- src/fingerprint.rs | 28 ---------------- src/lockscreen.rs | 38 +--------------------- 8 files changed, 22 insertions(+), 145 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1b10d43..1d17f4b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -35,13 +35,13 @@ 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), check_account() für pam_acct_mgmt-Only-Checks nach Fingerprint-Unlock +- `auth.rs` — PAM-Authentifizierung via Raw FFI (unsafe extern "C" conv callback, msg_style-aware, Zeroizing) - `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 clamped [0,200], 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 +- `lockscreen.rs` — GTK4 UI via LockscreenHandles, PAM-Auth via gio::spawn_blocking mit 30s Timeout und Generation Counter, FP-Success ruft unlock_callback direkt (PAM-Stack ohne account-Modul, Lockout via auth-Pfad und MAX_FP_ATTEMPTS), 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), Monitor-Hotplug via `connect_monitor`-Signal (v1_2), shared Blur/Avatar-Caches in Rc, systemd-Journal-Logging, Debug-Level per `MOONLOCK_DEBUG` Env-Var, async fprintd-Init nach window.present() ## Sicherheit @@ -52,7 +52,8 @@ LD_PRELOAD=/usr/lib/libgtk4-layer-shell.so ./target/release/moonlock - 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 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) +- Fingerprint-Unlock: ruft unlock_callback direkt nach verify-match (kein zusätzlicher PAM-acct-Check, weil PAM-Stack nur `auth include login` enthält — wie swaylock); FP-Lockout ist über MAX_FP_ATTEMPTS in fingerprint.rs umgesetzt +- PAM-Stack: nur `auth include login` (Standard-Pattern wie swaylock/gtklock); kein `account`/`session`-Stack, weil Lockscreen keinen Session-Lifecycle managed und `pam_unix(account)` setuid root benötigt - Wallpaper wird vor lock() geladen — connect_monitor feuert während lock() und braucht die Textur; lokales JPEG-Laden ist schnell genug - 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 diff --git a/Cargo.lock b/Cargo.lock index 6b2a38d..2b07668 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -575,7 +575,7 @@ dependencies = [ [[package]] name = "moonlock" -version = "0.6.12" +version = "0.6.13" dependencies = [ "gdk-pixbuf", "gdk4", diff --git a/Cargo.toml b/Cargo.toml index 1a07e09..3ab2c9d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "moonlock" -version = "0.6.12" +version = "0.6.13" edition = "2024" description = "A secure Wayland lockscreen with GTK4, PAM and fingerprint support" license = "MIT" diff --git a/DECISIONS.md b/DECISIONS.md index 05d9999..469d1e6 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -2,6 +2,13 @@ Architectural and design decisions for Moonlock, in reverse chronological order. +## 2026-05-04 – Drop PAM account/session stack, remove `check_account`, drop `pam_acct_mgmt` from password path (v0.6.13) + +- **Who**: ClaudeCode, Dom +- **Why**: 20 SIGSEGV coredumps in 6 days. All crashes preceded by `pam_unix(moonlock:account): setuid failed: Operation not permitted`. The previous PAM config (`auth/account/session include system-auth`) pulled `pam_unix(account)`, which needs setuid root for `/etc/shadow`. moonlock runs as the user, so `pam_acct_mgmt` always failed. The failure cascaded into the FP-resume path (`gio::spawn_blocking(check_account) → false → 2s timeout → resume_async`) where a use-after-free during `gtk_window_destroy` killed the process. Each crash left a dead `ext-session-lock-v1` client and the compositor stuck on its red fallback backdrop until manual recovery. Initial fix on 2026-04-30 dropped the FP-side `check_account` and the account/session lines from the PAM config, but left the `pam_acct_mgmt` call in `auth.rs::authenticate()` for the password path. Result: a PAM stack with no `account` module fell back to `/etc/pam.d/other` (`pam_deny`) and rejected every password — Dom got locked out on 2026-05-04. +- **Tradeoffs**: Aligned with the swaylock/gtklock pattern (only `auth include login`). Lost: PAM-driven account expiry/lockout in both paths. Acceptable because (a) FP attempts are still bounded by `MAX_FP_ATTEMPTS` in `fingerprint.rs`, (b) password-path lockout still works through `pam_faillock.so preauth/authfail/authsucc` in the inherited `auth` stack, and (c) account validity was already verified by the login manager when the session was opened — a lockscreen unlocks an existing session, it does not gate access to a new one. Not chosen: a custom `account` stack with `pam_faillock.so` only — would have kept PAM-level FP lockout but adds a non-standard config that other lockers do not use, and `pam_faillock` standalone in `account` is rarely tested in the wild. +- **How**: (1) `config/moonlock-pam` reduced to a single `auth include login` line. (2) `auth.rs::check_account()` and its two unit tests removed. (3) `auth.rs::authenticate()` no longer calls `pam_acct_mgmt`; `pam_authenticate` result is returned directly. The `pam_acct_mgmt` extern declaration is removed. (4) `lockscreen.rs::start_fingerprint` simplified — the `gio::spawn_blocking(check_account)` async block, the failure-side error UI, and the 2-second `resume_async` retry path all removed; on FP success the closure now goes `label.set_text(success); fp.stop(); cb()`. (5) `fingerprint.rs::resume_async()` removed. (6) `CLAUDE.md` architecture and security sections updated to describe the new PAM stack. + ## 2026-04-24 – Audit LOW fixes: docs, rustdoc, check_account scope, debug gating, lto fat (v0.6.12) - **Who**: ClaudeCode, Dom diff --git a/config/moonlock-pam b/config/moonlock-pam index ab053c5..08ecce3 100644 --- a/config/moonlock-pam +++ b/config/moonlock-pam @@ -1,4 +1,2 @@ #%PAM-1.0 -auth include system-auth -account include system-auth -session include system-auth +auth include login diff --git a/src/auth.rs b/src/auth.rs index fcc5dc4..4e4a6f6 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -54,8 +54,6 @@ unsafe extern "C" { fn pam_authenticate(pamh: *mut libc::c_void, flags: libc::c_int) -> libc::c_int; - fn pam_acct_mgmt(pamh: *mut libc::c_void, flags: libc::c_int) -> libc::c_int; - fn pam_end(pamh: *mut libc::c_void, pam_status: libc::c_int) -> libc::c_int; } @@ -197,68 +195,17 @@ pub fn authenticate(username: &str, password: &str) -> bool { // refreshing them would duplicate work done by the session's login manager. // If per-unlock credential refresh (Kerberos tickets, pam_gnome_keyring) // is ever desired, hook it here with PAM_ESTABLISH_CRED. + // + // pam_acct_mgmt is intentionally NOT called: the PAM stack (`auth include + // login`) has no `account` module, and pam_unix(account) requires setuid + // root for /etc/shadow. Lockout/faillock and policy checks happen inside + // the inherited auth stack via pam_faillock. See DECISIONS.md 2026-04-30. let auth_ret = unsafe { pam_authenticate(handle, 0) }; - let acct_ret = if auth_ret == PAM_SUCCESS { - // Safety: handle is valid, check account restrictions - unsafe { pam_acct_mgmt(handle, 0) } - } else { - auth_ret - }; // Safety: handle is valid, pam_end cleans up the PAM session - unsafe { pam_end(handle, acct_ret) }; + unsafe { pam_end(handle, auth_ret) }; - acct_ret == PAM_SUCCESS -} - -/// Check account restrictions via PAM without authentication. -/// -/// Used after fingerprint unlock to enforce account policies (lockout, expiry) -/// that would otherwise be bypassed when not going through pam_authenticate. -/// Returns true if the account is valid and allowed to log in. -/// -/// **Precondition**: `username` must be the authenticated system user, derived -/// via `users::get_current_user()` (which reads `getuid()`). Calling this with -/// an attacker-controlled username is unsafe — `pam_acct_mgmt` returns SUCCESS -/// for any valid unlocked account, giving a trivial unlock bypass. -pub(crate) fn check_account(username: &str) -> bool { - let service = match CString::new("moonlock") { - Ok(c) => c, - Err(_) => return false, - }; - - let username_cstr = match CString::new(username) { - Ok(c) => c, - Err(_) => return false, - }; - - // No password needed — we only check account status, not authenticate. - // PAM conv callback is required by pam_start but won't be called for acct_mgmt. - let empty_password = Zeroizing::new(CString::new("").unwrap()); - let conv = PamConv { - conv: pam_conv_callback, - appdata_ptr: std::ptr::from_ref::(&empty_password) as *mut libc::c_void, - }; - - let mut handle: *mut libc::c_void = ptr::null_mut(); - - let ret = unsafe { - pam_start( - service.as_ptr(), - username_cstr.as_ptr(), - &conv, - &mut handle, - ) - }; - - if ret != PAM_SUCCESS || handle.is_null() { - return false; - } - - let acct_ret = unsafe { pam_acct_mgmt(handle, 0) }; - unsafe { pam_end(handle, acct_ret) }; - - acct_ret == PAM_SUCCESS + auth_ret == PAM_SUCCESS } #[cfg(test)] @@ -295,16 +242,4 @@ 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/fingerprint.rs b/src/fingerprint.rs index 5bed210..a5b6a46 100644 --- a/src/fingerprint.rs +++ b/src/fingerprint.rs @@ -175,29 +175,6 @@ impl FingerprintListener { Self::begin_verification(listener, username).await; } - /// 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. 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, - ) { - // 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; - } - /// Claim device, start verification, and connect D-Bus signal handler. /// Assumes device_proxy is set and callbacks are already stored. async fn begin_verification( @@ -366,11 +343,6 @@ impl FingerprintListener { /// 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); diff --git a/src/lockscreen.rs b/src/lockscreen.rs index eefd5ff..8f8d672 100644 --- a/src/lockscreen.rs +++ b/src/lockscreen.rs @@ -427,52 +427,16 @@ pub fn start_fingerprint( let unlock_cb_fp = handles.unlock_callback.clone(); let fp_rc_success = fp_rc.clone(); - let fp_username = handles.username.clone(); let on_success = move || { let label = fp_label_success.clone(); let cb = unlock_cb_fp.clone(); let fp = fp_rc_success.clone(); - let username = fp_username.clone(); glib::idle_add_local_once(move || { let strings = load_strings(None); label.set_text(strings.fingerprint_success); label.add_css_class("success"); - // stop() is idempotent — cleanup_dbus() already ran inside on_verify_status, - // but this mirrors the PAM success path for defense-in-depth. fp.borrow_mut().stop(); - - // Enforce PAM account policies (lockout, expiry) before unlocking. - // Fingerprint auth bypasses pam_authenticate, so we must explicitly - // check account restrictions via pam_acct_mgmt. - glib::spawn_future_local(async move { - let user = username.clone(); - let result = gio::spawn_blocking(move || { - auth::check_account(&user) - }).await; - match result { - Ok(true) => cb(), - _ => { - log::error!("PAM account check failed after fingerprint auth"); - let strings = load_strings(None); - label.set_text(strings.wrong_password); - label.remove_css_class("success"); - label.add_css_class("failed"); - // Restart FP verification after delay — the failure may be - // transient (e.g. PAM module timeout). If the account is truly - // locked, check_account will fail again on next match. - glib::timeout_add_local_once( - std::time::Duration::from_secs(2), - move || { - label.set_text(load_strings(None).fingerprint_prompt); - label.remove_css_class("failed"); - glib::spawn_future_local(async move { - FingerprintListener::resume_async(&fp, &username).await; - }); - }, - ); - } - } - }); + cb(); }); };