net: nrf_provisioning: Fix possible buffer overflow #12484
Conversation
Enable CONFIG_ASAN on tests, fix possible buffer overflow and fix one memory leak on test case. Signed-off-by: Seppo Takalo <[email protected]>
github-actions
bot
added
the
changelog-entry-required
SeppoTakalo
removed
the
changelog-entry-required
Test specificationCI/Jenkins/NRF
CI/Jenkins/integration
Detailed information of selected test modules Note: This message is automatically posted and updated by the CI |
| @@ -284,6 +284,7 @@ static int exec_at_cmd(struct command *cmd_req, struct cdc_out_fmt_data *out) | |||
| LOG_ERR("Unable to write response msg field"); | |||
| return -ENOMEM; | |||
| } | |||
| memset(resp, 0, resp_sz); | |||
There was a problem hiding this comment.
This is the buffer overflow?
There was a problem hiding this comment.
Yes.
I believe so. The ASAN warning was coming from strlen() on this buffer, so probably this code copies content here without nul terminating.
I did not check too deeply, as the warning disappeared.
There was a problem hiding this comment.
*** Booting nRF Connect SDK v2.4.99-dev2-15-g4f612c9527bf ***
src/main.c:413:test_http_commands_auth_hdr_valid:PASS
src/main.c:440:test_http_commands_auth_hdr_invalid:PASS
src/main.c:468:test_http_commands_url_valid:PASS
src/main.c:495:test_http_commands_no_content_valid:PASS
src/main.c:522:test_http_commands_internal_server_error_invalid:PASS
src/main.c:549:test_http_commands_unknown_error_invalid:PASS
src/main.c:592:test_http_responses_valid:PASS
src/main.c:641:test_codec_finished_valid:PASS
=================================================================
==3811815==ERROR: AddressSanitizer: heap-buffer-overflow on address 0xf2a03680 at pc 0xf7921195 bp 0xf43fd708 sp 0xf43fd2d8
READ of size 1025 at 0xf2a03680 thread T2
#0 0xf7921194 in __interceptor_strlen ../../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:354
#1 0x56661b32 in put_at_resp /home/seta/src/ncs/nrf/subsys/net/lib/nrf_provisioning/src/nrf_provisioning_codec.c:145
0xf2a03680 is located 0 bytes to the right of 1024-byte region [0xf2a03280,0xf2a03680)
allocated by thread T2 here:
#0 0xf79ce817 in __interceptor_malloc ../../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144
#1 0x5664dcdd in __wrap_k_malloc /home/seta/src/ncs/nrf/tests/subsys/net/lib/nrf_provisioning/src/main.c:63
#2 0x566627c6 in exec_at_cmd /home/seta/src/ncs/nrf/subsys/net/lib/nrf_provisioning/src/nrf_provisioning_codec.c:282
#3 0x56663c59 in nrf_provisioning_codec_process_commands /home/seta/src/ncs/nrf/subsys/net/lib/nrf_provisioning/src/nrf_provisioning_codec.c:507
#4 0x56650c4d in test_codec_priv_keygen_valid /home/seta/src/ncs/nrf/tests/subsys/net/lib/nrf_provisioning/src/main.c:716
#5 0x5664d64b in run_test /home/seta/src/ncs/nrf/tests/subsys/net/lib/nrf_provisioning/build/runner/runner_main.c:130
#6 0x5664d777 in unity_main /home/seta/src/ncs/nrf/tests/subsys/net/lib/nrf_provisioning/build/runner/runner_main.c:155
#7 0x56653488 in _posix_zephyr_main /home/seta/src/ncs/nrf/tests/subsys/net/lib/nrf_provisioning/src/main.c:1326
#8 0x5666fd9f in bg_thread_main /home/seta/src/ncs/zephyr/kernel/init.c:347
#9 0x56654222 in z_thread_entry /home/seta/src/ncs/zephyr/lib/os/thread_entry.c:36
#10 0x56659c77 in posix_thread_starter /home/seta/src/ncs/zephyr/arch/posix/core/posix_core.c:305
#11 0xf78ee856 in asan_thread_start ../../../../../src/libsanitizer/asan/asan_interceptors.cc:199
#12 0xf76a2899 in __clone (/lib/i386-linux-gnu/libc.so.6+0x104899)
There was a problem hiding this comment.
I am wondering because usually if you have a sequence like this:
p = malloc(size);
// no chaches to p value nor size
memop(p, ..., size);
where you should be sure that memop, here memset, does not change p and respects size in a way to not write/read pass range of [p, size) this means that there is something wrong with he obtained pointer.
Looks like memset overwrites something, while it should not because the code gets valid pointer p (non-null), by the error logic, and size given to memset is exactly the same as the one used to allocate the buffer.
I think the problem is somewhere else or you have found problem with memset (or k_malloc).
There was a problem hiding this comment.
Oh.. sorry.. I think I answered your question wrong, the memset() is fixing the overflow, not causing it.
The problem is strlen() somewhere expecting to get buffer with nul terminator, but it was not.
There was a problem hiding this comment.
A second look:
Allocated buffer is directly fed to nrf_modem_at_cmd() and if that fails, the content of buffer is uninitialized, then it is fed to put_at_resp() which expects it to contain a response.
So clearing the buffer in malloc is the correct fix as we cannot touch nrf_modem_at_cmd.
There was a problem hiding this comment.
Oh.. sorry.. I think I answered your question wrong, the
memset()is fixing the overflow, not causing it.The problem is
strlen()somewhere expecting to get buffer with nul terminator, but it was not.
My mistake, I was somehow thinking that the memset removal fixes problem. Sorry.
Btw. you can just set the start of buffer to 0, if you assume that strlen always get the unmodified p, will be more efficient then looping it with memset.
Enable CONFIG_ASAN on tests, fix possible buffer overflow and fix one memory leak on test case.