Fix jpeg and png decode overflows and error handling. Our png and jpeg decode methods suffer from overflow bugs caused when multiplying 32 bit dimensions that overflow 32 bits of storage. We fix this in two ways: - bump factors to size_t when multiplying to ensure large valid files can be decoded - check for overflow when passing row pointers for decoding or copying to ensure we don't access out of bounds memory Also includes some cleanup using unique ptr in png decoding. Also includes two tests with a bad png and a half-bad jpeg (half-bad as it can actually decode but provides incorrect/manipulated dimensions to attempt to cause overflow). Issue reported here: https://github.com/rive-app/rive-cpp/issues/373 Diffs= 93fb6eb83 Fix jpeg and png decode overflows and error handling. (#7535) Co-authored-by: Luigi Rosso <luigi-rosso@users.noreply.github.com>
diff --git a/.rive_head b/.rive_head index 21b7f3d..172e250 100644 --- a/.rive_head +++ b/.rive_head
@@ -1 +1 @@ -1e7b1c0301a9fb9404798d5b4c0e7175d199659e +93fb6eb8365dba5c803d3132f7526dc1c888293e
diff --git a/decoders/src/decode_jpeg.cpp b/decoders/src/decode_jpeg.cpp index 0eab5c7..2ed8c9a 100644 --- a/decoders/src/decode_jpeg.cpp +++ b/decoders/src/decode_jpeg.cpp
@@ -77,10 +77,13 @@ assert(cinfo.data_precision == 8); assert(cinfo.output_components == 3); - pixelBuffer = std::make_unique<uint8_t[]>(cinfo.output_width * cinfo.output_height * - cinfo.output_components); + size_t pixelBufferSize = static_cast<size_t>(cinfo.output_width) * + static_cast<size_t>(cinfo.output_height) * + static_cast<size_t>(cinfo.output_components); + pixelBuffer = std::make_unique<uint8_t[]>(pixelBufferSize); uint8_t* pixelWriteBuffer = (uint8_t*)pixelBuffer.get(); + const uint8_t* pixelWriteBufferEnd = pixelWriteBuffer + pixelBufferSize; // Samples per row in output buffer row_stride = cinfo.output_width * cinfo.output_components; @@ -100,6 +103,13 @@ // more than one scanline at a time if that's more convenient. jpeg_read_scanlines(&cinfo, buffer, 1); + if (pixelWriteBuffer + row_stride > pixelWriteBufferEnd) + { + // memcpy would cause an overflow. + jpeg_finish_decompress(&cinfo); + jpeg_destroy_decompress(&cinfo); + return nullptr; + } memcpy(pixelWriteBuffer, buffer[0], row_stride); pixelWriteBuffer += row_stride; }
diff --git a/decoders/src/decode_png.cpp b/decoders/src/decode_png.cpp index edfb1bb..fcf441c 100644 --- a/decoders/src/decode_png.cpp +++ b/decoders/src/decode_png.cpp
@@ -7,6 +7,7 @@ #include <algorithm> #include <cassert> #include <string.h> +#include <setjmp.h> struct EncodedImageBuffer { @@ -40,6 +41,8 @@ png_infop info_ptr; png_uint_32 width, height; int bit_depth, color_type, interlace_type; + std::unique_ptr<uint8_t[]> pixelBuffer; + std::unique_ptr<png_bytep[]> rowsPointer; png_ptr = png_create_read_struct(PNG_LIBPNG_VER_STRING, nullptr, nullptr, nullptr); @@ -57,6 +60,12 @@ return nullptr; } + if (setjmp(png_jmpbuf(png_ptr))) + { + png_destroy_read_struct(&png_ptr, &info_ptr, NULL); + return nullptr; + } + EncodedImageBuffer stream = {bytes, 0, byteCount}; png_set_read_fn(png_ptr, &stream, ReadDataFromMemory); @@ -101,22 +110,30 @@ png_read_update_info(png_ptr, info_ptr); uint8_t channels = png_get_channels(png_ptr, info_ptr); - auto pixelBuffer = new uint8_t[width * height * channels]; + size_t pixelBufferSize = + static_cast<size_t>(width) * static_cast<size_t>(height) * static_cast<size_t>(channels); + pixelBuffer = std::make_unique<uint8_t[]>(pixelBufferSize); + const uint8_t* pixelBufferEnd = pixelBuffer.get() + pixelBufferSize; - png_bytep* row_pointers = new png_bytep[height]; - - for (unsigned row = 0; row < height; row++) + rowsPointer = std::make_unique<png_bytep[]>(height); + png_bytep* rows = rowsPointer.get(); + size_t rowStride = (size_t)width * (size_t)channels; + uint8_t* rowWrite = pixelBuffer.get(); + for (png_uint_32 row = 0; row < height; row++) { - unsigned int rIndex = row; - row_pointers[rIndex] = pixelBuffer + (row * (width * channels)); + rows[row] = rowWrite; + rowWrite += rowStride; + if (rowWrite > pixelBufferEnd) + { + // Read would overflow. + return nullptr; + } } - png_read_image(png_ptr, row_pointers); + png_read_image(png_ptr, rows); png_read_end(png_ptr, info_ptr); png_destroy_read_struct(&png_ptr, &info_ptr, (png_infopp) nullptr); - delete[] row_pointers; - Bitmap::PixelFormat pixelFormat; assert(channels == 3 || channels == 4); switch (channels) @@ -128,5 +145,5 @@ pixelFormat = Bitmap::PixelFormat::RGB; break; } - return std::make_unique<Bitmap>(width, height, pixelFormat, pixelBuffer); + return std::make_unique<Bitmap>(width, height, pixelFormat, std::move(pixelBuffer)); }
diff --git a/test/assets/bad.jpg b/test/assets/bad.jpg new file mode 100644 index 0000000..27a5d74 --- /dev/null +++ b/test/assets/bad.jpg Binary files differ
diff --git a/test/assets/bad.png b/test/assets/bad.png new file mode 100644 index 0000000..5392a7e --- /dev/null +++ b/test/assets/bad.png Binary files differ
diff --git a/test/image_decoders_test.cpp b/test/image_decoders_test.cpp index d9fe827..21132c2 100644 --- a/test/image_decoders_test.cpp +++ b/test/image_decoders_test.cpp
@@ -27,3 +27,26 @@ REQUIRE(bitmap->width() == 350); REQUIRE(bitmap->height() == 200); } + +TEST_CASE("bad jpeg file doesn't cause an overflow", "[image-decoder]") +{ + auto file = ReadFile("../../test/assets/bad.jpg"); + REQUIRE(file.size() == 88731); + + auto bitmap = Bitmap::decode(file.data(), file.size()); + + REQUIRE(bitmap != nullptr); + + REQUIRE(bitmap->width() == 24566); + REQUIRE(bitmap->height() == 58278); +} + +TEST_CASE("bad png file doesn't cause an overflow", "[image-decoder]") +{ + auto file = ReadFile("../../test/assets/bad.png"); + REQUIRE(file.size() == 534283); + + auto bitmap = Bitmap::decode(file.data(), file.size()); + + REQUIRE(bitmap == nullptr); +}
