breadlock: fail-secure dispatch errors, zeroize password material, distinguish PAM config errors, fix output leak and key repeat

- main.rs: dispatch-loop errors no longer silently exit; bounded retry with
  loud logging, and explicitly never call unlock() on an error path (would
  turn a fail-secure crash into a fail-open one)
- Cargo.toml/state.rs/keyboard.rs/auth/*: password buffer is now
  Zeroizing<String>, with explicit zero-then-truncate on backspace and
  zeroize on clear/submit; also zeroes pam-client2's internal Conversation
  copy after use
- state.rs/main.rs: new AuthState::ConfigError, distinct on-screen message
  and error!-level log for PAM context-init failures vs ordinary wrong
  password
- lock/surface.rs/state.rs: LockSurface now tracks its wl_output so
  output_destroyed can remove it, fixing the surfaces Vec leak on
  monitor unplug
- input/keyboard.rs: bind the keyboard via get_keyboard_with_repeat so
  held keys (e.g. backspace) actually repeat, regardless of whether the
  compositor implements server-side wl_keyboard repeat

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"

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

breadlock/src/auth/mod.rs

@@ -35,8 +35,14 @@ pub fn register<Data: 'static>(
 /// 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.
+/// conversation consumes it — it is never logged. It's a `Zeroizing<String>`
-pub fn spawn_check(username: String, password: String, result_tx: Sender<AuthResult>) {
+/// 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<String>,
+    result_tx: Sender<AuthResult>,
+) {
     std::thread::spawn(move || {
         let result = pam::check(&username, &password);
         let _ = result_tx.send(result);

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)?;
+    let result = ctx
+        .authenticate(Flag::NONE)
+        .map_err(|_| AuthError::Authenticate)
+        .and_then(|()| {
             ctx.acct_mgmt(Flag::NONE)
-        .map_err(|_| AuthError::AccountInvalid)?;
+                .map_err(|_| AuthError::AccountInvalid)
-    Ok(())
+        });
+
+    ctx.conversation_mut().password.zeroize();
+
+    result
 }

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());
     }
 }

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<Self>,
-        _output: wl_output::WlOutput,
+        output: wl_output::WlOutput,
     ) {
+        self.surfaces.retain(|s| s.output != output);
     }
 }

breadlock/src/main.rs

@@ -57,8 +57,24 @@ fn main() {
                 state.exit = true;
             }
             Err(err) => {
+                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) {
+        match event_loop.dispatch(Duration::from_millis(250), &mut app_state) {
-            tracing::error!(%err, "event loop dispatch failed");
+            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();
 }
 

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<String>,
     pub auth_state: AuthState,
     pub auth_tx: Sender<AuthResult>,
 
@@ -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);
                     }