fix: audit MEDIUM fixes — FP race, async avatar, symlink, FD leak (v0.8.5)
- 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.
This commit is contained in:
parent
35f1a17cdf
commit
3a1af6471f
2
Cargo.lock
generated
2
Cargo.lock
generated
@ -575,7 +575,7 @@ dependencies = [
|
||||
|
||||
[[package]]
|
||||
name = "moongreet"
|
||||
version = "0.8.4"
|
||||
version = "0.8.5"
|
||||
dependencies = [
|
||||
"gdk-pixbuf",
|
||||
"gdk4",
|
||||
|
||||
@ -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"
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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");
|
||||
|
||||
@ -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<crate::fingerprint::FingerprintProbe>,
|
||||
/// 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);
|
||||
|
||||
@ -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");
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user