From 2069cdd3c7da0a2219d4a1b61095853a76e4fd39 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Tue, 6 Jul 2021 16:31:29 +0100 Subject: [PATCH 1/6] Regression test for https://github.com/Exiv2/exiv2/security/advisories/GHSA-hqjh-hpv8-8r9p --- .../github/test_issue_ghsa_hqjh_hpv8_8r9p.py | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 tests/bugfixes/github/test_issue_ghsa_hqjh_hpv8_8r9p.py diff --git a/tests/bugfixes/github/test_issue_ghsa_hqjh_hpv8_8r9p.py b/tests/bugfixes/github/test_issue_ghsa_hqjh_hpv8_8r9p.py new file mode 100644 index 00000000..d6c8d602 --- /dev/null +++ b/tests/bugfixes/github/test_issue_ghsa_hqjh_hpv8_8r9p.py @@ -0,0 +1,51 @@ +# -*- coding: utf-8 -*- + +from system_tests import CaseMeta, FileDecoratorBase, path +from struct import * + +# The PoC is a fairly large file, mostly consisting of zero bytes, +# so it would be a waste of storage to check it into the repo. +# Instead, we can generate the PoC with a small amount of code: +class CreatePoC(FileDecoratorBase): + """ + This class copies files from test/data to test/tmp + Copied files are NOT removed in tearDown + Example: @CopyTmpFiles("$data_path/test_issue_1180.exv") + """ + + #: override the name of the file list + FILE_LIST_NAME = '_tmp_files' + + def setUp_file_action(self, expanded_file_name): + size = 0x20040 + contents = pack('<2sI8sHHIIHHII', bytes(b'II'), 14, bytes(b'HEAPCCDR'), \ + 1, 0x300b, size - 26, 12, 1, 0x102a, size - 38, 12) + \ + bytes(bytearray(size-38)) + f = open(expanded_file_name, 'wb') + f.write(contents) + f.close() + + def tearDown_file_action(self, f): + """ + Do nothing. We don't clean up TmpFiles + """ + +# This decorator generates the PoC file. +@CreatePoC("$tmp_path/issue_ghsa_hqjh_hpv8_8r9p_poc.crw") + +class CrwMapDecodeArrayInfiniteLoop(metaclass=CaseMeta): + """ + Regression test for the bug described in: + https://github.com/Exiv2/exiv2/security/advisories/GHSA-hqjh-hpv8-8r9p + """ + url = "https://github.com/Exiv2/exiv2/security/advisories/GHSA-hqjh-hpv8-8r9p" + + filename = path("$tmp_path/issue_ghsa_hqjh_hpv8_8r9p_poc.crw") + + commands = ["$exiv2 $filename"] + stdout = [""] + stderr = [ +"""Exiv2 exception in print action for file $filename: +$kerCorruptedMetadata +"""] + retval = [1] From ad5e6c479c607c014109c42db9caad77e67e5505 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Tue, 6 Jul 2021 18:15:40 +0100 Subject: [PATCH 2/6] Extra checking to prevent the loop counter from wrapping around. --- src/crwimage_int.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/crwimage_int.cpp b/src/crwimage_int.cpp index eac64ddd..a2a28604 100644 --- a/src/crwimage_int.cpp +++ b/src/crwimage_int.cpp @@ -867,12 +867,16 @@ namespace Exiv2 { assert(ifdId != ifdIdNotSet); std::string groupName(Internal::groupName(ifdId)); + const uint32_t component_size = ciffComponent.size(); + enforce(component_size % 2 == 0, kerCorruptedMetadata); + enforce(component_size/2 <= static_cast(std::numeric_limits::max()), kerCorruptedMetadata); + const uint16_t num_components = static_cast(component_size/2); uint16_t c = 1; - while (uint32_t(c)*2 < ciffComponent.size()) { + while (c < num_components) { uint16_t n = 1; ExifKey key(c, groupName); UShortValue value; - if (ifdId == canonCsId && c == 23 && ciffComponent.size() > 50) n = 3; + if (ifdId == canonCsId && c == 23 && component_size >= 52) n = 3; value.read(ciffComponent.pData() + c*2, n*2, byteOrder); image.exifData().add(key, &value); if (ifdId == canonSiId && c == 21) aperture = value.toLong(); From 69d82ffe01ecb77b6129d07a8f79cb48cf810580 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Wed, 7 Jul 2021 14:41:42 +0100 Subject: [PATCH 3/6] Defensive coding changes to avoid integer overflow in loop conditions. --- src/actions.cpp | 4 ++-- src/basicio.cpp | 9 ++++----- src/convert.cpp | 8 ++++---- src/exif.cpp | 2 +- src/exiv2.cpp | 4 ++-- src/iptc.cpp | 9 ++++++--- src/preview.cpp | 2 +- src/tags_int.cpp | 5 ++++- src/tiffcomposite_int.cpp | 4 ++-- src/tiffvisitor_int.cpp | 6 +++--- src/types.cpp | 2 +- src/xmp.cpp | 2 +- src/xmpsidecar.cpp | 2 +- 13 files changed, 32 insertions(+), 27 deletions(-) diff --git a/src/actions.cpp b/src/actions.cpp index 7398d87f..f1d9f962 100644 --- a/src/actions.cpp +++ b/src/actions.cpp @@ -603,8 +603,8 @@ namespace Action { std::ostringstream os; // #1114 - show negative values for SByte if (md.typeId() == Exiv2::signedByte) { - for ( int c = 0 ; c < md.value().count() ; c++ ) { - int value = md.value().toLong(c); + for ( long c = 0 ; c < md.value().count() ; c++ ) { + long value = md.value().toLong(c); os << (c?" ":"") << std::dec << (value < 128 ? value : value - 256); } } else { diff --git a/src/basicio.cpp b/src/basicio.cpp index 34e6c852..85a1bc75 100644 --- a/src/basicio.cpp +++ b/src/basicio.cpp @@ -1780,9 +1780,10 @@ namespace Exiv2 { // find $right findDiff = false; - blockIndex = nBlocks - 1; - blockSize = p_->blocksMap_[blockIndex].getSize(); - while ((blockIndex + 1 > 0) && right < src.size() && !findDiff) { + blockIndex = nBlocks; + while (blockIndex > 0 && right < src.size() && !findDiff) { + blockIndex--; + blockSize = p_->blocksMap_[blockIndex].getSize(); if(src.seek(-1 * (blockSize + right), BasicIo::end)) { findDiff = true; } else { @@ -1797,8 +1798,6 @@ namespace Exiv2 { } } } - blockIndex--; - blockSize = static_cast(p_->blocksMap_[blockIndex].getSize()); } delete []buf; diff --git a/src/convert.cpp b/src/convert.cpp index b131a127..fc603395 100644 --- a/src/convert.cpp +++ b/src/convert.cpp @@ -549,7 +549,7 @@ namespace Exiv2 { auto pos = exifData_->findKey(ExifKey(from)); if (pos == exifData_->end()) return; if (!prepareXmpTarget(to)) return; - for (int i = 0; i < pos->count(); ++i) { + for (long i = 0; i < pos->count(); ++i) { std::string value = pos->toString(i); if (!pos->value().ok()) { #ifndef SUPPRESS_WARNINGS @@ -697,7 +697,7 @@ namespace Exiv2 { if (pos == exifData_->end()) return; if (!prepareXmpTarget(to)) return; std::ostringstream value; - for (int i = 0; i < pos->count(); ++i) { + for (long i = 0; i < pos->count(); ++i) { value << static_cast(pos->toLong(i)); } (*xmpData_)[to] = value.str(); @@ -710,7 +710,7 @@ namespace Exiv2 { if (pos == exifData_->end()) return; if (!prepareXmpTarget(to)) return; std::ostringstream value; - for (int i = 0; i < pos->count(); ++i) { + for (long i = 0; i < pos->count(); ++i) { if (i > 0) value << '.'; value << pos->toLong(i); } @@ -828,7 +828,7 @@ namespace Exiv2 { auto pos = xmpData_->findKey(XmpKey(from)); if (pos == xmpData_->end()) return; std::ostringstream array; - for (int i = 0; i < pos->count(); ++i) { + for (long i = 0; i < pos->count(); ++i) { std::string value = pos->toString(i); if (!pos->value().ok()) { #ifndef SUPPRESS_WARNINGS diff --git a/src/exif.cpp b/src/exif.cpp index 04f39020..cb2153e9 100644 --- a/src/exif.cpp +++ b/src/exif.cpp @@ -953,7 +953,7 @@ namespace { long sumToLong(const Exiv2::Exifdatum& md) { long sum = 0; - for (int i = 0; i < md.count(); ++i) { + for (long i = 0; i < md.count(); ++i) { sum += md.toLong(i); } return sum; diff --git a/src/exiv2.cpp b/src/exiv2.cpp index 9f7c3995..9f4dcf5e 100644 --- a/src/exiv2.cpp +++ b/src/exiv2.cpp @@ -1517,7 +1517,7 @@ namespace { std::string parseEscapes(const std::string& input) { std::string result; - for (unsigned int i = 0; i < input.length(); ++i) { + for (size_t i = 0; i < input.length(); ++i) { char ch = input[i]; if (ch != '\\') { result.push_back(ch); @@ -1544,7 +1544,7 @@ namespace { result.push_back('\t'); break; case 'u': // Escaping of unicode - if (input.length() - 4 > i) { + if (input.length() >= 4 && input.length() - 4 > i) { int acc = 0; for (int j = 0; j < 4; ++j) { ++i; diff --git a/src/iptc.cpp b/src/iptc.cpp index 4252a988..a7da58c5 100644 --- a/src/iptc.cpp +++ b/src/iptc.cpp @@ -22,6 +22,7 @@ #include "iptc.hpp" #include "types.hpp" #include "error.hpp" +#include "enforce.hpp" #include "value.hpp" #include "datasets.hpp" #include "jpgimage.hpp" @@ -344,22 +345,24 @@ namespace Exiv2 { void IptcData::printStructure(std::ostream& out, const Slice& bytes, uint32_t depth) { - uint32_t i = 0; - while (i < bytes.size() - 3 && bytes.at(i) != 0x1c) + size_t i = 0; + while (i + 3 < bytes.size() && bytes.at(i) != 0x1c) i++; depth++; out << Internal::indent(depth) << "Record | DataSet | Name | Length | Data" << std::endl; - while (i < bytes.size() - 3) { + while (i + 3 < bytes.size()) { if (bytes.at(i) != 0x1c) { break; } char buff[100]; uint16_t record = bytes.at(i + 1); uint16_t dataset = bytes.at(i + 2); + enforce(bytes.size() - i >= 5, kerCorruptedMetadata); uint16_t len = getUShort(bytes.subSlice(i + 3, bytes.size()), bigEndian); snprintf(buff, sizeof(buff), " %6d | %7d | %-24s | %6d | ", record, dataset, Exiv2::IptcDataSets::dataSetName(dataset, record).c_str(), len); + enforce(bytes.size() - i >= 5 + len, kerCorruptedMetadata); out << buff << Internal::binaryToString(makeSlice(bytes, i + 5, i + 5 + (len > 40 ? 40 : len))) << (len > 40 ? "..." : "") << std::endl; diff --git a/src/preview.cpp b/src/preview.cpp index de010057..bcfefae7 100644 --- a/src/preview.cpp +++ b/src/preview.cpp @@ -804,7 +804,7 @@ namespace { enforce(size_ <= static_cast(io.size()), kerCorruptedMetadata); DataBuf buf(size_); uint32_t idxBuf = 0; - for (int i = 0; i < sizes.count(); i++) { + for (long i = 0; i < sizes.count(); i++) { uint32_t offset = dataValue.toLong(i); uint32_t size = sizes.toLong(i); enforce(Safe::add(idxBuf, size) < size_, kerCorruptedMetadata); diff --git a/src/tags_int.cpp b/src/tags_int.cpp index a9643389..7a509e0a 100644 --- a/src/tags_int.cpp +++ b/src/tags_int.cpp @@ -24,6 +24,7 @@ #include "convert.hpp" #include "error.hpp" +#include "enforce.hpp" #include "i18n.h" // NLS support. #include "canonmn_int.hpp" @@ -2569,7 +2570,9 @@ namespace Exiv2 { { uint16_t bit = 0; uint16_t comma = 0; - for (uint16_t i = 0; i < value.count(); i++ ) { // for each element in value array + long count = value.count(); + enforce(0 <= count && count <= std::numeric_limits::max(), kerCorruptedMetadata); + for (uint16_t i = 0; i < count; i++ ) { // for each element in value array auto bits = static_cast(value.toLong(i)); for (uint16_t b = 0; b < 16; ++b) { // for every bit if (bits & (1 << b)) { diff --git a/src/tiffcomposite_int.cpp b/src/tiffcomposite_int.cpp index 3e13493c..26e189cc 100644 --- a/src/tiffcomposite_int.cpp +++ b/src/tiffcomposite_int.cpp @@ -398,7 +398,7 @@ namespace Exiv2 { return; } uint32_t size = 0; - for (int i = 0; i < pSize->count(); ++i) { + for (long i = 0; i < pSize->count(); ++i) { size += static_cast(pSize->toLong(i)); } auto offset = static_cast(pValue()->toLong(0)); @@ -455,7 +455,7 @@ namespace Exiv2 { #endif return; } - for (int i = 0; i < pValue()->count(); ++i) { + for (long i = 0; i < pValue()->count(); ++i) { const auto offset = static_cast(pValue()->toLong(i)); const byte* pStrip = pData + baseOffset + offset; const auto size = static_cast(pSize->toLong(i)); diff --git a/src/tiffvisitor_int.cpp b/src/tiffvisitor_int.cpp index ccf12a4d..97b54e7d 100644 --- a/src/tiffvisitor_int.cpp +++ b/src/tiffvisitor_int.cpp @@ -456,7 +456,7 @@ namespace Exiv2 { // create vector of signedShorts from unsignedShorts in Exif.Canon.AFInfo std::vector ints; std::vector uint; - for (int i = 0; i < object->pValue()->count(); i++) { + for (long i = 0; i < object->pValue()->count(); i++) { ints.push_back(static_cast(object->pValue()->toLong(i))); uint.push_back(static_cast(object->pValue()->toLong(i))); } @@ -505,10 +505,10 @@ namespace Exiv2 { auto v = Exiv2::Value::create(record.bSigned ? Exiv2::signedShort : Exiv2::unsignedShort); std::ostringstream s; if (record.bSigned) { - for (int16_t k = 0; k < record.size; k++) + for (uint16_t k = 0; k < record.size; k++) s << " " << ints.at(nStart++); } else { - for (int16_t k = 0; k < record.size; k++) + for (uint16_t k = 0; k < record.size; k++) s << " " << uint.at(nStart++); } diff --git a/src/types.cpp b/src/types.cpp index c1886371..9a51d89b 100644 --- a/src/types.cpp +++ b/src/types.cpp @@ -586,7 +586,7 @@ namespace Exiv2 { bool stringTo(const std::string& s, bool& ok) { std::string lcs(s); /* lowercase string */ - for(unsigned i = 0; i < lcs.length(); i++) { + for(size_t i = 0; i < lcs.length(); i++) { lcs[i] = std::tolower(s[i]); } /* handle the same values as xmp sdk */ diff --git a/src/xmp.cpp b/src/xmp.cpp index 7bdc5cd5..b21225d9 100644 --- a/src/xmp.cpp +++ b/src/xmp.cpp @@ -789,7 +789,7 @@ namespace Exiv2 { if (i.typeId() == xmpBag || i.typeId() == xmpSeq || i.typeId() == xmpAlt) { printNode(ns, i.tagName(), "", options); meta.SetProperty(ns.c_str(), i.tagName().c_str(), nullptr, options); - for (int idx = 0; idx < i.count(); ++idx) { + for (long idx = 0; idx < i.count(); ++idx) { const std::string item = i.tagName() + "[" + toString(idx + 1) + "]"; printNode(ns, item, i.toString(idx), 0); meta.SetProperty(ns.c_str(), item.c_str(), i.toString(idx).c_str()); diff --git a/src/xmpsidecar.cpp b/src/xmpsidecar.cpp index 621eec7c..70f26019 100644 --- a/src/xmpsidecar.cpp +++ b/src/xmpsidecar.cpp @@ -229,7 +229,7 @@ namespace Exiv2 { std::string head(reinterpret_cast(buf + start), len - start); if (head.substr(0, 5) == " Date: Wed, 7 Jul 2021 16:49:24 +0100 Subject: [PATCH 4/6] Better fix for potential integer overflow in `bytes.size() - 3`. --- src/iptc.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/iptc.cpp b/src/iptc.cpp index a7da58c5..c381c9c3 100644 --- a/src/iptc.cpp +++ b/src/iptc.cpp @@ -345,12 +345,15 @@ namespace Exiv2 { void IptcData::printStructure(std::ostream& out, const Slice& bytes, uint32_t depth) { + if (bytes.size() < 3) { + return; + } size_t i = 0; - while (i + 3 < bytes.size() && bytes.at(i) != 0x1c) + while (i < bytes.size() - 3 && bytes.at(i) != 0x1c) i++; depth++; out << Internal::indent(depth) << "Record | DataSet | Name | Length | Data" << std::endl; - while (i + 3 < bytes.size()) { + while (i < bytes.size() - 3) { if (bytes.at(i) != 0x1c) { break; } From 67a46f742d939479020019ea59a7efacd0010334 Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Thu, 8 Jul 2021 10:46:24 +0100 Subject: [PATCH 5/6] Type of escapeStart should be size_t. --- src/exiv2.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/exiv2.cpp b/src/exiv2.cpp index 9f4dcf5e..dcf86461 100644 --- a/src/exiv2.cpp +++ b/src/exiv2.cpp @@ -1523,7 +1523,7 @@ namespace { result.push_back(ch); continue; } - int escapeStart = i; + size_t escapeStart = i; if (!(input.length() - 1 > i)) { result.push_back(ch); continue; From eb2782e68568e555e53f8dc688632d847e4007bf Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 18 Jul 2021 15:15:06 +0100 Subject: [PATCH 6/6] Fix warning: comparison of integer expressions of different signedness --- src/iptc.cpp | 2 +- src/tags_int.cpp | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/iptc.cpp b/src/iptc.cpp index c381c9c3..55bf3d08 100644 --- a/src/iptc.cpp +++ b/src/iptc.cpp @@ -365,7 +365,7 @@ namespace Exiv2 { snprintf(buff, sizeof(buff), " %6d | %7d | %-24s | %6d | ", record, dataset, Exiv2::IptcDataSets::dataSetName(dataset, record).c_str(), len); - enforce(bytes.size() - i >= 5 + len, kerCorruptedMetadata); + enforce(bytes.size() - i >= 5 + static_cast(len), kerCorruptedMetadata); out << buff << Internal::binaryToString(makeSlice(bytes, i + 5, i + 5 + (len > 40 ? 40 : len))) << (len > 40 ? "..." : "") << std::endl; diff --git a/src/tags_int.cpp b/src/tags_int.cpp index 7a509e0a..b9f9c3ed 100644 --- a/src/tags_int.cpp +++ b/src/tags_int.cpp @@ -2570,9 +2570,7 @@ namespace Exiv2 { { uint16_t bit = 0; uint16_t comma = 0; - long count = value.count(); - enforce(0 <= count && count <= std::numeric_limits::max(), kerCorruptedMetadata); - for (uint16_t i = 0; i < count; i++ ) { // for each element in value array + for (long i = 0; i < value.count(); i++ ) { // for each element in value array auto bits = static_cast(value.toLong(i)); for (uint16_t b = 0; b < 16; ++b) { // for every bit if (bits & (1 << b)) {