From 5180526ebf0ad65ba8c6c97b0555cefbc43d1b97 Mon Sep 17 00:00:00 2001 From: Albert Hui Date: Tue, 4 Aug 2026 07:09:54 -0700 Subject: [PATCH 1/2] test(fuzz): RED - LBA arithmetic overflows on header-chosen values Both nightly fuzz targets crash the same way: src/lib.rs:477:18 attempt to add with overflow (parse_state) src/lib.rs:596:26 attempt to add with overflow (read_dir) Both sites add a partition start to a logical block number, and both operands are u32 fields read out of the image: fsd_lba: partition_start + fsd // File Set Descriptor location let fe_lba = partition_start + icb_lbn; // directory entry's File Entry Nothing bounds either, so a descriptor pair summing past u32::MAX panics. The release profile would wrap instead and address the wrong block -- a silently wrong answer rather than a loud one, which for a forensic reader is worse. The reproducers are the bytes libFuzzer actually found, taken from the failing run's uploaded artifact rather than hand-authored, so they exercise the real path rather than the one I would have guessed. libFuzzer could not minimise them further: the size is what carries the offsets past the boundary. They are kept as ordinary tests, not only as fuzz corpus, because a defect found by fuzzing should not need fuzzing to be caught again -- the fuzz job runs nightly, the suite runs on every push. The read_dir harness steers addressing from the input's leading bytes rather than parsing them from the image, so its test decodes the reproducer exactly as the target does; feeding the whole blob to the wrong entry point passes while exercising nothing, which it did on the first attempt. Assertions are deliberately weak on what comes back: a malformed image may legitimately yield None, an error, or a structure. The contract under test is only that it returns rather than panicking. --- .../fuzz-crash-parse_state-add-overflow.bin | Bin 0 -> 138555 bytes .../data/fuzz-crash-read_dir-add-overflow.bin | Bin 0 -> 14888 bytes tests/fuzz_regressions.rs | 54 ++++++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 tests/data/fuzz-crash-parse_state-add-overflow.bin create mode 100644 tests/data/fuzz-crash-read_dir-add-overflow.bin create mode 100644 tests/fuzz_regressions.rs diff --git a/tests/data/fuzz-crash-parse_state-add-overflow.bin b/tests/data/fuzz-crash-parse_state-add-overflow.bin new file mode 100644 index 0000000000000000000000000000000000000000..28490fe8d066864c5fc54e406a869a345db60bfe GIT binary patch literal 138555 zcmeI*&ud&o902fnNt$emLO}|Zg6v5oLP#6h8nM>&M?k49Hi=$5bdyaq*zSg;EfqxD zi+J=O5Crk+NxfQcUc7k|#Dk~eVZEiNI`g_)H(_npW>+@5pDD|meQ##od_UipeR*t$ zgrE>0K!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs z0RjXF5FkK+009C7Ix8^N+2M>vfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C7 z2oNAZfB*pk1PBlyK!5;&-V)gAt?8Hz0RjXF5FkK+009C72oNAZfB*pk1PBlyK!898 z1x}xvojQ_ra6m&5AV7cs0RlZQaAE$^)U^A5&*#Ym2@oJafIue&7CirVlG9iO2oNAZ zfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk1PBlyK%g4}Sr`sS zLl(Z8sb!br2^uQ_3+LA=*KaPKoojv4@hr9+ihWc2^eP-LO`j+on>wa27{aaC zIUD`AjQ+Qg_r?7G7$g7bi(0l6yAG$Z(-1G_3xiYb(WUU{!`(VM5(m!WM7K`HO0Vav zTL>K(cPwW7R?K+$#!F#2j+b21&q8=Ugg4W+7{Yqm{vN{TY5P1PBlyK!5-N z0t5&UAV7cs0RjXF5FkK+009C72oNB!rv=92`mbN(`mc9x--zo#V`*BK-n_#2z+<9J zZ71XFuect!-8H$XPd1ePJ!Wu+yC#s<|9xCv|987h&jC)p&b#2W$x2oNC9?*a>+|NA|~;v+zS009C72oNAZV9*6d1|2~B1PBlyK!5;&t_uu> z;czEp;k##QS=@wB4B>J*YucVl6{VjA3fXY@B_{rp*9v)lW;e^qec?%3$T!sN{16Q$w! zAe)|98qY2Hpcm&CW{YR4SJyXIHaAzw#l_0na<#nD?goxef6DNe@75mnOV>fn`O^=2q~j0D*B||JExb_Q*Vs1Ngr@7Ko%*(G>){|R zDiisIQ;ZOE^G%0GS_GTsl*PC%#VGC_Ka|Inh5aGfwrJio+Tc}F{$GwxZ~akwc-__W sU;pN?DgUqRt^9u<1$AqzN>nVW7B2xV>a16H2}Tn)@m0`~@vFEk8h$zX&s6`z+bA zWn1gpOP1WaILopu$+9eIcW*LjHtkRW&mP-B(0xJtNibuF-+eR^`^aA53(f8J@1|MK zFJU7oNfOo7T4a+m$Qp$txT^U}q_M#D-M3JM$ZE}Ls@bLeovg^Po~M0?l~)*|&!1bC zrbi^_X^Oq}JSTarNz zoF-K_=27!i);MwXxKNFzIh=&b+brWcR-6~N*m)Yawu=#oloK9HxDi&bHDVK7)qI@D za1j6!Ljo=7EA4hon3zFO4h%T4#ky+@IlethY}O*^MVy6R0rK?p#?>r<40=akpkP)F zv!Y^h2}Vnb>12X(wWi~lxT3^!9Ca;})fi6!xDQEl&?(hFO-*cp7e(13|Kbsr#o)1N z(qX4m)*O|YfaO(*9DoW$b91CYUi%M9XR-%92wjsav!X>BQF$N_usR^-c}GSSg9BK@ zd6=5JpT}FP6nYCN>Gzm_#wx%!i-L zR|9@uTN%0~2V6zNMY=JU`lclJX1g~OK3fkSeZn(_2G`BhZyy)!Y&n}x=j~!PTddk? zyMzUaM?WRF4{tn6WB}ROh}TGc~Kc6n9)8%U3uFjV2@*K93*o2xM>s7GPrWYAr=Gai=+)ur#wxb=> z?auR9^F_kVQLe6N`>!jq5sieEfBg z+r3DB+FPw+#5Ev2H?aocpP9#hFX73u02gZ&VITE}l}A3iTW5G-=g?p5_>=8TcWK)1 zP1omdtC{(d6kNJe!T+G*VZg7qVQ3=47!r!|@*AfTA#npoU?4XEP*xNWb?ddHc!f+* zDmhYqiFifg1zoX=k!(lZB#Q!Pk!8^kWihxpO{!w=fd6C6nuAF$zh3*42XDO*CKnI4 zHOQzyd6Hw=i3&uXIZ_gtn`ZA`LK2Ha19BqKtU^4fi=z`hY21ztuA3`IYzp!EN3>u! zh>Is_z8LS)B>YU-BlvX`TXa12>QCI_S|=z2q9Ii#G4h^_wgVIVBQ&0g%|ZGDYwEdb Qr%zP5LXjzTqK!2F0D>d8!~g&Q literal 0 HcmV?d00001 diff --git a/tests/fuzz_regressions.rs b/tests/fuzz_regressions.rs new file mode 100644 index 0000000..de3bf61 --- /dev/null +++ b/tests/fuzz_regressions.rs @@ -0,0 +1,54 @@ +//! Inputs that once panicked the parser, kept as ordinary tests. +//! +//! These are real reproducers minted by the nightly fuzz job, not hand-authored +//! fixtures — they are the bytes libFuzzer actually found, downloaded from the +//! failing run's artifact. Both crashed with `attempt to add with overflow` on a +//! logical-block address computed from two header fields. +//! +//! They live here rather than only in `fuzz/corpus/` so that `cargo test` fails +//! if the bound regresses. A defect found by fuzzing should not need fuzzing to +//! be caught a second time: the fuzz job runs nightly, the test suite runs on +//! every push. +//! +//! The assertion is deliberately weak on *what* comes back. A malformed image +//! may legitimately parse to `None`, to an error, or even to a structure — the +//! contract under test is only that it returns at all rather than panicking. + +use std::io::Cursor; + +/// libFuzzer `parse_state` reproducer: overflowed `partition_start + fsd_lbn` +/// while resolving the File Set Descriptor location. +const PARSE_STATE_CRASH: &[u8] = include_bytes!("data/fuzz-crash-parse_state-add-overflow.bin"); + +/// libFuzzer `read_dir` reproducer: overflowed `partition_start + icb_lbn` +/// while walking a directory's File Identifier Descriptors. +const READ_DIR_CRASH: &[u8] = include_bytes!("data/fuzz-crash-read_dir-add-overflow.bin"); + +#[test] +fn parse_state_reproducer_does_not_panic() { + let _ = udf_forensic::parse_udf_state_checked(&mut Cursor::new(PARSE_STATE_CRASH)); + let _ = udf_forensic::parse_udf_state(&mut Cursor::new(PARSE_STATE_CRASH)); +} + +/// The `read_dir` harness steers addressing from the leading bytes rather than +/// parsing them out of the image, so the reproducer has to be decoded the same +/// way its fuzz target does or it exercises nothing. +#[test] +fn read_dir_reproducer_does_not_panic() { + const BLOCK_SIZES: [u32; 4] = [2048, 512, 1024, 4096]; + let (&sel, rest) = READ_DIR_CRASH + .split_first() + .expect("reproducer is non-empty"); + assert!(rest.len() >= 8, "reproducer carries the addressing prefix"); + + let block_size = BLOCK_SIZES[(sel as usize) % BLOCK_SIZES.len()]; + let partition_start = u32::from_le_bytes([rest[0], rest[1], rest[2], rest[3]]); + let dir_fe_lba = u32::from_le_bytes([rest[4], rest[5], rest[6], rest[7]]); + + let _ = udf_forensic::read_dir_at_lba( + &mut Cursor::new(&rest[8..]), + block_size, + partition_start, + dir_fe_lba, + ); +} From 13b2460f227004d8c1b04f8038b135f0796e9d16 Mon Sep 17 00:00:00 2001 From: Albert Hui Date: Tue, 4 Aug 2026 07:23:50 -0700 Subject: [PATCH 2/2] fix(lba): GREEN - check the address arithmetic the image chooses Both overflow sites add a partition start to a logical block number, and both operands are u32 fields from the image, so their sum can leave the address space: * the File Set Descriptor address. An overflow means the descriptor pair does not describe a location, so there is nothing to report and the walk returns `Ok(None)`. * a directory entry's File Entry address, per entry. The entry is dropped and the walk continues, because one unreadable FID says nothing about its siblings. Release builds wrap rather than panic, which is the worse half of this bug: a wrapped LBA addresses a real but wrong block, so the reader would have reported a confidently incorrect File Set Descriptor, or a file size read from an unrelated sector. Loud is better, absent is better still. The name-field bounds in the same block are header-derived too and now saturate. Saturating rather than skipping is deliberate: an out-of-range span clamps to the end of the buffer and the existing `id_end > id_start` test then leaves the name empty, which is what an unreadable name should produce. One trap worth recording. The first attempt used `continue` for the per-entry case, which read naturally and hung the test suite: the loop advances `off` *below* that block, so skipping the iteration left the cursor unmoved and spun on the same descriptor forever. Dropping the entry with `if let` keeps the advance on the normal path. Trading a panic for an infinite loop would have been a worse bug than the one being fixed, and only running the test caught it. Both reproducers now pass through their real fuzz targets, not just the unit tests. Full workspace: 0 failures; fmt and clippy clean. --- src/lib.rs | 53 ++++++++++++++++++++++++++++----------- tests/fuzz_regressions.rs | 5 ++++ 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index e044f7c..e307827 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -472,9 +472,18 @@ fn read_vds_checked( return Ok(None); }; + // Both operands are u32 fields from the descriptors, so their sum can leave + // the address space. A wrapped LBA would point at a real but wrong block -- + // a confidently incorrect File Set Descriptor -- so an overflow means the + // descriptor pair does not describe a location and there is nothing to + // report. + let Some(fsd_lba) = partition_start.checked_add(fsd) else { + return Ok(None); + }; + Ok(Some(VdsInfo { partition_start, - fsd_lba: partition_start + fsd, + fsd_lba, partition_kind: kind, map_count, })) @@ -593,25 +602,41 @@ fn parse_fids( if file_chars & FC_PARENT == 0 { let is_dir = file_chars & FC_DIRECTORY != 0; - let fe_lba = partition_start + icb_lbn; - - let id_start = off + tag_size + 20 + impl_use_len; - let id_end = (id_start + file_id_len).min(data.len()); + // The name-field bounds are header-derived too. Saturating is right + // here rather than skipping: an out-of-range span clamps to the end + // of the buffer, and the `id_end > id_start` test below then leaves + // the name empty instead of decoding anything. + let id_start = off + .saturating_add(tag_size) + .saturating_add(20) + .saturating_add(impl_use_len); + let id_end = id_start.saturating_add(file_id_len).min(data.len()); let name = if id_end > id_start { decode_osta_cs0(&data[id_start..id_end]) } else { String::new() }; - // Read the FE to get the canonical file size. - let size = read_fe_info_len(reader, block_size, fe_lba).unwrap_or(0); - - entries.push(UdfFileEntry { - name, - is_dir, - size, - fe_lba, - }); + // Same hazard as the FSD address above, per directory entry. Both + // operands are u32 from the image, so their sum can leave the + // address space; a wrapped LBA would send the File Entry read at a + // real but wrong block and report a confidently incorrect size. + // + // Dropping the entry rather than `continue`-ing is deliberate: the + // loop advances `off` below this block, so skipping the iteration + // would leave the cursor where it was and spin forever on the same + // descriptor. + if let Some(fe_lba) = partition_start.checked_add(icb_lbn) { + // Read the FE to get the canonical file size. + let size = read_fe_info_len(reader, block_size, fe_lba).unwrap_or(0); + + entries.push(UdfFileEntry { + name, + is_dir, + size, + fe_lba, + }); + } } off += fid_advance.max(4); diff --git a/tests/fuzz_regressions.rs b/tests/fuzz_regressions.rs index de3bf61..c574f3d 100644 --- a/tests/fuzz_regressions.rs +++ b/tests/fuzz_regressions.rs @@ -14,6 +14,11 @@ //! may legitimately parse to `None`, to an error, or even to a structure — the //! contract under test is only that it returns at all rather than panicking. +// An integration test is a separate crate, so the workspace's panic-free lints +// do not reach it and it carries its own allow — the same shape the fleet +// standard prescribes. A fixture that fails to load should fail loudly. +#![allow(clippy::expect_used)] + use std::io::Cursor; /// libFuzzer `parse_state` reproducer: overflowed `partition_start + fsd_lbn`