From 46944c3a87ebc6c5d9a9a4962a94569ba1082bc3 Mon Sep 17 00:00:00 2001 From: Kimball Thurston Date: Mon, 5 Feb 2024 08:44:21 +1300 Subject: [PATCH] Fix CVE 2023 5841 (#1627) * enable deep file checks for core Signed-off-by: Kimball Thurston * fix possible int overflow Signed-off-by: Kimball Thurston * fix validation of deep sample counts Addresses CVE-2023-5841, fixing sample count check to not only check against 0 but previous sample as well. Signed-off-by: Kimball Thurston * add clarifying comment Signed-off-by: Kimball Thurston --------- Signed-off-by: Kimball Thurston --- src/lib/OpenEXRCore/decoding.c | 37 +++--- src/lib/OpenEXRCore/unpack.c | 9 +- src/lib/OpenEXRUtil/ImfCheckFile.cpp | 180 ++++++++++++++++++++++----- 3 files changed, 175 insertions(+), 51 deletions(-) diff --git a/src/lib/OpenEXRCore/decoding.c b/src/lib/OpenEXRCore/decoding.c index 322cbd8965..710d8a6db8 100644 --- a/src/lib/OpenEXRCore/decoding.c +++ b/src/lib/OpenEXRCore/decoding.c @@ -288,6 +288,9 @@ default_decompress_chunk (exr_decode_pipeline_t* decode) uint64_t sampsize = (((uint64_t) decode->chunk.width) * ((uint64_t) decode->chunk.height)); + + if ((decode->decode_flags & EXR_DECODE_SAMPLE_COUNTS_AS_INDIVIDUAL)) + sampsize += 1; sampsize *= sizeof (int32_t); rv = decompress_data ( @@ -340,7 +343,7 @@ unpack_sample_table ( exr_result_t rv = EXR_ERR_SUCCESS; int32_t w = decode->chunk.width; int32_t h = decode->chunk.height; - int32_t totsamp = 0; + uint64_t totsamp = 0; int32_t* samptable = decode->sample_count_table; size_t combSampSize = 0; @@ -351,38 +354,44 @@ unpack_sample_table ( { for (int32_t y = 0; y < h; ++y) { + int32_t *cursampline = samptable + y * w; int32_t prevsamp = 0; for (int32_t x = 0; x < w; ++x) { int32_t nsamps = - (int32_t) one_to_native32 ((uint32_t) samptable[y * w + x]); - if (nsamps < 0) return EXR_ERR_INVALID_SAMPLE_DATA; - samptable[y * w + x] = nsamps - prevsamp; - prevsamp = nsamps; + (int32_t) one_to_native32 ((uint32_t) cursampline[x]); + if (nsamps < prevsamp) return EXR_ERR_INVALID_SAMPLE_DATA; + + cursampline[x] = nsamps - prevsamp; + prevsamp = nsamps; } - totsamp += prevsamp; + totsamp += (uint64_t)prevsamp; } - samptable[w * h] = totsamp; + if (totsamp >= (uint64_t)INT32_MAX) + return EXR_ERR_INVALID_SAMPLE_DATA; + samptable[w * h] = (int32_t)totsamp; } else { for (int32_t y = 0; y < h; ++y) { + int32_t *cursampline = samptable + y * w; int32_t prevsamp = 0; for (int32_t x = 0; x < w; ++x) { int32_t nsamps = - (int32_t) one_to_native32 ((uint32_t) samptable[y * w + x]); - if (nsamps < 0) return EXR_ERR_INVALID_SAMPLE_DATA; - samptable[y * w + x] = nsamps; - prevsamp = nsamps; + (int32_t) one_to_native32 ((uint32_t) cursampline[x]); + if (nsamps < prevsamp) return EXR_ERR_INVALID_SAMPLE_DATA; + + cursampline[x] = nsamps; + prevsamp = nsamps; } - totsamp += prevsamp; + + totsamp += (uint64_t)prevsamp; } } - if (totsamp < 0 || - (((uint64_t) totsamp) * combSampSize) > decode->chunk.unpacked_size) + if ((totsamp * combSampSize) > decode->chunk.unpacked_size) { rv = pctxt->report_error ( pctxt, EXR_ERR_INVALID_SAMPLE_DATA, "Corrupt sample count table"); diff --git a/src/lib/OpenEXRCore/unpack.c b/src/lib/OpenEXRCore/unpack.c index b88c98974c..1324508c5b 100644 --- a/src/lib/OpenEXRCore/unpack.c +++ b/src/lib/OpenEXRCore/unpack.c @@ -1196,9 +1196,10 @@ generic_unpack_deep_pointers (exr_decode_pipeline_t* decode) if (outpix) { uint8_t* cdata = outpix; + UNPACK_SAMPLES (samps) } - srcbuffer += bpc * samps; + srcbuffer += ((size_t) bpc) * ((size_t) samps); } } sampbuffer += w; @@ -1242,12 +1243,14 @@ generic_unpack_deep (exr_decode_pipeline_t* decode) } else prevsamps = sampbuffer[w - 1]; + srcbuffer += ((size_t) bpc) * ((size_t) prevsamps); if (incr_tot) totsamps += (size_t) prevsamps; continue; } + cdata += totsamps * ((size_t) ubpc); for (int x = 0; x < w; ++x) @@ -1263,7 +1266,7 @@ generic_unpack_deep (exr_decode_pipeline_t* decode) UNPACK_SAMPLES (samps) - srcbuffer += bpc * samps; + srcbuffer += ((size_t) bpc) * ((size_t) samps); if (incr_tot) totsamps += (size_t) samps; } } @@ -1301,7 +1304,7 @@ internal_exr_match_decode ( if (isdeep) { - if ((decode->decode_flags & EXR_DECODE_SAMPLE_COUNTS_AS_INDIVIDUAL)) + if ((decode->decode_flags & EXR_DECODE_NON_IMAGE_DATA_AS_POINTERS)) return &generic_unpack_deep_pointers; return &generic_unpack_deep; } diff --git a/src/lib/OpenEXRUtil/ImfCheckFile.cpp b/src/lib/OpenEXRUtil/ImfCheckFile.cpp index 82f318a92f..4d45dad69f 100644 --- a/src/lib/OpenEXRUtil/ImfCheckFile.cpp +++ b/src/lib/OpenEXRUtil/ImfCheckFile.cpp @@ -1200,13 +1200,88 @@ runChecks (T& source, bool reduceMemory, bool reduceTime) return threw; } +// This is not entirely needed in that the chunk info has the +// total unpacked_size field which can be used for allocation +// but this adds an additional point to use when debugging issues. +static exr_result_t +realloc_deepdata(exr_decode_pipeline_t* decode) +{ + int32_t w = decode->chunk.width; + int32_t h = decode->chunk.height; + uint64_t totsamps = 0, bytes = 0; + const int32_t *sampbuffer = decode->sample_count_table; + std::vector* ud = static_cast*>( + decode->decoding_user_data); + + if ( ! ud ) + { + for (int c = 0; c < decode->channel_count; c++) + { + exr_coding_channel_info_t& outc = decode->channels[c]; + outc.decode_to_ptr = NULL; + outc.user_pixel_stride = outc.user_bytes_per_element; + outc.user_line_stride = 0; + } + return EXR_ERR_SUCCESS; + } + + if ((decode->decode_flags & + EXR_DECODE_SAMPLE_COUNTS_AS_INDIVIDUAL)) + { + for (int32_t y = 0; y < h; ++y) + { + for (int x = 0; x < w; ++x) + totsamps += sampbuffer[x]; + sampbuffer += w; + } + } + else + { + for (int32_t y = 0; y < h; ++y) + totsamps += sampbuffer[y*w + w - 1]; + } + + for (int c = 0; c < decode->channel_count; c++) + { + exr_coding_channel_info_t& outc = decode->channels[c]; + bytes += totsamps * outc.user_bytes_per_element; + } + + if (bytes >= gMaxBytesPerDeepScanline * h) + { + for (int c = 0; c < decode->channel_count; c++) + { + exr_coding_channel_info_t& outc = decode->channels[c]; + outc.decode_to_ptr = NULL; + outc.user_pixel_stride = outc.user_bytes_per_element; + outc.user_line_stride = 0; + } + return EXR_ERR_SUCCESS; + } + + if (ud->size () < bytes) + ud->resize (bytes); + + uint8_t* dptr = &((*ud)[0]); + for (int c = 0; c < decode->channel_count; c++) + { + exr_coding_channel_info_t& outc = decode->channels[c]; + outc.decode_to_ptr = dptr; + outc.user_pixel_stride = outc.user_bytes_per_element; + outc.user_line_stride = 0; + + dptr += totsamps * (uint64_t) outc.user_bytes_per_element; + } + return EXR_ERR_SUCCESS; +} + //////////////////////////////////////// bool readCoreScanlinePart ( exr_context_t f, int part, bool reduceMemory, bool reduceTime) { - exr_result_t rv; + exr_result_t rv, frv; exr_attr_box2i_t datawin; rv = exr_get_data_window (f, part, &datawin); if (rv != EXR_ERR_SUCCESS) return true; @@ -1224,6 +1299,8 @@ readCoreScanlinePart ( rv = exr_get_scanlines_per_chunk (f, part, &lines_per_chunk); if (rv != EXR_ERR_SUCCESS) return true; + frv = rv; + for (uint64_t chunk = 0; chunk < height; chunk += lines_per_chunk) { exr_chunk_info_t cinfo = {0}; @@ -1232,6 +1309,7 @@ readCoreScanlinePart ( rv = exr_read_scanline_chunk_info (f, part, y, &cinfo); if (rv != EXR_ERR_SUCCESS) { + frv = rv; if (reduceTime) break; continue; } @@ -1253,19 +1331,32 @@ readCoreScanlinePart ( (uint64_t) lines_per_chunk; } - // TODO: check we are supposed to multiple by lines per chunk above doread = true; - if (reduceMemory && bytes >= gMaxBytesPerScanline) doread = false; - if (doread) imgdata.resize (bytes); + if (cinfo.type == EXR_STORAGE_DEEP_SCANLINE) + { + decoder.decoding_user_data = &imgdata; + decoder.realloc_nonimage_data_fn = &realloc_deepdata; + } + else + { + if (reduceMemory && bytes >= gMaxBytesPerScanline) doread = false; + + if (doread) imgdata.resize (bytes); + } rv = exr_decoding_choose_default_routines (f, part, &decoder); - if (rv != EXR_ERR_SUCCESS) break; + if (rv != EXR_ERR_SUCCESS) + { + frv = rv; + break; + } } else { rv = exr_decoding_update (f, part, &cinfo, &decoder); if (rv != EXR_ERR_SUCCESS) { + frv = rv; if (reduceTime) break; continue; } @@ -1273,20 +1364,25 @@ readCoreScanlinePart ( if (doread) { - uint8_t* dptr = &(imgdata[0]); - for (int c = 0; c < decoder.channel_count; c++) + if (cinfo.type != EXR_STORAGE_DEEP_SCANLINE) { - exr_coding_channel_info_t& outc = decoder.channels[c]; - outc.decode_to_ptr = dptr; - outc.user_pixel_stride = outc.user_bytes_per_element; - outc.user_line_stride = outc.user_pixel_stride * width; - dptr += width * (uint64_t) outc.user_bytes_per_element * + uint8_t* dptr = &(imgdata[0]); + for (int c = 0; c < decoder.channel_count; c++) + { + exr_coding_channel_info_t& outc = decoder.channels[c]; + outc.decode_to_ptr = dptr; + outc.user_pixel_stride = outc.user_bytes_per_element; + outc.user_line_stride = outc.user_pixel_stride * width; + + dptr += width * (uint64_t) outc.user_bytes_per_element * (uint64_t) lines_per_chunk; + } } rv = exr_decoding_run (f, part, &decoder); if (rv != EXR_ERR_SUCCESS) { + frv = rv; if (reduceTime) break; } } @@ -1294,7 +1390,7 @@ readCoreScanlinePart ( exr_decoding_destroy (f, &decoder); - return (rv != EXR_ERR_SUCCESS); + return (frv != EXR_ERR_SUCCESS); } //////////////////////////////////////// @@ -1303,7 +1399,7 @@ bool readCoreTiledPart ( exr_context_t f, int part, bool reduceMemory, bool reduceTime) { - exr_result_t rv; + exr_result_t rv, frv; exr_attr_box2i_t datawin; rv = exr_get_data_window (f, part, &datawin); @@ -1321,6 +1417,7 @@ readCoreTiledPart ( rv = exr_get_tile_levels (f, part, &levelsx, &levelsy); if (rv != EXR_ERR_SUCCESS) return true; + frv = rv; bool keepgoing = true; for (int32_t ylevel = 0; keepgoing && ylevel < levelsy; ++ylevel) { @@ -1330,6 +1427,7 @@ readCoreTiledPart ( rv = exr_get_level_sizes (f, part, xlevel, ylevel, &levw, &levh); if (rv != EXR_ERR_SUCCESS) { + frv = rv; if (reduceTime) { keepgoing = false; @@ -1342,6 +1440,7 @@ readCoreTiledPart ( rv = exr_get_tile_sizes (f, part, xlevel, ylevel, &curtw, &curth); if (rv != EXR_ERR_SUCCESS) { + frv = rv; if (reduceTime) { keepgoing = false; @@ -1371,6 +1470,7 @@ readCoreTiledPart ( f, part, tx, ty, xlevel, ylevel, &cinfo); if (rv != EXR_ERR_SUCCESS) { + frv = rv; if (reduceTime) { keepgoing = false; @@ -1385,6 +1485,7 @@ readCoreTiledPart ( exr_decoding_initialize (f, part, &cinfo, &decoder); if (rv != EXR_ERR_SUCCESS) { + frv = rv; keepgoing = false; break; } @@ -1406,14 +1507,23 @@ readCoreTiledPart ( } doread = true; - if (reduceMemory && bytes >= gMaxTileBytes) - doread = false; + if (cinfo.type == EXR_STORAGE_DEEP_TILED) + { + decoder.decoding_user_data = &tiledata; + decoder.realloc_nonimage_data_fn = &realloc_deepdata; + } + else + { + if (reduceMemory && bytes >= gMaxTileBytes) + doread = false; - if (doread) tiledata.resize (bytes); + if (doread) tiledata.resize (bytes); + } rv = exr_decoding_choose_default_routines ( f, part, &decoder); if (rv != EXR_ERR_SUCCESS) { + frv = rv; keepgoing = false; break; } @@ -1423,6 +1533,7 @@ readCoreTiledPart ( rv = exr_decoding_update (f, part, &cinfo, &decoder); if (rv != EXR_ERR_SUCCESS) { + frv = rv; if (reduceTime) { keepgoing = false; @@ -1434,24 +1545,28 @@ readCoreTiledPart ( if (doread) { - uint8_t* dptr = &(tiledata[0]); - for (int c = 0; c < decoder.channel_count; c++) + if (cinfo.type != EXR_STORAGE_DEEP_TILED) { - exr_coding_channel_info_t& outc = - decoder.channels[c]; - outc.decode_to_ptr = dptr; - outc.user_pixel_stride = - outc.user_bytes_per_element; - outc.user_line_stride = - outc.user_pixel_stride * curtw; - dptr += (uint64_t) curtw * + uint8_t* dptr = &(tiledata[0]); + for (int c = 0; c < decoder.channel_count; c++) + { + exr_coding_channel_info_t& outc = + decoder.channels[c]; + outc.decode_to_ptr = dptr; + outc.user_pixel_stride = + outc.user_bytes_per_element; + outc.user_line_stride = + outc.user_pixel_stride * curtw; + dptr += (uint64_t) curtw * (uint64_t) outc.user_bytes_per_element * (uint64_t) curth; + } } rv = exr_decoding_run (f, part, &decoder); if (rv != EXR_ERR_SUCCESS) { + frv = rv; if (reduceTime) { keepgoing = false; @@ -1486,17 +1601,14 @@ checkCoreFile (exr_context_t f, bool reduceMemory, bool reduceTime) rv = exr_get_storage (f, p, &store); if (rv != EXR_ERR_SUCCESS) return true; - // TODO: Need to fill this in - if (store == EXR_STORAGE_DEEP_SCANLINE || - store == EXR_STORAGE_DEEP_TILED) - continue; - - if (store == EXR_STORAGE_SCANLINE) + if (store == EXR_STORAGE_SCANLINE || + store == EXR_STORAGE_DEEP_SCANLINE) { if (readCoreScanlinePart (f, p, reduceMemory, reduceTime)) return true; } - else if (store == EXR_STORAGE_TILED) + else if (store == EXR_STORAGE_TILED || + store == EXR_STORAGE_DEEP_TILED) { if (readCoreTiledPart (f, p, reduceMemory, reduceTime)) return true; }