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

Add libFuzzer integration + report bug #943

Closed
wants to merge 1 commit into from

Conversation

@yevgenypats
Copy link
Contributor

commented Jul 3, 2019

This commit places the basics for libFuzzer integration with one
fuzzer which fuzzes the readMetadata function. The fuzzer is
located at fuzz/read-metadata.

To add more fuzzers please add them to ./fuzz directory as
described in the README.

Also a memory corruption bug is found using this fuzzer which
might lead to additional bugs after fix is pushed.

@yevgenypats

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

Attached is the crash for the read-metadata.cpp fuzzer + screenshot of the backtrace.

I think this should also be given a CVE for a memory corruption vulnerability which affects programs that use the API to read bytes and not a file.

I'll be happy to contribute more fuzzers after this bug is fixed so it won't fail the other fuzzers.

Also, I'll be happy to contribute an integration to Fuzzit (I'm the CEO of the company) which will enable continuous fuzzing for the project for free as it's an open-source project. you can read also about systemd case study here @clanmills

Not sure who is the right person to review the PR. but CCing @kevinbackhouse as you merged a CVE bugfix lately.

Cheers,
Yevgeny

Screenshot from 2019-07-03 14-30-27

crash-e3569314e80df00b576bc49baca44f593ef78985

@kevinbackhouse

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

Please could you include exact reproduction steps? What command line did you run this with?

@codecov

This comment has been minimized.

Copy link

commented Jul 3, 2019

Codecov Report

Merging #943 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #943   +/-   ##
=======================================
  Coverage   70.95%   70.95%           
=======================================
  Files         147      147           
  Lines       19288    19288           
=======================================
  Hits        13685    13685           
  Misses       5603     5603

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a803ce3...a7791ab. Read the comment docs.

@yevgenypats

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

Sure. here it is (clang >6.0 must be installed).

cd <exivdir>
mkdir build
cd build
export CXX=clang++
export CC=clang
cmake .. -G "Unix Makefiles" "-DCMAKE_BUILD_TYPE=Debug"  "-DEXIV2_BUILD_FUZZ_TESTS=ON"
make -j4
./bin/read-metadata
fuzz/CMakeLists.txt Outdated Show resolved Hide resolved

@yevgenypats yevgenypats force-pushed the fuzzitdev:libfuzzer_integration branch from 00b07d9 to 65e4359 Jul 3, 2019

CMakeLists.txt Outdated Show resolved Hide resolved

@yevgenypats yevgenypats force-pushed the fuzzitdev:libfuzzer_integration branch from 65e4359 to 77fda42 Jul 3, 2019

@kevinbackhouse

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

@yevgenypats: I cannot reproduce this on the command line:

$ ./build/bin/exiv2 60588206-f9a14480-9d9e-11e9-981a-977e0741bb42.png 
Exiv2 exception in print action for file 60588206-f9a14480-9d9e-11e9-981a-977e0741bb42.png:
Failed to read image data

I also stepped through it in the debugger and all it does is throw an exception at mrwimage.cpp:123.

@yevgenypats

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@kevinbackhouse Yes, because you tried to reproduce with exiv2 which is a bit different because it's reading from a file i.e using this api: ImageFactory::open(const std::string& path, bool useCurl) and not ImageFactory::open(const byte* data, size_t size) which is a bit different and thus not getting to the buggy function which is readMetadata.

try to reproduce by running the fuzzer with the testcase I attached ./bin/read-metadata <test_case>

@yevgenypats yevgenypats force-pushed the fuzzitdev:libfuzzer_integration branch from 77fda42 to 2ef8874 Jul 3, 2019

Add libFuzzer integration + report bug
This commit places the basics for libFuzzer integration with one
fuzzer which fuzzes the readMetadata function. The fuzzer is
located at fuzz/read-metadata.

To add more fuzzers please add them to ./fuzz directory as
described in the README.

Also a memory corruption bug is found using this fuzzer which
might lead to additional bugs after fix is pushed.

@yevgenypats yevgenypats force-pushed the fuzzitdev:libfuzzer_integration branch from 2ef8874 to a7791ab Jul 3, 2019

@yevgenypats

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@kevinbackhouse Also I just push a review fix per @cryptomilk instructions so the compile line now is

mkdir build
cd build
cmake .. -G "Unix Makefiles" "-DEXIV2_BUILD_FUZZ_TESTS=ON"  "-DCMAKE_BUILD_TYPE=AddressSanitizer"
make -j4
./bin/read-metadata
@yevgenypats

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@kevinbackhouse can you please request/assign a CVE for this bug?

@D4N
Copy link
Member

left a comment

Wow, thanks for this contribution!

However, by removing EXIV2_TEAM_USE_SANITIZERS you effectively crippled our CI pipeline to no longer run with ASAN + UBSAN. Unless this is fixed, this PR is a nogo. Fixing this will probably require some refactoring of the CMake code, please tell if you want to undertake this (we can take care of that too).

@yevgenypats

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

@D4N

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

@yevgenypats

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

@FIOpwK

This comment has been minimized.

Copy link

commented Jul 11, 2019

CVE-2019-13504 has been assigned to this issue

@clanmills clanmills requested review from D4N, piponazo and clanmills Jul 11, 2019

@clanmills

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

Gentlemen:

I see you've all been busy while Alison and I have been having fun in the Baltic. Is this change in 0.27-maintenance?

This is quite a major change to introduce into v0.27.2. Can I proceed to release v0.27.2 from PR #940?

@cryptomilk @kevinbackhouse @piponazo @D4N @yevgenypats Is everybody happy about this being delayed for v0.27.3 scheduled for 2019-09-30?

@D4N

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

@clanmills This PR is not for 0.27-maintenance and will probably be dropped in favor of #945.

@clanmills

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

This morning, I did a $ git pull --rebase, and code relating to this arrived on my laptop:

690 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ git pull --rebase
remote: Enumerating objects: 4241, done.
remote: Counting objects: 100% (3174/3174), done.
remote: Compressing objects: 100% (857/857), done.
remote: Total 2422 (delta 1843), reused 2113 (delta 1540), pack-reused 0
Receiving objects: 100% (2422/2422), 478.72 KiB | 446.00 KiB/s, done.
Resolving deltas: 100% (1843/1843), completed with 286 local objects.
From https://github.com/exiv2/exiv2
   1a9bae4f..b7a97854  0.27-maintenance -> origin/0.27-maintenance
   a803ce3b..1de8e734  master           -> origin/master
Updating 1a9bae4f..b7a97854
Fast-forward
 src/mrwimage.cpp                        |  22 +++++++++++++++-------
 test/data/issue_943_poc1.mrm            | Bin 0 -> 37 bytes
 test/data/issue_943_poc2.mrm            | Bin 0 -> 25 bytes
 tests/bugfixes/github/test_issue_943.py |  25 +++++++++++++++++++++++++
 4 files changed, 40 insertions(+), 7 deletions(-)
 create mode 100644 test/data/issue_943_poc1.mrm
 create mode 100644 test/data/issue_943_poc2.mrm
 create mode 100644 tests/bugfixes/github/test_issue_943.py
691 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ 

I think these mrm files are minolta stuff and that's 946 and 948. Confused!

@kevinbackhouse

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

@clanmills: What you're seeing there is #946, which I targeted at 0.27-maintenance. The other fix is #944, but I forgot to target that at 0.27-maintenance when I created the PR, so it is currently targeting master instead. Would it be possible to get both #946 and #944 into v0.27.2?

@clanmills

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

We can put it into v0.27.2. However I don't like code changes between the final RC and Release.

If you feel this (and other good stuff) should go into 0.27.2, then we can do 0.27.2.3 (Release Candidate 3) this weekend, and release v0.27.2 at the end of July.

Team Exiv2 doesn't have a constitution or voting procedure because we usually agree! I'm happy to do what the team things is best!

@kevinbackhouse

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

@clanmills: Thanks! That plan sounds good to me.

@D4N

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

Closing this as #945 has been merged.

@D4N D4N closed this Jul 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.