Fixes from review

Author: embhorn

ChangeLog.md

@@ -7,26 +7,32 @@
       per [MQTT-1.5.3-2] / [MQTT-1.5.4-2]. Closes an embedded-NUL truncation
       attack that allowed broker-side auth bypass, ClientId collision, and
       topic-routing confusion when the broker compared decoded strings with
-      C-string semantics. The check is also applied to the CONNECT Password
-      field (which the spec defines as Binary Data) because wolfMQTT compares
-      passwords with `XSTRLEN`/`XSTRCMP`.
+      C-string semantics.
+    - The CONNECT Password field is decoded by a new internal helper
+      (`MqttDecode_Password`) that applies the same NUL rejection. Per
+      MQTT-3.1.3.5 the field is Binary Data so the spec does not require
+      this check, but wolfMQTT compares passwords with `XSTRLEN`/`XSTRCMP`,
+      so a binary password with an embedded NUL would be silently truncated
+      and could enable an auth bypass. Spec-compliant clients sending binary
+      passwords containing 0x00 will be rejected by the broker as a result.
 
 * API / Behavior Changes
     - `MqttDecode_String` may now return `MQTT_CODE_ERROR_MALFORMED_DATA`
-      when the decoded string contains an embedded NUL byte. Previously only
-      `MQTT_CODE_ERROR_BAD_ARG` and `MQTT_CODE_ERROR_OUT_OF_BUFFER` were
-      possible negative returns.
+      when the decoded string contains an embedded NUL byte. Previously
+      `MQTT_CODE_ERROR_OUT_OF_BUFFER` was the only possible negative return.
     - `MqttDecode_Publish` now propagates the underlying error from
       `MqttDecode_String` (e.g. `MALFORMED_DATA`) instead of always returning
       `MQTT_CODE_ERROR_OUT_OF_BUFFER` on topic decode failure.
-    - `MqttDecode_Props` similarly now propagates the underlying error from
+    - `MqttDecode_Props` propagates `MQTT_CODE_ERROR_MALFORMED_DATA` from
       `MqttDecode_String` for v5 STRING and STRING_PAIR property types
-      (Reason String, Content Type, User Property, etc.) instead of always
-      masking it as `MQTT_CODE_ERROR_PROPERTY`.
+      (Reason String, Content Type, User Property, etc.). Other negative
+      returns continue to be mapped to `MQTT_CODE_ERROR_PROPERTY` so client
+      code that branches on that error code is unaffected.
     - New `XMEMCHR(s, c, n)` portability macro added to `mqtt_types.h`
-      (defaults to `memchr`). Builds using `WOLFMQTT_CUSTOM_STRING` must
-      provide their own mapping, matching the pattern used for `XSTRLEN`,
-      `XMEMCMP`, etc.
+      (defaults to `memchr` for standard builds). Builds using
+      `WOLFMQTT_CUSTOM_STRING` must define `XMEMCHR` themselves, matching
+      the pattern used for `XSTRLEN`, `XMEMCMP`, etc.; the header emits
+      an explicit `#error` if it is missing.
 
 ### v2.0.0 (03/20/2026)
 Release 2.0.0 has been developed according to wolfSSL's development and QA

src/mqtt_broker.c

@@ -180,12 +180,18 @@ static int BrokerStrCompare(const char* a, const char* b, int cmp_len)
  *   unaffected by string contents.
  * Dynamic mode: frees old value, allocates new buffer, copies. The
  *   sensitive-wipe path uses XSTRLEN(buf)+1, which is correct only when
- *   stored values contain no embedded NUL. MqttDecode_String enforces
- *   [MQTT-1.5.3-2] / [MQTT-1.5.4-2] (rejects U+0000), so any string that
- *   reaches this function via the protocol path satisfies that invariant.
- *   Do not call the dynamic-mode sensitive path with src bytes that bypass
- *   that decoder without first capping src_len at the first NUL, or the
- *   wipe will leave trailing bytes uncleared. */
+ *   stored values contain no embedded NUL. The decode-side guards that
+ *   feed this function enforce that invariant:
+ *     - MqttDecode_String rejects U+0000 per [MQTT-1.5.3-2] /
+ *       [MQTT-1.5.4-2] for UTF-8 fields (client_id, username, topics).
+ *     - MqttDecode_Password applies the same NUL check to the CONNECT
+ *       Password field; that field is Binary Data per MQTT-3.1.3.5 and
+ *       the spec does not require it, but wolfMQTT compares passwords
+ *       with XSTRLEN/XSTRCMP, so the broker-side defensive check here
+ *       is what keeps the "no embedded NUL" assumption intact.
+ *   Do not call the dynamic-mode sensitive path with src bytes that
+ *   bypass those decoders without first capping src_len at the first
+ *   NUL, or the wipe will leave trailing bytes uncleared. */
 #ifdef WOLFMQTT_STATIC_MEMORY
 static void BrokerStore_String(char* dst, int max_len,
     const char* src, word16 src_len)

src/mqtt_packet.c

@@ -335,6 +335,8 @@ int MqttEncode_Int(byte* buf, word32 len)
 
 /* Returns number of buffer bytes decoded */
 /* Returns pointer to string (which is not guaranteed to be null terminated) */
+/* Note: MqttDecode_Password mirrors this implementation for the CONNECT
+ * Password binary-data field. Keep length and bounds handling in sync. */
 int MqttDecode_String(byte *buf, const char **pstr, word16 *pstr_len, word32 buf_len)
 {
     int len;
@@ -349,14 +351,7 @@ int MqttDecode_String(byte *buf, const char **pstr, word16 *pstr_len, word32 buf
     buf += len;
     /* [MQTT-1.5.3-2] / [MQTT-1.5.4-2]: a UTF-8 encoded string MUST NOT
      * include U+0000. Reject so downstream C-string handling cannot be
-     * tricked by an embedded NUL truncating the value.
-     *
-     * The CONNECT Password field is technically Binary Data per spec,
-     * not a UTF-8 string, so the prohibition does not formally apply.
-     * The check is still enforced here because wolfMQTT compares
-     * passwords with XSTRLEN/XSTRCMP, so a binary password with an
-     * embedded NUL would be silently truncated and enable an auth
-     * bypass — exactly the class of attack this validation prevents. */
+     * tricked by an embedded NUL truncating the value. */
     if (str_len > 0 && XMEMCHR(buf, 0x00, str_len) != NULL) {
         return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA);
     }
@@ -653,9 +648,16 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf,
                         (const char**)&cur_prop->data_str.str,
                         &cur_prop->data_str.len,
                         (word32)(buf_len - (buf - pbuf)));
-                if (tmp < 0) {
+                if (tmp == MQTT_CODE_ERROR_MALFORMED_DATA) {
+                    /* Propagate spec-required NUL rejection so callers can
+                     * distinguish malformed UTF-8 from generic property
+                     * decode failures. Other negative codes keep their
+                     * legacy MQTT_CODE_ERROR_PROPERTY mapping below. */
                     rc = tmp;
                 }
+                else if (tmp < 0) {
+                    rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PROPERTY);
+                }
                 else if ((word32)tmp <= (buf_len - (buf - pbuf))) {
                     buf += tmp;
                     total += tmp;
@@ -718,9 +720,14 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf,
                         (const char**)&cur_prop->data_str.str,
                         &cur_prop->data_str.len,
                         (word32)(buf_len - (buf - pbuf)));
-                if (tmp < 0) {
+                /* See MQTT_DATA_TYPE_STRING above for the rationale on
+                 * propagating MALFORMED_DATA distinctly from other errors. */
+                if (tmp == MQTT_CODE_ERROR_MALFORMED_DATA) {
                     rc = tmp;
                 }
+                else if (tmp < 0) {
+                    rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PROPERTY);
+                }
                 else if ((word32)tmp <= (buf_len - (buf - pbuf))) {
                     buf += tmp;
                     total += tmp;
@@ -730,9 +737,13 @@ int MqttDecode_Props(MqttPacketType packet, MqttProp** props, byte* pbuf,
                                 (const char**)&cur_prop->data_str2.str,
                                 &cur_prop->data_str2.len,
                                 (word32)(buf_len - (buf - pbuf)));
-                        if (tmp < 0) {
+                        /* See MQTT_DATA_TYPE_STRING above. */
+                        if (tmp == MQTT_CODE_ERROR_MALFORMED_DATA) {
                             rc = tmp;
                         }
+                        else if (tmp < 0) {
+                            rc = MQTT_TRACE_ERROR(MQTT_CODE_ERROR_PROPERTY);
+                        }
                         else if ((word32)tmp <=
                                  (buf_len - (buf - pbuf))) {
                             buf += tmp;
@@ -991,6 +1002,39 @@ int MqttEncode_Connect(byte *tx_buf, int tx_buf_len, MqttConnect *mc_connect)
 }
 
 #ifdef WOLFMQTT_BROKER
+/* Decode the CONNECT Password field. Password is Binary Data per
+ * MQTT-3.1.3.5, not a UTF-8 string, so the [MQTT-1.5.3-2] / [MQTT-1.5.4-2]
+ * U+0000 prohibition does not formally apply. The defensive NUL check is
+ * applied here because wolfMQTT compares stored passwords with
+ * XSTRLEN/XSTRCMP, so a binary password with an embedded NUL would be
+ * silently truncated and could enable an auth bypass. Decoupling this
+ * from MqttDecode_String keeps the spec-compliant UTF-8 path separate
+ * from the broker-specific binary-data validation. */
+static int MqttDecode_Password(byte *buf, const char **ppassword,
+    word16 *ppassword_len, word32 buf_len)
+{
+    int len;
+    word16 pass_len;
+    len = MqttDecode_Num(buf, &pass_len, buf_len);
+    if (len < 0) {
+        return len;
+    }
+    if ((word32)pass_len > buf_len - (word32)len) {
+        return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER);
+    }
+    buf += len;
+    if (pass_len > 0 && XMEMCHR(buf, 0x00, pass_len) != NULL) {
+        return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA);
+    }
+    if (ppassword_len) {
+        *ppassword_len = pass_len;
+    }
+    if (ppassword) {
+        *ppassword = (char*)buf;
+    }
+    return len + pass_len;
+}
+
 int MqttDecode_Connect(byte *rx_buf, int rx_buf_len, MqttConnect *mc_connect)
 {
     int header_len, remain_len;
@@ -1172,7 +1216,7 @@ int MqttDecode_Connect(byte *rx_buf, int rx_buf_len, MqttConnect *mc_connect)
     }
 
     if (packet.flags & MQTT_CONNECT_FLAG_PASSWORD) {
-        tmp = MqttDecode_String(rx_payload, &mc_connect->password, NULL,
+        tmp = MqttDecode_Password(rx_payload, &mc_connect->password, NULL,
                 (word32)(rx_buf_len - (rx_payload - rx_buf)));
         if (tmp < 0) {
             return tmp;

tests/test_mqtt_packet.c

@@ -616,6 +616,77 @@ TEST(decode_publish_v5_content_type_property)
 
     MqttProps_Free(pub.props);
 }
+
+/* [MQTT-1.5.4-2]: an embedded NUL in a v5 STRING property must be
+ * rejected. Uses a PUBLISH packet with a Content Type property whose
+ * value contains 0x00. MqttDecode_Props propagates MALFORMED_DATA from
+ * MqttDecode_String distinctly from generic MQTT_CODE_ERROR_PROPERTY. */
+TEST(decode_publish_v5_rejects_nul_in_string_property)
+{
+    /* Wire: PUBLISH QoS 0, remain_len=13, topic "a/b", props_len=7,
+     *       prop 0x03 CONTENT_TYPE, str_len=4, "t\0xt". No payload. */
+    byte buf[] = {
+        0x30, 13,
+        0x00, 0x03, 'a', '/', 'b',
+        0x07,
+        0x03, 0x00, 0x04, 't', 0x00, 'x', 't'
+    };
+    MqttPublish pub;
+    int rc;
+
+    XMEMSET(&pub, 0, sizeof(pub));
+    pub.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5;
+    rc = MqttDecode_Publish(buf, (int)sizeof(buf), &pub);
+    ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc);
+    MqttProps_Free(pub.props);
+}
+
+/* STRING_PAIR (USER_PROPERTY) NUL rejection — first string of pair. The
+ * MqttDecode_Props path runs MqttDecode_String twice for STRING_PAIR;
+ * this pins coverage on the first sub-decode propagating MALFORMED_DATA. */
+TEST(decode_publish_v5_rejects_nul_in_user_prop_key)
+{
+    /* Wire: PUBLISH QoS 0, remain_len=14, topic "a/b", props_len=8,
+     *       prop 0x26 USER_PROPERTY, key_len=2 "k\0", val_len=1 "v". */
+    byte buf[] = {
+        0x30, 14,
+        0x00, 0x03, 'a', '/', 'b',
+        0x08,
+        0x26, 0x00, 0x02, 'k', 0x00,
+              0x00, 0x01, 'v'
+    };
+    MqttPublish pub;
+    int rc;
+
+    XMEMSET(&pub, 0, sizeof(pub));
+    pub.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5;
+    rc = MqttDecode_Publish(buf, (int)sizeof(buf), &pub);
+    ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc);
+    MqttProps_Free(pub.props);
+}
+
+/* STRING_PAIR (USER_PROPERTY) NUL rejection — second string of pair.
+ * Pins coverage on the second sub-decode propagating MALFORMED_DATA. */
+TEST(decode_publish_v5_rejects_nul_in_user_prop_value)
+{
+    /* Wire: PUBLISH QoS 0, remain_len=14, topic "a/b", props_len=8,
+     *       prop 0x26 USER_PROPERTY, key_len=1 "k", val_len=2 "v\0". */
+    byte buf[] = {
+        0x30, 14,
+        0x00, 0x03, 'a', '/', 'b',
+        0x08,
+        0x26, 0x00, 0x01, 'k',
+              0x00, 0x02, 'v', 0x00
+    };
+    MqttPublish pub;
+    int rc;
+
+    XMEMSET(&pub, 0, sizeof(pub));
+    pub.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5;
+    rc = MqttDecode_Publish(buf, (int)sizeof(buf), &pub);
+    ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc);
+    MqttProps_Free(pub.props);
+}
 #endif /* WOLFMQTT_V5 */
 
 /* ============================================================================
@@ -1610,6 +1681,34 @@ TEST(decode_connect_rejects_nul_in_will_topic)
     rc = MqttDecode_Connect(buf, (int)sizeof(buf), &dec);
     ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc);
 }
+
+#ifdef WOLFMQTT_V5
+/* MQTT v5 has a different decode path than v3.1.1 (properties walked
+ * before client_id, separate LWT properties), but the NUL rejection
+ * lives inside MqttDecode_String / MqttDecode_Password and so applies
+ * uniformly. This pins coverage on the v5 branch so a future refactor
+ * cannot quietly bypass the check on one protocol level. */
+TEST(decode_connect_v5_rejects_nul_in_client_id)
+{
+    byte buf[] = {
+        0x10, 0x13,                         /* CONNECT, remain_len = 19 */
+        0x00, 0x04, 'M', 'Q', 'T', 'T',
+        0x05,                               /* protocol level v5 */
+        0x02,                               /* flags: clean_session */
+        0x00, 0x3C,                         /* keep alive */
+        0x00,                               /* props_len = 0 (VBI) */
+        0x00, 0x06, 'a', 'd', 0x00, 'm', 'i', 'n'  /* client_id with NUL */
+    };
+    MqttConnect dec;
+    int rc;
+
+    XMEMSET(&dec, 0, sizeof(dec));
+    dec.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5;
+    rc = MqttDecode_Connect(buf, (int)sizeof(buf), &dec);
+    ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc);
+    MqttProps_Free(dec.props);
+}
+#endif /* WOLFMQTT_V5 */
 #endif /* WOLFMQTT_BROKER */
 
 /* ============================================================================
@@ -1726,31 +1825,6 @@ TEST(decode_subscribe_v5_options_byte_qos_extracted)
     ASSERT_EQ(1, sub.topic_count);
     ASSERT_EQ(MQTT_QOS_1, topic_arr[0].qos);
 }
-
-/* [MQTT-1.5.4-2]: an embedded NUL in a v5 STRING property must be
- * rejected. Uses a PUBLISH packet with a Content Type property whose
- * value contains 0x00. The MqttDecode_Props path now propagates the
- * underlying MALFORMED_DATA from MqttDecode_String instead of masking
- * it as MQTT_CODE_ERROR_PROPERTY. */
-TEST(decode_publish_v5_rejects_nul_in_string_property)
-{
-    /* Wire: PUBLISH QoS 0, remain_len=13, topic "a/b", props_len=7,
-     *       prop 0x03 CONTENT_TYPE, str_len=4, "t\0xt". No payload. */
-    byte buf[] = {
-        0x30, 13,
-        0x00, 0x03, 'a', '/', 'b',
-        0x07,
-        0x03, 0x00, 0x04, 't', 0x00, 'x', 't'
-    };
-    MqttPublish pub;
-    int rc;
-
-    XMEMSET(&pub, 0, sizeof(pub));
-    pub.protocol_level = MQTT_CONNECT_PROTOCOL_LEVEL_5;
-    rc = MqttDecode_Publish(buf, (int)sizeof(buf), &pub);
-    ASSERT_EQ(MQTT_CODE_ERROR_MALFORMED_DATA, rc);
-    MqttProps_Free(pub.props);
-}
 #endif /* WOLFMQTT_V5 */
 
 /* ============================================================================
@@ -2311,6 +2385,9 @@ void run_mqtt_packet_tests(void)
     RUN_TEST(decode_publish_rejects_nul_in_topic);
 #ifdef WOLFMQTT_V5
     RUN_TEST(decode_publish_v5_content_type_property);
+    RUN_TEST(decode_publish_v5_rejects_nul_in_string_property);
+    RUN_TEST(decode_publish_v5_rejects_nul_in_user_prop_key);
+    RUN_TEST(decode_publish_v5_rejects_nul_in_user_prop_value);
 #endif
 
     /* MqttDecode_ConnectAck */
@@ -2364,14 +2441,16 @@ void run_mqtt_packet_tests(void)
     RUN_TEST(decode_connect_rejects_nul_in_username);
     RUN_TEST(decode_connect_rejects_nul_in_password);
     RUN_TEST(decode_connect_rejects_nul_in_will_topic);
+#ifdef WOLFMQTT_V5
+    RUN_TEST(decode_connect_v5_rejects_nul_in_client_id);
+#endif
 
     /* MqttDecode_Subscribe */
     RUN_TEST(decode_subscribe_v311_single_topic);
     RUN_TEST(decode_subscribe_v311_qos3_reserved);
     RUN_TEST(decode_subscribe_rejects_nul_in_filter);
 #ifdef WOLFMQTT_V5
     RUN_TEST(decode_subscribe_v5_options_byte_qos_extracted);
-    RUN_TEST(decode_publish_v5_rejects_nul_in_string_property);
 #endif
 
     /* MqttDecode_Unsubscribe */

wolfmqtt/mqtt_types.h

@@ -246,9 +246,6 @@ enum MqttPacketResponseCodes {
     #ifndef XMEMCMP
         #define XMEMCMP(s1,s2,n)    memcmp((s1),(s2),(n))
     #endif
-    #ifndef XMEMCHR
-        #define XMEMCHR(s,c,n)      memchr((s),(c),(n))
-    #endif
     #ifndef XATOI
         #define XATOI(s)            atoi((s))
     #endif
@@ -267,6 +264,19 @@ enum MqttPacketResponseCodes {
     #endif
 #endif
 
+/* XMEMCHR backstop. Standard builds and existing custom-string ports
+ * (which already pull in <string.h> via other paths) keep building
+ * without changes. Custom-string ports that intentionally avoid
+ * <string.h> get an explicit #error directing them to define XMEMCHR
+ * themselves, instead of a confusing missing-header diagnostic. */
+#ifndef XMEMCHR
+    #ifdef WOLFMQTT_CUSTOM_STRING
+        #error "WOLFMQTT_CUSTOM_STRING set: please define XMEMCHR"
+    #else
+        #define XMEMCHR(s,c,n)      memchr((s),(c),(n))
+    #endif
+#endif
+
 #ifndef WOLFMQTT_CUSTOM_MALLOC
     #ifndef WOLFMQTT_MALLOC
         #define WOLFMQTT_MALLOC(s)  malloc((s))