Fenrir fixes

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();

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();

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;
 }
 

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

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++;
         }
 

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);

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