fix: harden avatar load against symlink TOCTOU (v0.6.19)
SEC-01 (security audit, LOW): the avatar load followed symlinks via gio::File while the wallpaper load was already O_NOFOLLOW-hardened — the project's lock-path hardening was applied inconsistently. Share one read_file_nofollow loader for both file reads so they cannot diverge again; a symlinked ~/.face now fails open with ELOOP and falls back to the default avatar. Adds loader unit tests (regular file, symlink->ELOOP). Bundles clippy cleanup: c"" literal in auth.rs, let-chains, drop redundant gtk4_session_lock import, blur guard via .filter() (unifies with moongreet/moonset).
This commit is contained in:
@@ -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<Vec<u8>>` 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
|
||||
|
||||
Reference in New Issue
Block a user