From 6616da2cf3009230c6dac9dcf43ca1f1b77febbf Mon Sep 17 00:00:00 2001 From: Albert Hui Date: Fri, 21 Aug 2026 21:16:12 +0800 Subject: [PATCH 1/2] =?UTF-8?q?test(cell):=20RED=20=E2=80=94=20a=20negativ?= =?UTF-8?q?e=20serial=20type=20must=20not=20wrap=20into=20a=20length?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by fuzzing ios-backup-core, whose Manifest.db path feeds this decoder attacker-controlled SQLite bytes. Seeded with real fixtures, the target crashed in 143,853 runs. A serial type is a varint, so a damaged record can decode one as a negative i64. Every arm above 11 is written for the positive cases, so a negative value falls through to the catch-all text arm and `((n - 13) / 2) as usize` wraps: serial -1 becomes a length of 18446744073709551609. let len = ((n - 13) / 2) as usize; let bytes = buf.get(off..off + len).ok_or(Error::TruncatedCell)?; `buf.get(range)` reads as though the bounds check makes this safe, but the range is constructed before `get` ever sees it. Under overflow checks the arithmetic panics; in a release build it wraps to a small number, `get` succeeds, and the caller receives bytes that are not the value -- silently wrong evidence, which is the worse of the two outcomes. The test fails at the subtraction rather than the addition, which is a second overflow site in the same expression: `i64::MIN + 1 - 13` underflows before any range is built. A companion test pins serial 12 and 13 decoding to empty blob/text, so the fix cannot buy safety by rejecting the smallest legal values. --- core/src/lib.rs | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/core/src/lib.rs b/core/src/lib.rs index 22b9fea..63656aa 100644 --- a/core/src/lib.rs +++ b/core/src/lib.rs @@ -5280,6 +5280,44 @@ fn be_u32(buf: &[u8], off: usize) -> u32 { mod tests { use super::*; + /// A serial type is a varint, and a varint in a damaged record can decode to + /// a **negative** i64. Every arm above 11 is written for the positive cases, + /// so a negative one falls through to the catch-all text arm, where + /// `((n - 13) / 2) as usize` wraps: serial `-1` becomes a length of + /// 18446744073709551609. + /// + /// `buf.get(off..off + len)` reads as though the bounds check makes that + /// safe, but the range is built *before* `get` sees it. With overflow checks + /// the add panics; in a release build it wraps to a small number, `get` + /// succeeds, and the caller is handed bytes that are not the value — + /// silently wrong evidence, which is the worse of the two. + #[test] + fn a_negative_serial_type_is_refused_rather_than_wrapping() { + let buf = [0u8; 64]; + + for serial in [-1_i64, -3, -14, -4096, i64::MIN + 1] { + let result = decode_value(&buf, 0, serial, TextEncoding::Utf8); + assert!( + result.is_err(), + "serial {serial} decoded to {result:?}; a negative serial type \ + identifies no value and must be refused, never length-wrapped" + ); + } + } + + /// The boundary the arms actually turn on, pinned so a fix for the negative + /// case cannot quietly reject the smallest legal blob/text instead. + #[test] + fn the_smallest_legal_blob_and_text_serials_still_decode_empty() { + let buf = [0u8; 8]; + + let (blob, used) = decode_value(&buf, 0, 12, TextEncoding::Utf8).unwrap(); + assert_eq!((blob, used), (Value::Blob(Vec::new()), 0)); + + let (text, used) = decode_value(&buf, 0, 13, TextEncoding::Utf8).unwrap(); + assert_eq!((text, used), (Value::Text(String::new()), 0)); + } + fn page_rc(byte: u8) -> std::rc::Rc<[u8]> { std::rc::Rc::from(vec![byte].into_boxed_slice()) } From 14bb9b50b676eeb33384a15cc3dd24190cd86fbb Mon Sep 17 00:00:00 2001 From: Albert Hui Date: Fri, 21 Aug 2026 21:22:24 +0800 Subject: [PATCH 2/2] =?UTF-8?q?fix(cell)!:=20GREEN=20=E2=80=94=20refuse=20?= =?UTF-8?q?a=20negative=20serial=20type=20instead=20of=20wrapping=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The catch-all text arm was written for "odd, >= 13" and also caught every negative serial type, which a varint in a damaged record readily produces. `i64::MIN - 13` underflows, and -1 gives `((-1 - 13) / 2) as usize` = 18446744073709551609, after which `off + len` overflows. Three changes, smallest to largest: - the text arm is now guarded `n if n >= 13`, so its subtraction cannot underflow by construction rather than by hope; - negatives fall to an explicit arm returning the new `Error::MalformedSerialType { serial, offset }`, carrying the offending value and where it was read. Folding them into TruncatedCell would send a reader hunting for a truncation that is not there; - every evidence-derived span goes through a `span()` helper that `checked_add`s before building the range. `span()` is the part that generalises. `buf.get(off..off + len)` reads as though the bounds check covers the arithmetic, and it does not -- the range is built before `get` is handed it. Under overflow checks that panics; in a RELEASE build it wraps to a small number, `get` succeeds, and the caller receives bytes that are not the value. The silent-wrong-evidence case is the worse one and it is the one that ships. The same shape was already known here: line 4887 uses saturating_add for exactly this, and the knowledge never reached its siblings. read_be_u64 is moved onto span() too. The remaining `off + 2` / `off + 4` / `off + frame_stride` sites are left alone deliberately -- each addend is a small constant or is bounded by the enclosing slice walk, so none can overflow, and churning them would bury the two that mattered. BREAKING CHANGE: `Error` gains a variant and becomes `#[non_exhaustive]`. Both are breaking, so they land together and spend the break once. There is no Display impl to update -- `Error` derives only Debug today, which is worth addressing separately. Verified beyond the unit tests: the 4.1 KB minimized input that crashed ios-backup-core's parse_manifest target now executes cleanly through a patched build, and a fresh 2,214,953-run seeded session over the same target added 1,947 corpus units and produced no artifact. --- core/src/lib.rs | 61 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 6 deletions(-) diff --git a/core/src/lib.rs b/core/src/lib.rs index 63656aa..326c0e6 100644 --- a/core/src/lib.rs +++ b/core/src/lib.rs @@ -40,6 +40,12 @@ use forensicnomicon::sqlite::{ /// Errors that can arise while reading a `SQLite` database, all recoverable — /// the reader never panics on malformed input. +/// +/// `#[non_exhaustive]` so naming a newly-recognised malformation is an additive +/// change. It is applied in the same release that adds +/// [`Error::MalformedSerialType`], because that addition is breaking either way +/// and doing both at once spends the break once instead of twice. +#[non_exhaustive] #[derive(Debug, Clone, PartialEq, Eq)] pub enum Error { /// File is shorter than the 100-byte header. @@ -54,6 +60,16 @@ pub enum Error { NotATablePage(u8), /// A cell pointer or payload ran past the end of its page. TruncatedCell, + /// A record's serial type was negative, so it names no value at all. A + /// serial type is a varint, and a damaged record decodes one as a negative + /// `i64`. Carries the offending value and the body offset it was read for + /// (Show-the-unrecognized-value). + MalformedSerialType { + /// The serial type as decoded, verbatim. + serial: i64, + /// Offset into the cell body the value would have started at. + offset: usize, + }, /// The b-tree was deeper / wider than the safety cap allows. TooManyPages, /// The freelist trunk chain cycled or exceeded the file's page count. @@ -4691,22 +4707,55 @@ fn decode_value( 9 => (Value::Integer(1), 0), n if n >= 12 && n % 2 == 0 => { let len = ((n - 12) / 2) as usize; - let bytes = buf.get(off..off + len).ok_or(Error::TruncatedCell)?; + let bytes = span(buf, off, len)?; (Value::Blob(bytes.to_vec()), len) } - n => { - // odd, >= 13: text, decoded per the database's text encoding - // (UTF-8 / UTF-16LE / UTF-16BE). Lossy so a corrupt byte can't panic. + // odd, >= 13: text, decoded per the database's text encoding + // (UTF-8 / UTF-16LE / UTF-16BE). Lossy so a corrupt byte can't panic. + // + // The `>= 13` guard is what keeps the subtraction below in range. It was + // previously a catch-all `n =>`, which also swallowed every NEGATIVE + // serial type — and a serial type is a varint, so a damaged record + // produces those. `i64::MIN - 13` underflows outright, and `-1` yields + // `((-1 - 13) / 2) as usize` = 18446744073709551609. + n if n >= 13 => { let len = ((n - 13) / 2) as usize; - let bytes = buf.get(off..off + len).ok_or(Error::TruncatedCell)?; + let bytes = span(buf, off, len)?; (Value::Text(enc.decode(bytes)), len) } + // Only negatives reach here: 0..=11 are named above and everything from + // 12 up is claimed by the two arms. A negative serial type identifies no + // value at all, so it is a malformed record rather than a short one — + // reported with the offending value and its offset rather than folded + // into TruncatedCell, which would send a reader looking for a truncation + // that is not there. + n => { + return Err(Error::MalformedSerialType { + serial: n, + offset: off, + }) + } }) } +/// Take `buf[off..off + len]`, refusing rather than forming the range unchecked. +/// +/// `buf.get(off..off + len)` reads as though the bounds check covers everything, +/// and it does not: the range is constructed *before* `get` is given it, so a +/// length taken from the evidence overflows the add. Under overflow checks that +/// panics; in a release build it wraps to a small number, `get` succeeds, and +/// the caller is handed a slice that is not the value it asked for — wrong bytes +/// reported as fact, which is worse than the crash. +/// +/// Every span whose length comes from the file goes through here. +fn span(buf: &[u8], off: usize, len: usize) -> Result<&[u8], Error> { + let end = off.checked_add(len).ok_or(Error::TruncatedCell)?; + buf.get(off..end).ok_or(Error::TruncatedCell) +} + /// Read `width` (1..=8) big-endian bytes into a raw u64 (no sign extension). fn read_be_u64(buf: &[u8], off: usize, width: usize) -> Result { - let bytes = buf.get(off..off + width).ok_or(Error::TruncatedCell)?; + let bytes = span(buf, off, width)?; let mut acc: u64 = 0; for &b in bytes { acc = (acc << 8) | u64::from(b);