Use Safe::add() in tiffvisitor_int.cpp

src/tiffvisitor_int.cpp

@@ -9,6 +9,7 @@
 #include "jpgimage.hpp"
 #include "makernote_int.hpp"
 #include "photoshop.hpp"
+#include "safe_op.hpp"
 #include "sonymn_int.hpp"
 #include "tiffcomposite_int.hpp"  // Do not change the order of these 2 includes,
 #include "tiffimage_int.hpp"
@@ -1232,111 +1233,108 @@ void TiffReader::visitIfdMakernoteEnd(TiffIfdMakernote* /*object*/) {
 }  // TiffReader::visitIfdMakernoteEnd
 
 void TiffReader::readTiffEntry(TiffEntryBase* object) {
-  byte* p = object->start();
+  try {
+    byte* p = object->start();
 
-  if (p + 12 > pLast_) {
+    if (p + 12 > pLast_) {
 #ifndef SUPPRESS_WARNINGS
-    EXV_ERROR << "Entry in directory " << groupName(object->group())
-              << "requests access to memory beyond the data buffer. "
-              << "Skipping entry.\n";
+      EXV_ERROR << "Entry in directory " << groupName(object->group())
+                << "requests access to memory beyond the data buffer. "
+                << "Skipping entry.\n";
 #endif
-    return;
-  }
-  // Component already has tag
-  p += 2;
-  TiffType tiffType = getUShort(p, byteOrder());
-  TypeId typeId = toTypeId(tiffType, object->tag(), object->group());
-  size_t typeSize = TypeInfo::typeSize(typeId);
-  if (0 == typeSize) {
+      return;
+    }
+    // Component already has tag
+    p += 2;
+    TiffType tiffType = getUShort(p, byteOrder());
+    TypeId typeId = toTypeId(tiffType, object->tag(), object->group());
+    size_t typeSize = TypeInfo::typeSize(typeId);
+    if (0 == typeSize) {
 #ifndef SUPPRESS_WARNINGS
-    EXV_WARNING << "Directory " << groupName(object->group()) << ", entry 0x" << std::setw(4) << std::setfill('0')
-                << std::hex << object->tag() << " has unknown Exif (TIFF) type " << std::dec << tiffType
-                << "; setting type size 1.\n";
+      EXV_WARNING << "Directory " << groupName(object->group()) << ", entry 0x" << std::setw(4) << std::setfill('0')
+                  << std::hex << object->tag() << " has unknown Exif (TIFF) type " << std::dec << tiffType
+                  << "; setting type size 1.\n";
 #endif
-    typeSize = 1;
-  }
-  p += 2;
-  uint32_t count = getULong(p, byteOrder());
-  if (count >= 0x10000000) {
+      typeSize = 1;
+    }
+    p += 2;
+    uint32_t count = getULong(p, byteOrder());
+    if (count >= 0x10000000) {
 #ifndef SUPPRESS_WARNINGS
-    EXV_ERROR << "Directory " << groupName(object->group()) << ", entry 0x" << std::setw(4) << std::setfill('0')
-              << std::hex << object->tag() << " has invalid size " << std::dec << count << "*" << typeSize
-              << "; skipping entry.\n";
+      EXV_ERROR << "Directory " << groupName(object->group()) << ", entry 0x" << std::setw(4) << std::setfill('0')
+                << std::hex << object->tag() << " has invalid size " << std::dec << count << "*" << typeSize
+                << "; skipping entry.\n";
 #endif
-    return;
-  }
-  p += 4;
+      return;
+    }
+    p += 4;
 
-  if (count > std::numeric_limits<uint32_t>::max() / typeSize) {
-    throw Error(ErrorCode::kerArithmeticOverflow);
-  }
-  size_t size = typeSize * count;
-  uint32_t offset = getULong(p, byteOrder());
-  byte* pData = p;
-  if (size > 4 && (baseOffset() + offset >= size_ || baseOffset() + offset <= 0)) {
-    // #1143
-    if (object->tag() == 0x2001 && std::string(groupName(object->group())) == "Sony1") {
-      // This tag is Exif.Sony1.PreviewImage, which refers to a preview image which is
-      // not stored in the metadata. Instead it is stored at the end of the file, after
-      // the main image. The value of `size` refers to the size of the preview image, not
-      // the size of the tag's payload, so we set it to zero here so that we don't attempt
-      // to read those bytes from the metadata. We currently leave this tag as "undefined",
-      // although we may attempt to handle it better in the future. More discussion of
-      // this issue can be found here:
-      //
-      //   https://github.com/Exiv2/exiv2/issues/2001
-      //   https://github.com/Exiv2/exiv2/pull/2008
-      //   https://github.com/Exiv2/exiv2/pull/2013
-      typeId = undefined;
-      size = 0;
-    } else {
+    if (count > std::numeric_limits<uint32_t>::max() / typeSize) {
+      throw Error(ErrorCode::kerArithmeticOverflow);
+    }
+    size_t size = typeSize * count;
+    uint32_t offset = getULong(p, byteOrder());
+    byte* pData = p;
+    if (size > 4 && Safe::add<size_t>(baseOffset(), offset) >= size_) {
+      // #1143
+      if (object->tag() == 0x2001 && std::string(groupName(object->group())) == "Sony1") {
+        // This tag is Exif.Sony1.PreviewImage, which refers to a preview image which is
+        // not stored in the metadata. Instead it is stored at the end of the file, after
+        // the main image. The value of `size` refers to the size of the preview image, not
+        // the size of the tag's payload, so we set it to zero here so that we don't attempt
+        // to read those bytes from the metadata. We currently leave this tag as "undefined",
+        // although we may attempt to handle it better in the future. More discussion of
+        // this issue can be found here:
+        //
+        //   https://github.com/Exiv2/exiv2/issues/2001
+        //   https://github.com/Exiv2/exiv2/pull/2008
+        //   https://github.com/Exiv2/exiv2/pull/2013
+        typeId = undefined;
+        size = 0;
+      } else {
 #ifndef SUPPRESS_WARNINGS
-      EXV_ERROR << "Offset of directory " << groupName(object->group()) << ", entry 0x" << std::setw(4)
-                << std::setfill('0') << std::hex << object->tag() << " is out of bounds: "
-                << "Offset = 0x" << std::setw(8) << std::setfill('0') << std::hex << offset
-                << "; truncating the entry\n";
+        EXV_ERROR << "Offset of directory " << groupName(object->group()) << ", entry 0x" << std::setw(4)
+                  << std::setfill('0') << std::hex << object->tag() << " is out of bounds: "
+                  << "Offset = 0x" << std::setw(8) << std::setfill('0') << std::hex << offset
+                  << "; truncating the entry\n";
 #endif
+      }
+      size = 0;
     }
-    size = 0;
-  }
-  if (size > 4) {
-    // setting pData to pData_ + baseOffset() + offset can result in pData pointing to invalid memory,
-    // as offset can be arbitrarily large
-    if ((static_cast<uintptr_t>(baseOffset()) >
-         std::numeric_limits<uintptr_t>::max() - static_cast<uintptr_t>(offset)) ||
-        (static_cast<uintptr_t>(baseOffset() + offset) >
-         std::numeric_limits<uintptr_t>::max() - reinterpret_cast<uintptr_t>(pData_))) {
-      throw Error(ErrorCode::kerCorruptedMetadata);  // #562 don't throw kerArithmeticOverflow
-    }
-    if (pData_ + static_cast<uintptr_t>(baseOffset()) + static_cast<uintptr_t>(offset) > pLast_) {
-      throw Error(ErrorCode::kerCorruptedMetadata);
-    }
-    pData = const_cast<byte*>(pData_) + baseOffset() + offset;
+    if (size > 4) {
+      // setting pData to pData_ + baseOffset() + offset can result in pData pointing to invalid memory,
+      // as offset can be arbitrarily large
+      if (Safe::add<size_t>(baseOffset(), offset) > static_cast<size_t>(pLast_ - pData_)) {
+        throw Error(ErrorCode::kerCorruptedMetadata);
+      }
+      pData = const_cast<byte*>(pData_) + baseOffset() + offset;
 
-    // check for size being invalid
-    if (size > static_cast<size_t>(pLast_ - pData)) {
+      // check for size being invalid
+      if (size > static_cast<size_t>(pLast_ - pData)) {
 #ifndef SUPPRESS_WARNINGS
-      EXV_ERROR << "Upper boundary of data for "
-                << "directory " << groupName(object->group()) << ", entry 0x" << std::setw(4) << std::setfill('0')
-                << std::hex << object->tag() << " is out of bounds: "
-                << "Offset = 0x" << std::setw(8) << std::setfill('0') << std::hex << offset << ", size = " << std::dec
-                << size
-                << ", exceeds buffer size by "
-                // cast to make MSVC happy
-                << static_cast<uint32_t>(pData + size - pLast_) << " Bytes; truncating the entry\n";
+        EXV_ERROR << "Upper boundary of data for "
+                  << "directory " << groupName(object->group()) << ", entry 0x" << std::setw(4) << std::setfill('0')
+                  << std::hex << object->tag() << " is out of bounds: "
+                  << "Offset = 0x" << std::setw(8) << std::setfill('0') << std::hex << offset << ", size = " << std::dec
+                  << size
+                  << ", exceeds buffer size by "
+                  // cast to make MSVC happy
+                  << static_cast<uint32_t>(pData + size - pLast_) << " Bytes; truncating the entry\n";
 #endif
-      size = 0;
+        size = 0;
+      }
     }
-  }
-  auto v = Value::create(typeId);
-  enforce(v != nullptr, ErrorCode::kerCorruptedMetadata);
-  v->read(pData, size, byteOrder());
-
-  object->setValue(std::move(v));
-  object->setData(pData, size, std::make_shared<DataBuf>());
-  object->setOffset(offset);
-  object->setIdx(nextIdx(object->group()));
+    auto v = Value::create(typeId);
+    enforce(v != nullptr, ErrorCode::kerCorruptedMetadata);
+    v->read(pData, size, byteOrder());
 
+    object->setValue(std::move(v));
+    object->setData(pData, size, std::make_shared<DataBuf>());
+    object->setOffset(offset);
+    object->setIdx(nextIdx(object->group()));
+  } catch (std::overflow_error&) {
+    throw Error(ErrorCode::kerCorruptedMetadata);  // #562 don't throw std::overflow_error
+  }
 }  // TiffReader::readTiffEntry
 
 void TiffReader::visitBinaryArray(TiffBinaryArray* object) {

test/data/test_reference_files/2018-01-09-exiv2-crash-003.tiff.out

@@ -1,7 +1,6 @@
 Error: Directory Image: Next pointer is out of bounds; ignored.
 Error: Offset of directory Image, entry 0x0146 is out of bounds: Offset = 0xbb000002; truncating the entry
 Error: Offset of directory Image, entry 0x83bb is out of bounds: Offset = 0x0000e120; truncating the entry
-Error: Offset of directory Image, entry 0x0146 is out of bounds: Offset = 0x00000000; truncating the entry
 Error: Offset of directory Image, entry 0x0146 is out of bounds: Offset = 0xbb000002; truncating the entry
 Error: Offset of directory Image, entry 0x83c2 is out of bounds: Offset = 0xf4000121; truncating the entry
 Error: Offset of directory Image, entry 0x0145 is out of bounds: Offset = 0x03000301; truncating the entry

test/data/test_reference_files/issue_ghsa_g44w_q3vm_gwjq_poc.jpg.out

@@ -1,5 +1,4 @@
 Error: Directory Image: Next pointer is out of bounds; ignored.
-Error: Offset of directory Image, entry 0x010e is out of bounds: Offset = 0x00000000; truncating the entry
 Warning: Directory Image, entry 0x0000 has unknown Exif (TIFF) type 0; setting type size 1.
 Warning: Directory Image, entry 0x0000 has unknown Exif (TIFF) type 0; setting type size 1.
 Warning: Directory Image, entry 0x0000 has unknown Exif (TIFF) type 0; setting type size 1.
@@ -11,5 +10,5 @@ Warning: Directory Image, entry 0x0000 has unknown Exif (TIFF) type 55938; setti
 Error: Directory Image, entry 0x0000 has invalid size 2550137344*1; skipping entry.
 Warning: Directory Image, entry 0x0000 has unknown Exif (TIFF) type 135; setting type size 1.
 Error: Directory Image, entry 0x0000 has invalid size 1761608704*1; skipping entry.
-Exif.Image.ImageDescription                  Ascii       0  
+Exif.Image.ImageDescription                  Ascii      45  MM  MM
 Exif.Image.IPTCNAA                           Rational    0  

tests/bugfixes/github/test_CVE_2017_14859.py

@@ -11,7 +11,7 @@ class TestCvePoC(metaclass=system_tests.CaseMeta):
     commands = ["$exiv2 " + filename]
     stdout = [""]
     stderr = ["""$exiv2_exception_message """ + filename + """:
-$kerCorruptedMetadata
+$kerFailedToReadImageData
 """]
     retval = [1]
 

tests/bugfixes/github/test_CVE_2017_14861.py

@@ -41,7 +41,7 @@ class TestCvePoC(metaclass=system_tests.CaseMeta):
     ]
     stderr = [
         """Error: Directory Image: Next pointer is out of bounds; ignored.
-Error: Offset of directory Image, entry 0x00fe is out of bounds: Offset = 0x00000000; truncating the entry
+Error: Upper boundary of data for directory Image, entry 0x00fe is out of bounds: Offset = 0x00000000, size = 177156, exceeds buffer size by 176816 Bytes; truncating the entry
 Error: Directory Image, entry 0x0100 has invalid size 1935897193*2; skipping entry.
 Warning: Directory Image, entry 0x303e has unknown Exif (TIFF) type 12320; setting type size 1.
 Error: Offset of directory Image, entry 0x0116 is out of bounds: Offset = 0x0011302a; truncating the entry

tests/bugfixes/github/test_CVE_2017_14862.py

@@ -1,23 +1,17 @@
 # -*- coding: utf-8 -*-
 
 import system_tests
-
+from system_tests import check_no_ASAN_UBSAN_errors
 
 class TestCvePoC(metaclass=system_tests.CaseMeta):
 
     url = "https://github.com/Exiv2/exiv2/issues/75"
 
     filename = "$data_path/008-invalid-mem"
-    commands = ["$exiv2 " + filename]
-    stdout = [""]
-    stderr = ["""$exiv2_exception_message """ + filename + """:
-$kerCorruptedMetadata
-"""]
-    retval = [1]
+    commands = ["$exiv2 -q " + filename]
+
+    stderr = [""]
+    retval = [0]
+
+    compare_stdout = check_no_ASAN_UBSAN_errors
 
-    def compare_stderr(self, i, command, got_stderr, expected_stderr):
-        """
-        Only check that an exception is thrown for this file
-        ignore all the warnings on stderr on purpose.
-        """
-        self.assertIn(expected_stderr, got_stderr)

tests/bugfixes/github/test_CVE_2017_14864.py

@@ -8,9 +8,9 @@ class TestCvePoC(metaclass=system_tests.CaseMeta):
     url = "https://github.com/Exiv2/exiv2/issues/73"
 
     filename = "$data_path/02-Invalid-mem-def"
-    commands = ["$exiv2 " + filename]
+    commands = ["$exiv2 -q " + filename]
     stdout = [""]
     stderr = ["""$exiv2_exception_message """ + filename + """:
-$kerCorruptedMetadata
+$kerFailedToReadImageData
 """]
     retval = [1]

tests/bugfixes/github/test_CVE_2018_12265.py

@@ -17,7 +17,6 @@ class AdditionOverflowInLoaderExifJpeg(metaclass=system_tests.CaseMeta):
         """Error: Upper boundary of data for directory Image, entry 0x00fe is out of bounds: Offset = 0x0000002a, size = 64, exceeds buffer size by 22 Bytes; truncating the entry
 Warning: Directory Image, entry 0x0201: Strip 0 is outside of the data area; ignored.
 Warning: Directory Image, entry 0x0201: Strip 7 is outside of the data area; ignored.
-Error: Offset of directory Thumbnail, entry 0x0201 is out of bounds: Offset = 0x00000000; truncating the entry
 """
     ]
     retval = [0]

tests/bugfixes/github/test_issue_ghsa_pvjp_m4f6_q984.py

@@ -17,16 +17,14 @@ class MinoltaDivZero(metaclass=CaseMeta):
 Error: Upper boundary of data for directory Photo, entry 0x8822 is out of bounds: Offset = 0x00000003, size = 56834, exceeds buffer size by 11577 Bytes; truncating the entry
 Error: Upper boundary of data for directory Photo, entry 0x8827 is out of bounds: Offset = 0x00000640, size = 1179650, exceeds buffer size by 1135990 Bytes; truncating the entry
 Warning: Directory Photo, entry 0x8832 has unknown Exif (TIFF) type 49; setting type size 1.
-Error: Offset of directory Sony2, entry 0x2006 is out of bounds: Offset = 0x00000000; truncating the entry
+Error: Upper boundary of data for directory Sony2, entry 0x2006 is out of bounds: Offset = 0x00000000, size = 46661636, exceeds buffer size by 46616376 Bytes; truncating the entry
 Warning: Directory Sony2, entry 0x20c1 has unknown Exif (TIFF) type 181; setting type size 1.
-Error: Offset of directory Sony2, entry 0x2063 is out of bounds: Offset = 0x00000000; truncating the entry
 Error: Offset of directory Sony2, entry 0x3000 is out of bounds: Offset = 0x0057097c; truncating the entry
-Error: Offset of directory Sony2, entry 0x0115 is out of bounds: Offset = 0x00000000; truncating the entry
+Error: Upper boundary of data for directory Sony2, entry 0x0115 is out of bounds: Offset = 0x00000000, size = 59768836, exceeds buffer size by 59723576 Bytes; truncating the entry
 Error: Upper boundary of data for directory Sony2, entry 0x2013 is out of bounds: Offset = 0x00000002, size = 37486596, exceeds buffer size by 37441338 Bytes; truncating the entry
 Warning: Directory Photo, entry 0xa003 has unknown Exif (TIFF) type 242; setting type size 1.
 Warning: Directory Iop has an unexpected next pointer; ignored.
 Warning: Directory Photo, entry 0xa402 has unknown Exif (TIFF) type 89; setting type size 1.
-Error: Offset of directory Photo, entry 0xa402 is out of bounds: Offset = 0x00000000; truncating the entry
 Error: Offset of directory Thumbnail, entry 0x0132 is out of bounds: Offset = 0xff00968b; truncating the entry
 """]
     retval = [0]