search

tytso / e2fsprogs

Ext2/3/4 file system utilities
http://ext4.wiki.kernel.org
383 stars 224 forks source link

oss-fuzz targets need improvement #198

Open ramosian-glider opened 1 month ago

ramosian-glider commented 1 month ago

The existing libFuzzer targets used by oss-fuzz may require some massage. I was looking at ./tests/fuzz/ext2fs_image_read_write_fuzzer built with ./configure --enable-fuzzing --enable-addrsan, and want to share my findings.

  1. Running ext2fs_image_read_write_fuzzer produces a lot of warnings like "Attempt to read block from filesystem resulted in short read while trying to open file system". These are not needed during fuzzing, is it possible to suppress them (e.g. by using a special error table with empty strings?)
  2. At some point the test dies with OOM, with memory leaking from add_error_table(). We need to properly tear it down by calling remove_error_table(&et_ext2_error_table) at the end of LLVMFuzzerTestOneInput().
  3. At startup, ext2fs_image_read_write_fuzzer reports there are 203 coverage counters in the binary, which is suspiciously few. My understanding is that by default coverage instrumentation is only added to tests/fuzz, and e.g. lib/ is left uninstrumented. As a result, the fuzzer is mostly blindfolded. When I manually update the Makefiles to pass -fsanitize=fuzzer at compile time[^1], the number of coverage counters in the resulting binary goes up to 6K+.
  4. Even after this, the coverage reported by libFuzzer on local runs doesn't go above 191. I think we must be hitting some sort of CRC check, which is also confirmed by the lib/ext2fs coverage report from the oss-fuzz dashboard. I am not completely sure though, how oss-fuzz notices coverage in lib/ext2fs/crc16.c, which is also not instrumented. It might be a good idea to provide a debug config disabling checksums, or teach the fuzzer to recalculate them from the modified binary.

[^1]: Proper implementation requires some plumbing, because binaries that are not fuzz targets cannot be linked with -fsanitize=fuzzer.

tytso commented 1 month ago

The oss-fuzz was originally written by a intern, and I'm really not all that familiar with how the oss-fuzz project builds the fuzzer. I added it to e2fsprogs just to make ti easier for me to try to work oss-fuzz bugs. So I don't really know how to deal with (3).

As far as (1) is concerned, why are the warnings a problem?

ramosian-glider commented 1 month ago

Warnings aren't a problem per se, but they aren't useful either. They can also slow down the fuzzing process, which is unfortunate when fuzzing locally. For oss-fuzz it is probably less of an issue, because they have way more machine time.

But a more important thing is that the fuzzer does not gain new coverage over time. Does the report linked above look representative to you, or maybe we are indeed hitting some roadblock?

tytso commented 1 month ago

Checksums are only used if the metadata_csum feature is enabled. So there's no real need to have some magic debugging flag to disable checksums; you could just start the file system fuzzing seed without the metadata_csum feature.

But checksums are not the primary reason for the lack of coverage. Just take a look at the three fuzzing programs, ext2fs_check_directory_fuzzer, ext2fs_read_bitmap_fuzzer, ext2fs_image_read_write_fuzzer. The number of functions in the libext2fs library which these three fuzzing programs is a super tiny fraction. And in particular, the ext2fs_image_read_write_fuzzer program is super pointless. It exercises functions used primarily by e2image, which is rarely used, and when it is, it's by developers or users who are asked by developers as part of a bug report. In any case, the inode and directory iterator, the allocation functions, etc., can never be called given the nature of the fuzzers.

The bottom line is that the fuzzers were written by a intern, and given the huge number of false positives combined with the threat "you must fix this right away or we will tell the world" has caused me to largely lose most of my respect for oss-fuzz.
For example, if the fuzzer causes the file system appears to be say, 3TB, that's a completely valid file system, but it's going to result in more memory getting allocated (and this is normal) than the paltry 3 or 6MB that oss-fuzz triggers on, and then it will say, "security bug! Fix it or I will tell the whole world that this is a security vulnerability!" And I've complained to the oss-fuzz folks that this is just silly/wrong, and they rejected all of my suggestions for how to filter this sort of nonsense out. So if they aren't going to help me out reduce false positives, I'm not particularly incentivized to invest any time on improving it.

About the only system that has wasted more of my time is syzbot.....

tytso commented 1 month ago

By the way, the recently published 2024 State of the Open Source Maintainer Report noted that open source maintainers are swamped, and spend three times on security issues compared to 2021. So I really don't appreciate it when security teams published faulty tools that have a lot of false positives, and then dump the work on open source maintainers to have to deal with it.

But hey, if you want to work on improving things, as always, patches are greatly appreciated. :-)