fix: audit MEDIUM fixes — timeout guard, POSIX locale, button gate, wallpaper allowlist (v0.8.4)
- 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.
This commit is contained in:
+43
-8
@@ -62,21 +62,56 @@ pub fn resolve_background_path(config: &Config) -> Option<PathBuf> {
|
||||
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<PathBuf> {
|
||||
// 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());
|
||||
}
|
||||
|
||||
+10
-3
@@ -110,15 +110,22 @@ fn read_lang_from_conf(path: &Path) -> Option<String> {
|
||||
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 {
|
||||
|
||||
+15
-1
@@ -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<RefCell<Option<gtk::Box>>>,
|
||||
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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
+20
-14
@@ -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<AtomicBool>);
|
||||
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);
|
||||
|
||||
Reference in New Issue
Block a user