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

[WIP] Backporting security fixes to 0.26 #120

Merged
merged 18 commits into from Oct 16, 2017

Conversation

Projects
None yet
4 participants
@D4N
Copy link
Contributor

D4N commented Oct 15, 2017

I have backported the security fixes of the recent weeks to the newly created 0.26 branch.

Unfortunately, the bugfixes-test.out file was impossible to merge, I'll try to recreate it manually (the main issue here is, that some debug output was removed/changed since 0.26). However, none of the reproducers should result in crashes any more.

For completeness sake, the following issues were fixed:

  • CVE-2017-14864, CVE-2017-14862 and CVE-2017-14859 (#73, #74 and #75)
  • CVE-2017-14860 (#71)
  • CVE-2017-11337, CVE-2017-11338, CVE-2017-11339, CVE-2017-11340, CVE-2017-11553, CVE-2017-12955, CVE-2017-12956, CVE-2017-12957 (#50, #51, #52, #53, #54, #58, #59 and #60)
  • CVE-2017-11683 (#57)
  • CVE-2017-11592 (#56)
  • CVE-2017-11591 (#55)

D4N and others added some commits Sep 26, 2017

Adding missing test file.
(cherry picked from commit 9aad5cd)
Fix #55
(cherry picked from commit 6e3855a)
Fixed wrong brackets: size*count + pad can overflow before the cast
=> Should fix #76 (most of the work has been done by Robin Mills in
   6e3855a)

The problem with #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.
Add POC3, POC4, POC5, POC6, POC9, POC11, POC12 & POC13 to the test suite
These are files which reproduce the github issues #50, #51, #52, #53,
 #54, #58, #59 and #60
Use nullptr check instead of assertion, by Raphaël Hertzog
Source:
#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
Fix for CVE-2017-14860
A heap buffer overflow could occur in memcpy when icc.size_ is larger
than data.size_ - pad, as then memcpy would read out of bounds of data.

This commit adds a sanity check to iccLength (= icc.size_): if it is
larger than data.size_ - pad (i.e. an overflow would be caused) an
exception is thrown.

This fixes #71.
Fix for CVE-2017-14864, CVE-2017-14862 and CVE-2017-14859
The invalid memory dereference in
Exiv2::getULong()/Exiv2::StringValueBase::read()/Exiv2::DataValue::read()
is caused further up the call-stack, by
v->read(pData, size, byteOrder) in TiffReader::readTiffEntry()
passing an invalid pData pointer (pData points outside of the Tiff
file). pData can be set out of bounds in the (size > 4) branch where
baseOffset() and offset are added to pData_ without checking whether
the result is still in the file. As offset comes from an untrusted
source, an attacker can craft an arbitrarily large offset into the
file.

This commit adds a check into the problematic branch, whether the
result of the addition would be out of bounds of the Tiff
file. Furthermore the whole operation is checked for possible
overflows.
Added exception throw on Value pointer being null
v can be null if the typeId is invalid => throw an exception notifying
the user that his file is corrupted instead of the assertion

@D4N D4N requested a review from clanmills Oct 15, 2017

@clanmills
Copy link
Collaborator

clanmills left a comment

Thank You very much for doing this work. Not only do I appreciate this effort, I'm sure @rhertzog and the security folks will agree.

@clanmills

This comment has been minimized.

Copy link
Collaborator

clanmills commented Oct 15, 2017

Crap! I see one of the checks has failed. You're not quite there yet - however you will be soon.

@piponazo

This comment has been minimized.

Copy link
Collaborator

piponazo commented Oct 16, 2017

It seems that it was a failure in travis-ci (yes ... sometimes it fails). I just re-triggered the jobs to see if they pass now.

@piponazo

This comment has been minimized.

Copy link
Collaborator

piponazo commented Oct 16, 2017

Oh damn it! It seems that one of the PPAs that we are using to provision software in the Travis-CI virtual machines is discontinued. I will try to fix the situation in other Pull Request ASAP

@piponazo

This comment has been minimized.

Copy link
Collaborator

piponazo commented Oct 16, 2017

Oh ... I was not looking at this correctly.

Of course, in 0.26 we did not have all the travis-ci stuff. Actually travis should not run any check on this PR since there is not .travis.yml file in the branch ...

I will merge this since we already checked those fixes in master.

@piponazo piponazo merged commit 2d957cc into Exiv2:0.26 Oct 16, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@piponazo

This comment has been minimized.

Copy link
Collaborator

piponazo commented Oct 16, 2017

Thanks for taking care of this! 🥇

@c0deaddict c0deaddict referenced this pull request Jun 2, 2018

Closed

Vulnerability roundup 40 (release-18.03) #39366

11 of 27 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment