Compare commits

..

No commits in common. "main" and "v0.8.3" have entirely different histories.
main ... v0.8.3

9 changed files with 41 additions and 138 deletions

2
Cargo.lock generated
View File

@ -575,7 +575,7 @@ dependencies = [
[[package]] [[package]]
name = "moongreet" name = "moongreet"
version = "0.8.6" version = "0.8.3"
dependencies = [ dependencies = [
"gdk-pixbuf", "gdk-pixbuf",
"gdk4", "gdk4",

View File

@ -1,6 +1,6 @@
[package] [package]
name = "moongreet" name = "moongreet"
version = "0.8.6" version = "0.8.3"
edition = "2024" edition = "2024"
description = "A greetd greeter for Wayland with GTK4 and Layer Shell" description = "A greetd greeter for Wayland with GTK4 and Layer Shell"
license = "MIT" license = "MIT"

View File

@ -1,26 +1,5 @@
# Decisions # Decisions
## 2026-04-24 Audit LOW fixes: stdout null, utf-8 path, debug value, hidden sessions (v0.8.6)
- **Who**: ClaudeCode, Dom
- **Why**: Four LOW findings cleared in a single pass. (1) `power::run_command` piped stdout it never read — structurally fragile even though current callers stay well under the pipe buffer. (2) Relative wallpaper paths were resolved via `to_string_lossy`, silently substituting `U+FFFD` for non-UTF-8 bytes and producing a path that cannot be opened. (3) `MOONGREET_DEBUG` escalated log verbosity on mere presence, so an empty variable leaked auth metadata into the journal. (4) `Hidden=true` and `NoDisplay=true` `.desktop` entries appeared in the session dropdown even though they mark disabled or stub sessions.
- **Tradeoffs**: Gating debug on the literal value `"1"` is slightly stricter than most tools but matches the security-first posture. Filtering Hidden/NoDisplay means legitimately hidden but functional sessions are now unselectable from the greeter — acceptable, that is the convention these keys signal.
- **How**: (1) `.stdout(Stdio::null())` replaces the unused pipe. (2) `to_string_lossy().to_string()` replaced by `to_str().map(|s| s.to_string())` with a `log::warn!` fallback for non-UTF-8 paths. (3) `match std::env::var("MOONGREET_DEBUG").ok().as_deref()``Some("1")` selects Debug, everything else Info. (4) `parse_desktop_file` reads `Hidden=` and `NoDisplay=`, returns `None` if either is `true`.
## 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
- **Why**: Security audit flagged the GTK password path as holding more copies of the plaintext password in memory than necessary. `attempt_login` wrapped the already-`Zeroizing<String>` caller value into a second `Zeroizing<String>` (`password.to_string()`), and the GTK `GString` backing `entry.text()` persisted in libc malloc'd memory until the allocator reused the page.
- **Tradeoffs**: The GTK `GString` and the libc `strdup` copy on the PAM FFI boundary remain non-zeroizable — this is an inherent GTK/libc limitation, already documented in CLAUDE.md. This change reduces the Rust-owned copies to one and clears the `PasswordEntry` text field immediately after extraction to shorten the GTK-side window.
- **How**: (1) `attempt_login` now takes `password: Zeroizing<String>` by value instead of `&str`, moving ownership into the `spawn_blocking` closure. (2) The redundant `Zeroizing::new(password.to_string())` inside `attempt_login` is removed. (3) `password_entry.set_text("")` is called right after the password is extracted from the activate handler, shortening the lifetime of the GTK-internal buffer.
## 2026-04-21 Ship polkit rule in moongreet instead of moonarch (v0.8.3) ## 2026-04-21 Ship polkit rule in moongreet instead of moonarch (v0.8.3)
- **Who**: ClaudeCode, Dom - **Who**: ClaudeCode, Dom

View File

@ -68,14 +68,8 @@ pub fn load_config(config_paths: Option<&[PathBuf]>) -> Config {
if bg_path.is_absolute() { if bg_path.is_absolute() {
merged.background_path = Some(bg); merged.background_path = Some(bg);
} else if let Some(parent) = path.parent() { } else if let Some(parent) = path.parent() {
let joined = parent.join(&bg); merged.background_path =
match joined.to_str() { Some(parent.join(&bg).to_string_lossy().to_string());
Some(s) => merged.background_path = Some(s.to_string()),
None => log::warn!(
"Ignoring non-UTF-8 background path: {}",
joined.display()
),
}
} }
} }
if let Some(blur) = appearance.background_blur { if let Some(blur) = appearance.background_blur {
@ -129,15 +123,11 @@ pub fn resolve_background_path_with(config: &Config, moonarch_wallpaper: &Path)
log::debug!("Wallpaper: config path {} not usable, trying fallbacks", path.display()); log::debug!("Wallpaper: config path {} not usable, trying fallbacks", path.display());
} }
// Moonarch ecosystem default — apply the same symlink rejection as the // Moonarch ecosystem default
// user-configured path for defense in depth. The fallback target is a if moonarch_wallpaper.is_file() {
// 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()); log::debug!("Wallpaper: using moonarch default {}", moonarch_wallpaper.display());
return Some(moonarch_wallpaper.to_path_buf()); return Some(moonarch_wallpaper.to_path_buf());
} }
}
log::debug!("Wallpaper: no wallpaper found, using GTK background color"); log::debug!("Wallpaper: no wallpaper found, using GTK background color");
None None

View File

@ -22,8 +22,8 @@ use crate::sessions::{self, Session};
use crate::users::{self, User}; use crate::users::{self, User};
const AVATAR_SIZE: i32 = 128; const AVATAR_SIZE: i32 = 128;
const MAX_AVATAR_FILE_SIZE: u64 = 5 * 1024 * 1024; const MAX_AVATAR_FILE_SIZE: u64 = 10 * 1024 * 1024;
const MAX_WALLPAPER_FILE_SIZE: u64 = 10 * 1024 * 1024; const MAX_WALLPAPER_FILE_SIZE: u64 = 50 * 1024 * 1024;
const LAST_USER_PATH: &str = "/var/cache/moongreet/last-user"; const LAST_USER_PATH: &str = "/var/cache/moongreet/last-user";
const LAST_SESSION_DIR: &str = "/var/cache/moongreet/last-session"; const LAST_SESSION_DIR: &str = "/var/cache/moongreet/last-session";
const MAX_USERNAME_LENGTH: usize = 256; const MAX_USERNAME_LENGTH: usize = 256;
@ -233,9 +233,6 @@ struct GreeterState {
user_switch_generation: u64, user_switch_generation: u64,
/// Cached fprintd device proxy — initialized once on first use. /// Cached fprintd device proxy — initialized once on first use.
fingerprint_probe: Option<crate::fingerprint::FingerprintProbe>, 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. /// Create the main greeter window with login UI.
@ -285,7 +282,6 @@ pub fn create_greeter_window(
fingerprint_available: false, fingerprint_available: false,
user_switch_generation: 0, user_switch_generation: 0,
fingerprint_probe: None, fingerprint_probe: None,
fingerprint_probe_initializing: false,
})); }));
// Root overlay for layering // Root overlay for layering
@ -497,10 +493,6 @@ pub fn create_greeter_window(
let Some(user) = user else { return }; let Some(user) = user else { return };
let password = Zeroizing::new(entry.text().to_string()); let password = Zeroizing::new(entry.text().to_string());
// Clear the GTK entry's internal buffer as early as possible. GTK allocates
// the backing `GString` via libc malloc, which `zeroize` cannot reach — the
// best we can do is shorten the window during which it resides in memory.
entry.set_text("");
let session = get_selected_session(&session_dropdown, &sessions_rc); let session = get_selected_session(&session_dropdown, &sessions_rc);
let Some(session) = session else { let Some(session) = session else {
@ -510,7 +502,7 @@ pub fn create_greeter_window(
attempt_login( attempt_login(
&user, &user,
password, &password,
&session, &session,
strings, strings,
&state, &state,
@ -724,33 +716,12 @@ fn switch_to_user(
#[strong] #[strong]
state, state,
async move { async move {
// Initialize probe on first use, then reuse cached device proxy. // Initialize probe on first use, then reuse cached device proxy
// Atomic check-and-set on fingerprint_probe_initializing prevents let needs_init = state.borrow().fingerprint_probe.is_none();
// two concurrent probes (from a fast user switch) from both if needs_init {
// 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(); let mut probe = crate::fingerprint::FingerprintProbe::new();
probe.init_async().await; probe.init_async().await;
let mut s = state.borrow_mut(); state.borrow_mut().fingerprint_probe = Some(probe);
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 // Take probe out of state to avoid holding borrow across await
@ -807,40 +778,28 @@ fn set_avatar_from_file(
Ok(_) => {} Ok(_) => {}
} }
// Show fallback immediately; decode asynchronously via GIO so the greeter let Some(path_str) = path.to_str() else {
// stays responsive during a user-switch click. log::debug!("Non-UTF-8 avatar path, skipping: {}", path.display());
image.set_icon_name(Some("avatar-default-symbolic")); image.set_icon_name(Some("avatar-default-symbolic"));
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; return;
}
}; };
match Pixbuf::from_stream_at_scale_future(&stream, AVATAR_SIZE, AVATAR_SIZE, true).await {
match Pixbuf::from_file_at_scale(path_str, AVATAR_SIZE, AVATAR_SIZE, true) {
Ok(pixbuf) => { Ok(pixbuf) => {
let texture = gdk::Texture::for_pixbuf(&pixbuf); let texture = gdk::Texture::for_pixbuf(&pixbuf);
if let Some(ref name) = username_owned { if let Some(name) = username {
state_clone state
.borrow_mut() .borrow_mut()
.avatar_cache .avatar_cache
.insert(name.clone(), texture.clone()); .insert(name.to_string(), texture.clone());
} }
image_clone.set_paintable(Some(&texture)); image.set_paintable(Some(&texture));
} }
Err(e) => { Err(e) => {
log::debug!("Failed to decode avatar {}: {e}", display_path.display()); 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. /// Load the default avatar SVG from GResources, tinted with the foreground color.
@ -994,7 +953,7 @@ fn set_login_sensitive(
#[allow(clippy::too_many_arguments)] #[allow(clippy::too_many_arguments)]
fn attempt_login( fn attempt_login(
user: &User, user: &User,
password: Zeroizing<String>, password: &str,
session: &Session, session: &Session,
strings: &'static Strings, strings: &'static Strings,
state: &Rc<RefCell<GreeterState>>, state: &Rc<RefCell<GreeterState>>,
@ -1033,6 +992,7 @@ fn attempt_login(
set_login_sensitive(password_entry, session_dropdown, false); set_login_sensitive(password_entry, session_dropdown, false);
let username = user.username.clone(); let username = user.username.clone();
let password = Zeroizing::new(password.to_string());
let exec_cmd = session.exec_cmd.clone(); let exec_cmd = session.exec_cmd.clone();
let session_name = session.name.clone(); let session_name = session.name.clone();
let greetd_sock = state.borrow().greetd_sock.clone(); let greetd_sock = state.borrow().greetd_sock.clone();
@ -1073,13 +1033,6 @@ fn attempt_login(
glib::timeout_future(min_response - elapsed).await; 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 { match result {
Ok(Ok(LoginResult::Success { username })) => { Ok(Ok(LoginResult::Success { username })) => {
save_last_user(&username); save_last_user(&username);

View File

@ -286,10 +286,6 @@ mod tests {
assert!(!s.faillock_attempts_remaining.is_empty(), "{locale}: faillock_attempts_remaining"); assert!(!s.faillock_attempts_remaining.is_empty(), "{locale}: faillock_attempts_remaining");
assert!(!s.faillock_locked.is_empty(), "{locale}: faillock_locked"); assert!(!s.faillock_locked.is_empty(), "{locale}: faillock_locked");
assert!(!s.unexpected_greetd_response.is_empty(), "{locale}: unexpected_greetd_response"); 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");
} }
} }

View File

@ -127,12 +127,10 @@ fn setup_logging() {
eprintln!("Failed to create journal logger: {e}"); eprintln!("Failed to create journal logger: {e}");
} }
} }
// Require MOONGREET_DEBUG=1 to raise verbosity. Mere presence (e.g. an let level = if std::env::var("MOONGREET_DEBUG").is_ok() {
// empty value in a session-setup script) must not escalate the journal log::LevelFilter::Debug
// to Debug, which leaks socket paths, usernames, and auth round counts. } else {
let level = match std::env::var("MOONGREET_DEBUG").ok().as_deref() { log::LevelFilter::Info
Some("1") => log::LevelFilter::Debug,
_ => log::LevelFilter::Info,
}; };
log::set_max_level(level); log::set_max_level(level);
} }

View File

@ -40,9 +40,7 @@ fn run_command(action: &'static str, program: &str, args: &[&str]) -> Result<(),
log::debug!("Power action: {action} ({program} {args:?})"); log::debug!("Power action: {action} ({program} {args:?})");
let mut child = Command::new(program) let mut child = Command::new(program)
.args(args) .args(args)
// stdout is never read; piping without draining would deadlock on any .stdout(Stdio::piped())
// command that writes more than one OS pipe buffer before wait() returns.
.stdout(Stdio::null())
.stderr(Stdio::piped()) .stderr(Stdio::piped())
.spawn() .spawn()
.map_err(|e| PowerError::CommandFailed { .map_err(|e| PowerError::CommandFailed {

View File

@ -23,8 +23,6 @@ fn parse_desktop_file(path: &Path, session_type: &str) -> Option<Session> {
let mut in_section = false; let mut in_section = false;
let mut name: Option<String> = None; let mut name: Option<String> = None;
let mut exec_cmd: Option<String> = None; let mut exec_cmd: Option<String> = None;
let mut hidden = false;
let mut no_display = false;
for line in content.lines() { for line in content.lines() {
let line = line.trim(); let line = line.trim();
@ -46,18 +44,9 @@ fn parse_desktop_file(path: &Path, session_type: &str) -> Option<Session> {
&& exec_cmd.is_none() && exec_cmd.is_none()
{ {
exec_cmd = Some(value.to_string()); exec_cmd = Some(value.to_string());
} else if let Some(value) = line.strip_prefix("Hidden=") {
hidden = value.eq_ignore_ascii_case("true");
} else if let Some(value) = line.strip_prefix("NoDisplay=") {
no_display = value.eq_ignore_ascii_case("true");
} }
} }
if hidden || no_display {
log::debug!("Skipping {}: Hidden/NoDisplay entry", path.display());
return None;
}
let name = name.filter(|s| !s.is_empty()); let name = name.filter(|s| !s.is_empty());
let exec_cmd = exec_cmd.filter(|s| !s.is_empty()); let exec_cmd = exec_cmd.filter(|s| !s.is_empty());