Headline
CVE-2019-13504: Add libFuzzer integration + report bug by yevgenypats · Pull Request #943 · Exiv2/exiv2
There is an out-of-bounds read in Exiv2::MrwImage::readMetadata in mrwimage.cpp in Exiv2 through 0.27.2.
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
Conversation 37 Commits 1 Checks 0 Files changed
Conversation
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.
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
Please could you include exact reproduction steps? What command line did you run this with?
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
@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.
@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>
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.
@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
D4N requested changes Jul 4, 2019
Copy link
Member
** D4N 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).
Hi Dan, Thanks:) I changed this per @cryptomilk request. I’m not sure I’m the right person to fix this as I’m not very familiar with it. I can change it backward and we can fix it in a different PR. So this pull request will only add the fuzzers. Also, would you like me to contribute another pull request so the fuzzers will run on a daily basis via Fuzzit integration? This way it may find new bugs and also catch If bugs are introduced again. Cheers, Yevgeny.
…
On Thu, Jul 4, 2019, 10:36 AM D4N ***@***.***> wrote: ***@***.**** requested changes on this pull request. 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). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#943?email_source=notifications&email_token=AD52CDS4ZZZEBEFYE7JUSETP5WZBPA5CNFSM4H5EZH42YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB5PU2ZI#pullrequestreview-257903973>, or mute the thread https://github.com/notifications/unsubscribe-auth/AD52CDVPNEGL2WSEEC6Z5WDP5WZBPANCNFSM4H5EZH4Q\ .
Copy link
Member
****D4N** commented Jul 4, 2019**
Yevgeny Pats [email protected] writes:
Hi Dan, Thanks:) I changed this per @cryptomilk request. I’m not sure I’m the right person to fix this as I’m not very familiar with it. I can change it backward and we can fix it in a different PR. So this pull request will only add the fuzzers.
Could you push your previous state to another branch in your fork and post a link to that here?
Also, would you like me to contribute another pull request so the fuzzers will run on a daily basis via Fuzzit integration?
Once we merge this, why not? Before merging this, I see little benefit in that, beside cluttering my inbox ;-)
This way it may find new bugs and also catch If bugs are introduced again.
How is Fuzzit different from oss-fuzz, since I have the long term plan of getting exiv2 into oss-fuzz.
…
Cheers, Yevgeny. On Thu, Jul 4, 2019, 10:36 AM D4N ***@***.***> wrote: > ***@***.**** requested changes on this pull request. > > 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). > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#943?email_source=notifications&email_token=AD52CDS4ZZZEBEFYE7JUSETP5WZBPA5CNFSM4H5EZH42YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB5PU2ZI#pullrequestreview-257903973>, > or mute the thread > https://github.com/notifications/unsubscribe-auth/AD52CDVPNEGL2WSEEC6Z5WDP5WZBPANCNFSM4H5EZH4Q\ > . > – You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #943 (comment)
Sounds good. Will do regarding the additional PR with the link. Fuzzit has some more features that OSS currently missing like running the tests on PRs as well ( https://fuzzit.dev/2019/06/20/continuous-fuzzing-systemd-case-study/) and a bit better user management IMHO. Essentially you can use both if you want. Systemd uses both and we are working with envoy/the Google team on an integration as well so they also will use both. In my opinion you can just use Fuzzit, essentially we are running the same fuzzers. I think teams that uses both usually started with OSS fuzz and then added Fuzzit after we launched. But that’s up to you. So If it’s fine with you once we merge I’ll open an additional PR which should be pretty straightforward as we already have the foundations for the fuzzers themselves:)
…
On Thu, Jul 4, 2019, 3:57 PM D4N ***@***.***> wrote: Yevgeny Pats ***@***.***> writes: > Hi Dan, > > Thanks:) I changed this per @cryptomilk request. I’m not sure I’m the right > person to fix this as I’m not very familiar with it. I can change it > backward and we can fix it in a different PR. So this pull request will > only add the fuzzers. Could you push your previous state to another branch in your fork and post a link to that here? > > Also, would you like me to contribute another pull request so the fuzzers > will run on a daily basis via Fuzzit integration? Once we merge this, why not? Before merging this, I see little benefit in that, beside cluttering my inbox ;-) > This way it may find new bugs and also catch If bugs are introduced > again. How is Fuzzit different from oss-fuzz, since I have the long term plan of getting exiv2 into oss-fuzz. > > Cheers, > Yevgeny. > > On Thu, Jul 4, 2019, 10:36 AM D4N ***@***.***> wrote: > >> ***@***.**** requested changes on this pull request. >> >> 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). >> >> — >> You are receiving this because you were mentioned. >> Reply to this email directly, view it on GitHub >> < #943?email_source=notifications&email_token=AD52CDS4ZZZEBEFYE7JUSETP5WZBPA5CNFSM4H5EZH42YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB5PU2ZI#pullrequestreview-257903973 >, >> or mute the thread >> < https://github.com/notifications/unsubscribe-auth/AD52CDVPNEGL2WSEEC6Z5WDP5WZBPANCNFSM4H5EZH4Q > >> . >> > > > – > You are receiving this because you were mentioned. > Reply to this email directly or view it on GitHub: > #943 (comment) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#943?email_source=notifications&email_token=AD52CDRGV3NE734S4H6OGVTP5X6UTA5CNFSM4H5EZH42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZHPQZA#issuecomment-508491876>, or mute the thread https://github.com/notifications/unsubscribe-auth/AD52CDVX5GNQ2WV5OB7E3VDP5X6UTANCNFSM4H5EZH4Q\ .
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?
Copy link
Member
****D4N** commented Jul 11, 2019**
@clanmills This PR is not for 0.27-maintenance and will probably be dropped in favor of #945.
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!
@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?
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!
Copy link
Member
****D4N** commented Jul 18, 2019**
Closing this as #945 has been merged.