Memory corruption in ws_transport.c (IDFGH-13585)

ToyboxZach commented 2 months ago

Answers checklist.

[X] I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
[X] I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
[X] I have searched the issue tracker for a similar issue and not found a similar issue.

IDF version.

master

Espressif SoC revision.

esp32

Operating System used.

Windows

How did you build your project?

VS Code IDE

If you are using Windows, please specify command line type.

None

Development Kit.

from code

Power Supply used.

USB

What is the expected behavior?

No memory corruption

What is the actual behavior?

There is a memory corruption in this loop.

If you end up reading the full header size (my use case is if my websocket server is down, it returns static html) this ends up writing one bit past the end of the buffer

if ((len = esp_transport_read(ws->parent, ws->buffer + header_len, WS_BUFFER_SIZE - header_len, timeout_ms)) <= 0) {
            ESP_LOGE(TAG, "Error read response for Upgrade header %s", ws->buffer);
            return -1;
        }
        header_len += len;
        ws->buffer_len = header_len;
        ws->buffer[header_len] = '\0'; // We will mark the end of the header to ensure that strstr operations for parsing the headers don't fail.
        ESP_LOGD(TAG, "Read header chunk %d, current header size: %d", len, header_len);

In the init functio nthere is

  ws->buffer = malloc(WS_BUFFER_SIZE);

header_len can be up to the size of WS_BUFFER_SIZE, which is how much memory is allocated in ws->buffer

When you do ws->buffer[WS_BUFFER_SIZE] then you will corrupt some memory. In my complicated use case this ends up crashing when freeing the transport later.

Steps to reproduce.

Try to do a WS connect with a sit that doesn't return a valid header

Debug Logs.

Not valid, since memory corruption makes but hit much later than when it actually happens.

More Information.

No response

david-cermak commented 1 month ago

Thank you for reporting this issue, and for pointing us in the right direction; this is a bug.

Since we're inserting a null term, we shouldn't be reading the entire WS_BUFFER_SIZE. Instead, we need to leave space for the terminator.

https://github.com/espressif/esp-idf/blob/59e18382702b6986be3d3f55e9ac7763c1397cf7/components/tcp_transport/transport_ws.c#L294

PS: If you're interested in fixing this bug, feel free to submit a PR, we'd be happy to review and merge it!

filzek commented 3 weeks ago

so the idea to fix it is to: ws->buffer = malloc(WS_BUFFER_SIZE+1);

or to check the size if the total is the same? which one should be the best one to solve the issue?

espressif / esp-idf