Skip to content

Commit 42d123a

Browse files
embhornembhorn
committed
Fix f-2360 MqttEncode_String bug and tests
1 parent 78a4dba commit 42d123a

2 files changed

Lines changed: 224 additions & 0 deletions

File tree

‎src/mqtt_packet.c‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -801,6 +801,12 @@ int MqttEncode_Connect(byte *tx_buf, int tx_buf_len, MqttConnect *mc_connect)
801801
}
802802
#endif
803803

804+
/* MQTT UTF-8 strings are limited to 65535 bytes [MQTT-1.5.3]. Check here
805+
* (before writing the fixed header) so a later MqttEncode_String failure
806+
* cannot corrupt tx_payload via `tx_payload += -1`. */
807+
if (XSTRLEN(mc_connect->client_id) > (size_t)0xFFFF) {
808+
return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
809+
}
804810
remain_len += (int)XSTRLEN(mc_connect->client_id) + MQTT_DATA_LEN_SIZE;
805811
if (mc_connect->enable_lwt) {
806812
/* Verify all required fields are present */
@@ -811,6 +817,9 @@ int MqttEncode_Connect(byte *tx_buf, int tx_buf_len, MqttConnect *mc_connect)
811817
{
812818
return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
813819
}
820+
if (XSTRLEN(mc_connect->lwt_msg->topic_name) > (size_t)0xFFFF) {
821+
return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
822+
}
814823

815824
remain_len += (int)XSTRLEN(mc_connect->lwt_msg->topic_name);
816825
remain_len += MQTT_DATA_LEN_SIZE;
@@ -841,9 +850,15 @@ int MqttEncode_Connect(byte *tx_buf, int tx_buf_len, MqttConnect *mc_connect)
841850
#endif
842851
}
843852
if (mc_connect->username) {
853+
if (XSTRLEN(mc_connect->username) > (size_t)0xFFFF) {
854+
return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
855+
}
844856
remain_len += (int)XSTRLEN(mc_connect->username) + MQTT_DATA_LEN_SIZE;
845857
}
846858
if (mc_connect->password) {
859+
if (XSTRLEN(mc_connect->password) > (size_t)0xFFFF) {
860+
return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
861+
}
847862
remain_len += (int)XSTRLEN(mc_connect->password) + MQTT_DATA_LEN_SIZE;
848863
}
849864

@@ -1302,6 +1317,12 @@ int MqttEncode_Publish(byte *tx_buf, int tx_buf_len, MqttPublish *publish,
13021317
if (tx_buf == NULL || publish == NULL) {
13031318
return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
13041319
}
1320+
/* MQTT UTF-8 strings are limited to 65535 bytes [MQTT-1.5.3]. Check here
1321+
* before writing the fixed header so a later MqttEncode_String failure
1322+
* cannot corrupt tx_payload via `tx_payload += -1`. */
1323+
if (XSTRLEN(publish->topic_name) > (size_t)0xFFFF) {
1324+
return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
1325+
}
13051326

13061327
/* Determine packet length */
13071328
variable_len = (int)XSTRLEN(publish->topic_name) + MQTT_DATA_LEN_SIZE;
@@ -1721,6 +1742,10 @@ int MqttEncode_Subscribe(byte *tx_buf, int tx_buf_len,
17211742
for (i = 0; i < subscribe->topic_count; i++) {
17221743
topic = &subscribe->topics[i];
17231744
if ((topic != NULL) && (topic->topic_filter != NULL)) {
1745+
/* MQTT UTF-8 strings are limited to 65535 bytes [MQTT-1.5.3] */
1746+
if (XSTRLEN(topic->topic_filter) > (size_t)0xFFFF) {
1747+
return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
1748+
}
17241749
remain_len += (int)XSTRLEN(topic->topic_filter) + MQTT_DATA_LEN_SIZE;
17251750
remain_len++; /* For QoS */
17261751
}
@@ -2016,6 +2041,10 @@ int MqttEncode_Unsubscribe(byte *tx_buf, int tx_buf_len,
20162041
for (i = 0; i < unsubscribe->topic_count; i++) {
20172042
topic = &unsubscribe->topics[i];
20182043
if ((topic != NULL) && (topic->topic_filter != NULL)) {
2044+
/* MQTT UTF-8 strings are limited to 65535 bytes [MQTT-1.5.3] */
2045+
if (XSTRLEN(topic->topic_filter) > (size_t)0xFFFF) {
2046+
return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
2047+
}
20192048
remain_len += (int)XSTRLEN(topic->topic_filter) +
20202049
MQTT_DATA_LEN_SIZE;
20212050
}

‎tests/test_mqtt_packet.c‎

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -415,6 +415,37 @@ TEST(encode_publish_qos0_no_flags_in_header)
415415
ASSERT_EQ(0, (int)MQTT_PACKET_FLAGS_GET(tx_buf[0]));
416416
}
417417

418+
/* f-2360: topic_name with strlen > 65535 must not produce a "successful"
419+
* encode. MqttEncode_String returns -1 for oversize strings; the encoder
420+
* must surface that as a negative return rather than adding -1 to the
421+
* tx_payload pointer and reporting header_len+remain_len as success. */
422+
TEST(encode_publish_topic_oversized_rejected)
423+
{
424+
const int str_len = 0x10000; /* one byte past MQTT UTF-8 limit */
425+
const int buf_len = str_len + 64;
426+
byte *tx_buf = (byte*)WOLFMQTT_MALLOC(buf_len);
427+
char *topic = (char*)WOLFMQTT_MALLOC(str_len + 1);
428+
MqttPublish pub;
429+
int rc;
430+
431+
if (tx_buf == NULL || topic == NULL) {
432+
WOLFMQTT_FREE(tx_buf);
433+
WOLFMQTT_FREE(topic);
434+
FAIL("allocation failed");
435+
}
436+
XMEMSET(topic, 'A', str_len);
437+
topic[str_len] = '\0';
438+
439+
XMEMSET(&pub, 0, sizeof(pub));
440+
pub.topic_name = topic;
441+
pub.qos = MQTT_QOS_0;
442+
rc = MqttEncode_Publish(tx_buf, buf_len, &pub, 0);
443+
444+
WOLFMQTT_FREE(topic);
445+
WOLFMQTT_FREE(tx_buf);
446+
ASSERT_TRUE(rc < 0);
447+
}
448+
418449
/* ============================================================================
419450
* MqttDecode_Publish
420451
* ============================================================================ */
@@ -714,6 +745,40 @@ TEST(encode_subscribe_options_byte_qos2)
714745
ASSERT_EQ(0x02, tx_buf[rc - 1]);
715746
}
716747

748+
/* f-2360: topic_filter with strlen > 65535 must be rejected with a negative
749+
* return. Guards the unchecked tx_payload += MqttEncode_String(...) in the
750+
* SUBSCRIBE payload loop. */
751+
TEST(encode_subscribe_topic_filter_oversized_rejected)
752+
{
753+
const int str_len = 0x10000; /* one byte past MQTT UTF-8 limit */
754+
const int buf_len = str_len + 64;
755+
byte *tx_buf = (byte*)WOLFMQTT_MALLOC(buf_len);
756+
char *filter = (char*)WOLFMQTT_MALLOC(str_len + 1);
757+
MqttSubscribe sub;
758+
MqttTopic topic;
759+
int rc;
760+
761+
if (tx_buf == NULL || filter == NULL) {
762+
WOLFMQTT_FREE(tx_buf);
763+
WOLFMQTT_FREE(filter);
764+
FAIL("allocation failed");
765+
}
766+
XMEMSET(filter, 'A', str_len);
767+
filter[str_len] = '\0';
768+
769+
XMEMSET(&sub, 0, sizeof(sub));
770+
XMEMSET(&topic, 0, sizeof(topic));
771+
topic.topic_filter = filter;
772+
sub.topics = &topic;
773+
sub.topic_count = 1;
774+
sub.packet_id = 1;
775+
rc = MqttEncode_Subscribe(tx_buf, buf_len, &sub);
776+
777+
WOLFMQTT_FREE(filter);
778+
WOLFMQTT_FREE(tx_buf);
779+
ASSERT_TRUE(rc < 0);
780+
}
781+
717782
/* ============================================================================
718783
* MqttEncode_Unsubscribe
719784
* ============================================================================ */
@@ -959,6 +1024,130 @@ TEST(encode_connect_flags_lwt_qos1_retain)
9591024
ASSERT_EQ(0, flags & MQTT_CONNECT_FLAG_CLEAN_SESSION);
9601025
}
9611026

1027+
/* f-2360: client_id with strlen > 65535 must be rejected with a negative
1028+
* return. MqttEncode_String returns -1 for such strings; the encoder must
1029+
* not report header_len+remain_len as a successful encode while tx_payload
1030+
* silently moves backward by one byte. */
1031+
TEST(encode_connect_client_id_oversized_rejected)
1032+
{
1033+
const int str_len = 0x10000; /* one byte past MQTT UTF-8 limit */
1034+
const int buf_len = str_len + 64;
1035+
byte *tx_buf = (byte*)WOLFMQTT_MALLOC(buf_len);
1036+
char *client_id = (char*)WOLFMQTT_MALLOC(str_len + 1);
1037+
MqttConnect conn;
1038+
int rc;
1039+
1040+
if (tx_buf == NULL || client_id == NULL) {
1041+
WOLFMQTT_FREE(tx_buf);
1042+
WOLFMQTT_FREE(client_id);
1043+
FAIL("allocation failed");
1044+
}
1045+
XMEMSET(client_id, 'A', str_len);
1046+
client_id[str_len] = '\0';
1047+
1048+
XMEMSET(&conn, 0, sizeof(conn));
1049+
conn.client_id = client_id;
1050+
rc = MqttEncode_Connect(tx_buf, buf_len, &conn);
1051+
1052+
WOLFMQTT_FREE(client_id);
1053+
WOLFMQTT_FREE(tx_buf);
1054+
ASSERT_TRUE(rc < 0);
1055+
}
1056+
1057+
/* f-2360: username with strlen > 65535. Password is supplied so the
1058+
* USERNAME+PASSWORD branch exercises both credential encodes. */
1059+
TEST(encode_connect_username_oversized_rejected)
1060+
{
1061+
const int str_len = 0x10000;
1062+
const int buf_len = str_len + 128;
1063+
byte *tx_buf = (byte*)WOLFMQTT_MALLOC(buf_len);
1064+
char *username = (char*)WOLFMQTT_MALLOC(str_len + 1);
1065+
MqttConnect conn;
1066+
int rc;
1067+
1068+
if (tx_buf == NULL || username == NULL) {
1069+
WOLFMQTT_FREE(tx_buf);
1070+
WOLFMQTT_FREE(username);
1071+
FAIL("allocation failed");
1072+
}
1073+
XMEMSET(username, 'U', str_len);
1074+
username[str_len] = '\0';
1075+
1076+
XMEMSET(&conn, 0, sizeof(conn));
1077+
conn.client_id = "cid";
1078+
conn.username = username;
1079+
conn.password = "pw";
1080+
rc = MqttEncode_Connect(tx_buf, buf_len, &conn);
1081+
1082+
WOLFMQTT_FREE(username);
1083+
WOLFMQTT_FREE(tx_buf);
1084+
ASSERT_TRUE(rc < 0);
1085+
}
1086+
1087+
/* f-2360: password with strlen > 65535. */
1088+
TEST(encode_connect_password_oversized_rejected)
1089+
{
1090+
const int str_len = 0x10000;
1091+
const int buf_len = str_len + 128;
1092+
byte *tx_buf = (byte*)WOLFMQTT_MALLOC(buf_len);
1093+
char *password = (char*)WOLFMQTT_MALLOC(str_len + 1);
1094+
MqttConnect conn;
1095+
int rc;
1096+
1097+
if (tx_buf == NULL || password == NULL) {
1098+
WOLFMQTT_FREE(tx_buf);
1099+
WOLFMQTT_FREE(password);
1100+
FAIL("allocation failed");
1101+
}
1102+
XMEMSET(password, 'P', str_len);
1103+
password[str_len] = '\0';
1104+
1105+
XMEMSET(&conn, 0, sizeof(conn));
1106+
conn.client_id = "cid";
1107+
conn.username = "user";
1108+
conn.password = password;
1109+
rc = MqttEncode_Connect(tx_buf, buf_len, &conn);
1110+
1111+
WOLFMQTT_FREE(password);
1112+
WOLFMQTT_FREE(tx_buf);
1113+
ASSERT_TRUE(rc < 0);
1114+
}
1115+
1116+
/* f-2360: LWT topic_name with strlen > 65535. */
1117+
TEST(encode_connect_lwt_topic_oversized_rejected)
1118+
{
1119+
const int str_len = 0x10000;
1120+
const int buf_len = str_len + 128;
1121+
byte *tx_buf = (byte*)WOLFMQTT_MALLOC(buf_len);
1122+
char *lwt_topic = (char*)WOLFMQTT_MALLOC(str_len + 1);
1123+
byte lwt_payload[] = { 'b', 'y', 'e' };
1124+
MqttConnect conn;
1125+
MqttMessage lwt;
1126+
int rc;
1127+
1128+
if (tx_buf == NULL || lwt_topic == NULL) {
1129+
WOLFMQTT_FREE(tx_buf);
1130+
WOLFMQTT_FREE(lwt_topic);
1131+
FAIL("allocation failed");
1132+
}
1133+
XMEMSET(lwt_topic, 'T', str_len);
1134+
lwt_topic[str_len] = '\0';
1135+
1136+
XMEMSET(&conn, 0, sizeof(conn));
1137+
XMEMSET(&lwt, 0, sizeof(lwt));
1138+
lwt.topic_name = lwt_topic;
1139+
lwt.buffer = lwt_payload;
1140+
lwt.total_len = (word32)sizeof(lwt_payload);
1141+
conn.client_id = "cid";
1142+
conn.enable_lwt = 1;
1143+
conn.lwt_msg = &lwt;
1144+
rc = MqttEncode_Connect(tx_buf, buf_len, &conn);
1145+
1146+
WOLFMQTT_FREE(lwt_topic);
1147+
WOLFMQTT_FREE(tx_buf);
1148+
ASSERT_TRUE(rc < 0);
1149+
}
1150+
9621151
/* ============================================================================
9631152
* MqttDecode_Connect (broker-side)
9641153
* ============================================================================ */
@@ -1479,6 +1668,7 @@ void run_mqtt_packet_tests(void)
14791668
RUN_TEST(encode_publish_qos1_retain_flags_in_header);
14801669
RUN_TEST(encode_publish_qos2_duplicate_flags_in_header);
14811670
RUN_TEST(encode_publish_qos0_no_flags_in_header);
1671+
RUN_TEST(encode_publish_topic_oversized_rejected);
14821672

14831673
/* MqttDecode_Publish */
14841674
RUN_TEST(decode_publish_qos0_valid);
@@ -1501,6 +1691,7 @@ void run_mqtt_packet_tests(void)
15011691
RUN_TEST(encode_subscribe_options_byte_qos0);
15021692
RUN_TEST(encode_subscribe_options_byte_qos1);
15031693
RUN_TEST(encode_subscribe_options_byte_qos2);
1694+
RUN_TEST(encode_subscribe_topic_filter_oversized_rejected);
15041695

15051696
/* MqttEncode_Unsubscribe */
15061697
RUN_TEST(encode_unsubscribe_packet_id_zero);
@@ -1518,6 +1709,10 @@ void run_mqtt_packet_tests(void)
15181709
RUN_TEST(encode_connect_flags_clean_session_only);
15191710
RUN_TEST(encode_connect_flags_username_only);
15201711
RUN_TEST(encode_connect_flags_lwt_qos1_retain);
1712+
RUN_TEST(encode_connect_client_id_oversized_rejected);
1713+
RUN_TEST(encode_connect_username_oversized_rejected);
1714+
RUN_TEST(encode_connect_password_oversized_rejected);
1715+
RUN_TEST(encode_connect_lwt_topic_oversized_rejected);
15211716

15221717
#ifdef WOLFMQTT_BROKER
15231718
/* MqttDecode_Connect */

0 commit comments

Comments
 (0)