From 35b3e596edacd2437c2c5d3dd2b5c9502626163d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20=C4=8Cerm=C3=A1k?= Date: Fri, 17 Aug 2018 16:41:05 +0200 Subject: [PATCH] Add overflow & overread checks to PngChunk::parseTXTChunk() This function was creating a lot of new pointers and strings without properly checking the array bounds. This commit adds several calls to enforce(), making sure that the pointers stay within bounds. Strings are now created using the helper function string_from_unterminated() to prevent overreads in the constructor of std::string. This fixes #400 --- src/pngchunk_int.cpp | 63 ++++++++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/src/pngchunk.cpp b/src/pngchunk.cpp index e087e47cb..f13594bd8 100644 --- a/src/pngchunk.cpp +++ b/src/pngchunk.cpp @@ -34,6 +34,7 @@ #include "image.hpp" #include "error.hpp" #include "enforce.hpp" +#include "safe_op.hpp" // + standard includes #include @@ -55,6 +54,13 @@ EXIV2_RCSID("@(#) $Id: pngchunk.cpp 3777 2015-05-02 11:55:40Z ahuggel $") #include // To uncompress or compress text chunk +/* Added as support of CVE-2018-16336 */ +std::string string_from_unterminated(const char* data, size_t data_length) +{ + const size_t StringLength = strnlen(data, data_length); + + return std::string(data, StringLength); +} /* URLs to find informations about PNG chunks : @@ -133,6 +135,8 @@ namespace Exiv2 { if(type == zTXt_Chunk) { + enforce(data.size_ >= Safe::add(keysize, 2), Exiv2::kerCorruptedMetadata); + // Extract a deflate compressed Latin-1 text chunk // we get the compression method after the key @@ -149,11 +153,13 @@ namespace Exiv2 { // compressed string after the compression technique spec const byte* compressedText = data.pData_ + keysize + 2; unsigned int compressedTextSize = data.size_ - keysize - 2; + enforce(compressedTextSize < data.size_, kerCorruptedMetadata); zlibUncompress(compressedText, compressedTextSize, arr); } else if(type == tEXt_Chunk) { + enforce(data.size_ >= Safe::add(keysize, 1), Exiv2::kerCorruptedMetadata); // Extract a non-compressed Latin-1 text chunk // the text comes after the key, but isn't null terminated @@ -164,7 +170,8 @@ namespace Exiv2 { } else if(type == iTXt_Chunk) { + enforce(data.size_ >= Safe::add(keysize, 3), Exiv2::kerCorruptedMetadata); const int nullSeparators = std::count(&data.pData_[keysize+3], &data.pData_[data.size_], '\0'); enforce(nullSeparators >= 2, Exiv2::kerCorruptedMetadata); @@ -178,40 +185,44 @@ namespace Exiv2 { enforce(compressionMethod == 0x00, Exiv2::kerCorruptedMetadata); // language description string after the compression technique spec - std::string languageText((const char*)(data.pData_ + keysize + 3)); - unsigned int languageTextSize = static_cast(languageText.size()); + const size_t languageTextMaxSize = data.size_ - keysize - 3; + std::string languageText = + string_from_unterminated((const char*)(data.pData_ + Safe::add(keysize, 3)), languageTextMaxSize); + const unsigned int languageTextSize = static_cast(languageText.size()); + + enforce(data.size_ >= Safe::add(static_cast(Safe::add(keysize, 4)), languageTextSize), + Exiv2::kerCorruptedMetadata); // translated keyword string after the language description - std::string translatedKeyText((const char*)(data.pData_ + keysize + 3 + languageTextSize +1)); - unsigned int translatedKeyTextSize = static_cast(translatedKeyText.size()); + std::string translatedKeyText = + string_from_unterminated((const char*)(data.pData_ + keysize + 3 + languageTextSize + 1), + data.size_ - (keysize + 3 + languageTextSize + 1)); + const unsigned int translatedKeyTextSize = static_cast(translatedKeyText.size()); - if ( compressionFlag == 0x00 ) - { - // then it's an uncompressed iTXt chunk -#ifdef DEBUG - std::cout << "Exiv2::PngChunk::parseTXTChunk: We found an uncompressed iTXt field\n"; -#endif + if ((compressionFlag == 0x00) || (compressionFlag == 0x01 && compressionMethod == 0x00)) { + enforce(Safe::add(static_cast(keysize + 3 + languageTextSize + 1), + Safe::add(translatedKeyTextSize, 1u)) <= data.size_, + Exiv2::kerCorruptedMetadata); - // the text comes after the translated keyword, but isn't null terminated const byte* text = data.pData_ + keysize + 3 + languageTextSize + 1 + translatedKeyTextSize + 1; - long textsize = data.size_ - (keysize + 3 + languageTextSize + 1 + translatedKeyTextSize + 1); + const long textsize = data.size_ - (keysize + 3 + languageTextSize + 1 + translatedKeyTextSize + 1); - arr.alloc(textsize); - arr = DataBuf(text, textsize); - } - else if ( compressionFlag == 0x01 && compressionMethod == 0x00 ) - { - // then it's a zlib compressed iTXt chunk + if (compressionFlag == 0x00) { + // then it's an uncompressed iTXt chunk #ifdef DEBUG - std::cout << "Exiv2::PngChunk::parseTXTChunk: We found a zlib compressed iTXt field\n"; + std::cout << "Exiv2::PngChunk::parseTXTChunk: We found an uncompressed iTXt field\n"; #endif - // the compressed text comes after the translated keyword, but isn't null terminated - const byte* compressedText = data.pData_ + keysize + 3 + languageTextSize + 1 + translatedKeyTextSize + 1; - long compressedTextSize = data.size_ - (keysize + 3 + languageTextSize + 1 + translatedKeyTextSize + 1); + arr.alloc(textsize); + arr = DataBuf(text, textsize); + } else if (compressionFlag == 0x01 && compressionMethod == 0x00) { + // then it's a zlib compressed iTXt chunk +#ifdef DEBUG + std::cout << "Exiv2::PngChunk::parseTXTChunk: We found a zlib compressed iTXt field\n"; +#endif - zlibUncompress(compressedText, compressedTextSize, arr); - } - else - { + // the compressed text comes after the translated keyword, but isn't null terminated + zlibUncompress(text, textsize, arr); + } + } else { // then it isn't zlib compressed and we are sunk #ifdef DEBUG std::cerr << "Exiv2::PngChunk::parseTXTChunk: Non-standard iTXt compression method.\n";