From b57673ecd9fe79ae3d18b486692d8936b18502c2 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Fri, 2 Feb 2024 11:49:37 -0600 Subject: [PATCH 1/2] Fix string prop OOB read --- src/mqtt_packet.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/mqtt_packet.c b/src/mqtt_packet.c index 16e6d45d..96bd1106 100644 --- a/src/mqtt_packet.c +++ b/src/mqtt_packet.c @@ -549,7 +549,7 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, tmp = MqttDecode_String(buf, (const char**)&cur_prop->data_str.str, &cur_prop->data_str.len); - if (cur_prop->data_str.len <= (buf_len - (buf - pbuf))) { + if ((tmp >= 0) && ((word32)tmp <= (buf_len - (buf - pbuf)))) { buf += tmp; total += tmp; prop_len -= (word32)tmp; @@ -600,8 +600,7 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, tmp = MqttDecode_String(buf, (const char**)&cur_prop->data_str.str, &cur_prop->data_str.len); - if (cur_prop->data_str.len <= - (buf_len - (buf - pbuf))) { + if ((tmp >= 0) && ((word32)tmp <= (buf_len - (buf - pbuf)))) { buf += tmp; total += tmp; prop_len -= (word32)tmp; @@ -609,8 +608,8 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, tmp = MqttDecode_String(buf, (const char**)&cur_prop->data_str2.str, &cur_prop->data_str2.len); - if (cur_prop->data_str2.len <= - (buf_len - (buf - pbuf))) { + if ((tmp >= 0) && ((word32)tmp <= + (buf_len - (buf - pbuf)))) { buf += tmp; total += tmp; prop_len -= (word32)tmp; From d9f6be80f7041853e7d04a402b778812918fce64 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Thu, 22 Feb 2024 10:55:40 -0600 Subject: [PATCH 2/2] Check all variable byte decodes for valid length --- src/mqtt_packet.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/mqtt_packet.c b/src/mqtt_packet.c index 96bd1106..fa41863f 100644 --- a/src/mqtt_packet.c +++ b/src/mqtt_packet.c @@ -500,6 +500,10 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, } /* Decode the Identifier */ + if (buf_len < (word32)(buf - pbuf)) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); + break; + } rc = MqttDecode_Vbi(buf, (word32*)&cur_prop->type, (word32)(buf_len - (buf - pbuf))); if (rc < 0) { @@ -562,6 +566,10 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf, } case MQTT_DATA_TYPE_VAR_INT: { + if (buf_len < (word32)(buf - pbuf)) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); + break; + } tmp = MqttDecode_Vbi(buf, &cur_prop->data_int, (word32)(buf_len - (buf - pbuf))); if (tmp < 0) { @@ -829,6 +837,9 @@ int MqttDecode_ConnectAck(byte *rx_buf, int rx_buf_len, word32 props_len = 0; int tmp; /* Decode Length of Properties */ + if (rx_buf_len < (rx_payload - rx_buf)) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); + } tmp = MqttDecode_Vbi(rx_payload, &props_len, (word32)(rx_buf_len - (rx_payload - rx_buf))); if (tmp < 0) { @@ -1011,6 +1022,9 @@ int MqttDecode_Publish(byte *rx_buf, int rx_buf_len, MqttPublish *publish) int tmp; /* Decode Length of Properties */ + if (rx_buf_len < (rx_payload - rx_buf)) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); + } tmp = MqttDecode_Vbi(rx_payload, &props_len, (word32)(rx_buf_len - (rx_payload - rx_buf))); if (tmp < 0) @@ -1169,6 +1183,9 @@ int MqttDecode_PublishResp(byte* rx_buf, int rx_buf_len, byte type, int tmp; /* Decode Length of Properties */ + if (rx_buf_len < (rx_payload - rx_buf)) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); + } tmp = MqttDecode_Vbi(rx_payload, &props_len, (word32)(rx_buf_len - (rx_payload - rx_buf))); if (tmp < 0) @@ -1313,6 +1330,9 @@ int MqttDecode_SubscribeAck(byte* rx_buf, int rx_buf_len, int tmp; /* Decode Length of Properties */ + if (rx_buf_len < (rx_payload - rx_buf)) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); + } tmp = MqttDecode_Vbi(rx_payload, &props_len, (word32)(rx_buf_len - (rx_payload - rx_buf))); if (tmp < 0) @@ -1452,6 +1472,9 @@ int MqttDecode_UnsubscribeAck(byte *rx_buf, int rx_buf_len, int tmp; /* Decode Length of Properties */ + if (rx_buf_len < (rx_payload - rx_buf)) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); + } tmp = MqttDecode_Vbi(rx_payload, &props_len, (word32)(rx_buf_len - (rx_payload - rx_buf))); if (tmp < 0) @@ -1635,6 +1658,9 @@ int MqttDecode_Disconnect(byte *rx_buf, int rx_buf_len, MqttDisconnect *disc) if (remain_len > 1) { /* Decode Length of Properties */ + if (rx_buf_len < (rx_payload - rx_buf)) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); + } tmp = MqttDecode_Vbi(rx_payload, &props_len, (word32)(rx_buf_len - (rx_payload - rx_buf))); if (tmp < 0) @@ -1750,6 +1776,9 @@ int MqttDecode_Auth(byte *rx_buf, int rx_buf_len, MqttAuth *auth) auth->props = NULL; /* Decode Length of Properties */ + if (rx_buf_len < (rx_payload - rx_buf)) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); + } tmp = MqttDecode_Vbi(rx_payload, &props_len, (word32)(rx_buf_len - (rx_payload - rx_buf))); if (tmp < 0) @@ -2018,6 +2047,9 @@ int MqttPacket_Read(MqttClient *client, byte* rx_buf, int rx_buf_len, } /* Try and decode remaining length */ + if (rx_buf_len < (client->packet.header_len - (i + 1))) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); + } rc = MqttDecode_Vbi(header->len, (word32*)&client->packet.remain_len, rx_buf_len - (client->packet.header_len - (i + 1)));