Fixes for a few fuzzer issues

[keszybz]

No description provided.

[evverx]

This isn't going to work when AFL is used as a fuzzing engine so ClusterFuzz will keep opening issues you're trying to avoid. They recommend checking whether the length passed to LLVMFuzzerTestOneInput makes sense in fuzz targets.

[evverx]

google/oss-fuzz#1846 (comment)

[evverx]

I doubt ClusterFuzz would have been able to find a couple of stack overflows if it hadn't tried to feed too much data to, for example, fuzz-json so I'm wondering why you're trying to sweep issues like that under the rug. You could just comment on issues ClusterFuzz have opened and ignore them if you don't think they should be fixed.

[keszybz]

Updated to add internal checks on size.

fuzz-json and various other fuzzers are different, because they are supposed to be used for external data. Those three are for data that we either write ourselves or which is configuration data. Marking and ignoring issues on oss-fuzz is not really an option, because the number of such code paths is very very large, so oss-fuzz would keep finding new issues. I think it's more worthwhile to let the fuzzers find issues that are not this type of resource exhaustion. As I wrote in the commit message, complicating the implementation to "solve" this case is not a good solution, because it would make the code more complicated and pessimize the common case of very small inputs.

So... generally, I think we should let the fuzzers use large inputs, iff the input data is external, and has no natural bound (e.g. from packet size).

[evverx]

I'm wondering where 65535 came from? Could we at least use something that wouldn't look too correct?

(This reminds me that 65535 shouldn't be used in fuzz-dns-packet either as discussed in #8423 (comment))

[evverx]

fuzz-json and various other fuzzers are different, because they are supposed to be used for external data.

As far as I can tell, fuzz-journald-stream, which is supposed to get external data, was changed relatively recently so that it could reject input that is larger than 65535 bytes, so it looks like the difference between fuzz targets handling "internal" and "external" data isn't clear-cut. To be honest, I'd revert eafadd0 because it'd make it clear that the fuzzer needs improving (instead of hiding the assertion).

[keszybz]

@evverx sure, if you feel it worth the trouble, please rewrite the fuzzer to do reads and writes in loops. Using an event loop, this probably will not even be too much code. I just didn't think it worth the trouble, but I'd be happy to review a patch.

[evverx]

@keszybz I hope I'll get round to it, but in the meantime, what I'm trying to say is that it isn't clear to me why issues like that can't be just left open. It's not that we need to "close" as many issues as possible.

[evverx]

What's more interesting is that some fuzzers like fuzz-env-file are skipped when size is 0. That certainly doesn't feel right from a fuzzing perspective (and is probably another temporary solution to get around something else). I'm wondering if anyone can remember why it's done that way.

[keszybz]

I added another commit to allow empty inputs.

[evverx]

check_build is complaining that 5 fuzz targets are broken:

Broken fuzz targets (5):
fuzz-env-file:
BAD BUILD: /out/fuzz-env-file seems to have either startup crash or exit:
INFO: Seed: 3157224537
INFO: Loaded 2 modules   (40415 inline 8-bit counters): 40406 [0x7fe9c030fa00, 0x7fe9c03197d6), 9 [0x8c1f10, 0x8c1f19),
INFO: Loaded 2 PC tables (40415 PCs): 40406 [0x7fe9c03197d8,0x7fe9c03b7538), 9 [0x6694f0,0x669580),
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
Assertion 'f' failed at ../../src/systemd/src/fuzz/fuzz-env-file.c:19, function int LLVMFuzzerTestOneInput(const uint8_t *, size_t)(). Aborting.
AddressSanitizer:DEADLYSIGNAL
=================================================================
==79==ERROR: AddressSanitizer: ABRT on unknown address 0x00000000004f (pc 0x7fe9be99a428 bp 0x7fff9de2fab0 sp 0x7fff9de2f948 T0)
SCARINESS: 10 (signal)
    #0 0x7fe9be99a427 in gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x35427)
    #1 0x7fe9be99c029 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x37029)
    #2 0x7fe9bfc5dbaa in log_assert_failed_realm /work/build/../../src/systemd/src/basic/log.c:795:9
    #3 0x5359ce in LLVMFuzzerTestOneInput /work/build/../../src/systemd/src/fuzz/fuzz-env-file.c:19:9
    #4 0x5761b6 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:529:15
    #5 0x578eaa in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /src/libfuzzer/FuzzerLoop.cpp:729:3
    #6 0x57a7c8 in fuzzer::Fuzzer::Loop(std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, fuzzer::fuzzer_allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&) /src/libfuzzer/FuzzerLoop.cpp:779:3
    #7 0x542faf in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:776:6
    #8 0x535b7c in main /src/libfuzzer/FuzzerMain.cpp:19:10
    #9 0x7fe9be98582f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #10 0x41d208 in _start (/out/fuzz-env-file+0x41d208)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: ABRT (/lib/x86_64-linux-gnu/libc.so.6+0x35427) in gsignal
==79==ABORTING
MS: 0 ; base unit: 0000000000000000000000000000000000000000


artifact_prefix='./'; Test unit written to ./crash-da39a3ee5e6b4b0d3255bfef95601890afd80709
...
Base64:
ERROR: 17% of fuzz targets seem to be broken. See the list above for a detailed information.
Check build failed.

which seems to explain why the fuzzers have always been skipped when size is 0.

[poettering]

hmm, what's the state of this one? from my side this looks good, do you, @evverx have objections?

[keszybz]

The last commit (to allow size==0) was causing problems. I backed it out now. I think this should be good to merge.

[keszybz]

I'll return to the size==0 issue later.

[evverx]

do you, @evverx have objections?

@poettering it's not that I have any objections. It's just that I'm a little concerned that the fuzzers are slowly drifting into the point where they're starting to be less useful than they can be. It's not the first time issues that nobody supposedly is interested in have been swept under the rug by changing the fuzzers. If I recall correctly, the first one was about journal-remote allocating memory (4GB) in one fell swoop by receiving a 10 byte malformed message (which was fixed later because it was kind of related to a bug a CVE was assigned to). I just hope that (at least) someone looks at https://oss-fuzz.com/v2/coverage-report/job/libfuzzer_asan_systemd/latest from time to time to make sure that "fuzzing works best if the sample is not too huge anyway".

[evverx]

Speaking of the coverage report (and how useful it is), judging by https://storage.googleapis.com/oss-fuzz-coverage/systemd/reports/20190315/linux/src/systemd/src/fuzz/fuzz-calendarspec.c.html#L17, fuzz-calendarspec has never got past calendar_spec_from_string. Looks like >= should be used there instead of >.

[keszybz]

> 0 was a bug, but not related to issue size. PR submitted.

I'm not sure what you're trying to say. I'm quite sure that having fuzzers which produce known-bogus results regularly, and having somebody just click through them and mark them as bogus is not the solution. We have a similar situation with CI — and wasted a lot of human time and it quickly leads to the situation where everybody is ignoring the results. Once again: the problem is that any place which does strv appends in the parsing stack is going to be "vulnerable" to timeouts if the sample size is large enough. If you have a better idea how to avoid this other than limiting the sample size, I'm all ears.

[evverx]

I'm not saying that limiting the sample size is necessarily a bad thing. It's just unclear to me why you decided to use 65535 when, for example, ENTRY_SIZE_MAX in journald is 1024*1024*13 (if FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION is set). As I see it, fuzzing isn't exactly about tailoring input so that it can fit into a buffer of some kind without triggering edge cases.

[evverx]

We have a similar situation with CI — and wasted a lot of human time and it quickly leads to the situation where everybody is ignoring the results.

As far as I cat tell, the situation is getting better. Fedora CI, which was never actively maintained properly, was turned off and doesn't bother anybody any more. CentOS CI is taken care of by @mrc0mmand, Semaphore was turned into something useful by @martinpitt recenlty and Ubuntu CI, well, it's more or less fine as long as it isn't ignored and the maintainers are notified of any weirdness happening there so I don't know why anybody would ignore the CI now.