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

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

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

ac000
Copy link
Member

@ac000 ac000 commented Jun 25, 2024

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 ac000 requested a review from hongzhidao June 25, 2024 17:43
@ac000
Copy link
Member Author

ac000 commented Jun 25, 2024

@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
Copy link
Contributor

pkillarjun commented Jun 25, 2024

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
Copy link
Member Author

ac000 commented Jun 25, 2024

  • Slight re-ordering of commit tags to better reflect reality...
$ 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>

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
	      nginx#4 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
	      nginx#5 0x55dd798d6f84 in nxt_conf_vldt_access_log unit/src/nxt_conf_validation.c:3426:11
	      nginx#6 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
	      nginx#7 0x55dd798d47bd in nxt_conf_validate unit/src/nxt_conf_validation.c:1421:11
	      nginx#8 0x55dd79871c82 in LLVMFuzzerTestOneInput unit/fuzzing/nxt_json_fuzz.c:67:5
	      nginx#9 0x55dd79770620 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
	      nginx#10 0x55dd7975adb4 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327:6
	      nginx#11 0x55dd7976084a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862:9
	      nginx#12 0x55dd7978cc42 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
	      nginx#13 0x7e8192213082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
	      nginx#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.

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>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
[ Commit message/subject - Andrew ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
@ac000
Copy link
Member Author

ac000 commented Jun 25, 2024

  • Drop the issue report link. I think the important details are captured in the commit message and the link is still available on GH.
$ 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>

@ac000 ac000 merged commit 8e254a4 into nginx:master Jun 26, 2024
21 of 23 checks passed
@ac000 ac000 deleted the tstr-nul-terminate branch June 26, 2024 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants