From c6086b0ab35e3505d2ddff2a03e8fe6a5dcbd3c6 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 23 Aug 2026 15:40:05 -0700 Subject: [PATCH] Stop the file panel refusing ordinary paths, and hanging on a FIFO MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit H6 (HIGH) — `resolve_host_path` canonicalised the host path and then re-ran the *lexical* policy over the answer, hidden-component rule included. Because canonicalisation resolves through symlinks, that rule started judging where a path happens to live rather than where the user pointed: uploading out of a dependency under pnpm (`node_modules/pkg` → `node_modules/.pnpm/…`) was refused, and so was every download into, or upload out of, a visible directory that leads to `~/.local/share`, `~/.cache`, `~/.var/app`, `~/.nvm` or `~/.cargo`. None of it was refused before the H4 fix landed. The two questions are now separate functions. `validate_host_path` judges the string the user chose, unchanged. `validate_resolved_host_path` judges the canonical form for the things only it can answer — the system roots (a Mac's `/etc` *is* `/private/etc`), the login-item directories, and a new `HOST_CREDENTIAL_DIRS` list. That last one is what keeps H4's escape closed: `Downloads/pub` → `~/.ssh` with a leaf of `authorized_keys` is refused because of where it lands, not because of how the directory is spelled. macOS handling is untouched — `/private/tmp` stays out of `HOST_SYSTEM_ROOTS` and `/var/folders` stays in the exceptions. H8 (HIGH) — the upload reservation claimed its destination with `sh -c 'set -C; : > "$0"'`, and the comment claiming that is `O_EXCL` was wrong for a destination that is not a regular file. Against a FIFO the shell opens it and blocks in `open(2)` forever; `exec_oneshot_raw` has no timeout, so `upload_file_to_container` never returned and the Files pane sat on "Uploading…" for the session with the rest of the batch abandoned. Verified in a fresh ubuntu:24.04: the old form times out and the blocked `sh` stays in `ps`; the new form answers in 35 ms. The reservation is now a `link(2)` — it claims a name atomically, never opens anything, and `EEXIST` is immediate whatever is in the way. A staging file at an unguessable name in the same directory is linked into place and unlinked, under a `trap … EXIT`. `exec_oneshot_as_within` adds a wall-clock ceiling as the second line of defence, opt-in per call site so migration's `apt-get` is unaffected. The upload contract is unchanged: default-refuse, `overwrite: Option`, and `FILE_EXISTS: already exists`. Also fixed, all in the same surface: * A dangling symlink destination was a permanent dead end — `set -C` refused, the confirming `test -e` followed the link and said no, and raw shell text came back with no Replace on offer. `link(2)` does not follow the new-path link, and the script confirms with `[ -L ]`, so it reports as a collision. * An upload through a symlink renamed the file: the leaf came off the *resolved* path, so `~/Downloads/latest.log` landed as `2026-08-23.log` and the collision prompt named a file the user never chose. The name now comes from the path the user gave; the resolved path is still what gets opened. * `download_container_backup` leaked its partial file when the descriptor check fired. It now tracks `created` the way `stream_container_file_to_host` already did. * The failed-upload cleanup was `rm -f` on a path that, the reservation having succeeded, held whatever was written in the interim — a host file under `/workspace/…`. It now removes only an empty regular file, and the comment says what it is doing. * `resolve_container_dir` parsed a combined stdout+stderr buffer as a path. It uses the split-stream helper, like the listing next to it. * `verify_opened_path` failed open on a readlink error (`if let Ok(actual)`). A check that cannot see is not a check that saw nothing wrong; the macOS compile-time no-op is now spelled out too. * A trailing slash on a write path resolved to the directory itself. Nine new tests, all mutation-checked against the pre-fix behaviour. Two more cases added to the ignored live-Docker test: a FIFO and a dangling symlink, both timed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc --- app/src-tauri/src/commands/file_commands.rs | 742 +++++++++++++++++--- app/src-tauri/src/docker/exec.rs | 38 + 2 files changed, 685 insertions(+), 95 deletions(-) diff --git a/app/src-tauri/src/commands/file_commands.rs b/app/src-tauri/src/commands/file_commands.rs index 6b8f476..a301e40 100644 --- a/app/src-tauri/src/commands/file_commands.rs +++ b/app/src-tauri/src/commands/file_commands.rs @@ -12,8 +12,8 @@ use tauri::State; use crate::docker::client::get_docker; use crate::docker::exec::{ - build_single_file_tar, container_user_ids, exec_oneshot_as, exec_oneshot_streams_as, - now_epoch_secs, OUTPUT_LIMIT_MARKER, + build_single_file_tar, container_user_ids, exec_oneshot_as, exec_oneshot_as_within, + exec_oneshot_streams_as, now_epoch_secs, OUTPUT_LIMIT_MARKER, }; use crate::AppState; @@ -399,7 +399,12 @@ fn validate_container_write_path(what: &str, path: &str) -> Result<(), String> { async fn resolve_container_dir(container_id: &str, what: &str, dir: &str) -> Result<(), String> { validate_container_write_path(what, dir)?; - let (output, code) = exec_oneshot_as( + // Split streams, not the combined buffer: `realpath`'s answer is a *path* + // and its diagnostics are not, so parsing the two together is the same + // hazard the listing above took apart for `find`. A warning on stderr — + // and there is one whenever a component is unreadable — used to be spliced + // into the string this then compared against the write roots. + let (stdout, diagnostics, code) = exec_oneshot_streams_as( container_id, "claude", vec![ @@ -412,12 +417,17 @@ async fn resolve_container_dir(container_id: &str, what: &str, dir: &str) -> Res ) .await?; - let resolved = output.trim(); + let resolved = stdout.trim(); if code != 0 || resolved.is_empty() { log::warn!( - "Could not resolve {} in the container (exit {}); using the literal path", + "Could not resolve {} in the container (exit {}{}); using the literal path", dir, - code + code, + if diagnostics.trim().is_empty() { + String::new() + } else { + format!(": {}", diagnostics.trim()) + } ); return Ok(()); } @@ -495,6 +505,37 @@ const HOST_AUTORUN_DIRS: &[&[&str]] = &[ &["start menu", "programs", "startup"], ]; +/// Directory *sequences* that hold credentials or authority, judged on the +/// path a symlink chain really leads to. +/// +/// This is the part of the hidden-component rule that has to survive +/// resolution. The rule itself cannot: see [`validate_resolved_host_path`] for +/// why "the real location passes through a dot directory" describes ordinary +/// software far more often than it describes an attack — `node_modules/.pnpm`, +/// `~/.local/share`, `~/.cache`, `~/.var/app`, `~/.nvm`, `~/.cargo`. What +/// *is* worth refusing after resolution is the small set of directories whose +/// contents are keys, tokens and startup entries, and those can be named. +/// +/// Matched as a contiguous run of components anywhere in the path, so +/// `~/.ssh/keys/id_rsa` is as refused as `~/.ssh/id_rsa`. Same defence-in-depth +/// footing as [`HOST_AUTORUN_DIRS`], and the same honest caveat: it is a list +/// of the places that are known, not of the ones that exist. The boundary is +/// the file dialog; this is what stops a *planted symlink* aiming an otherwise +/// ordinary-looking path at the one directory the attack wants. +const HOST_CREDENTIAL_DIRS: &[&[&str]] = &[ + &[".ssh"], + &[".gnupg"], + &[".aws"], + &[".azure"], + &[".kube"], + &[".docker"], + &[".claude"], + &[".config", "gcloud"], + &[".config", "autostart"], + &[".config", "systemd", "user"], + &[".local", "share", "keyrings"], +]; + /// Length of a `C:` drive prefix at the head of `path`, or 0. fn drive_prefix_len(path: &str) -> usize { let b = path.as_bytes(); @@ -614,6 +655,23 @@ fn is_autorun_dir(names: &[String]) -> bool { }) } +/// The [`HOST_CREDENTIAL_DIRS`] entry `names` passes through, spelled the way +/// the list spells it so the refusal can name it. +/// +/// Anywhere in the path rather than at the end: what matters is that the path +/// goes *through* `~/.ssh`, not how much further it goes. +fn credential_dir_in(names: &[String]) -> Option { + HOST_CREDENTIAL_DIRS.iter().find_map(|seq| { + (0..names.len().saturating_sub(seq.len() - 1)).find_map(|start| { + names[start..start + seq.len()] + .iter() + .zip(seq.iter()) + .all(|(have, want)| have.eq_ignore_ascii_case(want)) + .then(|| seq.join("/")) + }) + }) +} + /// Structural and policy checks on a host path *as written*, returning it as a /// [`PathBuf`]. /// @@ -635,12 +693,16 @@ fn is_autorun_dir(names: &[String]) -> bool { /// dragging a project's own `.env` into the container is an ordinary thing /// to do and its parent is not hidden. /// -/// **This is a lexical check on a string, and lexical is not enough on its own.** -/// A path whose components are all visible can still lead somewhere hidden, so -/// nothing calls this directly any more: [`resolve_host_path`] resolves the -/// symlinks first and then applies this to the answer. Keeping the two apart is -/// what lets the policy stay pure and testable while the thing it judges is the -/// path that will really be opened. +/// **This is a lexical check on the string the user chose, and it judges only +/// that.** A path whose components are all visible can still *lead* somewhere +/// that deserves a second opinion, which is why [`resolve_host_path`] follows +/// this with [`validate_resolved_host_path`] over the canonical form. The two +/// ask different questions and must not be confused: this one is "did the user +/// point at a hidden place", that one is "where do these bytes actually land". +/// Re-running *this* function over a canonical path is the H6 regression — it +/// refuses `node_modules/pkg` under pnpm, and every visible directory that +/// symlinks into `~/.local/share`, `~/.cache`, `~/.var/app`, `~/.nvm` or +/// `~/.cargo`, none of which is anybody's attack. /// /// **And the policy itself is a denylist, which is losing by construction.** /// `~/Library/LaunchAgents`, `%AppData%\…\Startup`, `~/bin` and `/opt` are only @@ -666,6 +728,21 @@ fn validate_host_path(path: &str, use_for: HostPathUse) -> Result Result