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-11591: Floating point exception in the Exiv2::ValueType function #55

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

Comments

Projects
None yet
5 participants
@rhertzog

rhertzog commented Aug 31, 2017

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

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

Here's a copy of the report:

$./exiv2 POC8

[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Error: Directory Image, entry 0x0000 has invalid size 4286578688*8; skipping entry.
Warning: Directory Image, entry 0x0111: Strip 0 is outside of the data area; ignored.

Program received signal SIGFPE, Arithmetic exception.
0x000000000085bd64 in Exiv2::ValueType<std::pair<int, int> >::toLong(long) const ()
(gdb) bt
#0  0x000000000085bd64 in Exiv2::ValueType<std::pair<int, int> >::toLong(long) const ()
#1  0x000000000069e74b in Exiv2::Internal::TiffImageEntry::setStrips(Exiv2::Value const*, unsigned char const*, unsigned int, unsigned int) ()
#2  0x00000000006d87f2 in Exiv2::Internal::TiffReader::readDataEntryBase(Exiv2::Internal::TiffDataEntryBase*) ()
#3  0x00000000006a7226 in Exiv2::Internal::TiffDirectory::doAccept(Exiv2::Internal::TiffVisitor&) ()
#4  0x00000000006a6f45 in Exiv2::Internal::TiffComponent::accept(Exiv2::Internal::TiffVisitor&) ()
#5  0x00000000006c0618 in Exiv2::Internal::TiffParserWorker::parse(unsigned char const*, unsigned int, unsigned int, Exiv2::Internal::TiffHeaderBase*) ()
#6  0x00000000006bbd00 in Exiv2::Internal::TiffParserWorker::decode(Exiv2::ExifData&, Exiv2::IptcData&, Exiv2::XmpData&, unsigned char const*, unsigned int, unsigned int, void (Exiv2::Internal::TiffDecoder::*(*)(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unsigned int, Exiv2::Internal::IfdId))(Exiv2::Internal::TiffEntryBase const*), Exiv2::Internal::TiffHeaderBase*) ()
#7  0x00000000006b901f in Exiv2::TiffImage::readMetadata() ()
#8  0x0000000000464434 in Action::Print::printSummary() ()
#9  0x0000000000463e5c in Action::Print::run(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
#10 0x0000000000439762 in main ()

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 Sep 26, 2017

This error actually boils down to a problematic integer division: -2147483648 / -1
The result is bigger than INT_MAX and thus the calculation must be done on a long (not an integer that is later cast into a long). Here's my suggested patch. We don't have the same problem with URational as the unsigned nature makes it impossible to overflow the integer.

$ quilt diff
--- a/include/exiv2/value.hpp
+++ b/include/exiv2/value.hpp
@@ -1664,7 +1664,7 @@ namespace Exiv2 {
     {
         ok_ = (value_[n].second != 0);
         if (!ok_) return 0;
-        return value_[n].first / value_[n].second;
+        return (long)value_[n].first / value_[n].second;
     }
     // Specialization for unsigned rational
     template<>

rhertzog commented Sep 26, 2017

This error actually boils down to a problematic integer division: -2147483648 / -1
The result is bigger than INT_MAX and thus the calculation must be done on a long (not an integer that is later cast into a long). Here's my suggested patch. We don't have the same problem with URational as the unsigned nature makes it impossible to overflow the integer.

$ quilt diff
--- a/include/exiv2/value.hpp
+++ b/include/exiv2/value.hpp
@@ -1664,7 +1664,7 @@ namespace Exiv2 {
     {
         ok_ = (value_[n].second != 0);
         if (!ok_) return 0;
-        return value_[n].first / value_[n].second;
+        return (long)value_[n].first / value_[n].second;
     }
     // Specialization for unsigned rational
     template<>
@clanmills

This comment has been minimized.

Show comment
Hide comment
@clanmills

clanmills Sep 26, 2017

Collaborator
Collaborator

clanmills commented Sep 26, 2017

@clanmills

This comment has been minimized.

Show comment
Hide comment
@clanmills

clanmills Sep 26, 2017

Collaborator

Raphael

You're in the right place and have the right idea. Your fix isn't correct on MacOS-X. It's been a long day. I'll get this fixed and submitted tomorrow.

I'm really delighted that you've made the effort to investigate this. Thank You very much.

Robin

Collaborator

clanmills commented Sep 26, 2017

Raphael

You're in the right place and have the right idea. Your fix isn't correct on MacOS-X. It's been a long day. I'll get this fixed and submitted tomorrow.

I'm really delighted that you've made the effort to investigate this. Thank You very much.

Robin

@clanmills clanmills closed this in 8a8f60a Sep 26, 2017

@clanmills

This comment has been minimized.

Show comment
Hide comment
@clanmills

clanmills Sep 26, 2017

Collaborator

This matter isn't finished. Several issues:

  1. POC8 crashes on Linux and Visual Studio 2015 because printIFDStructure is being tricked into allocating a huge memory block. I'll add a check that he never attempts to request more memory than the size of the input file.
  2. INT_MIN and INT_MAX in include are -64k and 64k. I'll use LONG_MIN and LONG_MAX instead.
  3. ValueType::toLong(long n) should test ULONG_MAX

I'll add the code in the morning (2017-09-27)

Collaborator

clanmills commented Sep 26, 2017

This matter isn't finished. Several issues:

  1. POC8 crashes on Linux and Visual Studio 2015 because printIFDStructure is being tricked into allocating a huge memory block. I'll add a check that he never attempts to request more memory than the size of the input file.
  2. INT_MIN and INT_MAX in include are -64k and 64k. I'll use LONG_MIN and LONG_MAX instead.
  3. ValueType::toLong(long n) should test ULONG_MAX

I'll add the code in the morning (2017-09-27)

@clanmills clanmills reopened this Sep 26, 2017

@clanmills clanmills closed this in 6e3855a Sep 27, 2017

@rhertzog

This comment has been minimized.

Show comment
Hide comment
@rhertzog

rhertzog Sep 27, 2017

INT_MIN and INT_MAX in include are -64k and 64k. I'll use LONG_MIN and LONG_MAX instead.

What platform has such broken values? I would argue that the platform must be fixed... using
an arbitrary limit like your LARGE_INT seems rather ugly.

Also I don't understand why you make the function return 0 instead of throwing an error like in other places.

rhertzog commented Sep 27, 2017

INT_MIN and INT_MAX in include are -64k and 64k. I'll use LONG_MIN and LONG_MAX instead.

What platform has such broken values? I would argue that the platform must be fixed... using
an arbitrary limit like your LARGE_INT seems rather ugly.

Also I don't understand why you make the function return 0 instead of throwing an error like in other places.

@Kicer86

This comment has been minimized.

Show comment
Hide comment
@Kicer86

Kicer86 Sep 27, 2017

Contributor

How about using std::numeric_limits? It holds real limits and other stuff for each fundamental type.

Contributor

Kicer86 commented Sep 27, 2017

How about using std::numeric_limits? It holds real limits and other stuff for each fundamental type.

@clanmills

This comment has been minimized.

Show comment
Hide comment
@clanmills

clanmills Sep 27, 2017

Collaborator

You are welcome to investigate this. MacOSX wouldn't compile the code using #include LONG_MIN/LONG_MAX, so I invented LARGE_INT and that seems to be good enough.

I didn't write this code, so I don't know why it returns 0. The code has been untouched for years. If we start throwing exceptions, we'll probably cause "ripple" in the test suite.

Please remember that this code is there to prevent a crash from a fuzzed file (a file with deliberately corrupted data). The code I submitted this morning is sufficient for that extreme case. Under normal conditions, the previous code has been in service for years without incident.

I sent an email to you and Luis about the priorities (from my point of view). Items 1 and 2 in that list are very important. 1 = Integrating clanmills/exiv2 into exiv2/exiv2. 2 = Teaching me to work properly with GitHub. So, while I'm happy for you to look into this matter, I'd prefer to focus on the high priority matters. If we could resolve 1 and 2 before I leave for the States on Friday afternoon, life will be good.

Collaborator

clanmills commented Sep 27, 2017

You are welcome to investigate this. MacOSX wouldn't compile the code using #include LONG_MIN/LONG_MAX, so I invented LARGE_INT and that seems to be good enough.

I didn't write this code, so I don't know why it returns 0. The code has been untouched for years. If we start throwing exceptions, we'll probably cause "ripple" in the test suite.

Please remember that this code is there to prevent a crash from a fuzzed file (a file with deliberately corrupted data). The code I submitted this morning is sufficient for that extreme case. Under normal conditions, the previous code has been in service for years without incident.

I sent an email to you and Luis about the priorities (from my point of view). Items 1 and 2 in that list are very important. 1 = Integrating clanmills/exiv2 into exiv2/exiv2. 2 = Teaching me to work properly with GitHub. So, while I'm happy for you to look into this matter, I'd prefer to focus on the high priority matters. If we could resolve 1 and 2 before I leave for the States on Friday afternoon, life will be good.

D4N added a commit to D4N/exiv2 that referenced this issue Sep 28, 2017

Kicer86 added a commit to Kicer86/exiv2 that referenced this issue Oct 4, 2017

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

Fix Exiv2#55
(cherry picked from commit 6e3855a)

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

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

Fix Exiv2#55
(cherry picked from commit 6e3855a)

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

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

Fix Exiv2#55
(cherry picked from commit 6e3855a)
@D4N

This comment has been minimized.

Show comment
Hide comment
@D4N

D4N Oct 18, 2017

Contributor

This has been fixed on the master and backported to the 0.26 branch.

Contributor

D4N commented Oct 18, 2017

This has been fixed on the master and backported to the 0.26 branch.

@carnil

This comment has been minimized.

Show comment
Hide comment
@carnil

carnil Oct 19, 2017

carnil commented Oct 19, 2017

@D4N

This comment has been minimized.

Show comment
Hide comment
@D4N

D4N Oct 19, 2017

Contributor

@carnil Again very much related to #54, so the same commits should apply.

Contributor

D4N commented Oct 19, 2017

@carnil Again very much related to #54, so the same commits should apply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment