From d8fa4ff64ea9a43cef955481ac45c692371d0767 Mon Sep 17 00:00:00 2001 From: Jake Date: Thu, 14 May 2026 19:33:20 +0100 Subject: [PATCH] =?UTF-8?q?agent:=20lumotia=20=E2=80=94=20Phase=20B.5=20cl?= =?UTF-8?q?ose=20symlink-target=20bypass=20in=20write=5Ftext=5Ffile=5Fcmd?= =?UTF-8?q?=20path=20scope?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase B.5 audit of commits a2b47db/a48653c/b3da58c (Trust-1 — write path allowlist), 9653e25 (Trust-5 — transcribe_file extension allowlist + size cap), and ed449cc (Trust-2 — resolve_recording_path output_folder validation). The three Trust-1 commits were a corrective sequence that swapped the staged file in a parallel-agent race; b3da58c is the authoritative landing. Existing coverage is strong: * commands/fs.rs (Trust-1): 6 tests — outside-allowlist, traversal, accepts inside, accepts nested, rejects missing parent, prefix check. * commands/transcription.rs (Trust-5): 7 tests — accepts wav, accepts MP3 case-insensitive, accepts each allowed extension, rejects unsupported, rejects no-extension, rejects oversize, accepts exactly-at-cap, rejects traversal-with-disallowed-ext. * commands/audio.rs (Trust-2): 7 tests including validate_output_folder_rejects_symlink_pointing_out — the symlink bypass for output folders is already covered. One real residual found in commands/fs.rs: Asymmetric symlink handling between Trust-1 (fs.rs) and Trust-2 (audio.rs). Trust-2 canonicalises the FULL requested path (it's a directory that must already exist), so a symlink at the directory itself that points outside the base is resolved before the containment check and gets rejected. Trust-1 canonicalises only the PARENT of the requested path, because the target file typically does not exist yet (canonicalize() returns NotFound on missing paths). Concrete bypass: 1. A symlink at, e.g., ~/Downloads/notes.md -> ~/.bashrc — innocently created by the user, or planted via another vulnerability. 2. Compromised webview invokes write_text_file_cmd("/home/user/Downloads/notes.md", ""). 3. Path-scope check: parent canonicalises to /home/user/Downloads, file_name joins, canonical path string sits inside the Downloads allowlist. PASS. 4. tokio::fs::write -> File::create -> open(2) follows the symlink and writes "" to ~/.bashrc, exfiltrating shell startup. Fix: two-mode canonicalisation in resolve_export_path. If the target exists, canonicalise the full path — this follows any symlink at the target itself, and the subsequent containment check sees the resolved location. Only on NotFound do we fall back to parent-canonicalise + join-filename (the original save-dialog path). This mirrors the audio crate's canonicalisation discipline. Regression tests: * rejects_symlink_target_outside_allowlist — creates a symlink inside a base pointing OUT to a real outside file; resolve_export_path must return Err with "outside the allowed export directories". * accepts_symlink_target_inside_allowlist — symmetric, an in-base alias symlink must still resolve and pass, so legitimate uses of symlinks are not regressed. Both gated #[cfg(unix)] because std::os::unix::fs::symlink is unix-only. The Trust-1 surface ships symmetrically on Windows; the symlink class attack does not generalise the same way on NTFS (junctions vs symlinks have different ACL semantics), and a windows-specific test would be duplicate-effort outside the audit scope. Verification: * cargo test -p lumotia --lib commands::fs → 8/8 pass including the two new symlink tests. * cargo fmt --check → clean. * cargo clippy -p lumotia --all-targets -- -D warnings → clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- src-tauri/src/commands/fs.rs | 137 ++++++++++++++++++++++++++++------- 1 file changed, 110 insertions(+), 27 deletions(-) diff --git a/src-tauri/src/commands/fs.rs b/src-tauri/src/commands/fs.rs index 7651e30..0bbb424 100644 --- a/src-tauri/src/commands/fs.rs +++ b/src-tauri/src/commands/fs.rs @@ -72,36 +72,58 @@ fn allowed_export_bases(app: &tauri::AppHandle) -> Vec { bases } -/// Resolve the requested write target. The target's PARENT directory -/// must already exist (we canonicalise the parent, then re-join the -/// filename) and the canonicalised path must sit inside one of `bases`. +/// Resolve the requested write target. The canonicalised path must sit +/// inside one of `bases`. /// -/// We canonicalise the parent rather than the full path because the -/// file itself typically does not exist yet — canonicalise() returns -/// NotFound in that case on Linux/macOS. The parent must exist for -/// `tokio::fs::write` to succeed anyway, so checking it is no extra -/// cost. +/// Two-mode canonicalisation: +/// +/// - **If the target already exists**, canonicalise the WHOLE path. This +/// follows any symlink at the target itself, so a symlink that sits +/// inside an allowlisted base but points outside it gets rejected +/// here. `tokio::fs::write` follows symlinks on open(2), so without +/// this branch a symlink at `~/Downloads/notes.md -> ~/.bashrc` would +/// pass the containment check (the path string is inside Downloads) +/// and the subsequent write would silently land in `~/.bashrc`. +/// Trust-1 audit residual closed in Phase B.5 (2026-05-14); parallels +/// `validate_output_folder`'s full-path-canonicalize in +/// `commands/audio.rs` (Trust-2). +/// +/// - **If the target does not yet exist** (the typical save-dialog +/// case), canonicalise the parent and re-join the filename. The +/// parent must exist for `tokio::fs::write` to succeed anyway, so +/// checking it is no extra cost. pub(crate) fn resolve_export_path(path: &Path, bases: &[PathBuf]) -> Result { - let parent = path.parent().ok_or_else(|| { - format!( - "Refusing to write {}: path has no parent directory.", - path.display() - ) - })?; - let file_name = path.file_name().ok_or_else(|| { - format!( - "Refusing to write {}: path has no filename.", - path.display() - ) - })?; + let canon_path = match std::fs::canonicalize(path) { + Ok(canon) => canon, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + let parent = path.parent().ok_or_else(|| { + format!( + "Refusing to write {}: path has no parent directory.", + path.display() + ) + })?; + let file_name = path.file_name().ok_or_else(|| { + format!( + "Refusing to write {}: path has no filename.", + path.display() + ) + })?; - let canon_parent = std::fs::canonicalize(parent).map_err(|e| { - format!( - "Refusing to write {}: cannot resolve parent dir ({e}).", - path.display() - ) - })?; - let canon_path = canon_parent.join(file_name); + let canon_parent = std::fs::canonicalize(parent).map_err(|e| { + format!( + "Refusing to write {}: cannot resolve parent dir ({e}).", + path.display() + ) + })?; + canon_parent.join(file_name) + } + Err(e) => { + return Err(format!( + "Refusing to write {}: cannot resolve path ({e}).", + path.display() + )); + } + }; if !is_inside_any_base(&canon_path, bases) { return Err(format!( @@ -209,4 +231,65 @@ mod tests { assert!(is_inside_any_base(&a.join("nested/x.md"), &bases)); assert!(!is_inside_any_base(Path::new("/tmp/other/x.md"), &bases)); } + + /// Phase B.5 audit regression (2026-05-14). The Trust-1 fix + /// canonicalised only the parent of the target path because the file + /// typically does not exist yet. But if a symlink ALREADY exists at + /// the target path pointing outside the allowlisted base, + /// `tokio::fs::write` follows it on `File::create -> open(2)` and + /// silently writes through to the outside target. The path-string + /// containment check passed because the SYMLINK lives inside the + /// base, even though the symlink's target does not. + /// + /// The fix canonicalises the full path when it exists (so the + /// symlink resolves before the containment check) and only falls + /// back to parent-canonicalize on NotFound. This regression test + /// pins that contract. + #[cfg(unix)] + #[test] + fn rejects_symlink_target_outside_allowlist() { + let (_keep_base, base) = tempdir_canon("base"); + let (_keep_out, outside) = tempdir_canon("outside"); + // Real, writable file outside the base — the symlink target. + let outside_target = outside.join("victim.txt"); + std::fs::write(&outside_target, b"original").expect("seed outside target"); + + // Inside-base symlink pointing to the outside file. + let symlink_path = base.join("notes.md"); + std::os::unix::fs::symlink(&outside_target, &symlink_path) + .expect("create symlink inside base pointing outside"); + + let bases = vec![base.clone()]; + let result = resolve_export_path(&symlink_path, &bases); + assert!( + result.is_err(), + "symlink target outside allowlist must be rejected, got {result:?}" + ); + let err = result.err().unwrap(); + assert!( + err.contains("outside the allowed export directories"), + "unexpected error shape: {err}" + ); + } + + /// Symmetric to the rejection above: a symlink whose target stays + /// inside the same base must still be accepted, so legitimate users + /// of in-base symlinks (e.g. a tilde-expansion alias inside the base) + /// are not regressed. + #[cfg(unix)] + #[test] + fn accepts_symlink_target_inside_allowlist() { + let (_keep, base) = tempdir_canon("base"); + let real_path = base.join("real.md"); + std::fs::write(&real_path, b"original").expect("seed real target"); + + let symlink_path = base.join("alias.md"); + std::os::unix::fs::symlink(&real_path, &symlink_path) + .expect("create in-base alias symlink"); + + let bases = vec![base.clone()]; + let resolved = resolve_export_path(&symlink_path, &bases) + .expect("in-base symlink must resolve and pass"); + assert!(resolved.starts_with(&base)); + } }