From ed90f646bf243b12baaa27aa5fb6db9dcfb774f4 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Wed, 21 Jul 2021 19:53:59 +0100 Subject: [PATCH 1/7] Regression test for https://github.com/Exiv2/exiv2/security/advisories/GHSA-mvc4-g5pv-4qqq --- test/data/issue_ghsa_mvc4_g5pv_4qqq_poc.jpg | Bin 0 -> 66 bytes .../github/test_issue_ghsa_mvc4_g5pv_4qqq.py | 20 ++++++++++++++++++ 2 files changed, 20 insertions(+) create mode 100644 test/data/issue_ghsa_mvc4_g5pv_4qqq_poc.jpg create mode 100644 tests/bugfixes/github/test_issue_ghsa_mvc4_g5pv_4qqq.py diff --git a/test/data/issue_ghsa_mvc4_g5pv_4qqq_poc.jpg b/test/data/issue_ghsa_mvc4_g5pv_4qqq_poc.jpg new file mode 100644 index 0000000000000000000000000000000000000000..f3d75f95cc039523d6fb122d1b042a07e6cb7a73 GIT binary patch literal 66 zcmex=Q#zd*rQ&w#<)$&;6X VfgyttNH8!6Fy^FI>M{Jk2>@6C71IC! literal 0 HcmV?d00001 diff --git a/tests/bugfixes/github/test_issue_ghsa_mvc4_g5pv_4qqq.py b/tests/bugfixes/github/test_issue_ghsa_mvc4_g5pv_4qqq.py new file mode 100644 index 0000000000..1422239127 --- /dev/null +++ b/tests/bugfixes/github/test_issue_ghsa_mvc4_g5pv_4qqq.py @@ -0,0 +1,20 @@ +# -*- coding: utf-8 -*- + +from system_tests import CaseMeta, CopyTmpFiles, path +@CopyTmpFiles("$data_path/issue_ghsa_mvc4_g5pv_4qqq_poc.jpg") + +class JpegBasePrintStructureInfiniteLoop(metaclass=CaseMeta): + """ + Regression test for the bug described in: + https://github.com/Exiv2/exiv2/security/advisories/GHSA-mvc4-g5pv-4qqq + """ + url = "https://github.com/Exiv2/exiv2/security/advisories/GHSA-mvc4-g5pv-4qqq" + + filename = path("$tmp_path/issue_ghsa_mvc4_g5pv_4qqq_poc.jpg") + commands = ["$exiv2 -d I rm $filename"] + stdout = [""] + stderr = [ +"""Exiv2 exception in erase action for file $filename: +$kerFailedToReadImageData +"""] + retval = [1] From 6eec038592ac51d982ecd4e935ce206e22879c58 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 18 Jul 2021 10:39:57 +0100 Subject: [PATCH 2/7] bufRead needs to be adjusted after seek() --- src/jpgimage.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index be408a47f3..0176443b86 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -713,6 +713,7 @@ namespace Exiv2 { io_->seek(-bufRead, BasicIo::cur); iptcDataSegs.push_back(io_->tell()); iptcDataSegs.push_back(size); + bufRead = 0; } } else if (bPrint) { const size_t start = size > 0 ? 2 : 0; From b3c63dac854f27c366aeaba8b46a3125cf18a27b Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Wed, 21 Jul 2021 21:39:38 +0100 Subject: [PATCH 3/7] Improved handling of jpg segments to avoid out-of-bound reads. --- src/jpgimage.cpp | 568 +++++++++++----------- test/data/icc-test.out | 36 +- tests/bugfixes/redmine/test_issue_1247.py | 2 +- 3 files changed, 292 insertions(+), 314 deletions(-) diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index 0176443b86..10bace7261 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -95,6 +95,19 @@ namespace Exiv2 { const uint16_t Photoshop::iptc_ = 0x0404; const uint16_t Photoshop::preview_ = 0x040c; + // BasicIo::read() with error checking + static void readOrThrow(BasicIo& iIo, byte* buf, long rcount, ErrorCode err) { + const long nread = iIo.read(buf, rcount); + enforce(nread == rcount, err); + enforce(!iIo.error(), err); + } + + // BasicIo::seek() with error checking + static void seekOrThrow(BasicIo& iIo, long offset, BasicIo::Position pos, ErrorCode err) { + const int r = iIo.seek(offset, pos); + enforce(r == 0, err); + } + static inline bool inRange(int lo,int value, int hi) { return lo<=value && value <= hi; @@ -350,41 +363,48 @@ namespace Exiv2 { } clearMetadata(); int search = 6 ; // Exif, ICC, XMP, Comment, IPTC, SOF - const long bufMinSize = 36; - long bufRead = 0; - DataBuf buf(bufMinSize); Blob psBlob; bool foundCompletePsData = false; bool foundExifData = false; bool foundXmpData = false; bool foundIccData = false; + // which markers have a length field? + // TODO: move this to a utility function + bool mHasLength[256]; + for (int i = 0; i < 256; i++) + mHasLength[i] = (i >= sof0_ && i <= sof15_) || (i >= app0_ && i <= (app0_ | 0x0F)) || + (i == dht_ || i == dqt_ || i == dri_ || i == com_ || i == sos_); + // Read section marker int marker = advanceToMarker(); if (marker < 0) throw Error(kerNotAJpeg); while (marker != sos_ && marker != eoi_ && search > 0) { - // Read size and signature (ok if this hits EOF) - std::memset(buf.pData_, 0x0, buf.size_); - bufRead = io_->read(buf.pData_, bufMinSize); - if (io_->error()) throw Error(kerFailedToReadImageData); - if (bufRead < 2) throw Error(kerNotAJpeg); - uint16_t size = getUShort(buf.pData_, bigEndian); + // 2-byte buffer for reading the size. + byte sizebuf[2]; + uint16_t size = 0; + if (mHasLength[marker]) { + readOrThrow(*io_, sizebuf, 2, kerFailedToReadImageData); + size = getUShort(sizebuf, bigEndian); + // `size` is the size of the segment, including the 2-byte size field + // that we just read. + enforce(size >= 2, kerFailedToReadImageData); + } + + // Read the rest of the segment. + DataBuf buf(size); + if (size > 0) { + assert(size >= 2); // enforced above + readOrThrow(*io_, buf.pData_ + 2, size - 2, kerFailedToReadImageData); + memcpy(buf.pData_, sizebuf, 2); + } if ( !foundExifData - && marker == app1_ && memcmp(buf.pData_ + 2, exifId_, 6) == 0) { - if (size < 8) { - rc = 1; - break; - } - // Seek to beginning and read the Exif data - io_->seek(8 - bufRead, BasicIo::cur); - DataBuf rawExif(size - 8); - io_->read(rawExif.pData_, rawExif.size_); - if (io_->error() || io_->eof()) throw Error(kerFailedToReadImageData); - ByteOrder bo = ExifParser::decode(exifData_, rawExif.pData_, rawExif.size_); + && marker == app1_ && size >= 8 && memcmp(buf.pData_ + 2, exifId_, 6) == 0) { + ByteOrder bo = ExifParser::decode(exifData_, buf.pData_ + 8, size - 8); setByteOrder(bo); - if (rawExif.size_ > 0 && byteOrder() == invalidByteOrder) { + if (size > 8 && byteOrder() == invalidByteOrder) { #ifndef SUPPRESS_WARNINGS EXV_WARNING << "Failed to decode Exif metadata.\n"; #endif @@ -394,17 +414,8 @@ namespace Exiv2 { foundExifData = true; } else if ( !foundXmpData - && marker == app1_ && memcmp(buf.pData_ + 2, xmpId_, 29) == 0) { - if (size < 31) { - rc = 6; - break; - } - // Seek to beginning and read the XMP packet - io_->seek(31 - bufRead, BasicIo::cur); - DataBuf xmpPacket(size - 31); - io_->read(xmpPacket.pData_, xmpPacket.size_); - if (io_->error() || io_->eof()) throw Error(kerFailedToReadImageData); - xmpPacket_.assign(reinterpret_cast(xmpPacket.pData_), xmpPacket.size_); + && marker == app1_ && size >= 31 && memcmp(buf.pData_ + 2, xmpId_, 29) == 0) { + xmpPacket_.assign(reinterpret_cast(buf.pData_ + 31), size - 31); if (xmpPacket_.size() > 0 && XmpParser::decode(xmpData_, xmpPacket_)) { #ifndef SUPPRESS_WARNINGS EXV_WARNING << "Failed to decode XMP metadata.\n"; @@ -414,22 +425,13 @@ namespace Exiv2 { foundXmpData = true; } else if ( !foundCompletePsData - && marker == app13_ && memcmp(buf.pData_ + 2, Photoshop::ps3Id_, 14) == 0) { - if (size < 16) { - rc = 2; - break; - } - // Read the rest of the APP13 segment - io_->seek(16 - bufRead, BasicIo::cur); - DataBuf psData(size - 16); - io_->read(psData.pData_, psData.size_); - if (io_->error() || io_->eof()) throw Error(kerFailedToReadImageData); + && marker == app13_ && size >= 16 && memcmp(buf.pData_ + 2, Photoshop::ps3Id_, 14) == 0) { #ifdef EXIV2_DEBUG_MESSAGES std::cerr << "Found app13 segment, size = " << size << "\n"; //hexdump(std::cerr, psData.pData_, psData.size_); #endif // Append to psBlob - append(psBlob, psData.pData_, psData.size_); + append(psBlob, buf.pData_ + 16, size - 16); // Check whether psBlob is complete if (psBlob.size() > 0 && Photoshop::valid(&psBlob[0], (long) psBlob.size())) { --search; @@ -438,26 +440,18 @@ namespace Exiv2 { } else if (marker == com_ && comment_.empty()) { - if (size < 2) { - rc = 3; - break; - } // JPEGs can have multiple comments, but for now only read // the first one (most jpegs only have one anyway). Comments // are simple single byte ISO-8859-1 strings. - io_->seek(2 - bufRead, BasicIo::cur); - DataBuf comment(size - 2); - io_->read(comment.pData_, comment.size_); - if (io_->error() || io_->eof()) throw Error(kerFailedToReadImageData); - comment_.assign(reinterpret_cast(comment.pData_), comment.size_); + comment_.assign(reinterpret_cast(buf.pData_ + 2), size - 2); while ( comment_.length() && comment_.at(comment_.length()-1) == '\0') { comment_.erase(comment_.length()-1); } --search; } - else if ( marker == app2_ && memcmp(buf.pData_ + 2, iccId_,11)==0) { - if (size < 2+14) { + else if ( marker == app2_ && size >= 13 && memcmp(buf.pData_ + 2, iccId_,11)==0) { + if (size < 2+14+4) { rc = 8; break; } @@ -479,28 +473,19 @@ namespace Exiv2 { << (chunk==1 ? " size: " : "" ) << (chunk==1 ? s : 0) << std::endl ; #endif - io_->seek(-bufRead, BasicIo::cur); // back up to start of buffer (after marker) - io_->seek( 14+2, BasicIo::cur); // step header - // read in profile // #1286 profile can be padded long icc_size = size-2-14; if (chunk==1 && chunks==1) { enforce(s <= static_cast(icc_size), kerInvalidIccProfile); icc_size = s; } - DataBuf icc(icc_size); - io_->read( icc.pData_,icc.size_); - - if ( !iccProfileDefined() ) { // first block of profile - setIccProfile(icc,chunk==chunks); - } else { // extend existing profile - DataBuf profile(Safe::add(iccProfile_.size_, icc.size_)); - if ( iccProfile_.size_ ) { - ::memcpy(profile.pData_,iccProfile_.pData_,iccProfile_.size_); - } - ::memcpy(profile.pData_+iccProfile_.size_,icc.pData_,icc.size_); - setIccProfile(profile,chunk==chunks); + + DataBuf profile(Safe::add(iccProfile_.size_, icc_size)); + if ( iccProfile_.size_ ) { + ::memcpy(profile.pData_,iccProfile_.pData_,iccProfile_.size_); } + ::memcpy(profile.pData_+iccProfile_.size_, buf.pData_ + (2+14), icc_size); + setIccProfile(profile,chunk==chunks); } else if ( pixelHeight_ == 0 && inRange2(marker,sof0_,sof3_,sof5_,sof15_) ) { // We hit a SOFn (start-of-frame) marker @@ -511,17 +496,8 @@ namespace Exiv2 { pixelHeight_ = getUShort(buf.pData_ + 3, bigEndian); pixelWidth_ = getUShort(buf.pData_ + 5, bigEndian); if (pixelHeight_ != 0) --search; - // Skip the remainder of the segment - io_->seek(size-bufRead, BasicIo::cur); - } - else { - if (size < 2) { - rc = 4; - break; - } - // Skip the remainder of the unknown segment - if (io_->seek(size - bufRead, BasicIo::cur)) throw Error(kerFailedToReadImageData); } + // Read the beginning of the next segment marker = advanceToMarker(); if (marker < 0) { @@ -583,7 +559,7 @@ namespace Exiv2 { } bool bPrint = option == kpsBasic || option == kpsRecursive; - Exiv2::Uint32Vector iptcDataSegs; + std::vector iptcDataSegs; if (bPrint || option == kpsXMP || option == kpsIccProfile || option == kpsIptcErase) { // nmonic for markers @@ -610,6 +586,7 @@ namespace Exiv2 { } // which markers have a length field? + // TODO: move this to a utility function bool mHasLength[256]; for (int i = 0; i < 256; i++) mHasLength[i] = (i >= sof0_ && i <= sof15_) || (i >= app0_ && i <= (app0_ | 0x0F)) || @@ -617,9 +594,6 @@ namespace Exiv2 { // Container for the signature bool bExtXMP = false; - long bufRead = 0; - const long bufMinSize = 36; - DataBuf buf(bufMinSize); // Read section marker int marker = advanceToMarker(); @@ -638,20 +612,35 @@ namespace Exiv2 { first = false; bool bLF = bPrint; - // Read size and signature - std::memset(buf.pData_, 0x0, buf.size_); - bufRead = io_->read(buf.pData_, bufMinSize); - if (io_->error() || bufRead != bufMinSize) - throw Error(kerFailedToReadImageData); - const uint16_t size = mHasLength[marker] ? getUShort(buf.pData_, bigEndian) : 0; + // 2-byte buffer for reading the size. + byte sizebuf[2]; + uint16_t size = 0; + if (mHasLength[marker]) { + readOrThrow(*io_, sizebuf, 2, kerFailedToReadImageData); + size = getUShort(sizebuf, bigEndian); + // `size` is the size of the segment, including the 2-byte size field + // that we just read. + enforce(size >= 2, kerFailedToReadImageData); + } + + // Read the rest of the segment. + DataBuf buf(size); + if (size > 0) { + assert(size >= 2); // enforced above + readOrThrow(*io_, buf.pData_ + 2, size - 2, kerFailedToReadImageData); + memcpy(buf.pData_, sizebuf, 2); + } + if (bPrint && mHasLength[marker]) out << Internal::stringFormat(" | %7d ", size); // print signature for APPn if (marker >= app0_ && marker <= (app0_ | 0x0F)) { + assert(mHasLength[marker]); + assert(size >= 2); // Because this marker has a length field. // http://www.adobe.com/content/dam/Adobe/en/devnet/xmp/pdfs/XMPSpecificationPart3.pdf p75 const std::string signature = - string_from_unterminated(reinterpret_cast(buf.pData_ + 2), buf.size_ - 2); + string_from_unterminated(reinterpret_cast(buf.pData_ + 2), size - 2); // 728 rmills@rmillsmbp:~/gnu/exiv2/ttt $ exiv2 -pS test/data/exiv2-bug922.jpg // STRUCTURE OF JPEG FILE: test/data/exiv2-bug922.jpg @@ -662,62 +651,51 @@ namespace Exiv2 { // 1787 | 0xe1 APP1 | 65460 | http://ns.adobe.com/xmp/extensio if (option == kpsXMP && signature.rfind("http://ns.adobe.com/x", 0) == 0) { // extract XMP - if (size > 0) { - io_->seek(-bufRead, BasicIo::cur); - std::vector xmp(size + 1); - io_->read(&xmp[0], size); - int start = 0; - - // http://wwwimages.adobe.com/content/dam/Adobe/en/devnet/xmp/pdfs/XMPSpecificationPart3.pdf - // if we find HasExtendedXMP, set the flag and ignore this block - // the first extended block is a copy of the Standard block. - // a robust implementation allows extended blocks to be out of sequence - // we could implement out of sequence with a dictionary of sequence/offset - // and dumping the XMP in a post read operation similar to kpsIptcErase - // for the moment, dumping 'on the fly' is working fine - if (!bExtXMP) { - while (xmp.at(start)) { - start++; - } + const char* xmp = reinterpret_cast(buf.pData_); + size_t start = 2; + + // http://wwwimages.adobe.com/content/dam/Adobe/en/devnet/xmp/pdfs/XMPSpecificationPart3.pdf + // if we find HasExtendedXMP, set the flag and ignore this block + // the first extended block is a copy of the Standard block. + // a robust implementation allows extended blocks to be out of sequence + // we could implement out of sequence with a dictionary of sequence/offset + // and dumping the XMP in a post read operation similar to kpsIptcErase + // for the moment, dumping 'on the fly' is working fine + if (!bExtXMP) { + while (start < size && xmp[start]) { start++; - const std::string xmp_from_start = string_from_unterminated( - reinterpret_cast(&xmp.at(start)), size - start); + } + start++; + if (start < size) { + const std::string xmp_from_start = + string_from_unterminated(&xmp[start], size - start); if (xmp_from_start.find("HasExtendedXMP", start) != xmp_from_start.npos) { start = size; // ignore this packet, we'll get on the next time around bExtXMP = true; } - } else { - start = 2 + 35 + 32 + 4 + 4; // Adobe Spec, p19 } - - out.write(reinterpret_cast(&xmp.at(start)), size - start); - bufRead = size; - done = !bExtXMP; + } else { + start = 2 + 35 + 32 + 4 + 4; // Adobe Spec, p19 } + + enforce(start <= size, kerInvalidXmpText); + out.write(reinterpret_cast(&xmp[start]), size - start); + done = !bExtXMP; } else if (option == kpsIccProfile && signature.compare(iccId_) == 0) { // extract ICCProfile - if (size > 0) { - io_->seek(-bufRead, BasicIo::cur); // back to buffer (after marker) - io_->seek(14 + 2, BasicIo::cur); // step over header - DataBuf icc(size - (14 + 2)); - io_->read(icc.pData_, icc.size_); - out.write(reinterpret_cast(icc.pData_), icc.size_); + if (size >= 16) { + out.write(reinterpret_cast(buf.pData_ + 16), size - 16); #ifdef EXIV2_DEBUG_MESSAGES std::cout << "iccProfile size = " << icc.size_ << std::endl; #endif - bufRead = size; } } else if (option == kpsIptcErase && signature.compare("Photoshop 3.0") == 0) { // delete IPTC data segment from JPEG - if (size > 0) { - io_->seek(-bufRead, BasicIo::cur); - iptcDataSegs.push_back(io_->tell()); - iptcDataSegs.push_back(size); - bufRead = 0; - } + iptcDataSegs.push_back(io_->tell() - size); + iptcDataSegs.push_back(size); } else if (bPrint) { - const size_t start = size > 0 ? 2 : 0; - const size_t end = start + (size > 32 ? 32 : size); + const size_t start = 2; + const size_t end = size > 34 ? 34 : size; out << "| " << Internal::binaryToString(makeSlice(buf, start, end)); if (signature.compare(iccId_) == 0) { // extract the chunk information from the buffer @@ -729,7 +707,7 @@ namespace Exiv2 { // We cannot extract the variables A and B from the signature string, as they are beyond the // null termination (and signature ends there). // => Read the chunk info from the DataBuf directly - enforce(buf.size_ - 2 > 14, "Buffer too small to extract chunk information."); + enforce(size >= 16, "Buffer too small to extract chunk information."); const int chunk = buf.pData_[2 + 12]; const int chunks = buf.pData_[2 + 13]; out << Internal::stringFormat(" chunk %d/%d", chunk, chunks); @@ -744,79 +722,67 @@ namespace Exiv2 { bool bPS = option == kpsRecursive && signature.compare("Photoshop 3.0") == 0; if (bFlir || bExif || bMPF || bPS) { // extract Exif data block which is tiff formatted - if (size > 0) { - out << std::endl; - - // allocate storage and current file position - byte* exif = new byte[size]; - uint32_t restore = io_->tell(); - - // copy the data to memory - io_->seek(-bufRead, BasicIo::cur); - io_->read(exif, size); - uint32_t start = signature.compare("Exif") == 0 ? 8 : 6; - uint32_t max = (uint32_t)size - 1; - - // is this an fff block? - if (bFlir) { - start = 0; - bFlir = false; - while (start < max) { - if (std::strcmp((const char*)(exif + start), "FFF") == 0) { - bFlir = true; - break; - } - start++; - } - } - - // there is a header in FLIR, followed by a tiff block - // Hunt down the tiff using brute force - if (bFlir) { - // FLIRFILEHEAD* pFFF = (FLIRFILEHEAD*) (exif+start) ; - while (start < max) { - if (exif[start] == 'I' && exif[start + 1] == 'I') - break; - if (exif[start] == 'M' && exif[start + 1] == 'M') - break; - start++; + out << std::endl; + + byte* exif = buf.pData_; + uint32_t start = signature.compare("Exif") == 0 ? 8 : 6; + uint32_t max = (uint32_t)size - 1; + + // is this an fff block? + if (bFlir) { + start = 2; + bFlir = false; + while (start+3 <= max) { + if (std::strcmp((const char*)(exif + start), "FFF") == 0) { + bFlir = true; + break; } - if (start < max) - std::cout << " FFF start = " << start << std::endl; - // << " index = " << pFFF->dwIndexOff << std::endl; + start++; } + } - if (bPS) { - IptcData::printStructure(out, makeSlice(exif, 0, size), depth); - } else { - // create a copy on write memio object with the data, then print the structure - BasicIo::AutoPtr p = BasicIo::AutoPtr(new MemIo(exif + start, size - start)); - if (start < max) - printTiffStructure(*p, out, option, depth); + // there is a header in FLIR, followed by a tiff block + // Hunt down the tiff using brute force + if (bFlir) { + // FLIRFILEHEAD* pFFF = (FLIRFILEHEAD*) (exif+start) ; + while (start < max) { + if (exif[start] == 'I' && exif[start + 1] == 'I') + break; + if (exif[start] == 'M' && exif[start + 1] == 'M') + break; + start++; } + if (start < max) + std::cout << " FFF start = " << start << std::endl; + // << " index = " << pFFF->dwIndexOff << std::endl; + } - // restore and clean up - io_->seek(restore, Exiv2::BasicIo::beg); - delete[] exif; - bLF = false; + if (bPS) { + IptcData::printStructure(out, makeSlice(exif, 0, size), depth); + } else { + // create a copy on write memio object with the data, then print the structure + BasicIo::AutoPtr p = BasicIo::AutoPtr(new MemIo(exif + start, size - start)); + if (start < max) + printTiffStructure(*p, out, option, depth); } + + // restore and clean up + bLF = false; } } // print COM marker if (bPrint && marker == com_) { + assert(mHasLength[marker]); + assert(size >= 2); // Because this marker has a length field. // size includes 2 for the two bytes for size! - const int n = (size - 2) > 32 ? 32 : size - 2; + const size_t n = (size - 2) > 32 ? 32 : size - 2; // start after the two bytes out << "| " << Internal::binaryToString( makeSlice(buf, 2, n + 2 /* cannot overflow as n is at most size - 2 */)); } - // Skip the segment if the size is known - if (io_->seek(size - bufRead, BasicIo::cur)) - throw Error(kerFailedToReadImageData); - if (bLF) out << std::endl; @@ -835,7 +801,7 @@ namespace Exiv2 { #ifdef EXIV2_DEBUG_MESSAGES std::cout << "iptc data blocks: " << iptcDataSegs.size() << std::endl; uint32_t toggle = 0; - for (Uint32Vector_i i = iptcDataSegs.begin(); i != iptcDataSegs.end(); i++) { + for (std::vector::const_iterator i = iptcDataSegs.begin(); i != iptcDataSegs.end(); i++) { std::cout << *i; if (toggle++ % 2) std::cout << std::endl; @@ -843,22 +809,22 @@ namespace Exiv2 { std::cout << ' '; } #endif - uint32_t count = (uint32_t)iptcDataSegs.size(); + size_t count = iptcDataSegs.size(); // figure out which blocks to copy - uint64_t* pos = new uint64_t[count + 2]; + std::vector pos(count + 2); pos[0] = 0; // copy the data that is not iptc - Uint32Vector_i it = iptcDataSegs.begin(); - for (uint64_t i = 0; i < count; i++) { + std::vector::const_iterator it = iptcDataSegs.begin(); + for (size_t i = 0; i < count; i++) { bool bOdd = (i % 2) != 0; bool bEven = !bOdd; pos[i + 1] = bEven ? *it : pos[i] + *it; ++it; } - pos[count + 1] = io_->size() - pos[count]; + pos[count + 1] = static_cast(io_->size()); #ifdef EXIV2_DEBUG_MESSAGES - for (uint64_t i = 0; i < count + 2; i++) + for (size_t i = 0; i < count + 2; i++) std::cout << pos[i] << " "; std::cout << std::endl; #endif @@ -871,26 +837,25 @@ namespace Exiv2 { BasicIo::AutoPtr tempIo(new MemIo); assert(tempIo.get() != 0); - for (uint64_t i = 0; i < (count / 2) + 1; i++) { - uint64_t start = pos[2 * i] + 2; // step JPG 2 byte marker + for (size_t i = 0; i < (count / 2) + 1; i++) { + long start = pos[2 * i] + 2; // step JPG 2 byte marker if (start == 2) start = 0; // read the file 2 byte SOI - long length = (long)(pos[2 * i + 1] - start); + long length = pos[2 * i + 1] - start; if (length) { #ifdef EXIV2_DEBUG_MESSAGES std::cout << start << ":" << length << std::endl; #endif - io_->seek(start, BasicIo::beg); + seekOrThrow(*io_, start, BasicIo::beg, kerFailedToReadImageData); DataBuf buf(length); - io_->read(buf.pData_, buf.size_); + readOrThrow(*io_, buf.pData_, buf.size_, kerFailedToReadImageData); tempIo->write(buf.pData_, buf.size_); } } - delete[] pos; - io_->seek(0, BasicIo::beg); + seekOrThrow(*io_, 0, BasicIo::beg, kerFailedToReadImageData); io_->transfer(*tempIo); // may throw - io_->seek(0, BasicIo::beg); + seekOrThrow(*io_, 0, BasicIo::beg, kerFailedToReadImageData); readMetadata(); } } // JpegBase::printStructure @@ -923,21 +888,21 @@ namespace Exiv2 { throw Error(kerNoImageInInputData); } - const long bufMinSize = 36; - long bufRead = 0; - DataBuf buf(bufMinSize); + // Used to initialize search variables such as skipCom. + static const size_t notfound = std::numeric_limits::max(); + const long seek = io_->tell(); - int count = 0; - int search = 0; - int insertPos = 0; - int comPos = 0; - int skipApp1Exif = -1; - int skipApp1Xmp = -1; + size_t count = 0; + size_t search = 0; + size_t insertPos = 0; + size_t comPos = 0; + size_t skipApp1Exif = notfound; + size_t skipApp1Xmp = notfound; bool foundCompletePsData = false; bool foundIccData = false; - std::vector skipApp13Ps3; - std::vector skipApp2Icc; - int skipCom = -1; + std::vector skipApp13Ps3; + std::vector skipApp2Icc; + size_t skipCom = notfound; Blob psBlob; DataBuf rawExif; xmpData().usePacket(writeXmpFromPacket()); @@ -946,6 +911,13 @@ namespace Exiv2 { if (writeHeader(outIo)) throw Error(kerImageWriteFailed); + // which markers have a length field? + // TODO: move this to a utility function + bool mHasLength[256]; + for (int i = 0; i < 256; i++) + mHasLength[i] = (i >= sof0_ && i <= sof15_) || (i >= app0_ && i <= (app0_ | 0x0F)) || + (i == dht_ || i == dqt_ || i == dri_ || i == com_ || i == sos_); + // Read section marker int marker = advanceToMarker(); if (marker < 0) @@ -955,80 +927,67 @@ namespace Exiv2 { // to insert after it. But if app0 comes after com, app1 and app13 then // don't bother. while (marker != sos_ && marker != eoi_ && search < 6) { - // Read size and signature (ok if this hits EOF) - bufRead = io_->read(buf.pData_, bufMinSize); - if (io_->error()) - throw Error(kerInputDataReadFailed); - uint16_t size = getUShort(buf.pData_, bigEndian); + // 2-byte buffer for reading the size. + byte sizebuf[2]; + uint16_t size = 0; + if (mHasLength[marker]) { + readOrThrow(*io_, sizebuf, 2, kerFailedToReadImageData); + size = getUShort(sizebuf, bigEndian); + // `size` is the size of the segment, including the 2-byte size field + // that we just read. + enforce(size >= 2, kerFailedToReadImageData); + } + + // Read the rest of the segment. + DataBuf buf(size); + if (size > 0) { + assert(size >= 2); // enforced above + readOrThrow(*io_, buf.pData_ + 2, size - 2, kerFailedToReadImageData); + memcpy(buf.pData_, sizebuf, 2); + } if (marker == app0_) { - if (size < 2) - throw Error(kerNoImageInInputData); + assert(mHasLength[marker]); + assert(size >= 2); // Because this marker has a length field. insertPos = count + 1; - if (io_->seek(size - bufRead, BasicIo::cur)) - throw Error(kerNoImageInInputData); - } else if (skipApp1Exif == -1 && marker == app1_ && memcmp(buf.pData_ + 2, exifId_, 6) == 0) { - if (size < 8) - throw Error(kerNoImageInInputData); + } else if (skipApp1Exif == notfound && marker == app1_ && memcmp(buf.pData_ + 2, exifId_, 6) == 0) { + enforce(size >= 8, kerNoImageInInputData); skipApp1Exif = count; ++search; - // Seek to beginning and read the current Exif data - io_->seek(8 - bufRead, BasicIo::cur); rawExif.alloc(size - 8); - io_->read(rawExif.pData_, rawExif.size_); - if (io_->error() || io_->eof()) - throw Error(kerNoImageInInputData); - } else if (skipApp1Xmp == -1 && marker == app1_ && memcmp(buf.pData_ + 2, xmpId_, 29) == 0) { - if (size < 31) - throw Error(kerNoImageInInputData); + memcpy(rawExif.pData_, buf.pData_ + 8, size - 8); + } else if (skipApp1Xmp == notfound && marker == app1_ && memcmp(buf.pData_ + 2, xmpId_, 29) == 0) { + enforce(size >= 31, kerNoImageInInputData); skipApp1Xmp = count; ++search; - if (io_->seek(size - bufRead, BasicIo::cur)) - throw Error(kerNoImageInInputData); } else if (marker == app2_ && memcmp(buf.pData_ + 2, iccId_, 11) == 0) { - if (size < 31) - throw Error(kerNoImageInInputData); + enforce(size >= 31, kerNoImageInInputData); skipApp2Icc.push_back(count); if (!foundIccData) { ++search; foundIccData = true; } - if (io_->seek(size - bufRead, BasicIo::cur)) - throw Error(kerNoImageInInputData); } else if (!foundCompletePsData && marker == app13_ && memcmp(buf.pData_ + 2, Photoshop::ps3Id_, 14) == 0) { #ifdef EXIV2_DEBUG_MESSAGES std::cerr << "Found APP13 Photoshop PS3 segment\n"; #endif - if (size < 16) - throw Error(kerNoImageInInputData); + enforce(size >= 16, kerNoImageInInputData); skipApp13Ps3.push_back(count); - io_->seek(16 - bufRead, BasicIo::cur); - // Load PS data now to allow reinsertion at any point - DataBuf psData(size - 16); - io_->read(psData.pData_, size - 16); - if (io_->error() || io_->eof()) - throw Error(kerInputDataReadFailed); // Append to psBlob - append(psBlob, psData.pData_, psData.size_); + append(psBlob, buf.pData_ + 16, size - 16); // Check whether psBlob is complete if (psBlob.size() > 0 && Photoshop::valid(&psBlob[0], (long)psBlob.size())) { foundCompletePsData = true; } - } else if (marker == com_ && skipCom == -1) { - if (size < 2) - throw Error(kerNoImageInInputData); + } else if (marker == com_ && skipCom == notfound) { + assert(mHasLength[marker]); + assert(size >= 2); // Because this marker has a length field. // Jpegs can have multiple comments, but for now only handle // the first one (most jpegs only have one anyway). skipCom = count; ++search; - if (io_->seek(size - bufRead, BasicIo::cur)) - throw Error(kerNoImageInInputData); - } else { - if (size < 2) - throw Error(kerNoImageInInputData); - if (io_->seek(size - bufRead, BasicIo::cur)) - throw Error(kerNoImageInInputData); } + // As in jpeg-6b/wrjpgcom.c: // We will insert the new comment marker just before SOFn. // This (a) causes the new comment to appear after, rather than before, @@ -1046,7 +1005,7 @@ namespace Exiv2 { if (!foundCompletePsData && psBlob.size() > 0) throw Error(kerNoImageInInputData); - search += (int)skipApp13Ps3.size() + (int)skipApp2Icc.size(); + search += skipApp13Ps3.size() + skipApp2Icc.size(); if (comPos == 0) { if (marker == eoi_) @@ -1066,7 +1025,7 @@ namespace Exiv2 { if (!comment_.empty()) ++search; - io_->seek(seek, BasicIo::beg); + seekOrThrow(*io_, seek, BasicIo::beg, kerNoImageInInputData); count = 0; marker = advanceToMarker(); if (marker < 0) @@ -1077,16 +1036,26 @@ namespace Exiv2 { // potential to change segment ordering (which is allowed). // Segments are erased if there is no assigned metadata. while (marker != sos_ && search > 0) { - // Read size and signature (ok if this hits EOF) - bufRead = io_->read(buf.pData_, bufMinSize); - if (io_->error()) - throw Error(kerInputDataReadFailed); - // Careful, this can be a meaningless number for empty - // images with only an eoi_ marker - uint16_t size = getUShort(buf.pData_, bigEndian); + // 2-byte buffer for reading the size. + byte sizebuf[2]; + uint16_t size = 0; + if (mHasLength[marker]) { + readOrThrow(*io_, sizebuf, 2, kerFailedToReadImageData); + size = getUShort(sizebuf, bigEndian); + // `size` is the size of the segment, including the 2-byte size field + // that we just read. + enforce(size >= 2, kerFailedToReadImageData); + } + + // Read the rest of the segment. + DataBuf buf(size); + if (size > 0) { + assert(size >= 2); // enforced above + readOrThrow(*io_, buf.pData_ + 2, size - 2, kerFailedToReadImageData); + memcpy(buf.pData_, sizebuf, 2); + } if (insertPos == count) { - byte tmpBuf[64]; // Write Exif data first so that - if there is no app0 - we // create "Exif images" according to the Exif standard. if (exifData_.count() > 0) { @@ -1098,17 +1067,18 @@ namespace Exiv2 { } WriteMethod wm = ExifParser::encode(blob, rawExif.pData_, rawExif.size_, bo, exifData_); const byte* pExifData = rawExif.pData_; - uint32_t exifSize = rawExif.size_; + size_t exifSize = rawExif.size_; if (wm == wmIntrusive) { pExifData = blob.size() > 0 ? &blob[0] : 0; - exifSize = static_cast(blob.size()); + exifSize = blob.size(); } if (exifSize > 0) { + byte tmpBuf[10]; // Write APP1 marker, size of APP1 field, Exif id and Exif data tmpBuf[0] = 0xff; tmpBuf[1] = app1_; - if (exifSize + 8 > 0xffff) + if (exifSize > 0xffff - 8) throw Error(kerTooLargeJpegSegment, "Exif"); us2Data(tmpBuf + 2, static_cast(exifSize + 8), bigEndian); std::memcpy(tmpBuf + 4, exifId_, 6); @@ -1132,11 +1102,12 @@ namespace Exiv2 { } } if (xmpPacket_.size() > 0) { + byte tmpBuf[33]; // Write APP1 marker, size of APP1 field, XMP id and XMP packet tmpBuf[0] = 0xff; tmpBuf[1] = app1_; - if (xmpPacket_.size() + 31 > 0xffff) + if (xmpPacket_.size() > 0xffff - 31) throw Error(kerTooLargeJpegSegment, "XMP"); us2Data(tmpBuf + 2, static_cast(xmpPacket_.size() + 31), bigEndian); std::memcpy(tmpBuf + 4, xmpId_, 29); @@ -1153,32 +1124,35 @@ namespace Exiv2 { } if (iccProfileDefined()) { + byte tmpBuf[4]; // Write APP2 marker, size of APP2 field, and IccProfile // See comments in readMetadata() about the ICC embedding specification tmpBuf[0] = 0xff; tmpBuf[1] = app2_; - int chunk_size = 256 * 256 - 40; // leave bytes for marker, header and padding - int size = (int)iccProfile_.size_; - int chunks = 1 + (size - 1) / chunk_size; - if (iccProfile_.size_ > 256 * chunk_size) + const long chunk_size = 256 * 256 - 40; // leave bytes for marker, header and padding + long size = iccProfile_.size_; + assert(size > 0); // Because iccProfileDefined() == true + if (size >= 255 * chunk_size) throw Error(kerTooLargeJpegSegment, "IccProfile"); - for (int chunk = 0; chunk < chunks; chunk++) { - int bytes = size > chunk_size ? chunk_size : size; // bytes to write + const long chunks = 1 + (size - 1) / chunk_size; + assert(chunks <= 255); // Because size < 255 * chunk_size + for (long chunk = 0; chunk < chunks; chunk++) { + long bytes = size > chunk_size ? chunk_size : size; // bytes to write size -= bytes; // write JPEG marker (2 bytes) if (outIo.write(tmpBuf, 2) != 2) throw Error(kerImageWriteFailed); // JPEG Marker // write length (2 bytes). length includes the 2 bytes for the length - us2Data(tmpBuf + 2, 2 + 14 + bytes, bigEndian); + us2Data(tmpBuf + 2, static_cast(2 + 14 + bytes), bigEndian); if (outIo.write(tmpBuf + 2, 2) != 2) throw Error(kerImageWriteFailed); // JPEG Length // write the ICC_PROFILE header (14 bytes) - char pad[2]; - pad[0] = chunk + 1; - pad[1] = chunks; + uint8_t pad[2]; + pad[0] = static_cast(chunk + 1); + pad[1] = static_cast(chunks); outIo.write((const byte*)iccId_, 12); outIo.write((const byte*)pad, 2); if (outIo.write(iccProfile_.pData_ + (chunk * chunk_size), bytes) != bytes) @@ -1212,6 +1186,7 @@ namespace Exiv2 { } // Write APP13 marker, chunk size, and ps3Id + byte tmpBuf[18]; tmpBuf[0] = 0xff; tmpBuf[1] = app13_; us2Data(tmpBuf + 2, static_cast(chunkSize + 16), bigEndian); @@ -1239,7 +1214,7 @@ namespace Exiv2 { tmpBuf[0] = 0xff; tmpBuf[1] = com_; - if (comment_.length() + 3 > 0xffff) + if (comment_.length() > 0xffff - 3) throw Error(kerTooLargeJpegSegment, "JPEG comment"); us2Data(tmpBuf + 2, static_cast(comment_.length() + 3), bigEndian); @@ -1262,16 +1237,14 @@ namespace Exiv2 { std::find(skipApp2Icc.begin(), skipApp2Icc.end(), count) != skipApp2Icc.end() || skipCom == count) { --search; - io_->seek(size - bufRead, BasicIo::cur); } else { - if (size < 2) - throw Error(kerNoImageInInputData); - buf.alloc(size + 2); - io_->seek(-bufRead - 2, BasicIo::cur); - io_->read(buf.pData_, size + 2); - if (io_->error() || io_->eof()) - throw Error(kerInputDataReadFailed); - if (outIo.write(buf.pData_, size + 2) != size + 2) + byte tmpBuf[2]; + // Write marker and a copy of the segment. + tmpBuf[0] = 0xff; + tmpBuf[1] = marker; + if (outIo.write(tmpBuf, 2) != 2) + throw Error(kerImageWriteFailed); + if (outIo.write(buf.pData_, size) != size) throw Error(kerImageWriteFailed); if (outIo.error()) throw Error(kerImageWriteFailed); @@ -1288,9 +1261,14 @@ namespace Exiv2 { // it avoids allocating memory for parts of the file that contain image-date. io_->populateFakeData(); - // Copy rest of the Io - io_->seek(-2, BasicIo::cur); - buf.alloc(4096); + // Write the final marker, then copy rest of the Io. + byte tmpBuf[2]; + tmpBuf[0] = 0xff; + tmpBuf[1] = marker; + if (outIo.write(tmpBuf, 2) != 2) + throw Error(kerImageWriteFailed); + + DataBuf buf(4096); long readSize = 0; while ((readSize = io_->read(buf.pData_, buf.size_))) { if (outIo.write(buf.pData_, readSize) != readSize) diff --git a/test/data/icc-test.out b/test/data/icc-test.out index b495b2f334..7f8aa048ef 100644 --- a/test/data/icc-test.out +++ b/test/data/icc-test.out @@ -5,7 +5,7 @@ STRUCTURE OF JPEG FILE: Reagan.jpg 5722 | 0xffed APP13 | 3038 | Photoshop 3.0.8BIM..........Z... 8762 | 0xffe1 APP1 | 5329 | http://ns.adobe.com/xap/1.0/. Date: Wed, 21 Jul 2021 22:03:36 +0100 Subject: [PATCH 4/7] Fix compiler warning. --- src/jpgimage.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index 10bace7261..9da0839d2c 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -1086,7 +1086,7 @@ namespace Exiv2 { throw Error(kerImageWriteFailed); // Write new Exif data buffer - if (outIo.write(pExifData, exifSize) != static_cast(exifSize)) + if (outIo.write(pExifData, static_cast(exifSize)) != static_cast(exifSize)) throw Error(kerImageWriteFailed); if (outIo.error()) throw Error(kerImageWriteFailed); From 147e6d3645f183c5a6faa800761056db57551055 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 25 Jul 2021 21:37:30 +0100 Subject: [PATCH 5/7] Update src/jpgimage.cpp Co-authored-by: Christoph Hasse --- src/jpgimage.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index 9da0839d2c..44d012166a 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -395,7 +395,6 @@ namespace Exiv2 { // Read the rest of the segment. DataBuf buf(size); if (size > 0) { - assert(size >= 2); // enforced above readOrThrow(*io_, buf.pData_ + 2, size - 2, kerFailedToReadImageData); memcpy(buf.pData_, sizebuf, 2); } From 2637af0854033e2cc5d8eb956e0fd62049b789d1 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 25 Jul 2021 22:01:23 +0100 Subject: [PATCH 6/7] poc from GHSA-9jh3-fcc3-g6hv can now be parsed without error. --- tests/bugfixes/github/test_issue_ghsa_9jh3_fcc3_g6hv.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/bugfixes/github/test_issue_ghsa_9jh3_fcc3_g6hv.py b/tests/bugfixes/github/test_issue_ghsa_9jh3_fcc3_g6hv.py index 430b7a214d..add8ef9c16 100644 --- a/tests/bugfixes/github/test_issue_ghsa_9jh3_fcc3_g6hv.py +++ b/tests/bugfixes/github/test_issue_ghsa_9jh3_fcc3_g6hv.py @@ -13,9 +13,5 @@ class JpegBasePrintStructureInfiniteLoop(metaclass=CaseMeta): filename = path("$tmp_path/issue_ghsa_9jh3_fcc3_g6hv_poc.jpg") commands = ["$exiv2 -d I rm $filename"] stdout = [""] - stderr = [ -"""Warning: JPEG format error, rc = 2 -Exiv2 exception in erase action for file $filename: -$kerFailedToReadImageData -"""] - retval = [1] + stderr = [""] + retval = [0] From b5cbb479f8d3c95cbf176f8030062a4171f1595f Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 25 Jul 2021 22:05:22 +0100 Subject: [PATCH 7/7] Add comment to explain bounds-check. --- src/jpgimage.cpp | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index 44d012166a..f28c71127c 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -400,7 +400,9 @@ namespace Exiv2 { } if ( !foundExifData - && marker == app1_ && size >= 8 && memcmp(buf.pData_ + 2, exifId_, 6) == 0) { + && marker == app1_ + && size >= 8 // prevent out-of-bounds read in memcmp on next line + && memcmp(buf.pData_ + 2, exifId_, 6) == 0) { ByteOrder bo = ExifParser::decode(exifData_, buf.pData_ + 8, size - 8); setByteOrder(bo); if (size > 8 && byteOrder() == invalidByteOrder) { @@ -413,7 +415,9 @@ namespace Exiv2 { foundExifData = true; } else if ( !foundXmpData - && marker == app1_ && size >= 31 && memcmp(buf.pData_ + 2, xmpId_, 29) == 0) { + && marker == app1_ + && size >= 31 // prevent out-of-bounds read in memcmp on next line + && memcmp(buf.pData_ + 2, xmpId_, 29) == 0) { xmpPacket_.assign(reinterpret_cast(buf.pData_ + 31), size - 31); if (xmpPacket_.size() > 0 && XmpParser::decode(xmpData_, xmpPacket_)) { #ifndef SUPPRESS_WARNINGS @@ -424,7 +428,9 @@ namespace Exiv2 { foundXmpData = true; } else if ( !foundCompletePsData - && marker == app13_ && size >= 16 && memcmp(buf.pData_ + 2, Photoshop::ps3Id_, 14) == 0) { + && marker == app13_ + && size >= 16 // prevent out-of-bounds read in memcmp on next line + && memcmp(buf.pData_ + 2, Photoshop::ps3Id_, 14) == 0) { #ifdef EXIV2_DEBUG_MESSAGES std::cerr << "Found app13 segment, size = " << size << "\n"; //hexdump(std::cerr, psData.pData_, psData.size_); @@ -449,7 +455,9 @@ namespace Exiv2 { } --search; } - else if ( marker == app2_ && size >= 13 && memcmp(buf.pData_ + 2, iccId_,11)==0) { + else if ( marker == app2_ + && size >= 13 // prevent out-of-bounds read in memcmp on next line + && memcmp(buf.pData_ + 2, iccId_,11)==0) { if (size < 2+14+4) { rc = 8; break;