From e83bf7742e26056334185ba69c14c0647f7f8591 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 04:32:58 -0400 Subject: [PATCH] security: bound codec allocation to what a hostile file declares (#2769) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * security: bound codec allocation to what a hostile file declares A GIF's first image descriptor could name a frame up to four times the canvas budget and be charged that allocation before any size check saw it; PNG's compressed text chunks were expanded through an unbounded inflate; JPEG handed hostile frame headers to the allocating decoders (zune for the common cases, jpeg-decoder for CMYK/scaled/lossless) and let an APP1 flood drive quadratic metadata reassembly. Each now refuses on the same max_pixels budget the other decoders honour, with a regression test per gate. 🤖 Generated with Codebuff Co-Authored-By: Codebuff * Fix clippy collapsible_if in DCT-scale size check * Check jpeg-decoder header on scaled path; cap APP1 markers in decode_codes8 Co-Authored-By: Claude Sonnet 5.5 --------- Co-authored-by: Codebuff Co-authored-by: Dylan Taylor Co-authored-by: Claude Sonnet 5.5 --- crates/codecs/src/jpeg.rs | 116 ++++++++++++++++++++++++++++++--- crates/codecs/src/other.rs | 83 +++++++++++++++++++++++ crates/codecs/src/png_codec.rs | 65 +++++++++++++++++- 3 files changed, 255 insertions(+), 9 deletions(-) diff --git a/crates/codecs/src/jpeg.rs b/crates/codecs/src/jpeg.rs index b8de12bdd..d8b0241a1 100644 --- a/crates/codecs/src/jpeg.rs +++ b/crates/codecs/src/jpeg.rs @@ -17,6 +17,8 @@ pub(crate) struct Markers { pub sof: u8, /// Number of components in the first scan (< `components` for non-interleaved sequential files). pub first_scan_components: u8, + /// Number of APP1 segments: capped before the allocating decoders walk them again. + pub app1: u32, pub exif: Option>, /// Offset of the EXIF TIFF header within the file. pub exif_offset: usize, @@ -83,6 +85,7 @@ pub(crate) fn parse_markers(b: &[u8]) -> Option { m.components = seg[5]; } 0xE1 => { + m.app1 = m.app1.saturating_add(1); if seg.starts_with(b"Exif\0\0") && m.exif.is_none() { m.exif = Some(seg[6..].to_vec()); m.exif_offset = start + 6; @@ -121,6 +124,8 @@ pub(crate) fn decode(bytes: &[u8], opts: &DecodeOptions) -> Result { decode_with_fallback(bytes, opts, None) } +const MAX_APP1: u32 = 128; + /// Stored dimensions and EXIF orientation from the markers, without decoding the scan data. The /// file is refused, as a decode would refuse it, when it has no frame header, an unsupported frame /// type or component count, or when its markers don't run to an end-of-image marker after the @@ -178,6 +183,11 @@ pub(crate) fn decode_with_fallback(bytes: &[u8], opts: &DecodeOptions, fallback: return Err(Error::Malformed(F, "no frame header".into())); } check_size(F, m.width as u64, m.height as u64, opts)?; + // The allocating decoders below parse the markers again, all of them: the count is capped so + // their quadratic/unbounded handling of APP1 metadata stays linear in file size. + if m.app1 > MAX_APP1 { + return Err(Error::Malformed(F, "too many APP1 markers".into())); + } let scale_to = opts.max_size.filter(|&(mw, mh)| mw > 0 && mh > 0 && (m.width >= mw.saturating_mul(2) || m.height >= mh.saturating_mul(2))); // zune-jpeg handles the common cases fastest; jpeg-decoder covers DCT scaling, CMYK/YCCK, // 12-bit, non-interleaved sequential scans (which zune-jpeg 0.5 mis-decodes with subsampling) @@ -185,9 +195,9 @@ pub(crate) fn decode_with_fallback(bytes: &[u8], opts: &DecodeOptions, fallback: let lossless = m.sof == 0xC3; let non_interleaved = matches!(m.sof, 0xC0 | 0xC1) && m.first_scan_components < m.components; let raw = if m.components == 4 || scale_to.is_some() || m.precision > 8 || non_interleaved || lossless { - decode_jpeg_decoder(bytes, &m, scale_to)? + decode_jpeg_decoder(bytes, &m, scale_to, opts)? } else { - decode_zune(bytes, &m)? + decode_zune(bytes, &m, opts)? }; // A raw container may carry the colour declaration of an otherwise untagged preview. // Apply it before linearization/resizing; existing JPEG metadata always wins. @@ -196,7 +206,7 @@ pub(crate) fn decode_with_fallback(bytes: &[u8], opts: &DecodeOptions, fallback: finish(F, raw, meta, (m.width, m.height), opts) } -fn decode_zune(bytes: &[u8], m: &Markers) -> Result { +fn decode_zune(bytes: &[u8], m: &Markers, budget: &DecodeOptions) -> Result { use zune_core::bytestream::ZCursor; use zune_core::colorspace::ColorSpace; use zune_core::options::DecoderOptions; @@ -207,6 +217,11 @@ fn decode_zune(bytes: &[u8], m: &Markers) -> Result { ColorSpace::RGB }); let mut d = zune_jpeg::JpegDecoder::new_with_options(ZCursor::new(bytes), opts); + // zune's `decode` allocates straight from the frame header it parses, with its own caps + // set in us: check that header against the same limits the other decoders honour first. + d.decode_headers().map_err(|e| Error::Malformed(F, e.to_string()))?; + let (zw, zh) = d.info().map(|i| (i.width as u64, i.height as u64)).ok_or_else(|| Error::Malformed(F, "no info".into()))?; + check_size(F, zw, zh, budget)?; let px = d.decode().map_err(|e| Error::Malformed(F, e.to_string()))?; let info = d.info().ok_or_else(|| Error::Malformed(F, "no info".into()))?; let (w, h) = (info.width as usize, info.height as usize); @@ -217,10 +232,15 @@ fn decode_zune(bytes: &[u8], m: &Markers) -> Result { Ok(Raw { width: w, height: h, model, alpha: false, premultiplied: false, buf: Buf::U8(px), bit_depth: 8 }) } -fn decode_jpeg_decoder(bytes: &[u8], m: &Markers, scale_to: Option<(u32, u32)>) -> Result { +fn decode_jpeg_decoder(bytes: &[u8], m: &Markers, scale_to: Option<(u32, u32)>, budget: &DecodeOptions) -> Result { use jpeg_decoder::PixelFormat; let mut d = jpeg_decoder::Decoder::new(bytes); d.read_info().map_err(|e| Error::Malformed(F, e.to_string()))?; + // Its frame header is what a decode allocates from (progressive coefficient buffers follow the + // unscaled MCU grid): check it against the same limits the other decoders honour, before any + // DCT scale is requested. + let info = d.info().ok_or_else(|| Error::Malformed(F, "no info".into()))?; + check_size(F, info.width as u64, info.height as u64, budget)?; // `scale` is the DCT size, not a lossless resample: on a lossless frame it would cut the // output to the top-left of each plane. Those decode at full size; `finish` scales after. if let Some((mw, mh)) = scale_to.filter(|_| m.sof != 0xC3) { @@ -369,11 +389,18 @@ pub(crate) fn decode_codes8(bytes: &[u8]) -> Result<(usize, usize, usize, Vec MAX_APP1 { + return Err(Error::Malformed(F, "too many APP1 markers".into())); + } if !matches!(m.components, 1 | 3) || m.precision != 8 { return Err(Error::Malformed(F, "expected an 8-bit greyscale or RGB image".into())); } let non_interleaved = matches!(m.sof, 0xC0 | 0xC1) && m.first_scan_components < m.components; - let raw = if non_interleaved { decode_jpeg_decoder(bytes, &m, None)? } else { decode_zune(bytes, &m)? }; + let raw = if non_interleaved { + decode_jpeg_decoder(bytes, &m, None, &DecodeOptions::default())? + } else { + decode_zune(bytes, &m, &DecodeOptions::default())? + }; let ch = raw.model.channels(); match raw.buf { Buf::U8(v) if v.len() >= raw.width * raw.height * ch => Ok((raw.width, raw.height, ch, v)), @@ -513,7 +540,7 @@ mod tests { fn lossless_grayscale_samples_are_native_endian_and_left_justified() { let bytes = lossless_jpeg(1, 8, 12); let m = parse_markers(&bytes).unwrap(); - let raw = decode_jpeg_decoder(&bytes, &m, None).unwrap(); + let raw = decode_jpeg_decoder(&bytes, &m, None, &DecodeOptions::default()).unwrap(); let Buf::U16(v) = &raw.buf else { panic!("expected 16-bit samples") }; assert_eq!(v, &vec![2048u16 << 4; 8], "0x0800 native-endian, left-justified"); assert_eq!((raw.model, raw.bit_depth), (Model::Gray, 16)); @@ -525,7 +552,7 @@ mod tests { fn lossless_high_precision_rgb_is_16_bit() { let bytes = lossless_jpeg(3, 2, 12); let m = parse_markers(&bytes).unwrap(); - let raw = decode_jpeg_decoder(&bytes, &m, None).unwrap(); + let raw = decode_jpeg_decoder(&bytes, &m, None, &DecodeOptions::default()).unwrap(); let Buf::U16(v) = &raw.buf else { panic!("expected 16-bit samples") }; assert_eq!(v, &vec![2048u16 << 4; 6]); assert_eq!(raw.model, Model::Rgb); @@ -536,7 +563,7 @@ mod tests { fn lossless_high_precision_cmyk_is_16_bit() { let bytes = lossless_jpeg(4, 2, 12); let m = parse_markers(&bytes).unwrap(); - let raw = decode_jpeg_decoder(&bytes, &m, None).unwrap(); + let raw = decode_jpeg_decoder(&bytes, &m, None, &DecodeOptions::default()).unwrap(); let Buf::U16(v) = &raw.buf else { panic!("expected 16-bit samples") }; assert_eq!(v, &vec![2048u16 << 4; 8]); assert_eq!((raw.model, raw.bit_depth), (Model::Cmyk, 16)); @@ -551,4 +578,77 @@ mod tests { assert_eq!((d.width, d.height), (8, 1)); assert_eq!(d.bit_depth, 8); } + + /// Rewrites an encoded file's frame-header dimensions: same tables and scan, a hostile + /// annotated size. + fn with_dims(bytes: &[u8], w: u16, h: u16) -> Vec { + let mut out = bytes.to_vec(); + let mut p = 2usize; + while p + 1 < out.len() { + if out[p] != 0xFF { + p += 1; + continue; + } + match out[p + 1] { + 0xC0..=0xCF if out[p + 1] != 0xC4 && out[p + 1] != 0xC8 && out[p + 1] != 0xCC => { + out[p + 5..p + 7].copy_from_slice(&h.to_be_bytes()); + out[p + 7..p + 9].copy_from_slice(&w.to_be_bytes()); + return out; + } + 0x01 | 0xD0..=0xD9 => p += 2, + 0xFF => p += 1, + _ => { + let Some(l) = out.get(p + 2..p + 4) else { return out }; + p += 2 + usize::from(u16::from_be_bytes([l[0], l[1]])); + } + } + } + out + } + + /// A file claiming 65535×65535 must be refused for size before zune-jpeg allocates from + /// its frame header (4 GB+ of samples from roughly five hundred bytes of tables). + #[test] + fn zune_refuses_its_own_frame_header_over_the_budget() { + use crate::{ChromaSubsampling, EncodeImage, EncodeMeta, Samples, encode_jpeg}; + let pixels = vec![128u8; 32 * 24]; + let jpeg = encode_jpeg(&EncodeImage::new(32, 24, 1, Samples::U8(&pixels)), 90, ChromaSubsampling::S444, &EncodeMeta::default()).unwrap(); + let hostile = with_dims(&jpeg, 65535, 65535); + assert_ne!(hostile, jpeg); + match crate::decode(&hostile, DecodeOptions::default()) { + Err(Error::TooLarge(w, h)) => assert_eq!((w, h), (65535, 65535)), + other => panic!("unexpected: {other:?}"), + } + } + + /// The jpeg-decoder branch (a 4-component file) is checked the same way: its own + /// `read_info` result is what a full-size decode allocates from. + #[test] + fn jpeg_decoder_refuses_its_own_frame_header_over_the_budget() { + use crate::{ChromaSubsampling, EncodeImage, EncodeMeta, Samples, encode_jpeg}; + let pixels = vec![128u8; 32 * 24 * 4]; + let jpeg = encode_jpeg(&EncodeImage::new(32, 24, 4, Samples::U8(&pixels)), 90, ChromaSubsampling::S444, &EncodeMeta::default()).unwrap(); + let hostile = with_dims(&jpeg, 65535, 65535); + match crate::decode(&hostile, DecodeOptions::default()) { + Err(Error::TooLarge(w, h)) => assert_eq!((w, h), (65535, 65535)), + other => panic!("unexpected: {other:?}"), + } + } + + /// Extended XMP is reassembled from every APP1 fragment: an unbounded count is quadratic + /// memory in a marker walk, so the count is capped. + #[test] + fn an_app1_flood_is_refused_before_the_decoders_rewalk_it() { + let mut b = vec![0xFF, 0xD8]; + // a 1-component SOF0: 8-bit, 2×2, component (1, 0x11, Q0) + b.extend_from_slice(&[0xFF, 0xC0, 0x00, 0x0B, 8, 0, 2, 0, 2, 1, 0x01, 0x11, 0x00]); + for _ in 0..200 { + b.extend_from_slice(&[0xFF, 0xE1, 0x00, 0x02]); + } + b.extend_from_slice(&[0xFF, 0xD9]); + match crate::decode(&b, DecodeOptions::default()) { + Err(Error::Malformed(F, m)) => assert!(m.contains("APP1"), "{m}"), + other => panic!("unexpected: {other:?}"), + } + } } diff --git a/crates/codecs/src/other.rs b/crates/codecs/src/other.rs index d5659de90..ebba2eaed 100644 --- a/crates/codecs/src/other.rs +++ b/crates/codecs/src/other.rs @@ -5,6 +5,9 @@ use crate::{DecodeOptions, Decoded, Error, Format, Result}; use image::{DynamicImage, ImageDecoder}; pub(crate) fn decode(bytes: &[u8], format: Format, opts: &DecodeOptions) -> Result { + if format == Format::Gif { + gated_first_frame(bytes, opts)?; + } let err = |e: image::ImageError| Error::Malformed(format, e.to_string()); let cursor = std::io::Cursor::new(bytes); let (img, icc) = match format { @@ -36,3 +39,83 @@ pub(crate) fn decode(bytes: &[u8], format: Format, opts: &DecodeOptions) -> Resu let raw = Raw { width: w as usize, height: h as usize, model, alpha, premultiplied: false, buf: Buf::U8(buf), bit_depth: 8 }; finish(format, raw, Meta { icc, ..Default::default() }, (w, h), opts) } + +/// Bounds a GIF's first image descriptor: the decoder composites frames into the logical screen +/// (bounded by [`check_size`] on the canvas) but allocates its own frame buffers at the frame's +/// declared size, which a small file can inflate far past the canvas. The canvas check cannot +/// see that size, so it is read from the block headers here. +fn gated_first_frame(bytes: &[u8], opts: &DecodeOptions) -> Result<()> { + // The constants of the header grammar: signature, logical screen descriptor (with its global + // colour table), extension blocks (0x21, skipped by their sub-block chains) and image + // descriptors (0x2C). + const SCREEN: usize = 13; + if bytes.len() < SCREEN { + return Ok(()); // too broken for a frame; the decoder reports the damage itself + } + let gct_entries = 1usize << (u32::from(bytes[10] & 0x07) + 1); // at most 256 + let gct_bytes = if (bytes[10] & 0x80) != 0 { gct_entries * 3 } else { 0 }; + let mut p = SCREEN.saturating_add(gct_bytes); + while let Some(&kind) = bytes.get(p) { + match kind { + 0x2C => { + // Image descriptor: two LE coordinates then two LE edges, after the separator. + let end = p.checked_add(10).filter(|&e| e <= bytes.len()); + let Some(d) = end.and_then(|e| bytes.get(p + 1..e)) else { + break; // cut short mid-descriptor: the decoder reports that + }; + let (w, h) = (u16::from_le_bytes([d[4], d[5]]) as u64, u16::from_le_bytes([d[6], d[7]]) as u64); + return check_size(Format::Gif, w, h, opts); + } + 0x21 => { + // Extension: a label byte, then sub-blocks ended by a zero length. + p = p.saturating_add(2); + loop { + let Some(&len) = bytes.get(p) else { + return Ok(()); // ran past the data: decode reports the truncation + }; + p = p.saturating_add(1); + if len == 0 { + break; + } + p = p.saturating_add(len as usize); + } + } + _ => p = p.saturating_add(1), + } + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use crate::{DecodeOptions, Error}; + + fn gif(blocks: &[u8]) -> Vec { + let mut b = b"GIF89a".to_vec(); + // logical screen: 2×2, no global colour table, background 0, aspect 0 + b.extend_from_slice(&[2, 0, 2, 0, 0x00, 0, 0]); + b.extend_from_slice(blocks); + b + } + + /// `image`'s GIF decoder allocates its frame buffer at the image descriptor's declared + /// size, which the canvas bound ([`crate::decode`]'s `max_pixels` on the logical screen) + /// does not cover. A 20-byte file used to buy a 4 GB allocation before the gate. + #[test] + fn a_first_frame_over_the_bound_is_refused_without_allocating_it() { + let bytes = gif(&[0x2C, 0, 0, 0, 0, 0xFF, 0xFF, 0xFF, 0xFF, 0, 0x00]); + match crate::decode(&bytes, DecodeOptions::default()) { + Err(Error::TooLarge(w, h)) => assert_eq!((w, h), (65535, 65535)), + other => panic!("unexpected: {other:?}"), + } + } + + /// A frame within the bound still reaches the decoder (which reports the truncated data); + /// the gate may only refuse on size. + #[test] + fn a_first_frame_within_the_bound_is_not_refused() { + let bytes = gif(&[0x2C, 0, 0, 0, 0, 1, 0, 1, 0, 0, 0x00]); + let r = crate::decode(&bytes, DecodeOptions::default()); + assert!(matches!(r, Err(Error::Malformed(..)) | Err(Error::TooLarge(..))), "{r:?}"); + } +} diff --git a/crates/codecs/src/png_codec.rs b/crates/codecs/src/png_codec.rs index 098f1cfaa..6837d178e 100644 --- a/crates/codecs/src/png_codec.rs +++ b/crates/codecs/src/png_codec.rs @@ -7,6 +7,10 @@ use lightcraft_color::{Mat3, RgbSpace, Xy}; const F: Format = Format::Png; +/// Expansion cap for a compressed text chunk's payload: real XMP documents are a few hundred +/// kilobytes, so anything past this is hostile input, not metadata. +const MAX_TEXT_CHUNK: usize = 4 << 20; + fn err(e: impl std::fmt::Display) -> Error { Error::Malformed(F, e.to_string()) } @@ -42,11 +46,18 @@ pub(crate) fn decode(bytes: &[u8], opts: &DecodeOptions) -> Result { }; let raw = Raw { width: out.width as usize, height: out.height as usize, model, alpha, premultiplied: false, buf: samples, bit_depth: depth }; + // png's `get_text()` inflates a compressed chunk with no bound of its own: a tiny + // iTXt can claim an XMP document of arbitrary size. Expand through the bounded path + // instead, and drop any XMP bigger than the cap. let xmp = info .utf8_text .iter() .find(|t| t.keyword == "XML:com.adobe.xmp") - .and_then(|t| t.get_text().ok()) + .and_then(|t| { + let mut t = t.clone(); + t.decompress_text_with_limit(MAX_TEXT_CHUNK).ok()?; + t.get_text().ok() + }) .or_else(|| info.uncompressed_latin1_text.iter().find(|t| t.keyword == "XML:com.adobe.xmp").map(|t| t.text.clone())); let exif = info.exif_metadata.as_ref().map(|e| { let e = e.to_vec(); @@ -168,3 +179,55 @@ fn rgb_to_xyz_d50_checked(space: &RgbSpace) -> Option { } Some(crate::space::rgb_to_xyz_d50(space)) } + +#[cfg(test)] +mod tests { + use crate::DecodeOptions; + + /// A compressed text chunk is expanded by `png` through our own cap: an XMP document + /// bigger than it is dropped, not inflated. Compressing an 8 MiB string is a few + /// kilobytes — the reverse of what the unbounded `get_text()` path allowed. + #[test] + fn compressed_xmp_over_the_cap_is_dropped() { + let out = png_with_compressed_xmp(&"x".repeat(8 << 20)); + let d = crate::decode(&out, DecodeOptions::default()).unwrap(); + assert!(d.xmp.is_none()); + assert_eq!((d.width, d.height), (1, 1)); + } + + /// XMP within the cap is still read, compressed or not. + #[test] + fn compressed_xmp_under_the_cap_is_read() { + let out = png_with_compressed_xmp(&"".repeat(100)); + let d = crate::decode(&out, DecodeOptions::default()).unwrap(); + assert!(d.xmp.as_deref().is_some_and(|x| x.contains("xmp/"))); + } + + /// A little-endian PNG with one compressed iTXt chunk carrying `payload` as XMP. + fn png_with_compressed_xmp(payload: &str) -> Vec<u8> { + use png::text_metadata::{EncodableTextChunk, ITXtChunk}; + let mut base = Vec::new(); + { + let mut enc = png::Encoder::new(&mut base, 1, 1); + enc.set_color(png::ColorType::Grayscale); + enc.set_depth(png::BitDepth::Eight); + let mut w = enc.write_header().unwrap(); + w.write_image_data(&[0x7F]).unwrap(); + w.finish().unwrap(); + } + // `ITXtChunk::encode` writes the complete chunk (length, type, payload, CRC), with + // the text compressed. + let mut chunk = Vec::new(); + let mut xmp = ITXtChunk::new("XML:com.adobe.xmp", payload); + xmp.compressed = true; + xmp.compress_text().unwrap(); + xmp.encode(&mut chunk).unwrap(); + // Text chunks after IDAT land in `info` once the stream is finished: insert ours + // before the trailing IEND (a zero-length chunk is exactly 12 bytes). + let split = base.len() - 12; + let mut out = base[..split].to_vec(); + out.extend_from_slice(&chunk); + out.extend_from_slice(&base[split..]); + out + } +}