Fenrir broker fixes

Author: dgarske

src/mqtt_broker.c

@@ -35,6 +35,23 @@
 
 #ifdef WOLFMQTT_BROKER
 
+/* Secure memory zeroing - prevents compiler dead-store elimination */
+#ifdef ENABLE_MQTT_TLS
+    #include <wolfssl/wolfcrypt/memory.h>
+    #define BROKER_FORCE_ZERO(mem, len) wc_ForceZero(mem, (word32)(len))
+#else
+    /* Local implementation matching wolfCrypt's ForceZero */
+    static void BrokerForceZero(void* mem, word32 len)
+    {
+        volatile byte* p = (volatile byte*)mem;
+        word32 i;
+        for (i = 0; i < len; i++) {
+            p[i] = 0;
+        }
+    }
+    #define BROKER_FORCE_ZERO(mem, len) BrokerForceZero(mem, (word32)(len))
+#endif
+
 /* -------------------------------------------------------------------------- */
 /* Platform includes                                                           */
 /* -------------------------------------------------------------------------- */
@@ -137,10 +154,14 @@ static int BrokerStrCompare(const char* a, const char* b)
     int result = 0;
     int len_a = (int)XSTRLEN(a);
     int len_b = (int)XSTRLEN(b);
-    int len = (len_a < len_b) ? len_a : len_b;
+    int max_len = (len_a > len_b) ? len_a : len_b;
     int i;
-    for (i = 0; i < len; i++) {
-        result |= (a[i] ^ b[i]);
+    for (i = 0; i < max_len; i++) {
+        /* Branchless index clamp: when i >= len, reads position 0.
+         * Length mismatch is caught by the final XOR below. */
+        int ia = i & ((i - len_a) >> (sizeof(int) * 8 - 1));
+        int ib = i & ((i - len_b) >> (sizeof(int) * 8 - 1));
+        result |= (a[ia] ^ b[ib]);
     }
     result |= (len_a ^ len_b);
     return result;
@@ -164,7 +185,7 @@ static void BrokerStore_StringSensitive(char* dst, int max_len,
     const char* src, word16 src_len)
 {
     /* Wipe old value before overwriting */
-    XMEMSET(dst, 0, max_len);
+    BROKER_FORCE_ZERO(dst, max_len);
     if (src_len >= (word16)max_len) {
         src_len = (word16)(max_len - 1);
     }
@@ -177,7 +198,7 @@ static void BrokerStore_String(char** dst_ptr,
 {
     if (*dst_ptr != NULL) {
         if (sensitive) {
-            XMEMSET(*dst_ptr, 0, XSTRLEN(*dst_ptr) + 1);
+            BROKER_FORCE_ZERO(*dst_ptr, XSTRLEN(*dst_ptr) + 1);
         }
         WOLFMQTT_FREE(*dst_ptr);
         *dst_ptr = NULL;
@@ -225,6 +246,9 @@ static int BrokerTls_Init(MqttBroker* broker)
     }
     else {
         ctx = wolfSSL_CTX_new(wolfSSLv23_server_method());
+        if (ctx != NULL) {
+            wolfSSL_CTX_SetMinVersion(ctx, WOLFSSL_TLSV1_2);
+        }
     }
     if (ctx == NULL) {
         WBLOG_ERR(broker, "broker: wolfSSL_CTX_new failed");
@@ -1203,21 +1227,21 @@ static void BrokerClient_Free(BrokerClient* bc)
     }
 #ifdef WOLFMQTT_BROKER_AUTH
     if (bc->username) {
-        XMEMSET(bc->username, 0, XSTRLEN(bc->username) + 1);
+        BROKER_FORCE_ZERO(bc->username, XSTRLEN(bc->username) + 1);
         WOLFMQTT_FREE(bc->username);
     }
     if (bc->password) {
-        XMEMSET(bc->password, 0, XSTRLEN(bc->password) + 1);
+        BROKER_FORCE_ZERO(bc->password, XSTRLEN(bc->password) + 1);
         WOLFMQTT_FREE(bc->password);
     }
 #endif
 #ifdef WOLFMQTT_BROKER_WILL
     if (bc->will_topic) {
-        XMEMSET(bc->will_topic, 0, XSTRLEN(bc->will_topic) + 1);
+        BROKER_FORCE_ZERO(bc->will_topic, XSTRLEN(bc->will_topic) + 1);
         WOLFMQTT_FREE(bc->will_topic);
     }
     if (bc->will_payload) {
-        XMEMSET(bc->will_payload, 0, bc->will_payload_len);
+        BROKER_FORCE_ZERO(bc->will_payload, bc->will_payload_len);
         WOLFMQTT_FREE(bc->will_payload);
     }
 #endif
@@ -2468,6 +2492,11 @@ static int BrokerTopicMatch(const char* filter, const char* topic)
         return 0;
     }
 
+    /* [MQTT-4.7.2] Wildcard filters must not match $-prefixed topics */
+    if (*t == '$' && (*f == '+' || *f == '#')) {
+        return 0;
+    }
+
     while (*f && *t) {
         if (*f == '#') {
             return (f[1] == '\0');
@@ -3063,6 +3092,19 @@ static int BrokerHandle_Publish(BrokerClient* bc, int rx_len,
         return rc;
     }
 
+    /* [MQTT-3.3.2-2] PUBLISH topic must not contain wildcard characters */
+    if (pub.topic_name && pub.topic_name_len > 0) {
+        word16 i;
+        for (i = 0; i < pub.topic_name_len; i++) {
+            if (pub.topic_name[i] == '+' || pub.topic_name[i] == '#') {
+                WBLOG_ERR(broker,
+                    "broker: PUBLISH topic contains wildcard sock=%d",
+                    (int)bc->sock);
+                return MQTT_CODE_ERROR_BAD_ARG;
+            }
+        }
+    }
+
     /* Create null-terminated topic copy for matching/logging */
     if (pub.topic_name && pub.topic_name_len > 0) {
 #ifdef WOLFMQTT_STATIC_MEMORY
@@ -3363,6 +3405,15 @@ static int BrokerClient_Process(MqttBroker* broker, BrokerClient* bc)
             BrokerClient_Remove(broker, bc);
             return 0;
         }
+        /* [MQTT-3.1.0-2] Second CONNECT is a protocol violation */
+        if (type == MQTT_PACKET_TYPE_CONNECT && bc->connected) {
+            WBLOG_ERR(broker,
+                "broker: second CONNECT on sock=%d [MQTT-3.1.0-2]",
+                (int)bc->sock);
+            BrokerSubs_RemoveClient(broker, bc);
+            BrokerClient_Remove(broker, bc);
+            return 0;
+        }
         switch (type) {
             case MQTT_PACKET_TYPE_CONNECT:
             {

src/mqtt_packet.c

@@ -338,6 +338,9 @@ int MqttDecode_String(byte *buf, const char **pstr, word16 *pstr_len, word32 buf
     if (len < 0) {
         return len;
     }
+    if ((word32)str_len > buf_len - (word32)len) {
+        return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER);
+    }
     buf += len;
     if (pstr_len) {
         *pstr_len = str_len;
@@ -353,7 +356,13 @@ int MqttDecode_String(byte *buf, const char **pstr, word16 *pstr_len, word32 buf
 int MqttEncode_String(byte *buf, const char *str)
 {
     int str_len = (int)XSTRLEN(str);
-    int len = MqttEncode_Num(buf, (word16)str_len);
+    int len;
+
+    /* MQTT UTF-8 strings are limited to 65535 bytes [MQTT-1.5.3] */
+    if (str_len > (int)0xFFFF) {
+        return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
+    }
+    len = MqttEncode_Num(buf, (word16)str_len);
 
     if (buf != NULL) {
         buf += len;
@@ -790,6 +799,10 @@ int MqttEncode_Connect(byte *tx_buf, int tx_buf_len, MqttConnect *mc_connect)
 
         remain_len += (int)XSTRLEN(mc_connect->lwt_msg->topic_name);
         remain_len += MQTT_DATA_LEN_SIZE;
+        /* LWT payload uses word16 length prefix, validate it fits */
+        if (mc_connect->lwt_msg->total_len > (word32)0xFFFF) {
+            return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_BAD_ARG);
+        }
         remain_len += mc_connect->lwt_msg->total_len;
         remain_len += MQTT_DATA_LEN_SIZE;
 #ifdef WOLFMQTT_V5
@@ -1466,6 +1479,9 @@ int MqttDecode_Publish(byte *rx_buf, int rx_buf_len, MqttPublish *publish)
 #endif
 
     /* Decode Payload */
+    if (variable_len > remain_len) {
+        return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_OUT_OF_BUFFER);
+    }
     payload_len = remain_len - variable_len;
     publish->buffer = rx_payload;
     publish->buffer_new = 1;