Skip to content

Commit e643f99

Browse files
johnstiles-googlemibrunin
authored andcommitted
[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 <johnstiles@google.com> Reviewed-by: Peter Kasting <pkasting@chromium.org> Auto-Submit: John Stiles <johnstiles@google.com> Cr-Commit-Position: refs/heads/main@{#1254490} Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/551119 Reviewed-by: Michal Klocek <michal.klocek@qt.io>
1 parent c489ea4 commit e643f99

File tree

1 file changed

+13
-12
lines changed

1 file changed

+13
-12
lines changed

chromium/third_party/blink/renderer/platform/image-decoders/bmp/bmp_image_reader.cc

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -723,35 +723,36 @@ bool BMPImageReader::ProcessColorTable() {
723723

724724
const wtf_size_t header_end = header_offset_ + info_header_.size;
725725
wtf_size_t colors_in_palette = info_header_.clr_used;
726+
CHECK_LE(colors_in_palette, 256u); // Enforced by ProcessInfoHeader().
726727
wtf_size_t table_size_in_bytes = colors_in_palette * bytes_per_color;
727728
const wtf_size_t table_end = header_end + table_size_in_bytes;
728729
if (table_end < header_end)
729730
return parent_->SetFailed();
730731

731-
// Some BMPs don't contain a complete palette. Avoid reading off the end.
732+
// Some BMPs don't contain a complete palette. Truncate it instead of reading
733+
// off the end of the palette.
732734
if (img_data_offset_ && (img_data_offset_ < table_end)) {
733-
colors_in_palette = (img_data_offset_ - header_end) / bytes_per_color;
735+
wtf_size_t colors_in_truncated_palette =
736+
(img_data_offset_ - header_end) / bytes_per_color;
737+
CHECK_LE(colors_in_truncated_palette, colors_in_palette);
738+
colors_in_palette = colors_in_truncated_palette;
734739
table_size_in_bytes = colors_in_palette * bytes_per_color;
735740
}
736741

737-
// Read color table.
742+
// If we don't have enough data to read in the whole palette yet, stop here.
738743
if ((decoded_offset_ > data_->size()) ||
739744
((data_->size() - decoded_offset_) < table_size_in_bytes))
740745
return false;
741-
color_table_.resize(info_header_.clr_used);
746+
747+
// Read the color table.
748+
color_table_.resize(colors_in_palette);
742749

743750
for (wtf_size_t i = 0; i < colors_in_palette; ++i) {
744751
color_table_[i].rgb_blue = ReadUint8(0);
745752
color_table_[i].rgb_green = ReadUint8(1);
746753
color_table_[i].rgb_red = ReadUint8(2);
747754
decoded_offset_ += bytes_per_color;
748755
}
749-
// Explicitly zero any colors past the end of a truncated palette.
750-
for (wtf_size_t i = colors_in_palette; i < info_header_.clr_used; ++i) {
751-
color_table_[i].rgb_blue = 0;
752-
color_table_[i].rgb_green = 0;
753-
color_table_[i].rgb_red = 0;
754-
}
755756

756757
// We've now decoded all the non-image data we care about. Skip anything
757758
// else before the actual raster data.
@@ -927,7 +928,7 @@ BMPImageReader::ProcessingResult BMPImageReader::ProcessRLEData() {
927928
for (wtf_size_t which = 0; coord_.x() < end_x;) {
928929
// Some images specify color values past the end of the
929930
// color table; set these pixels to black.
930-
if (color_indexes[which] < info_header_.clr_used)
931+
if (color_indexes[which] < color_table_.size())
931932
SetI(color_indexes[which]);
932933
else
933934
SetRGBA(0, 0, 0, 255);
@@ -1001,7 +1002,7 @@ BMPImageReader::ProcessingResult BMPImageReader::ProcessNonRLEData(
10011002
}
10021003
} else {
10031004
// See comments near the end of ProcessRLEData().
1004-
if (color_index < info_header_.clr_used)
1005+
if (color_index < color_table_.size())
10051006
SetI(color_index);
10061007
else
10071008
SetRGBA(0, 0, 0, 255);

0 commit comments

Comments
 (0)