1459425 - oss-fuzz qcms integration

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect, P3)

Description

[u473386]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.139 Safari/537.36

Steps to reproduce:

I'd like to integrate qcms, which is the part of Firefox that processes ICC profiles (as found in images) and does color management, into oss-fuzz. It's one of the projects proposed for integration by Google.

https://github.com/google/oss-fuzz

I've already reported two bugs found from local fuzzing, and have everything prepared. Someone from Mozilla just needs to agree to be CC'd to bug reports.

Comment 1

[u473386]

I'm CC'ing you as per this post.

https://github.com/google/oss-fuzz/issues/1308#issuecomment-380140969

Comment 2

[u473386]

In a first step, the relevant code and files will be hosted by oss-fuzz. In a possible second step, it could be integrated into the Firefox repo. Since qcms is relatively simple, it may be a good candidate to attempt this, if Mozilla chooses to do so.

https://hg.mozilla.org/mozilla-central/file/tip/gfx/qcms/

Comment 3

[Christian Holler (:decoder)]

Ccing :posidron since he also did some work on qcms fuzzing some time ago.

I also did some local fuzzing with our in-tree qcms and libfuzzer but didn't find anything. Can you please cc me on the relevant bugs? Integrating this into oss-fuzz sounds like like a great idea though and I'll be glad to help with mozilla-central integration.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Based on our experience with OSS-Fuzz for SpiderMonkey, effectively we need to file bugzilla bugs for any OSS-Fuzz bugs, so that we can use our pre-existing processes for landing security bugs. So to integrate it we need to have someone committed to doing that work. Assuming the volume of QCMS bugs isn't super high, I'm happy to do it, unless someone from either the gfx team or the fuzzing team wants to :-)

Comment 5

[u473386]

Yep, I expect volume to be low. I've fuzzed it with ASAN (using the three fuzzers oss-fuzz also uses) and MSAN for quite a while already.

I found three bugs actually, with only one being security relevant. The others are a memory-leak (which hindered further fuzzing) and use-of-uninitialized-value found with MSAN.

https://bugzilla.mozilla.org/show_bug.cgi?id=1431637
https://bugzilla.mozilla.org/show_bug.cgi?id=1432067
https://bugzilla.mozilla.org/show_bug.cgi?id=1444734

(I CC'd you on the second.)

Comment 6

[u473386]

I think it'd be good to fix bug 1444734 before I submit this, otherwise it'll just land a duplicate report. (The bug report was closed for late reply.)

Comment 7

[u473386]

The fuzzer should be running now.

https://github.com/google/oss-fuzz/tree/master/projects/qcms

The next step could be someone from Mozilla to move the fuzz.* files somewhere into the Firefox repo. I think integrating it into the build system may not be necessary, considering how easy it is to build qcms stand-alone. Unless there's an easy way to just build that target that I'm not aware of.

And I'd like to ask a general question about how to best integrate other potential targets. I once wanted to fuzz the FTP directory parser, and noticed that you get a huge static library at the end of the build process. So I linked that in, but when I wanted to run the fuzzer I got an error like: must be used within XUL or so. Is there some documentation on that?

Comment 8

[u473386]

(Thanks for your mail. That was exactly what I was looking for.)

I found another qcms bug in the process, which may make the oss-fuzz fuzzer slightly less efficient, as a valid profile in the corpus is considered incorrect by Firefox. Although oss-fuzz may have already figured this out and produced a "correct" invalid profile accordingly.

A patch is attached, so should be an easy fix.

https://bugzilla.mozilla.org/show_bug.cgi?id=1464257

Comment 9

[u473386]

Alright, I've ported qcms to the native fuzzing interface, but I don't think oss-fuzz should switch to it just yet. (Other than for the fact it can't, because of the problem with the environment variables, as discussed on the oss-fuzz tracker.)

I'll list the advantages and disadvantages I noticed.

(+)

- Integration into the Firefox build system. That's nice by itself, but more importantly makes it easier for Firefox developers to reproduce oss-fuzz bugs (and verify them). They just set a few flags, and then build Firefox as normal. Then the test case they downloaded from the bug is put as an argument to the binary. Simple. That's substantial IMO. Right now, they more or less have to run the docker-based oss-fuzz system locally.

(-)

- Only libFuzzer, no AFL or honggfuzz. That's not too severe IMO, as libFuzzer will usually find the other fuzzer's bugs too, given enough time.

- Only ASAN, no MSAN and UBSAN. While I noticed MSAN support in theory, it probably doesn't work, as the complete code has to be MSAN-instrumented. Including libFuzzer itself, and libc++. I don't know if the Firefox build system allows for this. I also noticed UBSAN support, but only for signed-integer-overflow. Should be trivial to add more. List of the UBSAN flags oss-fuzz sets is below.

https://github.com/google/oss-fuzz/blob/c069a7c7804b323bb1128ea285e1abf0082161ae/infra/base-images/base-builder/Dockerfile#L25

- Fixed C(XX)FLAGS in moz.build for fuzz targets. oss-fuzz likes to set its own, to correspond with its clang version, and to use the latest features. Firefox assumes clang-5 and uses -fsanitize-coverage=trace-pc-guard, which is no longer recommended as of clang-6 (stable). oss-fuzz uses clang svn. I don't know if it'd be possible (or wise) to pass oss-fuzz flags through.

(End of list.)

So what I'd like to suggest for qcms is a hybrid approach. Integrate the ported qcms into the Firefox repo as planned. This allows Firefox developers to easily reproduce ASAN-bugs at least, and makes the fuzz target maintained. Then don't make oss-fuzz use the Firefox build system, but only pull in the single fuzz target file, to build it as before. This should not be too difficult with 1-2 #ifdefs in the fuzz target. What do you think?

Comment 10

[Christian Holler (:decoder)]

(In reply to pdknsk from comment #9)

> 
> - Only libFuzzer, no AFL or honggfuzz. That's not too severe IMO, as
> libFuzzer will usually find the other fuzzer's bugs too, given enough time.


The fuzzing interface supports AFL, it just isn't documented (yet) in the MDN doc. 

See e.g. https://searchfox.org/mozilla-central/source/tools/fuzzing/interface/FuzzingInterface.h#22

Won't work on oss-fuzz though due to the use of environment variables.


> - Fixed C(XX)FLAGS in moz.build for fuzz targets. oss-fuzz likes to set its
> own, to correspond with its clang version, and to use the latest features.
> Firefox assumes clang-5 and uses -fsanitize-coverage=trace-pc-guard, which
> is no longer recommended as of clang-6 (stable).

This is no longer the case as of yesterday (fixed by bug 1464202). The build system no detects the presence of -fsanitize=fuzzer-no-link and uses that if it is supported. It only uses the old trace-pc-guard if the compiler is too old for the other flag.

Comment 11

[u473386]

> This is no longer the case as of yesterday (fixed by bug 1464202).

Ah, that's good. Passing oss-fuzz C(XX)FLAGS isn't necessary then.

I've filed a separate bug for Firefox integration.

https://bugzilla.mozilla.org/show_bug.cgi?id=1466021

Comment 12

[u473386]

Attachment: qcms

This is the hybrid fuzz target I mentioned. It works as a native target, and also for oss-fuzz. I verified both. It just has two additional #ifndefs compared to what is in oss-fuzz right now. Please review it.

I wasn't sure passing nullptr works, but it does. Saves making an empty LLVMFuzzerInitialize.

Comment 13

[u473386]

Attachment: hybrid

Updated with Mozilla style.

Comment 14

[u473386]

Who should I put as potential reviewer? Or to take a step back, does Mozilla want the fuzz target in mozilla-central? I think it'd be beneficial, even if not strictly necessary.

Comment 15

[Christian Holler (:decoder)]

Comment on attachment 8982853 [details] [diff] [review]
hybrid

Review of attachment 8982853 [details] [diff] [review]:
-----------------------------------------------------------------

r+ from the fuzzing perspective with the comments addressed, but I would still like to have a review from a GFX peer just for the sake of following process.

Jeff is out and his review queue is super full, so I was hoping Bas could maybe help out here?

@Bas: The code here is already well tested in fuzzing, this is only about adding the necessary files to mozilla-central. From a fuzzing perspective it looks ok, and this doesn't change any production code (the moz.build changes are only active in FUZZING builds).

::: gfx/qcms/fuzztest/qcms_fuzzer.cpp
@@ +5,5 @@
> + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
> +
> +#ifndef BUILD_FOR_OSSFUZZ
> +#include "FuzzingInterface.h"
> +#include "gtest/gtest.h"

You probably don't need the gtest include here.

@@ +63,5 @@
> +  qcms_transform_release(transform);
> +}
> +
> +extern "C" int
> +LLVMFuzzerTestOneInput(const uint8_t* data, size_t size)

Can we make this static in the mozilla-central build?

@@ +75,5 @@
> +
> +  // Firefox respects this check, but ignoring it gives slightly higher
> +  // coverage. It only checks part of the profile for reasonable values,
> +  // to not render output caused by likely broken profiles.
> +  if (qcms_profile_is_bogus(profile)) {};

So for any results we would have to check if they still pass this function and if not, check if fixing is possible without breaking the test?

Comment 16

[u473386]

> @@ +75,5 @@
> > +
> > +  // Firefox respects this check, but ignoring it gives slightly higher
> > +  // coverage. It only checks part of the profile for reasonable values,
> > +  // to not render output caused by likely broken profiles.
> > +  if (qcms_profile_is_bogus(profile)) {};
> 
> So for any results we would have to check if they still pass this function
> and if not, check if fixing is possible without breaking the test?

Technically yes, so it's probably better to respect this check in alignment with Firefox. Note that Firefox only calls qcms_profile_is_bogus on the display (destination) profile, so I'll modify the target to match that.

I'll address your other comments in a separate patch.

I think I'll also remove the check on qcms_profile_sRGB. This function can only fail if it cannot allocate memory, at which point libFuzzer quits anyway I think, so this is effectively unreachable.

Comment 17

[u473386]

Just for clarification on your comment: with breaking the test, did you mean the qcms gtest? It only calls qcms_profile_is_bogus on a profile returned by qcms_profile_sRGB, which is an internal profile that can never fail the test. (Unless someone modifies either of these functions and breaks it, which is what the test is for I guess.)

https://dxr.mozilla.org/mozilla-central/source/gfx/tests/gtest/TestQcms.cpp#32

If the answer is yes, I'll proceed as planned and will modify the target to match current Firefox behaviour.

Comment 18

[Christian Holler (:decoder)]

No, I meant breaking the crash sample. As in, it might be that your resulting crash depends on the profile bogus check to be not present. If your plan is to adjust the fuzzing code to match what Firefox actually does (as in, respecting the bogus check), that seems like the best approach to me, simply because we wouldn't have to validate that the fuzzing result is valid.

Comment 19

[u473386]

Yes, that's what I though at first, but wasn't sure. And that's the plan.

Comment 20

[Bas Schouten (:bas.schouten)]

Comment on attachment 8982853 [details] [diff] [review]
hybrid

Review of attachment 8982853 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

Comment 21

[u473386]

Attachment: hybrid-2

> You probably don't need the gtest include here.
Done.

> Can we make this static in the mozilla-central build?
Done. I've restructured this a bit to not have 3 #ifndefs.

Plus changed qcms_profile_is_bogus to match Firefox, as discussed.

Also changed the qcms_profile_sRGB check. It's a simple static function that allocates a few KB. It can never fail (in a way the fuzzer doesn't handle). Still added exit to catch if it does, to be safe. Before the change, a (theoretical) bug in the function would go unnoticed.

Comment 22

[u473386]

Can I assume that this is now reviewed with the comments addressed? If so please tell me how to proceed.

Comment 23

[u473386]

Attachment: hybrid-3

This is identical to hybrid-1 (which was conditionally reviewed), but with two minor changes to address the comments. Please ignore hybrid-2, which I had previously attached. (The change to qcms_profile_sRGB was stupid.) If I've followed instrunctions correctly, the patch should be good to go.

Comment 24

[u473386]

Comment on attachment 8984437 [details] [diff] [review]
hybrid-2

It was hybrid (not hybrid-1).

Comment 25

[u473386]

Comment on attachment 8984437 [details] [diff] [review]
hybrid-2

Args, checkin on wrong patch.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22c4e17a6b30
add fuzzing target for qcms. r=bas sr=decoder

Comment 27

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/22c4e17a6b30