From a076cbcfb722fd94a6ce9851d3ef301819635632 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 2 Jun 2026 11:30:53 -0500 Subject: [PATCH 01/23] Fixes in MQTT-SN code --- .gitignore | 1 + src/mqtt_sn_packet.c | 40 ++++- tests/include.am | 15 ++ tests/test_mqtt_sn.c | 359 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 412 insertions(+), 3 deletions(-) create mode 100644 tests/test_mqtt_sn.c diff --git a/.gitignore b/.gitignore index 24940c97a..b37ee738d 100644 --- a/.gitignore +++ b/.gitignore @@ -129,4 +129,5 @@ src/mqtt_broker tests/fuzz/broker_fuzz tests/fuzz/corpus/ tests/test_broker_connect +tests/test_mqtt_sn tests/unit_tests diff --git a/src/mqtt_sn_packet.c b/src/mqtt_sn_packet.c index 80e5a88e2..6d59edd4e 100644 --- a/src/mqtt_sn_packet.c +++ b/src/mqtt_sn_packet.c @@ -101,6 +101,7 @@ int SN_Decode_Header(byte *rx_buf, int rx_buf_len, int rc; SN_MsgType packet_type; word16 total_len; + byte *rx_buf_orig = rx_buf; if (rx_buf == NULL || rx_buf_len < MQTT_PACKET_HEADER_MIN_SIZE) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); @@ -120,6 +121,15 @@ int SN_Decode_Header(byte *rx_buf, int rx_buf_len, if (total_len > rx_buf_len) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); } + /* Reject a declared total_len that does not cover the bytes already + * consumed plus the upcoming message-type read. Without this, a peer + * crafted SN_PACKET_LEN_IND packet whose 2-byte length field decodes + * to a value equal to rx_buf_len (e.g., rx_buf_len == 3 with + * total_len == 3) slips past the > rx_buf_len check above and the + * *rx_buf++ below reads one byte past the caller-supplied buffer. */ + if (total_len < (word16)(rx_buf - rx_buf_orig) + 1) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + } /* Message Type */ packet_type = (SN_MsgType)*rx_buf++; @@ -291,7 +301,15 @@ int SN_Decode_GWInfo(byte *rx_buf, int rx_buf_len, SN_GwInfo *gw_info) if (total_len > rx_buf_len) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); } - if (total_len < 3) { + /* Reject a frame whose total_len cannot cover the bytes still to be read + * after the length-indicator block (message type + gateway ID). The + * short-form header consumes one byte and the extended-length form + * consumes three, so the prior fixed "< 3" minimum was only valid for + * the short form: an extended-length GWINFO with total_len <= the + * header bytes already consumed would slip past it and the + * *rx_payload++ reads below would walk past the caller-supplied + * buffer. */ + if (total_len < (word16)(rx_payload - rx_buf) + 2) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); } /* Check message type */ @@ -302,11 +320,18 @@ int SN_Decode_GWInfo(byte *rx_buf, int rx_buf_len, SN_GwInfo *gw_info) /* Decode gateway info */ if (gw_info != NULL) { + word16 consumed; + gw_info->gwId = *rx_payload++; - if (total_len - 3 > 0) { + /* Use the bytes actually consumed so far (1-byte short-form or + * 3-byte extended-length header, plus type + gwId) rather than a + * fixed 3, so the address length is correct for both forms and the + * copy below cannot read past the buffer in the IND form. */ + consumed = (word16)(rx_payload - rx_buf); + if (total_len > consumed) { /* The gateway address is only present if sent by a client */ - word16 addr_len = total_len - 3; + word16 addr_len = total_len - consumed; if (addr_len > (word16)sizeof(SN_GwAddr)) { addr_len = (word16)sizeof(SN_GwAddr); } @@ -875,6 +900,15 @@ int SN_Decode_Register(byte *rx_buf, int rx_buf_len, SN_Register *regist) } rx_payload += rc; + /* total_len must cover at least the bytes consumed so far + * (length + type + topicId + packet_id); otherwise the topic-name + * length computation below underflows and the NUL terminator is + * written before regist->topicName, leaving the field pointing at + * non-terminated memory past the parsed packet. */ + if (total_len < (word16)(rx_payload - rx_buf)) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + } + /* Decode Topic Name */ regist->topicName = (char*)rx_payload; diff --git a/tests/include.am b/tests/include.am index 77d7ddae9..bad275466 100644 --- a/tests/include.am +++ b/tests/include.am @@ -15,6 +15,21 @@ tests_unit_tests_DEPENDENCIES = src/libwolfmqtt.la # Test framework header noinst_HEADERS += tests/unit_test.h +# MQTT-SN packet decoder tests. The SN_Decode_* routines are WOLFMQTT_LOCAL +# (hidden) so they cannot be linked from libwolfmqtt; src/mqtt_sn_packet.c is +# built directly into this binary (same approach as test_broker_connect) so the +# decoders are reachable. The test provides stubs for the LOCAL socket helpers +# the SN network paths reference but the decoder tests never exercise. +if BUILD_SN +check_PROGRAMS += tests/test_mqtt_sn +tests_test_mqtt_sn_SOURCES = tests/test_mqtt_sn.c \ + src/mqtt_sn_packet.c +tests_test_mqtt_sn_CFLAGS = -DWOLFMQTT_SN $(AM_CFLAGS) +tests_test_mqtt_sn_CPPFLAGS = -I$(top_srcdir) $(AM_CPPFLAGS) +tests_test_mqtt_sn_LDADD = src/libwolfmqtt.la +tests_test_mqtt_sn_DEPENDENCIES = src/libwolfmqtt.la +endif + # Broker CONNECT handler tests. Compiles src/mqtt_broker.c into the test # binary with WOLFMQTT_BROKER_CUSTOM_NET so the network layer is replaced # with mock callbacks driven by the harness. diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c new file mode 100644 index 000000000..90faab024 --- /dev/null +++ b/tests/test_mqtt_sn.c @@ -0,0 +1,359 @@ +/* test_mqtt_sn.c + * + * Copyright (C) 2006-2026 wolfSSL Inc. + * + * This file is part of wolfMQTT. + * + * wolfMQTT is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfMQTT is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + */ + +/* Standalone unit tests for the MQTT-SN packet decoders. + * + * The SN_Decode_* routines are WOLFMQTT_LOCAL (hidden visibility), so they + * cannot be linked from libwolfmqtt; src/mqtt_sn_packet.c is therefore built + * directly into this binary (with -DWOLFMQTT_SN) the same way mqtt_broker.c is + * built into test_broker_connect. The tests hand-build malformed and + * well-formed SN frames and feed them through the decoders, asserting on the + * return code and decoded fields. + * + * Coverage focuses on the declared-length bounds checks in SN_Decode_Header, + * SN_Decode_GWInfo and SN_Decode_Register: each guard rejects a total_len that + * does not cover the bytes the decoder still needs to read, which would + * otherwise over-read the caller-supplied buffer (or, in Register, underflow + * the topic-name length and write the NUL terminator before the field). The + * GWINFO suite also pins the extended-length (IND) gateway-address length: the + * copy must derive its length from the bytes actually consumed (1-byte short + * form vs 3-byte IND form) so the IND form cannot read past the frame. + */ + +#ifdef HAVE_CONFIG_H + #include +#endif + +#include "wolfmqtt/mqtt_client.h" +#include "wolfmqtt/mqtt_sn_packet.h" + +/* Provide storage for the unit-test framework's global counters. Must be + * defined before unit_test.h is included. */ +#define UNIT_TEST_IMPLEMENTATION +#include "tests/unit_test.h" + +/* I/O test doubles. + * + * src/mqtt_sn_packet.c is compiled into this binary so its WOLFMQTT_LOCAL + * decoders are reachable. That translation unit also contains the SN + * network send/receive wrappers, which reference these WOLFMQTT_LOCAL socket + * helpers. They are hidden in libwolfmqtt and never exercised by the decoder + * tests, so we satisfy the link with stubs that simply report no data. */ +int MqttSocket_Read(struct _MqttClient *client, byte* buf, int buf_len, + int timeout_ms) +{ + (void)client; (void)buf; (void)buf_len; (void)timeout_ms; + return MQTT_CODE_ERROR_NETWORK; +} +int MqttSocket_Peek(struct _MqttClient *client, byte* buf, int buf_len, + int timeout_ms) +{ + (void)client; (void)buf; (void)buf_len; (void)timeout_ms; + return MQTT_CODE_ERROR_NETWORK; +} +int MqttPacket_HandleNetError(MqttClient *client, int rc) +{ + (void)client; + return rc; +} + +static void setup(void) { } +static void teardown(void) { } + +/* ============================================================================ + * SN_Decode_Header + * ============================================================================ */ + +TEST(sn_header_short_form_valid) +{ + /* [len=2][type=PING_REQ] */ + byte buf[2] = { 0x02, SN_MSG_TYPE_PING_REQ }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, NULL); + ASSERT_EQ(2, rc); + ASSERT_EQ(SN_MSG_TYPE_PING_REQ, type); +} + +TEST(sn_header_regack_packet_id_extracted) +{ + /* [len][type=REGACK][topicId(2)][packetId(2)][retcode] */ + byte buf[7] = { 0x07, SN_MSG_TYPE_REGACK, 0x00, 0x01, 0x12, 0x34, 0x00 }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + word16 packet_id = 0; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, &packet_id); + ASSERT_EQ(7, rc); + ASSERT_EQ(SN_MSG_TYPE_REGACK, type); + ASSERT_EQ(0x1234, packet_id); +} + +TEST(sn_header_ind_form_total_len_equals_consumed_rejected) +{ + /* Extended-length (IND) header whose declared length (3) only covers the + * 3 header bytes already consumed, leaving nothing for the message-type + * read. Must be rejected rather than reading buf[3] one past the end. */ + byte buf[3] = { SN_PACKET_LEN_IND, 0x00, 0x03 }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, NULL); + ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); +} + +TEST(sn_header_total_len_exceeds_buffer_rejected) +{ + byte buf[2] = { 0x05, SN_MSG_TYPE_PING_REQ }; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), NULL, NULL); + ASSERT_EQ(MQTT_CODE_ERROR_OUT_OF_BUFFER, rc); +} + +TEST(sn_header_null_buf_rejected) +{ + int rc = SN_Decode_Header(NULL, 4, NULL, NULL); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_header_buf_too_short_rejected) +{ + byte buf[1] = { 0x02 }; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), NULL, NULL); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +/* ============================================================================ + * SN_Decode_GWInfo + * ============================================================================ */ + +TEST(sn_gwinfo_short_form_no_addr_valid) +{ + /* [len=3][type=GWINFO][gwId] - no gateway address present */ + byte buf[3] = { 0x03, SN_MSG_TYPE_GWINFO, 0x09 }; + SN_GwInfo info; + int rc; + XMEMSET(&info, 0, sizeof(info)); + rc = SN_Decode_GWInfo(buf, (int)sizeof(buf), &info); + ASSERT_EQ(3, rc); + ASSERT_EQ(0x09, info.gwId); +} + +TEST(sn_gwinfo_short_form_with_addr_valid) +{ + /* [len=5][type=GWINFO][gwId][addr0][addr1] */ + byte buf[5] = { 0x05, SN_MSG_TYPE_GWINFO, 0x09, 0xAA, 0xBB }; + const byte expect[2] = { 0xAA, 0xBB }; + SN_GwAddr addr; + SN_GwInfo info; + int rc; + XMEMSET(&info, 0, sizeof(info)); + XMEMSET(&addr, 0, sizeof(addr)); + info.gwAddr = &addr; + rc = SN_Decode_GWInfo(buf, (int)sizeof(buf), &info); + ASSERT_EQ(5, rc); + ASSERT_EQ(0x09, info.gwId); + ASSERT_MEM_EQ(expect, &addr, sizeof(addr)); +} + +TEST(sn_gwinfo_short_form_total_len_too_small_rejected) +{ + /* total_len=2 cannot cover type + gwId. */ + byte buf[2] = { 0x02, SN_MSG_TYPE_GWINFO }; + SN_GwInfo info; + int rc; + XMEMSET(&info, 0, sizeof(info)); + rc = SN_Decode_GWInfo(buf, (int)sizeof(buf), &info); + ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); +} + +TEST(sn_gwinfo_ind_form_total_len_equals_consumed_rejected) +{ + /* IND header consumes 3 bytes; total_len=3 leaves nothing for the + * type + gwId reads. Must be rejected, not walk past the buffer. */ + byte buf[3] = { SN_PACKET_LEN_IND, 0x00, 0x03 }; + SN_GwInfo info; + int rc; + XMEMSET(&info, 0, sizeof(info)); + rc = SN_Decode_GWInfo(buf, (int)sizeof(buf), &info); + ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); +} + +TEST(sn_gwinfo_ind_form_no_addr_no_overread) +{ + /* IND form, total_len=5: the 5 bytes are exactly [IND][len(2)][type][gwId] + * with NO address. The address length must be computed from the 5 bytes + * actually consumed (not a fixed 3), so no copy happens and gwAddr is left + * untouched. The old "total_len - 3" math would copy 2 bytes from offset 5, + * two bytes past this 5-byte buffer. */ + byte buf[5] = { SN_PACKET_LEN_IND, 0x00, 0x05, SN_MSG_TYPE_GWINFO, 0x09 }; + SN_GwAddr addr; + SN_GwInfo info; + int rc; + XMEMSET(&info, 0, sizeof(info)); + XMEMSET(&addr, 0x5A, sizeof(addr)); /* sentinel: must stay unchanged */ + info.gwAddr = &addr; + rc = SN_Decode_GWInfo(buf, (int)sizeof(buf), &info); + ASSERT_EQ(5, rc); + ASSERT_EQ(0x09, info.gwId); + ASSERT_EQ(0x5A5A, addr); /* no spurious copy */ +} + +TEST(sn_gwinfo_ind_form_with_addr_valid) +{ + /* IND form, total_len=7 with a 2-byte address at offset 5. Proves the + * copy reads from the correct offset for the extended-length form. */ + byte buf[7] = { SN_PACKET_LEN_IND, 0x00, 0x07, SN_MSG_TYPE_GWINFO, 0x09, + 0xAA, 0xBB }; + const byte expect[2] = { 0xAA, 0xBB }; + SN_GwAddr addr; + SN_GwInfo info; + int rc; + XMEMSET(&info, 0, sizeof(info)); + XMEMSET(&addr, 0, sizeof(addr)); + info.gwAddr = &addr; + rc = SN_Decode_GWInfo(buf, (int)sizeof(buf), &info); + ASSERT_EQ(7, rc); + ASSERT_EQ(0x09, info.gwId); + ASSERT_MEM_EQ(expect, &addr, sizeof(addr)); +} + +TEST(sn_gwinfo_wrong_type_rejected) +{ + byte buf[3] = { 0x03, SN_MSG_TYPE_ADVERTISE, 0x09 }; + SN_GwInfo info; + int rc; + XMEMSET(&info, 0, sizeof(info)); + rc = SN_Decode_GWInfo(buf, (int)sizeof(buf), &info); + ASSERT_EQ(MQTT_CODE_ERROR_PACKET_TYPE, rc); +} + +/* ============================================================================ + * SN_Decode_Register + * ============================================================================ */ + +TEST(sn_register_short_form_valid) +{ + /* [len=8][type=REGISTER][topicId=0x0102][packetId=0x0304]['a']['b'][\0] */ + byte buf[9] = { 0x08, SN_MSG_TYPE_REGISTER, 0x01, 0x02, 0x03, 0x04, + 'a', 'b', 0x00 }; + SN_Register reg; + int rc; + XMEMSET(®, 0, sizeof(reg)); + rc = SN_Decode_Register(buf, (int)sizeof(buf), ®); + ASSERT_EQ(8, rc); + ASSERT_EQ(0x0102, reg.topicId); + ASSERT_EQ(0x0304, reg.packet_id); + ASSERT_NOT_NULL(reg.topicName); + ASSERT_STR_EQ("ab", reg.topicName); +} + +TEST(sn_register_ind_form_valid) +{ + /* IND form: [IND][len=0x000A][type][topicId][packetId]['a']['b'][\0] */ + byte buf[11] = { SN_PACKET_LEN_IND, 0x00, 0x0A, SN_MSG_TYPE_REGISTER, + 0x01, 0x02, 0x03, 0x04, 'a', 'b', 0x00 }; + SN_Register reg; + int rc; + XMEMSET(®, 0, sizeof(reg)); + rc = SN_Decode_Register(buf, (int)sizeof(buf), ®); + ASSERT_EQ(10, rc); + ASSERT_EQ(0x0102, reg.topicId); + ASSERT_EQ(0x0304, reg.packet_id); + ASSERT_NOT_NULL(reg.topicName); + ASSERT_STR_EQ("ab", reg.topicName); +} + +TEST(sn_register_ind_form_total_len_too_small_rejected) +{ + /* IND form consumes 8 bytes (len + type + topicId + packetId). total_len=7 + * is below that, so the topic-name length would underflow and the NUL + * terminator would be written before topicName. Must be rejected. + * rx_buf_len is 8 (> total_len) so the OUT_OF_BUFFER check is passed and + * the new lower-bound guard is what rejects it. */ + byte buf[8] = { SN_PACKET_LEN_IND, 0x00, 0x07, SN_MSG_TYPE_REGISTER, + 0x01, 0x02, 0x03, 0x04 }; + SN_Register reg; + int rc; + XMEMSET(®, 0, sizeof(reg)); + rc = SN_Decode_Register(buf, (int)sizeof(buf), ®); + ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); +} + +TEST(sn_register_total_len_below_fixed_min_rejected) +{ + /* total_len=6 is below the 7-byte minimum REGISTER. rx_buf_len is larger + * so this exercises the fixed minimum, not the buffer check. */ + byte buf[10] = { 0x06, SN_MSG_TYPE_REGISTER, 0x01, 0x02, 0x03, 0x04, + 'a', 'b', 'c', 0x00 }; + SN_Register reg; + int rc; + XMEMSET(®, 0, sizeof(reg)); + rc = SN_Decode_Register(buf, (int)sizeof(buf), ®); + ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); +} + +TEST(sn_register_wrong_type_rejected) +{ + byte buf[9] = { 0x08, SN_MSG_TYPE_PUBLISH, 0x01, 0x02, 0x03, 0x04, + 'a', 'b', 0x00 }; + SN_Register reg; + int rc; + XMEMSET(®, 0, sizeof(reg)); + rc = SN_Decode_Register(buf, (int)sizeof(buf), ®); + ASSERT_EQ(MQTT_CODE_ERROR_PACKET_TYPE, rc); +} + +/* ============================================================================ + * Suite runner + * ============================================================================ */ + +int main(int argc, char** argv) +{ + (void)argc; + (void)argv; + + TEST_RUNNER_BEGIN(); + + TEST_SUITE_BEGIN("mqtt_sn_packet", setup, teardown); + + /* SN_Decode_Header */ + RUN_TEST(sn_header_short_form_valid); + RUN_TEST(sn_header_regack_packet_id_extracted); + RUN_TEST(sn_header_ind_form_total_len_equals_consumed_rejected); + RUN_TEST(sn_header_total_len_exceeds_buffer_rejected); + RUN_TEST(sn_header_null_buf_rejected); + RUN_TEST(sn_header_buf_too_short_rejected); + + /* SN_Decode_GWInfo */ + RUN_TEST(sn_gwinfo_short_form_no_addr_valid); + RUN_TEST(sn_gwinfo_short_form_with_addr_valid); + RUN_TEST(sn_gwinfo_short_form_total_len_too_small_rejected); + RUN_TEST(sn_gwinfo_ind_form_total_len_equals_consumed_rejected); + RUN_TEST(sn_gwinfo_ind_form_no_addr_no_overread); + RUN_TEST(sn_gwinfo_ind_form_with_addr_valid); + RUN_TEST(sn_gwinfo_wrong_type_rejected); + + /* SN_Decode_Register */ + RUN_TEST(sn_register_short_form_valid); + RUN_TEST(sn_register_ind_form_valid); + RUN_TEST(sn_register_ind_form_total_len_too_small_rejected); + RUN_TEST(sn_register_total_len_below_fixed_min_rejected); + RUN_TEST(sn_register_wrong_type_rejected); + + TEST_SUITE_END(); + + TEST_RUNNER_END(); +} From 3886c276e5c40925f76acbf929cb6cfd3704e5b2 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 2 Jun 2026 11:42:14 -0500 Subject: [PATCH 02/23] Fix f-5501 MQTT-SN CONNACK --- src/mqtt_sn_client.c | 10 ++++- tests/test_mqtt_sn.c | 104 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 2 deletions(-) diff --git a/src/mqtt_sn_client.c b/src/mqtt_sn_client.c index c9a092c55..674ae43e7 100644 --- a/src/mqtt_sn_client.c +++ b/src/mqtt_sn_client.c @@ -66,8 +66,14 @@ static int SN_Client_HandlePacket(MqttClient* client, SN_MsgType packet_type, else { XMEMSET(p_connect_ack, 0, sizeof(SN_ConnectAck)); } - p_connect_ack->return_code = - client->rx_buf[client->packet.buf_len-1]; + + /* Validate the fixed-length CONNACK (length, buffer size and + packet type) rather than blindly trusting the last byte. */ + rc = SN_Decode_ConnectAck(client->rx_buf, client->packet.buf_len, + p_connect_ack); + if (rc <= 0) { + return rc; + } break; } diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c index 90faab024..0bdbf5725 100644 --- a/tests/test_mqtt_sn.c +++ b/tests/test_mqtt_sn.c @@ -316,6 +316,101 @@ TEST(sn_register_wrong_type_rejected) ASSERT_EQ(MQTT_CODE_ERROR_PACKET_TYPE, rc); } +/* ============================================================================ + * SN_Decode_ConnectAck + * + * The MQTT-SN CONNACK is a fixed 3-byte frame: [len=3][type=CONNACK][retcode]. + * SN_Client_HandlePacket routes CONNACK through this decoder, so the length, + * buffer-size and packet-type guards here are what stop a malformed gateway + * frame (e.g. an over-long packet with a benign trailing byte) from being + * accepted as a successful connect by SN_Client_Connect. + * ============================================================================ */ + +TEST(sn_connack_accepted_valid) +{ + /* [len=3][type=CONNACK][return_code=ACCEPTED] */ + byte buf[3] = { 0x03, SN_MSG_TYPE_CONNACK, SN_RC_ACCEPTED }; + SN_ConnectAck ack; + int rc; + XMEMSET(&ack, 0xFF, sizeof(ack)); /* poison: decoder must overwrite */ + rc = SN_Decode_ConnectAck(buf, (int)sizeof(buf), &ack); + ASSERT_EQ(3, rc); + ASSERT_EQ(SN_RC_ACCEPTED, ack.return_code); +} + +TEST(sn_connack_rejected_valid) +{ + /* A well-formed CONNACK that refuses the connection. The decoder must + * succeed (rc=3) and surface the gateway's reject code unchanged so the + * caller can map it to MQTT_CODE_ERROR_CONNECT_REFUSED. */ + byte buf[3] = { 0x03, SN_MSG_TYPE_CONNACK, SN_RC_CONGESTION }; + SN_ConnectAck ack; + int rc; + XMEMSET(&ack, 0, sizeof(ack)); + rc = SN_Decode_ConnectAck(buf, (int)sizeof(buf), &ack); + ASSERT_EQ(3, rc); + ASSERT_EQ(SN_RC_CONGESTION, ack.return_code); +} + +TEST(sn_connack_short_len_rejected) +{ + /* total_len=2 cannot cover the 3-byte fixed CONNACK. rx_buf_len is larger + * so this exercises the fixed-length check, not the buffer guard. */ + byte buf[3] = { 0x02, SN_MSG_TYPE_CONNACK, SN_RC_ACCEPTED }; + SN_ConnectAck ack; + int rc; + XMEMSET(&ack, 0, sizeof(ack)); + rc = SN_Decode_ConnectAck(buf, (int)sizeof(buf), &ack); + ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); +} + +TEST(sn_connack_long_len_rejected) +{ + /* Regression for the trusted-last-byte bug: a 4-byte frame whose declared + * length is 4. The old handler read buf[buf_len-1] (the trailing ACCEPTED + * byte) and reported success even though the real return code (offset 2) + * was a reject. The decoder must reject the non-3 length outright. */ + byte buf[4] = { 0x04, SN_MSG_TYPE_CONNACK, SN_RC_CONGESTION, + SN_RC_ACCEPTED }; + SN_ConnectAck ack; + int rc; + XMEMSET(&ack, 0, sizeof(ack)); + rc = SN_Decode_ConnectAck(buf, (int)sizeof(buf), &ack); + ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); +} + +TEST(sn_connack_total_len_exceeds_buffer_rejected) +{ + /* Declared length 3 but only 2 bytes are actually available. Must be + * rejected by the buffer guard rather than reading buf[2] past the end. */ + byte buf[2] = { 0x03, SN_MSG_TYPE_CONNACK }; + SN_ConnectAck ack; + int rc; + XMEMSET(&ack, 0, sizeof(ack)); + rc = SN_Decode_ConnectAck(buf, (int)sizeof(buf), &ack); + ASSERT_EQ(MQTT_CODE_ERROR_OUT_OF_BUFFER, rc); +} + +TEST(sn_connack_wrong_type_rejected) +{ + /* Correct length but a non-CONNACK message type. */ + byte buf[3] = { 0x03, SN_MSG_TYPE_WILLTOPICREQ, SN_RC_ACCEPTED }; + SN_ConnectAck ack; + int rc; + XMEMSET(&ack, 0, sizeof(ack)); + rc = SN_Decode_ConnectAck(buf, (int)sizeof(buf), &ack); + ASSERT_EQ(MQTT_CODE_ERROR_PACKET_TYPE, rc); +} + +TEST(sn_connack_null_buf_rejected) +{ + SN_ConnectAck ack; + int rc; + XMEMSET(&ack, 0, sizeof(ack)); + rc = SN_Decode_ConnectAck(NULL, 3, &ack); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + /* ============================================================================ * Suite runner * ============================================================================ */ @@ -353,6 +448,15 @@ int main(int argc, char** argv) RUN_TEST(sn_register_total_len_below_fixed_min_rejected); RUN_TEST(sn_register_wrong_type_rejected); + /* SN_Decode_ConnectAck */ + RUN_TEST(sn_connack_accepted_valid); + RUN_TEST(sn_connack_rejected_valid); + RUN_TEST(sn_connack_short_len_rejected); + RUN_TEST(sn_connack_long_len_rejected); + RUN_TEST(sn_connack_total_len_exceeds_buffer_rejected); + RUN_TEST(sn_connack_wrong_type_rejected); + RUN_TEST(sn_connack_null_buf_rejected); + TEST_SUITE_END(); TEST_RUNNER_END(); From d9699e3daddc8924288bbf5be21c65b7d844b299 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 2 Jun 2026 14:10:54 -0500 Subject: [PATCH 03/23] Fix f-3388 SN with MT & NB --- .gitignore | 1 + src/mqtt_sn_client.c | 342 ++++++++++++++++++---------- tests/include.am | 13 ++ tests/test_mqtt_sn_client.c | 428 ++++++++++++++++++++++++++++++++++++ 4 files changed, 673 insertions(+), 111 deletions(-) create mode 100644 tests/test_mqtt_sn_client.c diff --git a/.gitignore b/.gitignore index b37ee738d..78c66be31 100644 --- a/.gitignore +++ b/.gitignore @@ -130,4 +130,5 @@ tests/fuzz/broker_fuzz tests/fuzz/corpus/ tests/test_broker_connect tests/test_mqtt_sn +tests/test_mqtt_sn_client tests/unit_tests diff --git a/src/mqtt_sn_client.c b/src/mqtt_sn_client.c index 674ae43e7..3a6cc9df4 100644 --- a/src/mqtt_sn_client.c +++ b/src/mqtt_sn_client.c @@ -941,145 +941,258 @@ int SN_Client_SearchGW(MqttClient *client, SN_SearchGw *search) static int SN_WillTopic(MqttClient *client, SN_Will *will) { - int rc; + int rc = 0; /* Validate required arguments */ if (client == NULL) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } -#ifdef WOLFMQTT_MULTITHREAD - rc = wm_SemLock(&client->lockClient); - if (rc == 0) { - /* inform other threads of expected response */ - rc = MqttClient_RespList_Add(client, - (MqttPacketType)SN_MSG_TYPE_WILLTOPICREQ, 0, - &will->pendResp, &will->resp.topicResp); - wm_SemUnlock(&client->lockClient); - } - if (rc != 0) { - return rc; /* Error locking client */ - } -#endif + /* The will exchange is a wait-then-respond flow. Track progress in + * will->stat.write so that, under WOLFMQTT_NONBLOCK, a MQTT_CODE_CONTINUE + * return resumes where it left off rather than re-running the whole + * function. In particular the pending-response is added only once (in + * MQTT_MSG_BEGIN); re-adding it on a retry would be rejected as a + * duplicate by MqttClient_RespList_Add. */ + switch (will->stat.write) + { + case MQTT_MSG_BEGIN: + { + #ifdef WOLFMQTT_MULTITHREAD + rc = wm_SemLock(&client->lockClient); + if (rc == 0) { + /* inform other threads of expected response */ + rc = MqttClient_RespList_Add(client, + (MqttPacketType)SN_MSG_TYPE_WILLTOPICREQ, 0, + &will->pendResp, &will->resp.topicResp); + wm_SemUnlock(&client->lockClient); + } + if (rc != 0) { + return rc; /* Error locking client */ + } + #endif - /* Wait for Will Topic Request packet */ - rc = SN_Client_WaitType(client, will, - SN_MSG_TYPE_WILLTOPICREQ, 0, client->cmd_timeout_ms); -#ifdef WOLFMQTT_NONBLOCK - if (rc == MQTT_CODE_CONTINUE) - return rc; -#endif + will->stat.write = MQTT_MSG_WAIT; + } + FALL_THROUGH; -#ifdef WOLFMQTT_MULTITHREAD - if (wm_SemLock(&client->lockClient) == 0) { - MqttClient_RespList_Remove(client, &will->pendResp); - wm_SemUnlock(&client->lockClient); - } -#endif - if (rc == 0) { - #ifdef WOLFMQTT_MULTITHREAD - /* Lock send socket mutex */ - rc = wm_SemLock(&client->lockSend); - if (rc != 0) { - return rc; + case MQTT_MSG_WAIT: + { + /* Wait for Will Topic Request packet */ + rc = SN_Client_WaitType(client, will, + SN_MSG_TYPE_WILLTOPICREQ, 0, client->cmd_timeout_ms); + #ifdef WOLFMQTT_NONBLOCK + if (rc == MQTT_CODE_CONTINUE) { + return rc; /* stay in MQTT_MSG_WAIT, do not re-add */ + } + #endif + + #ifdef WOLFMQTT_MULTITHREAD + if (wm_SemLock(&client->lockClient) == 0) { + MqttClient_RespList_Remove(client, &will->pendResp); + wm_SemUnlock(&client->lockClient); + } + #endif + + if (rc != 0) { + /* reset state on error */ + will->stat.write = MQTT_MSG_BEGIN; + return rc; + } + + will->stat.write = MQTT_MSG_HEADER; } - #endif + FALL_THROUGH; - /* Encode Will Topic */ - rc = SN_Encode_WillTopic(client->tx_buf, client->tx_buf_len, - will); - #ifdef WOLFMQTT_DEBUG_CLIENT - PRINTF("EncodePacket: Len %d, Type %s (%d)", - rc, SN_Packet_TypeDesc(SN_MSG_TYPE_WILLTOPIC), - SN_MSG_TYPE_WILLTOPIC); - #endif - if (rc > 0) { - /* Send Will Topic packet */ - client->write.len = rc; - rc = MqttPacket_Write(client, client->tx_buf, client->write.len); - if (rc == client->write.len) { - rc = 0; + case MQTT_MSG_HEADER: + { + #ifdef WOLFMQTT_MULTITHREAD + /* Lock send socket mutex */ + rc = wm_SemLock(&client->lockSend); + if (rc != 0) { + return rc; + } + #endif + + /* Encode Will Topic */ + rc = SN_Encode_WillTopic(client->tx_buf, client->tx_buf_len, + will); + #ifdef WOLFMQTT_DEBUG_CLIENT + PRINTF("EncodePacket: Len %d, Type %s (%d)", + rc, SN_Packet_TypeDesc(SN_MSG_TYPE_WILLTOPIC), + SN_MSG_TYPE_WILLTOPIC); + #endif + if (rc > 0) { + /* Send Will Topic packet */ + client->write.len = rc; + rc = MqttPacket_Write(client, client->tx_buf, + client->write.len); + if (rc == client->write.len) { + rc = 0; + } + } + #ifdef WOLFMQTT_MULTITHREAD + wm_SemUnlock(&client->lockSend); + #endif + + #ifdef WOLFMQTT_NONBLOCK + if (rc == MQTT_CODE_CONTINUE) { + return rc; /* resume send on next call */ } + #endif + + /* reset state */ + will->stat.write = MQTT_MSG_BEGIN; + break; } - #ifdef WOLFMQTT_MULTITHREAD - wm_SemUnlock(&client->lockSend); - #endif - } + + case MQTT_MSG_AUTH: + case MQTT_MSG_PAYLOAD: + case MQTT_MSG_PAYLOAD2: + case MQTT_MSG_ACK: + default: + { + #ifdef WOLFMQTT_DEBUG_CLIENT + PRINTF("SN_WillTopic: Invalid write state %d!", will->stat.write); + #endif + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_STAT); + break; + } + } /* switch (will->stat.write) */ return rc; } static int SN_WillMessage(MqttClient *client, SN_Will *will) { - int rc; + int rc = 0; /* Validate required arguments */ if (client == NULL) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } -#ifdef WOLFMQTT_MULTITHREAD - rc = wm_SemLock(&client->lockClient); - if (rc == 0) { - /* inform other threads of expected response */ - rc = MqttClient_RespList_Add(client, - (MqttPacketType)SN_MSG_TYPE_WILLMSGREQ, 0, - &will->pendResp, &will->resp.msgResp); - wm_SemUnlock(&client->lockClient); - } - if (rc != 0) { - return rc; /* Error locking client */ - } -#endif + /* See SN_WillTopic: will->stat.write tracks progress so the pending + * response is added exactly once and a WOLFMQTT_NONBLOCK retry resumes + * instead of re-adding it (which would be rejected as a duplicate). */ + switch (will->stat.write) + { + case MQTT_MSG_BEGIN: + { + #ifdef WOLFMQTT_MULTITHREAD + rc = wm_SemLock(&client->lockClient); + if (rc == 0) { + /* inform other threads of expected response */ + rc = MqttClient_RespList_Add(client, + (MqttPacketType)SN_MSG_TYPE_WILLMSGREQ, 0, + &will->pendResp, &will->resp.msgResp); + wm_SemUnlock(&client->lockClient); + } + if (rc != 0) { + return rc; /* Error locking client */ + } + #endif - /* Wait for Will Message Request */ - rc = SN_Client_WaitType(client, &will->resp.msgResp, - SN_MSG_TYPE_WILLMSGREQ, 0, client->cmd_timeout_ms); + will->stat.write = MQTT_MSG_WAIT; + } + FALL_THROUGH; -#ifdef WOLFMQTT_NONBLOCK - if (rc == MQTT_CODE_CONTINUE) - return rc; -#endif + case MQTT_MSG_WAIT: + { + /* Wait for Will Message Request */ + rc = SN_Client_WaitType(client, &will->resp.msgResp, + SN_MSG_TYPE_WILLMSGREQ, 0, client->cmd_timeout_ms); + #ifdef WOLFMQTT_NONBLOCK + if (rc == MQTT_CODE_CONTINUE) { + return rc; /* stay in MQTT_MSG_WAIT, do not re-add */ + } + #endif -#ifdef WOLFMQTT_MULTITHREAD - if (wm_SemLock(&client->lockClient) == 0) { - MqttClient_RespList_Remove(client, &will->pendResp); - wm_SemUnlock(&client->lockClient); - } -#endif + #ifdef WOLFMQTT_MULTITHREAD + if (wm_SemLock(&client->lockClient) == 0) { + MqttClient_RespList_Remove(client, &will->pendResp); + wm_SemUnlock(&client->lockClient); + } + #endif - if (rc == 0) { - #ifdef WOLFMQTT_MULTITHREAD - /* Lock send socket mutex */ - rc = wm_SemLock(&client->lockSend); - if (rc != 0) { - return rc; + if (rc != 0) { + /* reset state on error */ + will->stat.write = MQTT_MSG_BEGIN; + return rc; + } + + will->stat.write = MQTT_MSG_HEADER; } - #endif - /* Encode Will Message */ - rc = SN_Encode_WillMsg(client->tx_buf, - client->tx_buf_len, will); - #ifdef WOLFMQTT_DEBUG_CLIENT - PRINTF("EncodePacket: Len %d, Type %s (%d)", - rc, SN_Packet_TypeDesc(SN_MSG_TYPE_WILLMSG), - SN_MSG_TYPE_WILLMSG); - #endif - if (rc > 0) { - /* Send Will Topic packet */ - client->write.len = rc; - rc = MqttPacket_Write(client, client->tx_buf, client->write.len); - if (rc == client->write.len) { - rc = 0; + FALL_THROUGH; + + case MQTT_MSG_HEADER: + { + #ifdef WOLFMQTT_MULTITHREAD + /* Lock send socket mutex */ + rc = wm_SemLock(&client->lockSend); + if (rc != 0) { + return rc; + } + #endif + /* Encode Will Message */ + rc = SN_Encode_WillMsg(client->tx_buf, + client->tx_buf_len, will); + #ifdef WOLFMQTT_DEBUG_CLIENT + PRINTF("EncodePacket: Len %d, Type %s (%d)", + rc, SN_Packet_TypeDesc(SN_MSG_TYPE_WILLMSG), + SN_MSG_TYPE_WILLMSG); + #endif + if (rc > 0) { + /* Send Will Message packet */ + client->write.len = rc; + rc = MqttPacket_Write(client, client->tx_buf, + client->write.len); + if (rc == client->write.len) { + rc = 0; + } + } + #ifdef WOLFMQTT_MULTITHREAD + wm_SemUnlock(&client->lockSend); + #endif + + #ifdef WOLFMQTT_NONBLOCK + if (rc == MQTT_CODE_CONTINUE) { + return rc; /* resume send on next call */ } + #endif + + /* reset state */ + will->stat.write = MQTT_MSG_BEGIN; + break; } - #ifdef WOLFMQTT_MULTITHREAD - wm_SemUnlock(&client->lockSend); - #endif - } + + case MQTT_MSG_AUTH: + case MQTT_MSG_PAYLOAD: + case MQTT_MSG_PAYLOAD2: + case MQTT_MSG_ACK: + default: + { + #ifdef WOLFMQTT_DEBUG_CLIENT + PRINTF("SN_WillMessage: Invalid write state %d!", + will->stat.write); + #endif + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_STAT); + break; + } + } /* switch (will->stat.write) */ return rc; } +/* Progress of the two-step SN Last-Will exchange, stored in + * SN_Connect.will_done. The "all done" value is kept at 1 to preserve the + * previous boolean use of this field. */ +enum { + SN_WILL_DONE_NONE = 0, /* will exchange not started */ + SN_WILL_DONE_ALL = 1, /* will topic and message both sent */ + SN_WILL_DONE_TOPIC = 2 /* will topic sent, message still pending */ +}; + int SN_Client_Connect(MqttClient *client, SN_Connect *mc_connect) { int rc = 0; @@ -1091,7 +1204,7 @@ int SN_Client_Connect(MqttClient *client, SN_Connect *mc_connect) if (mc_connect->stat.write == MQTT_MSG_BEGIN) { - mc_connect->will_done = 0; + mc_connect->will_done = SN_WILL_DONE_NONE; #ifdef WOLFMQTT_MULTITHREAD /* Lock send socket mutex */ @@ -1150,19 +1263,26 @@ int SN_Client_Connect(MqttClient *client, SN_Connect *mc_connect) mc_connect->stat.write = MQTT_MSG_WAIT; } - if ((mc_connect->enable_lwt == 1) && (mc_connect->will_done != 1)) { + if ((mc_connect->enable_lwt == 1) && + (mc_connect->will_done != SN_WILL_DONE_ALL)) { /* If the will is enabled, then the gateway requests the topic and - message in separate packets. */ - rc = SN_WillTopic(client, &mc_connect->will); - if (rc != 0) { - return rc; + message in separate packets. will_done tracks how far the two-step + exchange has progressed so a non-blocking retry does not restart a + sub-step that already completed (which would re-add its pending + response). */ + if (mc_connect->will_done == SN_WILL_DONE_NONE) { + rc = SN_WillTopic(client, &mc_connect->will); + if (rc != 0) { + return rc; + } + mc_connect->will_done = SN_WILL_DONE_TOPIC; } rc = SN_WillMessage(client, &mc_connect->will); if (rc != 0) { return rc; } - mc_connect->will_done = 1; + mc_connect->will_done = SN_WILL_DONE_ALL; } /* Wait for connect ack packet */ diff --git a/tests/include.am b/tests/include.am index bad275466..211bf18bb 100644 --- a/tests/include.am +++ b/tests/include.am @@ -28,6 +28,19 @@ tests_test_mqtt_sn_CFLAGS = -DWOLFMQTT_SN $(AM_CFLAGS) tests_test_mqtt_sn_CPPFLAGS = -I$(top_srcdir) $(AM_CPPFLAGS) tests_test_mqtt_sn_LDADD = src/libwolfmqtt.la tests_test_mqtt_sn_DEPENDENCIES = src/libwolfmqtt.la + +# MQTT-SN client state-machine tests. These drive the public SN_Client_* entry +# points through a mock MqttNet by linking libwolfmqtt (so they run against +# whatever options the library was configured with). The happy-path will +# handshake is checked in every SN build; the non-blocking duplicate-pendResp +# regression tests compile only when WOLFMQTT_NONBLOCK is enabled (handled with +# #ifdefs in the test source). +check_PROGRAMS += tests/test_mqtt_sn_client +tests_test_mqtt_sn_client_SOURCES = tests/test_mqtt_sn_client.c +tests_test_mqtt_sn_client_CFLAGS = -DWOLFMQTT_SN $(AM_CFLAGS) +tests_test_mqtt_sn_client_CPPFLAGS = -I$(top_srcdir) $(AM_CPPFLAGS) +tests_test_mqtt_sn_client_LDADD = src/libwolfmqtt.la $(LIB_STATIC_ADD) +tests_test_mqtt_sn_client_DEPENDENCIES = src/libwolfmqtt.la endif # Broker CONNECT handler tests. Compiles src/mqtt_broker.c into the test diff --git a/tests/test_mqtt_sn_client.c b/tests/test_mqtt_sn_client.c new file mode 100644 index 000000000..2fbff2168 --- /dev/null +++ b/tests/test_mqtt_sn_client.c @@ -0,0 +1,428 @@ +/* test_mqtt_sn_client.c + * + * Copyright (C) 2006-2026 wolfSSL Inc. + * + * This file is part of wolfMQTT. + * + * wolfMQTT is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfMQTT is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + */ + +/* Client-level (state machine) unit tests for the MQTT-SN client. + * + * These tests drive the public SN_Client_* entry points through a mock + * MqttNet so the will-handshake state machine is exercised end to end. The + * suite builds in any SN configuration: the happy-path test runs in blocking + * builds too (guarding the refactored BEGIN/WAIT/HEADER flow against a dropped + * FALL_THROUGH/transition), while the retry tests are compiled only under + * WOLFMQTT_NONBLOCK. There the mock gateway returns MQTT_CODE_CONTINUE between + * scripted SN frames so every wait is re-entered at least once, the same way an + * application would call the API repeatedly. + * + * Regression focus: the Last-Will (LWT) connect handshake. SN_WillTopic and + * SN_WillMessage used to add their pending response to client->firstPendResp + * on *every* invocation. Under WOLFMQTT_MULTITHREAD + WOLFMQTT_NONBLOCK the + * first MQTT_CODE_CONTINUE left the entry linked, so the next SN_Client_Connect + * call re-added the same MqttPendResp and MqttClient_RespList_Add rejected the + * duplicate with MQTT_CODE_ERROR_BAD_ARG, permanently failing the connect. + * The add now happens once (in the MQTT_MSG_BEGIN state) and the helpers resume + * instead of restarting, so repeated calls converge on success. + */ + +#ifdef HAVE_CONFIG_H + #include +#endif + +#include "wolfmqtt/mqtt_client.h" +#include "wolfmqtt/mqtt_sn_packet.h" + +/* Provide storage for the unit-test framework's global counters. Must be + * defined before unit_test.h is included. */ +#define UNIT_TEST_IMPLEMENTATION +#include "tests/unit_test.h" + +#ifdef WOLFMQTT_SN + +/* A dangling pending response can only exist (and only be inspected) in + * multi-thread builds, where SN_Will keeps a MqttPendResp. Elsewhere this + * assertion is a no-op. */ +#ifdef WOLFMQTT_MULTITHREAD + #define ASSERT_NO_PENDRESP() ASSERT_NULL(g_client.firstPendResp) +#else + #define ASSERT_NO_PENDRESP() do {} while (0) +#endif + +/* ============================================================================ + * Mock MqttNet + * + * Models a datagram gateway driven through the SN datagram (IS_DTLS) read path: + * SN_Packet_Read pulls the 2-byte header then the remaining body with + * sequential reads, so the mock serves each scripted frame byte-region in + * order via read(). `continues` MQTT_CODE_CONTINUE results are returned at each + * frame boundary so the client's non-blocking wait is forced to re-enter, the + * same way an application would call the API repeatedly under WOLFMQTT_NONBLOCK. + * ============================================================================ */ + +#define MOCK_MAX_FRAMES 8 +#define MOCK_OUT_LEN 1024 + +typedef struct MockNet { + const byte* in_frame[MOCK_MAX_FRAMES]; + int in_len[MOCK_MAX_FRAMES]; + int in_count; + int in_idx; /* current frame being delivered */ + int in_off; /* read offset within the current frame */ + int continues; /* CONTINUE results remaining before this frame */ + int continues_cfg; /* CONTINUE results to arm before each frame */ + + byte out[MOCK_OUT_LEN]; + int out_len; /* bytes captured from write() */ + + int read_calls; + int write_calls; +} MockNet; + +static int mock_connect(void *ctx, const char* host, word16 port, + int timeout_ms) +{ + (void)ctx; (void)host; (void)port; (void)timeout_ms; + return MQTT_CODE_SUCCESS; +} + +static int mock_disconnect(void *ctx) +{ + (void)ctx; + return MQTT_CODE_SUCCESS; +} + +static int mock_write(void *ctx, const byte* buf, int buf_len, int timeout_ms) +{ + MockNet* net = (MockNet*)ctx; + (void)timeout_ms; + net->write_calls++; + if (buf_len > 0 && net->out_len + buf_len <= MOCK_OUT_LEN) { + XMEMCPY(&net->out[net->out_len], buf, (size_t)buf_len); + net->out_len += buf_len; + } + return buf_len; /* accept all bytes */ +} + +static int mock_read(void *ctx, byte* buf, int buf_len, int timeout_ms) +{ + MockNet* net = (MockNet*)ctx; + int avail, n; + (void)timeout_ms; + net->read_calls++; + + if (net->in_idx >= net->in_count) { + return MQTT_CODE_CONTINUE; /* nothing left to deliver */ + } + + /* At a frame boundary, optionally simulate "would block" first. */ + if (net->in_off == 0 && net->continues > 0) { + net->continues--; + return MQTT_CODE_CONTINUE; + } + + avail = net->in_len[net->in_idx] - net->in_off; + n = (buf_len < avail) ? buf_len : avail; + XMEMCPY(buf, &net->in_frame[net->in_idx][net->in_off], (size_t)n); + net->in_off += n; + + /* Advance to the next frame once this one is fully consumed. */ + if (net->in_off >= net->in_len[net->in_idx]) { + net->in_idx++; + net->in_off = 0; + net->continues = net->continues_cfg; + } + return n; +} + +/* peek is required by MqttSocket for the non-DTLS SN path, but the datagram + * (IS_DTLS) path used by these tests never calls it. Provide a non-consuming + * implementation for completeness. */ +static int mock_peek(void *ctx, byte* buf, int buf_len, int timeout_ms) +{ + MockNet* net = (MockNet*)ctx; + int avail, n; + (void)timeout_ms; + + if (net->in_idx >= net->in_count) { + return MQTT_CODE_CONTINUE; + } + if (net->in_off == 0 && net->continues > 0) { + return MQTT_CODE_CONTINUE; + } + avail = net->in_len[net->in_idx] - net->in_off; + n = (buf_len < avail) ? buf_len : avail; + XMEMCPY(buf, &net->in_frame[net->in_idx][net->in_off], (size_t)n); + return n; +} + +static void mock_net_init(MockNet* net, MqttNet* mqttNet, int continues_cfg) +{ + XMEMSET(net, 0, sizeof(*net)); + net->continues_cfg = continues_cfg; + net->continues = continues_cfg; + + XMEMSET(mqttNet, 0, sizeof(*mqttNet)); + mqttNet->context = net; + mqttNet->connect = mock_connect; + mqttNet->disconnect = mock_disconnect; + mqttNet->read = mock_read; + mqttNet->write = mock_write; + mqttNet->peek = mock_peek; +} + +static void mock_net_push(MockNet* net, const byte* frame, int len) +{ + if (net->in_count < MOCK_MAX_FRAMES) { + net->in_frame[net->in_count] = frame; + net->in_len[net->in_count] = len; + net->in_count++; + } +} + +/* Scripted gateway frames for the LWT connect handshake. */ +static const byte WILLTOPICREQ_FRAME[] = { 0x02, SN_MSG_TYPE_WILLTOPICREQ }; +static const byte WILLMSGREQ_FRAME[] = { 0x02, SN_MSG_TYPE_WILLMSGREQ }; +static const byte CONNACK_FRAME[] = { 0x03, SN_MSG_TYPE_CONNACK, + SN_RC_ACCEPTED }; + +/* ============================================================================ + * Test fixtures + * ============================================================================ */ + +static MqttClient g_client; +static MqttNet g_net; +static MockNet g_mock; +static byte g_tx[512]; +static byte g_rx[512]; + +static void sn_will_setup_connect(SN_Connect* mc) +{ + XMEMSET(mc, 0, sizeof(*mc)); + mc->keep_alive_sec = 60; + mc->clean_session = 1; + mc->client_id = "wolfMQTT-sn-test"; + mc->protocol_level = SN_PROTOCOL_ID; + mc->enable_lwt = 1; + mc->will.qos = 0; + mc->will.retain = 0; + mc->will.willTopic = "wolf/lwt"; + mc->will.willMsg = (byte*)"offline"; + mc->will.willMsgLen = 7; +} + +static int sn_client_init(int continues_cfg) +{ + int rc; + mock_net_init(&g_mock, &g_net, continues_cfg); + rc = MqttClient_Init(&g_client, &g_net, NULL, + g_tx, (int)sizeof(g_tx), g_rx, (int)sizeof(g_rx), + 1000 /* cmd_timeout_ms */); + /* MQTT-SN runs over datagrams; select the datagram framing in the SN + * packet reader (no TLS is enabled, so this only affects read framing). */ + MqttClient_Flags(&g_client, 0, MQTT_CLIENT_FLAG_IS_DTLS); + return rc; +} + +/* Drive SN_Client_Connect until it returns something other than CONTINUE, or + * until we exceed a sane iteration cap. Returns the terminal code and the + * number of iterations through *iters. */ +static int sn_connect_pump(SN_Connect* mc, int* iters) +{ + int rc = MQTT_CODE_CONTINUE; + int i; + const int max_iters = 50; + for (i = 0; i < max_iters; i++) { + rc = SN_Client_Connect(&g_client, mc); + if (rc != MQTT_CODE_CONTINUE) { + break; + } + } + if (iters) { + *iters = i + 1; + } + return rc; +} + +static void setup(void) { } +static void teardown(void) +{ + MqttClient_DeInit(&g_client); +} + +/* ============================================================================ + * SN LWT connect regression tests + * ============================================================================ */ + +/* Happy path: no induced CONTINUE, frames are immediately available. This runs + * in every SN configuration (blocking and non-blocking) and is the guard for + * the refactored BEGIN/WAIT/HEADER state machine: a dropped FALL_THROUGH or + * transition would break the will handshake here even in blocking builds. */ +TEST(sn_connect_lwt_no_continue) +{ + SN_Connect mc; + int rc; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(0 /* no CONTINUE */)); + + mock_net_push(&g_mock, WILLTOPICREQ_FRAME, (int)sizeof(WILLTOPICREQ_FRAME)); + mock_net_push(&g_mock, WILLMSGREQ_FRAME, (int)sizeof(WILLMSGREQ_FRAME)); + mock_net_push(&g_mock, CONNACK_FRAME, (int)sizeof(CONNACK_FRAME)); + + sn_will_setup_connect(&mc); + + rc = sn_connect_pump(&mc, NULL); + + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_EQ(SN_RC_ACCEPTED, mc.ack.return_code); + /* The client sent CONNECT, WILLTOPIC and WILLMSG. */ + ASSERT_TRUE(g_mock.write_calls >= 3); + ASSERT_NO_PENDRESP(); +} + +/* The non-blocking retry behavior is the actual regression and only applies + * when WOLFMQTT_NONBLOCK is built (otherwise SN_Client_WaitType blocks and + * never returns MQTT_CODE_CONTINUE). */ +#ifdef WOLFMQTT_NONBLOCK + +/* The headline regression: with a CONTINUE armed before every gateway frame + * the connect must still converge to SUCCESS and never surface BAD_ARG from a + * duplicate pending-response add. */ +TEST(sn_connect_lwt_nonblock_retry) +{ + SN_Connect mc; + int rc, iters = 0; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(1 /* one CONTINUE per frame */)); + + mock_net_push(&g_mock, WILLTOPICREQ_FRAME, (int)sizeof(WILLTOPICREQ_FRAME)); + mock_net_push(&g_mock, WILLMSGREQ_FRAME, (int)sizeof(WILLMSGREQ_FRAME)); + mock_net_push(&g_mock, CONNACK_FRAME, (int)sizeof(CONNACK_FRAME)); + + sn_will_setup_connect(&mc); + + rc = sn_connect_pump(&mc, &iters); + + /* Pre-fix this returned MQTT_CODE_ERROR_BAD_ARG on the second call. */ + ASSERT_NE(MQTT_CODE_ERROR_BAD_ARG, rc); + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_EQ(SN_RC_ACCEPTED, mc.ack.return_code); + + /* All three gateway frames were consumed and we re-entered at least once + * per frame (proving the non-blocking retry path was actually taken). */ + ASSERT_EQ(g_mock.in_count, g_mock.in_idx); + ASSERT_TRUE(iters >= 4); + + /* The client sent CONNECT, WILLTOPIC and WILLMSG. */ + ASSERT_TRUE(g_mock.write_calls >= 3); + + /* No pending response may be left dangling once connect completes. */ + ASSERT_NO_PENDRESP(); +} + +/* With multiple CONTINUE results armed before each frame the connect must + * still converge: the wait states resume rather than re-adding their pending + * response, regardless of how many times the application re-enters. */ +TEST(sn_connect_lwt_many_continues) +{ + SN_Connect mc; + int rc, iters = 0; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(3 /* three CONTINUE per frame */)); + + mock_net_push(&g_mock, WILLTOPICREQ_FRAME, (int)sizeof(WILLTOPICREQ_FRAME)); + mock_net_push(&g_mock, WILLMSGREQ_FRAME, (int)sizeof(WILLMSGREQ_FRAME)); + mock_net_push(&g_mock, CONNACK_FRAME, (int)sizeof(CONNACK_FRAME)); + + sn_will_setup_connect(&mc); + + rc = sn_connect_pump(&mc, &iters); + + ASSERT_NE(MQTT_CODE_ERROR_BAD_ARG, rc); + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_NO_PENDRESP(); +} + +/* Reusing the same client/connect struct for a second connect must work: the + * will write-state and will_done must have been reset so the second handshake + * adds its pending responses cleanly. */ +TEST(sn_connect_lwt_reconnect) +{ + SN_Connect mc; + int rc; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(1)); + + /* First connect. */ + mock_net_push(&g_mock, WILLTOPICREQ_FRAME, (int)sizeof(WILLTOPICREQ_FRAME)); + mock_net_push(&g_mock, WILLMSGREQ_FRAME, (int)sizeof(WILLMSGREQ_FRAME)); + mock_net_push(&g_mock, CONNACK_FRAME, (int)sizeof(CONNACK_FRAME)); + sn_will_setup_connect(&mc); + rc = sn_connect_pump(&mc, NULL); + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_NO_PENDRESP(); + + /* Re-arm the gateway and connect again with the same struct. */ + g_mock.in_count = 0; + g_mock.in_idx = 0; + g_mock.in_off = 0; + g_mock.continues = g_mock.continues_cfg; + mock_net_push(&g_mock, WILLTOPICREQ_FRAME, (int)sizeof(WILLTOPICREQ_FRAME)); + mock_net_push(&g_mock, WILLMSGREQ_FRAME, (int)sizeof(WILLMSGREQ_FRAME)); + mock_net_push(&g_mock, CONNACK_FRAME, (int)sizeof(CONNACK_FRAME)); + + rc = sn_connect_pump(&mc, NULL); + ASSERT_NE(MQTT_CODE_ERROR_BAD_ARG, rc); + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_NO_PENDRESP(); +} + +#endif /* WOLFMQTT_NONBLOCK */ + +#endif /* WOLFMQTT_SN */ + +/* ============================================================================ + * Suite runner + * ============================================================================ */ + +int main(int argc, char** argv) +{ + (void)argc; + (void)argv; + + TEST_RUNNER_BEGIN(); + +#ifdef WOLFMQTT_SN + TEST_SUITE_BEGIN("mqtt_sn_client", setup, teardown); + + /* Happy path runs in every SN build (blocking and non-blocking). */ + RUN_TEST(sn_connect_lwt_no_continue); + + /* The non-blocking retry regression only exists under WOLFMQTT_NONBLOCK. */ +#ifdef WOLFMQTT_NONBLOCK + RUN_TEST(sn_connect_lwt_nonblock_retry); + RUN_TEST(sn_connect_lwt_many_continues); + RUN_TEST(sn_connect_lwt_reconnect); +#endif + + TEST_SUITE_END(); +#else + PRINTF("test_mqtt_sn_client: skipped (requires WOLFMQTT_SN)"); +#endif + + TEST_RUNNER_END(); +} From 3dbec7dabbbd8b805ec5f6b6c1629086e909d359 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 2 Jun 2026 14:50:34 -0500 Subject: [PATCH 04/23] Add tests for f-5864 --- examples/sn-client/sn-multithread.c | 41 ++++++-- tests/test_mqtt_sn_client.c | 151 ++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+), 6 deletions(-) diff --git a/examples/sn-client/sn-multithread.c b/examples/sn-client/sn-multithread.c index 40f218b86..16b52e8b8 100644 --- a/examples/sn-client/sn-multithread.c +++ b/examples/sn-client/sn-multithread.c @@ -214,8 +214,12 @@ static int client_register(MQTTCtx *mqttCtx) PRINTF("MQTT-SN Register: topic = %s", regist.topicName); - /* Register topic name to get the assigned topic ID */ - rc = SN_Client_Register(&mqttCtx->client, ®ist); + /* Register topic name to get the assigned topic ID. Retry on + MQTT_CODE_CONTINUE (WOLFMQTT_NONBLOCK) so regist.pendResp is not left + linked in client->firstPendResp when this frame returns. */ + do { + rc = SN_Client_Register(&mqttCtx->client, ®ist); + } while (rc == MQTT_CODE_CONTINUE); if ((rc == 0) && (regist.regack.return_code == SN_RC_ACCEPTED)) { /* Topic ID is returned in RegAck */ @@ -370,7 +374,15 @@ static void *subscribe_task(void *param) subscribe.packet_id = mqtt_get_packetid_threadsafe(); PRINTF("MQTT-SN Subscribe: topic name = %s", subscribe.topicNameId); - rc = SN_Client_Subscribe(&mqttCtx->client, &subscribe); + /* Under WOLFMQTT_NONBLOCK the call returns MQTT_CODE_CONTINUE until the + SUBACK arrives. The pending response registered for this thread holds a + pointer into this stack frame (&subscribe.pendResp), so we must keep + retrying until the exchange finishes. Returning on CONTINUE would free the + frame while the entry is still linked in client->firstPendResp, and the + concurrent waitMessage_task would dereference it (use-after-free). */ + do { + rc = SN_Client_Subscribe(&mqttCtx->client, &subscribe); + } while (rc == MQTT_CODE_CONTINUE); PRINTF("....MQTT-SN Subscribe Ack: topic id = %d, rc = %d", subscribe.subAck.topicId, (rc != 0) ? rc : subscribe.subAck.return_code); @@ -498,7 +510,13 @@ static void *publish_task(void *param) publish.buffer = (byte*)buf; publish.total_len = (word16)XSTRLEN(buf); - rc = SN_Client_Publish(&mqttCtx->client, &publish); + /* Retry on MQTT_CODE_CONTINUE (WOLFMQTT_NONBLOCK): publish.pendResp lives in + this stack frame, so returning before the exchange finishes would leave a + dangling entry in client->firstPendResp for the reader thread to traverse + (use-after-free). See subscribe_task for the same rationale. */ + do { + rc = SN_Client_Publish(&mqttCtx->client, &publish); + } while (rc == MQTT_CODE_CONTINUE); PRINTF("MQTT-SN Publish: topic id = %d, rc = %d\r\nPayload = %s", (word16)*publish.topic_name, @@ -530,7 +548,14 @@ static void *ping_task(void *param) /* Keep Alive Ping */ PRINTF("Sending ping keep-alive"); - rc = SN_Client_Ping(&mqttCtx->client, &ping); + /* Retry on MQTT_CODE_CONTINUE (WOLFMQTT_NONBLOCK): ping.pendResp lives in + this stack frame, so breaking out and exiting the thread before the + exchange finishes would leave a dangling entry in + client->firstPendResp for the reader thread to traverse + (use-after-free). See subscribe_task for the same rationale. */ + do { + rc = SN_Client_Ping(&mqttCtx->client, &ping); + } while (rc == MQTT_CODE_CONTINUE); if (rc != MQTT_CODE_SUCCESS) { PRINTF("MQTT-SN Ping Error: %s (%d)", MqttClient_ReturnCodeToString(rc), rc); @@ -552,7 +577,11 @@ static int unsubscribe_do(MQTTCtx *mqttCtx) unsubscribe.topicNameId = mqttCtx->topic_name; unsubscribe.packet_id = mqtt_get_packetid_threadsafe(); - rc = SN_Client_Unsubscribe(&mqttCtx->client, &unsubscribe); + /* Retry on MQTT_CODE_CONTINUE (WOLFMQTT_NONBLOCK) so unsubscribe.pendResp is + not left linked in client->firstPendResp when this frame returns. */ + do { + rc = SN_Client_Unsubscribe(&mqttCtx->client, &unsubscribe); + } while (rc == MQTT_CODE_CONTINUE); PRINTF("MQTT Unsubscribe: %s (rc = %d)", MqttClient_ReturnCodeToString(rc), rc); diff --git a/tests/test_mqtt_sn_client.c b/tests/test_mqtt_sn_client.c index 2fbff2168..4208b7342 100644 --- a/tests/test_mqtt_sn_client.c +++ b/tests/test_mqtt_sn_client.c @@ -200,6 +200,15 @@ static const byte WILLMSGREQ_FRAME[] = { 0x02, SN_MSG_TYPE_WILLMSGREQ }; static const byte CONNACK_FRAME[] = { 0x03, SN_MSG_TYPE_CONNACK, SN_RC_ACCEPTED }; +/* Scripted SUBACK for packet_id 1: total_len=8, type, flags=0, + * topicId=0x000A, packet_id=0x0001, return_code=SN_RC_ACCEPTED. */ +#define SN_TEST_SUB_PACKET_ID 1 +#define SN_TEST_SUB_TOPIC_ID 0x0A +static const byte SUBACK_FRAME[] = { 0x08, SN_MSG_TYPE_SUBACK, 0x00, + 0x00, SN_TEST_SUB_TOPIC_ID, + 0x00, SN_TEST_SUB_PACKET_ID, + SN_RC_ACCEPTED }; + /* ============================================================================ * Test fixtures * ============================================================================ */ @@ -258,6 +267,36 @@ static int sn_connect_pump(SN_Connect* mc, int* iters) return rc; } +static void sn_subscribe_setup(SN_Subscribe* s) +{ + XMEMSET(s, 0, sizeof(*s)); + s->duplicate = 0; + s->qos = MQTT_QOS_0; + s->topic_type = SN_TOPIC_ID_TYPE_NORMAL; + s->topicNameId = "wolf/topic"; + s->packet_id = SN_TEST_SUB_PACKET_ID; +} + +/* Drive SN_Client_Subscribe until it returns something other than CONTINUE, or + * until we exceed a sane iteration cap. Returns the terminal code and the + * number of iterations through *iters. */ +static int sn_subscribe_pump(SN_Subscribe* s, int* iters) +{ + int rc = MQTT_CODE_CONTINUE; + int i; + const int max_iters = 50; + for (i = 0; i < max_iters; i++) { + rc = SN_Client_Subscribe(&g_client, s); + if (rc != MQTT_CODE_CONTINUE) { + break; + } + } + if (iters) { + *iters = i + 1; + } + return rc; +} + static void setup(void) { } static void teardown(void) { @@ -294,6 +333,43 @@ TEST(sn_connect_lwt_no_continue) ASSERT_NO_PENDRESP(); } +/* ============================================================================ + * SN subscribe pending-response lifecycle tests (CWE-416 regression, #5864) + * + * SN_Client_Subscribe registers &subscribe->pendResp in client->firstPendResp + * (MULTITHREAD) and must remove it exactly once the SUBACK arrives. Under + * WOLFMQTT_NONBLOCK the entry stays linked across MQTT_CODE_CONTINUE returns so + * a reader thread can route the response - which is precisely why the caller + * must keep retrying and must not free the subscribe object until the exchange + * completes. These tests pin both halves of that contract: no entry is leaked + * once subscribe finishes, and the entry IS still linked while a call is + * in-flight (so a "remove before CONTINUE" change would be caught here). + * ============================================================================ */ + +/* Happy path: SUBACK is immediately available. Runs in every SN build and + * guards that the pending response is removed once the subscribe completes. */ +TEST(sn_subscribe_no_continue) +{ + SN_Subscribe sub; + int rc; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(0 /* no CONTINUE */)); + + mock_net_push(&g_mock, SUBACK_FRAME, (int)sizeof(SUBACK_FRAME)); + + sn_subscribe_setup(&sub); + + rc = sn_subscribe_pump(&sub, NULL); + + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_EQ(SN_RC_ACCEPTED, sub.subAck.return_code); + ASSERT_EQ(SN_TEST_SUB_TOPIC_ID, sub.subAck.topicId); + /* The client sent SUBSCRIBE. */ + ASSERT_TRUE(g_mock.write_calls >= 1); + /* No pending response may be left dangling once subscribe completes. */ + ASSERT_NO_PENDRESP(); +} + /* The non-blocking retry behavior is the actual regression and only applies * when WOLFMQTT_NONBLOCK is built (otherwise SN_Client_WaitType blocks and * never returns MQTT_CODE_CONTINUE). */ @@ -391,6 +467,78 @@ TEST(sn_connect_lwt_reconnect) ASSERT_NO_PENDRESP(); } +/* The headline #5864 regression. With a CONTINUE armed before the SUBACK the + * subscribe must converge to SUCCESS and remove its pending response exactly + * once. Under MULTITHREAD it also pins the dangling-pointer contract directly: + * after the first in-flight CONTINUE the entry IS linked and points back into + * the caller's subscribe object - the exact pointer the sn-multithread + * subscribe_task used to abandon (freeing the stack frame) by returning on + * CONTINUE instead of retrying. */ +TEST(sn_subscribe_nonblock_pendresp_lifecycle) +{ + SN_Subscribe sub; + int rc, iters = 0; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(1 /* one CONTINUE per frame */)); + + mock_net_push(&g_mock, SUBACK_FRAME, (int)sizeof(SUBACK_FRAME)); + + sn_subscribe_setup(&sub); + + /* First call sends SUBSCRIBE and the armed CONTINUE forces an in-flight + * return before the SUBACK is delivered. */ + rc = SN_Client_Subscribe(&g_client, &sub); + ASSERT_EQ(MQTT_CODE_CONTINUE, rc); +#ifdef WOLFMQTT_MULTITHREAD + /* The pending response must stay registered while the exchange is in + * flight (so a reader thread could route the SUBACK) and must point back + * into the caller-owned object. This is the entry that becomes a dangling + * pointer if the caller returns/frees the object instead of retrying. */ + ASSERT_NOT_NULL(g_client.firstPendResp); + ASSERT_EQ((void*)&sub.pendResp, (void*)g_client.firstPendResp); +#endif + + /* Keep retrying, as a correct non-blocking caller must, until it resolves.*/ + rc = sn_subscribe_pump(&sub, &iters); + + /* Pre-fix subscribe_task would have returned on the CONTINUE above. */ + ASSERT_NE(MQTT_CODE_ERROR_BAD_ARG, rc); + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_EQ(SN_RC_ACCEPTED, sub.subAck.return_code); + ASSERT_EQ(SN_TEST_SUB_TOPIC_ID, sub.subAck.topicId); + + /* All scripted frames consumed and the non-blocking retry path was taken. */ + ASSERT_EQ(g_mock.in_count, g_mock.in_idx); + + /* The pending response is gone once subscribe completes - no dangling + * entry remains for another thread to dereference. */ + ASSERT_NO_PENDRESP(); +} + +/* With multiple CONTINUE results armed before the SUBACK the subscribe must + * still converge: repeated re-entry resumes the wait rather than re-adding the + * pending response (which would surface MQTT_CODE_ERROR_BAD_ARG as a duplicate). + */ +TEST(sn_subscribe_many_continues) +{ + SN_Subscribe sub; + int rc, iters = 0; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(3 /* three CONTINUE per frame */)); + + mock_net_push(&g_mock, SUBACK_FRAME, (int)sizeof(SUBACK_FRAME)); + + sn_subscribe_setup(&sub); + + rc = sn_subscribe_pump(&sub, &iters); + + ASSERT_NE(MQTT_CODE_ERROR_BAD_ARG, rc); + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_EQ(SN_RC_ACCEPTED, sub.subAck.return_code); + ASSERT_TRUE(iters >= 2); + ASSERT_NO_PENDRESP(); +} + #endif /* WOLFMQTT_NONBLOCK */ #endif /* WOLFMQTT_SN */ @@ -411,12 +559,15 @@ int main(int argc, char** argv) /* Happy path runs in every SN build (blocking and non-blocking). */ RUN_TEST(sn_connect_lwt_no_continue); + RUN_TEST(sn_subscribe_no_continue); /* The non-blocking retry regression only exists under WOLFMQTT_NONBLOCK. */ #ifdef WOLFMQTT_NONBLOCK RUN_TEST(sn_connect_lwt_nonblock_retry); RUN_TEST(sn_connect_lwt_many_continues); RUN_TEST(sn_connect_lwt_reconnect); + RUN_TEST(sn_subscribe_nonblock_pendresp_lifecycle); + RUN_TEST(sn_subscribe_many_continues); #endif TEST_SUITE_END(); From f77ffd80881696353faf09a58bddc0ef23863598 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 2 Jun 2026 15:11:37 -0500 Subject: [PATCH 05/23] Fix f-3137 harden SN_WillMessage --- src/mqtt_client.c | 9 ++-- src/mqtt_sn_client.c | 17 +++++-- tests/test_mqtt_sn_client.c | 93 +++++++++++++++++++++++++++++++++++++ wolfmqtt/mqtt_client.h | 7 +++ 4 files changed, 118 insertions(+), 8 deletions(-) diff --git a/src/mqtt_client.c b/src/mqtt_client.c index 7fd1ad533..f5bc6fac9 100644 --- a/src/mqtt_client.c +++ b/src/mqtt_client.c @@ -27,8 +27,10 @@ #include "wolfmqtt/mqtt_client.h" /* Secure memory zeroing - uses volatile pointer to prevent the compiler - * from optimizing away the stores (dead-store elimination). */ -static void MqttClient_ForceZero(void* mem, word32 len) + * from optimizing away the stores (dead-store elimination). + * Declared WOLFMQTT_LOCAL in mqtt_client.h so the MQTT-SN client can reuse it + * (see SN_WillMessage) via the shared CLIENT_FORCE_ZERO macro. */ +WOLFMQTT_LOCAL void MqttClient_ForceZero(void* mem, word32 len) { volatile byte* p = (volatile byte*)mem; word32 i; @@ -37,9 +39,6 @@ static void MqttClient_ForceZero(void* mem, word32 len) } } -#define CLIENT_FORCE_ZERO(mem, len) \ - MqttClient_ForceZero(mem, (word32)(len)) - /* DOCUMENTED BUILD OPTIONS: * * WOLFMQTT_MULTITHREAD: Enables multi-thread support with mutex protection on diff --git a/src/mqtt_sn_client.c b/src/mqtt_sn_client.c index 3a6cc9df4..37187b97b 100644 --- a/src/mqtt_sn_client.c +++ b/src/mqtt_sn_client.c @@ -1151,16 +1151,27 @@ static int SN_WillMessage(MqttClient *client, SN_Will *will) rc = 0; } } - #ifdef WOLFMQTT_MULTITHREAD - wm_SemUnlock(&client->lockSend); - #endif #ifdef WOLFMQTT_NONBLOCK if (rc == MQTT_CODE_CONTINUE) { + /* Send not complete: tx_buf still holds the will payload and is + * needed to resume, so do not scrub it yet. */ + #ifdef WOLFMQTT_MULTITHREAD + wm_SemUnlock(&client->lockSend); + #endif return rc; /* resume send on next call */ } #endif + /* The encoded WILLMSG contains the will payload (potentially + * sensitive). Scrub tx_buf before releasing lockSend so another + * thread cannot observe residual plaintext (mirrors the mitigation + * in MqttClient_Connect). */ + CLIENT_FORCE_ZERO(client->tx_buf, client->write.len); + #ifdef WOLFMQTT_MULTITHREAD + wm_SemUnlock(&client->lockSend); + #endif + /* reset state */ will->stat.write = MQTT_MSG_BEGIN; break; diff --git a/tests/test_mqtt_sn_client.c b/tests/test_mqtt_sn_client.c index 4208b7342..0a5691c36 100644 --- a/tests/test_mqtt_sn_client.c +++ b/tests/test_mqtt_sn_client.c @@ -303,6 +303,73 @@ static void teardown(void) MqttClient_DeInit(&g_client); } +/* memcmp-style search: returns 1 if `needle` (nlen bytes) appears in `hay`. */ +static int sn_buf_contains(const byte* hay, int hlen, + const byte* needle, int nlen) +{ + int i; + if (hay == NULL || nlen <= 0 || hlen < nlen) { + return 0; + } + for (i = 0; i + nlen <= hlen; i++) { + if (XMEMCMP(&hay[i], needle, (size_t)nlen) == 0) { + return 1; + } + } + return 0; +} + +/* Shared body for the #3137 will-payload scrub tests. Runs the scripted LWT + * connect (with `continues` MQTT_CODE_CONTINUE armed before each gateway frame) + * and asserts the will payload is scrubbed from tx_buf once the WILLMSG has been + * sent. The ASSERT_* macros bail out of this helper on failure and set the + * shared failure flag that RUN_TEST inspects, so factoring this out is safe. */ +static void sn_will_scrub_check(int continues) +{ + SN_Connect mc; + int rc, i; + const byte* willMsg; + int willMsgLen, willPktLen; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(continues)); + + mock_net_push(&g_mock, WILLTOPICREQ_FRAME, (int)sizeof(WILLTOPICREQ_FRAME)); + mock_net_push(&g_mock, WILLMSGREQ_FRAME, (int)sizeof(WILLMSGREQ_FRAME)); + mock_net_push(&g_mock, CONNACK_FRAME, (int)sizeof(CONNACK_FRAME)); + + sn_will_setup_connect(&mc); + willMsg = mc.will.willMsg; + willMsgLen = (int)mc.will.willMsgLen; + /* Small WILLMSG packet: 1-byte length + 1-byte type + payload. */ + willPktLen = 2 + willMsgLen; + + rc = sn_connect_pump(&mc, NULL); + + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_EQ(SN_RC_ACCEPTED, mc.ack.return_code); + + /* Positive control: the will payload really was written to the wire, so the + * scrub assertions below cannot pass trivially. */ + ASSERT_TRUE(sn_buf_contains(g_mock.out, g_mock.out_len, + willMsg, willMsgLen)); + + /* Core #3137 assertion: the will payload must not linger anywhere in the + * client tx buffer once the WILLMSG has been sent. */ + ASSERT_FALSE(sn_buf_contains(g_client.tx_buf, g_client.tx_buf_len, + willMsg, willMsgLen)); + + /* Stronger boundary check: every byte of the WILLMSG packet region must be + * zero. Catches both deletion of the CLIENT_FORCE_ZERO call and an + * xfer -> 0 mutation that turns the wipe into a no-op. */ + for (i = 0; i < willPktLen; i++) { + if (g_client.tx_buf[i] != 0) { + FAIL("tx_buf within WILLMSG range is non-zero after connect"); + } + } + + ASSERT_NO_PENDRESP(); +} + /* ============================================================================ * SN LWT connect regression tests * ============================================================================ */ @@ -333,6 +400,22 @@ TEST(sn_connect_lwt_no_continue) ASSERT_NO_PENDRESP(); } +/* ============================================================================ + * SN will-payload scrub test (#3137) + * + * SN_WillMessage encodes the last-will payload into client->tx_buf via + * SN_Encode_WillMsg and used to leave it there after the WILLMSG had been sent, + * so local memory inspection could recover the will plaintext until the next + * encode overwrote it. SN_WillMessage now scrubs tx_buf (CLIENT_FORCE_ZERO) + * before releasing lockSend once the send completes, mirroring the + * MqttClient_Connect credential mitigation. This runs in every SN build because + * the scrub happens on the send-complete path regardless of WOLFMQTT_NONBLOCK. + * ============================================================================ */ +TEST(sn_will_payload_scrubbed_after_send) +{ + sn_will_scrub_check(0 /* no CONTINUE */); +} + /* ============================================================================ * SN subscribe pending-response lifecycle tests (CWE-416 regression, #5864) * @@ -467,6 +550,14 @@ TEST(sn_connect_lwt_reconnect) ASSERT_NO_PENDRESP(); } +/* #3137 through the non-blocking retry path: with a CONTINUE armed before each + * gateway frame the will payload must still be scrubbed from tx_buf once the + * connect completes. */ +TEST(sn_will_payload_scrubbed_nonblock) +{ + sn_will_scrub_check(1 /* one CONTINUE per frame */); +} + /* The headline #5864 regression. With a CONTINUE armed before the SUBACK the * subscribe must converge to SUCCESS and remove its pending response exactly * once. Under MULTITHREAD it also pins the dangling-pointer contract directly: @@ -559,6 +650,7 @@ int main(int argc, char** argv) /* Happy path runs in every SN build (blocking and non-blocking). */ RUN_TEST(sn_connect_lwt_no_continue); + RUN_TEST(sn_will_payload_scrubbed_after_send); RUN_TEST(sn_subscribe_no_continue); /* The non-blocking retry regression only exists under WOLFMQTT_NONBLOCK. */ @@ -566,6 +658,7 @@ int main(int argc, char** argv) RUN_TEST(sn_connect_lwt_nonblock_retry); RUN_TEST(sn_connect_lwt_many_continues); RUN_TEST(sn_connect_lwt_reconnect); + RUN_TEST(sn_will_payload_scrubbed_nonblock); RUN_TEST(sn_subscribe_nonblock_pendresp_lifecycle); RUN_TEST(sn_subscribe_many_continues); #endif diff --git a/wolfmqtt/mqtt_client.h b/wolfmqtt/mqtt_client.h index ea30cc635..416be714c 100644 --- a/wolfmqtt/mqtt_client.h +++ b/wolfmqtt/mqtt_client.h @@ -609,6 +609,13 @@ WOLFMQTT_LOCAL int MqttClient_CheckPendResp(MqttClient *client, byte wait_type, #endif WOLFMQTT_LOCAL int MqttPacket_HandleNetError(MqttClient *client, int rc); +/* Securely zero `len` bytes of `mem` (e.g. client->tx_buf) to scrub plaintext + * such as credentials or will payloads before releasing the send lock. Uses a + * volatile pointer so the stores are not optimized away. */ +WOLFMQTT_LOCAL void MqttClient_ForceZero(void* mem, word32 len); +#define CLIENT_FORCE_ZERO(mem, len) \ + MqttClient_ForceZero(mem, (word32)(len)) + #ifdef __cplusplus } /* extern "C" */ #endif From e3a4157a3088b91d59221411854102d696fb072a Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 2 Jun 2026 15:41:47 -0500 Subject: [PATCH 06/23] Fix f-3138 harden SN_Client_WillMsgUpdate --- src/mqtt_sn_client.c | 29 ++++++- tests/test_mqtt_sn_client.c | 161 ++++++++++++++++++++++++++++++++++++ 2 files changed, 187 insertions(+), 3 deletions(-) diff --git a/src/mqtt_sn_client.c b/src/mqtt_sn_client.c index 37187b97b..f4a5cf1df 100644 --- a/src/mqtt_sn_client.c +++ b/src/mqtt_sn_client.c @@ -1423,6 +1423,7 @@ int SN_Client_WillMsgUpdate(MqttClient *client, SN_Will *will) } if (will->stat.write == MQTT_MSG_BEGIN) { + int xfer = 0; #ifdef WOLFMQTT_MULTITHREAD /* Lock send socket mutex */ rc = wm_SemLock(&client->lockSend); @@ -1461,9 +1462,28 @@ int SN_Client_WillMsgUpdate(MqttClient *client, SN_Will *will) } #endif - /* Send Will Message Update packet */ - rc = MqttPacket_Write(client, client->tx_buf, client->write.len); - if (rc != client->write.len) { + /* Send Will Message Update packet. Save write.len into xfer first: the + * encoded WILLMSGUPD holds will->willMsg (a possibly rotated/secret will + * payload) in tx_buf, which must be scrubbed before lockSend is released + * on every return path below. + * + * This differs intentionally from MqttClient_Connect and SN_WillMessage: + * those keep a resume state (MQTT_MSG_HEADER) and so must NOT scrub on + * MQTT_CODE_CONTINUE, because tx_buf is still needed to finish a partial + * non-blocking send. This function has no such resume state - it re-runs + * this whole MQTT_MSG_BEGIN block (re-encoding tx_buf) on every call. A + * non-blocking partial write returns MQTT_CODE_CONTINUE, which is != xfer + * and therefore lands in the error branch below; scrubbing there is safe + * because the next call re-encodes the identical bytes before the write + * resumes. */ + xfer = client->write.len; + rc = MqttPacket_Write(client, client->tx_buf, xfer); + if (rc != xfer) { + /* Send failed (or returned MQTT_CODE_CONTINUE): scrub the will + * payload from tx_buf before releasing lockSend so another thread - + * or a later memory/core-dump inspection - cannot recover residual + * plaintext. */ + CLIENT_FORCE_ZERO(client->tx_buf, xfer); #ifdef WOLFMQTT_MULTITHREAD wm_SemUnlock(&client->lockSend); if (wm_SemLock(&client->lockClient) == 0) { @@ -1473,6 +1493,9 @@ int SN_Client_WillMsgUpdate(MqttClient *client, SN_Will *will) #endif return rc; } + /* WILLMSGUPD sent: scrub the will payload from tx_buf before releasing + * lockSend, for the same reason as the error path above. */ + CLIENT_FORCE_ZERO(client->tx_buf, xfer); #ifdef WOLFMQTT_MULTITHREAD wm_SemUnlock(&client->lockSend); #endif diff --git a/tests/test_mqtt_sn_client.c b/tests/test_mqtt_sn_client.c index 0a5691c36..288990e08 100644 --- a/tests/test_mqtt_sn_client.c +++ b/tests/test_mqtt_sn_client.c @@ -89,6 +89,10 @@ typedef struct MockNet { byte out[MOCK_OUT_LEN]; int out_len; /* bytes captured from write() */ + int write_fail_rc; /* if nonzero, write() returns this instead of + * accepting the buffer (simulate short/failed + * write) */ + int read_calls; int write_calls; } MockNet; @@ -111,6 +115,12 @@ static int mock_write(void *ctx, const byte* buf, int buf_len, int timeout_ms) MockNet* net = (MockNet*)ctx; (void)timeout_ms; net->write_calls++; + if (net->write_fail_rc != 0) { + /* Simulate a short/failed write so the caller's rc != xfer error path is + * exercised. The buffer is left as the caller encoded it (not captured), + * matching a real transport that errored mid-send. */ + return net->write_fail_rc; + } if (buf_len > 0 && net->out_len + buf_len <= MOCK_OUT_LEN) { XMEMCPY(&net->out[net->out_len], buf, (size_t)buf_len); net->out_len += buf_len; @@ -200,6 +210,10 @@ static const byte WILLMSGREQ_FRAME[] = { 0x02, SN_MSG_TYPE_WILLMSGREQ }; static const byte CONNACK_FRAME[] = { 0x03, SN_MSG_TYPE_CONNACK, SN_RC_ACCEPTED }; +/* Gateway response to a WILLMSGUPD: total_len=3, type, return_code. */ +static const byte WILLMSGRESP_FRAME[] = { 0x03, SN_MSG_TYPE_WILLMSGRESP, + SN_RC_ACCEPTED }; + /* Scripted SUBACK for packet_id 1: total_len=8, type, flags=0, * topicId=0x000A, packet_id=0x0001, return_code=SN_RC_ACCEPTED. */ #define SN_TEST_SUB_PACKET_ID 1 @@ -370,6 +384,76 @@ static void sn_will_scrub_check(int continues) ASSERT_NO_PENDRESP(); } +/* Drive SN_Client_WillMsgUpdate until it returns something other than CONTINUE, + * or until we exceed a sane iteration cap. Returns the terminal code. */ +static int sn_will_msg_update_pump(SN_Will* will) +{ + int rc = MQTT_CODE_CONTINUE; + int i; + const int max_iters = 50; + for (i = 0; i < max_iters; i++) { + rc = SN_Client_WillMsgUpdate(&g_client, will); + if (rc != MQTT_CODE_CONTINUE) { + break; + } + } + return rc; +} + +/* Shared body for the #3138 WILLMSGUPD scrub tests. Drives a scripted + * SN_Client_WillMsgUpdate exchange (with `continues` MQTT_CODE_CONTINUE armed + * before the WILLMSGRESP) and asserts the updated will payload is scrubbed from + * tx_buf once the WILLMSGUPD has been sent. Mirrors sn_will_scrub_check, but for + * the standalone will-message update API rather than the connect handshake. */ +static void sn_will_msg_update_scrub_check(int continues) +{ + SN_Will will; + int rc, i; + /* A distinctive "rotated secret" payload so the scrub assertions below + * cannot pass by coincidence (e.g. against an already-zero buffer). */ + static const byte secret[] = "s3cret-rotated-will-payload"; + const int secretLen = (int)sizeof(secret) - 1; /* drop terminating NUL */ + /* Small WILLMSGUPD packet: 1-byte length + 1-byte type + payload. */ + const int willPktLen = 2 + secretLen; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(continues)); + + mock_net_push(&g_mock, WILLMSGRESP_FRAME, (int)sizeof(WILLMSGRESP_FRAME)); + + XMEMSET(&will, 0, sizeof(will)); + will.qos = 0; + will.retain = 0; + will.willTopic = "wolf/lwt"; + will.willMsg = (byte*)secret; + will.willMsgLen = (word16)secretLen; + + rc = sn_will_msg_update_pump(&will); + + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_EQ(SN_RC_ACCEPTED, will.resp.msgResp.return_code); + + /* Positive control: the will payload really was written to the wire, so the + * scrub assertions below cannot pass trivially. */ + ASSERT_TRUE(sn_buf_contains(g_mock.out, g_mock.out_len, + secret, secretLen)); + + /* Core #3138 assertion: the updated will payload must not linger anywhere in + * the client tx buffer once the WILLMSGUPD has been sent. */ + ASSERT_FALSE(sn_buf_contains(g_client.tx_buf, g_client.tx_buf_len, + secret, secretLen)); + + /* Stronger boundary check: every byte of the WILLMSGUPD packet region must + * be zero. Catches both deletion of the CLIENT_FORCE_ZERO call and an + * xfer -> 0 mutation that turns the wipe into a no-op. */ + for (i = 0; i < willPktLen; i++) { + if (g_client.tx_buf[i] != 0) { + FAIL("tx_buf within WILLMSGUPD range is non-zero after update"); + } + } + + ASSERT_NO_PENDRESP(); +} + /* ============================================================================ * SN LWT connect regression tests * ============================================================================ */ @@ -416,6 +500,71 @@ TEST(sn_will_payload_scrubbed_after_send) sn_will_scrub_check(0 /* no CONTINUE */); } +/* ============================================================================ + * SN will-message-update scrub test (#3138) + * + * SN_Client_WillMsgUpdate encodes the new last-will payload into client->tx_buf + * via SN_Encode_WillMsgUpdate and used to leave it there after the WILLMSGUPD + * had been sent, so a local attacker could recover the rotated will plaintext + * (until the next encode overwrote it) via memory inspection or a core dump. + * SN_Client_WillMsgUpdate now scrubs tx_buf (CLIENT_FORCE_ZERO) before releasing + * lockSend on both the success and short-write paths, mirroring SN_WillMessage + * and the MqttClient_Connect credential mitigation. This runs in every SN build + * because the scrub happens on the send path regardless of WOLFMQTT_NONBLOCK. + * ============================================================================ */ +TEST(sn_willmsgupd_payload_scrubbed_after_send) +{ + sn_will_msg_update_scrub_check(0 /* no CONTINUE */); +} + +/* #3138 error-path coverage. The success-path tests above never reach the + * rc != xfer branch because mock_write() always accepts the whole buffer, so + * the error-path CLIENT_FORCE_ZERO (and the pendResp removal beside it) would go + * untested. Here the mock is armed to fail the WILLMSGUPD write, forcing that + * branch: the will payload - already encoded into tx_buf before the write failed + * - must still be scrubbed, and the pending response removed, before the error + * is returned. Runs in every SN build (the write failure is independent of + * WOLFMQTT_NONBLOCK). */ +TEST(sn_willmsgupd_payload_scrubbed_on_write_error) +{ + SN_Will will; + int rc, i; + static const byte secret[] = "s3cret-rotated-will-payload"; + const int secretLen = (int)sizeof(secret) - 1; /* drop terminating NUL */ + const int willPktLen = 2 + secretLen; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(0 /* no CONTINUE */)); + + /* Arm the mock so the WILLMSGUPD write returns a network error, forcing the + * rc != xfer branch in SN_Client_WillMsgUpdate. */ + g_mock.write_fail_rc = MQTT_CODE_ERROR_NETWORK; + + XMEMSET(&will, 0, sizeof(will)); + will.willTopic = "wolf/lwt"; + will.willMsg = (byte*)secret; + will.willMsgLen = (word16)secretLen; + + rc = SN_Client_WillMsgUpdate(&g_client, &will); + + /* The failed send surfaces the network error to the caller. */ + ASSERT_EQ(MQTT_CODE_ERROR_NETWORK, rc); + + /* The will payload was encoded into tx_buf before the write failed; the + * error-path scrub must have wiped it. Deleting the rc != xfer + * CLIENT_FORCE_ZERO leaves the plaintext here and fails these assertions. */ + ASSERT_FALSE(sn_buf_contains(g_client.tx_buf, g_client.tx_buf_len, + secret, secretLen)); + for (i = 0; i < willPktLen; i++) { + if (g_client.tx_buf[i] != 0) { + FAIL("tx_buf within WILLMSGUPD range is non-zero after write error"); + } + } + + /* The pending response added before the send must be removed on the error + * path so no dangling entry is left behind. */ + ASSERT_NO_PENDRESP(); +} + /* ============================================================================ * SN subscribe pending-response lifecycle tests (CWE-416 regression, #5864) * @@ -558,6 +707,15 @@ TEST(sn_will_payload_scrubbed_nonblock) sn_will_scrub_check(1 /* one CONTINUE per frame */); } +/* #3138 through the non-blocking retry path: with a CONTINUE armed before the + * WILLMSGRESP the updated will payload must still be scrubbed from tx_buf once + * the update completes. The scrub happens on the send-complete path (before the + * wait re-enters), so re-entry after CONTINUE must not resurrect the plaintext. */ +TEST(sn_willmsgupd_payload_scrubbed_nonblock) +{ + sn_will_msg_update_scrub_check(1 /* one CONTINUE per frame */); +} + /* The headline #5864 regression. With a CONTINUE armed before the SUBACK the * subscribe must converge to SUCCESS and remove its pending response exactly * once. Under MULTITHREAD it also pins the dangling-pointer contract directly: @@ -651,6 +809,8 @@ int main(int argc, char** argv) /* Happy path runs in every SN build (blocking and non-blocking). */ RUN_TEST(sn_connect_lwt_no_continue); RUN_TEST(sn_will_payload_scrubbed_after_send); + RUN_TEST(sn_willmsgupd_payload_scrubbed_after_send); + RUN_TEST(sn_willmsgupd_payload_scrubbed_on_write_error); RUN_TEST(sn_subscribe_no_continue); /* The non-blocking retry regression only exists under WOLFMQTT_NONBLOCK. */ @@ -659,6 +819,7 @@ int main(int argc, char** argv) RUN_TEST(sn_connect_lwt_many_continues); RUN_TEST(sn_connect_lwt_reconnect); RUN_TEST(sn_will_payload_scrubbed_nonblock); + RUN_TEST(sn_willmsgupd_payload_scrubbed_nonblock); RUN_TEST(sn_subscribe_nonblock_pendresp_lifecycle); RUN_TEST(sn_subscribe_many_continues); #endif From c269605eb69789a775f375a7bc1d16bf9f2b7832 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 2 Jun 2026 15:53:44 -0500 Subject: [PATCH 07/23] Fix f-2756 SN_Client_Subscribe false return code --- src/mqtt_sn_client.c | 12 +++++++ tests/test_mqtt_sn_client.c | 71 +++++++++++++++++++++++++++++++++++++ wolfmqtt/mqtt_sn_client.h | 10 ++++-- 3 files changed, 91 insertions(+), 2 deletions(-) diff --git a/src/mqtt_sn_client.c b/src/mqtt_sn_client.c index f4a5cf1df..84188e831 100644 --- a/src/mqtt_sn_client.c +++ b/src/mqtt_sn_client.c @@ -1610,6 +1610,18 @@ int SN_Client_Subscribe(MqttClient *client, SN_Subscribe *subscribe) /* reset state */ subscribe->stat.write = MQTT_MSG_BEGIN; + /* SUBACK was received and decoded, but the gateway rejected the + * subscription. The specific reason is in subscribe->subAck.return_code + * (SN_ReturnCodes): SN_RC_CONGESTION, SN_RC_INVTOPICNAME, or + * SN_RC_NOTSUPPORTED. Unlike v3.1.1/v5 SUBSCRIBE there is only a single + * per-packet topic, so the one return code is the whole result. Surface a + * distinct error so a caller checking only the function return value does + * not wait for messages the gateway will never deliver. */ + if (rc == MQTT_CODE_SUCCESS && + subscribe->subAck.return_code != SN_RC_ACCEPTED) { + rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_SUBSCRIBE_REJECTED); + } + return rc; } diff --git a/tests/test_mqtt_sn_client.c b/tests/test_mqtt_sn_client.c index 288990e08..559e0d216 100644 --- a/tests/test_mqtt_sn_client.c +++ b/tests/test_mqtt_sn_client.c @@ -223,6 +223,14 @@ static const byte SUBACK_FRAME[] = { 0x08, SN_MSG_TYPE_SUBACK, 0x00, 0x00, SN_TEST_SUB_PACKET_ID, SN_RC_ACCEPTED }; +/* Scripted SUBACK rejecting packet_id 1: same framing as SUBACK_FRAME but the + * gateway returns SN_RC_INVTOPICNAME and topicId 0x0000 (no topic assigned), + * as a gateway does when it declines the subscription. */ +static const byte SUBACK_REJECT_FRAME[] = { 0x08, SN_MSG_TYPE_SUBACK, 0x00, + 0x00, 0x00, + 0x00, SN_TEST_SUB_PACKET_ID, + SN_RC_INVTOPICNAME }; + /* ============================================================================ * Test fixtures * ============================================================================ */ @@ -602,6 +610,37 @@ TEST(sn_subscribe_no_continue) ASSERT_NO_PENDRESP(); } +/* #2756: when the gateway rejects the subscription the SUBACK still decodes + * cleanly, so SN_Client_WaitType normalizes the non-negative decode result to + * MQTT_CODE_SUCCESS. Pre-fix SN_Client_Subscribe returned that SUCCESS verbatim + * and a caller would wait forever for messages the gateway never delivers. The + * function must now map a non-ACCEPTED subAck.return_code to + * MQTT_CODE_ERROR_SUBSCRIBE_REJECTED while still leaving the raw gateway code + * visible for diagnosis. Runs in every SN build. */ +TEST(sn_subscribe_rejected) +{ + SN_Subscribe sub; + int rc; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(0 /* no CONTINUE */)); + + mock_net_push(&g_mock, SUBACK_REJECT_FRAME, + (int)sizeof(SUBACK_REJECT_FRAME)); + + sn_subscribe_setup(&sub); + + rc = sn_subscribe_pump(&sub, NULL); + + /* Pre-fix this returned MQTT_CODE_SUCCESS. */ + ASSERT_EQ(MQTT_CODE_ERROR_SUBSCRIBE_REJECTED, rc); + /* The raw gateway reject code is still surfaced for the caller. */ + ASSERT_EQ(SN_RC_INVTOPICNAME, sub.subAck.return_code); + /* The client still sent the SUBSCRIBE before the rejection came back. */ + ASSERT_TRUE(g_mock.write_calls >= 1); + /* No pending response may be left dangling, even on the reject path. */ + ASSERT_NO_PENDRESP(); +} + /* The non-blocking retry behavior is the actual regression and only applies * when WOLFMQTT_NONBLOCK is built (otherwise SN_Client_WaitType blocks and * never returns MQTT_CODE_CONTINUE). */ @@ -788,6 +827,36 @@ TEST(sn_subscribe_many_continues) ASSERT_NO_PENDRESP(); } +/* #2756 through the non-blocking retry path: a rejected SUBACK arriving after a + * CONTINUE must still resolve to MQTT_CODE_ERROR_SUBSCRIBE_REJECTED. The + * rejection mapping must run only once the wait converges - the in-flight + * CONTINUE is returned verbatim and must never be turned into a rejection. */ +TEST(sn_subscribe_rejected_nonblock) +{ + SN_Subscribe sub; + int rc, iters = 0; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(1 /* one CONTINUE per frame */)); + + mock_net_push(&g_mock, SUBACK_REJECT_FRAME, + (int)sizeof(SUBACK_REJECT_FRAME)); + + sn_subscribe_setup(&sub); + + /* The armed CONTINUE forces an in-flight return before the SUBACK arrives; + * it must be surfaced as CONTINUE, not prematurely mapped to a rejection. */ + rc = SN_Client_Subscribe(&g_client, &sub); + ASSERT_EQ(MQTT_CODE_CONTINUE, rc); + + /* Keep retrying until the rejected SUBACK is delivered and resolved. */ + rc = sn_subscribe_pump(&sub, &iters); + + ASSERT_EQ(MQTT_CODE_ERROR_SUBSCRIBE_REJECTED, rc); + ASSERT_EQ(SN_RC_INVTOPICNAME, sub.subAck.return_code); + ASSERT_EQ(g_mock.in_count, g_mock.in_idx); + ASSERT_NO_PENDRESP(); +} + #endif /* WOLFMQTT_NONBLOCK */ #endif /* WOLFMQTT_SN */ @@ -812,6 +881,7 @@ int main(int argc, char** argv) RUN_TEST(sn_willmsgupd_payload_scrubbed_after_send); RUN_TEST(sn_willmsgupd_payload_scrubbed_on_write_error); RUN_TEST(sn_subscribe_no_continue); + RUN_TEST(sn_subscribe_rejected); /* The non-blocking retry regression only exists under WOLFMQTT_NONBLOCK. */ #ifdef WOLFMQTT_NONBLOCK @@ -822,6 +892,7 @@ int main(int argc, char** argv) RUN_TEST(sn_willmsgupd_payload_scrubbed_nonblock); RUN_TEST(sn_subscribe_nonblock_pendresp_lifecycle); RUN_TEST(sn_subscribe_many_continues); + RUN_TEST(sn_subscribe_rejected_nonblock); #endif TEST_SUITE_END(); diff --git a/wolfmqtt/mqtt_sn_client.h b/wolfmqtt/mqtt_sn_client.h index e3f6ef9ad..a569eac62 100644 --- a/wolfmqtt/mqtt_sn_client.h +++ b/wolfmqtt/mqtt_sn_client.h @@ -156,8 +156,14 @@ WOLFMQTT_API int SN_Client_Publish( * \param client Pointer to MqttClient structure * \param subscribe Pointer to SN_Subscribe structure initialized with subscription topic list and desired QoS. - * \return MQTT_CODE_SUCCESS or MQTT_CODE_ERROR_* - (see enum MqttPacketResponseCodes) + * \return MQTT_CODE_SUCCESS if the gateway accepted the subscription, + MQTT_CODE_ERROR_SUBSCRIBE_REJECTED if the gateway returned a + non-zero SUBACK return_code (check subscribe->subAck.return_code + for the specific SN_ReturnCodes value), or another + MQTT_CODE_ERROR_* for transport/protocol failures (see enum + MqttPacketResponseCodes). Callers must not assume messages will + be delivered for this topic when this function does not return + MQTT_CODE_SUCCESS. */ WOLFMQTT_API int SN_Client_Subscribe( MqttClient *client, From b9771a02a608d6d5cd854eed2b75ae9984cfd776 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Mon, 8 Jun 2026 16:52:19 -0500 Subject: [PATCH 08/23] Fix f-3132 SN_Client_Ping with null --- src/mqtt_sn_client.c | 13 ++- tests/test_mqtt_sn_client.c | 163 ++++++++++++++++++++++++++++++++++++ wolfmqtt/mqtt_sn_client.h | 6 ++ 3 files changed, 181 insertions(+), 1 deletion(-) diff --git a/src/mqtt_sn_client.c b/src/mqtt_sn_client.c index 84188e831..225a51427 100644 --- a/src/mqtt_sn_client.c +++ b/src/mqtt_sn_client.c @@ -2044,8 +2044,19 @@ int SN_Client_Ping(MqttClient *client, SN_PingReq *ping) rc = SN_Client_WaitType(client, ping, SN_MSG_TYPE_PING_RESP, 0, client->cmd_timeout_ms); #ifdef WOLFMQTT_NONBLOCK - if (rc == MQTT_CODE_CONTINUE) + if (rc == MQTT_CODE_CONTINUE && ping != &loc_ping) { + /* Caller owns 'ping': the object (and, under MULTITHREAD, its + * registered pendResp) lives across calls, so leave the pending + * response linked and let the caller resume on the next call. */ return rc; + } + /* For the ping==NULL fallback we used the stack-local loc_ping, which + * cannot persist across calls. Returning CONTINUE here would leave + * &loc_ping.pendResp linked on client->firstPendResp after this frame + * unwinds; a later call reuses the stack address, so the stale entry + * could be dereferenced by MqttClient_CheckPendResp on the receive path + * (use-after-scope). Fall through to remove the entry before returning, + * even on a CONTINUE result. */ #endif #ifdef WOLFMQTT_MULTITHREAD if (wm_SemLock(&client->lockClient) == 0) { diff --git a/tests/test_mqtt_sn_client.c b/tests/test_mqtt_sn_client.c index 559e0d216..f7556f717 100644 --- a/tests/test_mqtt_sn_client.c +++ b/tests/test_mqtt_sn_client.c @@ -231,6 +231,9 @@ static const byte SUBACK_REJECT_FRAME[] = { 0x08, SN_MSG_TYPE_SUBACK, 0x00, 0x00, SN_TEST_SUB_PACKET_ID, SN_RC_INVTOPICNAME }; +/* Gateway PINGRESP: total_len=2, type. */ +static const byte PINGRESP_FRAME[] = { 0x02, SN_MSG_TYPE_PING_RESP }; + /* ============================================================================ * Test fixtures * ============================================================================ */ @@ -319,6 +322,26 @@ static int sn_subscribe_pump(SN_Subscribe* s, int* iters) return rc; } +/* Drive SN_Client_Ping until it returns something other than CONTINUE, or until + * we exceed a sane iteration cap. `ping` may be NULL to exercise the internal + * fallback. Returns the terminal code and the iteration count through *iters. */ +static int sn_ping_pump(SN_PingReq* ping, int* iters) +{ + int rc = MQTT_CODE_CONTINUE; + int i; + const int max_iters = 50; + for (i = 0; i < max_iters; i++) { + rc = SN_Client_Ping(&g_client, ping); + if (rc != MQTT_CODE_CONTINUE) { + break; + } + } + if (iters) { + *iters = i + 1; + } + return rc; +} + static void setup(void) { } static void teardown(void) { @@ -859,6 +882,142 @@ TEST(sn_subscribe_rejected_nonblock) #endif /* WOLFMQTT_NONBLOCK */ +/* ============================================================================ + * SN ping pending-response lifecycle tests (use-after-scope regression, #3132) + * + * SN_Client_Ping accepts a NULL 'ping' and falls back to a function-local + * SN_PingReq (loc_ping) on the stack. Under WOLFMQTT_MULTITHREAD it registers + * &ping->pendResp on client->firstPendResp. Under WOLFMQTT_NONBLOCK an in-flight + * MQTT_CODE_CONTINUE used to be returned verbatim - which, for the NULL + * fallback, left &loc_ping.pendResp linked on the respList after the stack frame + * unwound. A later call reused the stack address, so the stale entry could be + * dereferenced by the receive path (use-after-scope). The fix removes the entry + * before any CONTINUE return for the NULL fallback, while a caller-owned object + * (which persists across calls) still keeps its entry linked so it can resume. + * These tests pin both halves of that contract. + * ============================================================================ */ + +/* Happy path with a caller-owned ping: PINGRESP is immediately available. Runs + * in every SN build and guards that the pending response is removed once the + * ping completes. */ +TEST(sn_ping_no_continue) +{ + SN_PingReq ping; + int rc; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(0 /* no CONTINUE */)); + + mock_net_push(&g_mock, PINGRESP_FRAME, (int)sizeof(PINGRESP_FRAME)); + + XMEMSET(&ping, 0, sizeof(ping)); + + rc = sn_ping_pump(&ping, NULL); + + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + /* The client sent PINGREQ. */ + ASSERT_TRUE(g_mock.write_calls >= 1); + /* No pending response may be left dangling once the ping completes. */ + ASSERT_NO_PENDRESP(); +} + +/* Happy path with ping==NULL: exercises the internal loc_ping fallback. Runs in + * every SN build and guards that the fallback's pending response is removed once + * the ping completes (no entry leaked for a blocking single-call ping). */ +TEST(sn_ping_null_no_continue) +{ + int rc; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(0 /* no CONTINUE */)); + + mock_net_push(&g_mock, PINGRESP_FRAME, (int)sizeof(PINGRESP_FRAME)); + + rc = sn_ping_pump(NULL, NULL); + + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_TRUE(g_mock.write_calls >= 1); + ASSERT_NO_PENDRESP(); +} + +/* The non-blocking dangling-pointer regression only manifests under + * WOLFMQTT_NONBLOCK (otherwise SN_Client_WaitType blocks and never returns + * MQTT_CODE_CONTINUE, so the early-return path that leaked the entry is never + * taken). */ +#ifdef WOLFMQTT_NONBLOCK + +/* The headline #3132 regression. With a CONTINUE armed before the PINGRESP, a + * ping==NULL call returns in-flight while &loc_ping.pendResp would (pre-fix) + * still be linked - a pointer into the now-dead stack frame. Under MULTITHREAD + * this pins the dangling-pointer contract directly: after the in-flight CONTINUE + * NO pending response may remain. The retry must still converge to SUCCESS. */ +TEST(sn_ping_null_nonblock_no_dangling_pendresp) +{ + int rc, iters = 0; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(1 /* one CONTINUE per frame */)); + + mock_net_push(&g_mock, PINGRESP_FRAME, (int)sizeof(PINGRESP_FRAME)); + + /* First call sends PINGREQ and the armed CONTINUE forces an in-flight return + * before the PINGRESP is delivered. With ping==NULL the request is built on + * a stack-local SN_PingReq that does not survive this return. */ + rc = SN_Client_Ping(&g_client, NULL); + ASSERT_EQ(MQTT_CODE_CONTINUE, rc); +#ifdef WOLFMQTT_MULTITHREAD + /* Pre-fix &loc_ping.pendResp was left linked on the client respList here and + * dangled once SN_Client_Ping returned. The fix removes it before the + * CONTINUE return for the NULL fallback, so no entry may remain. */ + ASSERT_NULL(g_client.firstPendResp); +#endif + + /* A correct non-blocking caller keeps retrying until the ping resolves. */ + rc = sn_ping_pump(NULL, &iters); + + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + /* All scripted frames consumed and no dangling entry remains. */ + ASSERT_EQ(g_mock.in_count, g_mock.in_idx); + ASSERT_NO_PENDRESP(); +} + +/* The complementary half of the contract: for a caller-owned ping the pending + * response MUST stay linked across an in-flight CONTINUE (so a reader thread can + * route the PINGRESP and the wait can resume), and must point back into the + * caller's object. A "remove before every CONTINUE" change would be caught here. + */ +TEST(sn_ping_nonblock_pendresp_lifecycle) +{ + SN_PingReq ping; + int rc, iters = 0; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(1 /* one CONTINUE per frame */)); + + mock_net_push(&g_mock, PINGRESP_FRAME, (int)sizeof(PINGRESP_FRAME)); + + XMEMSET(&ping, 0, sizeof(ping)); + + /* First call sends PINGREQ; the armed CONTINUE forces an in-flight return + * before the PINGRESP is delivered. */ + rc = SN_Client_Ping(&g_client, &ping); + ASSERT_EQ(MQTT_CODE_CONTINUE, rc); +#ifdef WOLFMQTT_MULTITHREAD + /* The entry stays registered while the exchange is in flight and points back + * into the caller-owned object - the fix must NOT remove it here (only the + * stack-local NULL fallback is cleaned up on CONTINUE). */ + ASSERT_NOT_NULL(g_client.firstPendResp); + ASSERT_EQ((void*)&ping.pendResp, (void*)g_client.firstPendResp); +#endif + + /* Resume until the PINGRESP is delivered and the ping resolves. */ + rc = sn_ping_pump(&ping, &iters); + + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_EQ(g_mock.in_count, g_mock.in_idx); + /* The pending response is gone once the ping completes - no dangling entry + * remains for another thread to dereference. */ + ASSERT_NO_PENDRESP(); +} + +#endif /* WOLFMQTT_NONBLOCK */ + #endif /* WOLFMQTT_SN */ /* ============================================================================ @@ -882,6 +1041,8 @@ int main(int argc, char** argv) RUN_TEST(sn_willmsgupd_payload_scrubbed_on_write_error); RUN_TEST(sn_subscribe_no_continue); RUN_TEST(sn_subscribe_rejected); + RUN_TEST(sn_ping_no_continue); + RUN_TEST(sn_ping_null_no_continue); /* The non-blocking retry regression only exists under WOLFMQTT_NONBLOCK. */ #ifdef WOLFMQTT_NONBLOCK @@ -893,6 +1054,8 @@ int main(int argc, char** argv) RUN_TEST(sn_subscribe_nonblock_pendresp_lifecycle); RUN_TEST(sn_subscribe_many_continues); RUN_TEST(sn_subscribe_rejected_nonblock); + RUN_TEST(sn_ping_null_nonblock_no_dangling_pendresp); + RUN_TEST(sn_ping_nonblock_pendresp_lifecycle); #endif TEST_SUITE_END(); diff --git a/wolfmqtt/mqtt_sn_client.h b/wolfmqtt/mqtt_sn_client.h index a569eac62..f91094af1 100644 --- a/wolfmqtt/mqtt_sn_client.h +++ b/wolfmqtt/mqtt_sn_client.h @@ -217,6 +217,12 @@ WOLFMQTT_API int SN_Client_Disconnect_ex( * \note This is a blocking function that will wait for MqttNet.read * \param client Pointer to MqttClient structure * \param ping Pointer to SN_PingReq structure. NULL is valid. + * Under WOLFMQTT_NONBLOCK a caller that must resume + * the request across MQTT_CODE_CONTINUE returns should + * pass a persistent (caller-owned) object: a NULL ping + * falls back to internal storage that cannot carry + * state between calls, so each call is an independent, + * self-contained request rather than a resumed one. * \return MQTT_CODE_SUCCESS or MQTT_CODE_ERROR_* (see enum MqttPacketResponseCodes) */ From a0ed6ea0da6d269a00a420166141d8524da1c32e Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Mon, 8 Jun 2026 17:02:26 -0500 Subject: [PATCH 09/23] Fix f-4518 SN_Client_HandlePacket type issue in debug --- src/mqtt_sn_client.c | 3 ++- tests/test_mqtt_sn.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/mqtt_sn_client.c b/src/mqtt_sn_client.c index 225a51427..005fdc39c 100644 --- a/src/mqtt_sn_client.c +++ b/src/mqtt_sn_client.c @@ -287,7 +287,8 @@ static int SN_Client_HandlePacket(MqttClient* client, SN_MsgType packet_type, client->tx_buf_len, resp_type, p_publish_resp); #ifdef WOLFMQTT_DEBUG_CLIENT PRINTF("MqttClient_EncodePacket: Len %d, Type %s (%d), ID %d", - rc, MqttPacket_TypeDesc(resp_type), resp_type, packet_id); + rc, SN_Packet_TypeDesc((SN_MsgType)resp_type), resp_type, + packet_id); #endif if (rc <= 0) { #ifdef WOLFMQTT_MULTITHREAD diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c index 0bdbf5725..fe55f63d7 100644 --- a/tests/test_mqtt_sn.c +++ b/tests/test_mqtt_sn.c @@ -411,6 +411,41 @@ TEST(sn_connack_null_buf_rejected) ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); } +/* ============================================================================ + * SN_Packet_TypeDesc + * ============================================================================ */ + +/* The QoS 2 PUBREC/PUBREL branch of SN_Client_HandlePacket logs the response + * message type (SN_MSG_TYPE_PUBREL or SN_MSG_TYPE_PUBCOMP) for its debug trace. + * MQTT-SN message-type values do not align with the standard MqttPacketType + * values, so describing them with MqttPacket_TypeDesc mis-renders PUBCOMP + * (0x0E) as "Disconnect" and PUBREL (0x10) as "Unknown". These tests pin the + * correct SN descriptions, including the two values that branch can produce. */ +#ifndef WOLFMQTT_NO_ERROR_STRINGS +TEST(sn_typedesc_pubcomp) +{ + /* 0x0E: would be "Disconnect" if described as an MqttPacketType. */ + ASSERT_STR_EQ("Publish complete", SN_Packet_TypeDesc(SN_MSG_TYPE_PUBCOMP)); +} + +TEST(sn_typedesc_pubrel) +{ + /* 0x10: outside the MqttPacketType range, would be "Unknown". */ + ASSERT_STR_EQ("Publish Release", SN_Packet_TypeDesc(SN_MSG_TYPE_PUBREL)); +} + +TEST(sn_typedesc_pubrec) +{ + ASSERT_STR_EQ("Publish Received", SN_Packet_TypeDesc(SN_MSG_TYPE_PUBREC)); +} + +TEST(sn_typedesc_unknown_type) +{ + /* 0x11 is reserved and unhandled, so it falls through to the default. */ + ASSERT_STR_EQ("Unknown", SN_Packet_TypeDesc((SN_MsgType)0x11)); +} +#endif /* !WOLFMQTT_NO_ERROR_STRINGS */ + /* ============================================================================ * Suite runner * ============================================================================ */ @@ -457,6 +492,14 @@ int main(int argc, char** argv) RUN_TEST(sn_connack_wrong_type_rejected); RUN_TEST(sn_connack_null_buf_rejected); + /* SN_Packet_TypeDesc */ +#ifndef WOLFMQTT_NO_ERROR_STRINGS + RUN_TEST(sn_typedesc_pubcomp); + RUN_TEST(sn_typedesc_pubrel); + RUN_TEST(sn_typedesc_pubrec); + RUN_TEST(sn_typedesc_unknown_type); +#endif + TEST_SUITE_END(); TEST_RUNNER_END(); From 820e5230ae543e2ff4d32e37c9a5f29992fdf597 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 9 Jun 2026 07:38:49 -0500 Subject: [PATCH 10/23] Fix f-5510: MQTT-SN publish encoder harden --- src/mqtt_sn_packet.c | 24 ++++- tests/test_mqtt_sn.c | 226 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 245 insertions(+), 5 deletions(-) diff --git a/src/mqtt_sn_packet.c b/src/mqtt_sn_packet.c index 6d59edd4e..fc112f907 100644 --- a/src/mqtt_sn_packet.c +++ b/src/mqtt_sn_packet.c @@ -1138,7 +1138,7 @@ int SN_Decode_SubscribeAck(byte* rx_buf, int rx_buf_len, int SN_Encode_Publish(byte *tx_buf, int tx_buf_len, SN_Publish *publish) { - int total_len; + word32 total_len; byte *tx_payload = tx_buf; byte flags = 0; @@ -1146,9 +1146,23 @@ int SN_Encode_Publish(byte *tx_buf, int tx_buf_len, SN_Publish *publish) if (tx_buf == NULL || publish == NULL) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } + if (tx_buf_len < 0) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); + } + + /* publish->total_len is a caller-supplied word32. Reject any payload large + * enough that adding the MQTT-SN header overhead would overflow or exceed + * the maximum packet length, before the length arithmetic below. The header + * is at most 9 bytes: a 3-byte extended length field, msgType, flags, topic + * ID (2), and msgID (2). Validating the word32 up front keeps the payload + * copy bounded and prevents the narrowing wrap that could bypass the + * SN_PACKET_MAX_LEN / tx_buf_len checks. */ + if (publish->total_len > (word32)(SN_PACKET_MAX_LEN - 9)) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); + } /* Determine packet length */ - total_len = (int)publish->total_len; + total_len = publish->total_len; /* Add length, msgType, flags, topic ID (2), and msgID (2) */ total_len += 7; @@ -1158,7 +1172,7 @@ int SN_Encode_Publish(byte *tx_buf, int tx_buf_len, SN_Publish *publish) total_len += 2; } - if (total_len > SN_PACKET_MAX_LEN || total_len > tx_buf_len) { + if (total_len > SN_PACKET_MAX_LEN || total_len > (word32)tx_buf_len) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER); } @@ -1168,7 +1182,7 @@ int SN_Encode_Publish(byte *tx_buf, int tx_buf_len, SN_Publish *publish) } else { *tx_payload++ = SN_PACKET_LEN_IND; - tx_payload += MqttEncode_Num(tx_payload, total_len); + tx_payload += MqttEncode_Num(tx_payload, (word16)total_len); } *tx_payload++ = SN_MSG_TYPE_PUBLISH; @@ -1213,7 +1227,7 @@ int SN_Encode_Publish(byte *tx_buf, int tx_buf_len, SN_Publish *publish) (void)tx_payload; /* Return length of packet placed into tx_buf */ - return total_len; + return (int)total_len; } int SN_Decode_Publish(byte *rx_buf, int rx_buf_len, SN_Publish *publish) diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c index fe55f63d7..6c69a6675 100644 --- a/tests/test_mqtt_sn.c +++ b/tests/test_mqtt_sn.c @@ -446,6 +446,221 @@ TEST(sn_typedesc_unknown_type) } #endif /* !WOLFMQTT_NO_ERROR_STRINGS */ +/* ============================================================================ + * SN_Encode_Publish + * + * publish->total_len is a caller-supplied word32 payload length. The encoder + * must keep its length arithmetic in an unsigned wide type and reject oversized + * lengths before they can wrap negative/small and slip past the + * SN_PACKET_MAX_LEN / tx_buf_len bounds checks (which previously preceded a + * XMEMCPY of the full word32 length - report 5510). + * ============================================================================ */ + +TEST(sn_encode_publish_short_topic_valid) +{ + /* QoS1, SHORT topic "tp", packet id 0x1234, payload "hi" -> 9-byte packet: + * [len=9][PUBLISH][flags][t][p][id hi][id lo][h][i] */ + byte tx_buf[32]; + char topic[2] = { 't', 'p' }; + byte payload[2] = { 'h', 'i' }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + publish.qos = MQTT_QOS_1; + publish.topic_type = SN_TOPIC_ID_TYPE_SHORT; + publish.topic_name = topic; + publish.packet_id = 0x1234; + publish.buffer = payload; + publish.total_len = sizeof(payload); + + rc = SN_Encode_Publish(tx_buf, (int)sizeof(tx_buf), &publish); + ASSERT_EQ(9, rc); + ASSERT_EQ(9, tx_buf[0]); + ASSERT_EQ(SN_MSG_TYPE_PUBLISH, tx_buf[1]); + /* flags = (QOS_MASK & (1<<5)) | (TOPICIDTYPE_MASK & SHORT) = 0x20 | 0x02 */ + ASSERT_EQ(0x22, tx_buf[2]); + ASSERT_EQ('t', tx_buf[3]); + ASSERT_EQ('p', tx_buf[4]); + ASSERT_EQ(0x12, tx_buf[5]); + ASSERT_EQ(0x34, tx_buf[6]); + ASSERT_EQ('h', tx_buf[7]); + ASSERT_EQ('i', tx_buf[8]); +} + +TEST(sn_encode_publish_ind_form_valid) +{ + /* A 300-byte payload pushes the packet past SN_PACKET_MAX_SMALL_SIZE, so the + * length is encoded in the 3-byte extended (IND) form. Total = 300 + 9. */ + static byte tx_buf[512]; + static byte payload[300]; + char topic[2] = { 't', 'p' }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + publish.topic_type = SN_TOPIC_ID_TYPE_SHORT; + publish.topic_name = topic; + publish.packet_id = 0x1234; + publish.buffer = payload; + publish.total_len = sizeof(payload); + + rc = SN_Encode_Publish(tx_buf, (int)sizeof(tx_buf), &publish); + ASSERT_EQ(309, rc); + ASSERT_EQ(SN_PACKET_LEN_IND, tx_buf[0]); + /* 309 = 0x0135, big-endian */ + ASSERT_EQ(0x01, tx_buf[1]); + ASSERT_EQ(0x35, tx_buf[2]); + ASSERT_EQ(SN_MSG_TYPE_PUBLISH, tx_buf[3]); +} + +TEST(sn_encode_publish_oversized_total_len_rejected) +{ + /* Regression for report 5510: total_len = 0xFFFFFFFF. The old code cast this + * to int (-1), added 7 (=6), passed both bounds checks, then XMEMCPY'd + * 0xFFFFFFFF bytes. A valid topic and buffer are supplied so the pre-fix + * path would have reached that copy; the fix must reject up front. */ + byte tx_buf[32]; + char topic[2] = { 't', 'p' }; + byte payload[2] = { 'h', 'i' }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + publish.topic_type = SN_TOPIC_ID_TYPE_SHORT; + publish.topic_name = topic; + publish.packet_id = 0x1234; + publish.buffer = payload; + publish.total_len = 0xFFFFFFFFU; + + rc = SN_Encode_Publish(tx_buf, (int)sizeof(tx_buf), &publish); + ASSERT_EQ(MQTT_CODE_ERROR_OUT_OF_BUFFER, rc); +} + +TEST(sn_encode_publish_total_len_int_max_rejected) +{ + /* total_len = INT_MAX (0x7FFFFFFF): adding the 7-byte header overflows + * signed int (wraps negative under -fwrapv), which the old code let slip + * past the bounds checks. Must be rejected. */ + byte tx_buf[32]; + char topic[2] = { 't', 'p' }; + byte payload[2] = { 'h', 'i' }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + publish.topic_type = SN_TOPIC_ID_TYPE_SHORT; + publish.topic_name = topic; + publish.packet_id = 0x1234; + publish.buffer = payload; + publish.total_len = 0x7FFFFFFFU; + + rc = SN_Encode_Publish(tx_buf, (int)sizeof(tx_buf), &publish); + ASSERT_EQ(MQTT_CODE_ERROR_OUT_OF_BUFFER, rc); +} + +TEST(sn_encode_publish_total_len_over_max_rejected) +{ + /* One byte past the largest encodable payload (SN_PACKET_MAX_LEN - 9). The + * buffer length is not the limiting factor here - the payload itself cannot + * fit in a valid MQTT-SN packet. */ + static byte tx_buf[SN_PACKET_MAX_LEN]; + char topic[2] = { 't', 'p' }; + byte payload[2] = { 'h', 'i' }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + publish.topic_type = SN_TOPIC_ID_TYPE_SHORT; + publish.topic_name = topic; + publish.packet_id = 0x1234; + publish.buffer = payload; + publish.total_len = (word32)(SN_PACKET_MAX_LEN - 9) + 1; + + rc = SN_Encode_Publish(tx_buf, (int)sizeof(tx_buf), &publish); + ASSERT_EQ(MQTT_CODE_ERROR_OUT_OF_BUFFER, rc); +} + +TEST(sn_encode_publish_max_payload_accepted) +{ + /* The largest payload that still fits a valid packet: SN_PACKET_MAX_LEN - 9 + * encodes to exactly SN_PACKET_MAX_LEN bytes (extended length form). Guards + * the bounds check against being off-by-one and rejecting a valid maximum. */ + static byte tx_buf[SN_PACKET_MAX_LEN]; + static byte payload[SN_PACKET_MAX_LEN - 9]; + char topic[2] = { 't', 'p' }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + publish.topic_type = SN_TOPIC_ID_TYPE_SHORT; + publish.topic_name = topic; + publish.packet_id = 0x1234; + publish.buffer = payload; + publish.total_len = (word32)sizeof(payload); + + rc = SN_Encode_Publish(tx_buf, (int)sizeof(tx_buf), &publish); + ASSERT_EQ(SN_PACKET_MAX_LEN, rc); + ASSERT_EQ(SN_PACKET_LEN_IND, tx_buf[0]); +} + +TEST(sn_encode_publish_buffer_too_small_rejected) +{ + /* total_len is encodable, but tx_buf cannot hold the resulting packet. The + * tx_buf_len bounds check (now unsigned) must still reject it. */ + byte tx_buf[8]; /* packet would need 9 bytes */ + char topic[2] = { 't', 'p' }; + byte payload[2] = { 'h', 'i' }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + publish.topic_type = SN_TOPIC_ID_TYPE_SHORT; + publish.topic_name = topic; + publish.packet_id = 0x1234; + publish.buffer = payload; + publish.total_len = sizeof(payload); + + rc = SN_Encode_Publish(tx_buf, (int)sizeof(tx_buf), &publish); + ASSERT_EQ(MQTT_CODE_ERROR_OUT_OF_BUFFER, rc); +} + +TEST(sn_encode_publish_negative_buf_len_rejected) +{ + /* A negative tx_buf_len must not be sign-converted into a huge unsigned + * limit that lets the bounds check pass. */ + byte tx_buf[32]; + char topic[2] = { 't', 'p' }; + byte payload[2] = { 'h', 'i' }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + publish.topic_type = SN_TOPIC_ID_TYPE_SHORT; + publish.topic_name = topic; + publish.packet_id = 0x1234; + publish.buffer = payload; + publish.total_len = sizeof(payload); + + rc = SN_Encode_Publish(tx_buf, -1, &publish); + ASSERT_EQ(MQTT_CODE_ERROR_OUT_OF_BUFFER, rc); +} + +TEST(sn_encode_publish_null_args_rejected) +{ + byte tx_buf[32]; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + + rc = SN_Encode_Publish(NULL, (int)sizeof(tx_buf), &publish); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); + + rc = SN_Encode_Publish(tx_buf, (int)sizeof(tx_buf), NULL); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + /* ============================================================================ * Suite runner * ============================================================================ */ @@ -492,6 +707,17 @@ int main(int argc, char** argv) RUN_TEST(sn_connack_wrong_type_rejected); RUN_TEST(sn_connack_null_buf_rejected); + /* SN_Encode_Publish */ + RUN_TEST(sn_encode_publish_short_topic_valid); + RUN_TEST(sn_encode_publish_ind_form_valid); + RUN_TEST(sn_encode_publish_oversized_total_len_rejected); + RUN_TEST(sn_encode_publish_total_len_int_max_rejected); + RUN_TEST(sn_encode_publish_total_len_over_max_rejected); + RUN_TEST(sn_encode_publish_max_payload_accepted); + RUN_TEST(sn_encode_publish_buffer_too_small_rejected); + RUN_TEST(sn_encode_publish_negative_buf_len_rejected); + RUN_TEST(sn_encode_publish_null_args_rejected); + /* SN_Packet_TypeDesc */ #ifndef WOLFMQTT_NO_ERROR_STRINGS RUN_TEST(sn_typedesc_pubcomp); From c9530641687573f8b0db9e193003aea6a7cce8aa Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 9 Jun 2026 07:46:17 -0500 Subject: [PATCH 11/23] Fix f-4659: topic_type check in SN_Decode_Publish --- src/mqtt_sn_packet.c | 8 ++++ tests/test_mqtt_sn.c | 97 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+) diff --git a/src/mqtt_sn_packet.c b/src/mqtt_sn_packet.c index fc112f907..2475424b0 100644 --- a/src/mqtt_sn_packet.c +++ b/src/mqtt_sn_packet.c @@ -1291,6 +1291,14 @@ int SN_Decode_Publish(byte *rx_buf, int rx_buf_len, SN_Publish *publish) publish->topic_type = flags & SN_PACKET_FLAG_TOPICIDTYPE_MASK; + /* Reject the reserved topic id type (0b11). MQTT-SN v1.2 defines only + * NORMAL (0), PREDEF (1) and SHORT (2); value 3 must not appear on the + * wire. Without this guard a spoofed gateway could hand topic_type=3 to + * the application callback, which mis-classifies the message. */ + if (publish->topic_type > SN_TOPIC_ID_TYPE_SHORT) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + } + /* Decode payload: use pointer difference to account for both short (7) * and extended-length (9) header formats */ if (total_len < (word16)(rx_payload - rx_buf)) { diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c index 6c69a6675..24d085779 100644 --- a/tests/test_mqtt_sn.c +++ b/tests/test_mqtt_sn.c @@ -661,6 +661,96 @@ TEST(sn_encode_publish_null_args_rejected) ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); } +/* ============================================================================ + * SN_Decode_Publish + * + * The PUBLISH flags byte carries the topic id type in bits[1:0]. MQTT-SN v1.2 + * defines only NORMAL (0), PREDEF (1) and SHORT (2); value 3 (0b11) is reserved + * and must not appear on the wire. The decoder must reject it rather than + * fail-open and hand topic_type=3 to the application callback, which would + * mis-classify the message (report 4659). These tests pin the rejection of the + * reserved value and confirm the three defined values still decode (boundary at + * SHORT=2). + * + * Frame layout (short form, 7-byte header): + * [len][PUBLISH][flags][topic hi][topic lo][id hi][id lo] payload... + * ============================================================================ */ + +TEST(sn_decode_publish_reserved_topic_type_rejected) +{ + /* Report 4659 PoC: flags=0x03 sets the reserved topic id type 0b11. The + * pre-fix decoder returned 7 (success) with topic_type=3. */ + byte buf[7] = { 0x07, 0x0C, 0x03, 0x00, 0x01, 0x00, 0x00 }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + rc = SN_Decode_Publish(buf, (int)sizeof(buf), &publish); + ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); +} + +TEST(sn_decode_publish_normal_topic_type_valid) +{ + /* topic_type bits = 0b00 (NORMAL): must still decode. */ + byte buf[7] = { 0x07, 0x0C, 0x00, 0x00, 0x01, 0x00, 0x00 }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + rc = SN_Decode_Publish(buf, (int)sizeof(buf), &publish); + ASSERT_EQ(7, rc); + ASSERT_EQ(SN_TOPIC_ID_TYPE_NORMAL, publish.topic_type); +} + +TEST(sn_decode_publish_predef_topic_type_valid) +{ + /* topic_type bits = 0b01 (PREDEF): must still decode. */ + byte buf[7] = { 0x07, 0x0C, 0x01, 0x00, 0x01, 0x00, 0x00 }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + rc = SN_Decode_Publish(buf, (int)sizeof(buf), &publish); + ASSERT_EQ(7, rc); + ASSERT_EQ(SN_TOPIC_ID_TYPE_PREDEF, publish.topic_type); +} + +TEST(sn_decode_publish_short_topic_type_valid) +{ + /* topic_type bits = 0b10 (SHORT): the boundary value the guard must accept. + * Full frame with QoS1, topic "tp", packet id 0x1234 and payload "hi"; pins + * the other decoded flag fields too. */ + byte buf[9] = { 0x09, 0x0C, 0x22, 't', 'p', 0x12, 0x34, 'h', 'i' }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + rc = SN_Decode_Publish(buf, (int)sizeof(buf), &publish); + ASSERT_EQ(9, rc); + /* flags = 0x22 -> QoS1, no retain/dup, SHORT topic type */ + ASSERT_EQ(SN_TOPIC_ID_TYPE_SHORT, publish.topic_type); + ASSERT_EQ(MQTT_QOS_1, publish.qos); + ASSERT_EQ(0, publish.retain); + ASSERT_EQ(0, publish.duplicate); + ASSERT_EQ(0x1234, publish.packet_id); + ASSERT_EQ(2, publish.total_len); +} + +TEST(sn_decode_publish_null_args_rejected) +{ + byte buf[7] = { 0x07, 0x0C, 0x00, 0x00, 0x01, 0x00, 0x00 }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + + rc = SN_Decode_Publish(NULL, (int)sizeof(buf), &publish); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); + + rc = SN_Decode_Publish(buf, (int)sizeof(buf), NULL); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + /* ============================================================================ * Suite runner * ============================================================================ */ @@ -718,6 +808,13 @@ int main(int argc, char** argv) RUN_TEST(sn_encode_publish_negative_buf_len_rejected); RUN_TEST(sn_encode_publish_null_args_rejected); + /* SN_Decode_Publish */ + RUN_TEST(sn_decode_publish_reserved_topic_type_rejected); + RUN_TEST(sn_decode_publish_normal_topic_type_valid); + RUN_TEST(sn_decode_publish_predef_topic_type_valid); + RUN_TEST(sn_decode_publish_short_topic_type_valid); + RUN_TEST(sn_decode_publish_null_args_rejected); + /* SN_Packet_TypeDesc */ #ifndef WOLFMQTT_NO_ERROR_STRINGS RUN_TEST(sn_typedesc_pubcomp); From 80d61da78076be05a8f3b2f20c84aaedc07d2eb3 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 9 Jun 2026 08:04:23 -0500 Subject: [PATCH 12/23] Fix f-4248: SN_Decode_Publish validate packet_id with QoS>0 --- src/mqtt_sn_packet.c | 13 +++++++ tests/test_mqtt_sn.c | 89 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+) diff --git a/src/mqtt_sn_packet.c b/src/mqtt_sn_packet.c index 2475424b0..687a1c283 100644 --- a/src/mqtt_sn_packet.c +++ b/src/mqtt_sn_packet.c @@ -1287,6 +1287,19 @@ int SN_Decode_Publish(byte *rx_buf, int rx_buf_len, SN_Publish *publish) publish->qos = (MqttQoS)((flags & SN_PACKET_FLAG_QOS_MASK) >> SN_PACKET_FLAG_QOS_SHIFT); + /* MQTT-SN v1.2 §5.2.10: a QoS 1 or QoS 2 PUBLISH must carry a non-zero + * MsgId so the matching PUBACK/PUBREC can be correlated. Reject MsgId=0 + * here; otherwise SN_Client_HandlePacket would emit a response carrying + * MsgId=0 that no conformant gateway can match, leaving its retransmit + * timer to replay the same message (CWE-20). Mirrors the standard MQTT + * decoder guard in mqtt_packet.c. QoS 0 and QoS -1 (MQTT_QOS_3, the + * connectionless publish) send no response and legitimately use MsgId=0, + * so they are intentionally excluded. */ + if ((publish->qos == MQTT_QOS_1 || publish->qos == MQTT_QOS_2) && + publish->packet_id == 0) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PACKET_ID); + } + publish->retain = flags & SN_PACKET_FLAG_RETAIN; publish->topic_type = flags & SN_PACKET_FLAG_TOPICIDTYPE_MASK; diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c index 24d085779..92872d185 100644 --- a/tests/test_mqtt_sn.c +++ b/tests/test_mqtt_sn.c @@ -751,6 +751,90 @@ TEST(sn_decode_publish_null_args_rejected) ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); } +/* ---------------------------------------------------------------------------- + * MsgId=0 guard for QoS > 0 (report 4248) + * + * MQTT-SN v1.2 §5.2.10 requires a QoS 1 or QoS 2 PUBLISH to carry a non-zero + * MsgId. The pre-fix decoder returned success with packet_id=0, so + * SN_Client_HandlePacket emitted a PUBACK/PUBREC carrying MsgId=0 that no + * conformant gateway can correlate; the gateway then retransmits the same + * PUBLISH, replaying the message to msg_cb. The guard rejects MsgId=0 for + * QoS > 0 while still accepting MsgId=0 for QoS 0 (where no response is sent). + * + * Frame layout (short form, 7-byte header): + * [len][PUBLISH][flags][topic hi][topic lo][id hi][id lo] payload... + * flags QoS bits = bits[6:5]: 0x20 -> QoS1, 0x40 -> QoS2. + * -------------------------------------------------------------------------- */ + +TEST(sn_decode_publish_qos1_zero_packet_id_rejected) +{ + /* Report 4248 PoC: flags=0x20 -> QoS1, MsgId=0x0000. */ + byte buf[7] = { 0x07, 0x0C, 0x20, 0x00, 0x01, 0x00, 0x00 }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + rc = SN_Decode_Publish(buf, (int)sizeof(buf), &publish); + ASSERT_EQ(MQTT_CODE_ERROR_PACKET_ID, rc); +} + +TEST(sn_decode_publish_qos2_zero_packet_id_rejected) +{ + /* flags=0x40 -> QoS2, MsgId=0x0000. */ + byte buf[7] = { 0x07, 0x0C, 0x40, 0x00, 0x01, 0x00, 0x00 }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + rc = SN_Decode_Publish(buf, (int)sizeof(buf), &publish); + ASSERT_EQ(MQTT_CODE_ERROR_PACKET_ID, rc); +} + +TEST(sn_decode_publish_qos0_zero_packet_id_valid) +{ + /* QoS0 (flags=0x00) carries no MsgId and sends no response, so MsgId=0 + * is legal and must still decode. */ + byte buf[7] = { 0x07, 0x0C, 0x00, 0x00, 0x01, 0x00, 0x00 }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + rc = SN_Decode_Publish(buf, (int)sizeof(buf), &publish); + ASSERT_EQ(7, rc); + ASSERT_EQ(MQTT_QOS_0, publish.qos); + ASSERT_EQ(0, publish.packet_id); +} + +TEST(sn_decode_publish_qos1_nonzero_packet_id_valid) +{ + /* flags=0x20 -> QoS1 with a valid non-zero MsgId 0x1234 must decode. */ + byte buf[7] = { 0x07, 0x0C, 0x20, 0x00, 0x01, 0x12, 0x34 }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + rc = SN_Decode_Publish(buf, (int)sizeof(buf), &publish); + ASSERT_EQ(7, rc); + ASSERT_EQ(MQTT_QOS_1, publish.qos); + ASSERT_EQ(0x1234, publish.packet_id); +} + +TEST(sn_decode_publish_qosneg1_zero_packet_id_valid) +{ + /* flags=0x60 -> QoS bits 0b11 = MQTT_QOS_3, MQTT-SN's QoS -1 + * (connectionless publish). It sends no PUBACK/PUBREC and uses MsgId=0, + * so the guard must NOT reject it. */ + byte buf[7] = { 0x07, 0x0C, 0x60, 0x00, 0x01, 0x00, 0x00 }; + SN_Publish publish; + int rc; + + XMEMSET(&publish, 0, sizeof(publish)); + rc = SN_Decode_Publish(buf, (int)sizeof(buf), &publish); + ASSERT_EQ(7, rc); + ASSERT_EQ(MQTT_QOS_3, publish.qos); + ASSERT_EQ(0, publish.packet_id); +} + /* ============================================================================ * Suite runner * ============================================================================ */ @@ -814,6 +898,11 @@ int main(int argc, char** argv) RUN_TEST(sn_decode_publish_predef_topic_type_valid); RUN_TEST(sn_decode_publish_short_topic_type_valid); RUN_TEST(sn_decode_publish_null_args_rejected); + RUN_TEST(sn_decode_publish_qos1_zero_packet_id_rejected); + RUN_TEST(sn_decode_publish_qos2_zero_packet_id_rejected); + RUN_TEST(sn_decode_publish_qos0_zero_packet_id_valid); + RUN_TEST(sn_decode_publish_qos1_nonzero_packet_id_valid); + RUN_TEST(sn_decode_publish_qosneg1_zero_packet_id_valid); /* SN_Packet_TypeDesc */ #ifndef WOLFMQTT_NO_ERROR_STRINGS From c63f381c2aea2105bbed576884b014d66f929e1e Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 9 Jun 2026 08:16:32 -0500 Subject: [PATCH 13/23] Fix f-2332: SN_Encode_Subscribe/Unsubscribe check topicNameId --- src/mqtt_sn_packet.c | 18 +++- tests/test_mqtt_sn.c | 231 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 245 insertions(+), 4 deletions(-) diff --git a/src/mqtt_sn_packet.c b/src/mqtt_sn_packet.c index 687a1c283..3bb1341ad 100644 --- a/src/mqtt_sn_packet.c +++ b/src/mqtt_sn_packet.c @@ -1018,8 +1018,13 @@ int SN_Encode_Subscribe(byte *tx_buf, int tx_buf_len, SN_Subscribe *subscribe) int total_len; byte *tx_payload, flags = 0x00; - /* Validate required arguments */ - if (tx_buf == NULL || subscribe == NULL) { + /* Validate required arguments. topicNameId is dereferenced for every + * topic_type below (XSTRLEN on the NORMAL path, a 2-byte XMEMCPY + * otherwise), so reject NULL up front. topic_type is SN_TOPIC_ID_TYPE_NORMAL + * (0x0) when the SN_Subscribe is zero-initialized, so a caller that forgets + * to assign topicNameId would otherwise crash in XSTRLEN(NULL). */ + if (tx_buf == NULL || subscribe == NULL || + subscribe->topicNameId == NULL) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } @@ -1421,8 +1426,13 @@ int SN_Encode_Unsubscribe(byte *tx_buf, int tx_buf_len, int total_len; byte *tx_payload, flags = 0x00; - /* Validate required arguments */ - if (tx_buf == NULL || unsubscribe == NULL) { + /* Validate required arguments. topicNameId is dereferenced for every + * topic_type below (XSTRLEN on the NORMAL path, a 2-byte XMEMCPY + * otherwise), so reject NULL up front. topic_type is SN_TOPIC_ID_TYPE_NORMAL + * (0x0) when the SN_Unsubscribe is zero-initialized, so a caller that forgets + * to assign topicNameId would otherwise crash in XSTRLEN(NULL). */ + if (tx_buf == NULL || unsubscribe == NULL || + unsubscribe->topicNameId == NULL) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c index 92872d185..6f859201c 100644 --- a/tests/test_mqtt_sn.c +++ b/tests/test_mqtt_sn.c @@ -835,6 +835,223 @@ TEST(sn_decode_publish_qosneg1_zero_packet_id_valid) ASSERT_EQ(0, publish.packet_id); } +/* ============================================================================ + * SN_Encode_Subscribe + * + * Identical topicNameId footgun to SN_Encode_Unsubscribe (the "seed" of report + * 2332): topicNameId is dereferenced for every topic_type - XSTRLEN on the + * NORMAL (string) path, a 2-byte XMEMCPY otherwise - and SN_TOPIC_ID_TYPE_NORMAL + * (0x0) is the zero-initialized default, so a caller that forgets to set + * topicNameId would crash in XSTRLEN(NULL). The encoder must reject a NULL + * topicNameId with BAD_ARG instead. + * ============================================================================ */ + +TEST(sn_encode_subscribe_null_topic_normal_rejected) +{ + /* Zero-initialized SN_Subscribe: topic_type == SN_TOPIC_ID_TYPE_NORMAL (0x0) + * and topicNameId == NULL. The pre-fix code called XSTRLEN(NULL) and + * crashed; it must now return BAD_ARG. */ + byte tx_buf[32]; + SN_Subscribe subscribe; + int rc; + + XMEMSET(&subscribe, 0, sizeof(subscribe)); + ASSERT_EQ(SN_TOPIC_ID_TYPE_NORMAL, subscribe.topic_type); + + rc = SN_Encode_Subscribe(tx_buf, (int)sizeof(tx_buf), &subscribe); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_encode_subscribe_null_topic_short_rejected) +{ + /* The non-NORMAL path also dereferences topicNameId (a 2-byte XMEMCPY), so a + * NULL topicNameId must be rejected there too rather than copying from NULL. */ + byte tx_buf[32]; + SN_Subscribe subscribe; + int rc; + + XMEMSET(&subscribe, 0, sizeof(subscribe)); + subscribe.topic_type = SN_TOPIC_ID_TYPE_SHORT; + subscribe.topicNameId = NULL; + + rc = SN_Encode_Subscribe(tx_buf, (int)sizeof(tx_buf), &subscribe); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_encode_subscribe_null_args_rejected) +{ + byte tx_buf[32]; + SN_Subscribe subscribe; + int rc; + + XMEMSET(&subscribe, 0, sizeof(subscribe)); + subscribe.topicNameId = "abc"; + + rc = SN_Encode_Subscribe(NULL, (int)sizeof(tx_buf), &subscribe); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); + + rc = SN_Encode_Subscribe(tx_buf, (int)sizeof(tx_buf), NULL); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_encode_subscribe_normal_topic_valid) +{ + /* NORMAL topic "abc", QoS0, msgid 0x1234 -> 8-byte packet: + * [len=8][SUBSCRIBE][flags=0x00][id hi][id lo][a][b][c] */ + byte tx_buf[32]; + SN_Subscribe subscribe; + int rc; + + XMEMSET(&subscribe, 0, sizeof(subscribe)); + subscribe.topic_type = SN_TOPIC_ID_TYPE_NORMAL; + subscribe.topicNameId = "abc"; + subscribe.packet_id = 0x1234; + + rc = SN_Encode_Subscribe(tx_buf, (int)sizeof(tx_buf), &subscribe); + ASSERT_EQ(8, rc); + ASSERT_EQ(8, tx_buf[0]); + ASSERT_EQ(SN_MSG_TYPE_SUBSCRIBE, tx_buf[1]); + ASSERT_EQ(0x00, tx_buf[2]); + ASSERT_EQ(0x12, tx_buf[3]); + ASSERT_EQ(0x34, tx_buf[4]); + ASSERT_EQ('a', tx_buf[5]); + ASSERT_EQ('b', tx_buf[6]); + ASSERT_EQ('c', tx_buf[7]); +} + +TEST(sn_encode_subscribe_short_topic_valid) +{ + /* SHORT topic "tp" (2 chars, no XSTRLEN), QoS0, msgid 0x1234 -> 7-byte + * packet: [len=7][SUBSCRIBE][flags=SHORT(0x02)][id hi][id lo][t][p] */ + byte tx_buf[32]; + char topic[2] = { 't', 'p' }; + SN_Subscribe subscribe; + int rc; + + XMEMSET(&subscribe, 0, sizeof(subscribe)); + subscribe.topic_type = SN_TOPIC_ID_TYPE_SHORT; + subscribe.topicNameId = topic; + subscribe.packet_id = 0x1234; + + rc = SN_Encode_Subscribe(tx_buf, (int)sizeof(tx_buf), &subscribe); + ASSERT_EQ(7, rc); + ASSERT_EQ(7, tx_buf[0]); + ASSERT_EQ(SN_MSG_TYPE_SUBSCRIBE, tx_buf[1]); + ASSERT_EQ(0x02, tx_buf[2]); + ASSERT_EQ(0x12, tx_buf[3]); + ASSERT_EQ(0x34, tx_buf[4]); + ASSERT_EQ('t', tx_buf[5]); + ASSERT_EQ('p', tx_buf[6]); +} + +/* ============================================================================ + * SN_Encode_Unsubscribe + * + * topicNameId is dereferenced for every topic_type: XSTRLEN on the NORMAL + * (string) path, a 2-byte XMEMCPY otherwise. SN_TOPIC_ID_TYPE_NORMAL is 0x0, + * which is also the zero-initialized default, so a caller that forgets to set + * topicNameId would crash in XSTRLEN(NULL). The encoder must reject a NULL + * topicNameId with BAD_ARG instead - report 2332. + * ============================================================================ */ + +TEST(sn_encode_unsubscribe_null_topic_normal_rejected) +{ + /* Regression for report 2332: a zero-initialized SN_Unsubscribe has + * topic_type == SN_TOPIC_ID_TYPE_NORMAL (0x0) and topicNameId == NULL. The + * pre-fix code called XSTRLEN(NULL) and crashed; it must now return BAD_ARG. */ + byte tx_buf[32]; + SN_Unsubscribe unsubscribe; + int rc; + + XMEMSET(&unsubscribe, 0, sizeof(unsubscribe)); + ASSERT_EQ(SN_TOPIC_ID_TYPE_NORMAL, unsubscribe.topic_type); + + rc = SN_Encode_Unsubscribe(tx_buf, (int)sizeof(tx_buf), &unsubscribe); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_encode_unsubscribe_null_topic_short_rejected) +{ + /* The non-NORMAL path also dereferences topicNameId (a 2-byte XMEMCPY), so a + * NULL topicNameId must be rejected there too rather than copying from NULL. */ + byte tx_buf[32]; + SN_Unsubscribe unsubscribe; + int rc; + + XMEMSET(&unsubscribe, 0, sizeof(unsubscribe)); + unsubscribe.topic_type = SN_TOPIC_ID_TYPE_SHORT; + unsubscribe.topicNameId = NULL; + + rc = SN_Encode_Unsubscribe(tx_buf, (int)sizeof(tx_buf), &unsubscribe); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_encode_unsubscribe_null_args_rejected) +{ + byte tx_buf[32]; + SN_Unsubscribe unsubscribe; + int rc; + + XMEMSET(&unsubscribe, 0, sizeof(unsubscribe)); + unsubscribe.topicNameId = "abc"; + + rc = SN_Encode_Unsubscribe(NULL, (int)sizeof(tx_buf), &unsubscribe); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); + + rc = SN_Encode_Unsubscribe(tx_buf, (int)sizeof(tx_buf), NULL); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_encode_unsubscribe_normal_topic_valid) +{ + /* NORMAL topic "abc", QoS0, msgid 0x1234 -> 8-byte packet: + * [len=8][UNSUBSCRIBE][flags=0x00][id hi][id lo][a][b][c] */ + byte tx_buf[32]; + SN_Unsubscribe unsubscribe; + int rc; + + XMEMSET(&unsubscribe, 0, sizeof(unsubscribe)); + unsubscribe.topic_type = SN_TOPIC_ID_TYPE_NORMAL; + unsubscribe.topicNameId = "abc"; + unsubscribe.packet_id = 0x1234; + + rc = SN_Encode_Unsubscribe(tx_buf, (int)sizeof(tx_buf), &unsubscribe); + ASSERT_EQ(8, rc); + ASSERT_EQ(8, tx_buf[0]); + ASSERT_EQ(SN_MSG_TYPE_UNSUBSCRIBE, tx_buf[1]); + ASSERT_EQ(0x00, tx_buf[2]); + ASSERT_EQ(0x12, tx_buf[3]); + ASSERT_EQ(0x34, tx_buf[4]); + ASSERT_EQ('a', tx_buf[5]); + ASSERT_EQ('b', tx_buf[6]); + ASSERT_EQ('c', tx_buf[7]); +} + +TEST(sn_encode_unsubscribe_short_topic_valid) +{ + /* SHORT topic "tp" (2 chars, no XSTRLEN), QoS0, msgid 0x1234 -> 7-byte + * packet: [len=7][UNSUBSCRIBE][flags=SHORT(0x02)][id hi][id lo][t][p] */ + byte tx_buf[32]; + char topic[2] = { 't', 'p' }; + SN_Unsubscribe unsubscribe; + int rc; + + XMEMSET(&unsubscribe, 0, sizeof(unsubscribe)); + unsubscribe.topic_type = SN_TOPIC_ID_TYPE_SHORT; + unsubscribe.topicNameId = topic; + unsubscribe.packet_id = 0x1234; + + rc = SN_Encode_Unsubscribe(tx_buf, (int)sizeof(tx_buf), &unsubscribe); + ASSERT_EQ(7, rc); + ASSERT_EQ(7, tx_buf[0]); + ASSERT_EQ(SN_MSG_TYPE_UNSUBSCRIBE, tx_buf[1]); + ASSERT_EQ(0x02, tx_buf[2]); + ASSERT_EQ(0x12, tx_buf[3]); + ASSERT_EQ(0x34, tx_buf[4]); + ASSERT_EQ('t', tx_buf[5]); + ASSERT_EQ('p', tx_buf[6]); +} + /* ============================================================================ * Suite runner * ============================================================================ */ @@ -904,6 +1121,20 @@ int main(int argc, char** argv) RUN_TEST(sn_decode_publish_qos1_nonzero_packet_id_valid); RUN_TEST(sn_decode_publish_qosneg1_zero_packet_id_valid); + /* SN_Encode_Subscribe */ + RUN_TEST(sn_encode_subscribe_null_topic_normal_rejected); + RUN_TEST(sn_encode_subscribe_null_topic_short_rejected); + RUN_TEST(sn_encode_subscribe_null_args_rejected); + RUN_TEST(sn_encode_subscribe_normal_topic_valid); + RUN_TEST(sn_encode_subscribe_short_topic_valid); + + /* SN_Encode_Unsubscribe */ + RUN_TEST(sn_encode_unsubscribe_null_topic_normal_rejected); + RUN_TEST(sn_encode_unsubscribe_null_topic_short_rejected); + RUN_TEST(sn_encode_unsubscribe_null_args_rejected); + RUN_TEST(sn_encode_unsubscribe_normal_topic_valid); + RUN_TEST(sn_encode_unsubscribe_short_topic_valid); + /* SN_Packet_TypeDesc */ #ifndef WOLFMQTT_NO_ERROR_STRINGS RUN_TEST(sn_typedesc_pubcomp); From 873fc140c803bf827a808a299213cb40f8934fc8 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 9 Jun 2026 08:41:46 -0500 Subject: [PATCH 14/23] Fix f-4058: SN_Decode_Header check buf_len --- src/mqtt_sn_packet.c | 67 ++++++++++------ tests/test_mqtt_sn.c | 176 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 218 insertions(+), 25 deletions(-) diff --git a/src/mqtt_sn_packet.c b/src/mqtt_sn_packet.c index 3bb1341ad..f9dbd0069 100644 --- a/src/mqtt_sn_packet.c +++ b/src/mqtt_sn_packet.c @@ -138,40 +138,30 @@ int SN_Decode_Header(byte *rx_buf, int rx_buf_len, *p_packet_type = packet_type; if (p_packet_id) { + /* Bytes already consumed from rx_buf_orig: the 1-byte length field + * (plus the 2-byte extended length when SN_PACKET_LEN_IND was used) + * and the 1-byte message type. The 2-byte MsgId sits id_offset bytes + * past the current rx_buf position; where it begins depends on the + * packet type. */ + int consumed = (int)(rx_buf - rx_buf_orig); + int id_offset; + 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 */ - rc = MqttDecode_Num(rx_buf + 2, p_packet_id, - (word32)(rx_buf_len - 3)); - if (rc < 0) { - return rc; - } + /* TopicId(2) precedes the MsgId(2): octet 4-5 */ + id_offset = 2; break; case SN_MSG_TYPE_PUBCOMP: case SN_MSG_TYPE_PUBREC: case SN_MSG_TYPE_PUBREL: case SN_MSG_TYPE_UNSUBACK: - /* octet 2-3 */ - rc = MqttDecode_Num(rx_buf, p_packet_id, - (word32)(rx_buf_len - 1)); - if (rc < 0) { - return rc; - } + /* MsgId(2) immediately follows the type: octet 2-3 */ + id_offset = 0; break; case SN_MSG_TYPE_SUBACK: - if (rx_buf_len < 4) { - return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); - } - /* octet 5-6 */ - rc = MqttDecode_Num(rx_buf + 3, p_packet_id, - (word32)(rx_buf_len - 4)); - if (rc < 0) { - return rc; - } + /* Flags(1) + TopicId(2) precede the MsgId(2): octet 5-6 */ + id_offset = 3; break; case SN_MSG_TYPE_ADVERTISE: case SN_MSG_TYPE_SEARCHGW: @@ -196,9 +186,36 @@ int SN_Decode_Header(byte *rx_buf, int rx_buf_len, case SN_MSG_TYPE_ENCAPMSG: case SN_MSG_TYPE_RESERVED: default: - *p_packet_id = 0; + /* No MsgId carried in this packet type */ + id_offset = -1; break; } + + if (id_offset < 0) { + *p_packet_id = 0; + } + else { + /* Bytes the declared packet leaves for the MsgId at + * rx_buf + id_offset. Bound the read by total_len (the declared + * packet length, already validated <= rx_buf_len above), not by + * rx_buf_len: this keeps the read inside the buffer (CWE-125) and + * additionally rejects a frame whose declared length stops short + * of the MsgId rather than reading adjacent bytes. Evaluate as a + * signed int and reject before the unsigned cast below, so a short + * frame cannot wrap to a huge length and slip past MqttDecode_Num's + * internal bound check. Measuring from consumed keeps the bound + * correct for the IND form, where the header occupies 4 bytes + * rather than 2. */ + int id_avail = (int)total_len - consumed - id_offset; + if (id_avail < (int)MQTT_DATA_LEN_SIZE) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); + } + rc = MqttDecode_Num(rx_buf + id_offset, p_packet_id, + (word32)id_avail); + if (rc < 0) { + return rc; + } + } } return (int)total_len; diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c index 6f859201c..14e7ebc08 100644 --- a/tests/test_mqtt_sn.c +++ b/tests/test_mqtt_sn.c @@ -135,6 +135,169 @@ TEST(sn_header_buf_too_short_rejected) ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); } +TEST(sn_header_puback_packet_id_extracted) +{ + /* [len][type=PUBACK][topicId(2)][packetId(2)][retcode] */ + byte buf[7] = { 0x07, SN_MSG_TYPE_PUBACK, 0x00, 0x01, 0x9A, 0xBC, 0x00 }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + word16 packet_id = 0; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, &packet_id); + ASSERT_EQ(7, rc); + ASSERT_EQ(SN_MSG_TYPE_PUBACK, type); + ASSERT_EQ(0x9ABC, packet_id); +} + +TEST(sn_header_pubcomp_packet_id_extracted) +{ + /* [len=4][type=PUBCOMP][packetId(2)] - MsgId immediately follows type */ + byte buf[4] = { 0x04, SN_MSG_TYPE_PUBCOMP, 0xAB, 0xCD }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + word16 packet_id = 0; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, &packet_id); + ASSERT_EQ(4, rc); + ASSERT_EQ(SN_MSG_TYPE_PUBCOMP, type); + ASSERT_EQ(0xABCD, packet_id); +} + +TEST(sn_header_suback_packet_id_extracted) +{ + /* [len=8][type=SUBACK][flags][topicId(2)][packetId(2)][retcode] */ + byte buf[8] = { 0x08, SN_MSG_TYPE_SUBACK, 0x00, 0x00, 0x01, + 0x56, 0x78, 0x00 }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + word16 packet_id = 0; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, &packet_id); + ASSERT_EQ(8, rc); + ASSERT_EQ(SN_MSG_TYPE_SUBACK, type); + ASSERT_EQ(0x5678, packet_id); +} + +/* The MsgId bound handed to MqttDecode_Num must be measured from the bytes + * already consumed, not from rx_buf_len. Each case below declares a total_len + * that fits the buffer (so the > rx_buf_len guard passes) yet leaves the + * 2-byte MsgId one byte short. The old rx_buf_len-derived bound read one past + * the end; the decoder must now reject these. */ +TEST(sn_header_pubcomp_short_id_rejected) +{ + /* [len=3][type=PUBCOMP][1 stray byte]: MsgId(2) needs byte 3 (OOB). */ + byte buf[3] = { 0x03, SN_MSG_TYPE_PUBCOMP, 0x00 }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + word16 packet_id = 0xFFFF; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, &packet_id); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_header_regack_short_id_rejected) +{ + /* [len=5][type=REGACK][topicId(2)][1 byte]: MsgId(2) at octet 4-5 needs + * byte 5 (OOB). Old bound rx_buf_len-3 = 2 let MqttDecode_Num read it. */ + byte buf[5] = { 0x05, SN_MSG_TYPE_REGACK, 0x00, 0x01, 0x12 }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + word16 packet_id = 0xFFFF; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, &packet_id); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_header_suback_short_id_rejected) +{ + /* [len=6][type=SUBACK][flags][topicId(2)][1 byte]: MsgId(2) at octet 5-6 + * needs byte 6 (OOB). Old bound rx_buf_len-4 = 2 over-read buf[6]. */ + byte buf[6] = { 0x06, SN_MSG_TYPE_SUBACK, 0x00, 0x00, 0x01, 0x12 }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + word16 packet_id = 0xFFFF; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, &packet_id); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_header_ind_form_pubcomp_short_id_rejected) +{ + /* Extended-length (IND) PUBCOMP whose total_len(4) covers only the 4 + * header bytes (len-ind + 2-byte length + type), leaving no room for the + * MsgId. Because the IND form consumes 4 header bytes (not 2), the old + * rx_buf_len-1 bound read two bytes past this 4-byte buffer. */ + byte buf[4] = { SN_PACKET_LEN_IND, 0x00, 0x04, SN_MSG_TYPE_PUBCOMP }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + word16 packet_id = 0xFFFF; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, &packet_id); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_header_ind_form_pubcomp_packet_id_extracted) +{ + /* Extended-length (IND) PUBCOMP carrying a MsgId at octet 4-5: the bound + * must be measured from the 4 consumed header bytes. */ + byte buf[6] = { SN_PACKET_LEN_IND, 0x00, 0x06, SN_MSG_TYPE_PUBCOMP, + 0xDE, 0xF0 }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + word16 packet_id = 0; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, &packet_id); + ASSERT_EQ(6, rc); + ASSERT_EQ(SN_MSG_TYPE_PUBCOMP, type); + ASSERT_EQ(0xDEF0, packet_id); +} + +/* PUBREC, PUBREL and UNSUBACK share PUBCOMP's id_offset=0 case grouping; pin + * each so an accidental removal of a case label would fall through to the + * no-MsgId default (returning packet_id=0) and fail a test. */ +TEST(sn_header_pubrec_packet_id_extracted) +{ + byte buf[4] = { 0x04, SN_MSG_TYPE_PUBREC, 0x30, 0x31 }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + word16 packet_id = 0; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, &packet_id); + ASSERT_EQ(4, rc); + ASSERT_EQ(SN_MSG_TYPE_PUBREC, type); + ASSERT_EQ(0x3031, packet_id); +} + +TEST(sn_header_pubrel_packet_id_extracted) +{ + byte buf[4] = { 0x04, SN_MSG_TYPE_PUBREL, 0x40, 0x41 }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + word16 packet_id = 0; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, &packet_id); + ASSERT_EQ(4, rc); + ASSERT_EQ(SN_MSG_TYPE_PUBREL, type); + ASSERT_EQ(0x4041, packet_id); +} + +TEST(sn_header_unsuback_packet_id_extracted) +{ + byte buf[4] = { 0x04, SN_MSG_TYPE_UNSUBACK, 0x20, 0x21 }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + word16 packet_id = 0; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, &packet_id); + ASSERT_EQ(4, rc); + ASSERT_EQ(SN_MSG_TYPE_UNSUBACK, type); + ASSERT_EQ(0x2021, packet_id); +} + +TEST(sn_header_declared_len_short_of_msgid_rejected) +{ + /* total_len=3 declares a 3-byte PUBCOMP, but the buffer holds more bytes + * (e.g. adjacent/trailing data). The MsgId is bounded by the declared + * length, not the physical buffer, so this malformed frame is rejected + * rather than reading the id from bytes past the declared packet. */ + byte buf[6] = { 0x03, SN_MSG_TYPE_PUBCOMP, 0x11, 0x22, 0x33, 0x44 }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + word16 packet_id = 0xFFFF; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, &packet_id); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_header_no_msgid_type_zeroes_packet_id) +{ + /* A packet type that carries no MsgId must zero a caller-supplied id + * (the id_offset < 0 branch), not leave the pre-set value untouched. */ + byte buf[2] = { 0x02, SN_MSG_TYPE_DISCONNECT }; + SN_MsgType type = SN_MSG_TYPE_RESERVED; + word16 packet_id = 0xFFFF; + int rc = SN_Decode_Header(buf, (int)sizeof(buf), &type, &packet_id); + ASSERT_EQ(2, rc); + ASSERT_EQ(SN_MSG_TYPE_DISCONNECT, type); + ASSERT_EQ(0, packet_id); +} + /* ============================================================================ * SN_Decode_GWInfo * ============================================================================ */ @@ -1068,10 +1231,23 @@ int main(int argc, char** argv) /* SN_Decode_Header */ RUN_TEST(sn_header_short_form_valid); RUN_TEST(sn_header_regack_packet_id_extracted); + RUN_TEST(sn_header_puback_packet_id_extracted); + RUN_TEST(sn_header_pubcomp_packet_id_extracted); + RUN_TEST(sn_header_suback_packet_id_extracted); RUN_TEST(sn_header_ind_form_total_len_equals_consumed_rejected); RUN_TEST(sn_header_total_len_exceeds_buffer_rejected); RUN_TEST(sn_header_null_buf_rejected); RUN_TEST(sn_header_buf_too_short_rejected); + RUN_TEST(sn_header_pubcomp_short_id_rejected); + RUN_TEST(sn_header_regack_short_id_rejected); + RUN_TEST(sn_header_suback_short_id_rejected); + RUN_TEST(sn_header_ind_form_pubcomp_short_id_rejected); + RUN_TEST(sn_header_ind_form_pubcomp_packet_id_extracted); + RUN_TEST(sn_header_pubrec_packet_id_extracted); + RUN_TEST(sn_header_pubrel_packet_id_extracted); + RUN_TEST(sn_header_unsuback_packet_id_extracted); + RUN_TEST(sn_header_declared_len_short_of_msgid_rejected); + RUN_TEST(sn_header_no_msgid_type_zeroes_packet_id); /* SN_Decode_GWInfo */ RUN_TEST(sn_gwinfo_short_form_no_addr_valid); From dd12b55c647099efabc2f4d6de69247d78c5d6f6 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 9 Jun 2026 08:47:02 -0500 Subject: [PATCH 15/23] Fix f-3632: Add tests for SN_Decode_GWInfo --- tests/test_mqtt_sn.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c index 14e7ebc08..5af5c2baf 100644 --- a/tests/test_mqtt_sn.c +++ b/tests/test_mqtt_sn.c @@ -354,6 +354,22 @@ TEST(sn_gwinfo_ind_form_total_len_equals_consumed_rejected) ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); } +TEST(sn_gwinfo_ind_form_poc_4byte_datagram_rejected) +{ + /* Exact attacker PoC from f-3632: a 4-byte IND-form GWINFO datagram. + * IND header consumes 3 bytes; total_len=4 leaves room for only one of + * the two remaining reads (type + gwId), so the gwId read (and the old + * "total_len - 3" address copy) would walk past this 4-byte buffer. + * total_len=4 is the boundary value: the largest total_len that must + * still be rejected for the extended-length form. */ + byte buf[4] = { SN_PACKET_LEN_IND, 0x00, 0x04, SN_MSG_TYPE_GWINFO }; + SN_GwInfo info; + int rc; + XMEMSET(&info, 0, sizeof(info)); + rc = SN_Decode_GWInfo(buf, (int)sizeof(buf), &info); + ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); +} + TEST(sn_gwinfo_ind_form_no_addr_no_overread) { /* IND form, total_len=5: the 5 bytes are exactly [IND][len(2)][type][gwId] @@ -1254,6 +1270,7 @@ int main(int argc, char** argv) RUN_TEST(sn_gwinfo_short_form_with_addr_valid); RUN_TEST(sn_gwinfo_short_form_total_len_too_small_rejected); RUN_TEST(sn_gwinfo_ind_form_total_len_equals_consumed_rejected); + RUN_TEST(sn_gwinfo_ind_form_poc_4byte_datagram_rejected); RUN_TEST(sn_gwinfo_ind_form_no_addr_no_overread); RUN_TEST(sn_gwinfo_ind_form_with_addr_valid); RUN_TEST(sn_gwinfo_wrong_type_rejected); From bd88c3b24c7801ec0f255562678f6ebbd22763af Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 9 Jun 2026 08:56:04 -0500 Subject: [PATCH 16/23] Fix f-2333 and f-2334: SN_Encode_WillTopic/Update check willTopic --- src/mqtt_sn_packet.c | 18 +++-- tests/test_mqtt_sn.c | 161 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 175 insertions(+), 4 deletions(-) diff --git a/src/mqtt_sn_packet.c b/src/mqtt_sn_packet.c index f9dbd0069..fa234e547 100644 --- a/src/mqtt_sn_packet.c +++ b/src/mqtt_sn_packet.c @@ -456,8 +456,13 @@ int SN_Encode_WillTopic(byte *tx_buf, int tx_buf_len, SN_Will *willTopic) int total_len; byte *tx_payload, flags = 0; - /* Validate required arguments */ - if (tx_buf == NULL) { + /* Validate required arguments. A NULL willTopic is valid: it produces an + * empty WILLTOPIC message (2 octets) used to delete the will. But when + * willTopic is non-NULL its willTopic string is dereferenced by XSTRLEN + * (and XMEMCPY) below, so a non-NULL struct carrying a NULL string must be + * rejected here rather than crashing in XSTRLEN(NULL). */ + if (tx_buf == NULL || + (willTopic != NULL && willTopic->willTopic == NULL)) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } @@ -601,8 +606,13 @@ int SN_Encode_WillTopicUpdate(byte *tx_buf, int tx_buf_len, SN_Will *willTopic) int total_len; byte *tx_payload, flags = 0; - /* Validate required arguments */ - if (tx_buf == NULL) { + /* Validate required arguments. A NULL willTopic is valid: it produces an + * empty WILLTOPICUPD message (2 octets) used to delete the will. But when + * willTopic is non-NULL its willTopic string is dereferenced by XSTRLEN + * (and XMEMCPY) below, so a non-NULL struct carrying a NULL string must be + * rejected here rather than crashing in XSTRLEN(NULL). */ + if (tx_buf == NULL || + (willTopic != NULL && willTopic->willTopic == NULL)) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c index 5af5c2baf..82f2608fe 100644 --- a/tests/test_mqtt_sn.c +++ b/tests/test_mqtt_sn.c @@ -1231,6 +1231,155 @@ TEST(sn_encode_unsubscribe_short_topic_valid) ASSERT_EQ('p', tx_buf[6]); } +/* ============================================================================ + * SN_Encode_WillTopic + * + * A NULL willTopic argument is valid and produces an empty (2-octet) WILLTOPIC + * message that deletes the will stored on the gateway. But when willTopic is + * non-NULL the encoder dereferences willTopic->willTopic via XSTRLEN (length + * sizing) and XMEMCPY (payload copy). A caller that passes a non-NULL SN_Will + * but leaves the willTopic string unset (NULL) would crash in XSTRLEN(NULL); + * the encoder must reject that with BAD_ARG instead - report 2333. + * ============================================================================ */ + +TEST(sn_encode_willtopic_null_topic_string_rejected) +{ + /* Regression for report 2333: a zero-initialized SN_Will is non-NULL but + * its willTopic string is NULL. The pre-fix code called XSTRLEN(NULL) and + * crashed; it must now return BAD_ARG. */ + byte tx_buf[32]; + SN_Will will; + int rc; + + XMEMSET(&will, 0, sizeof(will)); + /* willTopic string left NULL by the memset */ + + rc = SN_Encode_WillTopic(tx_buf, (int)sizeof(tx_buf), &will); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_encode_willtopic_null_buf_rejected) +{ + SN_Will will; + int rc; + + XMEMSET(&will, 0, sizeof(will)); + will.willTopic = "abc"; + + rc = SN_Encode_WillTopic(NULL, 32, &will); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_encode_willtopic_empty_will_valid) +{ + /* A NULL willTopic deletes the will: an empty 2-octet WILLTOPIC message + * [len=2][WILLTOPIC]. This must keep working after the NULL-string fix. */ + byte tx_buf[32]; + int rc; + + rc = SN_Encode_WillTopic(tx_buf, (int)sizeof(tx_buf), NULL); + ASSERT_EQ(2, rc); + ASSERT_EQ(2, tx_buf[0]); + ASSERT_EQ(SN_MSG_TYPE_WILLTOPIC, tx_buf[1]); +} + +TEST(sn_encode_willtopic_topic_valid) +{ + /* Will topic "abc", QoS2, retain=1 -> 6-byte packet: + * [len=6][WILLTOPIC][flags][a][b][c] + * flags = ((2 << 5) & 0x60) | RETAIN(0x10) = 0x40 | 0x10 = 0x50 */ + byte tx_buf[32]; + SN_Will will; + int rc; + + XMEMSET(&will, 0, sizeof(will)); + will.willTopic = "abc"; + will.qos = 2; + will.retain = 1; + + rc = SN_Encode_WillTopic(tx_buf, (int)sizeof(tx_buf), &will); + ASSERT_EQ(6, rc); + ASSERT_EQ(6, tx_buf[0]); + ASSERT_EQ(SN_MSG_TYPE_WILLTOPIC, tx_buf[1]); + ASSERT_EQ(0x50, tx_buf[2]); + ASSERT_EQ('a', tx_buf[3]); + ASSERT_EQ('b', tx_buf[4]); + ASSERT_EQ('c', tx_buf[5]); +} + +/* ============================================================================ + * SN_Encode_WillTopicUpdate + * + * Identical NULL-string footgun to SN_Encode_WillTopic: a NULL willTopic + * argument is valid (empty 2-octet WILLTOPICUPD that deletes the will), but a + * non-NULL SN_Will whose willTopic string is NULL is dereferenced by XSTRLEN + * (and XMEMCPY) and must be rejected with BAD_ARG instead of crashing. + * ============================================================================ */ + +TEST(sn_encode_willtopicupd_null_topic_string_rejected) +{ + /* Zero-initialized SN_Will is non-NULL with a NULL willTopic string. The + * pre-fix code called XSTRLEN(NULL) and crashed; it must return BAD_ARG. */ + byte tx_buf[32]; + SN_Will will; + int rc; + + XMEMSET(&will, 0, sizeof(will)); + /* willTopic string left NULL by the memset */ + + rc = SN_Encode_WillTopicUpdate(tx_buf, (int)sizeof(tx_buf), &will); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_encode_willtopicupd_null_buf_rejected) +{ + SN_Will will; + int rc; + + XMEMSET(&will, 0, sizeof(will)); + will.willTopic = "abc"; + + rc = SN_Encode_WillTopicUpdate(NULL, 32, &will); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_encode_willtopicupd_empty_will_valid) +{ + /* A NULL willTopic deletes the will: an empty 2-octet WILLTOPICUPD message + * [len=2][WILLTOPICUPD]. This must keep working after the NULL-string fix. */ + byte tx_buf[32]; + int rc; + + rc = SN_Encode_WillTopicUpdate(tx_buf, (int)sizeof(tx_buf), NULL); + ASSERT_EQ(2, rc); + ASSERT_EQ(2, tx_buf[0]); + ASSERT_EQ(SN_MSG_TYPE_WILLTOPICUPD, tx_buf[1]); +} + +TEST(sn_encode_willtopicupd_topic_valid) +{ + /* Will topic "abc", QoS2, retain=1 -> 6-byte packet: + * [len=6][WILLTOPICUPD][flags][a][b][c] + * flags = ((2 << 5) & 0x60) | RETAIN(0x10) = 0x40 | 0x10 = 0x50 */ + byte tx_buf[32]; + SN_Will will; + int rc; + + XMEMSET(&will, 0, sizeof(will)); + will.willTopic = "abc"; + will.qos = 2; + will.retain = 1; + + rc = SN_Encode_WillTopicUpdate(tx_buf, (int)sizeof(tx_buf), &will); + ASSERT_EQ(6, rc); + ASSERT_EQ(6, tx_buf[0]); + ASSERT_EQ(SN_MSG_TYPE_WILLTOPICUPD, tx_buf[1]); + ASSERT_EQ(0x50, tx_buf[2]); + ASSERT_EQ('a', tx_buf[3]); + ASSERT_EQ('b', tx_buf[4]); + ASSERT_EQ('c', tx_buf[5]); +} + /* ============================================================================ * Suite runner * ============================================================================ */ @@ -1328,6 +1477,18 @@ int main(int argc, char** argv) RUN_TEST(sn_encode_unsubscribe_normal_topic_valid); RUN_TEST(sn_encode_unsubscribe_short_topic_valid); + /* SN_Encode_WillTopic */ + RUN_TEST(sn_encode_willtopic_null_topic_string_rejected); + RUN_TEST(sn_encode_willtopic_null_buf_rejected); + RUN_TEST(sn_encode_willtopic_empty_will_valid); + RUN_TEST(sn_encode_willtopic_topic_valid); + + /* SN_Encode_WillTopicUpdate */ + RUN_TEST(sn_encode_willtopicupd_null_topic_string_rejected); + RUN_TEST(sn_encode_willtopicupd_null_buf_rejected); + RUN_TEST(sn_encode_willtopicupd_empty_will_valid); + RUN_TEST(sn_encode_willtopicupd_topic_valid); + /* SN_Packet_TypeDesc */ #ifndef WOLFMQTT_NO_ERROR_STRINGS RUN_TEST(sn_typedesc_pubcomp); From 509b1f37d4df9b440977de41e99652a13134915c Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 9 Jun 2026 09:01:46 -0500 Subject: [PATCH 17/23] Fix f-2330: SN_Encode_Register check topicName --- src/mqtt_sn_packet.c | 7 +++-- tests/test_mqtt_sn.c | 72 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/src/mqtt_sn_packet.c b/src/mqtt_sn_packet.c index fa234e547..c7c6c2e0c 100644 --- a/src/mqtt_sn_packet.c +++ b/src/mqtt_sn_packet.c @@ -824,8 +824,11 @@ int SN_Encode_Register(byte *tx_buf, int tx_buf_len, SN_Register *regist) int total_len, topic_len; byte *tx_payload; - /* Validate required arguments */ - if (tx_buf == NULL || regist == NULL) { + /* Validate required arguments. topicName is dereferenced unconditionally + * via XSTRLEN below (and again before the XMEMCPY), so reject NULL up front. + * A caller that zero-initializes an SN_Register and forgets to set topicName + * would otherwise crash in XSTRLEN(NULL) instead of getting BAD_ARG. */ + if (tx_buf == NULL || regist == NULL || regist->topicName == NULL) { return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG); } diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c index 82f2608fe..c619a89d1 100644 --- a/tests/test_mqtt_sn.c +++ b/tests/test_mqtt_sn.c @@ -1380,6 +1380,74 @@ TEST(sn_encode_willtopicupd_topic_valid) ASSERT_EQ('c', tx_buf[5]); } +/* ============================================================================ + * SN_Encode_Register + * + * topicName is dereferenced unconditionally - XSTRLEN for length sizing and + * again before the XMEMCPY that copies the payload. A caller that zero- + * initializes an SN_Register and forgets to set topicName would crash in + * XSTRLEN(NULL); the encoder must reject that with BAD_ARG instead - report + * 2330. + * ============================================================================ */ + +TEST(sn_encode_register_null_topic_string_rejected) +{ + /* Regression for report 2330: a zero-initialized SN_Register is non-NULL + * but its topicName string is NULL. The pre-fix code called XSTRLEN(NULL) + * and crashed; it must now return BAD_ARG. */ + byte tx_buf[32]; + SN_Register regist; + int rc; + + XMEMSET(®ist, 0, sizeof(regist)); + /* topicName left NULL by the memset */ + + rc = SN_Encode_Register(tx_buf, (int)sizeof(tx_buf), ®ist); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_encode_register_null_args_rejected) +{ + byte tx_buf[32]; + SN_Register regist; + int rc; + + XMEMSET(®ist, 0, sizeof(regist)); + regist.topicName = "abc"; + + rc = SN_Encode_Register(NULL, (int)sizeof(tx_buf), ®ist); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); + + rc = SN_Encode_Register(tx_buf, (int)sizeof(tx_buf), NULL); + ASSERT_EQ(MQTT_CODE_ERROR_BAD_ARG, rc); +} + +TEST(sn_encode_register_topic_valid) +{ + /* Topic "abc", topicId 0x1234, msgid 0x5678 -> 9-byte packet: + * [len=9][REGISTER][id hi][id lo][pid hi][pid lo][a][b][c] */ + byte tx_buf[32]; + SN_Register regist; + int rc; + + XMEMSET(®ist, 0, sizeof(regist)); + regist.topicId = 0x1234; + regist.packet_id = 0x5678; + regist.topicName = "abc"; + + rc = SN_Encode_Register(tx_buf, (int)sizeof(tx_buf), ®ist); + ASSERT_EQ(9, rc); + ASSERT_EQ(9, tx_buf[0]); + ASSERT_EQ(SN_MSG_TYPE_REGISTER, tx_buf[1]); + ASSERT_EQ(0x12, tx_buf[2]); + ASSERT_EQ(0x34, tx_buf[3]); + ASSERT_EQ(0x56, tx_buf[4]); + ASSERT_EQ(0x78, tx_buf[5]); + ASSERT_EQ('a', tx_buf[6]); + ASSERT_EQ('b', tx_buf[7]); + ASSERT_EQ('c', tx_buf[8]); +} + /* ============================================================================ * Suite runner * ============================================================================ */ @@ -1489,6 +1557,10 @@ int main(int argc, char** argv) RUN_TEST(sn_encode_willtopicupd_empty_will_valid); RUN_TEST(sn_encode_willtopicupd_topic_valid); + RUN_TEST(sn_encode_register_null_topic_string_rejected); + RUN_TEST(sn_encode_register_null_args_rejected); + RUN_TEST(sn_encode_register_topic_valid); + /* SN_Packet_TypeDesc */ #ifndef WOLFMQTT_NO_ERROR_STRINGS RUN_TEST(sn_typedesc_pubcomp); From 0fe135c433f9013dd5f10c1643f7c06ccbc55d7e Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 9 Jun 2026 09:14:57 -0500 Subject: [PATCH 18/23] Fix f-3831: SN_Decode_Register bounds check rejects all valid REGISTER packets --- src/mqtt_sn_client.c | 8 ++++- src/mqtt_sn_packet.c | 6 ++++ tests/test_mqtt_sn.c | 84 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 1 deletion(-) diff --git a/src/mqtt_sn_client.c b/src/mqtt_sn_client.c index 005fdc39c..b893ad108 100644 --- a/src/mqtt_sn_client.c +++ b/src/mqtt_sn_client.c @@ -94,7 +94,13 @@ static int SN_Client_HandlePacket(MqttClient* client, SN_MsgType packet_type, XMEMSET(®_s, 0, sizeof(SN_Register)); - rc = SN_Decode_Register(client->rx_buf, client->packet.buf_len, + /* Pass the full receive-buffer capacity (rx_buf_len), not the + * decoded packet length (packet.buf_len). SN_Decode_Register + * NUL-terminates topicName in place one byte past the packet, so + * its strict bounds check needs the writable buffer size to leave + * room for that terminator. Passing packet.buf_len (== total_len) + * made the check reject every valid REGISTER. */ + rc = SN_Decode_Register(client->rx_buf, client->rx_buf_len, ®_s); if (rc > 0) { diff --git a/src/mqtt_sn_packet.c b/src/mqtt_sn_packet.c index c7c6c2e0c..3205e2377 100644 --- a/src/mqtt_sn_packet.c +++ b/src/mqtt_sn_packet.c @@ -877,6 +877,12 @@ int SN_Encode_Register(byte *tx_buf, int tx_buf_len, SN_Register *regist) return total_len; } +/* Note: rx_buf_len must be the writable capacity of rx_buf, not the decoded + * packet length. Unlike the other SN decoders this one NUL-terminates topicName + * in place at offset total_len (one byte past the packet), so the strict + * total_len >= rx_buf_len guard below relies on rx_buf_len leaving room for that + * terminator. Callers therefore pass client->rx_buf_len (see + * SN_Client_HandlePacket), not client->packet.buf_len. */ int SN_Decode_Register(byte *rx_buf, int rx_buf_len, SN_Register *regist) { word16 total_len; diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c index c619a89d1..e8fef70bd 100644 --- a/tests/test_mqtt_sn.c +++ b/tests/test_mqtt_sn.c @@ -495,6 +495,87 @@ TEST(sn_register_wrong_type_rejected) ASSERT_EQ(MQTT_CODE_ERROR_PACKET_TYPE, rc); } +TEST(sn_register_no_room_for_terminator_rejected) +{ + /* Regression for report 3831. SN_Decode_Register NUL-terminates topicName + * in place at offset total_len, one byte past the packet. When rx_buf_len + * only covers the packet itself (rx_buf_len == total_len) there is no slot + * for that terminator, so the strict guard must reject with OUT_OF_BUFFER + * rather than write out of bounds. This is exactly why SN_Client_HandlePacket + * must hand the decoder the full rx_buf capacity (client->rx_buf_len), not + * the decoded packet length (client->packet.buf_len): the latter equals + * total_len, which made this guard reject every valid REGISTER. */ + byte buf[8] = { 0x08, SN_MSG_TYPE_REGISTER, 0x01, 0x02, 0x03, 0x04, + 'a', 'b' }; + SN_Register reg; + int rc; + XMEMSET(®, 0, sizeof(reg)); + /* rx_buf_len == total_len (8): no room for the trailing NUL */ + rc = SN_Decode_Register(buf, 8, ®); + ASSERT_EQ(MQTT_CODE_ERROR_OUT_OF_BUFFER, rc); +} + +TEST(sn_register_roundtrip_short_form) +{ + /* Report 3831: encode then decode a normal REGISTER and confirm every + * field survives the round trip. "sensors/temp" is 12 bytes, so + * total_len = 12 + 6 = 18 (<= 255 -> short, single-byte length). The decode + * buffer is larger than the packet so the in-place NUL terminator fits. */ + byte buf[64]; + SN_Register enc, dec; + int enc_len, rc; + + XMEMSET(&enc, 0, sizeof(enc)); + enc.topicId = 0x1234; + enc.packet_id = 0x5678; + enc.topicName = "sensors/temp"; + + enc_len = SN_Encode_Register(buf, (int)sizeof(buf), &enc); + ASSERT_EQ(18, enc_len); + ASSERT_EQ(18, buf[0]); /* short form: length in the first byte */ + + XMEMSET(&dec, 0, sizeof(dec)); + rc = SN_Decode_Register(buf, (int)sizeof(buf), &dec); + ASSERT_EQ(enc_len, rc); + ASSERT_EQ(0x1234, dec.topicId); + ASSERT_EQ(0x5678, dec.packet_id); + ASSERT_NOT_NULL(dec.topicName); + ASSERT_STR_EQ("sensors/temp", dec.topicName); +} + +TEST(sn_register_roundtrip_ind_form) +{ + /* Report 3831: same round trip but with an extended-length (IND) encoding. + * A 260-byte topic gives total_len = 260 + 6 = 266 (> 255), so the encoder + * switches to the 3-byte length header (IND + 2 length bytes) and + * total_len becomes 268. Confirms the decoder honors the IND form and that + * a long topic name survives the round trip intact. */ + byte buf[300]; + char topic[261]; + SN_Register enc, dec; + int enc_len, rc; + + XMEMSET(topic, 'x', 260); + topic[260] = '\0'; + + XMEMSET(&enc, 0, sizeof(enc)); + enc.topicId = 0x0102; + enc.packet_id = 0x0304; + enc.topicName = topic; + + enc_len = SN_Encode_Register(buf, (int)sizeof(buf), &enc); + ASSERT_EQ(268, enc_len); + ASSERT_EQ(SN_PACKET_LEN_IND, buf[0]); /* extended-length indicator */ + + XMEMSET(&dec, 0, sizeof(dec)); + rc = SN_Decode_Register(buf, (int)sizeof(buf), &dec); + ASSERT_EQ(enc_len, rc); + ASSERT_EQ(0x0102, dec.topicId); + ASSERT_EQ(0x0304, dec.packet_id); + ASSERT_NOT_NULL(dec.topicName); + ASSERT_STR_EQ(topic, dec.topicName); +} + /* ============================================================================ * SN_Decode_ConnectAck * @@ -1498,6 +1579,9 @@ int main(int argc, char** argv) RUN_TEST(sn_register_ind_form_total_len_too_small_rejected); RUN_TEST(sn_register_total_len_below_fixed_min_rejected); RUN_TEST(sn_register_wrong_type_rejected); + RUN_TEST(sn_register_no_room_for_terminator_rejected); + RUN_TEST(sn_register_roundtrip_short_form); + RUN_TEST(sn_register_roundtrip_ind_form); /* SN_Decode_ConnectAck */ RUN_TEST(sn_connack_accepted_valid); From 535a669a9bf444d8829f8a2dad382cdd7011836d Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 9 Jun 2026 09:41:48 -0500 Subject: [PATCH 19/23] Fix f-5663: example use of sn_reg_callback --- examples/include.am | 1 + examples/mqtt_log.h | 127 +++++++++++++++++++++++++++ examples/sn-client/sn-client.c | 9 +- examples/sn-client/sn-multithread.c | 9 +- tests/test_mqtt_sn.c | 130 ++++++++++++++++++++++++++++ 5 files changed, 274 insertions(+), 2 deletions(-) create mode 100644 examples/mqtt_log.h diff --git a/examples/include.am b/examples/include.am index b4d6fcda0..6b806736b 100644 --- a/examples/include.am +++ b/examples/include.am @@ -29,6 +29,7 @@ noinst_HEADERS += examples/mqttclient/mqttclient.h \ examples/wiot/wiot.h \ examples/mqttnet.h \ examples/mqttexample.h \ + examples/mqtt_log.h \ examples/mqttport.h \ examples/nbclient/nbclient.h \ examples/multithread/multithread.h \ diff --git a/examples/mqtt_log.h b/examples/mqtt_log.h new file mode 100644 index 000000000..06ae8a6a6 --- /dev/null +++ b/examples/mqtt_log.h @@ -0,0 +1,127 @@ +/* mqtt_log.h + * + * Copyright (C) 2006-2026 wolfSSL Inc. + * + * This file is part of wolfMQTT. + * + * wolfMQTT is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfMQTT is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + */ + +#ifndef WOLFMQTT_EXAMPLE_LOG_H +#define WOLFMQTT_EXAMPLE_LOG_H + +#include "wolfmqtt/mqtt_types.h" /* for byte / word32 */ + +#ifdef __cplusplus +extern "C" { +#endif + +/* mqtt_log_sanitize is a header-only helper, so a translation unit that + * includes it but whose log sink is compiled out (e.g. sn-multithread.c built + * without WOLFMQTT_MULTITHREAD) leaves it unreferenced. Declaring it INLINE + * keeps that quiet on GCC/Clang (-Wunused-function) and MSVC (C4505); the + * attribute is a fallback for GCC/Clang builds that set NO_INLINE, where the + * INLINE macro expands to nothing. */ +#if defined(__GNUC__) || defined(__clang__) + #define MQTT_LOG_MAYBE_UNUSED __attribute__((unused)) +#else + #define MQTT_LOG_MAYBE_UNUSED +#endif + +/* Sanitize an untrusted string for safe logging (CWE-117 defense). + * + * MQTT/MQTT-SN strings such as a REGISTER topic name are controlled by the + * remote peer. For MQTT-SN this peer is reachable over UDP, so an attacker who + * can spoof the gateway's source address controls the bytes outright. Writing + * such a string straight to a PRINTF/log sink lets the peer inject forged log + * lines (embedded CR/LF) or hijack the operator's terminal with ANSI escape + * sequences (ESC), making malicious output indistinguishable from genuine log + * lines. + * + * This helper copies src into the caller's fixed-size dst buffer, replacing + * every control byte (anything < 0x20, plus DEL 0x7f) with a printable escape + * so the result is always safe to hand to PRINTF: + * CR -> "\r" LF -> "\n" TAB -> "\t" VT -> "\v" ESC -> "\e" + * any other control byte -> "\xHH" + * + * dst is always NUL-terminated when dstLen > 0. Output is truncated to fit dst + * (never overflowed) and a multi-byte escape is emitted all-or-nothing, so the + * result never ends in a dangling backslash. Returns dst so the call can be + * used inline as a PRINTF argument. A NULL src yields "(null)". + */ +MQTT_LOG_MAYBE_UNUSED +static INLINE const char* mqtt_log_sanitize(char* dst, word32 dstLen, + const char* src) +{ + static const char hex_digits[] = "0123456789abcdef"; + word32 di = 0; + + if (dst == NULL || dstLen == 0) { + return dst; + } + if (src == NULL) { + src = "(null)"; + } + + while (*src != '\0') { + byte c = (byte)*src++; + char rep[4]; + word32 repLen = 0; + + switch (c) { + case '\r': rep[0] = '\\'; rep[1] = 'r'; repLen = 2; break; + case '\n': rep[0] = '\\'; rep[1] = 'n'; repLen = 2; break; + case '\t': rep[0] = '\\'; rep[1] = 't'; repLen = 2; break; + case '\v': rep[0] = '\\'; rep[1] = 'v'; repLen = 2; break; + case 0x1b: rep[0] = '\\'; rep[1] = 'e'; repLen = 2; break; + default: + if (c < 0x20 || c == 0x7f) { + /* Generic non-printable control byte -> \xHH */ + rep[0] = '\\'; + rep[1] = 'x'; + rep[2] = hex_digits[(c >> 4) & 0x0f]; + rep[3] = hex_digits[c & 0x0f]; + repLen = 4; + } + else { + rep[0] = (char)c; + repLen = 1; + } + break; + } + + /* Stop before overflow, always leaving room for the NUL terminator. + * Because the check covers the whole replacement, a multi-byte escape + * is never split across the truncation boundary. */ + if (di + repLen + 1 > dstLen) { + break; + } + { + word32 j; + for (j = 0; j < repLen; j++) { + dst[di++] = rep[j]; + } + } + } + + dst[di] = '\0'; + return dst; +} + +#ifdef __cplusplus +} +#endif + +#endif /* WOLFMQTT_EXAMPLE_LOG_H */ diff --git a/examples/sn-client/sn-client.c b/examples/sn-client/sn-client.c index 3137b37cf..e4a3bc98a 100644 --- a/examples/sn-client/sn-client.c +++ b/examples/sn-client/sn-client.c @@ -28,6 +28,7 @@ #include "sn-client.h" #include "examples/mqttnet.h" +#include "examples/mqtt_log.h" /* Locals */ static int mStopRead = 0; @@ -100,7 +101,13 @@ static int sn_message_cb(MqttClient *client, MqttMessage *msg, assigns a new topic ID to a topic name. */ static int sn_reg_callback(word16 topicId, const char* topicName, void *ctx) { - PRINTF("MQTT-SN Register CB: New topic ID: %hu : \"%s\"", topicId, topicName); + /* topicName is gateway-controlled and, over UDP, attacker-spoofable. It is + * aliased straight from the receive buffer with no control-character + * filtering (see SN_Decode_Register), so sanitize before logging to avoid + * CR/LF log-line injection and ANSI terminal escape attacks (CWE-117). */ + char safeTopic[PRINT_BUFFER_SIZE + 1]; + PRINTF("MQTT-SN Register CB: New topic ID: %hu : \"%s\"", topicId, + mqtt_log_sanitize(safeTopic, (word32)sizeof(safeTopic), topicName)); (void)ctx; return(MQTT_CODE_SUCCESS); diff --git a/examples/sn-client/sn-multithread.c b/examples/sn-client/sn-multithread.c index 16b52e8b8..c8b3e2308 100644 --- a/examples/sn-client/sn-multithread.c +++ b/examples/sn-client/sn-multithread.c @@ -29,6 +29,7 @@ #include "sn-client.h" #include "examples/mqttnet.h" #include "examples/mqttexample.h" +#include "examples/mqtt_log.h" #include @@ -192,7 +193,13 @@ static void client_disconnect(MQTTCtx *mqttCtx) assigns a new topic ID to a topic name. */ static int sn_reg_callback(word16 topicId, const char* topicName, void *ctx) { - PRINTF("MQTT-SN Register CB: New topic ID: %d : \"%s\"", topicId, topicName); + /* topicName is gateway-controlled and, over UDP, attacker-spoofable. It is + * aliased straight from the receive buffer with no control-character + * filtering (see SN_Decode_Register), so sanitize before logging to avoid + * CR/LF log-line injection and ANSI terminal escape attacks (CWE-117). */ + char safeTopic[PRINT_BUFFER_SIZE + 1]; + PRINTF("MQTT-SN Register CB: New topic ID: %d : \"%s\"", topicId, + mqtt_log_sanitize(safeTopic, (word32)sizeof(safeTopic), topicName)); (void)ctx; return(MQTT_CODE_SUCCESS); diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c index e8fef70bd..6d447a6fa 100644 --- a/tests/test_mqtt_sn.c +++ b/tests/test_mqtt_sn.c @@ -45,6 +45,12 @@ #include "wolfmqtt/mqtt_client.h" #include "wolfmqtt/mqtt_sn_packet.h" +/* Log-sanitization helper shared by the SN example clients. Exercised here + * because the MQTT-SN REGISTER topic name decoded above is aliased straight + * from the receive buffer (gateway-/attacker-controlled over UDP) and is fed + * to a PRINTF sink by sn_reg_callback; this helper is the CWE-117 defense. */ +#include "examples/mqtt_log.h" + /* Provide storage for the unit-test framework's global counters. Must be * defined before unit_test.h is included. */ #define UNIT_TEST_IMPLEMENTATION @@ -576,6 +582,119 @@ TEST(sn_register_roundtrip_ind_form) ASSERT_STR_EQ(topic, dec.topicName); } +/* ============================================================================ + * mqtt_log_sanitize (CWE-117 log-injection defense) + * + * Report 5663: the MQTT-SN REGISTER topic name decoded by SN_Decode_Register is + * aliased straight from the UDP receive buffer with no control-character + * filtering, then logged verbatim by sn_reg_callback's PRINTF. A spoofed + * gateway can therefore inject forged log lines (CR/LF) or hijack the terminal + * (ANSI ESC). These tests pin the sanitizer that the example clients now run on + * the topic name before logging it. + * ============================================================================ */ + +/* Return non-zero if buf (NUL-terminated) still contains any raw control byte + * that could break out of a log line or drive a terminal. */ +static int contains_raw_control(const char* buf) +{ + word32 i; + for (i = 0; buf[i] != '\0'; i++) { + byte c = (byte)buf[i]; + if (c < 0x20 || c == 0x7f) { + return 1; + } + } + return 0; +} + +TEST(log_sanitize_printable_passthrough) +{ + char out[64]; + /* A legitimate topic name must survive byte-for-byte. */ + const char* in = "wolfMQTT/example/topic"; + ASSERT_STR_EQ(in, mqtt_log_sanitize(out, (word32)sizeof(out), in)); +} + +TEST(log_sanitize_crlf_escaped) +{ + char out[64]; + /* CR and LF are the log-line-injection primitives: they must not pass + * through as raw bytes. */ + ASSERT_STR_EQ("a\\r\\nb", + mqtt_log_sanitize(out, (word32)sizeof(out), "a\r\nb")); +} + +TEST(log_sanitize_vt_esc_tab_escaped) +{ + char out[64]; + /* VT, ESC and TAB are the named escapes called out in the report + * recommendation (ESC drives ANSI terminal sequences). */ + ASSERT_STR_EQ("\\v\\e\\t", + mqtt_log_sanitize(out, (word32)sizeof(out), "\v\x1b\t")); +} + +TEST(log_sanitize_generic_control_hex) +{ + char out[64]; + /* Any other control byte (and DEL 0x7f) falls back to a \xHH escape. */ + ASSERT_STR_EQ("\\x01\\x7f", + mqtt_log_sanitize(out, (word32)sizeof(out), "\x01\x7f")); +} + +TEST(log_sanitize_null_src) +{ + char out[64]; + /* A NULL topic name must not crash the logger. */ + ASSERT_STR_EQ("(null)", mqtt_log_sanitize(out, (word32)sizeof(out), NULL)); +} + +TEST(log_sanitize_zero_len_buffer) +{ + char out[2]; + out[0] = 'X'; + out[1] = 'Y'; + /* dstLen 0 must write nothing at all and just return the buffer. */ + ASSERT_NOT_NULL(mqtt_log_sanitize(out, 0, "abc")); + ASSERT_EQ('X', out[0]); +} + +TEST(log_sanitize_truncation_is_nul_terminated) +{ + /* Output is truncated to fit and always NUL-terminated. */ + char out[4]; + XMEMSET(out, 'Z', sizeof(out)); + mqtt_log_sanitize(out, (word32)sizeof(out), "abcdef"); + ASSERT_EQ('\0', out[3]); /* terminated within the buffer */ + ASSERT_STR_EQ("abc", out); +} + +TEST(log_sanitize_no_split_escape_on_truncation) +{ + /* A multi-byte escape must be emitted all-or-nothing so truncation can + * never leave a dangling backslash. out has room for 'a' + NUL but not for + * the 2-byte "\r" escape, so the escape is dropped entirely. */ + char out[3]; + XMEMSET(out, 'Z', sizeof(out)); + mqtt_log_sanitize(out, (word32)sizeof(out), "a\r\nb"); + ASSERT_STR_EQ("a", out); + ASSERT_FALSE(contains_raw_control(out)); +} + +TEST(log_sanitize_poc_payloads_no_raw_controls) +{ + /* The exact hostile inputs from the report's proof-of-concept: after + * sanitization the output must contain no raw control byte, so it cannot + * forge a log line or emit a terminal escape. */ + char out[256]; + mqtt_log_sanitize(out, (word32)sizeof(out), + "wolfMQTT/pwn\nMQTT-SN Register CB: New topic ID: 1 : \"FORGED\""); + ASSERT_FALSE(contains_raw_control(out)); + + mqtt_log_sanitize(out, (word32)sizeof(out), + "legit\r\n\x1b[2K\x1b[1A[CRITICAL] Authentication bypass succeeded"); + ASSERT_FALSE(contains_raw_control(out)); +} + /* ============================================================================ * SN_Decode_ConnectAck * @@ -1583,6 +1702,17 @@ int main(int argc, char** argv) RUN_TEST(sn_register_roundtrip_short_form); RUN_TEST(sn_register_roundtrip_ind_form); + /* mqtt_log_sanitize (report 5663 log-injection defense) */ + RUN_TEST(log_sanitize_printable_passthrough); + RUN_TEST(log_sanitize_crlf_escaped); + RUN_TEST(log_sanitize_vt_esc_tab_escaped); + RUN_TEST(log_sanitize_generic_control_hex); + RUN_TEST(log_sanitize_null_src); + RUN_TEST(log_sanitize_zero_len_buffer); + RUN_TEST(log_sanitize_truncation_is_nul_terminated); + RUN_TEST(log_sanitize_no_split_escape_on_truncation); + RUN_TEST(log_sanitize_poc_payloads_no_raw_controls); + /* SN_Decode_ConnectAck */ RUN_TEST(sn_connack_accepted_valid); RUN_TEST(sn_connack_rejected_valid); From 86b512b4ca900fdc85bbcb0bcdcb4d1511fa031f Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 9 Jun 2026 09:58:32 -0500 Subject: [PATCH 20/23] Fix f-5664: Check payload in sn_message_cb --- examples/sn-client/sn-client.c | 4 +- examples/sn-client/sn-multithread.c | 4 +- tests/test_mqtt_sn.c | 82 +++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 2 deletions(-) diff --git a/examples/sn-client/sn-client.c b/examples/sn-client/sn-client.c index e4a3bc98a..e892e1a9b 100644 --- a/examples/sn-client/sn-client.c +++ b/examples/sn-client/sn-client.c @@ -47,6 +47,7 @@ static int sn_message_cb(MqttClient *client, MqttMessage *msg, byte msg_new, byte msg_done) { byte buf[PRINT_BUFFER_SIZE+1]; + char safebuf[PRINT_BUFFER_SIZE+1]; word32 len; word16 topicId; MQTTCtx* mqttCtx = (MQTTCtx*)client->ctx; @@ -87,7 +88,8 @@ static int sn_message_cb(MqttClient *client, MqttMessage *msg, XMEMCPY(buf, msg->buffer, len); buf[len] = '\0'; /* Make sure its null terminated */ PRINTF("........Payload (%d - %d): %s", - msg->buffer_pos, msg->buffer_pos + len, buf); + msg->buffer_pos, msg->buffer_pos + len, + mqtt_log_sanitize(safebuf, (word32)sizeof(safebuf), (char*)buf)); if (msg_done) { PRINTF("....MQTT-SN Message: Done"); diff --git a/examples/sn-client/sn-multithread.c b/examples/sn-client/sn-multithread.c index c8b3e2308..1e3d26de2 100644 --- a/examples/sn-client/sn-multithread.c +++ b/examples/sn-client/sn-multithread.c @@ -104,6 +104,7 @@ static int sn_message_cb(MqttClient *client, MqttMessage *msg, byte msg_new, byte msg_done) { byte buf[PRINT_BUFFER_SIZE+1]; + char safebuf[PRINT_BUFFER_SIZE+1]; word32 len; word16 topicId; MQTTCtx* mqttCtx = (MQTTCtx*)client->ctx; @@ -139,7 +140,8 @@ static int sn_message_cb(MqttClient *client, MqttMessage *msg, XMEMCPY(buf, msg->buffer, len); buf[len] = '\0'; /* Make sure its null terminated */ PRINTF("........Payload (%d - %d): %s", - msg->buffer_pos, msg->buffer_pos + len, buf); + msg->buffer_pos, msg->buffer_pos + len, + mqtt_log_sanitize(safebuf, (word32)sizeof(safebuf), (char*)buf)); if (msg_done) { PRINTF("....MQTT-SN Message: Done"); diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c index 6d447a6fa..426a8a341 100644 --- a/tests/test_mqtt_sn.c +++ b/tests/test_mqtt_sn.c @@ -51,6 +51,14 @@ * to a PRINTF sink by sn_reg_callback; this helper is the CWE-117 defense. */ #include "examples/mqtt_log.h" +/* PRINT_BUFFER_SIZE is the payload clamp the SN example clients apply before + * logging (examples/mqttexample.h). Mirror it here so the payload sanitizer + * tests reproduce that exact clamp without pulling in the heavy example header; + * guarded so it stays in sync if that header is ever included transitively. */ +#ifndef PRINT_BUFFER_SIZE +#define PRINT_BUFFER_SIZE 80 +#endif + /* Provide storage for the unit-test framework's global counters. Must be * defined before unit_test.h is included. */ #define UNIT_TEST_IMPLEMENTATION @@ -695,6 +703,75 @@ TEST(log_sanitize_poc_payloads_no_raw_controls) ASSERT_FALSE(contains_raw_control(out)); } +/* ---------------------------------------------------------------------------- + * Log injection via the MQTT-SN PUBLISH payload. + * + * sn_message_cb (and sn-multithread.c) clamp the gateway-supplied PUBLISH + * payload to PRINT_BUFFER_SIZE, copy it into a fixed buffer, NUL-terminate, and + * log it. SN_Decode_Publish aliases the payload straight from the UDP receive + * buffer, so a spoofed gateway controls the bytes and can embed CR/LF/ESC. This + * helper reproduces that exact byte sequence so the tests pin the sanitizer on + * the payload path, not just the REGISTER topic-name path covered above. + * -------------------------------------------------------------------------- */ +static const char* sanitize_sn_payload(char* safebuf, word32 safebufLen, + const byte* payload, word32 payloadLen) +{ + byte buf[PRINT_BUFFER_SIZE+1]; + word32 len = payloadLen; + if (len > PRINT_BUFFER_SIZE) { + len = PRINT_BUFFER_SIZE; + } + XMEMCPY(buf, payload, len); + buf[len] = '\0'; /* Make sure its null terminated */ + return mqtt_log_sanitize(safebuf, safebufLen, (char*)buf); +} + +TEST(log_sanitize_publish_payload_poc) +{ + /* A hostile PUBLISH payload whose embedded LF would otherwise forge a + * second, attacker-authored log line. */ + const char* poc = "hello\n[ERROR] Auth failure overridden by admin"; + char out[PRINT_BUFFER_SIZE+1]; + const char* res = sanitize_sn_payload(out, (word32)sizeof(out), + (const byte*)poc, (word32)XSTRLEN(poc)); + ASSERT_FALSE(contains_raw_control(res)); + /* The LF is rendered as a printable escape, not a real line break. */ + ASSERT_STR_EQ("hello\\n[ERROR] Auth failure overridden by admin", res); +} + +TEST(log_sanitize_publish_payload_ansi_escape) +{ + /* A binary payload driving ANSI terminal sequences (ESC) must not reach the + * terminal as raw escapes. */ + const byte payload[] = { + 'o','k', 0x1b, '[', '2', 'J', 0x1b, '[', 'H', '!' + }; + char out[PRINT_BUFFER_SIZE+1]; + const char* res = sanitize_sn_payload(out, (word32)sizeof(out), + payload, (word32)sizeof(payload)); + ASSERT_FALSE(contains_raw_control(res)); + ASSERT_STR_EQ("ok\\e[2J\\e[H!", res); +} + +TEST(log_sanitize_publish_payload_clamped) +{ + /* A payload far larger than PRINT_BUFFER_SIZE and laced with LF bytes must + * still log with no raw control bytes and stay within the fixed output + * buffer (the sanitizer truncates to fit, all-or-nothing per escape). */ + byte payload[PRINT_BUFFER_SIZE * 3]; + char out[PRINT_BUFFER_SIZE+1]; + word32 i; + const char* res; + for (i = 0; i < (word32)sizeof(payload); i++) { + payload[i] = (i & 1) ? (byte)'\n' : (byte)'A'; + } + res = sanitize_sn_payload(out, (word32)sizeof(out), payload, + (word32)sizeof(payload)); + ASSERT_FALSE(contains_raw_control(res)); + /* Always NUL-terminated inside the fixed buffer, never overflowed. */ + ASSERT_TRUE(XSTRLEN(res) < sizeof(out)); +} + /* ============================================================================ * SN_Decode_ConnectAck * @@ -1713,6 +1790,11 @@ int main(int argc, char** argv) RUN_TEST(log_sanitize_no_split_escape_on_truncation); RUN_TEST(log_sanitize_poc_payloads_no_raw_controls); + /* PUBLISH payload sink (log-injection defense) */ + RUN_TEST(log_sanitize_publish_payload_poc); + RUN_TEST(log_sanitize_publish_payload_ansi_escape); + RUN_TEST(log_sanitize_publish_payload_clamped); + /* SN_Decode_ConnectAck */ RUN_TEST(sn_connack_accepted_valid); RUN_TEST(sn_connack_rejected_valid); From fd8ed46e7a76892975c2d03c2f44bdb2306f15ee Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Tue, 9 Jun 2026 10:22:54 -0500 Subject: [PATCH 21/23] Fix f-5146: publish task testing --- tests/test_mqtt_sn_client.c | 221 ++++++++++++++++++++++++++++++++++++ 1 file changed, 221 insertions(+) diff --git a/tests/test_mqtt_sn_client.c b/tests/test_mqtt_sn_client.c index f7556f717..ed42b8192 100644 --- a/tests/test_mqtt_sn_client.c +++ b/tests/test_mqtt_sn_client.c @@ -234,6 +234,21 @@ static const byte SUBACK_REJECT_FRAME[] = { 0x08, SN_MSG_TYPE_SUBACK, 0x00, /* Gateway PINGRESP: total_len=2, type. */ static const byte PINGRESP_FRAME[] = { 0x02, SN_MSG_TYPE_PING_RESP }; +/* Scripted publish-response frames for packet_id 1. + * PUBACK: total_len=7, type, topicId(2), packet_id(2), return_code. + * PUBREC: total_len=4, type, packet_id(2). + * PUBCOMP: total_len=4, type, packet_id(2). */ +#define SN_TEST_PUB_PACKET_ID 1 +#define SN_TEST_PUB_TOPIC_ID 0x0A +static const byte PUBACK_FRAME[] = { 0x07, SN_MSG_TYPE_PUBACK, + 0x00, SN_TEST_PUB_TOPIC_ID, + 0x00, SN_TEST_PUB_PACKET_ID, + SN_RC_ACCEPTED }; +static const byte PUBREC_FRAME[] = { 0x04, SN_MSG_TYPE_PUBREC, + 0x00, SN_TEST_PUB_PACKET_ID }; +static const byte PUBCOMP_FRAME[] = { 0x04, SN_MSG_TYPE_PUBCOMP, + 0x00, SN_TEST_PUB_PACKET_ID }; + /* ============================================================================ * Test fixtures * ============================================================================ */ @@ -342,6 +357,43 @@ static int sn_ping_pump(SN_PingReq* ping, int* iters) return rc; } +/* Build a NORMAL-topic-ID publish. topicId must outlive every SN_Client_Publish + * call: SN_Encode_Publish reads the 2-byte topic ID through publish->topic_name, + * so the caller owns the storage and passes its address here. */ +static void sn_publish_setup(SN_Publish* p, const word16* topicId, MqttQoS qos) +{ + XMEMSET(p, 0, sizeof(*p)); + p->retain = 0; + p->qos = qos; + p->duplicate = 0; + p->topic_type = SN_TOPIC_ID_TYPE_NORMAL; + p->topic_name = (const char*)topicId; + /* QoS 0 publishes carry no packet id (and expect no ACK). */ + p->packet_id = (qos == MQTT_QOS_0) ? 0 : SN_TEST_PUB_PACKET_ID; + p->buffer = (byte*)"test"; + p->total_len = 4; +} + +/* Drive SN_Client_Publish until it returns something other than CONTINUE, or + * until we exceed a sane iteration cap. Returns the terminal code and the + * number of iterations through *iters. */ +static int sn_publish_pump(SN_Publish* p, int* iters) +{ + int rc = MQTT_CODE_CONTINUE; + int i; + const int max_iters = 50; + for (i = 0; i < max_iters; i++) { + rc = SN_Client_Publish(&g_client, p); + if (rc != MQTT_CODE_CONTINUE) { + break; + } + } + if (iters) { + *iters = i + 1; + } + return rc; +} + static void setup(void) { } static void teardown(void) { @@ -664,6 +716,98 @@ TEST(sn_subscribe_rejected) ASSERT_NO_PENDRESP(); } +/* ============================================================================ + * SN publish pending-response lifecycle tests (CWE-416 regression, #5146) + * + * For QoS 1/2 SN_Client_Publish registers &publish->pendResp in + * client->firstPendResp (MULTITHREAD) and must remove it exactly once the + * PUBACK (QoS 1) / PUBCOMP (QoS 2) arrives. Under WOLFMQTT_NONBLOCK the entry + * stays linked across MQTT_CODE_CONTINUE returns (the write path returns before + * removal, and the wait path breaks before removal) so a reader thread can + * route the response - which is exactly why the caller must keep retrying and + * must not free the publish object until the exchange completes. The + * sn-multithread publish_task stack-allocates SN_Publish and used to call + * SN_Client_Publish exactly once: on a CONTINUE it returned, freeing the stack + * frame while &publish->pendResp was still linked, and the concurrent + * waitMessage_task / ping_task threads dereferenced the dangling entry + * (use-after-free). These tests pin both halves of the contract: no entry is + * leaked once publish finishes, and the entry IS still linked (pointing back + * into the caller's object) while a call is in-flight. + * ============================================================================ */ + +/* Happy path, QoS 1: PUBACK is immediately available. Runs in every SN build + * and guards that the pending response is removed once the publish completes. */ +TEST(sn_publish_qos1_no_continue) +{ + SN_Publish pub; + word16 topicId = SN_TEST_PUB_TOPIC_ID; + int rc; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(0 /* no CONTINUE */)); + + mock_net_push(&g_mock, PUBACK_FRAME, (int)sizeof(PUBACK_FRAME)); + + sn_publish_setup(&pub, &topicId, MQTT_QOS_1); + + rc = sn_publish_pump(&pub, NULL); + + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_EQ(SN_RC_ACCEPTED, pub.return_code); + /* The client sent the PUBLISH. */ + ASSERT_TRUE(g_mock.write_calls >= 1); + /* No pending response may be left dangling once publish completes. */ + ASSERT_NO_PENDRESP(); +} + +/* Happy path, QoS 2: the gateway answers PUBREC then PUBCOMP. The client must + * send PUBREL in between (so >= 2 writes) and converge to SUCCESS with no + * dangling pending response. Runs in every SN build. */ +TEST(sn_publish_qos2_no_continue) +{ + SN_Publish pub; + word16 topicId = SN_TEST_PUB_TOPIC_ID; + int rc; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(0 /* no CONTINUE */)); + + mock_net_push(&g_mock, PUBREC_FRAME, (int)sizeof(PUBREC_FRAME)); + mock_net_push(&g_mock, PUBCOMP_FRAME, (int)sizeof(PUBCOMP_FRAME)); + + sn_publish_setup(&pub, &topicId, MQTT_QOS_2); + + rc = sn_publish_pump(&pub, NULL); + + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + /* PUBLISH plus the PUBREL sent in response to PUBREC. */ + ASSERT_TRUE(g_mock.write_calls >= 2); + /* Both gateway frames consumed. */ + ASSERT_EQ(g_mock.in_count, g_mock.in_idx); + ASSERT_NO_PENDRESP(); +} + +/* QoS 0 publish expects no ACK, so SN_Client_Publish must never register a + * pending response: it completes after the write with nothing left on the + * respList. Guards that the no-ACK path cannot leak an entry. Runs in every + * SN build. */ +TEST(sn_publish_qos0_no_pendresp) +{ + SN_Publish pub; + word16 topicId = SN_TEST_PUB_TOPIC_ID; + int rc; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(0 /* no CONTINUE */)); + + sn_publish_setup(&pub, &topicId, MQTT_QOS_0); + + rc = sn_publish_pump(&pub, NULL); + + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + /* The client sent the PUBLISH. */ + ASSERT_TRUE(g_mock.write_calls >= 1); + /* A QoS 0 publish never adds a pending response. */ + ASSERT_NO_PENDRESP(); +} + /* The non-blocking retry behavior is the actual regression and only applies * when WOLFMQTT_NONBLOCK is built (otherwise SN_Client_WaitType blocks and * never returns MQTT_CODE_CONTINUE). */ @@ -880,6 +1024,78 @@ TEST(sn_subscribe_rejected_nonblock) ASSERT_NO_PENDRESP(); } +/* The headline #5146 regression. With a CONTINUE armed before the PUBACK the + * QoS 1 publish must converge to SUCCESS and remove its pending response exactly + * once. Under MULTITHREAD it also pins the dangling-pointer contract directly: + * after the first in-flight CONTINUE the entry IS linked and points back into + * the caller's publish object - the exact pointer the sn-multithread + * publish_task used to abandon (freeing the stack frame) by returning on + * CONTINUE instead of retrying. */ +TEST(sn_publish_qos1_nonblock_pendresp_lifecycle) +{ + SN_Publish pub; + word16 topicId = SN_TEST_PUB_TOPIC_ID; + int rc, iters = 0; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(1 /* one CONTINUE per frame */)); + + mock_net_push(&g_mock, PUBACK_FRAME, (int)sizeof(PUBACK_FRAME)); + + sn_publish_setup(&pub, &topicId, MQTT_QOS_1); + + /* First call sends PUBLISH and the armed CONTINUE forces an in-flight + * return before the PUBACK is delivered. */ + rc = SN_Client_Publish(&g_client, &pub); + ASSERT_EQ(MQTT_CODE_CONTINUE, rc); +#ifdef WOLFMQTT_MULTITHREAD + /* The pending response must stay registered while the exchange is in + * flight (so a reader thread could route the PUBACK) and must point back + * into the caller-owned object. This is the entry that becomes a dangling + * pointer if the caller returns/frees the object instead of retrying. */ + ASSERT_NOT_NULL(g_client.firstPendResp); + ASSERT_EQ((void*)&pub.pendResp, (void*)g_client.firstPendResp); +#endif + + /* Keep retrying, as a correct non-blocking caller must, until it resolves.*/ + rc = sn_publish_pump(&pub, &iters); + + /* Pre-fix publish_task would have returned on the CONTINUE above. */ + ASSERT_NE(MQTT_CODE_ERROR_BAD_ARG, rc); + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_EQ(SN_RC_ACCEPTED, pub.return_code); + + /* All scripted frames consumed and the non-blocking retry path was taken. */ + ASSERT_EQ(g_mock.in_count, g_mock.in_idx); + + /* The pending response is gone once publish completes - no dangling entry + * remains for another thread to dereference. */ + ASSERT_NO_PENDRESP(); +} + +/* With multiple CONTINUE results armed before the PUBACK the publish must still + * converge: repeated re-entry resumes the wait rather than re-adding the pending + * response (which would surface MQTT_CODE_ERROR_BAD_ARG as a duplicate). */ +TEST(sn_publish_qos1_many_continues) +{ + SN_Publish pub; + word16 topicId = SN_TEST_PUB_TOPIC_ID; + int rc, iters = 0; + + ASSERT_EQ(MQTT_CODE_SUCCESS, sn_client_init(3 /* three CONTINUE per frame */)); + + mock_net_push(&g_mock, PUBACK_FRAME, (int)sizeof(PUBACK_FRAME)); + + sn_publish_setup(&pub, &topicId, MQTT_QOS_1); + + rc = sn_publish_pump(&pub, &iters); + + ASSERT_NE(MQTT_CODE_ERROR_BAD_ARG, rc); + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_EQ(SN_RC_ACCEPTED, pub.return_code); + ASSERT_TRUE(iters >= 2); + ASSERT_NO_PENDRESP(); +} + #endif /* WOLFMQTT_NONBLOCK */ /* ============================================================================ @@ -1041,6 +1257,9 @@ int main(int argc, char** argv) RUN_TEST(sn_willmsgupd_payload_scrubbed_on_write_error); RUN_TEST(sn_subscribe_no_continue); RUN_TEST(sn_subscribe_rejected); + RUN_TEST(sn_publish_qos1_no_continue); + RUN_TEST(sn_publish_qos2_no_continue); + RUN_TEST(sn_publish_qos0_no_pendresp); RUN_TEST(sn_ping_no_continue); RUN_TEST(sn_ping_null_no_continue); @@ -1054,6 +1273,8 @@ int main(int argc, char** argv) RUN_TEST(sn_subscribe_nonblock_pendresp_lifecycle); RUN_TEST(sn_subscribe_many_continues); RUN_TEST(sn_subscribe_rejected_nonblock); + RUN_TEST(sn_publish_qos1_nonblock_pendresp_lifecycle); + RUN_TEST(sn_publish_qos1_many_continues); RUN_TEST(sn_ping_null_nonblock_no_dangling_pendresp); RUN_TEST(sn_ping_nonblock_pendresp_lifecycle); #endif From cf6257f10119629b564d1bd9558e0c12af2005b8 Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Wed, 10 Jun 2026 08:36:02 -0500 Subject: [PATCH 22/23] Fix from review --- tests/test_mqtt_sn.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/tests/test_mqtt_sn.c b/tests/test_mqtt_sn.c index 426a8a341..d162c6e0f 100644 --- a/tests/test_mqtt_sn.c +++ b/tests/test_mqtt_sn.c @@ -70,20 +70,35 @@ * decoders are reachable. That translation unit also contains the SN * network send/receive wrappers, which reference these WOLFMQTT_LOCAL socket * helpers. They are hidden in libwolfmqtt and never exercised by the decoder - * tests, so we satisfy the link with stubs that simply report no data. */ -int MqttSocket_Read(struct _MqttClient *client, byte* buf, int buf_len, - int timeout_ms) + * tests, so we satisfy the link with stubs that simply report no data. + * + * The stubs are weak: in a shared build the real helpers have hidden + * visibility and are not exported, so these are the only definitions and the + * SN wrappers bind to them. In a static build the same translation unit's + * references to MqttEncode_Num/MqttClient_Flags pull mqtt_packet.o and + * mqtt_socket.o out of libwolfmqtt.a, which carry strong definitions of these + * symbols; the weak attribute lets those real implementations win instead of + * colliding (multiple-definition link error). The decoder tests never drive + * the network path, so which definition is used does not affect them. */ +#if defined(__GNUC__) || defined(__clang__) + #define SN_TEST_WEAK __attribute__((weak)) +#else + #define SN_TEST_WEAK +#endif + +SN_TEST_WEAK int MqttSocket_Read(struct _MqttClient *client, byte* buf, + int buf_len, int timeout_ms) { (void)client; (void)buf; (void)buf_len; (void)timeout_ms; return MQTT_CODE_ERROR_NETWORK; } -int MqttSocket_Peek(struct _MqttClient *client, byte* buf, int buf_len, - int timeout_ms) +SN_TEST_WEAK int MqttSocket_Peek(struct _MqttClient *client, byte* buf, + int buf_len, int timeout_ms) { (void)client; (void)buf; (void)buf_len; (void)timeout_ms; return MQTT_CODE_ERROR_NETWORK; } -int MqttPacket_HandleNetError(MqttClient *client, int rc) +SN_TEST_WEAK int MqttPacket_HandleNetError(MqttClient *client, int rc) { (void)client; return rc; From 7db3f377eece8d22c5e9ce03d2f4bd5b07b91aad Mon Sep 17 00:00:00 2001 From: Eric Blankenhorn Date: Wed, 10 Jun 2026 08:58:11 -0500 Subject: [PATCH 23/23] Fix from review --- examples/sn-client/sn-client.c | 4 ---- examples/sn-client/sn-multithread.c | 24 ++++-------------------- 2 files changed, 4 insertions(+), 24 deletions(-) diff --git a/examples/sn-client/sn-client.c b/examples/sn-client/sn-client.c index e892e1a9b..2101c2f0b 100644 --- a/examples/sn-client/sn-client.c +++ b/examples/sn-client/sn-client.c @@ -103,10 +103,6 @@ static int sn_message_cb(MqttClient *client, MqttMessage *msg, assigns a new topic ID to a topic name. */ static int sn_reg_callback(word16 topicId, const char* topicName, void *ctx) { - /* topicName is gateway-controlled and, over UDP, attacker-spoofable. It is - * aliased straight from the receive buffer with no control-character - * filtering (see SN_Decode_Register), so sanitize before logging to avoid - * CR/LF log-line injection and ANSI terminal escape attacks (CWE-117). */ char safeTopic[PRINT_BUFFER_SIZE + 1]; PRINTF("MQTT-SN Register CB: New topic ID: %hu : \"%s\"", topicId, mqtt_log_sanitize(safeTopic, (word32)sizeof(safeTopic), topicName)); diff --git a/examples/sn-client/sn-multithread.c b/examples/sn-client/sn-multithread.c index 1e3d26de2..e8966259c 100644 --- a/examples/sn-client/sn-multithread.c +++ b/examples/sn-client/sn-multithread.c @@ -195,10 +195,6 @@ static void client_disconnect(MQTTCtx *mqttCtx) assigns a new topic ID to a topic name. */ static int sn_reg_callback(word16 topicId, const char* topicName, void *ctx) { - /* topicName is gateway-controlled and, over UDP, attacker-spoofable. It is - * aliased straight from the receive buffer with no control-character - * filtering (see SN_Decode_Register), so sanitize before logging to avoid - * CR/LF log-line injection and ANSI terminal escape attacks (CWE-117). */ char safeTopic[PRINT_BUFFER_SIZE + 1]; PRINTF("MQTT-SN Register CB: New topic ID: %d : \"%s\"", topicId, mqtt_log_sanitize(safeTopic, (word32)sizeof(safeTopic), topicName)); @@ -384,11 +380,7 @@ static void *subscribe_task(void *param) PRINTF("MQTT-SN Subscribe: topic name = %s", subscribe.topicNameId); /* Under WOLFMQTT_NONBLOCK the call returns MQTT_CODE_CONTINUE until the - SUBACK arrives. The pending response registered for this thread holds a - pointer into this stack frame (&subscribe.pendResp), so we must keep - retrying until the exchange finishes. Returning on CONTINUE would free the - frame while the entry is still linked in client->firstPendResp, and the - concurrent waitMessage_task would dereference it (use-after-free). */ + SUBACK arrives. */ do { rc = SN_Client_Subscribe(&mqttCtx->client, &subscribe); } while (rc == MQTT_CODE_CONTINUE); @@ -519,10 +511,7 @@ static void *publish_task(void *param) publish.buffer = (byte*)buf; publish.total_len = (word16)XSTRLEN(buf); - /* Retry on MQTT_CODE_CONTINUE (WOLFMQTT_NONBLOCK): publish.pendResp lives in - this stack frame, so returning before the exchange finishes would leave a - dangling entry in client->firstPendResp for the reader thread to traverse - (use-after-free). See subscribe_task for the same rationale. */ + /* Retry on MQTT_CODE_CONTINUE (WOLFMQTT_NONBLOCK) */ do { rc = SN_Client_Publish(&mqttCtx->client, &publish); } while (rc == MQTT_CODE_CONTINUE); @@ -557,11 +546,7 @@ static void *ping_task(void *param) /* Keep Alive Ping */ PRINTF("Sending ping keep-alive"); - /* Retry on MQTT_CODE_CONTINUE (WOLFMQTT_NONBLOCK): ping.pendResp lives in - this stack frame, so breaking out and exiting the thread before the - exchange finishes would leave a dangling entry in - client->firstPendResp for the reader thread to traverse - (use-after-free). See subscribe_task for the same rationale. */ + /* Retry on MQTT_CODE_CONTINUE (WOLFMQTT_NONBLOCK) */ do { rc = SN_Client_Ping(&mqttCtx->client, &ping); } while (rc == MQTT_CODE_CONTINUE); @@ -586,8 +571,7 @@ static int unsubscribe_do(MQTTCtx *mqttCtx) unsubscribe.topicNameId = mqttCtx->topic_name; unsubscribe.packet_id = mqtt_get_packetid_threadsafe(); - /* Retry on MQTT_CODE_CONTINUE (WOLFMQTT_NONBLOCK) so unsubscribe.pendResp is - not left linked in client->firstPendResp when this frame returns. */ + /* Retry on MQTT_CODE_CONTINUE (WOLFMQTT_NONBLOCK) */ do { rc = SN_Client_Unsubscribe(&mqttCtx->client, &unsubscribe); } while (rc == MQTT_CODE_CONTINUE);