agent: lumotia — Phase B.5 close symlink-target bypass in write_text_file_cmd path scope
Phase B.5 audit of commits a2b47db/a48653c/b3da58c (Trust-1 — write path allowlist),9653e25(Trust-5 — transcribe_file extension allowlist + size cap), anded449cc(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;b3da58cis 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", "<payload>"). 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 "<payload>" 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) <noreply@anthropic.com>
This commit is contained in:
@@ -72,16 +72,30 @@ fn allowed_export_bases(app: &tauri::AppHandle) -> Vec<PathBuf> {
|
|||||||
bases
|
bases
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Resolve the requested write target. The target's PARENT directory
|
/// Resolve the requested write target. The canonicalised path must sit
|
||||||
/// must already exist (we canonicalise the parent, then re-join the
|
/// inside one of `bases`.
|
||||||
/// filename) and the canonicalised path must sit inside one of `bases`.
|
|
||||||
///
|
///
|
||||||
/// We canonicalise the parent rather than the full path because the
|
/// Two-mode canonicalisation:
|
||||||
/// file itself typically does not exist yet — canonicalise() returns
|
///
|
||||||
/// NotFound in that case on Linux/macOS. The parent must exist for
|
/// - **If the target already exists**, canonicalise the WHOLE path. This
|
||||||
/// `tokio::fs::write` to succeed anyway, so checking it is no extra
|
/// follows any symlink at the target itself, so a symlink that sits
|
||||||
/// cost.
|
/// 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<PathBuf, String> {
|
pub(crate) fn resolve_export_path(path: &Path, bases: &[PathBuf]) -> Result<PathBuf, String> {
|
||||||
|
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(|| {
|
let parent = path.parent().ok_or_else(|| {
|
||||||
format!(
|
format!(
|
||||||
"Refusing to write {}: path has no parent directory.",
|
"Refusing to write {}: path has no parent directory.",
|
||||||
@@ -101,7 +115,15 @@ pub(crate) fn resolve_export_path(path: &Path, bases: &[PathBuf]) -> Result<Path
|
|||||||
path.display()
|
path.display()
|
||||||
)
|
)
|
||||||
})?;
|
})?;
|
||||||
let canon_path = canon_parent.join(file_name);
|
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) {
|
if !is_inside_any_base(&canon_path, bases) {
|
||||||
return Err(format!(
|
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(&a.join("nested/x.md"), &bases));
|
||||||
assert!(!is_inside_any_base(Path::new("/tmp/other/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));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user