From 16dc5f73a511b4cf51baf7eb664a0bc4c1ddf909 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Wed, 21 Jul 2021 19:53:59 +0100 Subject: [PATCH 1/9] 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 00000000..14222391 --- /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 deb41bd1172f8f9b5a360f12de9b159249d4b345 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 18 Jul 2021 10:39:57 +0100 Subject: [PATCH 2/9] 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 f35e6250..de273048 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -709,6 +709,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 483a1497a05f15f9069fca1dc617ebc0a3769254 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Wed, 21 Jul 2021 21:39:38 +0100 Subject: [PATCH 3/9] Improved handling of jpg segments to avoid out-of-bound reads. --- src/jpgimage.cpp | 570 +++++++++++----------- test/data/icc-test.out | 36 +- tests/bugfixes/redmine/test_issue_1247.py | 2 +- 3 files changed, 293 insertions(+), 315 deletions(-) diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index de273048..fb63b309 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -94,6 +94,19 @@ namespace Exiv2 { constexpr uint16_t Photoshop::iptc_ = 0x0404; constexpr 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; @@ -349,41 +362,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 @@ -393,17 +413,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_.empty() && XmpParser::decode(xmpData_, xmpPacket_)) { #ifndef SUPPRESS_WARNINGS EXV_WARNING << "Failed to decode XMP metadata.\n"; @@ -413,22 +424,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.empty() && Photoshop::valid(&psBlob[0], static_cast(psBlob.size()))) { --search; @@ -437,26 +439,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; } @@ -478,28 +472,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 @@ -510,17 +495,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) { @@ -580,7 +556,7 @@ namespace Exiv2 { } bool bPrint = option == kpsBasic || option == kpsRecursive; - std::vector iptcDataSegs; + std::vector iptcDataSegs; if (bPrint || option == kpsXMP || option == kpsIccProfile || option == kpsIptcErase) { // nmonic for markers @@ -606,6 +582,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)) || @@ -613,9 +590,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(); @@ -634,20 +608,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 @@ -658,62 +647,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; + 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 (xmp.at(start)) { - start++; - } + // 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) != std::string::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 } - } else if (option == kpsIccProfile && signature == iccId_) { + + 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 == "Photoshop 3.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 == iccId_) { // extract the chunk information from the buffer @@ -725,7 +703,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); @@ -740,79 +718,67 @@ namespace Exiv2 { bool bPS = option == kpsRecursive && signature == "Photoshop 3.0"; if (bFlir || bExif || bMPF || bPS) { // extract Exif data block which is tiff formatted - if (size > 0) { - out << std::endl; + out << std::endl; - // allocate storage and current file position - auto exif = new byte[size]; - uint32_t restore = io_->tell(); + byte* exif = buf.pData_; + uint32_t start = signature == "Exif" ? 8 : 6; + uint32_t max = static_cast(size) - 1; - // copy the data to memory - io_->seek(-bufRead, BasicIo::cur); - io_->read(exif, size); - uint32_t start = signature == "Exif" ? 8 : 6; - uint32_t max = static_cast(size) - 1; - - // is this an fff block? - if (bFlir) { - start = 0; - bFlir = false; - while (start < max) { - if (std::strcmp(reinterpret_cast(exif + start), "FFF") == 0) { - bFlir = true; - break; - } - start++; + // is this an fff block? + if (bFlir) { + start = 2; + bFlir = false; + while (start+3 <= max) { + if (std::strcmp(reinterpret_cast(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++; - } - if (start < max) - std::cout << " FFF start = " << start << std::endl; - // << " index = " << pFFF->dwIndexOff << std::endl; - } - - 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::UniquePtr p = BasicIo::UniquePtr(new MemIo(exif + start, size - start)); - if (start < max) - printTiffStructure(*p, out, option, depth); - } - - // restore and clean up - io_->seek(restore, Exiv2::BasicIo::beg); - delete[] exif; - bLF = false; } + + // 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; + } + + 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::UniquePtr p = BasicIo::UniquePtr(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; @@ -839,22 +805,22 @@ namespace Exiv2 { std::cout << ' '; } #endif - auto count = static_cast(iptcDataSegs.size()); + size_t count = iptcDataSegs.size(); // figure out which blocks to copy - auto pos = new uint64_t[count + 2]; + std::vector pos(count + 2); pos[0] = 0; // copy the data that is not iptc auto it = iptcDataSegs.begin(); - for (uint64_t i = 0; i < count; i++) { + 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 @@ -867,26 +833,25 @@ namespace Exiv2 { BasicIo::UniquePtr 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 = static_cast(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 @@ -919,21 +884,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()); @@ -942,6 +907,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) @@ -951,80 +923,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.empty() && Photoshop::valid(&psBlob[0], static_cast(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, @@ -1042,7 +1001,7 @@ namespace Exiv2 { if (!foundCompletePsData && !psBlob.empty()) throw Error(kerNoImageInInputData); - search += static_cast(skipApp13Ps3.size()) + static_cast(skipApp2Icc.size()); + search += skipApp13Ps3.size() + skipApp2Icc.size(); if (comPos == 0) { if (marker == eoi_) @@ -1062,7 +1021,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) @@ -1073,16 +1032,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) { @@ -1094,17 +1063,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.empty() ? &blob[0] : nullptr; - 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); @@ -1128,11 +1098,12 @@ namespace Exiv2 { } } if (!xmpPacket_.empty()) { + 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); @@ -1149,34 +1120,37 @@ 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 sizeProfile = static_cast(iccProfile_.size_); - int chunks = 1 + (sizeProfile - 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 = sizeProfile > chunk_size ? chunk_size : sizeProfile; // bytes to write - sizeProfile -= bytes; + 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; - outIo.write(reinterpret_cast(iccId_), 12); - outIo.write(reinterpret_cast(pad), 2); + 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) throw Error(kerImageWriteFailed); if (outIo.error()) @@ -1208,6 +1182,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); @@ -1235,7 +1210,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); @@ -1259,16 +1234,14 @@ namespace Exiv2 { std::find(skipApp13Ps3.begin(), skipApp13Ps3.end(), count) != skipApp13Ps3.end() || 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); @@ -1285,9 +1258,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 b495b2f3..7f8aa048 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/9] 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 fb63b309..5997f832 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -1082,7 +1082,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 10bd09871fd517170076c5902ddfe255110f77ab Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 25 Jul 2021 21:37:30 +0100 Subject: [PATCH 5/9] 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 5997f832..7f002986 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -394,7 +394,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 ce39438371aa5f5e823c5e441ab8d87fc0ce85c5 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 25 Jul 2021 22:01:23 +0100 Subject: [PATCH 6/9] 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 430b7a21..add8ef9c 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 96b85751ee811e98e9c303718092b0be980f2573 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 25 Jul 2021 22:05:22 +0100 Subject: [PATCH 7/9] 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 7f002986..e8c80925 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -399,7 +399,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) { @@ -412,7 +414,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_.empty() && XmpParser::decode(xmpData_, xmpPacket_)) { #ifndef SUPPRESS_WARNINGS @@ -423,7 +427,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_); @@ -448,7 +454,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; From 2532f6db40f03787908fb4ac26ddf291ec7136c9 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Mon, 26 Jul 2021 12:48:33 +0100 Subject: [PATCH 8/9] Add `markerHasLength` utility function. --- include/exiv2/jpgimage.hpp | 12 ++++- src/jpgimage.cpp | 89 +++++++++++++++----------------------- 2 files changed, 45 insertions(+), 56 deletions(-) diff --git a/include/exiv2/jpgimage.hpp b/include/exiv2/jpgimage.hpp index 881b9a63..f317c685 100644 --- a/include/exiv2/jpgimage.hpp +++ b/include/exiv2/jpgimage.hpp @@ -28,6 +28,7 @@ // included header files #include "image.hpp" +#include "error.hpp" // ***************************************************************************** // namespace extensions @@ -277,12 +278,19 @@ namespace Exiv2 { Jpeg marker and returns the marker. This method should be called when the BasicIo instance is positioned one byte past the end of a Jpeg segment. + @param err the error code to throw if no marker is found @return the next Jpeg segment marker if successful;
- -1 if a maker was not found before EOF + throws an Error if not successful */ - int advanceToMarker() const; + byte advanceToMarker(ErrorCode err) const; //@} + /*! + @brief Is the marker followed by a non-zero payload? + @param marker The marker at the start of a segment + @return true if the marker is followed by a non-zero payload + */ + static bool markerHasLength(byte marker); }; // class JpegBase /*! diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index e8c80925..d4c203c7 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -313,6 +313,16 @@ namespace Exiv2 { } // Photoshop::setIptcIrb + bool JpegBase::markerHasLength(byte marker) { + return (marker >= sof0_ && marker <= sof15_) || + (marker >= app0_ && marker <= (app0_ | 0x0F)) || + marker == dht_ || + marker == dqt_ || + marker == dri_ || + marker == com_ || + marker == sos_; + } + JpegBase::JpegBase(int type, BasicIo::UniquePtr io, bool create, const byte initData[], long dataSize) : Image(type, mdExif | mdIptc | mdXmp | mdComment, std::move(io)) @@ -334,19 +344,22 @@ namespace Exiv2 { return 0; } - int JpegBase::advanceToMarker() const + byte JpegBase::advanceToMarker(ErrorCode err) const { int c = -1; // Skips potential padding between markers while ((c=io_->getb()) != 0xff) { if (c == EOF) - return -1; + throw Error(err); } // Markers can start with any number of 0xff while ((c=io_->getb()) == 0xff) { } - return c; + if (c == EOF) + throw Error(err); + + return static_cast(c); } void JpegBase::readMetadata() @@ -368,22 +381,14 @@ namespace Exiv2 { 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); + byte marker = advanceToMarker(kerNotAJpeg); while (marker != sos_ && marker != eoi_ && search > 0) { // 2-byte buffer for reading the size. byte sizebuf[2]; uint16_t size = 0; - if (mHasLength[marker]) { + if (markerHasLength(marker)) { readOrThrow(*io_, sizebuf, 2, kerFailedToReadImageData); size = getUShort(sizebuf, bigEndian); // `size` is the size of the segment, including the 2-byte size field @@ -505,8 +510,9 @@ namespace Exiv2 { } // Read the beginning of the next segment - marker = advanceToMarker(); - if (marker < 0) { + try { + marker = advanceToMarker(kerFailedToReadImageData); + } catch (Error&) { rc = 5; break; } @@ -588,20 +594,11 @@ 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)) || - (i == dht_ || i == dqt_ || i == dri_ || i == com_ || i == sos_); - // Container for the signature bool bExtXMP = false; // Read section marker - int marker = advanceToMarker(); - if (marker < 0) - throw Error(kerNotAJpeg); + byte marker = advanceToMarker(kerNotAJpeg); bool done = false; bool first = true; @@ -618,7 +615,7 @@ namespace Exiv2 { // 2-byte buffer for reading the size. byte sizebuf[2]; uint16_t size = 0; - if (mHasLength[marker]) { + if (markerHasLength(marker)) { readOrThrow(*io_, sizebuf, 2, kerFailedToReadImageData); size = getUShort(sizebuf, bigEndian); // `size` is the size of the segment, including the 2-byte size field @@ -634,12 +631,12 @@ namespace Exiv2 { memcpy(buf.pData_, sizebuf, 2); } - if (bPrint && mHasLength[marker]) + if (bPrint && markerHasLength(marker)) out << Internal::stringFormat(" | %7d ", size); // print signature for APPn if (marker >= app0_ && marker <= (app0_ | 0x0F)) { - assert(mHasLength[marker]); + assert(markerHasLength(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 = @@ -776,7 +773,7 @@ namespace Exiv2 { // print COM marker if (bPrint && marker == com_) { - assert(mHasLength[marker]); + assert(markerHasLength(marker)); assert(size >= 2); // Because this marker has a length field. // size includes 2 for the two bytes for size! const size_t n = (size - 2) > 32 ? 32 : size - 2; @@ -791,8 +788,7 @@ namespace Exiv2 { if (marker != sos_) { // Read the beginning of the next segment - marker = advanceToMarker(); - enforce(marker>=0, kerNoImageInInputData); + marker = advanceToMarker(kerNoImageInInputData); REPORT_MARKER; } done |= marker == eoi_ || marker == sos_; @@ -914,17 +910,8 @@ 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) - throw Error(kerNoImageInInputData); + byte marker = advanceToMarker(kerNoImageInInputData); // First find segments of interest. Normally app0 is first and we want // to insert after it. But if app0 comes after com, app1 and app13 then @@ -933,7 +920,7 @@ namespace Exiv2 { // 2-byte buffer for reading the size. byte sizebuf[2]; uint16_t size = 0; - if (mHasLength[marker]) { + if (markerHasLength(marker)) { readOrThrow(*io_, sizebuf, 2, kerFailedToReadImageData); size = getUShort(sizebuf, bigEndian); // `size` is the size of the segment, including the 2-byte size field @@ -950,7 +937,7 @@ namespace Exiv2 { } if (marker == app0_) { - assert(mHasLength[marker]); + assert(markerHasLength(marker)); assert(size >= 2); // Because this marker has a length field. insertPos = count + 1; } else if (skipApp1Exif == notfound && marker == app1_ && memcmp(buf.pData_ + 2, exifId_, 6) == 0) { @@ -983,7 +970,7 @@ namespace Exiv2 { foundCompletePsData = true; } } else if (marker == com_ && skipCom == notfound) { - assert(mHasLength[marker]); + assert(markerHasLength(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). @@ -1000,9 +987,7 @@ namespace Exiv2 { comPos = count; ++search; } - marker = advanceToMarker(); - if (marker < 0) - throw Error(kerNoImageInInputData); + marker = advanceToMarker(kerNoImageInInputData); ++count; } @@ -1030,9 +1015,7 @@ namespace Exiv2 { seekOrThrow(*io_, seek, BasicIo::beg, kerNoImageInInputData); count = 0; - marker = advanceToMarker(); - if (marker < 0) - throw Error(kerNoImageInInputData); + marker = advanceToMarker(kerNoImageInInputData); // To simplify this a bit, new segments are inserts at either the start // or right after app0. This is standard in most jpegs, but has the @@ -1042,7 +1025,7 @@ namespace Exiv2 { // 2-byte buffer for reading the size. byte sizebuf[2]; uint16_t size = 0; - if (mHasLength[marker]) { + if (markerHasLength(marker)) { readOrThrow(*io_, sizebuf, 2, kerFailedToReadImageData); size = getUShort(sizebuf, bigEndian); // `size` is the size of the segment, including the 2-byte size field @@ -1255,9 +1238,7 @@ namespace Exiv2 { } // Next marker - marker = advanceToMarker(); - if (marker < 0) - throw Error(kerNoImageInInputData); + marker = advanceToMarker(kerNoImageInInputData); ++count; } From 35a2b25d642d4ba85256a9e1bc63bacff171fb71 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Mon, 26 Jul 2021 13:12:29 +0100 Subject: [PATCH 9/9] Fix build error when EXIV2_DEBUG_MESSAGES is enabled. --- src/jpgimage.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/jpgimage.cpp b/src/jpgimage.cpp index d4c203c7..5b89409e 100644 --- a/src/jpgimage.cpp +++ b/src/jpgimage.cpp @@ -686,7 +686,7 @@ namespace Exiv2 { if (size >= 16) { out.write(reinterpret_cast(buf.pData_ + 16), size - 16); #ifdef EXIV2_DEBUG_MESSAGES - std::cout << "iccProfile size = " << icc.size_ << std::endl; + std::cout << "iccProfile size = " << size - 16 << std::endl; #endif } } else if (option == kpsIptcErase && signature == "Photoshop 3.0") {