From b430f2c93b17bb3d269abd9ac035bc696aefc326 Mon Sep 17 00:00:00 2001 From: Badr Bacem KAABIA Date: Sat, 1 Nov 2025 11:44:27 +0100 Subject: [PATCH] Fix: Enforce MQTT v5 property-packet validation and improve decoding safety Implements necessary protocol validation in MqttEncode_Props and MqttDecode_Props to ensure that properties are only used in their allowed packet types, addressing the 'TODO: validate packet type'. - Defines new error code: MQTT_CODE_ERROR_PROPERTY_MISMATCH. - Adds critical buffer boundary checks in MqttDecode_Props before VBI and string decoding to prevent potential buffer overruns. Signed-off-by: Badr Bacem KAABIA --- src/mqtt_packet.c | 32 ++++++++++++++++++++++++-------- wolfmqtt/mqtt_types.h | 1 + 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/mqtt_packet.c b/src/mqtt_packet.c index de06fe0e..f65562c0 100644 --- a/src/mqtt_packet.c +++ b/src/mqtt_packet.c @@ -377,13 +377,20 @@ int MqttEncode_Props(MqttPacketType packet, MqttProp* props, byte* buf) /* TODO: Check against max size. Sometimes all properties are not expected to be added */ - /* TODO: Validate property type is allowed for packet type */ /* loop through the list properties */ while ((cur_prop != NULL) && (rc >= 0)) { - /* TODO: validate packet type */ - (void)packet; + if (cur_prop->type >= sizeof(gPropMatrix) / sizeof(gPropMatrix[0])) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PROPERTY); + break; + } + + /* Validate property type is allowed for this packet type */ + if (!(gPropMatrix[cur_prop->type].packet_type_mask & (1 << packet))) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PROPERTY_MISMATCH); + break; + } /* Encode the Identifier */ tmp = MqttEncode_Vbi(buf, (word32)cur_prop->type); @@ -505,11 +512,12 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, break; } - /* Decode the Identifier */ - if (buf_len < (word32)(buf - pbuf)) { + /* Check boundary before VBI decoding */ + if ((buf - pbuf) > (int)buf_len) { rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); break; } + /* Decode the Identifier */ rc = MqttDecode_Vbi(buf, (word32*)&cur_prop->type, (word32)(buf_len - (buf - pbuf))); if (rc < 0) { @@ -520,14 +528,17 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, total += tmp; prop_len -= tmp; - /* TODO: Validate property type is allowed for packet type */ - (void)packet; - if (cur_prop->type >= sizeof(gPropMatrix) / sizeof(gPropMatrix[0])) { rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PROPERTY); break; } + /* Validate property type is allowed for packet type */ + if (!(gPropMatrix[cur_prop->type].packet_type_mask & (1 << packet))) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PROPERTY_MISMATCH); + break; + } + switch (gPropMatrix[cur_prop->type].data) { case MQTT_DATA_TYPE_BYTE: @@ -561,6 +572,11 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, } case MQTT_DATA_TYPE_STRING: { + if ((buf - pbuf) > (int)buf_len) { + /* Should already be caught earlier, but safe to recheck */ + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); + break; + } tmp = MqttDecode_String(buf, (const char**)&cur_prop->data_str.str, &cur_prop->data_str.len, diff --git a/wolfmqtt/mqtt_types.h b/wolfmqtt/mqtt_types.h index fa166e58..3487dc7c 100644 --- a/wolfmqtt/mqtt_types.h +++ b/wolfmqtt/mqtt_types.h @@ -202,6 +202,7 @@ enum MqttPacketResponseCodes { MQTT_CODE_ERROR_CURL = -16, /* An error in libcurl that is not clearly * a network, memory, TLS, or system error. */ #endif + MQTT_CODE_ERROR_PROPERTY_MISMATCH = -17, MQTT_CODE_CONTINUE = -101, MQTT_CODE_STDIN_WAKE = -102,