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

Add PHAR fuzzer #5424

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add PHAR fuzzer #5424

wants to merge 3 commits into from

Conversation

smalyshev
Copy link
Contributor

@smalyshev smalyshev commented Apr 20, 2020

This adds phar extension fuzzer, based on existing phar tests.

@nikic

This comment has been minimized.

@nikic
Copy link
Member

nikic commented Apr 21, 2020

After fixing that, we get:

==23848== ERROR: libFuzzer: out-of-memory (malloc(3305111552))
   To change the out-of-memory limit use -rss_limit_mb=<N>

    #0 0x4f7ed1 in __sanitizer_print_stack_trace (/home/nikic/php-src-fuzz/sapi/fuzzer/php-fuzz-phar+0x4f7ed1)
    #1 0x476f6b in fuzzer::PrintStackTrace() (/home/nikic/php-src-fuzz/sapi/fuzzer/php-fuzz-phar+0x476f6b)
    #2 0x45af45 in fuzzer::Fuzzer::HandleMalloc(unsigned long) (/home/nikic/php-src-fuzz/sapi/fuzzer/php-fuzz-phar+0x45af45)
    #3 0x45ae4a in fuzzer::MallocHook(void const volatile*, unsigned long) (/home/nikic/php-src-fuzz/sapi/fuzzer/php-fuzz-phar+0x45ae4a)
    #4 0x4fe237 in __sanitizer::RunMallocHooks(void const*, unsigned long) (/home/nikic/php-src-fuzz/sapi/fuzzer/php-fuzz-phar+0x4fe237)
    #5 0x4793f5 in __asan::Allocator::Allocate(unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType, bool) (/home/nikic/php-src-fuzz/sapi/fuzzer/php-fuzz-phar+0x4793f5)
    #6 0x478bc3 in __asan::asan_malloc(unsigned long, __sanitizer::BufferedStackTrace*) (/home/nikic/php-src-fuzz/sapi/fuzzer/php-fuzz-phar+0x478bc3)
    #7 0x4ef67b in malloc (/home/nikic/php-src-fuzz/sapi/fuzzer/php-fuzz-phar+0x4ef67b)
    #8 0x105ce69 in __zend_malloc /home/nikic/php-src-fuzz/Zend/zend_alloc.c:2976:14
    #9 0xa82c7d in phar_parse_zipfile /home/nikic/php-src-fuzz/ext/phar/zip.c:426:19
    #10 0xad6f82 in phar_open_from_fp /home/nikic/php-src-fuzz/ext/phar/phar.c:1720:12
    #11 0xad4e48 in phar_create_or_parse_filename /home/nikic/php-src-fuzz/ext/phar/phar.c:1364:7

Could be resolved either by limiting max uncompressed size (via ini setting?) or by making memory limit work under USE_ZEND_ALLOC=0, which is implemented as part of #5030.

@nikic

This comment has been minimized.

@nikic
Copy link
Member

nikic commented Apr 21, 2020

I've opened a bug for that issue at https://bugs.php.net/bug.php?id=79503.

A general problem with the fuzzing here is that tars have checksums (even though they seem primitive). We probably need to disable checksum verification under FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION, but would then also need tooling to insert correct checksums for normal test cases.

I'm going to leave it at that for now, I think this needs more work before it's usable for oss-fuzz.

@smalyshev
Copy link
Contributor Author

Not sure what to do with OOM errors - for a format that includes object sizes, it's trivial to specify very large object size, and I think for PHP OOM is exactly what should happen in this case - that's why emalloc has memory limits. Adding another check there would not be of much use. It'd be the best if we could just shut up OOM errors, as emalloc handles them just fine and it's not what we want the fuzzer for - but I am not sure whether there's an option for that.

@smalyshev
Copy link
Contributor Author

Hmm, I wasn't aware of .phar/.metadata.bin thing. This seems dangerous as unserialize() is not safe, so phar is not safe then too?

smalyshev and others added 3 commits April 22, 2020 16:55
@nikic
Copy link
Member

nikic commented Apr 22, 2020

With USE_TRACKED_ALLOC=1 we now avoid leaks on bailout and OOM issues. However, we get continuously increasing (but non-leaking) memory usage, likely because phar has some global state that does not get cleared.

We'd have to check coverage data to see whether the fuzzer is actually doing anything useful, or getting stumped by checksum checks.

@nikic
Copy link
Member

nikic commented Apr 22, 2020

It looks the the increasing memory usage (that causes leak sanitizer to be auto disabled) is caused by realpath cache:

4194225 byte(s) (12%) in 55923 allocation(s)
    #0 0x4ef61d in malloc (/home/nikic/php-src-fuzz/sapi/fuzzer/php-fuzz-phar+0x4ef61d)
    #1 0x125d7aa in realpath_cache_add /home/nikic/php-src-fuzz/Zend/zend_virtual_cwd.c:390:35
    #2 0x125d7aa in tsrm_realpath_r /home/nikic/php-src-fuzz/Zend/zend_virtual_cwd.c:958:4
    #3 0x125ba5b in virtual_file_ex /home/nikic/php-src-fuzz/Zend/zend_virtual_cwd.c:1098:16
    #4 0xf4df8f in expand_filepath_with_mode /home/nikic/php-src-fuzz/main/fopen_wrappers.c:818:6
    #5 0xfb3ce2 in _php_stream_fopen /home/nikic/php-src-fuzz/main/streams/plain_wrapper.c:1040:7
    #6 0xfb5cc4 in php_plain_files_stream_opener /home/nikic/php-src-fuzz/main/streams/plain_wrapper.c:1130:9
    #7 0xf9e78e in _php_stream_open_wrapper_ex /home/nikic/php-src-fuzz/main/streams/streams.c:2104:13
    #8 0xad4f61 in phar_create_or_parse_filename /home/nikic/php-src-fuzz/ext/phar/phar.c:1356:7
    #9 0x15e3523 in LLVMFuzzerTestOneInput /home/nikic/php-src-fuzz/sapi/fuzzer/fuzzer-phar.c:57:3

There was a similar issue with the original exif fuzzer. It would be best if we can avoid going through a file at all, or open the file without going through realpath cache (iirc there's a flag for that).

@smalyshev
Copy link
Contributor Author

smalyshev commented Apr 22, 2020

Unfortunately phar.c only exports the file endpoint, if we made the fp function non-static we could avoid all path cache issues probably, as we do with exif using memory file.

We could probably export the fp function and try with that if it improves things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants