MQTT-C Security Issue Report (mqtt_unpack_publish_response)

[GANGE666]

Overview

An issue was discovered in MQTT-C through 1.1.5. The MQTT input data processing function mqtt_unpack_publish_response in mqtt.c does not validate the length of incoming topic_name_size, which leads to an out-of-bounds read when subsequent processing of the input data. And this could also lead to an integer overflow when calculating the remaining length of incoming response. Eventually causing Denial-of-Service or an information leak, even remote code execution.

Description

In mqtt_unpack_publish_response, topic_name_size is unpack from input data directly (Line 1352). And then buf pointer add topic_name_size without checking if it exceeds the range of buf, which leads to a buffer overflow. ([Line 1355])

And if attacker provide a topic_name_size is bigger than remaining_length, which could leads to an integer overflow. ([Line 1365] and [Line 1367])

ssize_t mqtt_unpack_publish_response(struct mqtt_response *mqtt_response, const uint8_t *buf)
{
...
		/* parse variable header */
    response->topic_name_size = __mqtt_unpack_uint16(buf);
    buf += 2;
    response->topic_name = buf;
    buf += response->topic_name_size;                           // buffer overflow
		
if (response->qos_level > 0) {
        response->packet_id = __mqtt_unpack_uint16(buf);
        buf += 2;
    }

    /* get payload */
    response->application_message = buf;
    if (response->qos_level == 0) {
        response->application_message_size = fixed_header->remaining_length - response->topic_name_size - 2;  // integer overflow                
    } else {
        response->application_message_size = fixed_header->remaining_length - response->topic_name_size - 4;  // integer overflow
    }
    buf += response->application_message_size;
...
}

MQTT-C/src/mqtt.c

Lines 1332 to 1373 in be12c34

	ssize_t mqtt_unpack_publish_response(struct mqtt_response *mqtt_response, const uint8_t *buf)
	{
	const uint8_t *const start = buf;
	struct mqtt_fixed_header *fixed_header;
	struct mqtt_response_publish *response;
	fixed_header = &(mqtt_response->fixed_header);
	response = &(mqtt_response->decoded.publish);
	/* get flags */
	response->dup_flag = (fixed_header->control_flags & MQTT_PUBLISH_DUP) >> 3;
	response->qos_level = (fixed_header->control_flags & MQTT_PUBLISH_QOS_MASK) >> 1;
	response->retain_flag = fixed_header->control_flags & MQTT_PUBLISH_RETAIN;
	/* make sure that remaining length is valid */
	if (mqtt_response->fixed_header.remaining_length < 4) {
	return MQTT_ERROR_MALFORMED_RESPONSE;
	}
	/* parse variable header */
	response->topic_name_size = __mqtt_unpack_uint16(buf);
	buf += 2;
	response->topic_name = buf;
	buf += response->topic_name_size;
	if (response->qos_level > 0) {
	response->packet_id = __mqtt_unpack_uint16(buf);
	buf += 2;
	}
	/* get payload */
	response->application_message = buf;
	if (response->qos_level == 0) {
	response->application_message_size = fixed_header->remaining_length - response->topic_name_size - 2;
	} else {
	response->application_message_size = fixed_header->remaining_length - response->topic_name_size - 4;
	}
	buf += response->application_message_size;
	/* return number of bytes consumed */
	return buf - start;
	}

Impact

Denial-of-Service or an information leak, even remote code execution.

[shayneoneill]

Is this fixed in any forks? Its a little disturbing this has sat around since febuary not even acknowledged.

For all we know this could have been actively exploited for the last 8+ months without even a "We'll look into it".

Does anyone have any recommendations for an alternative library for embedded microcontrollers that is, in fact, maintained?

[LiamBindle]

Happy to accept a PR fixing this. I maintain this repo casually and I'm not paid; MQTT-C in it's current form is stable and pretty widely used. It looks like the author described how to fix the problem---not sure why this wasn't submitted as a PR, but I'm happy to merge a fix.