From 0789e8fc27a5739be468a6cf751162d479c7f622 Mon Sep 17 00:00:00 2001 From: nevaforget Date: Fri, 24 Apr 2026 13:49:48 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20audit=20MEDIUM=20fixes=20=E2=80=94=20tim?= =?UTF-8?q?eout=20guard,=20POSIX=20locale,=20button=20gate,=20wallpaper=20?= =?UTF-8?q?allowlist=20(v0.8.4)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - power: RAII DoneGuard sets done=true on every wait() exit path, so the timeout thread no longer sleeps its full 30 s holding a spawn_blocking slot when child.wait() errors. A separate timed_out AtomicBool marks our own SIGKILL so we do not misclassify an external OOM-kill. Memory ordering on the flags is now Release/Acquire. - i18n: detect_locale now reads LC_ALL, LC_MESSAGES, LANG in POSIX priority order before falling back to /etc/locale.conf, so systems installed in English with LC_ALL=de_DE.UTF-8 pick up the correct UI. - panel: execute_action desensitizes button_box on entry and re-enables it on error paths, so double-click or keyboard repeat cannot fire the same power action twice. - config: accept_wallpaper helper applies an extension allowlist (jpg, jpeg, png, webp) plus symlink rejection and a 10 MB size cap, applied to both the user-configured path and the Moonarch ecosystem fallback. Bounds worst-case decode latency and narrows the gdk-pixbuf parser attack surface. --- Cargo.lock | 2 +- Cargo.toml | 2 +- DECISIONS.md | 7 +++++++ src/config.rs | 51 +++++++++++++++++++++++++++++++++++++++++++-------- src/i18n.rs | 13 ++++++++++--- src/panel.rs | 16 +++++++++++++++- src/power.rs | 34 ++++++++++++++++++++-------------- 7 files changed, 97 insertions(+), 28 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 895ccf3..b633a0f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -616,7 +616,7 @@ dependencies = [ [[package]] name = "moonset" -version = "0.8.3" +version = "0.8.4" dependencies = [ "dirs", "gdk-pixbuf", diff --git a/Cargo.toml b/Cargo.toml index f3cbc9b..3c3d84a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "moonset" -version = "0.8.3" +version = "0.8.4" edition = "2024" description = "Wayland session power menu with GTK4 and Layer Shell" license = "MIT" diff --git a/DECISIONS.md b/DECISIONS.md index 72d4dd7..a03015a 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -2,6 +2,13 @@ Architectural and design decisions for Moonset, in reverse chronological order. +## 2026-04-24 – Audit MEDIUM fixes: timeout guard, POSIX locale, button desensitize, wallpaper allowlist (v0.8.4) + +- **Who**: ClaudeCode, Dom +- **Why**: Five MEDIUM findings: (1) `run_command`'s timeout thread leaked a 30 s gio::spawn_blocking slot if `child.wait()` errored, because `done.store(true)` ran after the `?`. (2) Timeout detection compared `status.signal() == Some(9)` — a hardcoded signal number that also misclassifies OOM-killer SIGKILL as our timeout. (3) `execute_action` never desensitized the button_box, so a double-click or accidental keyboard repeat fired the action twice. (4) `detect_locale` read only `LANG`, ignoring POSIX priority order (`LC_ALL` > `LC_MESSAGES` > `LANG`) — a common dual-language setup picked the wrong UI language. (5) The wallpaper path was passed to gdk-pixbuf without extension or size restriction, widening the image-parser attack surface and allowing unbounded decode latency. +- **Tradeoffs**: The extension allowlist (`jpg`, `jpeg`, `png`, `webp`) rejects exotic formats users might have used before. The 10 MB size cap rejects uncompressed/high-quality 4K wallpapers; acceptable for a power menu. Memory ordering on the `done` flag is now `Release`/`Acquire` instead of `Relaxed` — no runtime cost but correct across threads. +- **How**: (1) RAII `DoneGuard` struct sets `done.store(true, Release)` in its `Drop`, so the flag fires on every function exit path. A second `timed_out` AtomicBool distinguishes our SIGKILL from an external one. (2) Replace `Some(9)` with the `timed_out` flag check. (3) `execute_action` now takes `button_box: >k::Box`, calls `set_sensitive(false)` on entry and re-enables it on error paths; success paths that quit skip the re-enable. All call sites updated. (4) `detect_locale` reads `LC_ALL`, `LC_MESSAGES`, `LANG` in order, picking the first non-empty value before falling back to `/etc/locale.conf`. (5) `accept_wallpaper` helper applies extension allowlist + symlink rejection + `MAX_WALLPAPER_FILE_SIZE = 10 MB`, and is called for both config-path and Moonarch fallback. + ## 2026-04-24 – Audit fix: avoid latent stdout pipe deadlock in run_command (v0.8.3) - **Who**: ClaudeCode, Dom diff --git a/src/config.rs b/src/config.rs index 61f6280..9bcaf0f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -62,21 +62,56 @@ pub fn resolve_background_path(config: &Config) -> Option { resolve_background_path_with(config, Path::new(MOONARCH_WALLPAPER)) } +/// Wallpapers are passed to gdk-pixbuf's image loader; restrict to common image +/// extensions to reduce the parser-attack surface for user-controlled paths. +const ALLOWED_BG_EXT: &[&str] = &["jpg", "jpeg", "png", "webp"]; + +/// Bound wallpaper decode latency (10 MB covers typical 4K JPEGs at Q95). +const MAX_WALLPAPER_FILE_SIZE: u64 = 10 * 1024 * 1024; + +fn is_allowed_wallpaper(path: &Path) -> bool { + match path.extension().and_then(|e| e.to_str()) { + Some(ext) => ALLOWED_BG_EXT.iter().any(|a| a.eq_ignore_ascii_case(ext)), + None => false, + } +} + +fn accept_wallpaper(path: &Path) -> bool { + if !is_allowed_wallpaper(path) { + log::warn!("Wallpaper rejected (extension not in allowlist): {}", path.display()); + return false; + } + match path.symlink_metadata() { + Ok(meta) if meta.file_type().is_symlink() => { + log::warn!("Wallpaper rejected (symlink): {}", path.display()); + false + } + Ok(meta) if !meta.is_file() => false, + Ok(meta) if meta.len() > MAX_WALLPAPER_FILE_SIZE => { + log::warn!( + "Wallpaper rejected ({} bytes > {} limit): {}", + meta.len(), MAX_WALLPAPER_FILE_SIZE, path.display() + ); + false + } + Ok(_) => true, + Err(_) => false, + } +} + /// Resolve with configurable moonarch wallpaper path (for testing). pub fn resolve_background_path_with(config: &Config, moonarch_wallpaper: &Path) -> Option { - // User-configured path — reject symlinks to prevent path traversal + // User-configured path — reject symlinks, non-image extensions, and oversized files if let Some(ref bg) = config.background_path { let path = PathBuf::from(bg); - if let Ok(meta) = path.symlink_metadata() { - if meta.is_file() && !meta.file_type().is_symlink() { - log::debug!("Wallpaper source: config ({})", path.display()); - return Some(path); - } + if accept_wallpaper(&path) { + log::debug!("Wallpaper source: config ({})", path.display()); + return Some(path); } } - // Moonarch ecosystem default - if moonarch_wallpaper.is_file() { + // Moonarch ecosystem default — apply the same checks for consistency + if accept_wallpaper(moonarch_wallpaper) { log::debug!("Wallpaper source: moonarch default ({})", moonarch_wallpaper.display()); return Some(moonarch_wallpaper.to_path_buf()); } diff --git a/src/i18n.rs b/src/i18n.rs index 6452f36..a55e3ba 100644 --- a/src/i18n.rs +++ b/src/i18n.rs @@ -110,15 +110,22 @@ fn read_lang_from_conf(path: &Path) -> Option { None } -/// Determine the system language from LANG env var or /etc/locale.conf. +/// Determine the system language from POSIX locale env vars or /etc/locale.conf. +/// Checks LC_ALL, LC_MESSAGES, LANG in POSIX priority order (LC_ALL overrides +/// everything; LC_MESSAGES overrides LANG for text categories). pub fn detect_locale() -> String { - detect_locale_with(env::var("LANG").ok().as_deref(), Path::new(DEFAULT_LOCALE_CONF)) + let env_val = env::var("LC_ALL") + .ok() + .filter(|s| !s.is_empty()) + .or_else(|| env::var("LC_MESSAGES").ok().filter(|s| !s.is_empty())) + .or_else(|| env::var("LANG").ok().filter(|s| !s.is_empty())); + detect_locale_with(env_val.as_deref(), Path::new(DEFAULT_LOCALE_CONF)) } /// Determine locale with configurable inputs (for testing). pub fn detect_locale_with(env_lang: Option<&str>, locale_conf_path: &Path) -> String { let (raw, source) = if let Some(val) = env_lang.filter(|s| !s.is_empty()) { - (Some(val.to_string()), "LANG env") + (Some(val.to_string()), "env") } else if let Some(val) = read_lang_from_conf(locale_conf_path) { (Some(val), "locale.conf") } else { diff --git a/src/panel.rs b/src/panel.rs index f82066a..2cc6571 100644 --- a/src/panel.rs +++ b/src/panel.rs @@ -445,7 +445,7 @@ fn on_action_clicked( error_label.set_visible(false); if !action_def.needs_confirm { - execute_action(action_def, strings, app, confirm_area, confirm_box, error_label); + execute_action(action_def, strings, app, confirm_area, confirm_box, error_label, button_box); return; } @@ -488,6 +488,8 @@ fn show_confirm( confirm_box, #[weak] error_label, + #[weak] + button_box, move |_| { execute_action( &action_def_clone, @@ -496,6 +498,7 @@ fn show_confirm( &confirm_area, &confirm_box, &error_label, + &button_box, ); } )); @@ -543,6 +546,7 @@ fn execute_action( confirm_area: >k::Box, confirm_box: &Rc>>, error_label: >k::Label, + button_box: >k::Box, ) { dismiss_confirm(confirm_area, confirm_box); log::debug!("Executing power action: {}", action_def.name); @@ -552,6 +556,10 @@ fn execute_action( let quit_after = action_def.quit_after; let error_message = (action_def.error_attr)(strings).to_string(); + // Desensitize buttons so a double-click or accidental keyboard repeat + // cannot fire the same action twice while it is in flight. + button_box.set_sensitive(false); + // Use glib::spawn_future_local + gio::spawn_blocking to avoid Send issues // with GTK objects. The blocking closure runs on a thread pool, the result // is handled back on the main thread. @@ -560,6 +568,8 @@ fn execute_action( app, #[weak] error_label, + #[weak] + button_box, async move { let result = gio::spawn_blocking(move || action_fn()).await; @@ -567,17 +577,21 @@ fn execute_action( Ok(Ok(())) => { if quit_after { fade_out_and_quit(&app); + } else { + button_box.set_sensitive(true); } } Ok(Err(e)) => { log::error!("Power action '{}' failed: {}", action_name, e); error_label.set_text(&error_message); error_label.set_visible(true); + button_box.set_sensitive(true); } Err(_) => { log::error!("Power action '{}' panicked", action_name); error_label.set_text(&error_message); error_label.set_visible(true); + button_box.set_sensitive(true); } } } diff --git a/src/power.rs b/src/power.rs index d8eb508..c041957 100644 --- a/src/power.rs +++ b/src/power.rs @@ -52,44 +52,50 @@ fn run_command(action: &'static str, program: &str, args: &[&str]) -> Result<(), let child_pid = nix::unistd::Pid::from_raw(child.id() as i32); let done = Arc::new(AtomicBool::new(false)); + let timed_out = Arc::new(AtomicBool::new(false)); let done_clone = done.clone(); + let timed_out_clone = timed_out.clone(); - let timeout_thread = std::thread::spawn(move || { - // Sleep in short intervals so we can exit early when the child finishes + let _timeout_thread = std::thread::spawn(move || { let interval = Duration::from_millis(100); let mut elapsed = Duration::ZERO; while elapsed < POWER_TIMEOUT { std::thread::sleep(interval); - if done_clone.load(Ordering::Relaxed) { + if done_clone.load(Ordering::Acquire) { return; } elapsed += interval; } + // Record that we fired the kill so we don't misclassify an external + // SIGKILL (OOM killer, kill -9) as our timeout. + timed_out_clone.store(true, Ordering::Release); // ESRCH if the process already exited — harmless let _ = nix::sys::signal::kill(child_pid, nix::sys::signal::Signal::SIGKILL); }); + // Drop guard ensures the timeout thread sees done=true even if child.wait() + // errors out — otherwise the thread sleeps its full 30 s holding a slot in + // the gio::spawn_blocking pool. + struct DoneGuard(Arc); + impl Drop for DoneGuard { + fn drop(&mut self) { + self.0.store(true, Ordering::Release); + } + } + let _done_guard = DoneGuard(done); + let status = child.wait().map_err(|e| PowerError::CommandFailed { action, message: e.to_string(), })?; - done.store(true, Ordering::Relaxed); - let _ = timeout_thread.join(); - if status.success() { log::debug!("Power action {action} completed"); Ok(()) } else { - // Check if killed by our timeout (SIGKILL = signal 9) - #[cfg(unix)] - { - use std::os::unix::process::ExitStatusExt; - if status.signal() == Some(9) { - return Err(PowerError::Timeout { action }); - } + if timed_out.load(Ordering::Acquire) { + return Err(PowerError::Timeout { action }); } - let mut stderr_buf = String::new(); if let Some(mut stderr) = child.stderr.take() { let _ = stderr.read_to_string(&mut stderr_buf);