From 5405d3081e7ceebbdaf854540c4e884aff4e6a8f Mon Sep 17 00:00:00 2001 From: "Brian J. Tarricone" Date: Mon, 30 May 2022 18:27:19 -0700 Subject: [PATCH] Increase dialog exit status parsing safety in the locker This also stops using -1 as auth failed, and moves all failure statuses to positive numbers. It looks like Exiting with -1 ends up setting the status to 255 on exit, but then in the locker, it sees this as 255 and not -1, since things get coerced into 32-bit integers. --- dialog-gtk3/src/main.rs | 2 +- locker/src/screensaver.rs | 28 +++++++++------- util/src/dialog.rs | 68 +++++++++++++++++++++++++++++++++++++++ util/src/lib.rs | 17 +--------- 4 files changed, 87 insertions(+), 28 deletions(-) create mode 100644 util/src/dialog.rs diff --git a/dialog-gtk3/src/main.rs b/dialog-gtk3/src/main.rs index 47a8894..cabac19 100644 --- a/dialog-gtk3/src/main.rs +++ b/dialog-gtk3/src/main.rs @@ -6,7 +6,7 @@ use gtk::{prelude::*, Button, Entry, Label, Plug, Window}; use log::{debug, error, warn}; use std::{io::{self, Write}, thread, time::Duration}; -use bscreensaver_util::{init_logging, DialogExitStatus, settings::Configuration, desktop::NewLoginCommand}; +use bscreensaver_util::{init_logging, dialog::DialogExitStatus, settings::Configuration, desktop::NewLoginCommand}; const DIALOG_UPDATE_INTERVAL: Duration = Duration::from_millis(100); const DIALOG_TIMEOUT: Duration = Duration::from_secs(60); diff --git a/locker/src/screensaver.rs b/locker/src/screensaver.rs index 9b7138f..df61b56 100644 --- a/locker/src/screensaver.rs +++ b/locker/src/screensaver.rs @@ -1,7 +1,6 @@ use log::{debug, error, info, trace, warn}; use std::{ io::Read, - os::unix::process::ExitStatusExt, process::{Child, Command, Stdio}, time::Instant, path::{PathBuf, Path}, }; @@ -9,7 +8,7 @@ use xcb::{randr, x, Xid, XidNew}; use xcb_xembed::embedder::Embedder; use bscreensaver_command::{BCommand, create_command_window}; -use bscreensaver_util::{*, settings::Configuration}; +use bscreensaver_util::{*, dialog::DialogExitStatus, settings::Configuration}; use crate::{ monitor::*, @@ -308,17 +307,24 @@ impl<'a> Screensaver<'a> { warn!("Failed to check unlock dialog's status: {}", err); self.unlock_dialog = Some(unlock_dialog); }, - Ok(Some(status)) if status.success() => { - info!("Authentication succeeded"); - self.unlock_screen()?; - } - Ok(Some(status)) if status.signal().is_some() => { - if let Some(signum) = status.signal() { - warn!("Unlock dialog crashed with signal {}", signum); + Ok(Some(status)) => { + let ok = match DialogExitStatus::try_from(status) { + Ok(result) => { + info!("{}", result); + result == DialogExitStatus::AuthSucceeded + }, + Err(error) => { + warn!("{}", error); + false + }, + }; + + if ok { + self.unlock_screen()?; + } else { + self.hide_cursor(); } - self.hide_cursor(); }, - Ok(Some(_)) => self.hide_cursor(), // auth failed, dialog has quit, do nothing Ok(None) => self.unlock_dialog = Some(unlock_dialog), // dialog still running } } diff --git a/util/src/dialog.rs b/util/src/dialog.rs new file mode 100644 index 0000000..f8847e8 --- /dev/null +++ b/util/src/dialog.rs @@ -0,0 +1,68 @@ +use std::{error, fmt, process::ExitStatus, os::unix::prelude::ExitStatusExt}; + +#[derive(Debug, Clone, Copy)] +pub enum DialogExitStatusError { + NoStatus, + Crashed(i32), + InvalidStatus(i32), +} + +impl fmt::Display for DialogExitStatusError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use DialogExitStatusError::*; + match self { + InvalidStatus(other) => write!(f, "Unlock dialog exited with unexpected status {}", other), + Crashed(signum) => write!(f, "Unlock dialog crashed with signal {}", signum), + NoStatus => write!(f, "Unlock dialog exited unexpectedly"), + } + } +} + +impl error::Error for DialogExitStatusError {} + +#[repr(i32)] +#[derive(Debug, Clone, Copy, PartialEq)] +pub enum DialogExitStatus { + AuthSucceeded = 0, + AuthFailed = 1, + AuthCanceled = 2, + AuthTimedOut = 3, + OtherError = 4, +} + +impl DialogExitStatus { + pub fn exit(&self) -> ! { + std::process::exit(*self as i32); + } +} + +impl fmt::Display for DialogExitStatus { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use DialogExitStatus::*; + match self { + AuthSucceeded => write!(f, "Authentication succeeded"), + AuthFailed => write!(f, "Authentication failed"), + AuthCanceled => write!(f, "Authentication canceled"), + AuthTimedOut => write!(f, "Authentication timed out"), + OtherError => write!(f, "An unexpected error occurred with the unlock dialog"), + } + } +} + +impl TryFrom for DialogExitStatus { + type Error = DialogExitStatusError; + fn try_from(value: ExitStatus) -> Result { + match value.code() { + Some(0) => Ok(DialogExitStatus::AuthSucceeded), + Some(1) => Ok(DialogExitStatus::AuthFailed), + Some(2) => Ok(DialogExitStatus::AuthCanceled), + Some(3) => Ok(DialogExitStatus::AuthTimedOut), + Some(4) => Ok(DialogExitStatus::OtherError), + Some(other) => Err(DialogExitStatusError::InvalidStatus(other)), + None => match value.signal() { + Some(signum) => Err(DialogExitStatusError::Crashed(signum)), + None => Err(DialogExitStatusError::NoStatus), + }, + } + } +} diff --git a/util/src/lib.rs b/util/src/lib.rs index 7ecb590..f899c1b 100644 --- a/util/src/lib.rs +++ b/util/src/lib.rs @@ -2,26 +2,11 @@ use std::{ffi::CStr, io}; use xcb::x; pub mod desktop; +pub mod dialog; pub mod settings; pub const BSCREENSAVER_WM_CLASS: &[u8] = b"bscreensaver\0Bscreensaver\0"; -#[repr(i32)] -#[derive(Debug, Clone, Copy, PartialEq)] -pub enum DialogExitStatus { - AuthFailed = -1, - AuthSucceeded = 0, - AuthCanceled = 1, - AuthTimedOut = 2, - OtherError = 3, -} - -impl DialogExitStatus { - pub fn exit(&self) -> ! { - std::process::exit(*self as i32); - } -} - pub fn init_logging(env_name: &str) { env_logger::builder() .format_timestamp_millis()