Skip to content

Commit 1076737

Browse files
lilithlilith
authored
Merge pull request #704 from imazen/fix/gif-security-hardening-v2
fix: harden GIF decoder against malformed input
2 parents 0635e38 + 7f319d3 commit 1076737

File tree

8 files changed

+281
-31
lines changed

8 files changed

+281
-31
lines changed

‎imageflow_core/src/codecs/gif/disposal.rs‎

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -28,17 +28,17 @@ impl<Pixel: Copy> Default for Disposal<Pixel> {
2828

2929
impl<Pixel: Copy> Disposal<Pixel> {
3030
pub fn dispose(&mut self, pixels: &mut [Pixel], stride: usize, bg_color: Pixel) {
31-
if self.width == 0 || self.height == 0 {
32-
return;
33-
}
34-
35-
let pixels_iter = pixels.iter_mut().subimage(
36-
self.left as usize,
37-
self.top as usize,
31+
let (w, h, l, t) = (
3832
self.width as usize,
3933
self.height as usize,
40-
stride,
34+
self.left as usize,
35+
self.top as usize,
4136
);
37+
if w == 0 || h == 0 || l.saturating_add(w) > stride {
38+
return;
39+
}
40+
41+
let pixels_iter = pixels.iter_mut().subimage(l, t, w, h, stride);
4242
match self.method {
4343
Background => {
4444
for px in pixels_iter {
@@ -56,25 +56,26 @@ impl<Pixel: Copy> Disposal<Pixel> {
5656
}
5757
}
5858

59-
pub fn new(frame: &gif::Frame, pixels: &[Pixel], stride: usize) -> Self {
59+
pub fn new(frame: &gif::Frame, pixels: &[Pixel], stride: usize, canvas_height: usize) -> Self {
60+
// Clip frame bounds to canvas dimensions.
61+
let l = (frame.left as usize).min(stride);
62+
let t = (frame.top as usize).min(canvas_height);
63+
let w = (frame.width as usize).min(stride.saturating_sub(l));
64+
let h = (frame.height as usize).min(canvas_height.saturating_sub(t));
65+
let valid = w > 0 && h > 0;
66+
6067
Disposal {
6168
method: frame.dispose,
62-
left: frame.left,
63-
top: frame.top,
64-
width: frame.width,
65-
height: frame.height,
69+
left: l as u16,
70+
top: t as u16,
71+
width: w as u16,
72+
height: h as u16,
6673
previous_pixels: match frame.dispose {
67-
Previous => Some(
74+
Previous if valid => Some(
6875
pixels
6976
.iter()
7077
.cloned()
71-
.subimage(
72-
frame.left as usize,
73-
frame.top as usize,
74-
frame.width as usize,
75-
frame.height as usize,
76-
stride,
77-
)
78+
.subimage(l, t, w, h, stride)
7879
.collect(),
7980
),
8081
_ => None,

‎imageflow_core/src/codecs/gif/screen.rs‎

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ impl Screen {
3131

3232
let pixels = reader.width() as usize * reader.height() as usize;
3333
let bg_color = if let (Some(bg_index), Some(pal)) = (reader.bg_color(), pal.as_ref()) {
34-
pal[bg_index]
34+
pal.get(bg_index).copied().unwrap_or_default()
3535
} else {
3636
BGRA8::default()
3737
};
@@ -56,27 +56,39 @@ impl Screen {
5656
.or(self.global_pal.as_ref())
5757
.ok_or("the frame must have _some_ palette")?;
5858

59+
// Clip frame bounds to canvas (matches browser behavior for malformed GIFs).
60+
let left = (frame.left as usize).min(self.width);
61+
let top = (frame.top as usize).min(self.height);
62+
let sub_w = (frame.width as usize).min(self.width.saturating_sub(left));
63+
let sub_h = (frame.height as usize).min(self.height.saturating_sub(top));
64+
5965
self.disposal.dispose(&mut self.pixels, self.width, self.bg_color);
60-
self.disposal = Disposal::new(frame, &self.pixels, self.width);
66+
self.disposal = Disposal::new(frame, &self.pixels, self.width, self.height);
67+
68+
if sub_w == 0 || sub_h == 0 {
69+
return Ok(());
70+
}
71+
72+
// Offset into the frame's pixel buffer to account for clipping.
73+
let frame_left_clip = left.saturating_sub(frame.left as usize);
74+
let frame_top_clip = top.saturating_sub(frame.top as usize);
6175

6276
for (dst, &src) in self
6377
.pixels
6478
.iter_mut()
65-
.subimage(
66-
frame.left as usize,
67-
frame.top as usize,
68-
frame.width as usize,
69-
frame.height as usize,
70-
self.width,
79+
.subimage(left, top, sub_w, sub_h, self.width)
80+
.zip(
81+
buffer
82+
.iter()
83+
.subimage(frame_left_clip, frame_top_clip, sub_w, sub_h, frame.width as usize),
7184
)
72-
.zip(buffer.iter())
7385
{
7486
if let Some(transparent) = frame.transparent {
7587
if src == transparent {
7688
continue;
7789
}
7890
}
79-
*dst = pal[src as usize];
91+
*dst = pal.get(src as usize).copied().unwrap_or(BGRA8 { r: 0, g: 0, b: 0, a: 0 });
8092
}
8193

8294
Ok(())

‎imageflow_core/tests/crash_repro/gif_bad_bg_index.gif‎

35 Bytes
Loading

‎imageflow_core/tests/crash_repro/gif_frame_exceeds_canvas.gif‎

35 Bytes
Loading

‎imageflow_core/tests/crash_repro/gif_oob_palette_index.gif‎

40 Bytes
Loading

‎imageflow_core/tests/crash_repro/gif_overflow_frame_pos.gif‎

35 Bytes
Loading

‎imageflow_core/tests/crash_repro/gif_zero_size_frame.gif‎

35 Bytes
Loading

‎imageflow_core/tests/integration/robustness.rs‎

Lines changed: 237 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -373,6 +373,243 @@ fn test_gif_palette_bounds() {
373373
}
374374
}
375375

376+
// =============================================================================
377+
// GIF frame bounds clipping (frames extending beyond canvas)
378+
// =============================================================================
379+
380+
#[test]
381+
fn test_gif_frame_exceeds_canvas() {
382+
let gif =
383+
fs::read(repo_root().join("imageflow_core/tests/crash_repro/gif_frame_exceeds_canvas.gif"))
384+
.expect("read fixture");
385+
let mut ctx = create_context();
386+
let _ = ctx.add_copied_input_buffer(0, &gif);
387+
388+
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
389+
let _ = ctx.add_output_buffer(1);
390+
let _ = ctx.build_1(s::Build001 {
391+
builder_config: None,
392+
io: vec![
393+
s::IoObject {
394+
direction: s::IoDirection::In,
395+
io_id: 0,
396+
io: s::IoEnum::Placeholder,
397+
},
398+
s::IoObject {
399+
direction: s::IoDirection::Out,
400+
io_id: 1,
401+
io: s::IoEnum::OutputBuffer,
402+
},
403+
],
404+
framewise: s::Framewise::Steps(vec![
405+
s::Node::Decode { io_id: 0, commands: None },
406+
s::Node::Encode { io_id: 1, preset: s::EncoderPreset::Gif },
407+
]),
408+
});
409+
}));
410+
411+
match result {
412+
Ok(_) => println!("GIF with frame exceeding canvas handled safely (clipped)"),
413+
Err(e) => panic!("Panic on GIF with frame exceeding canvas: {:?}", e),
414+
}
415+
}
416+
417+
#[test]
418+
fn test_gif_overflow_frame_position() {
419+
let gif =
420+
fs::read(repo_root().join("imageflow_core/tests/crash_repro/gif_overflow_frame_pos.gif"))
421+
.expect("read fixture");
422+
let mut ctx = create_context();
423+
let _ = ctx.add_copied_input_buffer(0, &gif);
424+
425+
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
426+
let _ = ctx.add_output_buffer(1);
427+
let _ = ctx.build_1(s::Build001 {
428+
builder_config: None,
429+
io: vec![
430+
s::IoObject {
431+
direction: s::IoDirection::In,
432+
io_id: 0,
433+
io: s::IoEnum::Placeholder,
434+
},
435+
s::IoObject {
436+
direction: s::IoDirection::Out,
437+
io_id: 1,
438+
io: s::IoEnum::OutputBuffer,
439+
},
440+
],
441+
framewise: s::Framewise::Steps(vec![
442+
s::Node::Decode { io_id: 0, commands: None },
443+
s::Node::Encode { io_id: 1, preset: s::EncoderPreset::Gif },
444+
]),
445+
});
446+
}));
447+
448+
match result {
449+
Ok(_) => println!("GIF with overflow frame position handled safely"),
450+
Err(e) => panic!("Panic on GIF with overflow frame position: {:?}", e),
451+
}
452+
}
453+
454+
#[test]
455+
fn test_gif_zero_size_frame() {
456+
let gif =
457+
fs::read(repo_root().join("imageflow_core/tests/crash_repro/gif_zero_size_frame.gif"))
458+
.expect("read fixture");
459+
let mut ctx = create_context();
460+
let _ = ctx.add_copied_input_buffer(0, &gif);
461+
462+
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
463+
let _ = ctx.add_output_buffer(1);
464+
let _ = ctx.build_1(s::Build001 {
465+
builder_config: None,
466+
io: vec![
467+
s::IoObject {
468+
direction: s::IoDirection::In,
469+
io_id: 0,
470+
io: s::IoEnum::Placeholder,
471+
},
472+
s::IoObject {
473+
direction: s::IoDirection::Out,
474+
io_id: 1,
475+
io: s::IoEnum::OutputBuffer,
476+
},
477+
],
478+
framewise: s::Framewise::Steps(vec![
479+
s::Node::Decode { io_id: 0, commands: None },
480+
s::Node::Encode { io_id: 1, preset: s::EncoderPreset::Gif },
481+
]),
482+
});
483+
}));
484+
485+
match result {
486+
Ok(_) => println!("GIF with zero-size frame handled safely"),
487+
Err(e) => panic!("Panic on GIF with zero-size frame: {:?}", e),
488+
}
489+
}
490+
491+
// =============================================================================
492+
// GIF invalid background color index
493+
// =============================================================================
494+
495+
#[test]
496+
fn test_gif_bad_bg_index() {
497+
let gif = fs::read(repo_root().join("imageflow_core/tests/crash_repro/gif_bad_bg_index.gif"))
498+
.expect("read fixture");
499+
let mut ctx = create_context();
500+
let _ = ctx.add_copied_input_buffer(0, &gif);
501+
502+
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
503+
let _ = ctx.add_output_buffer(1);
504+
let _ = ctx.build_1(s::Build001 {
505+
builder_config: None,
506+
io: vec![
507+
s::IoObject {
508+
direction: s::IoDirection::In,
509+
io_id: 0,
510+
io: s::IoEnum::Placeholder,
511+
},
512+
s::IoObject {
513+
direction: s::IoDirection::Out,
514+
io_id: 1,
515+
io: s::IoEnum::OutputBuffer,
516+
},
517+
],
518+
framewise: s::Framewise::Steps(vec![
519+
s::Node::Decode { io_id: 0, commands: None },
520+
s::Node::Encode { io_id: 1, preset: s::EncoderPreset::Gif },
521+
]),
522+
});
523+
}));
524+
525+
match result {
526+
Ok(_) => println!("GIF with invalid bg_index handled safely"),
527+
Err(e) => panic!("Panic on GIF with invalid bg_index: {:?}", e),
528+
}
529+
}
530+
531+
// =============================================================================
532+
// GIF out-of-bounds palette index in pixel data
533+
// =============================================================================
534+
535+
#[test]
536+
fn test_gif_oob_palette_in_pixels() {
537+
let gif =
538+
fs::read(repo_root().join("imageflow_core/tests/crash_repro/gif_oob_palette_index.gif"))
539+
.expect("read fixture");
540+
let mut ctx = create_context();
541+
let _ = ctx.add_copied_input_buffer(0, &gif);
542+
543+
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
544+
let _ = ctx.add_output_buffer(1);
545+
let _ = ctx.build_1(s::Build001 {
546+
builder_config: None,
547+
io: vec![
548+
s::IoObject {
549+
direction: s::IoDirection::In,
550+
io_id: 0,
551+
io: s::IoEnum::Placeholder,
552+
},
553+
s::IoObject {
554+
direction: s::IoDirection::Out,
555+
io_id: 1,
556+
io: s::IoEnum::OutputBuffer,
557+
},
558+
],
559+
framewise: s::Framewise::Steps(vec![
560+
s::Node::Decode { io_id: 0, commands: None },
561+
s::Node::Encode { io_id: 1, preset: s::EncoderPreset::Gif },
562+
]),
563+
});
564+
}));
565+
566+
match result {
567+
Ok(_) => println!("GIF with OOB palette indices in pixel data handled safely"),
568+
Err(e) => panic!("Panic on GIF with OOB palette index: {:?}", e),
569+
}
570+
}
571+
572+
// =============================================================================
573+
// GIF frame buffer OOB (existing crash repro)
574+
// =============================================================================
575+
576+
#[test]
577+
fn test_gif_frame_buffer_oob() {
578+
let gif =
579+
fs::read(repo_root().join("imageflow_core/tests/crash_repro/gif_frame_buffer_oob.gif"))
580+
.expect("read fixture");
581+
let mut ctx = create_context();
582+
let _ = ctx.add_copied_input_buffer(0, &gif);
583+
584+
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
585+
let _ = ctx.add_output_buffer(1);
586+
let _ = ctx.build_1(s::Build001 {
587+
builder_config: None,
588+
io: vec![
589+
s::IoObject {
590+
direction: s::IoDirection::In,
591+
io_id: 0,
592+
io: s::IoEnum::Placeholder,
593+
},
594+
s::IoObject {
595+
direction: s::IoDirection::Out,
596+
io_id: 1,
597+
io: s::IoEnum::OutputBuffer,
598+
},
599+
],
600+
framewise: s::Framewise::Steps(vec![
601+
s::Node::Decode { io_id: 0, commands: None },
602+
s::Node::Encode { io_id: 1, preset: s::EncoderPreset::Gif },
603+
]),
604+
});
605+
}));
606+
607+
match result {
608+
Ok(_) => println!("GIF frame buffer OOB handled safely"),
609+
Err(e) => panic!("Panic on GIF frame buffer OOB: {:?}", e),
610+
}
611+
}
612+
376613
// =============================================================================
377614
// WebP with oversized RIFF claim
378615
// =============================================================================

0 commit comments

Comments
 (0)