Skip to content

Always check MqttDecode_Num's return code. #479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kareem-wolfssl wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Always check MqttDecode_Num's return code. #479

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a98ef95
Always check MqttDecode_Num's return code.
kareem-wolfssl Apr 3, 2026
8e9f06d
Fix usage of MQTT_TRACE_ERROR
kareem-wolfssl Apr 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 124 additions & 22 deletions src/mqtt_sn_packet.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ const char* SN_Packet_TypeDesc(SN_MsgType packet_type)
int SN_Decode_Header(byte *rx_buf, int rx_buf_len,
SN_MsgType* p_packet_type, word16* p_packet_id)
{
int rc;
SN_MsgType packet_type;
word16 total_len;

Expand All @@ -109,7 +110,11 @@ int SN_Decode_Header(byte *rx_buf, int rx_buf_len,
total_len = *rx_buf++;
if (total_len == SN_PACKET_LEN_IND) {
/* The length is stored in the next two bytes */
rx_buf += MqttDecode_Num(rx_buf, &total_len, rx_buf_len - 1);
rc = MqttDecode_Num(rx_buf, &total_len, rx_buf_len - 1);
if (rc < 0) {
return rc;
}
rx_buf += rc;
}

if (total_len > rx_buf_len) {
Expand All @@ -126,19 +131,37 @@ int SN_Decode_Header(byte *rx_buf, int rx_buf_len,
switch(packet_type) {
case SN_MSG_TYPE_REGACK:
case SN_MSG_TYPE_PUBACK:
if (rx_buf_len < 3) {
return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
}
/* octet 4-5 */
MqttDecode_Num(rx_buf + 2, p_packet_id, (word32)(rx_buf_len - 3));
rc = MqttDecode_Num(rx_buf + 2, p_packet_id,
(word32)(rx_buf_len - 3));
if (rc < 0) {
return rc;
}
Comment on lines +138 to +142
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In SN_Decode_Header, the buffer-length argument passed to MqttDecode_Num is derived from rx_buf_len - 3 (and then cast to word32). If rx_buf_len < 3 (or the packet is malformed such that the header length is too small for this packet type), this subtraction becomes negative and the cast turns it into a huge positive value, bypassing MqttDecode_Num’s buf_len < 2 guard and risking an out-of-bounds read at rx_buf + 2. Please add an explicit bounds check using a signed type before the cast (e.g., compute remaining bytes from the pointer offset and ensure it’s >= MQTT_DATA_LEN_SIZE) and return a decode error when the packet is too short for a packet id.

Copilot uses AI. Check for mistakes.
break;
case SN_MSG_TYPE_PUBCOMP:
case SN_MSG_TYPE_PUBREC:
case SN_MSG_TYPE_PUBREL:
case SN_MSG_TYPE_UNSUBACK:
/* octet 2-3 */
MqttDecode_Num(rx_buf, p_packet_id, (word32)(rx_buf_len - 1));
rc = MqttDecode_Num(rx_buf, p_packet_id,
(word32)(rx_buf_len - 1));
if (rc < 0) {
return rc;
}
Comment on lines +149 to +153
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as above: rx_buf_len - 1 is cast to word32 when calling MqttDecode_Num. If rx_buf_len is smaller than the required bytes for a packet id for these message types (e.g., malformed/truncated packet), the subtraction can go negative and the cast defeats the out-of-buffer check, leading to potential out-of-bounds reads. Compute the remaining length as a signed value (based on pointer offsets) and validate it before calling MqttDecode_Num.

Copilot uses AI. Check for mistakes.
break;
case SN_MSG_TYPE_SUBACK:
if (rx_buf_len < 4) {
return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
}
/* octet 5-6 */
MqttDecode_Num(rx_buf + 3, p_packet_id, (word32)(rx_buf_len - 4));
rc = MqttDecode_Num(rx_buf + 3, p_packet_id,
(word32)(rx_buf_len - 4));
if (rc < 0) {
return rc;
}
Comment on lines +160 to +164
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as above for the SUBACK case: (word32)(rx_buf_len - 4) can underflow on short/malformed packets and become a very large word32, allowing MqttDecode_Num to read beyond rx_buf. Please add explicit signed bounds checks for the required fixed offsets before decoding the packet id.

Copilot uses AI. Check for mistakes.
break;
case SN_MSG_TYPE_ADVERTISE:
case SN_MSG_TYPE_SEARCHGW:
Expand Down Expand Up @@ -199,9 +222,15 @@ int SN_Decode_Advertise(byte *rx_buf, int rx_buf_len, SN_Advertise *gw_info)

/* Decode gateway info */
if (gw_info != NULL) {
int rc;
gw_info->gwId = *rx_payload++;

rx_payload += MqttDecode_Num(rx_payload, &gw_info->duration, (word32)(rx_buf_len - (rx_payload - rx_buf)));
rc = MqttDecode_Num(rx_payload, &gw_info->duration,
(word32)(rx_buf_len - (rx_payload - rx_buf)));
if (rc < 0) {
return rc;
}
rx_payload += rc;
}
(void)rx_payload;

Expand Down Expand Up @@ -239,6 +268,7 @@ int SN_Encode_SearchGW(byte *tx_buf, int tx_buf_len, byte hops)
int SN_Decode_GWInfo(byte *rx_buf, int rx_buf_len, SN_GwInfo *gw_info)
{
word16 total_len;
int rc;
byte *rx_payload = rx_buf, type;

/* Validate required arguments */
Expand All @@ -250,7 +280,12 @@ int SN_Decode_GWInfo(byte *rx_buf, int rx_buf_len, SN_GwInfo *gw_info)
total_len = *rx_payload++;
if (total_len == SN_PACKET_LEN_IND) {
/* The length is stored in the next two bytes */
rx_payload += MqttDecode_Num(rx_payload, &total_len, (word32)(rx_buf_len - (rx_payload - rx_buf)));
rc = MqttDecode_Num(rx_payload, &total_len,
(word32)(rx_buf_len - (rx_payload - rx_buf)));
if (rc < 0) {
return rc;
}
rx_payload += rc;
}

if (total_len > rx_buf_len) {
Expand Down Expand Up @@ -790,6 +825,7 @@ int SN_Encode_Register(byte *tx_buf, int tx_buf_len, SN_Register *regist)
int SN_Decode_Register(byte *rx_buf, int rx_buf_len, SN_Register *regist)
{
word16 total_len;
int rc;
byte *rx_payload = rx_buf, type;

/* Validate required arguments */
Expand All @@ -801,7 +837,12 @@ int SN_Decode_Register(byte *rx_buf, int rx_buf_len, SN_Register *regist)
total_len = *rx_payload++;
if (total_len == SN_PACKET_LEN_IND) {
/* The length is stored in the next two bytes */
rx_payload += MqttDecode_Num(rx_payload, &total_len, (word32)(rx_buf_len - (rx_payload - rx_buf)));
rc = MqttDecode_Num(rx_payload, &total_len,
(word32)(rx_buf_len - (rx_payload - rx_buf)));
if (rc < 0) {
return rc;
}
rx_payload += rc;
}

if (total_len >= rx_buf_len) {
Expand All @@ -819,10 +860,20 @@ int SN_Decode_Register(byte *rx_buf, int rx_buf_len, SN_Register *regist)

if (regist != NULL) {
/* Decode Topic ID assigned by GW */
rx_payload += MqttDecode_Num(rx_payload, &regist->topicId, (word32)(rx_buf_len - (rx_payload - rx_buf)));
rc = MqttDecode_Num(rx_payload, &regist->topicId,
(word32)(rx_buf_len - (rx_payload - rx_buf)));
if (rc < 0) {
return rc;
}
rx_payload += rc;

/* Decode packet ID */
rx_payload += MqttDecode_Num(rx_payload, &regist->packet_id, (word32)(rx_buf_len - (rx_payload - rx_buf)));
rc = MqttDecode_Num(rx_payload, &regist->packet_id,
(word32)(rx_buf_len - (rx_payload - rx_buf)));
if (rc < 0) {
return rc;
}
rx_payload += rc;

/* Decode Topic Name */
regist->topicName = (char*)rx_payload;
Expand Down Expand Up @@ -879,7 +930,7 @@ int SN_Encode_RegAck(byte *tx_buf, int tx_buf_len, SN_RegAck *regack)

int SN_Decode_RegAck(byte *rx_buf, int rx_buf_len, SN_RegAck *regack)
{
int total_len;
int rc, total_len;
byte *rx_payload = rx_buf, type;

/* Validate required arguments */
Expand All @@ -904,10 +955,20 @@ int SN_Decode_RegAck(byte *rx_buf, int rx_buf_len, SN_RegAck *regack)

if (regack != NULL) {
/* Decode Topic ID assigned by GW */
rx_payload += MqttDecode_Num(rx_payload, &regack->topicId, (word32)(rx_buf_len - (rx_payload - rx_buf)));
rc = MqttDecode_Num(rx_payload, &regack->topicId,
(word32)(rx_buf_len - (rx_payload - rx_buf)));
if (rc < 0) {
return rc;
}
rx_payload += rc;

/* Decode packet ID */
rx_payload += MqttDecode_Num(rx_payload, &regack->packet_id, (word32)(rx_buf_len - (rx_payload - rx_buf)));
rc = MqttDecode_Num(rx_payload, &regack->packet_id,
(word32)(rx_buf_len - (rx_payload - rx_buf)));
if (rc < 0) {
return rc;
}
rx_payload += rc;

/* Decode return code */
regack->return_code = *rx_payload++;
Expand Down Expand Up @@ -994,6 +1055,7 @@ int SN_Encode_Subscribe(byte *tx_buf, int tx_buf_len, SN_Subscribe *subscribe)
int SN_Decode_SubscribeAck(byte* rx_buf, int rx_buf_len,
SN_SubAck *subscribe_ack)
{
int rc;
word16 total_len;
byte* rx_payload = rx_buf, type;

Expand All @@ -1020,8 +1082,18 @@ int SN_Decode_SubscribeAck(byte* rx_buf, int rx_buf_len,
/* Decode SubAck fields */
if (subscribe_ack) {
subscribe_ack->flags = *rx_payload++;
rx_payload += MqttDecode_Num(rx_payload, &subscribe_ack->topicId, (word32)(rx_buf_len - (rx_payload - rx_buf)));
rx_payload += MqttDecode_Num(rx_payload, &subscribe_ack->packet_id, (word32)(rx_buf_len - (rx_payload - rx_buf)));
rc = MqttDecode_Num(rx_payload, &subscribe_ack->topicId,
(word32)(rx_buf_len - (rx_payload - rx_buf)));
if (rc < 0) {
return rc;
}
rx_payload += rc;
rc = MqttDecode_Num(rx_payload, &subscribe_ack->packet_id,
(word32)(rx_buf_len - (rx_payload - rx_buf)));
if (rc < 0) {
return rc;
}
rx_payload += rc;
subscribe_ack->return_code = *rx_payload++;
}
(void)rx_payload;
Expand Down Expand Up @@ -1112,6 +1184,7 @@ int SN_Encode_Publish(byte *tx_buf, int tx_buf_len, SN_Publish *publish)

int SN_Decode_Publish(byte *rx_buf, int rx_buf_len, SN_Publish *publish)
{
int rc;
word16 total_len;
byte *rx_payload = rx_buf;
byte flags = 0, type;
Expand All @@ -1125,7 +1198,12 @@ int SN_Decode_Publish(byte *rx_buf, int rx_buf_len, SN_Publish *publish)
total_len = *rx_payload++;
if (total_len == SN_PACKET_LEN_IND) {
/* The length is stored in the next two bytes */
rx_payload += MqttDecode_Num(rx_payload, &total_len, (word32)(rx_buf_len - (rx_payload - rx_buf)));
rc = MqttDecode_Num(rx_payload, &total_len,
(word32)(rx_buf_len - (rx_payload - rx_buf)));
if (rc < 0) {
return rc;
}
rx_payload += rc;
}

if (total_len > rx_buf_len) {
Expand All @@ -1148,7 +1226,12 @@ int SN_Decode_Publish(byte *rx_buf, int rx_buf_len, SN_Publish *publish)
rx_payload += MQTT_DATA_LEN_SIZE;
publish->topic_name_len = MQTT_DATA_LEN_SIZE;

rx_payload += MqttDecode_Num(rx_payload, &publish->packet_id, (word32)(rx_buf_len - (rx_payload - rx_buf)));
rc = MqttDecode_Num(rx_payload, &publish->packet_id,
(word32)(rx_buf_len - (rx_payload - rx_buf)));
if (rc < 0) {
return rc;
}
rx_payload += rc;

/* Set flags */
publish->duplicate = flags & SN_PACKET_FLAG_DUPLICATE;
Expand Down Expand Up @@ -1214,7 +1297,7 @@ int SN_Encode_PublishResp(byte* tx_buf, int tx_buf_len, byte type,
int SN_Decode_PublishResp(byte* rx_buf, int rx_buf_len, byte type,
SN_PublishResp *publish_resp)
{
int total_len;
int rc, total_len;
byte rec_type, *rx_payload = rx_buf;

/* Validate required arguments */
Expand All @@ -1238,10 +1321,20 @@ int SN_Decode_PublishResp(byte* rx_buf, int rx_buf_len, byte type,

if (publish_resp) {
if (type == SN_MSG_TYPE_PUBACK) {
rx_payload += MqttDecode_Num(rx_payload, &publish_resp->topicId, (word32)(rx_buf_len - (rx_payload - rx_buf)));
rc = MqttDecode_Num(rx_payload, &publish_resp->topicId,
(word32)(rx_buf_len - (rx_payload - rx_buf)));
if (rc < 0) {
return rc;
}
rx_payload += rc;
}

rx_payload += MqttDecode_Num(rx_payload, &publish_resp->packet_id, (word32)(rx_buf_len - (rx_payload - rx_buf)));
rc = MqttDecode_Num(rx_payload, &publish_resp->packet_id,
(word32)(rx_buf_len - (rx_payload - rx_buf)));
if (rc < 0) {
return rc;
}
rx_payload += rc;

if (type == SN_MSG_TYPE_PUBACK) {
publish_resp->return_code = *rx_payload++;
Expand Down Expand Up @@ -1330,6 +1423,7 @@ int SN_Encode_Unsubscribe(byte *tx_buf, int tx_buf_len,
int SN_Decode_UnsubscribeAck(byte *rx_buf, int rx_buf_len,
SN_UnsubscribeAck *unsubscribe_ack)
{
int rc;
word16 total_len;
byte *rx_payload = rx_buf, type;

Expand All @@ -1355,7 +1449,12 @@ int SN_Decode_UnsubscribeAck(byte *rx_buf, int rx_buf_len,

/* Decode SubAck fields */
if (unsubscribe_ack) {
rx_payload += MqttDecode_Num(rx_payload, &unsubscribe_ack->packet_id, (word32)(rx_buf_len - (rx_payload - rx_buf)));
rc = MqttDecode_Num(rx_payload, &unsubscribe_ack->packet_id,
(word32)(rx_buf_len - (rx_payload - rx_buf)));
if (rc < 0) {
return rc;
}
rx_payload += rc;
}
(void)rx_payload;

Expand All @@ -1367,7 +1466,7 @@ int SN_Encode_Disconnect(byte *tx_buf, int tx_buf_len,
SN_Disconnect* disconnect)
{
int total_len = 2; /* length and message type */
byte *tx_payload = tx_buf;;
byte *tx_payload = tx_buf;

/* Validate required arguments */
if (tx_buf == NULL) {
Expand Down Expand Up @@ -1542,7 +1641,10 @@ int SN_Packet_Read(MqttClient *client, byte* rx_buf, int rx_buf_len,
len = rc;
}

(void)MqttDecode_Num(&rx_buf[1], &total_len, (word32)(rx_buf_len - 1));
rc = MqttDecode_Num(&rx_buf[1], &total_len, (word32)(rx_buf_len - 1));
if (rc < 0) {
return MqttPacket_HandleNetError(client, rc);
}
client->packet.header_len = len;
}
else {
Expand Down
Loading