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 322 forks source link

tstr, conf: Ensure error strings are nul-terminated #1342

Closed ac000 closed 5 days ago

ac000 commented 6 days ago

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.

ac000 commented 6 days ago

@pkillarjun

I can't add you as a reviewer, but could you check it anyway and just comment if you're OK with it?

I've kept the link to the oss-fuzz report in, but could you confirm whether this bug report is publicly accessible or not (no point having it in the commit message if it's not...)

Cheers, Andrew

pkillarjun commented 6 days ago

I can't add you as a reviewer, but could you check it anyway and just comment if you're OK with it?

Ok, and this is much better.

I've kept the link to the oss-fuzz report in, but could you confirm whether this bug report is publicly accessible or not (no point having it in the commit message if it's not...)

The report will not be made public until 90 days have passed or the issue is fixed.

ac000 commented 6 days ago
$ git range-diff efdd5540...4c99cb43
1:  efdd5540 ! 1:  4c99cb43 tstr, conf: Ensure error strings are nul-terminated
    @@ Commit message
         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>
    +    Signed-off-by: Arjun <pkillarjun@protonmail.com>
         Co-developed-by: Zhidao HONG <z.hong@f5.com>
         Signed-off-by: Zhidao HONG <z.hong@f5.com>
    +    Link: <https://github.com/google/oss-fuzz>
    +    Link: <https://oss-fuzz.com/testcase-detail/5545917827055616>
         Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
    -    Signed-off-by: Arjun <pkillarjun@protonmail.com>
         [ Commit message/subject - Andrew ]
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
ac000 commented 5 days ago
$ git range-diff 4c99cb43...8e254a4d
1:  4c99cb43 ! 1:  8e254a4d tstr, conf: Ensure error strings are nul-terminated
    @@ Commit message
         Co-developed-by: Zhidao HONG <z.hong@f5.com>
         Signed-off-by: Zhidao HONG <z.hong@f5.com>
         Link: <https://github.com/google/oss-fuzz>
    -    Link: <https://oss-fuzz.com/testcase-detail/5545917827055616>
         Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
         [ Commit message/subject - Andrew ]
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>