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.

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

Fixed wrong brackets: size*count + pad can overflow before the cast #79

Merged
merged 1 commit into from
Sep 29, 2017

Conversation

D4N
Copy link
Member

@D4N D4N commented Sep 27, 2017

Should fix #76 for the current git head.
However the credit goes to Robin Mills (@clanmills), as he mostly fixed it in 6e3855a.

The problem with #76 is the contents of the 26th TIFF IFD, with the following contents:
tag: 0x8649
type: 0x1
count: 0xffff ffff
offset: 0x4974

The issue is the size of count (uint32_t), as adding anything to it causes an overflow. Especially the expression:
(size*count + pad+20) (from here)
results in an overflow and gives 20 as a result instead of 0x100000014, thus the condition in the if in the next line is false and the program continues to run (until it crashes at io.read where the overflow does not occur).

To properly account for the overflow, the brackets have to be removed, as then the result is saved in the correctly sized type and not cast after being calculated in the smaller type.

The brackets have also been removed from bigtiffimage.cpp, where the same issue is present.

I am not 100% sure, if this fully fixes #76, as it was reported against v0.26 and I fixed in in the current git head. I'll try to cherry pick the necessary patches onto v0.26 and see if they fix it there, too.

=> Should fix Exiv2#76 (most of the work has been done by Robin Mills in
   6e3855a)

The problem with Exiv2#76 is the contents of the 26th IFD, with the
following contents:
tag: 0x8649
type: 0x1
count: 0xffff ffff
offset: 0x4974

The issue is the size of count (uint32_t), as adding anything to it
causes an overflow. Especially the expression:
(size*count + pad+20)
results in an overflow and gives 20 as a result instead of
0x100000014, thus the condition in the if in the next line is false
and the program continues to run (until it crashes at io.read).

To properly account for the overflow, the brackets have to be removed,
as then the result is saved in the correctly sized type and not cast
after being calculated in the smaller type.

The brackets have also been removed from bigtiffimage.cpp, where the
same issue is present.
@D4N
Copy link
Member Author

D4N commented Sep 28, 2017

I have cherry-picked the respective commits on top of the commit tagged with v0.26 and pushed them here. The invalid free does then no longer appear in v0.26.

Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. There's similar code in bigtiffimage.cpp. bigtiffimage.cpp is in development and not used in production yet, however it's almost identical code. One day bigtiffimage.cpp will replace tiffimage.cpp (assuming the work involved doesn't kill Michal!).

  2. I'm amazed by this fix. I put in the brackets in an effort to tell the compiler "Do this in 64 bit". I was worried that your version would say "cast size" and do whatever you want with the rest of the expression!

  3. If you are confident that the whole expression is 64 bit, I'm happy. However please deal with bigtiffimage.cpp

@D4N
Copy link
Member Author

D4N commented Sep 29, 2017

  1. I believe I have applied the patch also to src/bigtiffimage.cpp (it should actually show up in the diff). Or have I missed something?
  2. The brackets are actually the culprit here. This is (imho) an oddity in the C standard, but if you do (long) (a*b), a*b is computed using the standard integer promotion rules. So if you get an overflow in a*b, you'll get it in (long)(a*b) too, even if a long would be large enough. In contrast, if you do (long)a*b, a*b is computed as a long and not in a smaller type (assuming sizeof(a,b) < sizeof(long)). The nasty thing about this is, that not many people know about this (including myself about an hour before I submitted the patch) and (as far as i know) there is no way how to do this with C++ style cast (e.g. static_cast).
  3. 64 bit should be enough. We know that size is between 1 and 8 and pad is either 0 or 1. The only value that can be arbitrarily large is count, thus the largest value that the expression size*count + pad+20 can produce is 0x80000000d, which fits into 64 bits.

@clanmills clanmills merged commit 272fc46 into Exiv2:master Sep 29, 2017
@clanmills
Copy link
Collaborator

This is really good. I've updated the test suite with the file POC2 from #49. 7fa8d31

543 rmills@rmillsmbp:~/gnu/github/exiv2/exiv2 $ exiv2 test/data/POC8
Exiv2 exception in print action for file test/data/POC8:
invalid memory allocation request
544 rmills@rmillsmbp:~/gnu/github/exiv2/exiv2 $ exiv2 test/data/POC2
Exiv2 exception in print action for file test/data/POC2:
invalid memory allocation request
545 rmills@rmillsmbp:~/gnu/github/exiv2/exiv2 $ 

I think we've fixed two issues with this. Unusual. It's more common to discover more issues with one fix. Occasionally we win and get two issues with one fix. Maybe it's an omen that VS11 will crash into the Atlantic this evening!

@D4N
Copy link
Member Author

D4N commented Oct 1, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bad free in Exiv2::Image::~Image (image.cpp:173)
2 participants