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.

Sign up for GitHub

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

Jump to bottom

Backport security fixes from 0.26 #123

Merged
merged 17 commits into from
Jan 7, 2018
Merged

Backport security fixes from 0.26 #123

merged 17 commits into from
Jan 7, 2018

Conversation

dirkmueller
Copy link

No description provided.

D4N and others added 9 commits October 17, 2017 13:52
@D4N @dirkmueller
Added missing include for INT_MIN & INT_MAX constants
70e15f5 
(cherry picked from commit d4e7510)
@clanmills @dirkmueller
Fix Exiv2#55 Thank You, Raphaël Hertzog.
df7f9b2 
(cherry picked from commit c037d73)
@clanmills @dirkmueller
Fix Exiv2#55
f271200 
(cherry picked from commit 6e3855a)
@D4N @dirkmueller
Use nullptr check instead of assertion, by Raphaël Hertzog
7b84183 
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 Exiv2#57

(cherry picked from commit 1f1715c)
@D4N @dirkmueller
Added new error message to warn about corrupted metadata
30269d3 
(cherry picked from commit a2f25c9)
@D4N @dirkmueller
Added arithmetic operation overflow error
c30abab 
(cherry picked from commit da67c16)
@D4N @dirkmueller
Fix for CVE-2017-14864, CVE-2017-14862 and CVE-2017-14859
fd85c16 
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.

(cherry picked from commit d4e4288)
@D4N @dirkmueller
Added check for overflows in calculation of size
dd3ad85 
(cherry picked from commit c2c9fab)
@D4N @dirkmueller
Added exception throw on Value pointer being null
336c9c6 
v can be null if the typeId is invalid => throw an exception notifying
the user that his file is corrupted instead of the assertion

(cherry picked from commit 1841c2a)
@D4N D4N self-assigned this Oct 17, 2017
@D4N
Copy link
Member

D4N commented Oct 17, 2017

Hi, thanks for cherry-picking the commits! I'll take a look at it later today or tomorrow.

Have you tried building exiv2 and the reproducers for the vulnerabilities?

@dirkmueller
Copy link
Author

I did try building, but I haven't tried the reproducers (it wasn't entirely obvious to me which reproducers apply where). I'll give this some further try later.

@D4N
Copy link
Member

D4N commented Oct 17, 2017

The reproducers are in located in test/data/ and should have been added by #120 to the 0.26 branch. They are called 00* and POC*.

Ideally the testsuite (run it via make test in the test directory) should be clean, but that involves updating a huge binary file which I have failed to yet. However, if exiv2 does not crash with ASAN enabled, it should be fine.

@dirkmueller
Copy link
Author

here's what I did:

cmake .. -DCMAKE_CXX_FLAGS="-O2 -fsanitize=address"
for i in /tmp/data/*; do bin/exiv2 $i 2>&1 | grep -a -i asan && echo $i; done
/tmp/data/exiv2-bug1080.jpg

none of the data/00* or data/POC* files trigger. there seems to be a missing fix for bug1080.

@clanmills @dirkmueller
Exiv2#1080 Fix submitted.
ed42388 
(cherry picked from commit 54ac67d)
@dirkmueller
Copy link
Author

so I added the fix for that one as well, now the above loop for a copy of the data dir from master is clean.

@D4N
Copy link
Member

D4N commented Oct 17, 2017 via email

@D4N
Copy link
Member

D4N commented Oct 18, 2017

Please also cherry pick the following commits: 73a4cdb, 0f3e646, c90991c, 751312f, fd3711f and 1b01704

You'll get an error on every single one of these for the file test/data/bugfixes-test.out. Don't bother with merging it, just take the version from 0.25 (and not from the commits you cherry pick). I'll then send you an updated version, so that the testsuite runs without issues.

@dirkmueller
Copy link
Author

I'll get to this in the week after next week.

clanmills and others added 7 commits January 7, 2018 14:28
@clanmills @dirkmueller
Adding missing test file.
fe7e791 
(cherry picked from commit 9aad5cd)
(cherry picked from commit 73a4cdb)
@clanmills @dirkmueller
Exiv2#49 and Exiv2#79 Thanks to Dan and Raffael for working with me o…
c479027 
…n this.

(cherry picked from commit 0f3e646)
@clanmills @dirkmueller
Fix Exiv2#57
1822871 
(cherry picked from commit c90991c)
@D4N @dirkmueller
Add POC3, POC4, POC5, POC6, POC9, POC11, POC12 & POC13 to the test suite
fae9762 
These are files which reproduce the github issues Exiv2#50, Exiv2#51, Exiv2#52, Exiv2#53,
 Exiv2#54, Exiv2#58, Exiv2#59 and Exiv2#60

(cherry picked from commit 751312f)
@D4N @dirkmueller
Added the reproducer for CVE-2017-14860 to the test suite
f0d659a 
(cherry picked from commit fd3711f)
@D4N @dirkmueller
Added reproducers for Exiv2#73, Exiv2#74 and Exiv2#75 to the test suite
541be8b 
(cherry picked from commit 1b01704)
@D4N @dirkmueller
Made tiff-test.sh executable
62c5a07
@D4N D4N merged commit caff2c8 into Exiv2:0.25 Jan 7, 2018
@D4N
Copy link
Member

D4N commented Jan 7, 2018

Thanks for your work!

@clanmills
Copy link
Collaborator

Thank you for doing this work. Lots of great team work here. Thank You all very much.

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

@D4N D4N D4N approved these changes

Assignees

@D4N D4N

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dirkmueller @D4N @clanmills