From 3a1af6471f9e31a37a76f09fc64f395efd8db1cd Mon Sep 17 00:00:00 2001 From: nevaforget Date: Fri, 24 Apr 2026 13:26:52 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20audit=20MEDIUM=20fixes=20=E2=80=94=20FP?= =?UTF-8?q?=20race,=20async=20avatar,=20symlink,=20FD=20leak=20(v0.8.5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - i18n test: assert four previously-missing string fields so future locales cannot ship empty strings unnoticed. - greeter: atomic check-and-set fingerprint_probe_initializing to keep a fast user switch from spawning two parallel fprintd D-Bus inits. - greeter: set_avatar_from_file decodes via gio::File::read_future + Pixbuf::from_stream_at_scale_future inside glib::spawn_future_local; shows default icon first, swaps on completion. - greeter: cap MAX_WALLPAPER_FILE_SIZE at 10 MB and MAX_AVATAR_FILE_SIZE at 5 MB to bound worst-case decode latency. - config: apply the same symlink-rejection check to the Moonarch wallpaper fallback that the user-configured path already uses. - greeter: after login_worker returns, drop the cloned greetd socket held in shared state so repeated failed logins do not leak FDs. --- Cargo.lock | 2 +- Cargo.toml | 2 +- DECISIONS.md | 7 ++++ src/config.rs | 12 ++++--- src/greeter.rs | 94 ++++++++++++++++++++++++++++++++++++-------------- src/i18n.rs | 4 +++ 6 files changed, 90 insertions(+), 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 586ac30..fcd3931 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -575,7 +575,7 @@ dependencies = [ [[package]] name = "moongreet" -version = "0.8.4" +version = "0.8.5" dependencies = [ "gdk-pixbuf", "gdk4", diff --git a/Cargo.toml b/Cargo.toml index 738c21b..05ab797 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "moongreet" -version = "0.8.4" +version = "0.8.5" edition = "2024" description = "A greetd greeter for Wayland with GTK4 and Layer Shell" license = "MIT" diff --git a/DECISIONS.md b/DECISIONS.md index 9a3ea48..ce6b36c 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1,5 +1,12 @@ # Decisions +## 2026-04-24 – Audit MEDIUM fixes: FP double-init, async avatar, symlink, FD leak (v0.8.5) + +- **Who**: ClaudeCode, Dom +- **Why**: Six MEDIUM findings: (1) i18n test `all_string_fields_nonempty` missed four string fields — future locales could ship empty strings unnoticed. (2) Fast user-switch could spawn two parallel fprintd `init_async` calls because both coroutines saw `fingerprint_probe = None` before either stored its probe. (3) Synchronous avatar decode via `Pixbuf::from_file_at_scale` on the GTK main thread, stalling clicks. (4) Wallpaper `MAX_WALLPAPER_FILE_SIZE = 50 MB` bounded decode at up to ~2 s. (5) Fallback wallpaper path used `is_file()` which follows symlinks, inconsistent with the symlink-rejecting user-config path. (6) After a failed login the cloned `greetd_sock` descriptor remained in shared state until the next user switch, accumulating stale FDs across retries. +- **Tradeoffs**: The init-race guard uses a bool flag on `GreeterState` + a 25 ms polling yield — cheap and race-free, but introduces a very short latency when a second probe waits. Lowering `MAX_WALLPAPER_FILE_SIZE` to 10 MB and `MAX_AVATAR_FILE_SIZE` to 5 MB caps worst-case decode but rejects legitimately huge (4K raw) wallpapers; acceptable for a greeter. Async avatar decode shows the default icon for a frame or two on cache miss. +- **How**: (1) Four new `assert!` lines in `i18n::tests::all_string_fields_nonempty`. (2) New `fingerprint_probe_initializing: bool` on `GreeterState`, atomic check-and-set under `borrow_mut`, losing coroutines yield via `glib::timeout_future` until the winning init completes. (3) `set_avatar_from_file` uses `gio::File::read_future` + `Pixbuf::from_stream_at_scale_future` inside a `glib::spawn_future_local`, sets the default icon first, swaps on success. (4) Lower both size constants. (5) `resolve_background_path_with` now applies the same `symlink_metadata` + `!is_symlink` check to the Moonarch fallback. (6) After the login worker returns, `state.greetd_sock.lock().take()` drops the stale clone regardless of login outcome. + ## 2026-04-24 – Audit fix: shrink password-in-memory window (v0.8.4) - **Who**: ClaudeCode, Dom diff --git a/src/config.rs b/src/config.rs index f86a736..4c7f4d6 100644 --- a/src/config.rs +++ b/src/config.rs @@ -123,10 +123,14 @@ pub fn resolve_background_path_with(config: &Config, moonarch_wallpaper: &Path) log::debug!("Wallpaper: config path {} not usable, trying fallbacks", path.display()); } - // Moonarch ecosystem default - if moonarch_wallpaper.is_file() { - log::debug!("Wallpaper: using moonarch default {}", moonarch_wallpaper.display()); - return Some(moonarch_wallpaper.to_path_buf()); + // Moonarch ecosystem default — apply the same symlink rejection as the + // user-configured path for defense in depth. The fallback target is a + // system file, but the caller consumes the result via the same path. + if let Ok(meta) = moonarch_wallpaper.symlink_metadata() { + if meta.is_file() && !meta.file_type().is_symlink() { + log::debug!("Wallpaper: using moonarch default {}", moonarch_wallpaper.display()); + return Some(moonarch_wallpaper.to_path_buf()); + } } log::debug!("Wallpaper: no wallpaper found, using GTK background color"); diff --git a/src/greeter.rs b/src/greeter.rs index e298beb..b6af489 100644 --- a/src/greeter.rs +++ b/src/greeter.rs @@ -22,8 +22,8 @@ use crate::sessions::{self, Session}; use crate::users::{self, User}; const AVATAR_SIZE: i32 = 128; -const MAX_AVATAR_FILE_SIZE: u64 = 10 * 1024 * 1024; -const MAX_WALLPAPER_FILE_SIZE: u64 = 50 * 1024 * 1024; +const MAX_AVATAR_FILE_SIZE: u64 = 5 * 1024 * 1024; +const MAX_WALLPAPER_FILE_SIZE: u64 = 10 * 1024 * 1024; const LAST_USER_PATH: &str = "/var/cache/moongreet/last-user"; const LAST_SESSION_DIR: &str = "/var/cache/moongreet/last-session"; const MAX_USERNAME_LENGTH: usize = 256; @@ -233,6 +233,9 @@ struct GreeterState { user_switch_generation: u64, /// Cached fprintd device proxy — initialized once on first use. fingerprint_probe: Option, + /// True while a probe init_async() is in flight. Prevents duplicate D-Bus + /// init when two user-switch probes race (both see probe == None). + fingerprint_probe_initializing: bool, } /// Create the main greeter window with login UI. @@ -282,6 +285,7 @@ pub fn create_greeter_window( fingerprint_available: false, user_switch_generation: 0, fingerprint_probe: None, + fingerprint_probe_initializing: false, })); // Root overlay for layering @@ -720,12 +724,33 @@ fn switch_to_user( #[strong] state, async move { - // Initialize probe on first use, then reuse cached device proxy - let needs_init = state.borrow().fingerprint_probe.is_none(); - if needs_init { + // Initialize probe on first use, then reuse cached device proxy. + // Atomic check-and-set on fingerprint_probe_initializing prevents + // two concurrent probes (from a fast user switch) from both + // running init_async, which would open duplicate D-Bus connections. + let should_init = { + let mut s = state.borrow_mut(); + if s.fingerprint_probe.is_some() || s.fingerprint_probe_initializing { + false + } else { + s.fingerprint_probe_initializing = true; + true + } + }; + + if should_init { let mut probe = crate::fingerprint::FingerprintProbe::new(); probe.init_async().await; - state.borrow_mut().fingerprint_probe = Some(probe); + let mut s = state.borrow_mut(); + s.fingerprint_probe = Some(probe); + s.fingerprint_probe_initializing = false; + } else { + // Another coroutine is initializing — yield until it publishes. + while state.borrow().fingerprint_probe.is_none() + && state.borrow().fingerprint_probe_initializing + { + glib::timeout_future(std::time::Duration::from_millis(25)).await; + } } // Take probe out of state to avoid holding borrow across await @@ -782,28 +807,40 @@ fn set_avatar_from_file( Ok(_) => {} } - let Some(path_str) = path.to_str() else { - log::debug!("Non-UTF-8 avatar path, skipping: {}", path.display()); - image.set_icon_name(Some("avatar-default-symbolic")); - return; - }; + // Show fallback immediately; decode asynchronously via GIO so the greeter + // stays responsive during a user-switch click. + image.set_icon_name(Some("avatar-default-symbolic")); - match Pixbuf::from_file_at_scale(path_str, AVATAR_SIZE, AVATAR_SIZE, true) { - Ok(pixbuf) => { - let texture = gdk::Texture::for_pixbuf(&pixbuf); - if let Some(name) = username { - state - .borrow_mut() - .avatar_cache - .insert(name.to_string(), texture.clone()); + let display_path = path.to_path_buf(); + let file = gio::File::for_path(path); + let image_clone = image.clone(); + let state_clone = state.clone(); + let username_owned = username.map(String::from); + + glib::spawn_future_local(async move { + let stream = match file.read_future(glib::Priority::default()).await { + Ok(s) => s, + Err(e) => { + log::debug!("Failed to open avatar {}: {e}", display_path.display()); + return; + } + }; + match Pixbuf::from_stream_at_scale_future(&stream, AVATAR_SIZE, AVATAR_SIZE, true).await { + Ok(pixbuf) => { + let texture = gdk::Texture::for_pixbuf(&pixbuf); + if let Some(ref name) = username_owned { + state_clone + .borrow_mut() + .avatar_cache + .insert(name.clone(), texture.clone()); + } + image_clone.set_paintable(Some(&texture)); + } + Err(e) => { + log::debug!("Failed to decode avatar {}: {e}", display_path.display()); } - image.set_paintable(Some(&texture)); } - Err(e) => { - log::debug!("Failed to load avatar {}: {e}", path.display()); - image.set_icon_name(Some("avatar-default-symbolic")); - } - } + }); } /// Load the default avatar SVG from GResources, tinted with the foreground color. @@ -1036,6 +1073,13 @@ fn attempt_login( glib::timeout_future(min_response - elapsed).await; } + // The login_worker's own socket is already dropped by now; drop the + // shared clone too so repeated failed attempts do not accumulate + // stale file descriptors in state.greetd_sock. + if let Ok(mut g) = state.borrow().greetd_sock.lock() { + g.take(); + } + match result { Ok(Ok(LoginResult::Success { username })) => { save_last_user(&username); diff --git a/src/i18n.rs b/src/i18n.rs index 7280d78..16719cb 100644 --- a/src/i18n.rs +++ b/src/i18n.rs @@ -286,6 +286,10 @@ mod tests { assert!(!s.faillock_attempts_remaining.is_empty(), "{locale}: faillock_attempts_remaining"); assert!(!s.faillock_locked.is_empty(), "{locale}: faillock_locked"); assert!(!s.unexpected_greetd_response.is_empty(), "{locale}: unexpected_greetd_response"); + assert!(!s.greetd_sock_not_absolute.is_empty(), "{locale}: greetd_sock_not_absolute"); + assert!(!s.invalid_session_command.is_empty(), "{locale}: invalid_session_command"); + assert!(!s.session_start_failed.is_empty(), "{locale}: session_start_failed"); + assert!(!s.socket_error.is_empty(), "{locale}: socket_error"); } }