From e643f99ed059521651f3b72be0885ddb2eb76dea Mon Sep 17 00:00:00 2001 From: John Stiles Date: Wed, 31 Jan 2024 14:28:47 +0000 Subject: [PATCH] [Backport] Security bug 41495984 Manual cherry-pick of patch originally reviewed on https://chromium-review.googlesource.com/c/chromium/src/+/5249171: Improve handling of malformed BMP palettes. Add CHECKs to guarantee that clr_used is reasonably sized when ProcessColorTable() is called. Out-of-bounds values are capped by ProcessInfoHeader() already, but since this happens at a distance, it's better to be sure. Additionally, we would previously add padding elements to a palette if it was shorter than expected. We already had bounds checks at the places where the palette was accessed, so we now rely on those checks instead. Bug: 1523030 Change-Id: I579c67d1029e1effba2036e9ec0c871418b140e2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5249171 Commit-Queue: John Stiles Reviewed-by: Peter Kasting Auto-Submit: John Stiles Cr-Commit-Position: refs/heads/main@{#1254490} Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/551119 Reviewed-by: Michal Klocek --- .../image-decoders/bmp/bmp_image_reader.cc | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/chromium/third_party/blink/renderer/platform/image-decoders/bmp/bmp_image_reader.cc b/chromium/third_party/blink/renderer/platform/image-decoders/bmp/bmp_image_reader.cc index b40c8aa5c1f..ee50d3a882d 100644 --- a/chromium/third_party/blink/renderer/platform/image-decoders/bmp/bmp_image_reader.cc +++ b/chromium/third_party/blink/renderer/platform/image-decoders/bmp/bmp_image_reader.cc @@ -723,22 +723,29 @@ bool BMPImageReader::ProcessColorTable() { const wtf_size_t header_end = header_offset_ + info_header_.size; wtf_size_t colors_in_palette = info_header_.clr_used; + CHECK_LE(colors_in_palette, 256u); // Enforced by ProcessInfoHeader(). wtf_size_t table_size_in_bytes = colors_in_palette * bytes_per_color; const wtf_size_t table_end = header_end + table_size_in_bytes; if (table_end < header_end) return parent_->SetFailed(); - // Some BMPs don't contain a complete palette. Avoid reading off the end. + // Some BMPs don't contain a complete palette. Truncate it instead of reading + // off the end of the palette. if (img_data_offset_ && (img_data_offset_ < table_end)) { - colors_in_palette = (img_data_offset_ - header_end) / bytes_per_color; + wtf_size_t colors_in_truncated_palette = + (img_data_offset_ - header_end) / bytes_per_color; + CHECK_LE(colors_in_truncated_palette, colors_in_palette); + colors_in_palette = colors_in_truncated_palette; table_size_in_bytes = colors_in_palette * bytes_per_color; } - // Read color table. + // If we don't have enough data to read in the whole palette yet, stop here. if ((decoded_offset_ > data_->size()) || ((data_->size() - decoded_offset_) < table_size_in_bytes)) return false; - color_table_.resize(info_header_.clr_used); + + // Read the color table. + color_table_.resize(colors_in_palette); for (wtf_size_t i = 0; i < colors_in_palette; ++i) { color_table_[i].rgb_blue = ReadUint8(0); @@ -746,12 +753,6 @@ bool BMPImageReader::ProcessColorTable() { color_table_[i].rgb_red = ReadUint8(2); decoded_offset_ += bytes_per_color; } - // Explicitly zero any colors past the end of a truncated palette. - for (wtf_size_t i = colors_in_palette; i < info_header_.clr_used; ++i) { - color_table_[i].rgb_blue = 0; - color_table_[i].rgb_green = 0; - color_table_[i].rgb_red = 0; - } // We've now decoded all the non-image data we care about. Skip anything // else before the actual raster data. @@ -927,7 +928,7 @@ BMPImageReader::ProcessingResult BMPImageReader::ProcessRLEData() { for (wtf_size_t which = 0; coord_.x() < end_x;) { // Some images specify color values past the end of the // color table; set these pixels to black. - if (color_indexes[which] < info_header_.clr_used) + if (color_indexes[which] < color_table_.size()) SetI(color_indexes[which]); else SetRGBA(0, 0, 0, 255); @@ -1001,7 +1002,7 @@ BMPImageReader::ProcessingResult BMPImageReader::ProcessNonRLEData( } } else { // See comments near the end of ProcessRLEData(). - if (color_index < info_header_.clr_used) + if (color_index < color_table_.size()) SetI(color_index); else SetRGBA(0, 0, 0, 255);