diff --git a/crates/rustmotion/src/encode/audio.rs b/crates/rustmotion/src/encode/audio.rs index 85f09796..6f284a44 100644 --- a/crates/rustmotion/src/encode/audio.rs +++ b/crates/rustmotion/src/encode/audio.rs @@ -64,12 +64,8 @@ pub(crate) fn decode_audio_file(path: &str) -> Result<(Vec, u32, u32)> { })?; let track_id = track.id; - let sample_rate = track.codec_params.sample_rate.unwrap_or(44100); - let channels = track - .codec_params - .channels - .map(|c| c.count() as u32) - .unwrap_or(2); + let header_sample_rate = track.codec_params.sample_rate; + let header_channels = track.codec_params.channels.map(|c| c.count() as u32); let mut decoder = symphonia::default::get_codecs() .make(&track.codec_params, &DecoderOptions::default()) @@ -79,6 +75,7 @@ pub(crate) fn decode_audio_file(path: &str) -> Result<(Vec, u32, u32)> { })?; let mut all_samples: Vec = Vec::new(); + let mut decoded_spec: Option<(u32, u32)> = None; loop { let packet = match format.next_packet() { @@ -107,11 +104,40 @@ pub(crate) fn decode_audio_file(path: &str) -> Result<(Vec, u32, u32)> { sample_buf.copy_interleaved_ref(decoded); all_samples.extend_from_slice(sample_buf.samples()); + + if decoded_spec.is_none() { + decoded_spec = Some((spec.rate, spec.channels.count() as u32)); + } } + let (sample_rate, channels) = + resolve_decoded_spec(decoded_spec, header_sample_rate, header_channels); + Ok((all_samples, sample_rate, channels)) } +/// `decode_audio_file`'s `(sample_rate, channels)` return value: the actual +/// spec of the first packet that decoded, when there was one — that's what +/// `all_samples` was interleaved from — falling back to the container +/// header's own declaration only when nothing ever decoded at all. Some +/// demuxers leave the header fields absent or, for formats whose per-frame +/// syntax can encode a channel count the container-level probe cannot see +/// (e.g. an ADTS/AAC stream with an implicit `channel_configuration`), +/// disagreeing with what the codec itself produces — trusting the header +/// unconditionally there silently mis-sizes every downstream stereo/mono +/// interpretation of `all_samples`. `44100`/`2` is the last-resort default, +/// unchanged from before this function tracked a decoded spec at all. +fn resolve_decoded_spec( + decoded_spec: Option<(u32, u32)>, + header_sample_rate: Option, + header_channels: Option, +) -> (u32, u32) { + decoded_spec.unwrap_or(( + header_sample_rate.unwrap_or(44100), + header_channels.unwrap_or(2), + )) +} + /// Duration, sample rate and channel count of a local audio file — the /// `rustmotion info` answer to "how long is this audio track?". #[derive(Debug, Clone, Copy, PartialEq)] @@ -479,6 +505,49 @@ fn resample_linear(samples: &[f32], src_rate: u32, dst_rate: u32) -> Vec { mod tests { use super::*; + // ── decode_audio_file must trust the decoded spec, not the header ──────── + // + // Reproducing the exact divergence through a real file needs a decoder + // whose *decoded* output can genuinely disagree with what the container + // probed ahead of time. Tracing one concrete case where this happens in + // the wild — `symphonia-codec-aac`'s ADTS reader leaving `codec_params.channels` + // `None` when `channel_configuration == 0` — into `AacDecoder::try_new` + // (symphonia-codec-aac 0.5.5, `aac/mod.rs`) shows that without an + // `extra_data`/channel-layout fallback it returns a hard + // `unsupported_error` instead of ever reaching a decoded packet: this + // crate's enabled `symphonia` features (`mp3, wav, ogg, flac, aac`, no + // `isomp4`) never populate that fallback for a bare ADTS stream. So this + // exact repro surfaces as a loud decode failure, not silent corruption — + // a real fixture can't exercise the discard/trust distinction at all. + // `resolve_decoded_spec` is exactly that distinction pulled out as a + // pure decision, tested directly with the divergence a real file could + // produce for a different codec/container pairing. + #[test] + fn resolve_decoded_spec_prefers_the_decoded_packets_own_spec_over_the_header() { + let decoded = Some((48_000, 1)); + let header_rate = Some(44_100); + let header_channels = Some(2); + assert_eq!( + resolve_decoded_spec(decoded, header_rate, header_channels), + (48_000, 1), + "a packet actually decoded — all_samples came from its spec, so the \ + return value must match it, not the header's own guess" + ); + } + + #[test] + fn resolve_decoded_spec_falls_back_to_the_header_when_nothing_ever_decoded() { + assert_eq!( + resolve_decoded_spec(None, Some(22_050), Some(1)), + (22_050, 1) + ); + } + + #[test] + fn resolve_decoded_spec_falls_back_to_the_hardcoded_default_as_a_last_resort() { + assert_eq!(resolve_decoded_spec(None, None, None), (44_100, 2)); + } + /// Write a minimal, hand-rolled canonical PCM WAV file (16-bit, mono) — /// no ffmpeg and no extra crate needed, `symphonia`'s built-in WAV demuxer /// decodes this directly. 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 {