Headline
CVE-2018-19535: Fix Issue #428 by piponazo · Pull Request #430 · Exiv2/exiv2
In Exiv2 0.26 and previous versions, PngChunk::readRawProfile in pngchunk_int.cpp may cause a denial of service (application crash due to a heap-based buffer over-read) via a crafted PNG file.
There’s something not quite right about this. I’ve download the POCs:
1148 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $ ls -salt ~/Downloads/*.png
4 -rw-r--r--+ 1 rmills staff 188 Sep 4 13:09 /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_2.png
4 -rw-r--r--+ 1 rmills staff 2598 Sep 4 13:09 /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_1.png
4 -rw-r--r--+ 1 rmills staff 456 Sep 4 13:08 /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_2.png
4 -rw-r--r--+ 1 rmills staff 1651 Sep 4 13:07 /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_1.png
4 -rw-r--r--+ 1 rmills staff 188 Sep 4 13:05 /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_2.png
4 -rw-r--r--+ 1 rmills staff 512 Sep 4 13:05 /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_1.png
1149 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $
When I apply your patch, I see:
for i in $(ls ~/Downloads/*.png|sort); do echo ---- $i ---- ; bin/exiv2 --verbose $i 2>&1 | grep SUMMARY ; echo '' ; done
---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_1.png ----
SUMMARY: AddressSanitizer: heap-buffer-overflow (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x52534) in wrap_atol
---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_2.png ----
---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_1.png ----
---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_2.png ----
---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_1.png ----
SUMMARY: AddressSanitizer: heap-buffer-overflow (libexiv2.26.dylib:x86_64+0x1256594) in Exiv2::Internal::PngChunk::readRawProfile(Exiv2::DataBuf const&, bool)
---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_2.png ----
SUMMARY: AddressSanitizer: heap-buffer-overflow (libexiv2.26.dylib:x86_64+0x1256594) in Exiv2::Internal::PngChunk::readRawProfile(Exiv2::DataBuf const&, bool)
1148 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $
Your patch is a step in the right direction. The code is cavalier in galloping forward without checking that it is reading within the buffer. atol() can also gallop, so I’ve added const char* startOfLength; and delayed atol() until I feel safe.
Having decoded the length, the code gallops decoding hex/nibbles without bothering to test if the buffer has 2*length bytes. I’ve added const char* endOfLength to know the first byte following the length string. When we calculate nibbles = 2*length, I test it for sanity.
Like you, I think the code needs a major review and the schedule doesn’t permit this at the moment.
I’ve put my modified version of PngChunk::readRawProfile() which includes the fixes that I’ve mentioned. This seems to fix the 6 POCs provided in #428
1197 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $ for i in $(ls ~/Downloads/*.png|sort); do echo ---- $i ---- ; bin/exiv2 --verbose $i ; echo '' ; done
---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_1.png ----
File 1/1: /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_1.png
Exiv2 exception in print action for file /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_1.png:
Failed to read image data
---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_2.png ----
File 1/1: /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_2.png
Exiv2 exception in print action for file /Users/rmills/Downloads/hbo_pngchunk_int.cpp-643_2.png:
Failed to read image data
---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_1.png ----
File 1/1: /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_1.png
Exiv2 exception in print action for file /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_1.png:
Failed to read image data
---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_2.png ----
File 1/1: /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_2.png
Exiv2 exception in print action for file /Users/rmills/Downloads/hbo_pngchunk_int.cpp-645_2.png:
Failed to read image data
---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_1.png ----
File 1/1: /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_1.png
Exiv2 exception in print action for file /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_1.png:
Failed to read image data
---- /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_2.png ----
File 1/1: /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_2.png
Exiv2 exception in print action for file /Users/rmills/Downloads/hbo_pngchunk_int.cpp-674_2.png:
Failed to read image data
1198 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/build $
DataBuf PngChunk::readRawProfile(const DataBuf& text,bool iTXt)
{
DataBuf info;
unsigned char unhex[103]={0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,
0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,
0,0,0,0,0,0,0,0,0,1, 2,3,4,5,6,7,8,9,0,0,
0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,
0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,10,11,12,
13,14,15};
if (text.size_ == 0) {
return DataBuf();
}
if ( iTXt ) {
info.alloc(text.size_);
::memcpy(info.pData_,text.pData_,text.size_);
return info;
}
const char *sp = (char*)text.pData_+1;
int pointerPos = 1;
// Look for newline
while (*sp != '\n' && pointerPos < (text.size_ - 1))
{
sp++;
pointerPos++;
}
if (pointerPos == (text.size_ - 1))
{
return DataBuf();
}
// Look for length
while ((*sp == '\0' || *sp == ' ' || *sp == '\n') && pointerPos < (text.size_ - 1))
{
sp++;
pointerPos++;
}
if (pointerPos == (text.size_ - 1))
{
return DataBuf();
}
const char* startOfLength = sp;
while (*sp != ' ' && *sp != '\n' && pointerPos < (text.size_ - 1))
{
sp++;
pointerPos++;
}
const char* endOfLength = sp;
if (pointerPos == (text.size_ - 1))
{
return DataBuf();
}
long length = (long) atol(startOfLength);
// Allocate space
if (length == 0)
{
#ifdef DEBUG
std::cerr << "Exiv2::PngChunk::readRawProfile: Unable To Copy Raw Profile: invalid profile length\n";
#endif
}
info.alloc(length);
if (info.size_ != length)
{
#ifdef DEBUG
std::cerr << "Exiv2::PngChunk::readRawProfile: Unable To Copy Raw Profile: cannot allocate memory\n";
#endif
return DataBuf();
}
// Copy profile, skipping white space and column 1 "=" signs
unsigned char *dp = (unsigned char*)info.pData_;
unsigned int nibbles = length * 2;
if ( (Exiv2::byte*)endOfLength + nibbles > info.pData_+info.size_ ) {
#ifdef DEBUG
std::cerr << "Exiv2::PngChunk::readRawProfile: Unable To Copy Raw Profile: nibble count overflow\n";
#endif
return DataBuf();
}
for (long i = 0; i < (long) nibbles; i++)
{
while (*sp < '0' || (*sp > '9' && *sp < 'a') || *sp > 'f')
{
if (*sp == '\0')
{
#ifdef DEBUG
std::cerr << "Exiv2::PngChunk::readRawProfile: Unable To Copy Raw Profile: ran out of data\n";
#endif
return DataBuf();
}
sp++;
}
if (i%2 == 0)
*dp = (unsigned char) (16*unhex[(int) *sp++]);
else
(*dp++) += unhex[(int) *sp++];
}
return info;
} // PngChunk::readRawProfile