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

fuzzing-created crashers #718

Closed
setharnold opened this issue Mar 2, 2016 · 35 comments
Closed

fuzzing-created crashers #718

setharnold opened this issue Mar 2, 2016 · 35 comments

Comments

@setharnold
Copy link
Contributor

Hello; I reviewed OpenJPEG recently for the Ubuntu Linux distribution. As part of this effort I ran our currently packaged 1.5.2 version through the AFL fuzzer for roughly a day and found several issues. I verified that these specific crashing files do not crash the 2.1.0 upstream release, but four hours of fuzzing time found a crashing file on 2.1.0.

I've uploaded the tarball of files that caused crashes to Launchpad:
https://bugs.launchpad.net/ubuntu/+source/openjpeg/+bug/711061/+attachment/4586223/+files/openjpeg-crashers.tar.gz
If that URL doesn't work, the bug is at:
https://bugs.launchpad.net/ubuntu/+source/openjpeg/+bug/711061

Please consider adding these files to your test suite.

When you've had an opportunity to diagnose and prepare fixes, it would be hugely beneficial to downstream consumers if you could also request CVEs on the oss-security mail list:
http://oss-security.openwall.org/wiki/mailing-lists/oss-security

I would be happy to help request CVEs when you're ready.

Thanks

@szukw000
Copy link
Contributor

szukw000 commented Mar 2, 2016

A first test:

szukw000: gm animate relax.jp2
ICC Profile CS 52474220
gm: jp2_dec.c:302: jp2_decode: Assertion `dec->image->cmprof_' failed.
gm animate: abort due to signal 6 (SIGABRT) "Abort"...
Aborted

szukw000: animate relax.jp2

szukw000: flimage relax.jp2

A second test:

diff -u relax.jp2 id:000000,sig:11,src:000008,op:flip2,pos:354.jp2 > test.dif

//vim test.dif:
Binary files relax.jp2 and id:000000,sig:11,src:000008,op:flip2,pos:354.jp2 differ

A second test:

szukw000: gm animate id:000000,sig:11,src:000008,op:flip2,pos:354.jp2
error: no code stream found
gm animate: Unable to decode image file (id:000000,sig:11,src:000008,op:flip2,pos:354.jp2).

szukw000: animate id:000000,sig:11,src:000008,op:flip2,pos:354.jp2
animate: Box length is inconsistent.
OpenJP2' @ error/jp2.c/JP2ErrorHandler/193. animate: Stream error while reading JP2 Header box OpenJP2' @ error/jp2.c/JP2ErrorHandler/193.
animate: unable to decode image file `id:000000,sig:11,src:000008,op:flip2,pos:354.jp2' @ error/jp2.c/ReadJP2Image/349.

szukw000: flimage id:000000,sig:11,src:000008,op:flip2,pos:354.jp2
JP2.c:913:
opj_read_header failed

GraphicsMagic and ImageMagic both use Jasper, FLIMAGE uses openjpeg-master-2015-12-27.

winfried

@szukw000
Copy link
Contributor

szukw000 commented Mar 2, 2016

Here is another test:

szukw000: kdu_expand -i relax.jp2 -o relax-jp2.tif

Consumed 1 tile-part(s) from a total of 1 tile(s).
Consumed 15,911 codestream bytes (excluding any file format) = 1.060733
bits/pel.
Processed using the multi-threaded environment, with
6 parallel threads of execution

szukw000: kdu_expand -i id:000000,sig:11,src:000008,op:flip2,pos:354.jp2 -o id:000000,sig:11,src:000008,op:flip2,pos:354-jp2.tif
Error in Kakadu File Format Support:
Illegal box length field encountered in JP2 file.

Both images are created by 'Kakadu-3.2'.

The 'Illegal box length' may be here:

relax:

read_jp2.c:900:
read_jp2h
BOX name(res ) len(26)

id:

read_jp2.c:900:
read_jp2h
BOX name(res ) len(2)

winfried

@boxerab
Copy link
Contributor

boxerab commented Apr 11, 2016

@setharnold I am interested in doing some of my own fuzzing on openjpeg using AFL. Can you please tell me how you set this up?

Thanks.

@setharnold
Copy link
Contributor Author

@boxerab the quick way to get started:

ccmake to change the CC to /usr/bin/afl-gcc
Build with AFL_HARDEN=1 make
mkdir -p fuzz/in fuzz/out
cd fuzz
cp one_or_two_openjpeg_files in/
afl-fuzz -i in -o out -f foo.j2k ../bin/ -i @@ -o foo.bmp

It's probably best to start with just one or two images, maybe created from different sources if you can, and smaller is probably going to give better results faster than large images.

If you've got a computer with enough cores, then it's worth running multiple fuzzers simultaneously; add -M to the first afl-fuzz command, and -S 1 .. -S n to subsequent commands. It's helpful to run it in tmux or screen, to manage all the fuzzers.

It's probably worth fiddling with different sanitizers too -- ASAN is insanely useful, but is best done on 32-bit versions. UBSAN looks like it's probably also useful, but I haven't tried fuzzing with that yet. ( -fsanitize=address or -fsanitize=undefined -fno-sanitize-recover )

There's a nice writeup by the Fuzzing Project at https://fuzzing-project.org/tutorial3.html that may be more useful than my ramblings.

Thanks

@boxerab
Copy link
Contributor

boxerab commented Apr 12, 2016

Awesome, thanks Seth! I am going to try to automate through the entire set of j2k files in the OpenJPEG test suite.

@setharnold
Copy link
Contributor Author

On Apr 11, 2016 17:56, "Aaron Boxer" [email protected] wrote:

Awesome, thanks Seth! I am going to try to automate through the entire
set of j2k files in the OpenJPEG test suite.

It's slightly paradoxical but you'll probably see better results with fewer
files, for a given length of time. Maybe start with afl-cmin to find just
the minimum number of files that expose unique runtime paths.

Have fun!

@julienmalik
Copy link
Collaborator

I've set up a UBSAN build on a nightly basis for master branch, which run the entire test suite with UBSAN, exiting at first error ( -fno-sanitize-recover ).

First results here : http://my.cdash.org/viewTest.php?onlyfailed&buildid=946315
This triggers quite a lot of errors. This will take some time to digest, but at first sight I see these errors :

  • some coming from thirdparties (libcms).
  • pi.c:759 runtime error: shift exponent 47 is too large for 32-bit type 'unsigned int'
  • t1.c:1517 runtime error: left shift of negative value -865
  • t1.c:1183 runtime error: null pointer passed as argument 1, which is declared to never be null
  • an important number of tests failing since other tests errored out and did not generate their output. I'll probably try to exclude some groups of tests to try to lower down the number of failing tests.

@boxerab
Copy link
Contributor

boxerab commented Apr 20, 2016

Julien, can you tell me how you set up the UBSAN build? I tried doing this, with latest gcc or clang,
and it didn't seem to work. First of all, I couldn't get the project to build unless I used static linking.
When I did use static linking, it built, but all ctests errored out.

I tried this on Ubuntu, with latest gcc and clang.

Thanks,
Aaron

@julienmalik
Copy link
Collaborator

I'm using gcc-5 (from ubuntu precise, using the toolchain ppa).
The full ctest script is accessible here : http://my.cdash.org/viewNotes.php?buildid=946322
But basically it's nothing more than using "-O0 -g -fsanitize=undefined -fno-sanitize-recover" in the build flags.

@boxerab
Copy link
Contributor

boxerab commented Apr 20, 2016

Thanks. Strange, I will have to try again.By the way, the results link you posted above was blank - no errors were listed.

@boxerab
Copy link
Contributor

boxerab commented Apr 20, 2016

Actually, I can see the results now, Thanks.

@julienmalik
Copy link
Collaborator

With an up-to-date cmake, UBScan results can be parsed by ctest_memcheck and posted to the Dashboard :
http://my.cdash.org/viewDynamicAnalysis.php?buildid=946322

The presentation is far from optimal, but certainly better than looking into every test output.
Fixing the most obvious errors should make it a lot more readable.

@boxerab
Copy link
Contributor

boxerab commented Apr 20, 2016

Awesome - what is the command line for this ? I am running ctest manually on my Ubuntu VM,
but ctest_memcheck is not a recognized command.

@boxerab
Copy link
Contributor

boxerab commented Apr 20, 2016

ahhhh, I see - ctest_memcheck goes in the cmake file.

@julienmalik
Copy link
Collaborator

@boxerab you should learn about ctest script.
Download the two files in here : http://my.cdash.org/viewNotes.php?buildid=946322
Customize the first one (openjpeg_common.cmake is meant to be generic)

Then :

$ ctest -S mytest.cmake [-VV]

This will build from scratch and push results to the dashboard

@malaterre
Copy link
Collaborator

Hum. I would have assumed you needed to replicate the CFLAGS onto the LDFLAGS. Eg:

$ CFLAGS=-fsanitize=undefined LDFLAGS=-fsanitize=undefined cmake . && make

@boxerab
Copy link
Contributor

boxerab commented Apr 20, 2016

@julienmalik yes, need to RTFM on ctest. @malaterre why would linker need to know about UBSAN setting ?

@boxerab
Copy link
Contributor

boxerab commented Apr 20, 2016

@julienmalik thanks for running these tests. I don't think it is as bad as it looks : most of the errors are seemingly related to the three issues you mention.

Left shift of negative number is indeed UB for C language.

Solution is to multiply instead - I think I may have introduced this bug :) In this case, we are multiplying by a constant, so I don't think performance would suffer.

For the shift exponent too large error, solution is to use 64 bit types for these few lines.

So, all in all, not too hard to fix this. My feeling is these fixes should be in upcoming release.

@malaterre
Copy link
Collaborator

@boxerab technically one would need to do LDFLAGS="-lasan -lubsan" but the other syntax I suggested is easier to read (remember).

@boxerab
Copy link
Contributor

boxerab commented Apr 23, 2016

@malaterre thanks for this tip : turns out I do need -lubsan in the shared linker flags in order
to compile dynamic build. Putting in -fsanitize=undefined didn't work.

@malaterre
Copy link
Collaborator

@boxerab You can also use:

-fsanitize-undefined-trap-on-error
The -fsanitize-undefined-trap-on-error option instructs the compiler to report undefined behavior using __builtin_trap rather than a libubsan library routine. The advantage of this is that the libubsan library is not needed and is not linked in, so this is usable even in freestanding environments.

ref:

@boxerab
Copy link
Contributor

boxerab commented Apr 26, 2016

Cool. Could be used in production.

@setharnold
Copy link
Contributor Author

On Apr 26, 2016 05:30, "Aaron Boxer" [email protected] wrote:

Cool. Could be used in production.

I know it's tempting but please don't turn on the sanitizers in production
code: http://www.openwall.com/lists/oss-security/2016/02/17/9

They are great aids for testing but not hardening tools, at least not yet.

Thanks

@mayeut
Copy link
Collaborator

mayeut commented Apr 26, 2016

The PR #706 shall fix a few (if not all) negative shift exp errors. It follows analysis of #135 issue with UBSan some time ago. Waiting for @detonin to merge it if it's legal regarding the standard.

@mayeut
Copy link
Collaborator

mayeut commented Apr 28, 2016

@julienmalik
Are you using any tricks in your script to get a clean report for ubsan ?
Mine is clobbered (I think), http://my.cdash.org/viewDynamicAnalysis.php?buildid=950937
Rather than kind of undefined behaviors, columns show kind & values (leading to much more column).
I use cmake 3.5.2 & clang 3.8.0 on Mac OS X 10.11.4

@julienmalik
Copy link
Collaborator

@mayeut yes I'm hacking the DynamicAnalysis.xml file before ctest_submit.
See http://my.cdash.org/viewNotes.php?buildid=950787 and grep for postprocess_ubsan.py

@mayeut
Copy link
Collaborator

mayeut commented Apr 29, 2016

@julienmalik, thanks, this cleans up the report for sure. I think you have one regex missing though:
(re.compile("load of misaligned address 0x[0-9a-f]+ for type '.*' \\(aka '.*'\\), which requires [0-9]+ byte alignment"), "misaligned address"),

It wouldn't help much on your build for some reason (only one address reported) but certainly helps mine.
before: http://my.cdash.org/viewDynamicAnalysis.php?buildid=951168
after: http://my.cdash.org/viewDynamicAnalysis.php?buildid=951173

I think all the trivial reports from UBSan are now gone. Remain the tricky ones...

@julienmalik
Copy link
Collaborator

@mayeut all these misaligned address seem to come from lcms. I checked upstream and they have patched these lines. See #544

@mayeut
Copy link
Collaborator

mayeut commented Apr 30, 2016

I will see about merging in #544 to get rid of these warnings then.

@mayeut
Copy link
Collaborator

mayeut commented May 1, 2016

I update lcmd to mm2/Little-CMS@0e8234e but this introduced signed integer overflow (that are harmless).
See mm2/Little-CMS#74

@boxerab
Copy link
Contributor

boxerab commented May 1, 2016

@mayeut I see you upgraded LCMS to 2.8 beta version. With upcoming opj release, don't you think it is better to use latest LCMS release: 2.7 ?

@boxerab
Copy link
Contributor

boxerab commented May 2, 2016

Ahh, never mind, I see this beta fixes some alignment issues

@detonin
Copy link
Contributor

detonin commented Sep 20, 2016

@setharnold Does this issue still need to stay opened or is it an obsolete version of #811 ?

@setharnold
Copy link
Contributor Author

@detonin, good question, I haven't re-tested these files with a new build of openjpeg. Thanks.

@detonin detonin modified the milestones: OPJ v2.1.2, OPJ v2.1.3 Sep 29, 2016
@rouault
Copy link
Collaborator

rouault commented Aug 9, 2017

With latest master, "for i in ../openjpeg-crashers/*; do echo $i; bin/opj_decompress -quiet -i $i -o out.bmp; done >log.txt 2>&1" on a -fsanitize=address,undefined now works cleanly

@rouault rouault closed this as completed Aug 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants