diff --git a/Cargo.lock b/Cargo.lock index d276bab..fbedeab 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -575,7 +575,7 @@ dependencies = [ [[package]] name = "moonlock" -version = "0.6.18" +version = "0.6.19" dependencies = [ "gdk-pixbuf", "gdk4", diff --git a/Cargo.toml b/Cargo.toml index 5166d99..ed0be4b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "moonlock" -version = "0.6.18" +version = "0.6.19" edition = "2024" description = "A secure Wayland lockscreen with GTK4, PAM and fingerprint support" license = "MIT" diff --git a/DECISIONS.md b/DECISIONS.md index 8bce45b..1aa40c4 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -2,6 +2,13 @@ Architectural and design decisions for Moonlock, in reverse chronological order. +## 2026-06-17 – Harden avatar load against symlink TOCTOU via a shared O_NOFOLLOW loader (v0.6.19) + +- **Who**: ClaudeCode, Dom +- **Why**: A security audit found SEC-01 (LOW): `set_avatar_from_file` (`lockscreen.rs`) opened `~/.face` / the AccountsService icon via `gio::File::for_path().read_future()`, which follows symlinks unconditionally, while the wallpaper load (`load_background_texture`) was already hardened with `O_NOFOLLOW` (2026-04-24). `users::get_avatar_path_with` rejects symlinks via `symlink_metadata()`, but a TOCTOU window remained between that stat and the GIO open. The root cause was not the avatar code per se but that the `O_NOFOLLOW` open idiom was inlined in one place and absent from the other — the project's own lock-path hardening contract was applied inconsistently. Impact is below the primary threat model (exploiting it needs write access to `~/.face`, i.e. an already-compromised session; the screen stays locked either way), hence LOW. +- **Tradeoffs**: Extracted a shared `read_file_nofollow` rather than duplicating the open idiom, so the two file reads on the lock path cannot diverge again. The avatar read moved onto a blocking thread via `gio::spawn_blocking` (was a GIO async read) to keep the open off the GTK main loop, then decodes from an in-memory `MemoryInputStream`, preserving the scaled `AVATAR_SIZE` decode. No integration test for `set_avatar_from_file` itself — GTK + async, not unit-testable without a harness; the security primitive (`O_NOFOLLOW` → `ELOOP`) is covered by a unit test that goes red if the flag is removed (verified by temporarily dropping it). SEC-02 (fallback wallpaper `is_file()`) and SEC-03 (PAM `strdup` not zeroed) were reviewed and left as-is: the former is still protected by `O_NOFOLLOW` at open time and is a root-only system path, the latter is an inherent PAM-API limitation outside the lock threat model, already documented in `CLAUDE.md`. +- **How**: (1) New `read_file_nofollow(&Path) -> io::Result>` in `lockscreen.rs`, used by both `load_background_texture` and `set_avatar_from_file`. (2) The avatar load reads via `gio::spawn_blocking(read_file_nofollow)` then decodes from a `MemoryInputStream`; a symlinked avatar now fails the open with `ELOOP`, logs a warning, and falls back to the default avatar. (3) Two unit tests for the loader (regular file → bytes; symlink → `ELOOP`). (4) Clippy cleanup bundled in: `c""` C-string literal in `auth.rs`, `let`-chains in `config.rs`/`users.rs`, removed redundant `use gtk4_session_lock;` in `main.rs`, blur guard collapsed to `blur_radius.filter(|s| *s > 0.0)` (unifies with moongreet/moonset, which already used that form). Verified: `cargo test` (48 passed), `cargo clippy --release` (0 warnings), `cargo build --release`. + ## 2026-06-17 – Add cargo-audit CI gate, remove orphaned `-git` PKGBUILD - **Who**: ClaudeCode, Dom diff --git a/src/auth.rs b/src/auth.rs index 4e4a6f6..e4d472a 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -115,7 +115,7 @@ unsafe extern "C" fn pam_conv_callback( } PAM_PROMPT_ECHO_ON => { // Visible prompt — provide empty string, never the password - let empty = libc::strdup(b"\0".as_ptr() as *const libc::c_char); + let empty = libc::strdup(c"".as_ptr()); if empty.is_null() { for j in 0..i { let prev = resp_array.offset(j); diff --git a/src/config.rs b/src/config.rs index 16ffa54..9ed9947 100644 --- a/src/config.rs +++ b/src/config.rs @@ -72,8 +72,11 @@ pub fn resolve_background_path(config: &Config) -> Option { pub fn resolve_background_path_with(config: &Config, moonarch_wallpaper: &Path) -> Option { 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() { return Some(path); } + if let Ok(meta) = path.symlink_metadata() + && meta.is_file() + && !meta.file_type().is_symlink() + { + return Some(path); } } if moonarch_wallpaper.is_file() { return Some(moonarch_wallpaper.to_path_buf()); } diff --git a/src/lockscreen.rs b/src/lockscreen.rs index 33a3184..7fcfca3 100644 --- a/src/lockscreen.rs +++ b/src/lockscreen.rs @@ -442,6 +442,23 @@ pub fn start_fingerprint( }); } +/// Read a file with O_NOFOLLOW so a symlink swapped in after a prior +/// stat-based check (TOCTOU) fails the open with ELOOP instead of being +/// followed. Shared by the wallpaper and avatar loads — the two file reads +/// on the lock path. +fn read_file_nofollow(path: &Path) -> std::io::Result> { + use std::io::Read; + use std::os::unix::fs::OpenOptionsExt; + + let mut file = std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW) + .open(path)?; + let mut bytes = Vec::new(); + file.read_to_end(&mut bytes)?; + Ok(bytes) +} + /// Load the wallpaper as a texture once, for sharing across all windows. /// Returns None if no wallpaper path is provided or the file cannot be loaded. /// Blur is applied at render time via GPU (GskBlurNode), not here. @@ -450,25 +467,13 @@ pub fn start_fingerprint( /// symlink check in `resolve_background_path_with` and this read. If the path /// was swapped for a symlink after the check, `open` fails with ELOOP. pub fn load_background_texture(bg_path: &Path) -> Option { - use std::io::Read; - use std::os::unix::fs::OpenOptionsExt; - - let mut file = match std::fs::OpenOptions::new() - .read(true) - .custom_flags(libc::O_NOFOLLOW) - .open(bg_path) - { - Ok(f) => f, + let bytes = match read_file_nofollow(bg_path) { + Ok(b) => b, Err(e) => { - log::warn!("Failed to open wallpaper {}: {e}", bg_path.display()); + log::warn!("Failed to read wallpaper {}: {e}", bg_path.display()); return None; } }; - let mut bytes = Vec::new(); - if let Err(e) = file.read_to_end(&mut bytes) { - log::warn!("Failed to read wallpaper {}: {e}", bg_path.display()); - return None; - } let glib_bytes = glib::Bytes::from_owned(bytes); match gdk::Texture::from_bytes(&glib_bytes) { Ok(texture) => Some(texture), @@ -495,21 +500,19 @@ fn create_background_picture( background.set_hexpand(true); background.set_vexpand(true); - if let Some(sigma) = blur_radius { - if sigma > 0.0 { - let texture = texture.clone(); - let cache = blur_cache.clone(); - background.connect_realize(move |picture| { - if let Some(ref cached) = *cache.borrow() { - picture.set_paintable(Some(cached)); - return; - } - if let Some(blurred) = render_blurred_texture(picture, &texture, sigma) { - picture.set_paintable(Some(&blurred)); - *cache.borrow_mut() = Some(blurred); - } - }); - } + if let Some(sigma) = blur_radius.filter(|s| *s > 0.0) { + let texture = texture.clone(); + let cache = blur_cache.clone(); + background.connect_realize(move |picture| { + if let Some(ref cached) = *cache.borrow() { + picture.set_paintable(Some(cached)); + return; + } + if let Some(blurred) = render_blurred_texture(picture, &texture, sigma) { + picture.set_paintable(Some(&blurred)); + *cache.borrow_mut() = Some(blurred); + } + }); } background @@ -585,18 +588,25 @@ fn set_avatar_from_file( image.set_icon_name(Some("avatar-default-symbolic")); let display_path = path.to_path_buf(); - let file = gio::File::for_path(path); + let read_path = path.to_path_buf(); let image_clone = image.clone(); let cache_clone = cache.clone(); glib::spawn_future_local(async move { - let stream = match file.read_future(glib::Priority::default()).await { - Ok(s) => s, - Err(e) => { + // Read with O_NOFOLLOW on a blocking thread to close the TOCTOU window + // between the symlink check in users::get_avatar_path_with and this open. + let bytes = match gio::spawn_blocking(move || read_file_nofollow(&read_path)).await { + Ok(Ok(b)) => b, + Ok(Err(e)) => { log::warn!("Failed to open avatar {}: {e}", display_path.display()); return; } + Err(_) => { + log::warn!("Avatar read task failed for {}", display_path.display()); + return; + } }; + let stream = gio::MemoryInputStream::from_bytes(&glib::Bytes::from_owned(bytes)); match Pixbuf::from_stream_at_scale_future(&stream, AVATAR_SIZE, AVATAR_SIZE, true).await { Ok(pixbuf) => { let texture = gdk::Texture::for_pixbuf(&pixbuf); @@ -831,4 +841,25 @@ mod tests { fn avatar_size_matches_css() { assert_eq!(AVATAR_SIZE, 128); } + + #[test] + fn read_file_nofollow_reads_regular_file() { + let dir = tempfile::tempdir().unwrap(); + let file = dir.path().join("avatar.png"); + std::fs::write(&file, b"avatar-bytes").unwrap(); + assert_eq!(read_file_nofollow(&file).unwrap(), b"avatar-bytes"); + } + + #[test] + fn read_file_nofollow_rejects_symlink() { + use std::os::unix::fs::symlink; + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("secret"); + std::fs::write(&target, b"secret").unwrap(); + let link = dir.path().join("avatar.png"); + symlink(&target, &link).unwrap(); + // O_NOFOLLOW makes the open fail with ELOOP instead of following the link. + let err = read_file_nofollow(&link).unwrap_err(); + assert_eq!(err.raw_os_error(), Some(libc::ELOOP)); + } } diff --git a/src/main.rs b/src/main.rs index 31addb2..0bc65ab 100644 --- a/src/main.rs +++ b/src/main.rs @@ -12,7 +12,6 @@ mod users; use gdk4 as gdk; use gtk4::prelude::*; use gtk4::{self as gtk, gio}; -use gtk4_session_lock; use std::cell::{Cell, RefCell}; use std::rc::Rc; diff --git a/src/users.rs b/src/users.rs index b3f5eff..003c6be 100644 --- a/src/users.rs +++ b/src/users.rs @@ -38,14 +38,20 @@ pub fn get_avatar_path(home: &Path, username: &str) -> Option { pub fn get_avatar_path_with(home: &Path, username: &str, accountsservice_dir: &Path) -> Option { // ~/.face takes priority — single stat via symlink_metadata to avoid TOCTOU let face = home.join(".face"); - if let Ok(meta) = face.symlink_metadata() { - if meta.is_file() && !meta.file_type().is_symlink() { return Some(face); } + if let Ok(meta) = face.symlink_metadata() + && meta.is_file() + && !meta.file_type().is_symlink() + { + return Some(face); } // AccountsService icon if accountsservice_dir.exists() { let icon = accountsservice_dir.join(username); - if let Ok(meta) = icon.symlink_metadata() { - if meta.is_file() && !meta.file_type().is_symlink() { return Some(icon); } + if let Ok(meta) = icon.symlink_metadata() + && meta.is_file() + && !meta.file_type().is_symlink() + { + return Some(icon); } } None