Insufficient verification(cycle) in function Image::printIFDStructure in image.cpp:335

[cool-tomato]

I found a bug in function printIFDStructure () in image.cpp:335. In the big do...while loop, there may be more than one IFD structures, namely multiple images. But there is a situation, if the offsets of the second IFD is same as the before, the program would go to infinite loop.

This can be reproduced with cmd:
./exiv2 -pR poc
The poc can be found at here.

[clanmills]

The option -pR is for debugging. In Exiv2 v0.27, option -pR will throw an exception on NDEBUG builds. We will also modify the code to trap this "fuzzed" file so that it doesn't loop on Debug builds.

[clanmills]

@cool-tomato We will fix this in Exiv2 v0.27 RC3 which is scheduled to be released on Friday 7 December 2018. We're busy today releasing Exiv2 v0.27 RC2 and don't want to loose focus.

[cool-tomato]

Oh, I got it, thank you.

[piponazo]

An update about this. When removing the option -pR from the release builds few tests fail on CI:

# Tests failing in the python suite
FAIL: test_run (bugfixes.github.test_CVE_2018_9145.SubBoxLengthDataBufAbort)
FAIL: test_run (bugfixes.github.test_issue_216.UncontrolledRecursion)
FAIL: test_run (bugfixes.github.test_issue_511.ThrowsWhenIFDsAreMalformed)
FAIL: test_run (bugfixes.redmine.test_issue_1108.CheckDumpSubFiles)

#Some other tests failing in webp-test.out

@D4N Do you remember if you put something in place in the python framework to skip tests in a particular compilation mode ?

Right now we are just compiling and running the tests in CI in Release mode. A temporary solution would be to skip these tests for the moment, and enable them back after the 0.27 Release with new job configurations in Travis and Appveyor to compile the project and run the tests in Debug mode. I would not try to setup the CI scripts now to build and run stuff in Debug mode, since it will probably take lot of work.

[clanmills]

I'll add an option to the Jenkins script to build/test in debug. You're right that we should test this before v0.27. I don't think we should delay GM. I'll review your code this afternoon.

[D4N]

@D4N Do you remember if you put something in place in the python framework to skip tests in a particular compilation mode ?

There is no such explicit option. If the build type is accessible via an environment variable, then you can just do a:

@unittest.skipif(os.getenv('BUILDTYPE') == 'Release')
class Test:
    pass

I was considering to add something more convenient to the test suite, but didn't have a good idea how to integrate that yet.

@clanmills What about the -pS option? Doesn't that invoke printIFDStructure too?

[clanmills]

There are several of those printXXXX villains that should throw in NDEBUG builds.

I think printIFDStructure() is only called when we use option kpsRecursive, but I can't remember! (it's my age, you know).

We can detect a debug build with the command bin/exiv2 -vVg debug, perhaps we can skip by detecting that. Or better during initialisation of the test suite, we could use os.setenv to set BUILDTYPE based on the output from bin/exiv2 -vVg debug

[clanmills]

Somehow the fix to exiv2 -pR to block release builds isn't in v0.27 RC3. I'll investigate.

[clanmills]

I'm mistaken. The fix for -pR is in RC3 (and RC4).

The issue with the test file poc.dms is caused by the tiff directory referring to itself and causes a loop. I've added code to detect such a loop. I'm going to reclose this issue.

[piponazo]

Should this issue then be closed? I see that you closed it and reopened it consecutively.

I double checked this, and it is fine on master (the -pR option does not work on Release).

[clanmills]

I changed my mind about closing it. I'd like to you inspect the code when I get 0.27-RC4 finished.

[clanmills]

@piponazo I've rewritten the dict_t dicts_t stuff (and removed the static which is clearly wrong).

In the image object I've added Reservations which is a vector. A Reservation is a pair<at,length>. When we parse tiff data in printIFD, with every read, I update the Reservations. For example, as we move along the chain of tiff directories, I update the reservations. If the tiff has been fuzzed to create a circular reference, it'll already be in Reservations and we throw an exception. That fixes poc.dms (and several other pocs).

Disabling option -pR in exiv2/Release is OK, however this change hardens the library code.

[clanmills]

We abandoned 0.27-RC4 because changes to the JP2 code broke the test suite when investigating #590

The code in 0.27-RC4 to detect a cyclic recursion in the "fuzzed" tiff will be added in 0.27.1 as a new PR.

#590 is a collection of issues. They will be addressed for v0.27.1.

[piponazo]

@clanmills What's the plan about this? Do you think it is still needed to address the issue in the library code? or now that the option -pR is gone in release mode could we drop it ?

[clanmills]

Fix submitted: #1210