nginx / unit

NGINX Unit - universal web app server - a lightweight and versatile open source server that simplifies the application stack by natively executing application code across eight different programming language runtimes.
https://unit.nginx.org
Apache License 2.0
5.25k stars 320 forks source link

fuzzing: add cifuzz workflow & fix some oss-fuzz issues #1327

Open pkillarjun opened 1 week ago

pkillarjun commented 1 week ago

Continuing the fuzzing integration: https://github.com/nginx/unit/pull/1291.

Adding cifuzz for checking build failures and PR fuzzing. Using the main example provided by the OSS-Fuzz team, with an updated time of only 300 seconds.

pkillarjun commented 1 week ago

FAQ

Upload Crash

Upload Crash uses artifacts to save crashes found by fuzzing as artifacts.

Artifacts: When the fuzzer crashes the input file that causes the crash is uploaded as an artifact.
To download the artifact, do the following steps:

Upload Sarif

There is no proper information available that I could find. But, when a crash is found, the OSS-Fuzz infrastructure is alerted using SARIF files. A nice detailed report is then generated for maintainers with a better UI.

My Souce: Souce-1, Souce-2 .

github-advanced-security[bot] commented 1 week ago

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

pkillarjun commented 1 week ago

I doubt that.

2024-06-18 02:24:35,946 - root - INFO - Fuzzer: fuzz_http_h1p_peer. Detected bug.
2024-06-18 02:24:35,946 - root - INFO - Trying to reproduce crash using: /tmp/tmp66kfxbhm/crash-293e3405de4ccb599b6e4f770ddf25362554727c.

Let's wait for 24 hours and then see.

Edit: Hmmm, indeed, it was a sync problem.

pkillarjun commented 1 week ago

Fix some harness bugs https://github.com/nginx/unit/pull/1327/commits/450b9f6d27b606c73beb603f139a439cd18e9449

hongzhidao commented 1 day ago

I'm fine with it. It can be a separate fix.

I wasn't even aware (or if I was I had completely forgotten) that nxt_sprintf() doesn't nul-terminate strings by default.

Yep, it's not nul-temmiate by default.

The commit subject prefix could probably be tstr, conf:. Perhaps something like tstr, conf: Ensure error strings are nul-terminated

We could show the concrete function nxt_tstr_test() which is not used there.

Btw, I'd like to use pkillarjun as the signed-off instead of me. He reported the issue and fixed it with my suggestion.

ac000 commented 1 day ago

I'm fine with it. It can be a separate fix.

It it's in a separate PR then it's not reliant on getting this whole thing in...

I wasn't even aware (or if I was I had completely forgotten) that nxt_sprintf() doesn't nul-terminate strings by default.

Yep, it's not nul-temmiate by default.

The commit subject prefix could probably be tstr, conf:. Perhaps something like tstr, conf: Ensure error strings are nul-terminated

We could show the concrete function nxt_tstr_test() which is not used there.

Yes, that would be part of the commit message explaining that you need to explicitly nul-terminate the string.

Btw, I'd like to use pkillarjun as the signed-off instead of me. He reported the issue and fixed it with my suggestion.

You can have multiple Signed-off-by:'s and a Co-developed-by: should be followed by a Signed-off-by:

So the whole thing might look like

tstr, conf: Ensure error strings are nul-terminated

This issue was found with oss-fuzz.                                             

  ==18420==WARNING: MemorySanitizer: use-of-uninitialized-value                 
              #0 0x55dd798a5797 in nxt_vsprintf unit/src/nxt_sprintf.c:163:31   
              #1 0x55dd798d5bdb in nxt_conf_vldt_error unit/src/nxt_conf_validation.c:1525:11
              #2 0x55dd798dd4cd in nxt_conf_vldt_var unit/src/nxt_conf_validation.c:1560:16
              #3 0x55dd798dd4cd in nxt_conf_vldt_if unit/src/nxt_conf_validation.c:1592:16
              #4 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
              #5 0x55dd798d6f84 in nxt_conf_vldt_access_log unit/src/nxt_conf_validation.c:3426:11
              #6 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
              #7 0x55dd798d47bd in nxt_conf_validate unit/src/nxt_conf_validation.c:1421:11
              #8 0x55dd79871c82 in LLVMFuzzerTestOneInput unit/fuzzing/nxt_json_fuzz.c:67:5
              #9 0x55dd79770620 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
              #10 0x55dd7975adb4 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327:6
              #11 0x55dd7976084a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862:9
              #12 0x55dd7978cc42 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
              #13 0x7e8192213082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
              #14 0x55dd7975188d in _start                                      

            Uninitialized value was created by an allocation of 'error.i' in the stack frame
              #0 0x55dd798dd42b in nxt_conf_vldt_var unit/src/nxt_conf_validation.c:1557:5
              #1 0x55dd798dd42b in nxt_conf_vldt_if unit/src/nxt_conf_validation.c:1592:16

The issue was in nxt_tstr_test() where we create an error message with
nxt_sprintf(), where this error message is then later used with the
'%s' format specifier which expects a nul-terminated string, but by
default nxt_sprintf() doesn't nul-terminate, you must use the '%Z'
specifier to signify a '\0' at the end of the string.            

Link: <https://github.com/google/oss-fuzz>
Link: <https://oss-fuzz.com/testcase-detail/5545917827055616>
Co-developed-by: Zhidao HONG <z.hong@f5.com>
Signed-off-by: Zhidao HONG <z.hong@f5.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Arjun <pkillarjun@protonmail.com>
[ Commit message/subject - Andrew ]
Signed-off-by: Andrew Clayton <andrew@nginx.com>

The second link may or may not be included, depending if it's publicly accessible or not...

ac000 commented 1 day ago

@hongzhidao @pkillarjun

If you're good with the commit message etc, then I'm happy to merge this patch in and then it can simply be dropped from this PR...

EDIT: Nuts, only I can't because it would need approving... I'll create a new PR for this patch...

ac000 commented 1 day ago

PR for bug fix https://github.com/nginx/unit/pull/1342

ac000 commented 17 hours ago

OK @pkillarjun you can remove this patch from this PR now, thanks!

conf: add %Z to resolve oss-fuzz issue 69758 use-of-uninitialized-value