lwIP - A Lightweight TCP/IP stack - Bugs: bug #61480, MQTT: RCE caused by buffer overflow
You are not allowed to post comments on this tracker with your current authentication level.
bug #61480: MQTT: RCE caused by buffer overflow
Submitter: | Hyeonsu Kim <icekrim> | ||
Submitted: | Sun 14 Nov 2021 07:22:57 AM UTC | ||
Category: | Security-related | Severity: | 3 - Normal |
Item Group: | Crash Error | Status: | None |
Privacy: | Public | Assigned to: | None |
Open/Closed: | Open | Planned Release: | None |
lwIP version: | git head |
Sun 02 Jan 2022 10:55:09 PM UTC, comment #5: |
Wouter van Gulik <wouter> |
Thu 30 Dec 2021 10:31:44 PM UTC, comment #4: I have written some fixes, but I keep battling corner cases.
|
Wouter van Gulik <wouter> |
Mon 27 Dec 2021 08:47:47 PM UTC, comment #3: New version that actually compiles |
Wouter van Gulik <wouter> |
Mon 27 Dec 2021 08:39:28 PM UTC, comment #2: I went ahead and created two unit tests that trigger the bugs. |
Wouter van Gulik <wouter> |
Wed 22 Dec 2021 03:17:57 PM UTC, comment #1: Can you please elaborate on how the overflow actually happens?
|
Wouter van Gulik <wouter> |
Sun 14 Nov 2021 07:22:57 AM UTC, original submission:
Summary:
|
Hyeonsu Kim <icekrim> |
Depends on the following items: None found
Items that depend on this one: None found
There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.
Follow 2 latest changes.
Date | Changed by | Updated Field | Previous Value | => | Replaced by |
---|---|---|---|---|---|
2021-12-27 | wouter | Attached File | - | Added mqtt_unit_tests.patch, #52574 | |
2021-12-27 | wouter | Attached File | - | Added mqtt_unit_tests.patch, #52573 |
Powered by Savane 3.13-bb6a.
Corresponding source code
One of the thins I do not get my head around is the use of pbuf_get_contiguous combined with the handling of a publish message. The publish message stills checks for sizeof(rx_buffer). Which is totally bogus if we receive a message via the zero-copy mechanism.
This means any publish message large than MQTT_VAR_HEADER_BUFFER_LEN will be dropped, while, if in one pbuf, could be parsed by mqtt_message_received. However if the topic would be split in multiple pbufs the mqtt_message_received function would still fail; the function can only properly handle a topic if received all at once.
This all-at-once behaviour is 'documented' by the comments on MQTT_VAR_HEADER_BUFFER_LEN. It states the buffer should be large enough to handle the maximum topic length.
So I my current guess is: removing the checks against MQTT_VAR_HEADER_BUFFER_LEN (via var_hdr_payload_bufsize) in the publish handling is a sensible thing todo; topic should be in a contigious buffer. Either the rx buffer or a single pbuf.
The checks seem to be a 'left over' from before the pbuf_get_contiguous addition.