diff --git a/crates/rustmotion/src/encode/video/ffmpeg.rs b/crates/rustmotion/src/encode/video/ffmpeg.rs index 63d338ae..2f79343d 100644 --- a/crates/rustmotion/src/encode/video/ffmpeg.rs +++ b/crates/rustmotion/src/encode/video/ffmpeg.rs @@ -290,6 +290,17 @@ fn ffmpeg_args( args } +/// Name of the scratch directory a single audio-bearing render call writes +/// its materialised PCM into. `pid` repeats across the machine's uptime and +/// `seq` is a small monotonic counter starting at zero, so together they are +/// a key an outside process could realistically pre-compute and occupy +/// ahead of time; folding in a nanosecond timestamp neither of those two +/// alone carries closes that gap without needing a random-number +/// dependency this crate doesn't already have. +fn audio_tmp_dir_name(pid: u32, seq: u32, nanos: u128) -> String { + format!("rustmotion_audio_{pid}_{seq}_{nanos:x}") +} + /// Scratch path ffmpeg actually writes to; promoted (renamed) onto the /// caller's real `output_path` only after a clean exit with no `pipe_error`. /// Kept as a sibling of `output_path` (same directory, same filesystem, so @@ -461,22 +472,32 @@ fn encode_with_ffmpeg_hw_impl( // encodes can run concurrently *within* one process (parallel test // threads today; `--frames` segments rendered concurrently by a future // distributed worker tomorrow — the exact shape this feature exists to - // enable). Two calls sharing a PID-only path would each `create_dir_all` + // enable). Two calls sharing a PID-only path would each try to create // the same directory, then whichever finishes first would // `remove_dir_all` it out from under the other mid-write, surfacing as // a bare `NotFound` on `std::fs::write` below. A monotonic counter on // top of PID makes every call's directory distinct regardless of - // timing. + // timing; a nanosecond timestamp on top of *that* keeps the full key + // from being small enough for something outside this process to + // pre-compute and occupy ahead of time — pid space and a + // monotonic-from-zero counter both are. `create_dir` below (not + // `_all`) is what actually refuses to proceed if something is already + // sitting at the computed path, symlink included; the timestamp only + // raises the cost of ever landing on that path in the first place. static AUDIO_TMP_DIR_SEQ: AtomicU32 = AtomicU32::new(0); let audio_tmp_dir = if !merged_audio.is_empty() { let seq = AUDIO_TMP_DIR_SEQ.fetch_add(1, Ordering::Relaxed); - Some(std::env::temp_dir().join(format!("rustmotion_audio_{}_{seq}", std::process::id()))) + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or_default(); + Some(std::env::temp_dir().join(audio_tmp_dir_name(std::process::id(), seq, nanos))) } else { None }; let pcm_data = if !merged_audio.is_empty() { if let Some(ref tmp_dir) = audio_tmp_dir { - std::fs::create_dir_all(tmp_dir)?; + std::fs::create_dir(tmp_dir)?; } super::super::audio::mix_audio_tracks_segment( &merged_audio, @@ -837,10 +858,63 @@ pub fn concat_mp4_segments(inputs: &[std::path::PathBuf], output_path: &str) -> #[cfg(test)] mod tests { use super::{ - ffmpeg_args, ffmpeg_partial_output_path, parse_encoder_names, select_hardware_encoder, - HardwareSelection, + audio_tmp_dir_name, ffmpeg_args, ffmpeg_partial_output_path, parse_encoder_names, + select_hardware_encoder, HardwareSelection, }; + // ── audio scratch directory naming: not fully predictable from outside ── + + #[test] + fn audio_tmp_dir_name_differs_across_calls_that_share_pid_and_seq() { + // A pid+seq pair is small enough to pre-seed exhaustively from + // outside the process; folding in a nanosecond timestamp neither of + // those two alone carries means a name computed ahead of time from + // pid+seq no longer identifies the exact directory this process + // will actually create. + let a = audio_tmp_dir_name(1234, 0, 111); + let b = audio_tmp_dir_name(1234, 0, 222); + assert_ne!( + a, b, + "same pid+seq, different nanos, must differ: {a} vs {b}" + ); + } + + #[test] + fn audio_tmp_dir_name_is_stable_for_identical_inputs() { + assert_eq!(audio_tmp_dir_name(1, 2, 3), audio_tmp_dir_name(1, 2, 3)); + } + + /// Characterizes the exact property this fix depends on: swapping + /// `create_dir_all` for `create_dir` at the audio scratch directory's + /// creation site turns "adopt whatever is already there" into "refuse + /// outright" the moment something — attacker-planted symlink included — + /// already occupies that path. + #[test] + fn create_dir_refuses_an_already_occupied_path_that_create_dir_all_would_have_adopted() { + let path = std::env::temp_dir().join(format!( + "rustmotion_audit_ws_c_preexisting_dir_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let _ = std::fs::remove_dir_all(&path); + std::fs::create_dir(&path).expect("set up a pre-existing directory at the target path"); + + assert!( + std::fs::create_dir_all(&path).is_ok(), + "create_dir_all silently succeeding on a pre-existing directory is exactly the \ + behavior that let a hostile pre-planted directory (or symlink) be adopted" + ); + assert!( + std::fs::create_dir(&path).is_err(), + "create_dir must refuse the same pre-existing path instead of adopting it" + ); + + let _ = std::fs::remove_dir_all(&path); + } + // ── partial-output-path naming (pure) ──────────────────────────────────── #[test] diff --git a/crates/rustmotion/src/encode/video_audio.rs b/crates/rustmotion/src/encode/video_audio.rs index caac9e1e..01a1c033 100644 --- a/crates/rustmotion/src/encode/video_audio.rs +++ b/crates/rustmotion/src/encode/video_audio.rs @@ -255,6 +255,32 @@ pub fn build_atempo_filter(rate: f64) -> Option { // ─── Cache-keyed temp WAV path ──────────────────────────────────────────────── +/// Base directory the extracted-audio WAV cache lives under. +/// +/// `std::env::temp_dir()` is shared and, on most Unix systems, world-writable +/// — combined with `wav_cache_path`'s hash being deterministic (which it has +/// to be, for the cache to ever hit twice), a different local user could +/// compute the exact cache path ahead of time and plant content there before +/// this process ever ran. `dirs::cache_dir()` is per-user (`~/Library/Caches` +/// on macOS, `~/.cache` on Linux), so the same determinism that makes +/// caching useful stops doubling as a cross-user attack surface. Falls back +/// to `temp_dir()` only on a platform with no notion of a user cache +/// directory at all — still better than failing outright, and consistent +/// with every other fallback in this codebase preferring a degraded mode +/// over an unusable one. +fn wav_cache_base_dir() -> PathBuf { + let base = dirs::cache_dir() + .unwrap_or_else(std::env::temp_dir) + .join("rustmotion"); + let _ = std::fs::create_dir_all(&base); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let _ = std::fs::set_permissions(&base, std::fs::Permissions::from_mode(0o700)); + } + base +} + fn wav_cache_path(src: &str, trim_start: f64, trim_end: Option, rate: f64) -> PathBuf { let mut hasher = DefaultHasher::new(); src.hash(&mut hasher); @@ -280,7 +306,21 @@ fn wav_cache_path(src: &str, trim_start: f64, trim_end: Option, rate: f64) } let hash = hasher.finish(); - std::env::temp_dir().join(format!("rustmotion_vidaud_{:016x}.wav", hash)) + wav_cache_base_dir().join(format!("rustmotion_vidaud_{:016x}.wav", hash)) +} + +/// Whether `path` is safe to reuse as a cache hit: a genuine regular file, +/// not a symlink. `wav_cache_path` now resolves under a per-user directory +/// (see `wav_cache_base_dir`), which already rules out a *different* user +/// planting one; this additionally refuses to follow a symlink planted by +/// anything running as the *same* user (a compromised sibling process, or a +/// leftover from before that directory existed) into wherever it points. +/// `symlink_metadata` — unlike `Path::exists`/`std::fs::metadata` — reports +/// on the directory entry itself rather than whatever it resolves to. +fn cached_wav_is_trustworthy(path: &std::path::Path) -> bool { + std::fs::symlink_metadata(path) + .map(|m| m.file_type().is_file()) + .unwrap_or(false) } /// Scratch path ffmpeg writes to before a successful extraction is promoted @@ -319,8 +359,9 @@ fn extract_audio_to_wav( ) -> Option { let wav_path = wav_cache_path(src, trim_start, trim_end, rate); - // Reuse cached extraction. - if wav_path.exists() { + // Reuse cached extraction — but only a genuine regular file placed here + // by a previous extraction; see `cached_wav_is_trustworthy`. + if cached_wav_is_trustworthy(&wav_path) { return Some(wav_path); } @@ -488,6 +529,87 @@ mod tests { use super::*; use crate::loader::load_scenario_from_source; + // ── Cache directory: per-user, not the shared world-writable temp dir ──── + + #[test] + fn wav_cache_path_does_not_sit_directly_inside_the_bare_shared_temp_dir() { + let cached = wav_cache_path("foo.mp4", 0.0, None, 1.0); + let shared_temp = std::env::temp_dir(); + assert_ne!( + cached.parent(), + Some(shared_temp.as_path()), + "the cached WAV must live under a dedicated subdirectory, not directly inside the \ + shared temp dir a same-machine, different-user attacker can also write to: got {}", + cached.display() + ); + } + + // ── Cache entries must be verified, not merely `exists()` ──────────────── + + #[cfg(unix)] + #[test] + fn a_symlink_at_the_cache_path_is_never_trusted_as_a_cache_hit() { + let target = std::env::temp_dir().join(format!( + "rm_vidaud_symlink_target_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::write(&target, b"not a wav, planted by someone else").unwrap(); + + let link = std::env::temp_dir().join(format!( + "rm_vidaud_symlink_link_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let _ = std::fs::remove_file(&link); + std::os::unix::fs::symlink(&target, &link).expect("create symlink fixture"); + + assert!( + !cached_wav_is_trustworthy(&link), + "a symlink sitting at the cache path must never be treated as a valid cache hit, \ + regardless of what it points to" + ); + + let mut real_file = link.with_file_name(format!( + "rm_vidaud_real_file_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + real_file.set_extension("wav"); + std::fs::write(&real_file, b"RIFF....").unwrap(); + assert!( + cached_wav_is_trustworthy(&real_file), + "a genuine regular file must still be trusted" + ); + + let _ = std::fs::remove_file(&target); + let _ = std::fs::remove_file(&link); + let _ = std::fs::remove_file(&real_file); + } + + #[test] + fn a_missing_path_is_not_trustworthy() { + let path = std::env::temp_dir().join(format!( + "rm_vidaud_never_created_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let _ = std::fs::remove_file(&path); + assert!(!cached_wav_is_trustworthy(&path)); + } + // ── Helpers ────────────────────────────────────────────────────────────── fn load(json: &str) -> ResolvedScenario {