Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix infinite loop in Image::printIFDStructure (backport #1778) #1780

Merged
merged 3 commits into from
Jul 16, 2021
Merged

Fix infinite loop in Image::printIFDStructure (backport #1778) #1780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
093dca0
Regression test for https://github.com/Exiv2/exiv2/security/advisorie…
kevinbackhouse Jul 13, 2021
caf08b2
dirLength == 0 can cause an infinite loop.
kevinbackhouse Jul 13, 2021
e4e3362
Defensive programming in Image::printIFDStructure
kevinbackhouse Jul 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 52 additions & 32 deletions src/image.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "image.hpp"
#include "image_int.hpp"
#include "error.hpp"
#include "enforce.hpp"
#include "futils.hpp"
#include "safe_op.hpp"
#include "slice.hpp"
Expand Down Expand Up @@ -137,6 +138,19 @@ namespace {
// *****************************************************************************
// class member definitions
namespace Exiv2 {
// 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);
}

Image::Image(int imageType, uint16_t supportedMetadata, BasicIo::UniquePtr io)
: io_(std::move(io)),
pixelWidth_(0),
Expand Down Expand Up @@ -328,12 +342,11 @@ namespace Exiv2 {

do {
// Read top of directory
const int seekSuccess = !io.seek(start,BasicIo::beg);
const long bytesRead = io.read(dir.pData_, 2);
if (!seekSuccess || bytesRead == 0) {
throw Error(kerCorruptedMetadata);
}
seekOrThrow(io, start, BasicIo::beg, kerCorruptedMetadata);
readOrThrow(io, dir.pData_, 2, kerCorruptedMetadata);
uint16_t dirLength = byteSwap2(dir,0,bSwap);
// Prevent infinite loops. (GHSA-m479-7frc-gqqg)
enforce(dirLength > 0, kerCorruptedMetadata);

if ( dirLength > 500 ) // tooBig
throw Error(kerTiffDirectoryTooLarge);
Expand All @@ -356,7 +369,7 @@ namespace Exiv2 {
}
bFirst = false;

io.read(dir.pData_, 12);
readOrThrow(io, dir.pData_, 12, kerCorruptedMetadata);
uint16_t tag = byteSwap2(dir,0,bSwap);
uint16_t type = byteSwap2(dir,2,bSwap);
uint32_t count = byteSwap4(dir,4,bSwap);
Expand Down Expand Up @@ -389,20 +402,27 @@ namespace Exiv2 {
// if ( offset > io.size() ) offset = 0; // Denial of service?

// #55 and #56 memory allocation crash test/data/POC8
long long allocate = static_cast<long long>(size) * count + pad + 20;
if (allocate > static_cast<long long>(io.size())) {
const uint64_t allocate64 = static_cast<uint64_t>(size) * count + pad + 20;
if ( allocate64 > io.size() ) {
throw Error(kerInvalidMalloc);
}
DataBuf buf(static_cast<long>(allocate)); // allocate a buffer
// Overflow check
enforce(allocate64 <= static_cast<uint64_t>(std::numeric_limits<uint32_t>::max()), kerCorruptedMetadata);
enforce(allocate64 <= static_cast<uint64_t>(std::numeric_limits<long>::max()), kerCorruptedMetadata);
hassec marked this conversation as resolved.
Show resolved Hide resolved
const long allocate = static_cast<long>(allocate64);
DataBuf buf(allocate); // allocate a buffer
std::memset(buf.pData_, 0, buf.size_);
std::memcpy(buf.pData_,dir.pData_+8,4); // copy dir[8:11] into buffer (short strings)
const bool bOffsetIsPointer = count*size > 4;

// We have already checked that this multiplication cannot overflow.
const uint32_t count_x_size = count*size;
const bool bOffsetIsPointer = count_x_size > 4;

if ( bOffsetIsPointer ) { // read into buffer
size_t restore = io.tell(); // save
io.seek(offset,BasicIo::beg); // position
io.read(buf.pData_,count*size);// read
io.seek(restore,BasicIo::beg); // restore
const long restore = io.tell(); // save
seekOrThrow(io, offset, BasicIo::beg, kerCorruptedMetadata); // position
readOrThrow(io, buf.pData_, static_cast<long>(count_x_size), kerCorruptedMetadata); // read
seekOrThrow(io, restore, BasicIo::beg, kerCorruptedMetadata); // restore
}

if ( bPrint ) {
Expand Down Expand Up @@ -441,49 +461,49 @@ namespace Exiv2 {

if ( option == kpsRecursive && (tag == 0x8769 /* ExifTag */ || tag == 0x014a/*SubIFDs*/ || type == tiffIfd) ) {
for ( size_t k = 0 ; k < count ; k++ ) {
size_t restore = io.tell();
const long restore = io.tell();
offset = byteSwap4(buf,k*size,bSwap);
printIFDStructure(io,out,option,offset,bSwap,c,depth);
io.seek(restore,BasicIo::beg);
seekOrThrow(io, restore, BasicIo::beg, kerCorruptedMetadata);
}
} else if ( option == kpsRecursive && tag == 0x83bb /* IPTCNAA */ ) {

if (static_cast<size_t>(Safe::add(count, offset)) > io.size()) {
throw Error(kerCorruptedMetadata);
}

const size_t restore = io.tell();
io.seek(offset, BasicIo::beg); // position
const long restore = io.tell();
seekOrThrow(io, offset, BasicIo::beg, kerCorruptedMetadata); // position
std::vector<byte> bytes(count) ; // allocate memory
// TODO: once we have C++11 use bytes.data()
const long read_bytes = io.read(&bytes[0], count);
io.seek(restore, BasicIo::beg);
readOrThrow(io, &bytes[0], count, kerCorruptedMetadata);
seekOrThrow(io, restore, BasicIo::beg, kerCorruptedMetadata);
// TODO: once we have C++11 use bytes.data()
IptcData::printStructure(out, makeSliceUntil(&bytes[0], read_bytes), depth);
IptcData::printStructure(out, makeSliceUntil(&bytes[0], count), depth);

} else if ( option == kpsRecursive && tag == 0x927c /* MakerNote */ && count > 10) {
size_t restore = io.tell(); // save
const long restore = io.tell(); // save

uint32_t jump= 10 ;
byte bytes[20] ;
const auto chars = reinterpret_cast<const char*>(&bytes[0]);
io.seek(offset,BasicIo::beg); // position
io.read(bytes,jump ) ; // read
seekOrThrow(io, offset, BasicIo::beg, kerCorruptedMetadata); // position
readOrThrow(io, bytes, jump, kerCorruptedMetadata) ; // read
bytes[jump]=0 ;
if ( ::strcmp("Nikon",chars) == 0 ) {
// tag is an embedded tiff
auto bytes2 = new byte[count - jump]; // allocate memory
io.read(bytes2,count-jump) ; // read
MemIo memIo(bytes2,count-jump) ; // create a file
const long byteslen = count-jump;
DataBuf bytes(byteslen); // allocate a buffer
readOrThrow(io, bytes.pData_, byteslen, kerCorruptedMetadata); // read
MemIo memIo(bytes.pData_, byteslen) ; // create a file
printTiffStructure(memIo,out,option,depth);
delete[] bytes2 ; // free
} else {
// tag is an IFD
io.seek(0,BasicIo::beg); // position
seekOrThrow(io, 0, BasicIo::beg, kerCorruptedMetadata); // position
printIFDStructure(io,out,option,offset,bSwap,c,depth);
}

io.seek(restore,BasicIo::beg); // restore
seekOrThrow(io, restore, BasicIo::beg, kerCorruptedMetadata); // restore
}
}

Expand All @@ -496,7 +516,7 @@ namespace Exiv2 {
}
}
if ( start ) {
io.read(dir.pData_, 4);
readOrThrow(io, dir.pData_, 4, kerCorruptedMetadata);
start = byteSwap4(dir,0,bSwap);
}
} while (start) ;
Expand All @@ -516,7 +536,7 @@ namespace Exiv2 {
DataBuf dir(dirSize);

// read header (we already know for certain that we have a Tiff file)
io.read(dir.pData_, 8);
readOrThrow(io, dir.pData_, 8, kerCorruptedMetadata);
char c = static_cast<char>(dir.pData_[0]);
bool bSwap = ( c == 'M' && isLittleEndianPlatform() )
|| ( c == 'I' && isBigEndianPlatform() )
Expand Down
Binary file added test/data/issue_ghsa_m479_7frc_gqqg_poc.crw
View file
Open in desktop
Binary file not shown.
18 changes: 18 additions & 0 deletions tests/bugfixes/github/test_issue_ghsa_m479_7frc_gqqg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# -*- coding: utf-8 -*-

from system_tests import CaseMeta, path, check_no_ASAN_UBSAN_errors

class ImagePrintIFDStructure(metaclass=CaseMeta):
"""
Regression test for the bug described in:
https://github.com/Exiv2/exiv2/security/advisories/GHSA-m479-7frc-gqqg
"""
url = "https://github.com/Exiv2/exiv2/security/advisories/GHSA-m479-7frc-gqqg"

filename = path("$data_path/issue_ghsa_m479_7frc_gqqg_poc.crw")
commands = ["$exiv2 -p C $filename"]
stdout = [""]
stderr = ["""Exiv2 exception in print action for file $filename:
$kerCorruptedMetadata
"""]
retval = [1]