From 62705a6615d82425965037df2c2845d56f922edd Mon Sep 17 00:00:00 2001 From: Kevin Backhouse Date: Sun, 23 Oct 2022 20:04:42 +0100 Subject: [PATCH] Use Safe::add() in tiffvisitor_int.cpp --- src/tiffvisitor_int.cpp | 176 +++++++++--------- .../2018-01-09-exiv2-crash-003.tiff.out | 1 - .../issue_ghsa_g44w_q3vm_gwjq_poc.jpg.out | 3 +- tests/bugfixes/github/test_CVE_2017_14859.py | 2 +- tests/bugfixes/github/test_CVE_2017_14861.py | 2 +- tests/bugfixes/github/test_CVE_2017_14862.py | 20 +- tests/bugfixes/github/test_CVE_2017_14864.py | 4 +- tests/bugfixes/github/test_CVE_2018_12265.py | 1 - .../github/test_issue_ghsa_pvjp_m4f6_q984.py | 6 +- 9 files changed, 101 insertions(+), 114 deletions(-) diff --git a/src/tiffvisitor_int.cpp b/src/tiffvisitor_int.cpp index a1aabd6231..96ffc06025 100644 --- a/src/tiffvisitor_int.cpp +++ b/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::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::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(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(baseOffset()) > - std::numeric_limits::max() - static_cast(offset)) || - (static_cast(baseOffset() + offset) > - std::numeric_limits::max() - reinterpret_cast(pData_))) { - throw Error(ErrorCode::kerCorruptedMetadata); // #562 don't throw kerArithmeticOverflow - } - if (pData_ + static_cast(baseOffset()) + static_cast(offset) > pLast_) { - throw Error(ErrorCode::kerCorruptedMetadata); - } - pData = const_cast(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(baseOffset(), offset) > static_cast(pLast_ - pData_)) { + throw Error(ErrorCode::kerCorruptedMetadata); + } + pData = const_cast(pData_) + baseOffset() + offset; - // check for size being invalid - if (size > static_cast(pLast_ - pData)) { + // check for size being invalid + if (size > static_cast(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(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(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()); - 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()); + 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) { diff --git a/test/data/test_reference_files/2018-01-09-exiv2-crash-003.tiff.out b/test/data/test_reference_files/2018-01-09-exiv2-crash-003.tiff.out index fe0c997a29..12373fe126 100644 --- a/test/data/test_reference_files/2018-01-09-exiv2-crash-003.tiff.out +++ b/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 diff --git a/test/data/test_reference_files/issue_ghsa_g44w_q3vm_gwjq_poc.jpg.out b/test/data/test_reference_files/issue_ghsa_g44w_q3vm_gwjq_poc.jpg.out index 25ff80d478..894d5008da 100644 --- a/test/data/test_reference_files/issue_ghsa_g44w_q3vm_gwjq_poc.jpg.out +++ b/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 diff --git a/tests/bugfixes/github/test_CVE_2017_14859.py b/tests/bugfixes/github/test_CVE_2017_14859.py index ff7073d0b6..01c10df175 100644 --- a/tests/bugfixes/github/test_CVE_2017_14859.py +++ b/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] diff --git a/tests/bugfixes/github/test_CVE_2017_14861.py b/tests/bugfixes/github/test_CVE_2017_14861.py index 9ca9e90e8d..bda9d8367c 100644 --- a/tests/bugfixes/github/test_CVE_2017_14861.py +++ b/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 diff --git a/tests/bugfixes/github/test_CVE_2017_14862.py b/tests/bugfixes/github/test_CVE_2017_14862.py index 6d6205dc9c..7b065cde2b 100644 --- a/tests/bugfixes/github/test_CVE_2017_14862.py +++ b/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) diff --git a/tests/bugfixes/github/test_CVE_2017_14864.py b/tests/bugfixes/github/test_CVE_2017_14864.py index f0deef9756..15e88ef24a 100644 --- a/tests/bugfixes/github/test_CVE_2017_14864.py +++ b/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] diff --git a/tests/bugfixes/github/test_CVE_2018_12265.py b/tests/bugfixes/github/test_CVE_2018_12265.py index 0b73c73c7c..735e67ceaa 100644 --- a/tests/bugfixes/github/test_CVE_2018_12265.py +++ b/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] diff --git a/tests/bugfixes/github/test_issue_ghsa_pvjp_m4f6_q984.py b/tests/bugfixes/github/test_issue_ghsa_pvjp_m4f6_q984.py index 1a81c59faa..c6bc601a4a 100644 --- a/tests/bugfixes/github/test_issue_ghsa_pvjp_m4f6_q984.py +++ b/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]