From 074b1ee829e6725bcef063edc503e49b9315ab06 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Tue, 11 Aug 2026 17:47:56 -0700 Subject: [PATCH] Stop an uncrackable VTS borrowing another VTS's title key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolve_vts_key` called the `Option`-returning `css::crack_key`, which collapses "no scrambled sector was seen" with "scrambled sectors were seen and no key came out", and then fell back to the disc-wide key. A multi-VTS CSS DVD whose second title set resists the Stevenson scan was descrambled under the FIRST set's key: corrupt PES behind an intact header, written out with `complete = true` at exit 0. `CrackOutcome` exists precisely to keep those two apart, and its doc says callers must surface the second as a hard error. Every sibling path in the crate already does — `Disc::decrypt_keys_for_title` and the mux path both map `ScrambledUncracked` to `CssKeyMissing`. This was the one path that did not. The ordering comment 15 lines above describes this exact outcome as the bug it exists to prevent; ordering makes the crack far more likely to succeed, but it cannot make a failed crack safe. Also, three things nothing could catch: - The demux output filename took the stream's language raw while the base beside it was sanitised. A language code is three raw STN bytes through `from_utf8_lossy`, and `00 00 00` is the ordinary "undefined" encoding on real discs — a NUL in a path fails `File::create` with InvalidInput, taking the whole export down before one track file opened. `sanitize` now maps control characters too; it did not. - `css::crack_key_scan`'s short-read handling was dead code under test: every source in the module returned the full request, so reverting `advance` to the requested count, or dropping the `.max(1)`, left the suite green. The `.max(1)` is load-bearing — without it a source returning `Ok(0)` never moves the cursor and never increments the budget, so the scan spins forever. That mutation now HANGS the test rather than failing it, which is the honest demonstration. - `MAX_SUBDIRS`'s const-assert carried `#[cfg(not(test))]` inside a `#[cfg(test)] mod tests`, so it compiled in no configuration and could never fire — the dead gate the test above it was written to replace. Moved to module scope, and verified it now rejects a wrong constant at compile time. --- src/css/mod.rs | 69 ++++++++++++++++++++++++- src/dirimage/layout.rs | 14 +++-- src/disc/extract.rs | 115 ++++++++++++++++++++++++++++++++++++++--- src/mux/demux_sink.rs | 59 ++++++++++++++++++++- 4 files changed, 242 insertions(+), 15 deletions(-) diff --git a/src/css/mod.rs b/src/css/mod.rs index c584e94..20adb9f 100644 --- a/src/css/mod.rs +++ b/src/css/mod.rs @@ -738,6 +738,11 @@ mod tests { /// instead of the uniform `flag_byte` fill. Lets the scan actually /// reach `CrackOutcome::Cracked` from a synthetic ISO. crackable: Option<(u32, Vec)>, + /// Sectors actually filled per batch, however many were asked for — + /// the SHORT READ a `recovery: true` source is allowed to return over + /// a damaged region. `Some(0)` is the degenerate case that must not + /// spin the scan. + short_read: Option, } impl MockSource { @@ -748,6 +753,7 @@ mod tests { fail_all: false, lock_all: false, crackable: None, + short_read: None, } } } @@ -798,14 +804,19 @@ mod tests { if self.fail_all { return Err(Error::DecryptFailed); } - let n = count as usize * 2048; + // A short read fills, and reports, fewer sectors than asked. + let filled = match self.short_read { + Some(k) => (k as u16).min(count), + None => count, + }; + let n = filled as usize * 2048; let end = n.min(buf.len()); for b in buf[..end].iter_mut() { *b = 0; } // Fill each sector in the batch with the uniform flag byte, EXCEPT a // designated crackable LBA which gets the full synthetic sector. - for s in 0..count as u32 { + for s in 0..filled as u32 { let sect_lba = lba + s; let base = s as usize * 2048; if base + 2048 > end { @@ -830,6 +841,60 @@ mod tests { } } + // ── Short reads: the branch nothing exercised ───────────────────────── + // + // `crack_key_scan` passes `recovery = true`, which is precisely the mode + // where a `SectorSource` may return Ok with fewer bytes than asked. Every + // source in this module returned the full request, so `usable`, `advance` + // and the `.max(1)` anti-spin guard were dead code under test: reverting + // `advance` to `n`, or dropping the `.max(1)`, left the whole suite green. + + /// A short batch is RE-READ from where it stopped, not skipped. Skipping + /// would quietly shrink the crack's coverage on exactly the damaged media + /// where a key is hardest to find. + #[test] + fn a_short_read_resumes_from_where_it_stopped() { + let mut src = MockSource::new(0x00); + src.short_read = Some(1); + let ext = [crate::disc::Extent { + start_lba: 100, + sector_count: 4, + }]; + let _ = crack_key_scan(&mut src, &ext, 4, None, false); + let reads = src.reads.borrow().clone(); + assert_eq!( + reads, + vec![100, 101, 102, 103], + "a source that filled one sector per batch must be asked for the \ + next one, not advanced a whole batch past it" + ); + } + + /// A source that reads NOTHING must terminate. Without the `.max(1)` the + /// cursor never moves and `tried` never increments — the budget cannot end + /// the loop, so the scan spins forever inside a library whose whole job is + /// surviving hostile input. + #[test] + fn a_source_that_returns_zero_sectors_terminates() { + let mut src = MockSource::new(0x00); + src.short_read = Some(0); + let ext = [crate::disc::Extent { + start_lba: 0, + sector_count: 8, + }]; + let outcome = crack_key_scan(&mut src, &ext, 4, None, false); + assert!( + matches!(outcome, CrackOutcome::Unencrypted), + "nothing was read, so nothing scrambled was seen" + ); + assert!( + src.reads.borrow().len() <= 8, + "the cursor must advance even on an empty read; got {} reads over \ + an 8-sector extent", + src.reads.borrow().len() + ); + } + /// crack_key caps total scanned sectors at 50_000 even when extents are /// far larger, and counts EVERY scanned sector (clear ones included) /// toward the budget. With one 200_000-sector extent of clear sectors, it diff --git a/src/dirimage/layout.rs b/src/dirimage/layout.rs index ff4974b..8bfb699 100644 --- a/src/dirimage/layout.rs +++ b/src/dirimage/layout.rs @@ -67,6 +67,15 @@ const MAX_SUBDIRS: usize = (u16::MAX - 1) as usize; #[cfg(test)] const MAX_SUBDIRS: usize = 4; +/// One entry per child plus the parent's own must still fit the 16-bit field. +/// +/// At MODULE scope, and it has to be. This assertion previously sat inside +/// `mod tests`, which is `#[cfg(test)]`, while carrying `#[cfg(not(test))]` +/// itself — so it was compiled in NO configuration and could never fire, which +/// made it exactly the dead gate the test above it was written to replace. +#[cfg(not(test))] +const _: () = assert!(MAX_SUBDIRS + 1 == u16::MAX as usize); + /// Largest image this planner will synthesize, in sectors (128 GiB). /// /// A DVD title set records where its VOBS begins as an offset in its own IFO, @@ -998,11 +1007,6 @@ mod tests { "expected DirImageFanout, got {err:?}" ); - // And the real production value is what ships: one per child plus the - // parent's own entry must still fit in the 16-bit field. - #[cfg(not(test))] - const _: () = assert!(MAX_SUBDIRS + 1 == u16::MAX as usize); - let _ = std::fs::remove_dir_all(&dir); } } diff --git a/src/disc/extract.rs b/src/disc/extract.rs index 73478f2..59fa2a4 100644 --- a/src/disc/extract.rs +++ b/src/disc/extract.rs @@ -254,7 +254,7 @@ impl Disc { let key = match vts_keys.get(&vts) { Some(k) => k.clone(), None => { - let k = self.resolve_vts_key(&vts, &planned, &mut dec, &base_keys); + let k = self.resolve_vts_key(&vts, &planned, &mut dec, &base_keys)?; vts_keys.insert(vts.clone(), k.clone()); k } @@ -291,13 +291,24 @@ impl Disc { /// VOB extents (keyless Stevenson attack). The disc-wide key is reused when /// it already covers this VTS (single-VTS discs, or this VTS's span). The /// reader is borrowed from the decrypting decorator (its inner source). + /// + /// FALLIBLE, because the three outcomes of a crack are not two. + /// [`crate::css::CrackOutcome`] exists precisely to separate "no scrambled + /// sector was seen, so there is nothing to decrypt" from "scrambled sectors + /// were seen and no key came out", and its doc says callers MUST surface + /// the second as a hard error. This function used the `Option`-returning + /// `crack_key`, which collapses both into `None`, and then fell back to the + /// disc-wide key — descrambling this VTS with ANOTHER VTS's title key. The + /// hazard is described in the ordering note below, which was written about + /// the same fallback; ordering makes the crack far more likely to succeed, + /// but it cannot make a failed crack safe. fn resolve_vts_key( &self, vts: &str, planned: &[PlannedFile], dec: &mut DecryptingSectorSource, base_keys: &DecryptKeys, - ) -> DecryptKeys { + ) -> Result { // Gather the title VOB extents for this VTS (VTS_xx_1.VOB .. _9.VOB; // VTS_xx_0.VOB is the menu and is clear, so excluded from the crack). // Gather this VTS's title VOBs BY NAME, ascending. @@ -336,7 +347,7 @@ impl Disc { } } if extents.is_empty() { - return base_keys.clone(); + return Ok(base_keys.clone()); } // PLAYBACK ORDER — do NOT sort. This is the 1.5.1 garbage bug, and it // grew back here in a new code path: the comment this replaced said it @@ -358,11 +369,20 @@ impl Disc { // in file order, which together is playback order for a DVD title set. // Crack against the raw (still-scrambled) inner reader, NOT the // decrypting view — `crack_key` runs the descrambler itself. - match crate::css::crack_key(dec.inner_mut(), &extents, 64) { - Some(state) => DecryptKeys::Css { + match crate::css::crack_key_outcome(dec.inner_mut(), &extents, 64, None) { + crate::css::CrackOutcome::Cracked(state) => Ok(DecryptKeys::Css { title_key: state.title_key, - }, - None => base_keys.clone(), + }), + // No scrambled sector anywhere in this VTS: the content is clear, + // and any key descrambles it as a no-op. The disc-wide key is the + // right answer, and this is the ONLY case that ever was. + crate::css::CrackOutcome::Unencrypted => Ok(base_keys.clone()), + // Scrambled sectors WERE seen and no key came out. Reusing the + // disc-wide key here writes corrupt PES behind an intact header and + // reports a complete extract at exit 0. Skippable per title, which + // is why this is the per-title code and not the disc-level one — a + // sibling VTS may still crack. + crate::css::CrackOutcome::ScrambledUncracked => Err(Error::CssKeyMissing), } } } @@ -1918,6 +1938,87 @@ mod tests { assert!(res.complete); } + /// A VTS that IS scrambled but whose key could not be recovered must + /// FAIL, not borrow another VTS's key. + /// + /// `crack_key` returns `Option`, which collapses "no scrambled sector was + /// seen" with "scrambled sectors were seen and no key came out"; the + /// fallback then descrambled this VTS under the disc-wide key and + /// `extract_tree` reported `complete = true` at exit 0, because no read had + /// failed. `CrackOutcome` exists to keep those two apart, and every sibling + /// path in the crate already honours it. + /// + /// The fixture: VTS_01 is crackable, VTS_02 is scrambled with NO periodic + /// crib, so its crack genuinely fails. + #[test] + fn a_scrambled_vts_that_cannot_be_cracked_fails_instead_of_borrowing_a_key() { + let key_1 = [0x10u8, 0x20, 0x30, 0x40, 0x50]; + let (_plain_1, scrambled_1) = { + let seed = [0x11u8, 0x22, 0x33, 0x44, 0x01]; + let mut plain = vec![0u8; 2048]; + plain[0x00..0x04].copy_from_slice(&[0x00, 0x00, 0x01, 0xBA]); + plain[0x14] = 0x10; + let pat: Vec = (0..8) + .map(|k| (0xA0u8.wrapping_add(k as u8) ^ 0x01) ^ 0x5A) + .collect(); + for (i, b) in plain.iter_mut().enumerate().skip(0x59) { + *b = pat[i % 8]; + } + plain[0x54..0x59].copy_from_slice(&seed); + let mut scrambled = plain.clone(); + lfsr::scramble_sector(&key_1, &mut scrambled); + (plain, scrambled) + }; + // Scrambled — the pack header and the 0x14 flag are set, so the scan + // SEES ciphertext — but the cleartext region carries no periodic run, + // so the Stevenson crib never forms and no key is recoverable. + let uncrackable = { + let mut sect = vec![0u8; 2048]; + sect[0x00..0x04].copy_from_slice(&[0x00, 0x00, 0x01, 0xBA]); + sect[0x14] = 0x10; + for (i, b) in sect.iter_mut().enumerate().skip(0x59) { + // Non-repeating, so no run of any period survives to 0x80. + *b = (i as u8).wrapping_mul(37).wrapping_add(11); + } + sect + }; + + let root = DirSpec { + name: String::new(), + icb_lba: 10, + dir_data_lba: 11, + files: Vec::new(), + subdirs: vec![DirSpec { + name: "VIDEO_TS".to_string(), + icb_lba: 20, + dir_data_lba: 21, + files: vec![ + file("VTS_01_1.VOB", 30, 5000, scrambled_1, false), + file("VTS_02_1.VOB", 32, 6000, uncrackable, false), + ], + subdirs: vec![], + }], + }; + let mut disc = build_disc(root); + let out = TmpDir::new("css_uncrackable_vts"); + let mut d = clear_disc(); + d.content_format = crate::disc::ContentFormat::MpegPs; + d.css = Some(crate::css::CssState { + title_key: [0xFFu8; 5], + crack_span: None, + }); + let err = d + .extract_tree(&mut disc, out.path(), &ExtractOptions::default()) + .expect_err( + "a VTS whose key could not be recovered must be a hard error, \ + not a silent extract under another VTS's key", + ); + assert!( + matches!(err, Error::CssKeyMissing), + "expected CssKeyMissing, got {err:?}" + ); + } + /// `Borrowed` is a thin forwarding wrapper the decrypting decorator uses /// to avoid taking ownership of the caller's reader -- every /// `SectorSource` method must forward to the wrapped `&mut dyn diff --git a/src/mux/demux_sink.rs b/src/mux/demux_sink.rs index 9f39f32..2699a53 100644 --- a/src/mux/demux_sink.rs +++ b/src/mux/demux_sink.rs @@ -637,10 +637,18 @@ fn xml_escape(s: &str) -> String { } /// Replace path-hostile characters in a filename component. +/// +/// Control characters included, NUL above all. Every string this touches is +/// disc bytes, and a language code is three raw STN bytes run through +/// `from_utf8_lossy` with no validation — `00 00 00` is the ordinary +/// "undefined" encoding on real Blu-rays. A NUL in a path aborts `File::create` +/// with `InvalidInput`, which took the whole demux export down before a single +/// track file was opened. fn sanitize(s: &str) -> String { s.chars() .map(|c| match c { '/' | '\\' | ':' | '*' | '?' | '"' | '<' | '>' | '|' => '_', + c if c.is_control() => '_', _ => c, }) .collect() @@ -787,8 +795,11 @@ impl DemuxSink { Naming::Pid => format!("{} {:04x}", sanitize(&opts.base), pid), Naming::Friendly => { let mut parts = vec![sanitize(&opts.base), format!("t{idx:02}")]; + // The language is disc bytes too, and got none of the + // treatment `opts.base` two lines up already had. + let lang = sanitize(lang); if !lang.is_empty() { - parts.push(lang.to_string()); + parts.push(lang); } parts.push(codec_label(codec).to_string()); parts.join(" ") @@ -987,6 +998,52 @@ mod tests { Resolution, SampleRate, VideoStream, }; + // ── The language component is disc bytes ────────────────────────────── + // + // `opts.base` was sanitised and the language beside it was not, though a + // language code is three raw STN bytes run through `from_utf8_lossy` with + // no validation. `00 00 00` is the ordinary "undefined" encoding on real + // Blu-rays, and a NUL in a path fails `File::create` with `InvalidInput` — + // which aborted the whole export before a single track file was opened. + + /// The stem stays one usable filename component whatever the disc says. + #[test] + fn a_hostile_language_code_cannot_break_the_output_filename() { + let opts = DemuxOptions { + base: "Movie".to_string(), + ..Default::default() + }; + assert!(matches!(opts.naming, Naming::Friendly), "default naming"); + + for lang in ["\u{0}\u{0}\u{0}", "a/b", "..", "a\nb", "e:s"] { + let stem = DemuxSink::stem_for(&opts, 1, 0x1100, lang, Codec::Ac3); + assert!( + !stem.chars().any(|c| c.is_control()), + "control character survived into the filename for {lang:?}: {stem:?}" + ); + assert!( + !stem.contains('/') && !stem.contains('\\') && !stem.contains(':'), + "a path separator survived for {lang:?}: {stem:?}" + ); + assert!( + std::path::Path::new(&stem).components().count() == 1, + "the stem must stay ONE component for {lang:?}: {stem:?}" + ); + } + } + + /// A legitimate language is still carried through untouched — the + /// sanitiser must not be so lossy that it stops naming the track. + #[test] + fn an_ordinary_language_code_survives_sanitising() { + let opts = DemuxOptions { + base: "Movie".to_string(), + ..Default::default() + }; + let stem = DemuxSink::stem_for(&opts, 1, 0x1100, "eng", Codec::Ac3); + assert!(stem.contains("eng"), "got {stem:?}"); + } + fn video_stream(codec: Codec) -> DiscStream { DiscStream::Video(VideoStream { pid: 0x1011,