buglwIP - A Lightweight TCP/IP stack - Bugs: bug #52345, MQTT buffer length check seems...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #52345: MQTT buffer length check seems wrong

Submitter:  David Bourgeois <jaguarondi>
Submitted:  Sun 05 Nov 2017 11:28:13 PM UTC
   
 
Category:  apps Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Fri 16 Feb 2018 10:55:26 AM UTC, comment #10: 

Fix confirmed on our side too. Thanks

David Bourgeois <jaguarondi>
Wed 14 Feb 2018 11:08:59 AM UTC, comment #9: 

Should be fixed in eea95459c9b0f150e9b9ab24e80e27e09c2612e5

Simon Goldschmidt <goldsimon>
Group administrator
Mon 12 Feb 2018 08:56:10 PM UTC, comment #8: 

After testing this agains test.mosquitto.org, I think the original fix was not too bad...

Simon Goldschmidt <goldsimon>
Group administrator
Fri 09 Feb 2018 02:34:26 AM UTC, comment #7: 

Here's how to trigger the issue and the wireshark capture (file mqtt_bug_52345_capture.pcapng attached)

Below is the code of the client:


extern mqtt_client_t *mqtt_client;
int ip[4] = {10,192,55,55};
mqtt_connect(ip, "client_test");
mqtt_subscribe(mqtt_client, "topic_qos1", MQTT_QOS_1, NULL, NULL);
osDelay(100);
mqtt_subscribe(mqtt_client, "topic_qos0", MQTT_QOS_0, NULL, NULL);
while(1){
    osDelay(1000);
}



The following commands where issued during the capture:

$ mosquitto_pub -t topic_qos1 -m "test1" -q 1
$ mosquitto_pub -t topic_qos1 -m "" -q 1
$ mosquitto_pub -t topic_qos0 -m "test0" -q 0
$ mosquitto_pub -t topic_qos0 -m "" -q 0


The last command triggers the bug: discard an otherwise valid packet (packet 22 of the capture) and closes the connection.

If I replace the following line in mqtt.c
    if ((after_topic + (qos ? 2 : 1)) > length) {
by
    if ((after_topic + (qos ? 2 : 0)) > length) {

the packet is accepted, but the code following the test will write one byte past the end of the buffer so this isn't a possible solution.

David Bourgeois <jaguarondi>
Mon 29 Jan 2018 06:04:32 AM UTC, comment #6: 

David, can you provide a wireshark monitoring of the packets that trigger the issue?

Dirk Ziegelmeier <dziegel>
Group administrator
Mon 18 Dec 2017 10:23:37 AM UTC, comment #5: 

Alright, reopened.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 18 Dec 2017 10:13:16 AM UTC, comment #4: 

Sorry for the late answer. We still found a case where this fix doesn't work. When the packet sent has no payload and a QoS of 0, then the last byte of the packet is actually the last byte of the payload which isn't null terminated. So the test will always fail in this case and there's no way to add the null termination for the callback.

So here the fix should not only consider the test but also deal with the buffer size.

David Bourgeois <jaguarondi>
Sun 17 Dec 2017 09:26:18 PM UTC, comment #3: 

Let's set this fixed until someone complains.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 26 Nov 2017 08:09:10 PM UTC, comment #2: 

Rewrote the fix, I think your fix was wrong. The problem is that the code tries to parse the variable header part of the publish although there is none in the telegram.

Dirk Ziegelmeier <dziegel>
Group administrator
Mon 13 Nov 2017 09:30:50 PM UTC, comment #1: 

From reading the code, it should IMHO be compared to MQTT_VAR_HEADER_BUFFER_LEN - fixed_hdr_idx.

Can you please test this and report back? Thanks in advance!

Dirk Ziegelmeier <dziegel>
Group administrator
Sun 05 Nov 2017 11:28:13 PM UTC, original submission:  

The buffer length check fails with an empty payload, it seems the comparison should be done towards MQTT_VAR_HEADER_BUFFER_LEN but I don't know if there are some other length to take into account also.

Here's what I did:

In LwIP/src/apps/mqtt/mqtt.c
       /* Check length, add one byte even for QoS 0 so that zero termination will fit */
-      if ((after_topic + (qos ? 2 : 1)) > length) {
+      if ((after_topic + (qos ? 2 : 1)) > MQTT_VAR_HEADER_BUFFER_LEN) {
         LWIP_DEBUGF(MQTT_DEBUG_WARN, ("mqtt_message_received: Receive buffer can not fit topic + pkt_id\n"));
         goto out_disconnect;


David Bourgeois <jaguarondi>

 

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

Attached Files
file #43210:  mqtt_bug_52345_capture.pcapng.gz added by jaguarondi (1KiB - application/x-gzip - Capture while the bug is triggered)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by jaguarondi (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-02-14 goldsimon StatusIn Progress Fixed
        Assigned todziegel goldsimon
        Open/ClosedOpen Closed
    2018-02-09 jaguarondi Attached File- Added mqtt_bug_52345_capture.pcapng.gz, #43210
    2017-12-18 goldsimon StatusFixed In Progress
        Open/ClosedClosed Open
    2017-12-17 goldsimon StatusReady For Test Fixed
        Assigned toNone dziegel
        Open/ClosedOpen Closed
    2017-11-13 dziegel StatusNone Ready For Test

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code