From fd9bcaa735c1bce62055f2b69084268fb960b3d2 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Fri, 6 Mar 2026 16:35:24 -0800 Subject: [PATCH] Fix heap buffer overflow in HDRDecoder RLE decoding (Issue #9748) (#9777) This commit addresses a critical security vulnerability (OOB write) and several stability issues in the Radiance HDR parser. Primary Fix: * Fixed a heap buffer overflow in the RLE decoding loop (Issue #9748). The decoder previously failed to verify if a run-length chunk exceeded the remaining space in the scanline buffer. Added strict bounds checking (`num_bytes + run_length > width`) before executing `memset` or `mStream.read` to prevent arbitrary memory corruption. Additional Security & Stability Improvements: * Prevented an infinite loop (DoS) in header parsing. Replaced the `do { ... } while(true);` loop with proper stream state checking (`while (mStream.getline(...))`) to handle unexpected EOFs gracefully. * Mitigated integer overflow and Out-Of-Memory (OOM) vulnerabilities by enforcing maximum sane dimensions (`MAX_IMAGE_DIMENSION` and `MAX_IMAGE_PIXELS`). This prevents catastrophic memory allocations triggered by maliciously crafted width/height values. * Initialized local variables and buffers (`buf`, `gamma`, `exposure`) to prevent undefined behavior and parsing of stack garbage upon stream read failures. Fixes #9748 --- libs/imageio/src/HDRDecoder.cpp | 65 +++++++++++++++++++++++++-------- 1 file changed, 50 insertions(+), 15 deletions(-) diff --git a/libs/imageio/src/HDRDecoder.cpp b/libs/imageio/src/HDRDecoder.cpp index 10091348fc..ba8ac93222 100644 --- a/libs/imageio/src/HDRDecoder.cpp +++ b/libs/imageio/src/HDRDecoder.cpp @@ -39,6 +39,10 @@ using namespace utils; namespace image { +// Define maximum sane image dimensions to prevent integer overflows and memory exhaustion (DoS). +constexpr size_t MAX_IMAGE_DIMENSION = 65536; +constexpr size_t MAX_IMAGE_PIXELS = 16384 * 16384; + const char HDRDecoder::sigRadiance[] = { '#', '?', 'R', 'A', 'D', 'I', 'A', 'N', 'C', 'E', 0xa }; const char HDRDecoder::sigRGBE[] = { '#', '?', 'R', 'G', 'B', 'E', 0xa }; @@ -59,16 +63,16 @@ HDRDecoder::HDRDecoder(std::istream& stream) HDRDecoder::~HDRDecoder() = default; LinearImage HDRDecoder::decode() { - float gamma; - float exposure; - char sy, sx; + float gamma = 1.0f; + float exposure = 1.0f; + char sy = 0, sx = 0; unsigned int height = 0, width = 0; { - char buf[1024]; - do { - char format[128]; - mStream.getline(buf, sizeof(buf), 0xa); + char buf[1024] = {}; // Initialize buffer to prevent reading stack garbage + // Check mStream status in the loop condition to prevent an infinite loop on EOF or read error + while (mStream.getline(buf, sizeof(buf), 0xa)) { if (buf[0] == '#') continue; + char format[128] = {0}; sscanf(buf, "FORMAT=%127s", format); // NOLINT sscanf(buf, "GAMMA=%f", &gamma); // NOLINT sscanf(buf, "EXPOSURE=%f", &exposure); // NOLINT @@ -76,14 +80,29 @@ LinearImage HDRDecoder::decode() { (sscanf(buf, "%cX %u %cY %u", &sx, &width, &sy, &height) == 4)) { // NOLINT break; } - } while (true); + } + + // Verify that dimensions were successfully found before proceeding + if (width == 0 || height == 0) { + slog.e << "Invalid or missing image dimensions" << io::endl; + return {}; + } } + + // Enforce sane limits on dimensions to prevent integer overflow and massive allocations + if (width > MAX_IMAGE_DIMENSION || height > MAX_IMAGE_DIMENSION || + (size_t(width) * size_t(height) > MAX_IMAGE_PIXELS)) { + slog.e << "Image dimensions exceed safety limits" << io::endl; + return {}; + } + LinearImage image(width, height, 3); if (sx == '-') image = (image); if (sy == '+') image = verticalFlip(image); // Allocate memory to hold one row of decoded pixel data. + // width is now validated, so width * 4 will not integer overflow. std::unique_ptr rgbe(new uint8_t[width * 4]); // First, test for non-RLE images. @@ -127,21 +146,37 @@ LinearImage HDRDecoder::decode() { size_t num_bytes = 0; while (num_bytes < width) { uint8_t rle_count; - mStream.read((char*) &rle_count, 1); + if (!mStream.read((char*) &rle_count, 1)) { + slog.e << "Unexpected EOF during RLE read" << io::endl; + return {}; + } + if (rle_count > 128) { + size_t run_length = rle_count - 128; + // Prevent Heap Buffer Overflow by checking bounds + if (num_bytes + run_length > width) { + slog.e << "RLE buffer overflow detected" << io::endl; + return {}; + } char v; mStream.read(&v, 1); - memset(d, v, size_t(rle_count - 128)); - d += rle_count - 128; - num_bytes += rle_count - 128; + memset(d, v, run_length); + d += run_length; + num_bytes += run_length; } else { if (rle_count == 0) { slog.e << "run length is zero" << io::endl; return {}; } - mStream.read(d, rle_count); - d += rle_count; - num_bytes += rle_count; + size_t run_length = rle_count; + // Prevent Heap Buffer Overflow by checking bounds + if (num_bytes + run_length > width) { + slog.e << "RLE buffer overflow detected" << io::endl; + return {}; + } + mStream.read(d, run_length); + d += run_length; + num_bytes += run_length; } } }