CVE-2017-11591: Floating point exception in the Exiv2::ValueType function about exiv2

[rhertzog]

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]

Thanks, Raphael. I’ll review, test and submit this after dinner. Thanks for helping. You know, this team-work stuff really works!

…

On 26 Sep 2017, at 16:42, Raphaël Hertzog ***@***.***> wrote: 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<> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#55 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgWPlStv1XfjnPyRnwe7jKY2OaQELrOks5smRt5gaJpZM4PI73C>.

[clanmills]

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]

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)

[rhertzog]

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]

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

[clanmills]

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]

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

[carnil]

On Wed, Oct 18, 2017 at 11:10:07PM +0000, D4N wrote: This has been fixed on the master and backported to the 0.26 branch.

Which commit in master fixes the issue?

[D4N]

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