diff --git a/examples/mqttsimple/mqttsimple.c b/examples/mqttsimple/mqttsimple.c index ef23c0852..fd1bf541a 100644 --- a/examples/mqttsimple/mqttsimple.c +++ b/examples/mqttsimple/mqttsimple.c @@ -411,6 +411,7 @@ int mqttsimple_test(void) /* Subscribe and wait for Ack */ XMEMSET(&mqttObj, 0, sizeof(mqttObj)); + XMEMSET(topics, 0, sizeof(topics)); topics[0].topic_filter = MQTT_TOPIC_NAME; topics[0].qos = MQTT_QOS; mqttObj.subscribe.packet_id = mqtt_get_packetid(); diff --git a/examples/websocket/websocket_client.c b/examples/websocket/websocket_client.c index 3db8d463b..c7bd059fa 100644 --- a/examples/websocket/websocket_client.c +++ b/examples/websocket/websocket_client.c @@ -181,6 +181,7 @@ int main(int argc, char *argv[]) MqttSubscribe subscribe; MqttTopic topics[1]; XMEMSET(&subscribe, 0, sizeof(subscribe)); + XMEMSET(topics, 0, sizeof(topics)); topics[0].topic_filter = "test/topic"; topics[0].qos = MQTT_QOS_0; subscribe.packet_id = mqtt_get_packetid(); diff --git a/src/mqtt_broker.c b/src/mqtt_broker.c index 9f555ef32..898455b84 100644 --- a/src/mqtt_broker.c +++ b/src/mqtt_broker.c @@ -4967,6 +4967,26 @@ static int BrokerHandle_Connect(BrokerClient* bc, int rx_len, } #endif + /* CONNECT accepted. Authentication is complete, and neither the plaintext + * CONNECT still in bc->rx_buf (mc.username/mc.password were in-place + * pointers into it) nor the dedicated bc->password copy is read again for + * the life of this connection - the broker has no re-auth path. Scrub both + * now so credentials reside in memory only for the duration of CONNECT + * processing rather than the whole session. Refused/failed CONNECTs return + * earlier and are wiped when the caller frees the client. Mirrors the + * client-side CLIENT_FORCE_ZERO hardening in mqtt_client.c. */ + BROKER_FORCE_ZERO(bc->rx_buf, (word32)rx_len); +#ifdef WOLFMQTT_BROKER_AUTH +#ifdef WOLFMQTT_STATIC_MEMORY + BROKER_FORCE_ZERO(bc->password, BROKER_MAX_PASSWORD_LEN); +#else + if (bc->password != NULL) { + BROKER_FORCE_ZERO(bc->password, (size_t)bc->password_len + 1); + } +#endif + bc->password_len = 0; +#endif + return rc; } diff --git a/src/mqtt_client.c b/src/mqtt_client.c index 1b8a91b49..5ae009394 100644 --- a/src/mqtt_client.c +++ b/src/mqtt_client.c @@ -1968,6 +1968,33 @@ int MqttClient_Connect(MqttClient *client, MqttConnect *mc_connect) } #endif +#ifdef WOLFMQTT_V5 + /* Scrub the decoded v5 CONNACK from rx_buf. Its properties (e.g. the + * MQTT_PROP_AUTH_DATA SASL server-final blob used by enhanced + * authentication) decode as pointers into rx_buf and would otherwise + * linger until the next read overwrites them - for an idle or QoS-0-only + * client, potentially the process lifetime. MqttClient_WaitType already + * delivered and freed the property list above, so the bytes are consumed + * and no live pointer into rx_buf remains. Same hardening as + * MqttClient_Auth; v3.1.1 CONNACK carries no properties so gate on v5. */ + if (mc_connect->protocol_level > MQTT_CONNECT_PROTOCOL_LEVEL_4) { + #ifdef WOLFMQTT_MULTITHREAD + /* Hold lockRecv so the scrub cannot race a concurrent rx_buf read. If + * the lock cannot be taken, still scrub: leaving the AUTH_DATA + * plaintext behind is worse than an unsynchronized wipe. */ + if (wm_SemLock(&client->lockRecv) == 0) { + CLIENT_FORCE_ZERO(client->rx_buf, client->rx_buf_len); + wm_SemUnlock(&client->lockRecv); + } + else { + CLIENT_FORCE_ZERO(client->rx_buf, client->rx_buf_len); + } + #else + CLIENT_FORCE_ZERO(client->rx_buf, client->rx_buf_len); + #endif + } +#endif + /* reset state */ mc_connect->stat.write = MQTT_MSG_BEGIN; @@ -3004,11 +3031,16 @@ int MqttClient_Auth(MqttClient *client, MqttAuth* auth) * already delivered and freed auth->props, so the bytes are consumed * before this scrub and the caller has no live pointer into rx_buf. */ #ifdef WOLFMQTT_MULTITHREAD - /* Hold lockRecv so the scrub cannot race a concurrent read into rx_buf. */ + /* Hold lockRecv so the scrub cannot race a concurrent read into rx_buf. If + * the lock cannot be taken, still scrub: leaving the AUTH_DATA plaintext + * behind is worse than an unsynchronized wipe. */ if (wm_SemLock(&client->lockRecv) == 0) { CLIENT_FORCE_ZERO(client->rx_buf, client->rx_buf_len); wm_SemUnlock(&client->lockRecv); } + else { + CLIENT_FORCE_ZERO(client->rx_buf, client->rx_buf_len); + } #else CLIENT_FORCE_ZERO(client->rx_buf, client->rx_buf_len); #endif diff --git a/src/mqtt_packet.c b/src/mqtt_packet.c index 8b5ea3156..fc51f0991 100644 --- a/src/mqtt_packet.c +++ b/src/mqtt_packet.c @@ -2567,11 +2567,28 @@ int MqttEncode_Subscribe(byte *tx_buf, int tx_buf_len, /* Encode payload */ for (i = 0; i < subscribe->topic_count; i++) { + byte options; topic = &subscribe->topics[i]; tx_payload += MqttEncode_String(tx_payload, topic->topic_filter); + options = (byte)topic->qos; + #ifdef WOLFMQTT_V5 + /* [MQTT v5 3.8.3.1] merge No Local, Retain As Published and Retain + * Handling (bits 2-5). These bits are reserved (MUST be 0) in + * v3.1.1, so only fold them in for a v5 SUBSCRIBE. */ + if (subscribe->protocol_level >= MQTT_CONNECT_PROTOCOL_LEVEL_5) { + /* Reject options the decoder would treat as malformed: bits + * outside 2-5 must be 0 and Retain Handling (bits 4-5) = 3 is + * reserved, so the encoder cannot emit an undecodable SUBSCRIBE. */ + if ((topic->sub_options & ~MQTT_SUBSCRIBE_OPTIONS_MASK) != 0 || + ((topic->sub_options >> 4) & 0x03) == 0x03) { + return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA); + } + options |= (byte)(topic->sub_options & MQTT_SUBSCRIBE_OPTIONS_MASK); + } + #endif /* Sanity check for compilers */ if (tx_payload != NULL) { - *tx_payload = topic->qos; + *tx_payload = options; } tx_payload++; } @@ -2713,6 +2730,14 @@ int MqttDecode_Subscribe(byte *rx_buf, int rx_buf_len, MqttSubscribe *subscribe) } } topic->qos = (MqttQoS)(options & 0x03); + #ifdef WOLFMQTT_V5 + /* Preserve v5 No Local / Retain As Published / Retain Handling + * (bits 2-5) so the full options byte round-trips. Reserved and + * QoS bits were validated above; for a valid v3.1.1 packet these + * bits are 0. The broker stores these but does not yet enforce + * them on delivery. */ + topic->sub_options = (byte)(options & MQTT_SUBSCRIBE_OPTIONS_MASK); + #endif subscribe->topic_count++; } diff --git a/tests/test_broker_connect.c b/tests/test_broker_connect.c index 5f72df10a..280f83fa9 100644 --- a/tests/test_broker_connect.c +++ b/tests/test_broker_connect.c @@ -539,7 +539,7 @@ TEST(connect_v311_binary_password_with_embedded_nul_refused) reset_mock_state(connect, sizeof(connect)); run_broker_one_connect(&broker); - /* Auth must fail - CONNACK return code 0x05 (Not Authorized) and the + /* Auth must fail - CONNACK return code 0x04 (Bad user/pass) and the * connection is closed. Pre-fix, XSTRLEN truncation would let this * authenticate and emit return code 0x00. */ ASSERT_TRUE(g_out_len >= 4); @@ -590,6 +590,53 @@ TEST(connect_v311_binary_password_exact_match_accepted) MqttBroker_Free(&broker); } +/* Guard the length-fold backstop in BrokerBufCompare. The constant-time + * byte loop clamps an out-of-range index to position 0, so it cannot see a + * length mismatch when the shorter input's bytes repeat through the longer + * one. Username "a" against configured "aaaaa" must be refused by the length + * fold alone; deleting the fold would authenticate it. */ +TEST(connect_auth_username_length_fold_repeating_byte_refused) +{ + MqttBroker broker; + MqttBrokerNet net; + /* CONNECT v3.1.1, ClientId "id", Username "a" (len 1), Password + * "aaaaa" (len 5, exact match for auth_pass). connect_flags 0xC2 = + * username + password + clean session. */ + static const byte connect[] = { + 0x10, 24, /* fixed header, rl 24 */ + 0x00, 0x04, 'M', 'Q', 'T', 'T', /* protocol name */ + 0x04, /* level 4 (v3.1.1) */ + 0xC2, /* flags: user+pass+clean */ + 0x00, 0x3C, /* keep-alive 60 */ + 0x00, 0x02, 'i', 'd', /* ClientId "id" */ + 0x00, 0x01, 'a', /* Username "a" */ + 0x00, 0x05, 'a', 'a', 'a', 'a', 'a' /* Password "aaaaa" */ + }; + + install_mock_net(&net); + XMEMSET(&broker, 0, sizeof(broker)); + ASSERT_EQ(MQTT_CODE_SUCCESS, MqttBroker_Init(&broker, &net)); + broker.auth_user = "aaaaa"; + broker.auth_pass = "aaaaa"; + ASSERT_EQ(MQTT_CODE_SUCCESS, MqttBroker_Start(&broker)); + + reset_mock_state(connect, sizeof(connect)); + run_broker_one_connect(&broker); + + /* Auth must fail - CONNACK return code 0x04 (Bad user/pass) and the + * connection closed. Deleting the length fold would authenticate the + * shorter repeating-byte username and emit return code 0x00. */ + ASSERT_TRUE(g_out_len >= 4); + ASSERT_EQ(0x20, g_out_buf[0]); + ASSERT_EQ(0x02, g_out_buf[1]); + ASSERT_EQ(0x00, g_out_buf[2]); + ASSERT_EQ(MQTT_CONNECT_ACK_CODE_REFUSED_BAD_USER_PWD, g_out_buf[3]); + ASSERT_TRUE(g_client_closed); + + MqttBroker_Stop(&broker); + MqttBroker_Free(&broker); +} + /* An unauthenticated CONNECT must not mutate another * client's session. A victim authenticates and stays connected; an attacker * then reuses the victim's client_id with a wrong password. The broker must @@ -717,7 +764,7 @@ TEST(connect_auth_partial_config_fails_closed) /* Pre-fix the username matched and the absent password was never * checked, so the broker accepted (return code 0x00). The gate must now - * refuse with 0x05 (Bad user/pass) and close the connection. */ + * refuse with 0x04 (Bad user/pass) and close the connection. */ ASSERT_TRUE(g_out_len >= 4); ASSERT_EQ(0x20, g_out_buf[0]); ASSERT_EQ(0x02, g_out_buf[1]); @@ -761,7 +808,7 @@ TEST(connect_auth_partial_pass_only_fails_closed) run_broker_one_connect(&broker); /* Pre-fix the password matched and the username was never checked, so the - * broker accepted (return code 0x00). The gate must now refuse with 0x05 + * broker accepted (return code 0x00). The gate must now refuse with 0x04 * (Bad user/pass) and close the connection. */ ASSERT_TRUE(g_out_len >= 4); ASSERT_EQ(0x20, g_out_buf[0]); @@ -928,6 +975,86 @@ static BrokerClient* find_broker_client(MqttBroker* broker, const char* id) return NULL; } +#ifdef WOLFMQTT_BROKER_AUTH +/* Return 1 if the byte sequence `needle` (length nlen) appears anywhere in + * the first hlen bytes of `hay`, else 0. Used to prove credential plaintext + * has been scrubbed from a buffer. */ +static int region_contains(const byte* hay, int hlen, + const char* 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; +} + +/* After an accepted CONNECT the plaintext credentials must not linger for + * the connection lifetime. BrokerHandle_Connect scrubs bc->rx_buf and + * bc->password on the accepted path; verify the password is gone from rx_buf + * and bc->password_len is cleared. Dynamic-memory only. */ +TEST(connect_credentials_scrubbed_after_accept) +{ + MqttBroker broker; + MqttBrokerNet net; + BrokerClient* bc; + /* CONNECT v3.1.1, ClientId "id", Username "alice", Password "s3cr3tPW". + * connect_flags 0xC2 = username + password + clean session. */ + static const byte connect[] = { + 0x10, 31, + 0x00, 0x04, 'M', 'Q', 'T', 'T', + 0x04, + 0xC2, + 0x00, 0x3C, + 0x00, 0x02, 'i', 'd', + 0x00, 0x05, 'a', 'l', 'i', 'c', 'e', + 0x00, 0x08, 's', '3', 'c', 'r', '3', 't', 'P', 'W' + }; + + install_mock_net(&net); + XMEMSET(&broker, 0, sizeof(broker)); + ASSERT_EQ(MQTT_CODE_SUCCESS, MqttBroker_Init(&broker, &net)); + broker.auth_user = "alice"; + broker.auth_pass = "s3cr3tPW"; + ASSERT_EQ(MQTT_CODE_SUCCESS, MqttBroker_Start(&broker)); + + reset_mock_state(connect, sizeof(connect)); + run_broker_one_connect(&broker); + + /* Auth accepted (0x00) and the client is retained. */ + ASSERT_TRUE(g_out_len >= 4); + ASSERT_EQ(MQTT_CONNECT_ACK_CODE_ACCEPTED, g_out_buf[3]); + ASSERT_FALSE(g_client_closed); + + bc = find_broker_client(&broker, "id"); + ASSERT_TRUE(bc != NULL); + + /* Password copy wiped: verify the length is cleared AND the buffer bytes + * are zeroed as independent checks - a regression that dropped only the + * buffer wipe while keeping password_len = 0 must still be caught. The + * dynamic store allocates password_len+1 bytes, so scanning the 8-byte + * plaintext stays within the allocation. */ + ASSERT_EQ(0, (int)bc->password_len); + ASSERT_EQ(0, region_contains((const byte*)bc->password, 8, "s3cr3tPW", 8)); + /* Both credential fields lived in the decoded CONNECT; neither plaintext + * may remain in the receive buffer. Scan only the received packet region + * (== the scrubbed rx_len); rx_buf is malloc'd without zeroing, so the + * bytes past the packet are uninitialized and must not be read. */ + ASSERT_EQ(0, region_contains(bc->rx_buf, (int)sizeof(connect), + "s3cr3tPW", 8)); + ASSERT_EQ(0, region_contains(bc->rx_buf, (int)sizeof(connect), + "alice", 5)); + + MqttBroker_Stop(&broker); + MqttBroker_Free(&broker); +} +#endif /* WOLFMQTT_BROKER_AUTH */ + #ifdef WOLFMQTT_V5 /* Return the Reason Code byte of the first DISCONNECT packet in a captured * stream, or -1 if none carries a reason code. A v5 DISCONNECT with a @@ -3201,11 +3328,15 @@ int main(int argc, char** argv) #ifdef WOLFMQTT_BROKER_AUTH RUN_TEST(connect_v311_binary_password_with_embedded_nul_refused); RUN_TEST(connect_v311_binary_password_exact_match_accepted); + RUN_TEST(connect_auth_username_length_fold_repeating_byte_refused); RUN_TEST(connect_unauth_client_id_does_not_take_over_victim); RUN_TEST(connect_auth_user_only_start_rejected); RUN_TEST(connect_auth_pass_only_start_rejected); RUN_TEST(connect_auth_partial_config_fails_closed); RUN_TEST(connect_auth_partial_pass_only_fails_closed); +#ifndef WOLFMQTT_STATIC_MEMORY + RUN_TEST(connect_credentials_scrubbed_after_accept); +#endif #endif #ifdef WOLFMQTT_V5 RUN_TEST(connect_v5_emptyid_assigned_id_emitted); diff --git a/tests/test_mqtt_client.c b/tests/test_mqtt_client.c index f98029c8c..406faaae7 100644 --- a/tests/test_mqtt_client.c +++ b/tests/test_mqtt_client.c @@ -590,6 +590,57 @@ TEST(connect_accepted_connack_latches_v5_props) ASSERT_EQ(1024, test_client.packet_sz_max); } +/* An accepted v5 CONNACK can carry MQTT_PROP_AUTH_DATA, which decodes as a + * pointer into rx_buf with no copy. After MqttClient_Connect returns that + * plaintext must not linger in rx_buf. Feed a CONNACK with a distinctive + * AUTH_DATA blob and assert it is scrubbed. */ +TEST(connect_v5_scrubs_connack_auth_data_from_rx_buf) +{ + int rc; + int i; + MqttConnect connect; + /* CONNACK v5: type=0x20, remain=0x14, flags=0x00, return_code=0x00, + * prop_len=0x11, then AUTH_DATA (0x16) binary length 14 = + * "SASLfinalPROOF". */ + static const byte connack[] = { + 0x20, 0x14, 0x00, 0x00, 0x11, + 0x16, 0x00, 0x0E, + 'S', 'A', 'S', 'L', 'f', 'i', 'n', 'a', 'l', 'P', 'R', 'O', 'O', 'F' + }; + static const char blob[] = "SASLfinalPROOF"; + const int blob_len = (int)sizeof(blob) - 1; + + rc = test_init_client(); + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + test_client.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5; + + test_net.write = mock_net_write_accept; + test_net.read = mock_net_read_canned; + XMEMCPY(g_canned_buf, connack, sizeof(connack)); + g_canned_len = (int)sizeof(connack); + g_canned_pos = 0; + + XMEMSET(&connect, 0, sizeof(connect)); + connect.keep_alive_sec = 60; + connect.clean_session = 1; + connect.client_id = "test_client"; + + rc = MQTT_CODE_CONTINUE; + for (i = 0; i < 10 && rc == MQTT_CODE_CONTINUE; i++) { + rc = MqttClient_Connect(&test_client, &connect); + } + + /* Success + accepted proves the AUTH_DATA CONNACK was decoded (and thus + * was resident in rx_buf). */ + ASSERT_EQ(MQTT_CODE_SUCCESS, rc); + ASSERT_EQ(MQTT_CONNECT_ACK_CODE_ACCEPTED, connect.ack.return_code); + /* The AUTH_DATA plaintext must not remain anywhere in rx_buf. rx_buf was + * zero-initialized at setup, so scanning the full length reads only + * initialized bytes. */ + ASSERT_FALSE(buf_contains(test_client.rx_buf, test_client.rx_buf_len, + blob, blob_len)); +} + /* MQTT v5: a refused CONNACK (non-zero return code) must NOT mutate long-lived * client state even when it carries server properties, otherwise a rejected or * malicious broker could shrink the client's packet-size cap or lower its QoS @@ -1847,6 +1898,7 @@ void run_mqtt_client_tests(void) RUN_TEST(connect_refused_connack_returns_connect_refused); #if defined(WOLFMQTT_V5) RUN_TEST(connect_accepted_connack_latches_v5_props); + RUN_TEST(connect_v5_scrubs_connack_auth_data_from_rx_buf); RUN_TEST(connect_refused_connack_preserves_v5_defaults); RUN_TEST(connect_accepted_connack_clamps_illegal_max_qos); #endif diff --git a/tests/test_mqtt_packet.c b/tests/test_mqtt_packet.c index ff63f828f..3eea48884 100644 --- a/tests/test_mqtt_packet.c +++ b/tests/test_mqtt_packet.c @@ -1849,6 +1849,83 @@ TEST(encode_subscribe_options_byte_qos2) ASSERT_EQ(0x02, tx_buf[rc - 1]); } +#ifdef WOLFMQTT_V5 +/* The v5 option bits (bits 2-5) are reserved on a v3.1.1 SUBSCRIBE and MUST + * be 0. With protocol_level < 5 the encoder must ignore sub_options and emit + * a QoS-only options byte. */ +TEST(encode_subscribe_v311_ignores_sub_options) +{ + byte tx_buf[256]; + MqttSubscribe sub; + MqttTopic topic; + int rc; + + XMEMSET(&sub, 0, sizeof(sub)); + XMEMSET(&topic, 0, sizeof(topic)); + topic.topic_filter = "a"; + topic.qos = MQTT_QOS_1; + /* Request every v5 option bit; a v3.1.1 encode must drop them all. */ + topic.sub_options = MQTT_SUBSCRIBE_NO_LOCAL | + MQTT_SUBSCRIBE_RETAIN_AS_PUBLISHED | + MQTT_SUBSCRIBE_RETAIN_HANDLING_2; + sub.topics = &topic; + sub.topic_count = 1; + sub.packet_id = 1; + sub.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_4; + rc = MqttEncode_Subscribe(tx_buf, (int)sizeof(tx_buf), &sub); + ASSERT_TRUE(rc > 0); + /* Options byte is the last byte: QoS 1 only, no v5 option bits. */ + ASSERT_EQ(0x01, tx_buf[rc - 1]); +} + +/* Retain Handling = 3 is reserved [MQTT v5 3.8.3.1] and the decoder rejects + * it, so the encoder must refuse it too rather than emit a packet its own + * decoder would reject. */ +TEST(encode_subscribe_v5_retain_handling_3_rejected) +{ + byte tx_buf[256]; + MqttSubscribe sub; + MqttTopic topic; + int rc; + + XMEMSET(&sub, 0, sizeof(sub)); + XMEMSET(&topic, 0, sizeof(topic)); + topic.topic_filter = "a"; + topic.qos = MQTT_QOS_1; + /* Bits 4-5 both set is Retain Handling = 3 (reserved). */ + topic.sub_options = MQTT_SUBSCRIBE_RETAIN_HANDLING_1 | + MQTT_SUBSCRIBE_RETAIN_HANDLING_2; + sub.topics = &topic; + sub.topic_count = 1; + sub.packet_id = 1; + sub.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5; + rc = MqttEncode_Subscribe(tx_buf, (int)sizeof(tx_buf), &sub); + ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); +} + +/* Bits 6-7 are reserved [MQTT v5 3.8.3.1]; sub_options should only hold bits + * 2-5, so any bit outside that mask must be refused by the encoder. */ +TEST(encode_subscribe_v5_reserved_sub_options_bit_rejected) +{ + byte tx_buf[256]; + MqttSubscribe sub; + MqttTopic topic; + int rc; + + XMEMSET(&sub, 0, sizeof(sub)); + XMEMSET(&topic, 0, sizeof(topic)); + topic.topic_filter = "a"; + topic.qos = MQTT_QOS_1; + topic.sub_options = 0x40; /* reserved bit 6 */ + sub.topics = &topic; + sub.topic_count = 1; + sub.packet_id = 1; + sub.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5; + rc = MqttEncode_Subscribe(tx_buf, (int)sizeof(tx_buf), &sub); + ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); +} +#endif /* WOLFMQTT_V5 */ + /* f-2360: topic_filter with strlen > 65535 must be rejected with a negative * return. Guards the unchecked tx_payload += MqttEncode_String(...) in the * SUBSCRIBE payload loop. */ @@ -3485,6 +3562,70 @@ TEST(decode_subscribe_v5_options_reserved_bits_rejected) rc = MqttDecode_Subscribe(rx_buf, (int)sizeof(rx_buf), &sub); ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc); } + +/* The v5 SUBSCRIBE options byte carries No Local, Retain As Published and + * Retain Handling (bits 2-5) alongside QoS (bits 0-1). Encode then decode + * must preserve the full options byte via MqttTopic.sub_options. Covers + * every valid combination: QoS 0-2 x NL x RAP x Retain Handling 0-2. */ +TEST(subscribe_v5_options_roundtrip_all_combos) +{ + static const byte rh_vals[] = { + MQTT_SUBSCRIBE_RETAIN_HANDLING_0, + MQTT_SUBSCRIBE_RETAIN_HANDLING_1, + MQTT_SUBSCRIBE_RETAIN_HANDLING_2 + }; + byte tx_buf[64]; + MqttSubscribe sub; + MqttTopic topic; + MqttSubscribe dec; + MqttTopic dec_topic[1]; + int qos, nl, rap, rh_i; + int enc_len, dec_len; + byte want_opts; + + for (qos = MQTT_QOS_0; qos <= MQTT_QOS_2; qos++) { + for (nl = 0; nl <= 1; nl++) { + for (rap = 0; rap <= 1; rap++) { + for (rh_i = 0; + rh_i < (int)(sizeof(rh_vals) / sizeof(rh_vals[0])); + rh_i++) { + want_opts = (byte)( + (nl ? MQTT_SUBSCRIBE_NO_LOCAL : 0) | + (rap ? MQTT_SUBSCRIBE_RETAIN_AS_PUBLISHED : 0) | + rh_vals[rh_i]); + + XMEMSET(&sub, 0, sizeof(sub)); + XMEMSET(&topic, 0, sizeof(topic)); + topic.topic_filter = "a"; + topic.qos = (MqttQoS)qos; + topic.sub_options = want_opts; + sub.topics = &topic; + sub.topic_count = 1; + sub.packet_id = 1; + sub.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5; + + enc_len = MqttEncode_Subscribe(tx_buf, (int)sizeof(tx_buf), + &sub); + ASSERT_TRUE(enc_len > 0); + /* Last byte is the options byte: QoS (bits 0-1) plus the + * v5 option bits 2-5. */ + ASSERT_EQ((int)((byte)qos | want_opts), + (int)tx_buf[enc_len - 1]); + + XMEMSET(&dec, 0, sizeof(dec)); + XMEMSET(dec_topic, 0, sizeof(dec_topic)); + dec.topics = dec_topic; + dec.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5; + dec_len = MqttDecode_Subscribe(tx_buf, enc_len, &dec); + ASSERT_TRUE(dec_len > 0); + ASSERT_EQ(1, dec.topic_count); + ASSERT_EQ((int)qos, (int)dec_topic[0].qos); + ASSERT_EQ((int)want_opts, (int)dec_topic[0].sub_options); + } + } + } + } +} #endif /* WOLFMQTT_V5 */ /* ============================================================================ @@ -4969,6 +5110,11 @@ void run_mqtt_packet_tests(void) RUN_TEST(encode_subscribe_options_byte_qos0); RUN_TEST(encode_subscribe_options_byte_qos1); RUN_TEST(encode_subscribe_options_byte_qos2); +#ifdef WOLFMQTT_V5 + RUN_TEST(encode_subscribe_v311_ignores_sub_options); + RUN_TEST(encode_subscribe_v5_retain_handling_3_rejected); + RUN_TEST(encode_subscribe_v5_reserved_sub_options_bit_rejected); +#endif RUN_TEST(encode_subscribe_topic_filter_oversized_rejected); RUN_TEST(encode_subscribe_topic_filter_oversized_second_rejected); RUN_TEST(encode_subscribe_has_qos1_flag); @@ -5050,6 +5196,7 @@ void run_mqtt_packet_tests(void) RUN_TEST(decode_subscribe_v5_qos3_rejected); RUN_TEST(decode_subscribe_v5_retain_handling_3_rejected); RUN_TEST(decode_subscribe_v5_options_reserved_bits_rejected); + RUN_TEST(subscribe_v5_options_roundtrip_all_combos); #endif /* MqttDecode_Unsubscribe */ diff --git a/wolfmqtt/mqtt_packet.h b/wolfmqtt/mqtt_packet.h index 04452d967..d10ffd7d4 100644 --- a/wolfmqtt/mqtt_packet.h +++ b/wolfmqtt/mqtt_packet.h @@ -208,6 +208,21 @@ typedef enum _MqttQoS { #endif +#ifdef WOLFMQTT_V5 +/* MQTT v5 SUBSCRIBE options byte (section 3.8.3.1), stored in + * MqttTopic.sub_options in their on-the-wire bit positions. QoS (bits 0-1) + * is carried separately in MqttTopic.qos; bits 6-7 are reserved (MUST be 0). + * The v5 encoder rejects sub_options with any bit outside 2-5 set, or with + * Retain Handling = 3 (reserved), returning MQTT_CODE_ERROR_MALFORMED_DATA; + * supply only spec-valid combinations. */ +#define MQTT_SUBSCRIBE_NO_LOCAL 0x04 /* bit 2 */ +#define MQTT_SUBSCRIBE_RETAIN_AS_PUBLISHED 0x08 /* bit 3 */ +#define MQTT_SUBSCRIBE_RETAIN_HANDLING_0 0x00 /* bits 4-5: send at subscribe */ +#define MQTT_SUBSCRIBE_RETAIN_HANDLING_1 0x10 /* send at subscribe if new */ +#define MQTT_SUBSCRIBE_RETAIN_HANDLING_2 0x20 /* do not send at subscribe */ +#define MQTT_SUBSCRIBE_OPTIONS_MASK 0x3C /* bits 2-5 */ +#endif + /* Topic */ typedef struct _MqttTopic { const char* topic_filter; @@ -217,6 +232,11 @@ typedef struct _MqttTopic { byte return_code; /* MqttSubscribeAckReturnCodes */ #ifdef WOLFMQTT_V5 word16 alias; + byte sub_options; /* v5 SUBSCRIBE options bits 2-5 (No Local, + * Retain As Published, Retain Handling); see + * MQTT_SUBSCRIBE_*. Zero-initialize MqttTopic so + * unset options default to 0 - the v5 encoder now + * reads this field. */ #endif } MqttTopic;