Add basic image parser fuzzing

[pauldreik]

Hi!
I wrote a fuzzer, and it found some issues. Asking for it to be merged even if the problems are not fixed.
I wrote a unit test to have a place to put the bad input I found, so it's easier to reproduce and possible to prevent the problem from appearing in the future. Unfortunately, the unit test works differently on posix and non-posix, since I did not want to demand C++17 or boost filesystem to recurse the input directory.

These problems were found, undefined behaviour sanitizer is needed to reproduce. I also could not reproduce all problems with gcc, clang was needed.

integer overflow when allocating data

this has been fixed, by adding the overflow check (integer_utils.h). There is a binary file which triggers the error, but since it's fixed now the file passes without problems. To reproduce, build the unit test with clang and address sanitizer.
Status: fixed

out of range value casted to unsigned char

This is commented in the pixel.h file, and there is an example file.
Status: unfixed

reference binding to null pointer of type 'dlib::rgb_pixel

I believe this is because the data pointer is empty or the index is invalid, but I'm not sure since I did not look into it. I just added an example file integer_overflow_and_nullpointer.bin that provokes it, and added an assert in the pixel.h file (maybe insufficient).
Status: unfixed

signed integer overflow in image_loader.h

0 - biHeight overflows if biHeight is INT_MIN. added a comment, and there is an example file to provoke it.
Status: unfixed

large allocations

I think it's problematic if a small input file can result in a (unreasonably) large allocation. One of these is inside giflib, so that is not dlib's problem. But there are other places where it would make sense to be a bit more skeptic to the input, I wrote a comment about it in serialize.h
Fuzzing will find these allocations quite quick if using address sanitizer. I think there's a problematic middle ground between reasonably small allocations and ridiculously large ones. The former will be fast anyway and the latter will cause an exception std::bad_alloc to be thrown which is good. However, inbetween a lot of memory will be used and if it's initialized, it may slow down the process a lot. This warrants a more pessimistic attitude against the input data, IMHO.

Fuzzing

I had to modify the code here and there to make it go faster - fuzzing this was extremely slow (one case per second or so) but with the fixes, it is at least 50 a second or so, which also is slow. You are probably the best person to point out what could be changed to get the speed up.
The HangChecker class could be adjusted a bit, to account for evil input not hogging the parser. For now, it's only limited in fuzz mode and does nothing in production mode.

Cheers,
Paul

[pauldreik]

Hmm, I don't get why appveyour doesn't like check_c_source_compiles ?

Unknown CMake command "check_c_source_compiles"

I used cmake 3.10 on Ubuntu while developing, and appveyour has 3.13.3 according to the output.
I'm too tired to troubleshoot this now, sorry.

[davisking]

It's very reasonable to allocate arrays larger than 200x200.

[pauldreik]

Yes, I agree! This is only during fuzzing, to speed it up. That's why it's inside the ifdef check.

[davisking]

Did this actually happen? What input causes this?

[pauldreik]

Yes, try running the newly added unit test with the inputs in the problematic directory, and you will see it. (compiling with clang and undefined behaviour sanitizer)

[davisking]

Can you find out the exact pixel value that triggers it? That would make fixing and testing it easier for me :)

[pauldreik]

This will trigger the undefined behaviour, but it won't be detected by DLIB_TEST, because the UB is inside the function. I put this inside the unit test in pixel.cpp

            rgb_pixel src;
            src.red=246;
            src.green=170;
            src.blue=245;
            hsi_pixel dest;
            assign_pixel(dest,src);
            DLIB_TEST(dest.h>=0);
            DLIB_TEST(dest.h<=360);

[pauldreik]

I think the missing part is the check for hue<0, it should be wrapped around by adding 360. Looking at the code, it seems like it gives -60<hue<300

Looking at the wikipedia entry, it seems like the function could be simplified a bit:
https://en.wikipedia.org/wiki/HSL_and_HSV#From_RGB

[pauldreik]

I pushed a minimal fix.

[davisking]

I'm not worried about this kind of thing. If someone makes a file that has crazy stuff in it then that's a user error.

[pauldreik]

Ok, that indirectly means dlib should never be used on untrusted input?

[davisking]

Yes, don't call deserialize() on inputs you don't trust. There shouldn't ever be arbitrary code execution type of errors, but if someone wants to try and deserialize a 100GB block of data they are free to do that. But most users would not be happy with the results, although some would.

[pauldreik]

I get your argument regarding allocations, but what about a mismatch on the size of the buffer and what is actually read? If the file is short, shouldn't that be flagged as an error? Maybe the input file has been truncated, and it would be nice to alert the user about that.

[davisking]

Most definitely. I think this is already caught by existing code. If it isn’t it should be fixed.

[davisking]

I would rather keep code like this out of the main dlib repo, or potentially in a branch. But not on master. This includes any #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION type of thing. That stuff should be in a branch or somewhere outside maser, and when errors are surfaced, fixed on master.

[pauldreik]

I understand, but that makes the fuzzing prone to bit rot.
The macro is the defacto standard, see https://llvm.org/docs/LibFuzzer.html#fuzzer-friendly-build-mode

Here's an example from fmt: https://github.com/fmtlib/fmt/blob/6a497e1d061993cce54c2d71506a90155a3725e6/include/fmt/format.h#L599

Here are some recommendations from oss-fuzz:
https://github.com/google/oss-fuzz/blob/master/docs/ideal_integration.md#tldr

[davisking]

I hear you. But you are talking about parts of dlib that have not changed in many years, and are not likely to change. I doubt such bit rot will be a problem, and if this fuzzing ends up being used long term rather than being eventually abandoned and such problems do arise they can be handled then, maybe by merging into main.

[robertramey]

Hmm - have you considered using boost/safe_numerics to address these sorts of problems? https://github.com/boostorg/safe_numerics

[pauldreik]

Hmm - have you considered using boost/safe_numerics to address these sorts of problems? https://github.com/boostorg/safe_numerics

@robertramey Yes, I did not want to bring in a dependency on boost (especially when safe_numerics is so recently added to boost, it takes a while until new boost versions trickle down to end users).

[davisking]

Yes, please don’t add any new dependency.

…

On Jul 19, 2019, at 2:48 PM, Paul Dreik ***@***.***> wrote: Hmm - have you considered using boost/safe_numerics to address these sorts of problems? https://github.com/boostorg/safe_numerics @robertramey Yes, I did not want to bring in a dependency on boost (especially when it's so newly added). — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[pauldreik]

I fixed some of the issues, but I really would like you to reconsider having the fuzzing constructs in the master branch. Having a separate branch for fuzzing leads to manual maintenance - every time you make a change on master, someone needs to update that branch. I have done that during development of the fuzzers in fmt, and it's not a long term solution.
The fuzzer itself lives in a separate directory, just like the tests, so it should not be interfering with anyone.
I think the results above, from a very basic shot at fuzzing, show that it is indeed useful and worth a bit of uglification of the source code.

[davisking]

I fixed some of the issues, but I really would like you to reconsider having the fuzzing constructs in the master branch. Having a separate branch for fuzzing leads to manual maintenance - every time you make a change on master, someone needs to update that branch. I have done that during development of the fuzzers in fmt, and it's not a long term solution.
The fuzzer itself lives in a separate directory, just like the tests, so it should not be interfering with anyone.
I think the results above, from a very basic shot at fuzzing, show that it is indeed useful and worth a bit of uglification of the source code.

I do a lot of this kind of testing myself, but I don't commit the code that involves intrusive changes to the actual library. So I'm already accustomed to the churn, and normally just throw away the code.

Can you break this PR up into specific PRs targeted at fixing the issues surfaced by fuzzing while keeping any instrumentation code used for finding them in a separate fork? I'm cool with stuff like safe multiplies being in dlib main though. That's all to the good.

[pauldreik]

I submitted the bugfixes on separate pull requests as instructed.

Would you be ok with add the fuzzers in the fuzzer/ directory on master? They won't interfere since it's just a subdirectory not being referenced. The constructs for preventing slow input could be kept on another branch, preferably in this repo instead of mine. That means people wanting to use or contribute to fuzzing, could find them in this repo.

Closing this pull request anyhow.

[davisking]

Yeah I'm happy with that, so long as no intrusive code is in master. But certainly fuzzer tooling could be in master, in tools/fuzzer, dlib/testing/fuzzer, or some similar location. I'm not totally sure why the intrusive code is needed at all though. Why can't a timeout at a higher level be used? That's what I typically do when I do this kind of testing. This keeps any fuzzing code out of the actual codebase.