buglwIP - 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: 

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.

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.
I do not get why a large initial packet should still be limited to the client rxbuf?
Is not better to first try maximum from current pbuf? Let's say you receive 200 bytes, then it will be split and processed in 127 and then the remainder?.
And does reassembly within the first 127 bytes really help? It will only help on pbuf borders. Does that really occur?
I imagine it a mqtt message is usually within a single pbuf.

Wouter van Gulik <wouter>
Mon 27 Dec 2021 08:47:47 PM UTC, comment #3: 

New version that actually compiles

(file #52574)

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.

(file #52573)

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?

If everything is in one pbuf the following code will limit the size to only what is available in the pbuf, which will cause a "zero-copy" by pbuf_get_contiguous . And no overflow will happen.


/* Adjust cpy_len to ensure zero-copy operation for remaining parts of current message */
if (client->msg_idx >= MQTT_VAR_HEADER_BUFFER_LEN) {
  if (cpy_len > (p->len - in_offset))
    cpy_len = p->len - in_offset;
}


The overflow will only happen if the pbuf is actually a chain of pbufs.

The 'p->len - in_offset' could underflow; in_offset is counted against p->tot_len. So only if multiple pbufs are used this check will underflow and cause the pbuf_get_contiguous call to make a write beyond the rx_buffer.

I am not familiar with internals of lwip to know when a pbuf queue is actually created for TCP RX. So perhaps this is always the case?

The 'p->len - in_offset' is just wrong. p is never updated and is always the first p, regardless of the current in_offset.

Wouter van Gulik <wouter>
Sun 14 Nov 2021 07:22:57 AM UTC, original submission:  

Summary:
Integer Overflow in mqtt_parse_incoming of src/apps/mqtt/mqtt.c allows attackers to perform a Remote Code Execution via MQTT packet with fixed header length of 129 bytes or more

Description:
When the length of fixed_header of mqtt packet is 128 or more, buffer overflow of virtually infinite length is possible by appropriately setting the header value.

mqtt.c 893
cpy_len = (u16_t)LWIP_MIN((u16_t)(p->tot_len - in_offset), msg_rem_len);
/* Limit to available space in buffer */
buffer_space = MQTT_VAR_HEADER_BUFFER_LEN - fixed_hdr_len;
if (cpy_len > buffer_space) {
  cpy_len = buffer_space;
}
/* Adjust cpy_len to ensure zero-copy operation for remaining parts of current message */
if (client->msg_idx >= MQTT_VAR_HEADER_BUFFER_LEN) {
  if (cpy_len > (p->len - in_offset))
    cpy_len = p->len - in_offset;
}
var_hdr_payload = (u8_t*)pbuf_get_contiguous(p, client->rx_buffer + fixed_hdr_len, buffer_space, cpy_len, in_offset);
....

As described in bug #61479, attacker can make fixed_hdr_len higher than 129.
Because MQTT_VAR_HEADER_BUFFER_LEN is 128, a negative number (in this case -1) is assigned to buffer_space, so buffer_space becomes a very large number.

cpy_len can be freely changed within the range of u16 type by a hacker by modifying the remaining length of the MQTT packet.
Therefore, an attacker can overwrite up to 65535 bytes from client->rx_buffer + fixed_hdr_len through pbuf_get_contiguous just by making fixed_hdr_len to 129.
This allows remote code execution in embedded systems to which memory protection is not applied.

I have completed the POC on the stm32 board I have personally, and I will prepare linux porting if necessary. Thank you.

Hyeonsu Kim <icekrim>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #52574:  mqtt_unit_tests.patch added by wouter (5KiB - text/x-patch - Unit tests that show the two bugs. v2, with compile fix. Never do last minute change.)
file #52573:  mqtt_unit_tests.patch added by wouter (5KiB - text/x-patch - Unit test that shows exposes the two bugs)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by wouter (Posted a comment)
  • -email is unavailable- added by icekrim (Submitted the item)
  •  

    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

    Back to the top

    Powered by Savane 3.13-bb6a.
    Corresponding source code