diff --git a/Cargo.lock b/Cargo.lock index 36b2706..009cee8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -108,6 +108,7 @@ dependencies = [ "tracing", "tracing-subscriber", "wayland-client", + "zeroize", ] [[package]] @@ -2247,6 +2248,12 @@ version = "0.3.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6df3dc4292935e51816d896edcd52aa30bc297907c26167fec31e2b0c6a32524" +[[package]] +name = "zeroize" +version = "1.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e13c156562582aa81c60cb29407084cdb54c4164760106ab78e6c5b0858cf64e" + [[package]] name = "zmij" version = "1.0.21" diff --git a/breadlock/Cargo.toml b/breadlock/Cargo.toml index aed348c..13bda01 100644 --- a/breadlock/Cargo.toml +++ b/breadlock/Cargo.toml @@ -24,6 +24,7 @@ wayland-client = "0.31" tiny-skia = "0.12" chrono = "0.4" pam-client2 = { version = "0.5", default-features = false } +zeroize = { version = "1", features = ["std"] } serde.workspace = true toml.workspace = true tracing.workspace = true diff --git a/breadlock/src/auth/mod.rs b/breadlock/src/auth/mod.rs index 49eaf40..d1fad8d 100644 --- a/breadlock/src/auth/mod.rs +++ b/breadlock/src/auth/mod.rs @@ -35,8 +35,14 @@ pub fn register( /// Spawns a PAM check for `username`/`password` on its own thread; the /// outcome arrives later as an event on the loop registered via /// [`register`]. `password` is moved in and dropped as soon as the PAM -/// conversation consumes it — it is never logged. -pub fn spawn_check(username: String, password: String, result_tx: Sender) { +/// conversation consumes it — it is never logged. It's a `Zeroizing` +/// so the buffer is wiped the moment it goes out of scope at the end of this +/// closure, rather than just deallocated with the bytes intact. +pub fn spawn_check( + username: String, + password: zeroize::Zeroizing, + result_tx: Sender, +) { std::thread::spawn(move || { let result = pam::check(&username, &password); let _ = result_tx.send(result); diff --git a/breadlock/src/auth/pam.rs b/breadlock/src/auth/pam.rs index c41d9cc..e245dba 100644 --- a/breadlock/src/auth/pam.rs +++ b/breadlock/src/auth/pam.rs @@ -4,6 +4,7 @@ use pam_client2::conv_mock::Conversation; use pam_client2::{Context, Flag}; +use zeroize::Zeroize; /// The PAM service name — matches `/etc/pam.d/breadlock` /// (packaging/pam.d/breadlock), which is what actually determines the auth @@ -24,12 +25,24 @@ pub enum AuthError { /// `acct_mgmt` (no `open_session` — the graphical session is already open; /// this only re-proves who's sitting at the keyboard). pub fn check(username: &str, password: &str) -> Result<(), AuthError> { + // `Conversation::with_credentials` copies `password` into its own + // `String` field (it has to — PAM's conversation callback is invoked + // later, synchronously, by libpam via FFI). That struct has no Drop/ + // zeroize of its own, so we reach back in and zero it explicitly below + // before `ctx` (and the conversation it owns) is dropped. let conv = Conversation::with_credentials(username, password); let mut ctx = Context::new(SERVICE, Some(username), conv).map_err(|_| AuthError::ContextInit)?; - ctx.authenticate(Flag::NONE) - .map_err(|_| AuthError::Authenticate)?; - ctx.acct_mgmt(Flag::NONE) - .map_err(|_| AuthError::AccountInvalid)?; - Ok(()) + + let result = ctx + .authenticate(Flag::NONE) + .map_err(|_| AuthError::Authenticate) + .and_then(|()| { + ctx.acct_mgmt(Flag::NONE) + .map_err(|_| AuthError::AccountInvalid) + }); + + ctx.conversation_mut().password.zeroize(); + + result } diff --git a/breadlock/src/input/keyboard.rs b/breadlock/src/input/keyboard.rs index 75c6c74..0add42b 100644 --- a/breadlock/src/input/keyboard.rs +++ b/breadlock/src/input/keyboard.rs @@ -4,6 +4,7 @@ use smithay_client_toolkit::seat::keyboard::{ use smithay_client_toolkit::seat::{Capability, SeatHandler, SeatState}; use wayland_client::protocol::{wl_keyboard, wl_seat, wl_surface}; use wayland_client::{Connection, QueueHandle}; +use zeroize::Zeroize; use crate::auth; use crate::state::{AppState, AuthState}; @@ -23,7 +24,27 @@ impl SeatHandler for AppState { capability: Capability, ) { if capability == Capability::Keyboard && self.keyboard.is_none() { - match self.seat_state.get_keyboard(qh, &seat, None) { + // Plain `get_keyboard` never populates SCTK's internal repeat + // timer, so `KeyboardHandler::repeat_key` below only ever fires + // for compositors that implement server-side key repeat + // (wl_keyboard >= v10's "repeated" pseudo key-state) themselves — + // Hyprland does not reliably do this. `get_keyboard_with_repeat` + // registers SCTK's own client-side repeat timer driven by the + // compositor's `repeat_info` (delay/rate); if a compositor *does* + // do server-side repeat it advertises `rate = 0`, which this + // timer already treats as disabled, so the two mechanisms can't + // double-fire. + let repeat_qh = qh.clone(); + let loop_handle = self.loop_handle.clone(); + match self.seat_state.get_keyboard_with_repeat( + qh, + &seat, + None, + loop_handle, + Box::new(move |state: &mut AppState, _keyboard, event| { + state.handle_key(&repeat_qh, event); + }), + ) { Ok(keyboard) => self.keyboard = Some(keyboard), Err(err) => tracing::error!(%err, "failed to bind keyboard"), } @@ -127,11 +148,26 @@ impl AppState { match event.keysym { Keysym::Return | Keysym::KP_Enter => self.submit(), Keysym::BackSpace => { - self.password.pop(); + if let Some((idx, _)) = self.password.char_indices().last() { + // Plain `String::pop()` shrinks the logical length but + // leaves the removed character's bytes sitting in the + // buffer's spare capacity. Zero them explicitly before + // truncating. + // + // SAFETY: `idx` comes from `char_indices()`, so it is a + // valid char boundary; the retained prefix `[..idx]` + // is untouched and still valid UTF-8, and we truncate to + // exactly that boundary immediately after zeroing the + // (now-discarded) tail. + unsafe { + self.password.as_mut_vec()[idx..].zeroize(); + } + self.password.truncate(idx); + } self.clear_failed_state(); } Keysym::Escape => { - self.password.clear(); + self.password.zeroize(); self.clear_failed_state(); } _ => { @@ -151,7 +187,7 @@ impl AppState { } fn clear_failed_state(&mut self) { - if self.auth_state == AuthState::Failed { + if matches!(self.auth_state, AuthState::Failed | AuthState::ConfigError) { self.auth_state = AuthState::Idle; } } @@ -161,7 +197,15 @@ impl AppState { return; } self.auth_state = AuthState::Checking; - let password = std::mem::take(&mut self.password); + // Hand ownership of the buffer to the auth thread; re-reserve + // capacity up front so the next password typed doesn't reallocate + // (see the `password` field doc in state.rs). The taken buffer is + // zeroized automatically when it's dropped at the end of the PAM + // check (`auth::spawn_check`/`pam::check`). + let password = std::mem::replace( + &mut self.password, + zeroize::Zeroizing::new(String::with_capacity(128)), + ); auth::spawn_check(self.username.clone(), password, self.auth_tx.clone()); } } diff --git a/breadlock/src/lock/surface.rs b/breadlock/src/lock/surface.rs index 691a840..bb6d520 100644 --- a/breadlock/src/lock/surface.rs +++ b/breadlock/src/lock/surface.rs @@ -73,6 +73,7 @@ impl OutputHandler for AppState { let lock_surface = session_lock.create_lock_surface(surface, &output, qh); self.surfaces.push(LockSurface { surface: lock_surface, + output, width: 0, height: 0, }); @@ -86,11 +87,16 @@ impl OutputHandler for AppState { ) { } + /// A monitor disappeared (unplug, or Hyprland dropping/recreating it on + /// a mode change). Drop the lock surface tied to it — otherwise + /// `surfaces` only ever grows across hotplug cycles and `redraw_all` + /// keeps trying to commit to a surface whose output is gone. fn output_destroyed( &mut self, _conn: &Connection, _qh: &QueueHandle, - _output: wl_output::WlOutput, + output: wl_output::WlOutput, ) { + self.surfaces.retain(|s| s.output != output); } } diff --git a/breadlock/src/main.rs b/breadlock/src/main.rs index e351b1a..484b06a 100644 --- a/breadlock/src/main.rs +++ b/breadlock/src/main.rs @@ -57,8 +57,24 @@ fn main() { state.exit = true; } Err(err) => { - tracing::warn!(%err, "authentication failed"); - state.auth_state = AuthState::Failed; + match err { + // A broken PAM setup (missing/invalid /etc/pam.d/breadlock, + // context init failure) is a config problem, not a typo — + // rendering it identically to "wrong password" would lock + // the user out with zero indication of what's actually + // wrong. Log loudly and show a distinct on-screen message. + auth::AuthError::ContextInit => { + tracing::error!( + %err, + "PAM context initialization failed — check /etc/pam.d/breadlock exists and is valid; authentication cannot succeed until this is fixed" + ); + state.auth_state = AuthState::ConfigError; + } + auth::AuthError::Authenticate | auth::AuthError::AccountInvalid => { + tracing::warn!(%err, "authentication failed"); + state.auth_state = AuthState::Failed; + } + } state.schedule_clear_failed(auth_result_qh.clone()); } } @@ -88,7 +104,10 @@ fn main() { background, text_renderer: breadlock_ui::painter::TextRenderer::new(), username, - password: String::new(), + // Pre-reserve capacity so ordinary typing doesn't reallocate — a + // reallocation leaves the old (unzeroized) backing buffer, with the + // password bytes still in it, on the heap. + password: zeroize::Zeroizing::new(String::with_capacity(128)), auth_state: AuthState::Idle, auth_tx, exit: false, @@ -107,6 +126,7 @@ fn main() { let lock_surface = session_lock.create_lock_surface(surface, &output, &qh); app_state.surfaces.push(LockSurface { surface: lock_surface, + output, width: 0, height: 0, }); @@ -129,15 +149,48 @@ fn main() { ) .expect("failed to register the clock-tick timer"); + // A dispatch error here is the one path that can end this process while + // the session lock is still up: `SessionLockInner::drop` deliberately + // does *not* send `unlock`, only `destroy` (see the crate's own doc + // comment — "choosing not to unlock here results in us failing secure"), + // so an abrupt exit stays fail-secure at the protocol level; the failure + // mode is a frozen/unusable lock screen (Hyprland's "lock client + // crashed" state), not an unlocked one. We do NOT call `.unlock()` from + // here — doing so on an error path would make an unattended failure + // capable of unlocking the session, i.e. turn a fail-secure bug into a + // fail-open one. Instead: tolerate a burst of transient errors (a single + // `dispatch()` hiccup shouldn't be fatal) and only give up, loudly, after + // several consecutive failures. + const MAX_CONSECUTIVE_DISPATCH_ERRORS: u32 = 5; + let mut consecutive_errors = 0u32; while !app_state.exit { - if let Err(err) = event_loop.dispatch(Duration::from_millis(250), &mut app_state) { - tracing::error!(%err, "event loop dispatch failed"); - break; + match event_loop.dispatch(Duration::from_millis(250), &mut app_state) { + Ok(()) => consecutive_errors = 0, + Err(err) => { + consecutive_errors += 1; + tracing::error!( + %err, + consecutive_errors, + "event loop dispatch failed — session remains locked (fail-secure); \ + if this persists the lock screen may become unresponsive and require \ + a VT switch or `loginctl` to recover" + ); + if consecutive_errors >= MAX_CONSECUTIVE_DISPATCH_ERRORS { + tracing::error!( + "giving up after {consecutive_errors} consecutive dispatch failures; \ + exiting WITHOUT unlocking — this is intentional (fail-secure), but \ + the screen will likely be stuck and need a VT switch to recover" + ); + break; + } + } } } // Make sure the compositor actually receives the unlock/destroy - // requests queued above before the process exits. + // requests queued above (from a successful auth) before the process + // exits. This is a no-op if we got here via the dispatch-error path + // above, since nothing queued an unlock in that case. let _ = app_state.conn.roundtrip(); } diff --git a/breadlock/src/state.rs b/breadlock/src/state.rs index 0dc852e..8726cc9 100644 --- a/breadlock/src/state.rs +++ b/breadlock/src/state.rs @@ -9,7 +9,7 @@ use smithay_client_toolkit::seat::SeatState; use smithay_client_toolkit::session_lock::{SessionLock, SessionLockState, SessionLockSurface}; use smithay_client_toolkit::shm::{Shm, ShmHandler}; use std::time::Duration; -use wayland_client::protocol::{wl_keyboard, wl_shm}; +use wayland_client::protocol::{wl_keyboard, wl_output, wl_shm}; use wayland_client::{Connection, QueueHandle}; use crate::auth::AuthResult; @@ -18,9 +18,12 @@ use crate::config::Config; use crate::render; /// Per-output lock surface plus the size the compositor last `configure`d it -/// to (0x0 until the first configure arrives). +/// to (0x0 until the first configure arrives). `output` is kept so +/// `output_destroyed` can find and drop the surface belonging to an unplugged +/// monitor — without it, hotplug/unplug cycles only ever grow `surfaces`. pub struct LockSurface { pub surface: SessionLockSurface, + pub output: wl_output::WlOutput, pub width: u32, pub height: u32, } @@ -31,7 +34,16 @@ pub enum AuthState { /// A PAM check is running on its own thread; input is ignored until it /// resolves so a second Enter can't race the first attempt. Checking, + /// The password (or account state) was rejected by PAM — an ordinary + /// wrong-password/locked-account outcome the user can retry. Failed, + /// PAM itself failed to initialize (e.g. `/etc/pam.d/breadlock` is + /// missing or unreadable) — this is a config/deployment problem, not + /// something the user's password can fix. Rendered with a distinct + /// message so a broken install doesn't look like an endless string of + /// typos with no way to discover the real cause. See `main.rs`'s + /// auth-result callback, which is the only place this is set. + ConfigError, } pub struct AppState { @@ -53,7 +65,13 @@ pub struct AppState { pub text_renderer: breadlock_ui::painter::TextRenderer, pub username: String, - pub password: String, + /// Wrapped in `Zeroizing` so the buffer is wiped on every drop/replace + /// (e.g. when `submit()` swaps in a fresh one) rather than just + /// deallocated with the password bytes left sitting in freed heap + /// memory. Individual edits (backspace, clear) still need their own + /// explicit zeroing — see `input/keyboard.rs` — since `Zeroizing` only + /// hooks `Drop`, not in-place mutation. + pub password: zeroize::Zeroizing, pub auth_state: AuthState, pub auth_tx: Sender, @@ -81,6 +99,9 @@ impl AppState { let status_text = match self.auth_state { AuthState::Checking => Some("Checking…".to_string()), AuthState::Failed => Some("Wrong password".to_string()), + AuthState::ConfigError => { + Some("PAM config error — check logs (breadlock service not set up correctly)".to_string()) + } AuthState::Idle => None, }; @@ -92,7 +113,7 @@ impl AppState { font_family: &self.config.appearance.font.family, clock_text: &clock_text, password_len: self.password.len(), - failed: self.auth_state == AuthState::Failed, + failed: matches!(self.auth_state, AuthState::Failed | AuthState::ConfigError), status_text: status_text.as_deref(), }; @@ -151,7 +172,7 @@ impl AppState { let _ = self.loop_handle .insert_source(Timer::from_duration(timeout), move |_, _, state| { - if state.auth_state == AuthState::Failed { + if matches!(state.auth_state, AuthState::Failed | AuthState::ConfigError) { state.auth_state = AuthState::Idle; state.redraw_all(&qh); }