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

test_spdm_responder_chunk_send_ack: Heap-buffer-overflow in libspdm_copy_mem #2631

Closed
Zhiqiang520 opened this issue Mar 25, 2024 · 3 comments · Fixed by #2640
Closed

test_spdm_responder_chunk_send_ack: Heap-buffer-overflow in libspdm_copy_mem #2631

Zhiqiang520 opened this issue Mar 25, 2024 · 3 comments · Fixed by #2640
Assignees
Labels
bug Something isn't working

Comments

@Zhiqiang520
Copy link
Contributor

Zhiqiang520 commented Mar 25, 2024

New issue 67585 by ClusterFuzz-External: libspdm:test_spdm_responder_chunk_send_ack: Heap-buffer-overflow in libspdm_copy_mem
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=67585

Detailed Report: https://oss-fuzz.com/testcase?key=5274620108275712

Project: libspdm
Fuzzing Engine: libFuzzer
Fuzz Target: test_spdm_responder_chunk_send_ack
Job Type: libfuzzer_asan_libspdm
Platform Id: linux

Crash Type: Heap-buffer-overflow READ 1
Crash Address: 0x62b000006ebd
Crash State:
libspdm_copy_mem
libspdm_get_response_chunk_send
libspdm_get_response_chunk_send

Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Regressed: https://oss-fuzz.com/revisions?job=libfuzzer_asan_libspdm&range=202403210612:202403220618

Reproducer Testcase: https://oss-fuzz.com/download?testcase_id=5274620108275712

  1. Verified on d83ef43
    Ubuntu 20.04.2 LTS

cd libspdm
mkdir build
cd build
cmake -DARCH=x64 -DTOOLCHAIN=LIBFUZZER -DTARGET=Release -DCRYPTO=mbedtls -DGCOV=ON ..
make copy_sample_key
make
cd bin
./test_spdm_responder_chunk_send_ack ./clusterfuzz-testcase-minimized-test_spdm_responder_chunk_send_ack-5274620108275712
image

  1. The binary seed file: clusterfuzz-testcase-minimized-test_spdm_responder_chunk_send_ack-5274620108275712
    clusterfuzz-testcase-minimized-test_spdm_responder_chunk_send_ack-5274620108275712.zip
@Zhiqiang520
Copy link
Contributor Author

The root cause is as following:

  1. The Libfuzzer generate one scenario that try to use CHUNK_SEND request to send a large request, and this large request to be sent is also a CHUNK_SEND request.

  2. But Refer to the paragraph 805 In [DSP0274_1.3.0.pdf]
    (https://www.dmtf.org/sites/default/files/standards/documents/DSP0274_1.3.0.pdf).
    image

  3. Before entering into the following code block line 205~208, I think we should check the large request is not a CHUNK_SEND request or CHUNK_GET request.

    } else if (send_info->chunk_bytes_transferred == send_info->large_message_size) {
    libspdm_get_spdm_response_func response_func =
    libspdm_get_response_func_via_request_code(
    ((spdm_message_header_t*)send_info->large_message)->request_response_code);
    if (response_func != NULL) {
    status = response_func(
    spdm_context,
    send_info->large_message_size, send_info->large_message,
    &chunk_response_size, chunk_response);
    } else {

  4. It will not fail if we change the above code as following:
    image

@jyao1 jyao1 assigned Zhiqiang520 and jyao1 and unassigned Zhiqiang520 Mar 25, 2024
@jyao1
Copy link
Member

jyao1 commented Mar 27, 2024

I think the root-cause of the copy mem overflow is as follows:

  1. libspdm_get_response_chunk_send() function only checks: if (request_size < sizeof(spdm_chunk_send_request_t)) {
    https://github.com/DMTF/libspdm/blob/main/library/spdm_responder_lib/libspdm_rsp_chunk_send_ack.c#L64

But the first chunk message should at least sizeof(spdm_chunk_send_request_t) + sizeof(uint32_t).

  1. The malformed seed input first chunk message to be sizeof(spdm_chunk_send_request_t) only, which bypassed above check.

It caused problem later - large_message_size becomes garbage. calc_max_chunk_size becomes negative or big integer.

        large_message_size = *(const uint32_t*) (spdm_request + 1);
        chunk = (((const uint8_t*) (spdm_request + 1)) + sizeof(uint32_t));
        calc_max_chunk_size =
            ((uint32_t)request_size - (uint32_t)(chunk - (const uint8_t*) spdm_request));

https://github.com/DMTF/libspdm/blob/main/library/spdm_responder_lib/libspdm_rsp_chunk_send_ack.c#L95-L98

Then it impacts copy_mem later.

            libspdm_copy_mem(
                send_info->large_message, send_info->large_message_capacity,
                chunk, spdm_request->chunk_size);
  1. The attacker can control spdm_request->chunk_size, and can control at most 3 bytes after chunk, but cannot control more bytes after chunk. (That is because if attacker input 4 bytes, then it becomes a legal CHUNK_SEND_REQUEST.)

send_info->large_message_capacity is definitely bigger than 3 bytes. As such, the attacker cannot control what data overrides the buffer after send_info->large_message.

jyao1 added a commit to jyao1/libspdm that referenced this issue Mar 27, 2024
@jyao1 jyao1 added the bug Something isn't working label Mar 28, 2024
jyao1 added a commit that referenced this issue Mar 30, 2024
@jyao1
Copy link
Member

jyao1 commented Apr 1, 2024

@steven-bellock , do we need a CVE for this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants