collective
Browse / github.com/storytold/gridcraft / patch
Security: print decodes pictures within explicit decoder limits (part of #2179)
Security: print decodes pictures within explicit decoder limits (part of #2179) Part of #2179, covering the print/PDF path. The chart-view path in `chartview.rs` is not in this PR. **Problem.** `decode_image` in `crates/engine/src/cmd/print/objects.rs`: 1. capped pixels (60M) using `io::image_size`, a separate hand-written header sniffer; 2. then decoded with `image::load_from_memory`, which has default limits and no side limit; 3. then made a second full copy with `to_rgba8`. The sniffer isn't the decoder's own reader. For JPEG it reads a 0xFF fill byte as a segment marker with a length, so a crafted file can show it one size and the decoder another. Reproduced on a r
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- Higgs32584 (imported from pull request #2721, open)
- Base
45f5ab18e4e57bf50ee88d2526deb4b87c5b24cb- Size
- +100 −17 in 2 files
- Review
- pending
- Shared
- 2026-10-28
- Id
f16a408d1247ab7c0c4903fe
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/cmd/print/objects.rs crates/engine/src/cmd/print/tests.rs
@@ -10,17 +10,8 @@ pub(super) fn draw_cell_picture(pg: &mut Page, data: &[u8], rect: (f32, f32, f32 if w <= 0.0 || h <= 0.0 || data.len() > 16 * 1024 * 1024 { return; }- let Some((iw, ih)) = crate::io::image_size(data) else { return };- if iw == 0 || ih == 0 || iw > 8192 || ih > 8192 || u64::from(iw) * u64::from(ih) > 16_000_000 {- return;- }- let Ok(mut reader) = image::ImageReader::new(std::io::Cursor::new(data)).with_guessed_format() else { return };- let mut limits = image::Limits::default();- limits.max_image_width = Some(8192);- limits.max_image_height = Some(8192);- limits.max_alloc = Some(64 * 1024 * 1024);- reader.limits(limits);- let Ok(image) = reader.decode() else { return };+ let Some(image) = decode_limited(data, CELL_PICTURE_CAPS) else { return };+ let (iw, ih) = (image.width(), image.height()); let image = image.thumbnail(((w / 72.0) * 150.0).ceil().clamp(1.0, 4000.0) as u32, ((h / 72.0) * 150.0).ceil().clamp(1.0, 4000.0) as u32).to_rgba8(); let fit = (w / iw as f32).min(h / ih as f32);@@ -33,15 +24,44 @@ fn anchor_rect(sh: &Sheet, a: &Anchor) -> (f64, f64, f64, f64) { (sh.col_left(a.cell.col) + a.dx as f64, sh.row_top(a.cell.row) + a.dy as f64, a.width.max(0.0) as f64, a.height.max(0.0) as f64) } -/// Decodes an embedded picture to RGBA, downscaled to at most `max_w`×`max_h` pixels.-fn decode_image(data: &[u8], max_w: u32, max_h: u32) -> Option<(Vec<u8>, u32, u32)> {- let (w, h) = crate::io::image_size(data)?;- if w == 0 || h == 0 || w as u64 * h as u64 > 60_000_000 {+/// How large a picture the print path decodes: longest side, pixels, and bytes the decoder+/// may allocate (which counts the bit depth and channels the header declares).+#[derive(Clone, Copy)]+pub(super) struct DecodeCaps {+ pub(super) max_side: u32,+ pub(super) max_pixels: u64,+ pub(super) max_alloc: u64,+}++/// In-cell pictures: the same limits `insert.picture` accepts.+const CELL_PICTURE_CAPS: DecodeCaps = DecodeCaps { max_side: 8192, max_pixels: 16_000_000, max_alloc: 64 * 1024 * 1024 };+/// Floating pictures come from any opened file. 256 MiB holds a 60-megapixel 8-bit RGBA+/// picture, but not a header declaring as many 16-bit pixels (twice that, before conversion).+pub(super) const FLOATING_PICTURE_CAPS: DecodeCaps = DecodeCaps { max_side: 16_384, max_pixels: 60_000_000, max_alloc: 256 * 1024 * 1024 };++/// Decodes a picture within `caps`. The size comes from the decoder's own header reader (not a+/// separate sniffer that a crafted file can disagree with), and the decoder is given the same+/// limits, so a picture over them is refused before its pixels are allocated.+pub(super) fn decode_limited(data: &[u8], caps: DecodeCaps) -> Option<image::DynamicImage> {+ let format = image::guess_format(data).ok()?;+ let (w, h) = image::ImageReader::with_format(std::io::Cursor::new(data), format).into_dimensions().ok()?;+ if w == 0 || h == 0 || w > caps.max_side || h > caps.max_side || u64::from(w) * u64::from(h) > caps.max_pixels { return None; }- let img = image::load_from_memory(data).ok()?;+ let mut reader = image::ImageReader::with_format(std::io::Cursor::new(data), format);+ let mut limits = image::Limits::default();+ limits.max_image_width = Some(caps.max_side);+ limits.max_image_height = Some(caps.max_side);+ limits.max_alloc = Some(caps.max_alloc);+ reader.limits(limits);+ reader.decode().ok()+}++/// Decodes an embedded picture to RGBA, downscaled to at most `max_w`×`max_h` pixels.+pub(super) fn decode_image(data: &[u8], max_w: u32, max_h: u32) -> Option<(Vec<u8>, u32, u32)> {+ let img = decode_limited(data, FLOATING_PICTURE_CAPS)?; let img = if img.width() > max_w.max(1) || img.height() > max_h.max(1) { img.thumbnail(max_w.max(1), max_h.max(1)) } else { img };- let rgba = img.to_rgba8();+ let rgba = img.into_rgba8(); let (pw, ph) = (rgba.width(), rgba.height()); Some((rgba.into_raw(), pw, ph)) }@@ -565,3 +565,66 @@ fn rotated_wrapped_text_prints_rotated() { assert_eq!(up + down, blocks.len(), "a header line printed flat: {blocks:?}"); assert!((1..=3).contains(&up) && (1..=3).contains(&down), "up {up}, down {down}: {blocks:?}"); }++fn png_of(img: image::DynamicImage) -> Vec<u8> {+ let mut png = Vec::new();+ img.write_to(&mut std::io::Cursor::new(&mut png), image::ImageFormat::Png).unwrap();+ png+}++/// Rewrites a PNG's IHDR size (and its CRC) so the header declares `w`×`h` pixels.+fn forge_png_size(png: &mut [u8], w: u32, h: u32) {+ let ihdr = png.windows(4).position(|c| c == b"IHDR").unwrap();+ png[ihdr + 4..ihdr + 8].copy_from_slice(&w.to_be_bytes());+ png[ihdr + 8..ihdr + 12].copy_from_slice(&h.to_be_bytes());+ let mut crc = !0u32;+ for &b in &png[ihdr..ihdr + 17] {+ crc ^= u32::from(b);+ for _ in 0..8 {+ crc = if crc & 1 == 1 { (crc >> 1) ^ 0xEDB8_8320 } else { crc >> 1 };+ }+ }+ png[ihdr + 17..ihdr + 21].copy_from_slice(&(!crc).to_be_bytes());+}++#[test]+fn print_decodes_pictures_within_byte_limits() {+ use super::objects::{DecodeCaps, FLOATING_PICTURE_CAPS, decode_image, decode_limited};+ // The byte limit counts the bit depth and channels the header declares, not just pixels:+ // 64×64 is 16 KiB as 8-bit RGBA but 32 KiB as 16-bit RGBA.+ let rgba8 = png_of(image::DynamicImage::ImageRgba8(image::RgbaImage::new(64, 64)));+ let rgba16 = png_of(image::DynamicImage::ImageRgba16(image::ImageBuffer::new(64, 64)));+ let caps = DecodeCaps { max_side: 1024, max_pixels: 1_000_000, max_alloc: 24 * 1024 };+ assert!(decode_limited(&rgba8, caps).is_some());+ assert!(decode_limited(&rgba16, caps).is_none());+ assert!(decode_limited(&rgba8, DecodeCaps { max_side: 32, ..caps }).is_none());+ assert!(decode_limited(&rgba8, DecodeCaps { max_pixels: 64 * 64 - 1, ..caps }).is_none());+ assert!(decode_limited(b"not a picture", caps).is_none());++ // A header declaring 7000×7000 16-bit RGBA pixels (49 megapixels, under the pixel cap, but+ // about 392 MB) is refused from its header, before anything that size is allocated.+ let mut bomb = rgba16.clone();+ forge_png_size(&mut bomb, 7000, 7000);+ assert_eq!(image::ImageReader::with_format(std::io::Cursor::new(&bomb), image::ImageFormat::Png).into_dimensions().unwrap(), (7000, 7000));+ assert!(decode_limited(&bomb, FLOATING_PICTURE_CAPS).is_none());+ assert!(decode_image(&bomb, 100, 100).is_none());+ assert!(decode_image(&rgba16, 100, 100).is_some_and(|(px, w, h)| (w, h) == (64, 64) && px.len() == 64 * 64 * 4));++ // Printing a sheet holding such a picture leaves it out instead of failing.+ let mut s = s();+ let d = s.doc_mut().unwrap();+ let wb = std::sync::Arc::make_mut(&mut d.wb);+ let id = wb.next_object_id();+ wb.sheet_mut(0).unwrap().images.push(Image {+ id,+ anchor: Anchor { cell: CellRef::new(1, 1), dx: 0.0, dy: 0.0, width: 40.0, height: 40.0, mode: gridcraft_model::AnchorMode::MoveAndSize },+ data: bomb.into(),+ mime: "image/png".into(),+ alt: String::new(),+ });+ s.execute("cell.set", json!({"cell": "A1", "input": "Report"})).unwrap();+ let (pdf, pages) = pdf_of(&mut s, json!({}));+ assert_eq!(pages, 1);+ assert!(contains(&pdf, "(Report) Tj"));+ assert!(!contains(&pdf, "/Subtype /Image"));+}