From da6b039b297c67e26fbc3ab93d228f6de00b8c63 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Thu, 20 Aug 2026 20:01:52 +0200 Subject: [PATCH] fix(downloads): confine download paths to the download root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both halves of the path a download writes to arrived from the frontend unchecked. `start_download` and the queue pump built their target as `PathBuf::from(target_dir).join(file_path)`, and `mark_download_completed` stored a frontend-supplied `file_path` on the row verbatim — the same column that is later read back into `std::fs::remove_file` when a download is deleted. A correct sanitiser already existed and `download_item_and_start` used it, but `download_item` is a command in its own right, so calling it directly routed the guard around. The guard moves inside. `confine_to_root` folds `..` away lexically and requires the result to sit inside the storage root, modelled on `media_server::resolve_path` — the check comes after the join because `Path::join` drops the base when the joined half is absolute, so an absolute `file_path` is obeyed rather than folded. `confine_queued_path` sanitises a queued path per component (so the already-safe name `download_item_and_start` passes in is not sanitised into a second, different one) and confines it. Applied in `download_item`, at both join sites, and to what `mark_download_completed` writes. Every path the app builds for itself is returned unchanged, including the absolute ones `download_series`/`download_season` produce from `${targetDir}/videos`, so no existing row or file on disk is orphaned. The pump fails an offending row rather than skipping it, because the pump re-queries and would otherwise not terminate. Not a live vulnerability: reaching these commands with hostile input needs script execution in a webview whose CSP is `script-src 'self'`. This is hardening and consistency. TRACES: DR-211 | UT-205 --- src-tauri/src/commands/download/mod.rs | 221 ++++++++++++++++++++++++- 1 file changed, 216 insertions(+), 5 deletions(-) diff --git a/src-tauri/src/commands/download/mod.rs b/src-tauri/src/commands/download/mod.rs index 0cd61748..cde58859 100644 --- a/src-tauri/src/commands/download/mod.rs +++ b/src-tauri/src/commands/download/mod.rs @@ -3,7 +3,7 @@ #[cfg(test)] use crate::utils::lock::MutexSafe; use log::{debug, error, info, warn}; -use std::path::PathBuf; +use std::path::{Component, Path, PathBuf}; use std::sync::{Arc, Mutex}; use tauri::{Manager, State}; @@ -132,6 +132,73 @@ fn sanitize_filename(name: &str) -> String { .collect() } +/// The directory every download has to stay inside: the storage root +/// `storage_get_path` hands the frontend, which is the database's parent. +/// +/// TRACES: DR-211 | UT-205 +fn download_root(db: &DatabaseWrapper) -> Result { + let database = db.0.lock().map_err(|e| e.to_string())?; + database + .path() + .parent() + .map(|p| p.to_path_buf()) + .ok_or_else(|| "Database path has no parent directory".to_string()) +} + +/// Fold `..` out of `candidate` and require what is left to sit inside `root`. +/// +/// Lexical rather than `canonicalize`, the same way `media_server::resolve_path` +/// does it: the file usually does not exist yet, so canonicalising would fail on +/// the ordinary case. The check has to come *after* the caller's join, because +/// `Path::join` drops the base when the joined half is absolute — such a path is +/// not folded, it is obeyed, and only the `starts_with` below catches it. +/// +/// TRACES: DR-211 | UT-205 +fn confine_to_root(root: &Path, candidate: &Path) -> Result { + let mut resolved = PathBuf::new(); + for component in candidate.components() { + match component { + Component::ParentDir => { + resolved.pop(); + } + Component::CurDir => {} + other => resolved.push(other), + } + } + + if resolved.starts_with(root) { + Ok(resolved) + } else { + Err(format!( + "Refusing a download path outside the download directory: {}", + candidate.display() + )) + } +} + +/// Sanitize a queued download's path and confine it to the download directory. +/// +/// Every path the app builds for itself comes back unchanged — files on disk and +/// `downloads` rows point at these exact spellings — and [`sanitize_filename`] +/// is idempotent, so the already-safe name `download_item_and_start` passes in +/// is not sanitized into a second, different one. +/// +/// TRACES: DR-211 | UT-205 +fn confine_queued_path(root: &Path, file_path: &str) -> Result { + let mut sanitized = PathBuf::new(); + for component in Path::new(file_path).components() { + match component { + Component::Normal(part) => sanitized.push(sanitize_filename(&part.to_string_lossy())), + // Kept as they are, so `confine_to_root` is the single thing + // deciding whether what they add up to is still inside the root. + other => sanitized.push(other), + } + } + + confine_to_root(root, &root.join(&sanitized))?; + Ok(sanitized.to_string_lossy().to_string()) +} + /// Request payload for download_item_and_start (bundled to stay within specta's /// 10-argument command limit). #[derive(Debug, specta::Type, serde::Deserialize)] @@ -253,6 +320,18 @@ pub async fn download_item( album_name, expected_size, } = request; + + // `start_download` joins this onto the target directory, and `Path::join` + // drops the base when the second half is absolute, so the row itself has to + // be confined — not only the place it is used. `download_item_and_start` + // sanitizes the name it builds, but `download_item` is a command in its own + // right, so that guard was simply routed around by calling this directly. + // TRACES: DR-211 | UT-205 + let file_path = { + let root = download_root(&db)?; + confine_queued_path(&root, &file_path)? + }; + let db_service = { let database = db.0.lock().map_err(|e| e.to_string())?; Arc::new(database.service()) @@ -1286,6 +1365,20 @@ pub async fn mark_download_completed( bytes_downloaded: i64, file_path: String, ) -> Result<(), String> { + // Deleting a download reads this straight back into `std::fs::remove_file`, + // so a row must never come to name a file outside the download directory. + // The worker reports the absolute path it wrote, and joining an absolute + // path onto the root yields it unchanged, so that case is stored verbatim; + // the frontend's fallback to the row's own (relative) path resolves under + // the root, where the worker put it. + // TRACES: DR-211 | UT-205 + let file_path = { + let root = download_root(&db)?; + confine_to_root(&root, &root.join(&file_path))? + .to_string_lossy() + .to_string() + }; + let db_service = { let database = db.0.lock().map_err(|e| e.to_string())?; Arc::new(database.service()) @@ -1416,6 +1509,14 @@ pub async fn start_download( item_id, file_path, file_size ); + // Both halves of this join reached us from the frontend, so resolve them + // against the download directory before a single byte is written. + // TRACES: DR-211 | UT-205 + let target_path = { + let root = download_root(&db)?; + confine_to_root(&root, &PathBuf::from(&target_dir).join(&file_path))? + }; + // Make a HEAD request to get the file size from Content-Length header debug!("Making HEAD request to get file size..."); let head_response = reqwest::Client::new().head(&stream_url).send().await; @@ -1489,9 +1590,6 @@ pub async fn start_download( Err(e) => error!(" Event emit failed: {:?}", e), } - // Build target path - let target_path = PathBuf::from(&target_dir).join(&file_path); - // Get a clone of the active downloads Arc for unregistering later let active_downloads = { let manager = download_manager.0.lock().map_err(|e| e.to_string())?; @@ -1762,6 +1860,36 @@ pub(crate) async fn pump_download_queue( None => return, // Nothing pending to start }; + // Confine the row's path before it takes a slot. A row whose target + // escapes the download directory can never start, so it is failed here + // rather than picked again on the next pass — this loop re-queries, so + // merely skipping it would not terminate. + // TRACES: DR-211 | UT-205 + let confined = { + let db_state = app.state::(); + download_root(&db_state).and_then(|root| { + confine_to_root(&root, &PathBuf::from(&target_dir).join(&file_path)) + }) + }; + let target_path = match confined { + Ok(path) => path, + Err(e) => { + error!("[pump] Refusing download {}: {}", download_id, e); + let fail_query = Query::with_params( + "UPDATE downloads SET status = 'failed', error_message = ? WHERE id = ?", + vec![QueryParam::String(e), QueryParam::Int64(download_id)], + ); + if let Err(db_err) = db_service.execute(fail_query).await { + error!( + "[pump] Failed to mark download {} failed: {}", + download_id, db_err + ); + return; + } + continue; + } + }; + // Register the slot. If registration fails (race: another pump filled // the last slot), stop — we'll be re-pumped when a slot frees. { @@ -1809,7 +1937,6 @@ pub(crate) async fn pump_download_queue( }, ); - let target_path = PathBuf::from(&target_dir).join(&file_path); spawn_download_worker( app.clone(), download_id, @@ -2421,6 +2548,90 @@ mod tests { assert_eq!(sanitize_filename("track/1.flac"), "track_1.flac"); } + /// The download directory as it looks on a device, for the path tests. + const TEST_ROOT: &str = "/data/data/com.dtourolle.jellytau/files"; + + /// A queued `file_path` cannot walk out of the download directory. + /// + /// `download_item` is a command in its own right, so sanitizing in + /// `download_item_and_start` was routed around by invoking it directly, and + /// `start_download` then joined the raw string onto the target directory. + /// + /// TRACES: DR-211 | UT-205 + #[test] + fn test_queued_download_paths_cannot_escape_the_download_directory() { + let root = Path::new(TEST_ROOT); + + assert!(confine_queued_path(root, "downloads/../../../../etc/cron.d/pwn").is_err()); + assert!(confine_queued_path(root, "../.bashrc").is_err()); + assert!(confine_queued_path(root, "/etc/cron.d/pwn").is_err()); + + // Why the absolute case needs its own guard rather than folding: the + // join the download path performs discards the base entirely. + assert_eq!( + PathBuf::from(root).join("/etc/cron.d/pwn"), + PathBuf::from("/etc/cron.d/pwn") + ); + } + + /// The paths the app builds for itself have to survive unchanged: files are + /// already on disk and `downloads` rows point at these exact spellings. + /// + /// TRACES: DR-211 | UT-205 + #[test] + fn test_queued_download_paths_are_otherwise_unchanged() { + let root = Path::new(TEST_ROOT); + + for path in [ + "downloads/9f8e7d6c", // MediaCard's queue-for-reconnect + "videos/movies/Arrival.mp4", // VideoDownloadButton + "albums/abc123/01 - Opening.mp3", // queue_album_tracks + // download_series/download_season build an absolute path, because + // their base_path is `${targetDir}/videos`. + "/data/data/com.dtourolle.jellytau/files/videos/Show/S01E02_Pilot.mp4", + ] { + assert_eq!(confine_queued_path(root, path).unwrap(), path); + } + + // `download_item_and_start` sanitizes the name before calling + // `download_item`; sanitizing it again must not yield a second, different + // name, which would orphan the row and the file it names. + let already = format!("downloads/{}.mp3", sanitize_filename("AC/DC: Live?")); + assert_eq!(confine_queued_path(root, &already).unwrap(), already); + } + + /// A completed row's `file_path` is read straight back into + /// `std::fs::remove_file` when the download is deleted, so `mark_download_completed` + /// must not be able to register a file outside the download directory. + /// + /// TRACES: DR-211 | UT-205 + #[test] + fn test_a_completed_download_cannot_register_a_file_outside_the_root() { + let root = Path::new(TEST_ROOT); + + // What the worker actually reports — the absolute path it wrote. Stored + // exactly as it arrives. + let written = "/data/data/com.dtourolle.jellytau/files/downloads/9f8e7d6c"; + assert_eq!( + confine_to_root(root, Path::new(written)).unwrap(), + PathBuf::from(written) + ); + + // The row's own path, if the frontend falls back to it: relative, and it + // resolves to where the worker wrote the file. + assert_eq!( + confine_to_root(root, &root.join("downloads/9f8e7d6c")).unwrap(), + PathBuf::from(written) + ); + + assert!(confine_to_root(root, Path::new("/home/u/.ssh/id_ed25519")).is_err()); + assert!(confine_to_root( + root, + Path::new("/data/data/com.dtourolle.jellytau/files/../../../../etc/passwd") + ) + .is_err()); + } + /// Helper to set up test database with required foreign key data fn setup_test_db() -> Database { let db = Database::open_in_memory().unwrap();