Merge pull request #1787 from Exiv2/mergify/bp/main/pr-1766

Extra checking to prevent loop counter from wrapping around (backport #1766)

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 {

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<long>(p_->blocksMap_[blockIndex].getSize());
         }
 
         delete []buf;

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<char>(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

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<uint32_t>(std::numeric_limits<uint16_t>::max()), kerCorruptedMetadata);
+        const uint16_t num_components = static_cast<uint16_t>(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();

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;

src/exiv2.cpp

@@ -1517,13 +1517,13 @@ 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);
                 continue;
             }
-            int escapeStart = i;
+            size_t escapeStart = i;
             if (!(input.length() - 1 > i)) {
                 result.push_back(ch);
                 continue;
@@ -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;

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,7 +345,10 @@ namespace Exiv2 {
 
     void IptcData::printStructure(std::ostream& out, const Slice<byte*>& bytes, uint32_t depth)
     {
-        uint32_t i = 0;
+        if (bytes.size() < 3) {
+            return;
+        }
+        size_t i = 0;
         while (i < bytes.size() - 3 && bytes.at(i) != 0x1c)
             i++;
         depth++;
@@ -356,10 +360,12 @@ namespace Exiv2 {
             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 + static_cast<size_t>(len), kerCorruptedMetadata);
             out << buff << Internal::binaryToString(makeSlice(bytes, i + 5, i + 5 + (len > 40 ? 40 : len)))
                 << (len > 40 ? "..." : "")
                 << std::endl;

src/preview.cpp

@@ -804,7 +804,7 @@ namespace {
                     enforce(size_ <= static_cast<uint32_t>(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);

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"
@@ -2570,7 +2571,7 @@ 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
+            for (long i = 0; i < value.count(); i++ ) { // for each element in value array
                 auto bits = static_cast<uint16_t>(value.toLong(i));
                 for (uint16_t b = 0; b < 16; ++b) { // for every bit
                     if (bits & (1 << 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<uint32_t>(pSize->toLong(i));
         }
         auto offset = static_cast<uint32_t>(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<uint32_t>(pValue()->toLong(i));
             const byte* pStrip = pData + baseOffset + offset;
             const auto size = static_cast<uint32_t>(pSize->toLong(i));

src/tiffvisitor_int.cpp

@@ -456,7 +456,7 @@ namespace Exiv2 {
         // create vector of signedShorts from unsignedShorts in Exif.Canon.AFInfo
         std::vector<int16_t>  ints;
         std::vector<uint16_t> uint;
-        for (int i = 0; i < object->pValue()->count(); i++) {
+        for (long i = 0; i < object->pValue()->count(); i++) {
             ints.push_back(static_cast<int16_t>(object->pValue()->toLong(i)));
             uint.push_back(static_cast<uint16_t>(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++);
                 }
 

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());

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]