Reject null chars in strings

ChangeLog.md

@@ -1,5 +1,39 @@
 ## Release Notes
 
+### v2.0.1 (Pending)
+
+* Security Hardening
+    - Reject MQTT UTF-8 encoded strings containing U+0000 in `MqttDecode_String`
+      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 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
+      `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` propagates `MQTT_CODE_ERROR_MALFORMED_DATA` from
+      `MqttDecode_String` for v5 STRING and STRING_PAIR property types
+      (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` 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
 process (see link below) and successfully passed the quality criteria.

src/mqtt_broker.c

@@ -175,8 +175,23 @@ static int BrokerStrCompare(const char* a, const char* b, int cmp_len)
 #endif /* WOLFMQTT_BROKER_AUTH */
 
 /* Store a string of known length into a BrokerClient field.
- * Static mode: copies into fixed-size buffer with truncation.
- * Dynamic mode: frees old value, allocates new buffer, copies. */
+ * Static mode: copies into fixed-size buffer with truncation. The
+ *   sensitive-wipe variant zeros the full max_len buffer, so it is
+ *   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. 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;
@@ -347,6 +349,12 @@ int MqttDecode_String(byte *buf, const char **pstr, word16 *pstr_len, word32 buf
         return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER);
     }
     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. */
+    if (str_len > 0 && XMEMCHR(buf, 0x00, str_len) != NULL) {
+        return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA);
+    }
     if (pstr_len) {
         *pstr_len = str_len;
     }
@@ -640,7 +648,17 @@ 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) && ((word32)tmp <= (buf_len - (buf - pbuf)))) {
+                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;
                     prop_len -= (word32)tmp;
@@ -702,7 +720,15 @@ 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) && ((word32)tmp <= (buf_len - (buf - pbuf)))) {
+                /* 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;
                     prop_len -= (word32)tmp;
@@ -711,8 +737,15 @@ 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) && ((word32)tmp <=
-                            (buf_len - (buf - pbuf)))) {
+                        /* 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;
                             total += tmp;
                             prop_len -= (word32)tmp;
@@ -969,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;
@@ -1150,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;
@@ -1464,12 +1530,13 @@ int MqttDecode_Publish(byte *rx_buf, int rx_buf_len, MqttPublish *publish)
     /* Decode variable header */
     variable_len = MqttDecode_String(rx_payload, &publish->topic_name,
         &publish->topic_name_len, (word32)(rx_buf_len - (rx_payload - rx_buf)));
-    if ((variable_len >= 0) && (variable_len + header_len <= rx_buf_len)) {
-        rx_payload += variable_len;
+    if (variable_len < 0) {
+        return variable_len;
     }
-    else {
+    if (variable_len + header_len > rx_buf_len) {
         return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER);
     }
+    rx_payload += variable_len;
 
     /* If QoS > 0 then get packet Id */
     if (publish->qos > MQTT_QOS_0) {