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.

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

CVE-2017-11683: reachable assertion in the Internal::TiffReader::visitDirectory function in tiffvisitor.cpp #57

Closed
rhertzog opened this Issue Aug 31, 2017 · 10 comments

Comments

Projects
None yet
3 participants
@rhertzog

rhertzog commented Aug 31, 2017

I'm forwarding a security vulnerability reported here:
https://bugzilla.redhat.com/show_bug.cgi?id=1475124

The file used to reproduce the issue is here:
https://bugzilla.redhat.com/attachment.cgi?id=1310025
(this is a rar archive containing the actual reproducer file)

Here's a copy of the report:

$./exiv2 POC

invalid type value detected in Image::printIFDStructure:  0
Error: Directory Image: Next pointer is out of bounds; ignored.
Warning: Directory Image, entry 0x0002 has unknown Exif (TIFF) type 0; setting type size 1.
Error: Upper boundary of data for directory Image, entry 0x0002 is out of bounds: Offset = 0x00000002, size = 65540, exceeds buffer size by 64830 Bytes; truncating the entry
Warning: Directory Image, entry 0x0000 has unknown Exif (TIFF) type 497; setting type size 1.
Error: Offset of directory Image, entry 0x0000 is out of bounds: Offset = 0x02297fba; truncating the entry
Warning: Directory Image, entry 0x4900 has unknown Exif (TIFF) type 18697; setting type size 1.
Error: Directory Image, entry 0x4900 has invalid size 524428033*1; skipping entry.
Warning: Directory Image, entry 0x8000 has unknown Exif (TIFF) type 65535; setting type size 1.
Error: Offset of directory Image, entry 0x8000 is out of bounds: Offset = 0xff7f0222; truncating the entry
Warning: Directory Image, entry 0x02ef has unknown Exif (TIFF) type 0; setting type size 1.
Error: Directory Image, entry 0x02ef has invalid size 1325435904*1; skipping entry.
Error: Offset of directory Image, entry 0x0149 is out of bounds: Offset = 0x03020200; truncating the entry
Warning: Directory Image, entry 0x8800 has unknown Exif (TIFF) type 65279; setting type size 1.
Warning: Directory Image, entry 0xff02 has unknown Exif (TIFF) type 4866; setting type size 1.
Error: Offset of directory Image, entry 0xff02 is out of bounds: Offset = 0x00007f00; truncating the entry
Warning: Directory Image, entry 0x0100 has unknown Exif (TIFF) type 2377; setting type size 1.
Error: Upper boundary of data for directory Image, entry 0x0100 is out of bounds: Offset = 0x00000100, size = 131401, exceeds buffer size by 130945 Bytes; truncating the entry
Warning: Directory Image, entry 0x0200 has unknown Exif (TIFF) type 514; setting type size 1.
Error: Directory Image, entry 0x0200 has invalid size 4278125056*1; skipping 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.
Error: Offset of directory Image, entry 0x0000 is out of bounds: Offset = 0x490a4901; truncating the entry
Warning: Directory Image, entry 0x0201 has unknown Exif (TIFF) type 0; setting type size 1.
Error: Offset of directory Image, entry 0x0201 is out of bounds: Offset = 0x02020000; truncating the entry
Warning: Directory Image, entry 0x0002 has unknown Exif (TIFF) type 65416; setting type size 1.
Error: Directory Image, entry 0x0002 has invalid size 284819469*1; skipping entry.
Warning: Directory Image, entry 0x0207 has unknown Exif (TIFF) type 767; setting type size 1.
Error: Offset of directory Image, entry 0x0207 is out of bounds: Offset = 0x007f5d00; truncating the entry
Warning: Directory Image, entry 0x0000 has unknown Exif (TIFF) type 18689; setting type size 1.
Error: Offset of directory Image, entry 0x0000 is out of bounds: Offset = 0x00010000; truncating the entry
Warning: Directory Image, entry 0x0000 has unknown Exif (TIFF) type 0; setting type size 1.
Error: Offset of directory Image, entry 0x0000 is out of bounds: Offset = 0x02020000; truncating the entry
Warning: Directory Image, entry 0x0200 has unknown Exif (TIFF) type 511; setting type size 1.
Error: Offset of directory Image, entry 0x0200 is out of bounds: Offset = 0x01010101; truncating the entry
Warning: Directory Image, entry 0x0101 has unknown Exif (TIFF) type 257; setting type size 1.
Error: Offset of directory Image, entry 0x0101 is out of bounds: Offset = 0x01010101; truncating the entry
Warning: Directory Image, entry 0x0101 has unknown Exif (TIFF) type 257; setting type size 1.
Error: Offset of directory Image, entry 0x0101 is out of bounds: Offset = 0x01010101; truncating the entry
Warning: Directory Image, entry 0x0101 has unknown Exif (TIFF) type 257; setting type size 1.
Error: Offset of directory Image, entry 0x0101 is out of bounds: Offset = 0x01010101; truncating the entry
Warning: Directory Image, entry 0x0101 has unknown Exif (TIFF) type 257; setting type size 1.
Error: Offset of directory Image, entry 0x0101 is out of bounds: Offset = 0x00007f00; truncating the entry
Warning: Directory Image, entry 0x0100 has unknown Exif (TIFF) type 2377; setting type size 1.
Error: Upper boundary of data for directory Image, entry 0x0100 is out of bounds: Offset = 0x00000100, size = 131401, exceeds buffer size by 130945 Bytes; truncating the entry
Warning: Directory Image, entry 0x0200 has unknown Exif (TIFF) type 514; setting type size 1.
Error: Directory Image, entry 0x0200 has invalid size 4143941632*1; skipping entry.
Warning: Directory Image, entry 0x0227 has unknown Exif (TIFF) type 1794; setting type size 1.
Error: Offset of directory Image, entry 0x0227 is out of bounds: Offset = 0x7f022202; truncating the entry
Warning: Directory Image, entry 0xefff has unknown Exif (TIFF) type 767; setting type size 1.
Error: Offset of directory Image, entry 0xefff is out of bounds: Offset = 0x02020202; truncating the entry
Warning: Directory Image, entry 0x1002 has unknown Exif (TIFF) type 6914; setting type size 1.
Error: Offset of directory Image, entry 0x1002 is out of bounds: Offset = 0x7f020202; truncating the entry
Warning: Directory Image, entry 0x4947 has unknown Exif (TIFF) type 14406; setting type size 1.
Error: Directory Image, entry 0x4947 has invalid size 587292985*1; skipping entry.
Warning: Directory Image, entry 0x0202 has unknown Exif (TIFF) type 32768; setting type size 1.
Error: Directory Image, entry 0x0202 has invalid size 2147680255*1; skipping entry.
Warning: Directory Image, entry 0x0201: Size or data offset value not set, ignoring them.
Warning: Directory Image, entry 0xfeff has unknown Exif (TIFF) type 256; setting type size 1.
Error: Offset of directory Image, entry 0xfeff is out of bounds: Offset = 0x0000fef5; truncating the entry
Warning: Directory Image, entry 0x0200 has unknown Exif (TIFF) type 18688; setting type size 1.
Error: Offset of directory Image, entry 0x0200 is out of bounds: Offset = 0x0201ff02; truncating the entry
Error: Directory Image, entry 0x0002 has invalid size 4278125129*1; skipping entry.
Error: Directory Image, entry 0x8825 Sub-IFD pointer 3 is out of bounds; ignoring it.
Error: Directory GPSInfo with 257 entries considered invalid; not read.
Error: Directory Iop with 18761 entries considered invalid; not read.
exiv2: tiffvisitor.cpp:1299: virtual void Exiv2::Internal::TiffReader::visitDirectory(Exiv2::Internal::TiffDirectory *): Assertion `tc.get()' failed.
Aborted


GDB debugging information is as follows:
(gdb) set args POC
(gdb) b tiffvisitor.cpp:1299 
Breakpoint 1 at 0x7ffff75c08bd: file tiffvisitor.cpp, line 1299.
(gdb) r
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /home/icy/real/exiv2-trunk/install/bin/exiv2 ../output/crashes/id:000034,sig:06,src:004666,op:int32,pos:198,val:be:+100
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
invalid type value detected in Image::printIFDStructure:  0

Breakpoint 1, Exiv2::Internal::TiffReader::visitDirectory (this=0x7fffffffd490, object=0x68c1b0) at tiffvisitor.cpp:1299
1299	            assert(tc.get());
(gdb) c 42 
Will ignore next 41 crossings of breakpoint 1.  Continuing.
Error: Directory Image: Next pointer is out of bounds; ignored.
Warning: Directory Image, entry 0x0002 has unknown Exif (TIFF) type 0; setting type size 1.
Error: Upper boundary of data for directory Image, entry 0x0002 is out of bounds: Offset = 0x00000002, size = 65540, exceeds buffer size by 64830 Bytes; truncating the entry
Warning: Directory Image, entry 0x0000 has unknown Exif (TIFF) type 497; setting type size 1.
Error: Offset of directory Image, entry 0x0000 is out of bounds: Offset = 0x02297fba; truncating the entry
Warning: Directory Image, entry 0x4900 has unknown Exif (TIFF) type 18697; setting type size 1.
Error: Directory Image, entry 0x4900 has invalid size 524428033*1; skipping entry.
Warning: Directory Image, entry 0x8000 has unknown Exif (TIFF) type 65535; setting type size 1.
Error: Offset of directory Image, entry 0x8000 is out of bounds: Offset = 0xff7f0222; truncating the entry
Warning: Directory Image, entry 0x02ef has unknown Exif (TIFF) type 0; setting type size 1.
Error: Directory Image, entry 0x02ef has invalid size 1325435904*1; skipping entry.
Error: Offset of directory Image, entry 0x0149 is out of bounds: Offset = 0x03020200; truncating the entry
Warning: Directory Image, entry 0x8800 has unknown Exif (TIFF) type 65279; setting type size 1.
Warning: Directory Image, entry 0xff02 has unknown Exif (TIFF) type 4866; setting type size 1.
Error: Offset of directory Image, entry 0xff02 is out of bounds: Offset = 0x00007f00; truncating the entry
Warning: Directory Image, entry 0x0100 has unknown Exif (TIFF) type 2377; setting type size 1.
Error: Upper boundary of data for directory Image, entry 0x0100 is out of bounds: Offset = 0x00000100, size = 131401, exceeds buffer size by 130945 Bytes; truncating the entry
Warning: Directory Image, entry 0x0200 has unknown Exif (TIFF) type 514; setting type size 1.
Error: Directory Image, entry 0x0200 has invalid size 4278125056*1; skipping 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.
Error: Offset of directory Image, entry 0x0000 is out of bounds: Offset = 0x490a4901; truncating the entry
Warning: Directory Image, entry 0x0201 has unknown Exif (TIFF) type 0; setting type size 1.
Error: Offset of directory Image, entry 0x0201 is out of bounds: Offset = 0x02020000; truncating the entry
Warning: Directory Image, entry 0x0002 has unknown Exif (TIFF) type 65416; setting type size 1.
Error: Directory Image, entry 0x0002 has invalid size 284819469*1; skipping entry.
Warning: Directory Image, entry 0x0207 has unknown Exif (TIFF) type 767; setting type size 1.
Error: Offset of directory Image, entry 0x0207 is out of bounds: Offset = 0x007f5d00; truncating the entry
Warning: Directory Image, entry 0x0000 has unknown Exif (TIFF) type 18689; setting type size 1.
Error: Offset of directory Image, entry 0x0000 is out of bounds: Offset = 0x00010000; truncating the entry
Warning: Directory Image, entry 0x0000 has unknown Exif (TIFF) type 0; setting type size 1.
Error: Offset of directory Image, entry 0x0000 is out of bounds: Offset = 0x02020000; truncating the entry
Warning: Directory Image, entry 0x0200 has unknown Exif (TIFF) type 511; setting type size 1.
Error: Offset of directory Image, entry 0x0200 is out of bounds: Offset = 0x01010101; truncating the entry
Warning: Directory Image, entry 0x0101 has unknown Exif (TIFF) type 257; setting type size 1.
Error: Offset of directory Image, entry 0x0101 is out of bounds: Offset = 0x01010101; truncating the entry
Warning: Directory Image, entry 0x0101 has unknown Exif (TIFF) type 257; setting type size 1.
Error: Offset of directory Image, entry 0x0101 is out of bounds: Offset = 0x01010101; truncating the entry
Warning: Directory Image, entry 0x0101 has unknown Exif (TIFF) type 257; setting type size 1.
Error: Offset of directory Image, entry 0x0101 is out of bounds: Offset = 0x01010101; truncating the entry
Warning: Directory Image, entry 0x0101 has unknown Exif (TIFF) type 257; setting type size 1.
Error: Offset of directory Image, entry 0x0101 is out of bounds: Offset = 0x00007f00; truncating the entry
Warning: Directory Image, entry 0x0100 has unknown Exif (TIFF) type 2377; setting type size 1.
Error: Upper boundary of data for directory Image, entry 0x0100 is out of bounds: Offset = 0x00000100, size = 131401, exceeds buffer size by 130945 Bytes; truncating the entry
Warning: Directory Image, entry 0x0200 has unknown Exif (TIFF) type 514; setting type size 1.
Error: Directory Image, entry 0x0200 has invalid size 4143941632*1; skipping entry.
Warning: Directory Image, entry 0x0227 has unknown Exif (TIFF) type 1794; setting type size 1.
Error: Offset of directory Image, entry 0x0227 is out of bounds: Offset = 0x7f022202; truncating the entry
Warning: Directory Image, entry 0xefff has unknown Exif (TIFF) type 767; setting type size 1.
Error: Offset of directory Image, entry 0xefff is out of bounds: Offset = 0x02020202; truncating the entry
Warning: Directory Image, entry 0x1002 has unknown Exif (TIFF) type 6914; setting type size 1.
Error: Offset of directory Image, entry 0x1002 is out of bounds: Offset = 0x7f020202; truncating the entry
Warning: Directory Image, entry 0x4947 has unknown Exif (TIFF) type 14406; setting type size 1.
Error: Directory Image, entry 0x4947 has invalid size 587292985*1; skipping entry.
Warning: Directory Image, entry 0x0202 has unknown Exif (TIFF) type 32768; setting type size 1.
Error: Directory Image, entry 0x0202 has invalid size 2147680255*1; skipping entry.
Warning: Directory Image, entry 0x0201: Size or data offset value not set, ignoring them.
Warning: Directory Image, entry 0xfeff has unknown Exif (TIFF) type 256; setting type size 1.
Error: Offset of directory Image, entry 0xfeff is out of bounds: Offset = 0x0000fef5; truncating the entry
Warning: Directory Image, entry 0x0200 has unknown Exif (TIFF) type 18688; setting type size 1.
Error: Offset of directory Image, entry 0x0200 is out of bounds: Offset = 0x0201ff02; truncating the entry
Error: Directory Image, entry 0x0002 has invalid size 4278125129*1; skipping entry.
Error: Directory Image, entry 0x8825 Sub-IFD pointer 3 is out of bounds; ignoring it.
Error: Directory GPSInfo with 257 entries considered invalid; not read.
Error: Directory Iop with 18761 entries considered invalid; not read.

Breakpoint 1, Exiv2::Internal::TiffReader::visitDirectory (this=0x7fffffffd490, object=0x68b020) at tiffvisitor.cpp:1299
1299	            assert(tc.get());
(gdb) n
exiv2: tiffvisitor.cpp:1299: virtual void Exiv2::Internal::TiffReader::visitDirectory(Exiv2::Internal::TiffDirectory *): Assertion `tc.get()' failed.

Program received signal SIGABRT, Aborted.
0x00007ffff66901c7 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:55
55	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) 
(gdb) bt
#0  0x00007ffff66901c7 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:55
#1  0x00007ffff6691e2a in __GI_abort () at abort.c:89
#2  0x00007ffff66890bd in __assert_fail_base (fmt=0x7ffff67eaf78 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
    assertion=assertion@entry=0x7ffff770d9d2 "tc.get()", file=file@entry=0x7ffff770ccc2 "tiffvisitor.cpp", 
    line=line@entry=1299, 
    function=function@entry=0x7ffff770d8d7 "virtual void Exiv2::Internal::TiffReader::visitDirectory(Exiv2::Internal::TiffDirectory *)") at assert.c:92
#3  0x00007ffff6689172 in __GI___assert_fail (assertion=0x7ffff770d9d2 "tc.get()", 
    file=0x7ffff770ccc2 "tiffvisitor.cpp", line=1299, 
    function=0x7ffff770d8d7 "virtual void Exiv2::Internal::TiffReader::visitDirectory(Exiv2::Internal::TiffDirectory *)") at assert.c:101
#4  0x00007ffff75c17ba in Exiv2::Internal::TiffReader::visitDirectory (this=<optimized out>, object=<optimized out>)
    at tiffvisitor.cpp:1299
#5  0x00007ffff758842a in Exiv2::Internal::TiffDirectory::doAccept (this=0x68b020, visitor=...) at tiffcomposite.cpp:916
#6  0x00007ffff758883d in Exiv2::Internal::TiffComponent::accept (this=0x68b020, visitor=...) at tiffcomposite.cpp:891
#7  Exiv2::Internal::TiffSubIfd::doAccept (this=0x68b290, visitor=...) at tiffcomposite.cpp:931
#8  0x00007ffff758850c in Exiv2::Internal::TiffComponent::accept (this=0x68b290, visitor=...) at tiffcomposite.cpp:891
#9  Exiv2::Internal::TiffDirectory::doAccept (this=0x68c1b0, visitor=...) at tiffcomposite.cpp:919
#10 0x00007ffff7588268 in Exiv2::Internal::TiffComponent::accept (this=0x68c1b0, visitor=...) at tiffcomposite.cpp:891
#11 0x00007ffff759f7d4 in Exiv2::Internal::TiffParserWorker::parse (pData=<optimized out>, size=<optimized out>, 
    root=<optimized out>, pHeader=<optimized out>) at tiffimage.cpp:2011
#12 0x00007ffff759bf9f in Exiv2::Internal::TiffParserWorker::decode (exifData=..., iptcData=..., xmpData=..., 
    pData=0x7ffff7ff4000 "II*", size=712, root=131072, findDecoderFct=0x2c8, pHeader=<optimized out>)
    at tiffimage.cpp:1900
---Type <return> to continue, or q <return> to quit---
#13 0x00007ffff75995fa in Exiv2::TiffParser::decode (exifData=..., iptcData=..., xmpData=..., 
    pData=0x7ffff7ff4000 "II*", size=712) at tiffimage.cpp:260
#14 Exiv2::TiffImage::readMetadata (this=0x68c000) at tiffimage.cpp:192
#15 0x0000000000426ecb in Action::Print::printSummary (this=0x68bd10) at actions.cpp:289
#16 0x0000000000426a4c in Action::Print::run (this=0x68bd10, path=...) at actions.cpp:244
#17 0x00000000004078c0 in main (argc=<optimized out>, argv=<optimized out>) at exiv2.cpp:170

This vulnerability was triggered after the TiffReader::visitDirectory(TiffDirectory* object) at tiffvisitor.cpp:1299

1260     void TiffReader::visitDirectory(TiffDirectory* object)
 ...
1286         for (uint16_t i = 0; i < n; ++i) {
1287             if (p + 12 > pLast_) {
1288 #ifndef SUPPRESS_WARNINGS
1289                 EXV_ERROR << "Directory " << groupName(object->group())
1290                           << ": IFD entry " << i
1291                           << " lies outside of the data buffer.\n";
1292 #endif
1293                 return;
1294             }
1295             uint16_t tag = getUShort(p, byteOrder());
1296             TiffComponent::AutoPtr tc = TiffCreator::create(tag, object->group());
1297             // The assertion typically fails if a component is not configured in
1298             // the TIFF structure table
1299             assert(tc.get());
1300             tc->setStart(p);
1301             object->addChild(tc);
1302             p += 12;
1303         }
 ...

This vulnerability is detected by team OWL337, with our custom fuzzer collAFL. Please contact ganshuitao@gmail.com and chaoz@tsinghua.edu.cn if you need more info about the team, the tool or the vulnerability.

@rhertzog

This comment has been minimized.

Show comment
Hide comment
@rhertzog

rhertzog Aug 31, 2017

Actually, this seems to be also reported here: http://dev.exiv2.org/issues/1307

rhertzog commented Aug 31, 2017

Actually, this seems to be also reported here: http://dev.exiv2.org/issues/1307

@rhertzog

This comment has been minimized.

Show comment
Hide comment
@rhertzog

rhertzog Sep 29, 2017

Looking at the comment here it looks like the assertion is somewhat expected when encountering components not registered in the TIFF structure table. So I guess that we should just ignore them and skip over them. Or throw a warning. Here's a possible patch:

--- a/src/tiffvisitor.cpp
+++ b/src/tiffvisitor.cpp
@@ -1290,11 +1290,12 @@ namespace Exiv2 {
             }
             uint16_t tag = getUShort(p, byteOrder());
             TiffComponent::AutoPtr tc = TiffCreator::create(tag, object->group());
-            // The assertion typically fails if a component is not configured in
-            // the TIFF structure table
-            assert(tc.get());
-            tc->setStart(p);
-            object->addChild(tc);
+            if (tc.get()) {
+                tc->setStart(p);
+                object->addChild(tc);
+            } else {
+               EXV_WARNING << "Unable to handle tag " << tag << ".\n";
+            }
             p += 12;
         }
 

What do you think?

rhertzog commented Sep 29, 2017

Looking at the comment here it looks like the assertion is somewhat expected when encountering components not registered in the TIFF structure table. So I guess that we should just ignore them and skip over them. Or throw a warning. Here's a possible patch:

--- a/src/tiffvisitor.cpp
+++ b/src/tiffvisitor.cpp
@@ -1290,11 +1290,12 @@ namespace Exiv2 {
             }
             uint16_t tag = getUShort(p, byteOrder());
             TiffComponent::AutoPtr tc = TiffCreator::create(tag, object->group());
-            // The assertion typically fails if a component is not configured in
-            // the TIFF structure table
-            assert(tc.get());
-            tc->setStart(p);
-            object->addChild(tc);
+            if (tc.get()) {
+                tc->setStart(p);
+                object->addChild(tc);
+            } else {
+               EXV_WARNING << "Unable to handle tag " << tag << ".\n";
+            }
             p += 12;
         }
 

What do you think?

@clanmills

This comment has been minimized.

Show comment
Hide comment
@clanmills

clanmills Sep 29, 2017

Collaborator

Thanks, Raffaele. That's one tough little section of code. However, I know exactly what it does. I'll write some notes about this in the airport.

I think it's reasonable to spit out a warning and not assert. After all, nobody has ever survived a visit! So saying "I cannot accept you as valid metadata" seems rational and safe.

Although I have said that I will retire, I'm going to continue with the project as new Team Members come up to speed. Very happy with how this week has progressed. Thanks for your contribution.

Collaborator

clanmills commented Sep 29, 2017

Thanks, Raffaele. That's one tough little section of code. However, I know exactly what it does. I'll write some notes about this in the airport.

I think it's reasonable to spit out a warning and not assert. After all, nobody has ever survived a visit! So saying "I cannot accept you as valid metadata" seems rational and safe.

Although I have said that I will retire, I'm going to continue with the project as new Team Members come up to speed. Very happy with how this week has progressed. Thanks for your contribution.

@clanmills

This comment has been minimized.

Show comment
Hide comment
@clanmills

clanmills Sep 29, 2017

Collaborator

I've fixed this already (about a couple of months ago). I explained that to Mr Salo in http://dev.exiv2.org/issues/1307

I'll add POC to our test suite as a regression detector.

502 rmills@rmillsmbp:~ $ exiv2 ~/Desktop/POC
invalid type value detected in Image::printIFDStructure:  0
Exiv2 exception in print action for file /Users/rmills/Desktop/POC:
invalid type value detected in Image::printIFDStructure
503 rmills@rmillsmbp:~ $ 

I'm going to close this (and http://dev.exiv2.org/issues/1307), however I'm happy to continue discussing this if you wish.

Collaborator

clanmills commented Sep 29, 2017

I've fixed this already (about a couple of months ago). I explained that to Mr Salo in http://dev.exiv2.org/issues/1307

I'll add POC to our test suite as a regression detector.

502 rmills@rmillsmbp:~ $ exiv2 ~/Desktop/POC
invalid type value detected in Image::printIFDStructure:  0
Exiv2 exception in print action for file /Users/rmills/Desktop/POC:
invalid type value detected in Image::printIFDStructure
503 rmills@rmillsmbp:~ $ 

I'm going to close this (and http://dev.exiv2.org/issues/1307), however I'm happy to continue discussing this if you wish.

@clanmills clanmills closed this Sep 29, 2017

clanmills added a commit that referenced this issue Sep 29, 2017

@rhertzog

This comment has been minimized.

Show comment
Hide comment
@rhertzog

rhertzog Sep 29, 2017

It would be nice if you could explain how you fixed it... because the assert() at least is still here in the master branch:

assert(tc.get());
And I can certainly reproduce a crash with 0.26. Maybe you are not reaching the problematic code path due to changes somewhere else but maybe it's still possible to reach that code if we tweak the fuzzed file.

Furthermore in order to backport security fixes, we need to know which commit fixed which CVE. Closing without explanation is not really helpful for distributors who have to fix CVE in old released versions.

rhertzog commented Sep 29, 2017

It would be nice if you could explain how you fixed it... because the assert() at least is still here in the master branch:

assert(tc.get());
And I can certainly reproduce a crash with 0.26. Maybe you are not reaching the problematic code path due to changes somewhere else but maybe it's still possible to reach that code if we tweak the fuzzed file.

Furthermore in order to backport security fixes, we need to know which commit fixed which CVE. Closing without explanation is not really helpful for distributors who have to fix CVE in old released versions.

@clanmills

This comment has been minimized.

Show comment
Hide comment
@clanmills

clanmills Sep 29, 2017

Collaborator

I'm glad you asked. It was fixed in image.cpp#377

                // Break for unknown tag types else we may segfault.
                if ( !typeValid(type) ) {
                    std::cerr << "invalid type value detected in Image::printIFDStructure:  " << type << std::endl;
                    start = 0; // break from do loop
                    throw Error(56);
                    break; // break from for loop
                }

I attach my drawing of the architecture of a TIFF. It lists the types of data in a tag (1= Byte, 2 = Ascii etc). This file has an illegal value in the type field. So I throw.

I'm sure if you get somebody to go through our code, you'll find more and more ways to break in. The code has been written to process "legal" files and now you're testing me to destruction. I'm rather worried that the number of checks required to deal with every possible way into the code could be larger than Exiv2. That's why I've proposed libimagelint to do this before I open the file. All those checks (and there could thousands of them) need to be performed by all code that parses images.

Collaborator

clanmills commented Sep 29, 2017

I'm glad you asked. It was fixed in image.cpp#377

                // Break for unknown tag types else we may segfault.
                if ( !typeValid(type) ) {
                    std::cerr << "invalid type value detected in Image::printIFDStructure:  " << type << std::endl;
                    start = 0; // break from do loop
                    throw Error(56);
                    break; // break from for loop
                }

I attach my drawing of the architecture of a TIFF. It lists the types of data in a tag (1= Byte, 2 = Ascii etc). This file has an illegal value in the type field. So I throw.

I'm sure if you get somebody to go through our code, you'll find more and more ways to break in. The code has been written to process "legal" files and now you're testing me to destruction. I'm rather worried that the number of checks required to deal with every possible way into the code could be larger than Exiv2. That's why I've proposed libimagelint to do this before I open the file. All those checks (and there could thousands of them) need to be performed by all code that parses images.

@rhertzog

This comment has been minimized.

Show comment
Hide comment
@rhertzog

rhertzog Oct 5, 2017

FWIW there's nothing attached to your github comment. But your comment seems to just confirm what I said... the assertion can likely still be reached by using the same fuzzed file but by ensuring we have a legal "type" value (to bypass the new check that you added), no?

Yes, the checks are likely to be numerous but there's no way to get around them when you code in C/C++ if you want your code to work securely in all circumstances.

rhertzog commented Oct 5, 2017

FWIW there's nothing attached to your github comment. But your comment seems to just confirm what I said... the assertion can likely still be reached by using the same fuzzed file but by ensuring we have a legal "type" value (to bypass the new check that you added), no?

Yes, the checks are likely to be numerous but there's no way to get around them when you code in C/C++ if you want your code to work securely in all circumstances.

@clanmills

This comment has been minimized.

Show comment
Hide comment
@clanmills

clanmills Oct 5, 2017

Collaborator

OK. I attach the drawing. @D4N is working on the security issues you have reported and I expect he'll close them soon. I'm on vacation at the moment and have limited time. However, we're working on your issues.

Tiff.pdf

Collaborator

clanmills commented Oct 5, 2017

OK. I attach the drawing. @D4N is working on the security issues you have reported and I expect he'll close them soon. I'm on vacation at the moment and have limited time. However, we're working on your issues.

Tiff.pdf

D4N added a commit to D4N/exiv2 that referenced this issue Oct 5, 2017

Use nullptr check instead of assertion, by Raphaël Hertzog
Source:
Exiv2#57 (comment)

tc can be a null pointer when the TIFF tag is unknown (the factory
then returns an auto_ptr(0)) => as this can happen for corrupted
files, an explicit check should be used because an assertion can be
turned of in release mode (with NDEBUG defined)

D4N added a commit to D4N/exiv2 that referenced this issue Oct 5, 2017

Use nullptr check instead of assertion, by Raphaël Hertzog
Source:
Exiv2#57 (comment)

tc can be a null pointer when the TIFF tag is unknown (the factory
then returns an auto_ptr(0)) => as this can happen for corrupted
files, an explicit check should be used because an assertion can be
turned of in release mode (with NDEBUG defined)

This also fixes #57
@D4N

This comment has been minimized.

Show comment
Hide comment
@D4N

D4N Oct 5, 2017

Contributor

I think Raphaël's patch should be merged anyway, as it ensures that no null pointer is dereferenced when assertions are turned off. I have therefore prepared a PR: #107.

Contributor

D4N commented Oct 5, 2017

I think Raphaël's patch should be merged anyway, as it ensures that no null pointer is dereferenced when assertions are turned off. I have therefore prepared a PR: #107.

D4N added a commit to D4N/exiv2 that referenced this issue Oct 15, 2017

D4N added a commit to D4N/exiv2 that referenced this issue Oct 15, 2017

Use nullptr check instead of assertion, by Raphaël Hertzog
Source:
Exiv2#57 (comment)

tc can be a null pointer when the TIFF tag is unknown (the factory
then returns an auto_ptr(0)) => as this can happen for corrupted
files, an explicit check should be used because an assertion can be
turned of in release mode (with NDEBUG defined)

This also fixes #57

dirkmueller added a commit to dirkmueller/exiv2 that referenced this issue Oct 17, 2017

Use nullptr check instead of assertion, by Raphaël Hertzog
Source:
Exiv2#57 (comment)

tc can be a null pointer when the TIFF tag is unknown (the factory
then returns an auto_ptr(0)) => as this can happen for corrupted
files, an explicit check should be used because an assertion can be
turned of in release mode (with NDEBUG defined)

This also fixes #57

(cherry picked from commit 1f1715c)

dirkmueller added a commit to dirkmueller/exiv2 that referenced this issue Oct 17, 2017

Use nullptr check instead of assertion, by Raphaël Hertzog
Source:
Exiv2#57 (comment)

tc can be a null pointer when the TIFF tag is unknown (the factory
then returns an auto_ptr(0)) => as this can happen for corrupted
files, an explicit check should be used because an assertion can be
turned of in release mode (with NDEBUG defined)

This also fixes #57

(cherry picked from commit 1f1715c)
@D4N

This comment has been minimized.

Show comment
Hide comment
@D4N

D4N Oct 18, 2017

Contributor

The patches for this issue were backported to the 0.26 branch.

Contributor

D4N commented Oct 18, 2017

The patches for this issue were backported to the 0.26 branch.

dirkmueller added a commit to dirkmueller/exiv2 that referenced this issue Jan 7, 2018

Fix Exiv2#57
(cherry picked from commit c90991c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment