Merge pull request #2605 from image-rs/bmp-encoder-careful-arithmetic

Harden bmp encoder arithmetic against overflows

Author: 197g

src/codecs/bmp/encoder.rs

@@ -75,18 +75,32 @@ impl<'a, W: Write + 'a> BmpEncoder<'a, W> {
         let bmp_header_size = BITMAPFILEHEADER_SIZE;
 
         let (dib_header_size, written_pixel_size, palette_color_count) =
-            get_pixel_info(color_type, palette)?;
-        let row_pad_size = (4 - (width * written_pixel_size) % 4) % 4; // each row must be padded to a multiple of 4 bytes
-        let image_size = width
-            .checked_mul(height)
-            .and_then(|v| v.checked_mul(written_pixel_size))
-            .and_then(|v| v.checked_add(height * row_pad_size))
+            written_pixel_info(color_type, palette)?;
+
+        let (padded_row, image_size) = width
+            .checked_mul(written_pixel_size)
+            // each row must be padded to a multiple of 4 bytes
+            .and_then(|v| v.checked_next_multiple_of(4))
+            .and_then(|v| {
+                let image_bytes = v.checked_mul(height)?;
+                Some((v, image_bytes))
+            })
             .ok_or_else(|| {
                 ImageError::Parameter(ParameterError::from_kind(
                     ParameterErrorKind::DimensionMismatch,
                 ))
             })?;
-        let palette_size = palette_color_count * 4; // all palette colors are BGRA
+
+        let row_padding = padded_row - width * written_pixel_size;
+
+        // all palette colors are BGRA
+        let palette_size = palette_color_count.checked_mul(4).ok_or_else(|| {
+            ImageError::Encoding(EncodingError::new(
+                ImageFormatHint::Exact(ImageFormat::Bmp),
+                "calculated palette size larger than 2^32",
+            ))
+        })?;
+
         let file_size = bmp_header_size
             .checked_add(dib_header_size)
             .and_then(|v| v.checked_add(palette_size))
@@ -98,14 +112,23 @@ impl<'a, W: Write + 'a> BmpEncoder<'a, W> {
                 ))
             })?;
 
+        let image_data_offset = bmp_header_size
+            .checked_add(dib_header_size)
+            .and_then(|v| v.checked_add(palette_size))
+            .ok_or_else(|| {
+                ImageError::Encoding(EncodingError::new(
+                    ImageFormatHint::Exact(ImageFormat::Bmp),
+                    "calculated BMP size larger than 2^32",
+                ))
+            })?;
+
         // write BMP header
         self.writer.write_u8(b'B')?;
         self.writer.write_u8(b'M')?;
         self.writer.write_u32::<LittleEndian>(file_size)?; // file size
         self.writer.write_u16::<LittleEndian>(0)?; // reserved 1
         self.writer.write_u16::<LittleEndian>(0)?; // reserved 2
-        self.writer
-            .write_u32::<LittleEndian>(bmp_header_size + dib_header_size + palette_size)?; // image data offset
+        self.writer.write_u32::<LittleEndian>(image_data_offset)?; // image data offset
 
         // write DIB header
         self.writer.write_u32::<LittleEndian>(dib_header_size)?;
@@ -141,13 +164,13 @@ impl<'a, W: Write + 'a> BmpEncoder<'a, W> {
 
         // write image data
         match color_type {
-            ExtendedColorType::Rgb8 => self.encode_rgb(image, width, height, row_pad_size, 3)?,
-            ExtendedColorType::Rgba8 => self.encode_rgba(image, width, height, row_pad_size, 4)?,
+            ExtendedColorType::Rgb8 => self.encode_rgb(image, width, height, row_padding, 3)?,
+            ExtendedColorType::Rgba8 => self.encode_rgba(image, width, height, row_padding, 4)?,
             ExtendedColorType::L8 => {
-                self.encode_gray(image, width, height, row_pad_size, 1, palette)?;
+                self.encode_gray(image, width, height, row_padding, 1, palette)?;
             }
             ExtendedColorType::La8 => {
-                self.encode_gray(image, width, height, row_pad_size, 2, palette)?;
+                self.encode_gray(image, width, height, row_padding, 2, palette)?;
             }
             _ => {
                 return Err(ImageError::Unsupported(
@@ -167,7 +190,7 @@ impl<'a, W: Write + 'a> BmpEncoder<'a, W> {
         image: &[u8],
         width: u32,
         height: u32,
-        row_pad_size: u32,
+        row_padding: u32,
         bytes_per_pixel: u32,
     ) -> io::Result<()> {
         let width = width as usize;
@@ -184,7 +207,7 @@ impl<'a, W: Write + 'a> BmpEncoder<'a, W> {
                 // written as BGR
                 self.writer.write_all(&[b, g, r])?;
             }
-            self.write_row_pad(row_pad_size)?;
+            self.write_row_pad(row_padding)?;
         }
 
         Ok(())
@@ -195,7 +218,7 @@ impl<'a, W: Write + 'a> BmpEncoder<'a, W> {
         image: &[u8],
         width: u32,
         height: u32,
-        row_pad_size: u32,
+        row_padding: u32,
         bytes_per_pixel: u32,
     ) -> io::Result<()> {
         let width = width as usize;
@@ -213,7 +236,7 @@ impl<'a, W: Write + 'a> BmpEncoder<'a, W> {
                 // written as BGRA
                 self.writer.write_all(&[b, g, r, a])?;
             }
-            self.write_row_pad(row_pad_size)?;
+            self.write_row_pad(row_padding)?;
         }
 
         Ok(())
@@ -224,7 +247,7 @@ impl<'a, W: Write + 'a> BmpEncoder<'a, W> {
         image: &[u8],
         width: u32,
         height: u32,
-        row_pad_size: u32,
+        row_padding: u32,
         bytes_per_pixel: u32,
         palette: Option<&[[u8; 3]]>,
     ) -> io::Result<()> {
@@ -261,7 +284,7 @@ impl<'a, W: Write + 'a> BmpEncoder<'a, W> {
                 }
             }
 
-            self.write_row_pad(row_pad_size)?;
+            self.write_row_pad(row_padding)?;
         }
 
         Ok(())
@@ -297,37 +320,42 @@ impl<W: Write> ImageEncoder for BmpEncoder<'_, W> {
     }
 }
 
-fn get_unsupported_error_message(c: ExtendedColorType) -> String {
-    format!("Unsupported color type {c:?}.  Supported types: RGB(8), RGBA(8), Gray(8), GrayA(8).")
-}
-
 /// Returns a tuple representing: (dib header size, written pixel size, palette color count).
-fn get_pixel_info(
+fn written_pixel_info(
     c: ExtendedColorType,
     palette: Option<&[[u8; 3]]>,
-) -> io::Result<(u32, u32, u32)> {
-    let sizes = match c {
-        ExtendedColorType::Rgb8 => (BITMAPINFOHEADER_SIZE, 3, 0),
-        ExtendedColorType::Rgba8 => (BITMAPV4HEADER_SIZE, 4, 0),
+) -> Result<(u32, u32, u32), ImageError> {
+    let (header, color_bytes, palette_count) = match c {
+        ExtendedColorType::Rgb8 => (BITMAPINFOHEADER_SIZE, 3, Some(0)),
+        ExtendedColorType::Rgba8 => (BITMAPV4HEADER_SIZE, 4, Some(0)),
         ExtendedColorType::L8 => (
             BITMAPINFOHEADER_SIZE,
             1,
-            palette.map(|p| p.len()).unwrap_or(256) as u32,
+            u32::try_from(palette.map(|p| p.len()).unwrap_or(256)).ok(),
         ),
         ExtendedColorType::La8 => (
             BITMAPINFOHEADER_SIZE,
             1,
-            palette.map(|p| p.len()).unwrap_or(256) as u32,
+            u32::try_from(palette.map(|p| p.len()).unwrap_or(256)).ok(),
         ),
         _ => {
-            return Err(io::Error::new(
-                io::ErrorKind::InvalidInput,
-                &get_unsupported_error_message(c)[..],
-            ))
+            return Err(ImageError::Unsupported(
+                UnsupportedError::from_format_and_kind(
+                    ImageFormat::Bmp.into(),
+                    UnsupportedErrorKind::Color(c),
+                ),
+            ));
         }
     };
 
-    Ok(sizes)
+    let palette_count = palette_count.ok_or_else(|| {
+        ImageError::Encoding(EncodingError::new(
+            ImageFormatHint::Exact(ImageFormat::Bmp),
+            "calculated palette size larger than 2^32",
+        ))
+    })?;
+
+    Ok((header, color_bytes, palette_count))
 }
 
 #[cfg(test)]
@@ -421,4 +449,13 @@ mod tests {
         assert_eq!(2, decoded[7]);
         assert_eq!(2, decoded[8]);
     }
+
+    #[test]
+    fn regression_issue_2604() {
+        let mut image = vec![];
+        let mut encoder = BmpEncoder::new(&mut image);
+        encoder
+            .encode(&[], 1 << 31, 0, ExtendedColorType::Rgb8)
+            .unwrap_err();
+    }
 }

src/codecs/dxt.rs

@@ -196,6 +196,7 @@ fn alpha_table_dxt5(alpha0: u8, alpha1: u8) -> [u8; 8] {
 
 /// decodes an 8-byte dxt color block into the RGB channels of a 16xRGB or 16xRGBA block.
 /// source should have a length of 8, dest a length of 48 (RGB) or 64 (RGBA)
+#[allow(clippy::needless_range_loop)] // False positive, the 0..3 loop is not an enumerate
 fn decode_dxt_colors(source: &[u8], dest: &mut [u8], is_dxt1: bool) {
     // sanity checks, also enable the compiler to elide all following bound checks
     assert!(source.len() == 8 && (dest.len() == 48 || dest.len() == 64));