fix: audit fixes — blur offset, lock-before-IO, FP signal lifecycle, TOCTOU (v0.6.6)
Update PKGBUILD version / update-pkgver (push) Successful in 2s
Update PKGBUILD version / update-pkgver (push) Successful in 2s
Third triple audit (quality, performance, security). Key fixes: - Blur padding offset: texture at (-pad,-pad) prevents edge darkening on all sides - Wallpaper loads after lock.lock() — disk I/O no longer delays lock acquisition - begin_verification disconnects old signal handler before registering new one - resume_async resets failed_attempts to prevent premature exhaustion - Unknown VerifyStatus with done=true triggers restart instead of hanging - symlink_metadata() replaces separate is_file()+is_symlink() (TOCTOU) - faillock_warning dead code removed, blur sigma clamped to [0,100] - Redundant Zeroizing<Vec<u8>> removed, on_verify_status restricted to pub(crate) - Warn logging for non-UTF-8 GECOS and avatar path errors - Default impl for FingerprintListener, 3 new tests (47 total)
This commit is contained in:
+14
-3
@@ -149,9 +149,8 @@ unsafe extern "C" fn pam_conv_callback(
|
||||
/// Returns true on success, false on authentication failure.
|
||||
/// The password is wiped from memory after use via zeroize.
|
||||
pub fn authenticate(username: &str, password: &str) -> bool {
|
||||
// Use Zeroizing to ensure password bytes are wiped on drop
|
||||
let password_bytes = Zeroizing::new(password.as_bytes().to_vec());
|
||||
let password_cstr = match CString::new(password_bytes.as_slice()) {
|
||||
// CString::new takes ownership of the Vec — Zeroizing<CString> wipes on drop
|
||||
let password_cstr = match CString::new(password.as_bytes().to_vec()) {
|
||||
Ok(c) => Zeroizing::new(c),
|
||||
Err(_) => return false, // Password contains null byte
|
||||
};
|
||||
@@ -286,4 +285,16 @@ mod tests {
|
||||
let result = authenticate("", "password");
|
||||
assert!(!result);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn check_account_empty_username_fails() {
|
||||
let result = check_account("");
|
||||
assert!(!result);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn check_account_null_byte_username_fails() {
|
||||
let result = check_account("user\0name");
|
||||
assert!(!result);
|
||||
}
|
||||
}
|
||||
|
||||
+6
-2
@@ -51,7 +51,9 @@ pub fn load_config(config_paths: Option<&[PathBuf]>) -> Config {
|
||||
match toml::from_str::<RawConfig>(&content) {
|
||||
Ok(parsed) => {
|
||||
if parsed.background_path.is_some() { merged.background_path = parsed.background_path; }
|
||||
if parsed.background_blur.is_some() { merged.background_blur = parsed.background_blur; }
|
||||
if let Some(blur) = parsed.background_blur {
|
||||
merged.background_blur = Some(blur.clamp(0.0, 100.0));
|
||||
}
|
||||
if let Some(fp) = parsed.fingerprint_enabled { merged.fingerprint_enabled = fp; }
|
||||
}
|
||||
Err(e) => {
|
||||
@@ -70,7 +72,9 @@ pub fn resolve_background_path(config: &Config) -> Option<PathBuf> {
|
||||
pub fn resolve_background_path_with(config: &Config, moonarch_wallpaper: &Path) -> Option<PathBuf> {
|
||||
if let Some(ref bg) = config.background_path {
|
||||
let path = PathBuf::from(bg);
|
||||
if path.is_file() && !path.is_symlink() { return Some(path); }
|
||||
if let Ok(meta) = path.symlink_metadata() {
|
||||
if meta.is_file() && !meta.file_type().is_symlink() { return Some(path); }
|
||||
}
|
||||
}
|
||||
if moonarch_wallpaper.is_file() { return Some(moonarch_wallpaper.to_path_buf()); }
|
||||
None
|
||||
|
||||
+24
-7
@@ -35,10 +35,8 @@ pub struct FingerprintListener {
|
||||
on_exhausted: Option<Box<dyn Fn() + 'static>>,
|
||||
}
|
||||
|
||||
impl FingerprintListener {
|
||||
/// Create a lightweight FingerprintListener without any D-Bus calls.
|
||||
/// Call `init_async().await` afterwards to connect to fprintd.
|
||||
pub fn new() -> Self {
|
||||
impl Default for FingerprintListener {
|
||||
fn default() -> Self {
|
||||
FingerprintListener {
|
||||
device_proxy: None,
|
||||
signal_id: None,
|
||||
@@ -50,6 +48,14 @@ impl FingerprintListener {
|
||||
on_exhausted: None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl FingerprintListener {
|
||||
/// Create a lightweight FingerprintListener without any D-Bus calls.
|
||||
/// Call `init_async().await` afterwards to connect to fprintd.
|
||||
pub fn new() -> Self {
|
||||
Self::default()
|
||||
}
|
||||
|
||||
/// Connect to fprintd and get the default device asynchronously.
|
||||
pub async fn init_async(&mut self) {
|
||||
@@ -171,6 +177,7 @@ impl FingerprintListener {
|
||||
listener: &Rc<RefCell<FingerprintListener>>,
|
||||
username: &str,
|
||||
) {
|
||||
listener.borrow_mut().failed_attempts = 0;
|
||||
Self::begin_verification(listener, username).await;
|
||||
}
|
||||
|
||||
@@ -181,11 +188,16 @@ impl FingerprintListener {
|
||||
username: &str,
|
||||
) {
|
||||
let proxy = {
|
||||
let inner = listener.borrow();
|
||||
match inner.device_proxy.clone() {
|
||||
let mut inner = listener.borrow_mut();
|
||||
let proxy = match inner.device_proxy.clone() {
|
||||
Some(p) => p,
|
||||
None => return,
|
||||
};
|
||||
// Disconnect any previous signal handler to prevent duplicates on resume
|
||||
if let Some(old_id) = inner.signal_id.take() {
|
||||
proxy.disconnect(old_id);
|
||||
}
|
||||
proxy
|
||||
};
|
||||
|
||||
// Claim the device
|
||||
@@ -265,7 +277,7 @@ impl FingerprintListener {
|
||||
}
|
||||
|
||||
/// Process a VerifyStatus signal from fprintd.
|
||||
pub fn on_verify_status(&mut self, status: &str, done: bool) {
|
||||
pub(crate) fn on_verify_status(&mut self, status: &str, done: bool) {
|
||||
if !self.running {
|
||||
return;
|
||||
}
|
||||
@@ -305,6 +317,9 @@ impl FingerprintListener {
|
||||
}
|
||||
|
||||
log::debug!("Unhandled fprintd status: {status}");
|
||||
if done {
|
||||
self.restart_verify_async();
|
||||
}
|
||||
}
|
||||
|
||||
/// Restart fingerprint verification asynchronously after a completed attempt.
|
||||
@@ -403,11 +418,13 @@ mod tests {
|
||||
let called_clone = called.clone();
|
||||
let mut listener = FingerprintListener::new();
|
||||
listener.running = true;
|
||||
listener.running_flag.set(true);
|
||||
listener.on_failure = Some(Box::new(move || { called_clone.set(true); }));
|
||||
|
||||
listener.on_verify_status("verify-no-match", false);
|
||||
assert!(called.get());
|
||||
assert!(listener.running);
|
||||
assert!(listener.running_flag.get());
|
||||
assert_eq!(listener.failed_attempts, 1);
|
||||
}
|
||||
|
||||
|
||||
+20
-3
@@ -100,9 +100,10 @@ pub fn load_strings(locale: Option<&str>) -> &'static Strings {
|
||||
match locale { "de" => &STRINGS_DE, _ => &STRINGS_EN }
|
||||
}
|
||||
|
||||
/// Returns a warning when the user is one attempt away from lockout.
|
||||
/// Caller is responsible for handling the locked state (count >= max_attempts).
|
||||
pub fn faillock_warning(attempt_count: u32, max_attempts: u32, strings: &Strings) -> Option<String> {
|
||||
if attempt_count >= max_attempts { return Some(strings.faillock_locked.to_string()); }
|
||||
let remaining = max_attempts - attempt_count;
|
||||
let remaining = max_attempts.saturating_sub(attempt_count);
|
||||
if remaining == 1 { return Some(strings.faillock_attempts_remaining.replace("{n}", &remaining.to_string())); }
|
||||
None
|
||||
}
|
||||
@@ -139,5 +140,21 @@ mod tests {
|
||||
#[test] fn faillock_zero() { assert!(faillock_warning(0, 3, load_strings(Some("en"))).is_none()); }
|
||||
#[test] fn faillock_one() { assert!(faillock_warning(1, 3, load_strings(Some("en"))).is_none()); }
|
||||
#[test] fn faillock_two() { assert!(faillock_warning(2, 3, load_strings(Some("en"))).is_some()); }
|
||||
#[test] fn faillock_three() { assert_eq!(faillock_warning(3, 3, load_strings(Some("en"))).unwrap(), "Account may be locked"); }
|
||||
#[test] fn faillock_three() { assert!(faillock_warning(3, 3, load_strings(Some("en"))).is_none()); }
|
||||
|
||||
#[test]
|
||||
fn faillock_caller_contract() {
|
||||
// Mirrors the lockscreen.rs usage: caller handles count >= max separately,
|
||||
// faillock_warning is only called when count < max.
|
||||
let max = 3u32;
|
||||
let strings = load_strings(Some("en"));
|
||||
for count in 0..max {
|
||||
let result = faillock_warning(count, max, strings);
|
||||
if max - count == 1 {
|
||||
assert!(result.is_some(), "should warn at count={count}");
|
||||
} else {
|
||||
assert!(result.is_none(), "should not warn at count={count}");
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
+12
-3
@@ -609,7 +609,7 @@ fn render_blurred_texture(
|
||||
snapshot.push_clip(&graphene::Rect::new(pad, pad, w, h));
|
||||
snapshot.push_blur(scaled_sigma as f64);
|
||||
// Render texture with padding on all sides (edges repeat via oversized bounds)
|
||||
snapshot.append_texture(texture, &graphene::Rect::new(0.0, 0.0, w + 2.0 * pad, h + 2.0 * pad));
|
||||
snapshot.append_texture(texture, &graphene::Rect::new(-pad, -pad, w + 2.0 * pad, h + 2.0 * pad));
|
||||
snapshot.pop(); // blur
|
||||
snapshot.pop(); // clip
|
||||
|
||||
@@ -624,13 +624,22 @@ fn set_avatar_from_file(
|
||||
path: &Path,
|
||||
cache: &Rc<RefCell<Option<gdk::Texture>>>,
|
||||
) {
|
||||
match Pixbuf::from_file_at_scale(path.to_str().unwrap_or(""), AVATAR_SIZE, AVATAR_SIZE, true) {
|
||||
let path_str = match path.to_str() {
|
||||
Some(s) => s,
|
||||
None => {
|
||||
log::warn!("Avatar path is not valid UTF-8: {:?}", path);
|
||||
image.set_icon_name(Some("avatar-default-symbolic"));
|
||||
return;
|
||||
}
|
||||
};
|
||||
match Pixbuf::from_file_at_scale(path_str, AVATAR_SIZE, AVATAR_SIZE, true) {
|
||||
Ok(pixbuf) => {
|
||||
let texture = gdk::Texture::for_pixbuf(&pixbuf);
|
||||
image.set_paintable(Some(&texture));
|
||||
*cache.borrow_mut() = Some(texture);
|
||||
}
|
||||
Err(_) => {
|
||||
Err(e) => {
|
||||
log::warn!("Failed to load avatar from {:?}: {e}", path);
|
||||
image.set_icon_name(Some("avatar-default-symbolic"));
|
||||
}
|
||||
}
|
||||
|
||||
+11
-8
@@ -40,16 +40,14 @@ fn activate(app: >k::Application) {
|
||||
load_css(&display);
|
||||
|
||||
let config = config::load_config(None);
|
||||
let bg_texture = config::resolve_background_path(&config)
|
||||
.and_then(|path| lockscreen::load_background_texture(&path));
|
||||
|
||||
if gtk4_session_lock::is_supported() {
|
||||
activate_with_session_lock(app, &display, bg_texture.as_ref(), &config);
|
||||
activate_with_session_lock(app, &display, &config);
|
||||
} else {
|
||||
#[cfg(debug_assertions)]
|
||||
{
|
||||
log::warn!("ext-session-lock-v1 not supported — running in development mode");
|
||||
activate_without_lock(app, bg_texture.as_ref(), &config);
|
||||
activate_without_lock(app, &config);
|
||||
}
|
||||
#[cfg(not(debug_assertions))]
|
||||
{
|
||||
@@ -62,12 +60,15 @@ fn activate(app: >k::Application) {
|
||||
fn activate_with_session_lock(
|
||||
app: >k::Application,
|
||||
display: &gdk::Display,
|
||||
bg_texture: Option<&gdk::Texture>,
|
||||
config: &config::Config,
|
||||
) {
|
||||
let lock = gtk4_session_lock::Instance::new();
|
||||
lock.lock();
|
||||
|
||||
// Load wallpaper AFTER lock — disk I/O must not delay the lock acquisition
|
||||
let bg_texture = config::resolve_background_path(config)
|
||||
.and_then(|path| lockscreen::load_background_texture(&path));
|
||||
|
||||
let monitors = display.monitors();
|
||||
|
||||
// Shared unlock callback — unlocks session and quits.
|
||||
@@ -99,7 +100,7 @@ fn activate_with_session_lock(
|
||||
.and_then(|obj| obj.downcast::<gdk::Monitor>().ok())
|
||||
{
|
||||
let handles = lockscreen::create_lockscreen_window(
|
||||
bg_texture,
|
||||
bg_texture.as_ref(),
|
||||
config,
|
||||
app,
|
||||
unlock_callback.clone(),
|
||||
@@ -158,9 +159,11 @@ fn init_fingerprint_async(all_handles: Vec<lockscreen::LockscreenHandles>) {
|
||||
#[cfg(debug_assertions)]
|
||||
fn activate_without_lock(
|
||||
app: >k::Application,
|
||||
bg_texture: Option<&gdk::Texture>,
|
||||
config: &config::Config,
|
||||
) {
|
||||
let bg_texture = config::resolve_background_path(config)
|
||||
.and_then(|path| lockscreen::load_background_texture(&path));
|
||||
|
||||
let app_clone = app.clone();
|
||||
let unlock_callback: Rc<dyn Fn()> = Rc::new(move || {
|
||||
app_clone.quit();
|
||||
@@ -169,7 +172,7 @@ fn activate_without_lock(
|
||||
let blur_cache = Rc::new(RefCell::new(None));
|
||||
let avatar_cache = Rc::new(RefCell::new(None));
|
||||
let handles = lockscreen::create_lockscreen_window(
|
||||
bg_texture,
|
||||
bg_texture.as_ref(),
|
||||
config,
|
||||
app,
|
||||
unlock_callback,
|
||||
|
||||
+14
-4
@@ -18,7 +18,13 @@ pub struct User {
|
||||
pub fn get_current_user() -> Option<User> {
|
||||
let uid = getuid();
|
||||
let nix_user = NixUser::from_uid(uid).ok()??;
|
||||
let gecos = nix_user.gecos.to_str().unwrap_or("").to_string();
|
||||
let gecos = match nix_user.gecos.to_str() {
|
||||
Ok(s) => s.to_string(),
|
||||
Err(_) => {
|
||||
log::warn!("GECOS field is not valid UTF-8, falling back to username");
|
||||
String::new()
|
||||
}
|
||||
};
|
||||
let display_name = if !gecos.is_empty() {
|
||||
let first = gecos.split(',').next().unwrap_or("");
|
||||
if first.is_empty() { nix_user.name.clone() } else { first.to_string() }
|
||||
@@ -31,13 +37,17 @@ pub fn get_avatar_path(home: &Path, username: &str) -> Option<PathBuf> {
|
||||
}
|
||||
|
||||
pub fn get_avatar_path_with(home: &Path, username: &str, accountsservice_dir: &Path) -> Option<PathBuf> {
|
||||
// ~/.face takes priority
|
||||
// ~/.face takes priority — single stat via symlink_metadata to avoid TOCTOU
|
||||
let face = home.join(".face");
|
||||
if face.exists() && !face.is_symlink() { return Some(face); }
|
||||
if let Ok(meta) = face.symlink_metadata() {
|
||||
if meta.is_file() && !meta.file_type().is_symlink() { return Some(face); }
|
||||
}
|
||||
// AccountsService icon
|
||||
if accountsservice_dir.exists() {
|
||||
let icon = accountsservice_dir.join(username);
|
||||
if icon.exists() && !icon.is_symlink() { return Some(icon); }
|
||||
if let Ok(meta) = icon.symlink_metadata() {
|
||||
if meta.is_file() && !meta.file_type().is_symlink() { return Some(icon); }
|
||||
}
|
||||
}
|
||||
None
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user