Skip to content

Commit dd89fa2

Browse files
aidangarskeaidangarske
committed
Address skoll review and david feedback
1 parent 73ccdb3 commit dd89fa2

4 files changed

Lines changed: 42 additions & 36 deletions

File tree

‎src/mqtt_broker.c‎

Lines changed: 16 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -35,22 +35,6 @@
3535

3636
#ifdef WOLFMQTT_BROKER
3737

38-
/* Secure memory zeroing - prevents compiler dead-store elimination */
39-
#ifdef ENABLE_MQTT_TLS
40-
#include <wolfssl/wolfcrypt/memory.h>
41-
#define BROKER_FORCE_ZERO(mem, len) wc_ForceZero(mem, (word32)(len))
42-
#else
43-
/* Local implementation matching wolfCrypt's ForceZero */
44-
static void BrokerForceZero(void* mem, word32 len)
45-
{
46-
volatile byte* p = (volatile byte*)mem;
47-
word32 i;
48-
for (i = 0; i < len; i++) {
49-
p[i] = 0;
50-
}
51-
}
52-
#define BROKER_FORCE_ZERO(mem, len) BrokerForceZero(mem, (word32)(len))
53-
#endif
5438

5539
/* -------------------------------------------------------------------------- */
5640
/* Platform includes */
@@ -187,7 +171,7 @@ static void BrokerStore_StringSensitive(char* dst, int max_len,
187171
const char* src, word16 src_len)
188172
{
189173
/* Wipe old value before overwriting */
190-
BROKER_FORCE_ZERO(dst, max_len);
174+
WOLFMQTT_FORCE_ZERO(dst, max_len);
191175
if (src_len >= (word16)max_len) {
192176
src_len = (word16)(max_len - 1);
193177
}
@@ -200,7 +184,7 @@ static void BrokerStore_String(char** dst_ptr,
200184
{
201185
if (*dst_ptr != NULL) {
202186
if (sensitive) {
203-
BROKER_FORCE_ZERO(*dst_ptr, XSTRLEN(*dst_ptr) + 1);
187+
WOLFMQTT_FORCE_ZERO(*dst_ptr, XSTRLEN(*dst_ptr) + 1);
204188
}
205189
WOLFMQTT_FREE(*dst_ptr);
206190
*dst_ptr = NULL;
@@ -1236,21 +1220,21 @@ static void BrokerClient_Free(BrokerClient* bc)
12361220
}
12371221
#ifdef WOLFMQTT_BROKER_AUTH
12381222
if (bc->username) {
1239-
BROKER_FORCE_ZERO(bc->username, XSTRLEN(bc->username) + 1);
1223+
WOLFMQTT_FORCE_ZERO(bc->username, XSTRLEN(bc->username) + 1);
12401224
WOLFMQTT_FREE(bc->username);
12411225
}
12421226
if (bc->password) {
1243-
BROKER_FORCE_ZERO(bc->password, XSTRLEN(bc->password) + 1);
1227+
WOLFMQTT_FORCE_ZERO(bc->password, XSTRLEN(bc->password) + 1);
12441228
WOLFMQTT_FREE(bc->password);
12451229
}
12461230
#endif
12471231
#ifdef WOLFMQTT_BROKER_WILL
12481232
if (bc->will_topic) {
1249-
BROKER_FORCE_ZERO(bc->will_topic, XSTRLEN(bc->will_topic) + 1);
1233+
WOLFMQTT_FORCE_ZERO(bc->will_topic, XSTRLEN(bc->will_topic) + 1);
12501234
WOLFMQTT_FREE(bc->will_topic);
12511235
}
12521236
if (bc->will_payload) {
1253-
BROKER_FORCE_ZERO(bc->will_payload, bc->will_payload_len);
1237+
WOLFMQTT_FORCE_ZERO(bc->will_payload, bc->will_payload_len);
12541238
WOLFMQTT_FREE(bc->will_payload);
12551239
}
12561240
#endif
@@ -2030,12 +2014,12 @@ static void BrokerClient_ClearWill(BrokerClient* bc)
20302014
bc->will_topic[0] = '\0';
20312015
#else
20322016
if (bc->will_topic) {
2033-
BROKER_FORCE_ZERO(bc->will_topic, XSTRLEN(bc->will_topic) + 1);
2017+
WOLFMQTT_FORCE_ZERO(bc->will_topic, XSTRLEN(bc->will_topic) + 1);
20342018
WOLFMQTT_FREE(bc->will_topic);
20352019
bc->will_topic = NULL;
20362020
}
20372021
if (bc->will_payload) {
2038-
BROKER_FORCE_ZERO(bc->will_payload, bc->will_payload_len);
2022+
WOLFMQTT_FORCE_ZERO(bc->will_payload, bc->will_payload_len);
20392023
WOLFMQTT_FREE(bc->will_payload);
20402024
bc->will_payload = NULL;
20412025
}
@@ -2135,14 +2119,14 @@ static int BrokerPendingWill_Add(MqttBroker* broker, BrokerClient* bc)
21352119
}
21362120
else if (pw != NULL) {
21372121
if (pw->topic) {
2138-
BROKER_FORCE_ZERO(pw->topic, XSTRLEN(pw->topic) + 1);
2122+
WOLFMQTT_FORCE_ZERO(pw->topic, XSTRLEN(pw->topic) + 1);
21392123
WOLFMQTT_FREE(pw->topic);
21402124
}
21412125
if (pw->client_id) {
21422126
WOLFMQTT_FREE(pw->client_id);
21432127
}
21442128
if (pw->payload) {
2145-
BROKER_FORCE_ZERO(pw->payload, pw->payload_len);
2129+
WOLFMQTT_FORCE_ZERO(pw->payload, pw->payload_len);
21462130
WOLFMQTT_FREE(pw->payload);
21472131
}
21482132
WOLFMQTT_FREE(pw);
@@ -2196,11 +2180,11 @@ static void BrokerPendingWill_Cancel(MqttBroker* broker,
21962180
}
21972181
WOLFMQTT_FREE(pw->client_id);
21982182
if (pw->topic) {
2199-
BROKER_FORCE_ZERO(pw->topic, XSTRLEN(pw->topic) + 1);
2183+
WOLFMQTT_FORCE_ZERO(pw->topic, XSTRLEN(pw->topic) + 1);
22002184
WOLFMQTT_FREE(pw->topic);
22012185
}
22022186
if (pw->payload) {
2203-
BROKER_FORCE_ZERO(pw->payload, pw->payload_len);
2187+
WOLFMQTT_FORCE_ZERO(pw->payload, pw->payload_len);
22042188
WOLFMQTT_FREE(pw->payload);
22052189
}
22062190
WOLFMQTT_FREE(pw);
@@ -2227,11 +2211,11 @@ static void BrokerPendingWill_FreeAll(MqttBroker* broker)
22272211
BrokerPendingWill* next = pw->next;
22282212
if (pw->client_id) WOLFMQTT_FREE(pw->client_id);
22292213
if (pw->topic) {
2230-
BROKER_FORCE_ZERO(pw->topic, XSTRLEN(pw->topic) + 1);
2214+
WOLFMQTT_FORCE_ZERO(pw->topic, XSTRLEN(pw->topic) + 1);
22312215
WOLFMQTT_FREE(pw->topic);
22322216
}
22332217
if (pw->payload) {
2234-
BROKER_FORCE_ZERO(pw->payload, pw->payload_len);
2218+
WOLFMQTT_FORCE_ZERO(pw->payload, pw->payload_len);
22352219
WOLFMQTT_FREE(pw->payload);
22362220
}
22372221
WOLFMQTT_FREE(pw);
@@ -2295,11 +2279,11 @@ static int BrokerPendingWill_Process(MqttBroker* broker)
22952279
}
22962280
if (pw->client_id) WOLFMQTT_FREE(pw->client_id);
22972281
if (pw->topic) {
2298-
BROKER_FORCE_ZERO(pw->topic, XSTRLEN(pw->topic) + 1);
2282+
WOLFMQTT_FORCE_ZERO(pw->topic, XSTRLEN(pw->topic) + 1);
22992283
WOLFMQTT_FREE(pw->topic);
23002284
}
23012285
if (pw->payload) {
2302-
BROKER_FORCE_ZERO(pw->payload, pw->payload_len);
2286+
WOLFMQTT_FORCE_ZERO(pw->payload, pw->payload_len);
23032287
WOLFMQTT_FREE(pw->payload);
23042288
}
23052289
WOLFMQTT_FREE(pw);

‎src/mqtt_client.c‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1722,15 +1722,16 @@ int MqttClient_Connect(MqttClient *client, MqttConnect *mc_connect)
17221722
&& client->write.total > 0
17231723
#endif
17241724
) {
1725-
/* keep send locked and return early */
1725+
/* keep send locked and return early.
1726+
* Note: tx_buf still contains credentials until write completes */
17261727
return rc;
17271728
}
17281729
#endif
17291730
MqttWriteStop(client, &mc_connect->stat);
17301731

17311732
/* Clear tx_buf to remove any plaintext credentials from memory.
17321733
* Use xfer (saved before MqttWriteStop zeroes client->write) */
1733-
XMEMSET(client->tx_buf, 0, xfer);
1734+
WOLFMQTT_FORCE_ZERO(client->tx_buf, xfer);
17341735

17351736
if (rc != xfer) {
17361737
MqttClient_CancelMessage(client, (MqttObject*)mc_connect);

‎tests/unit_test.c‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -229,15 +229,17 @@ static void test_decode_connack(void)
229229
/* Malformed CONNACK: remain_len=0 */
230230
buf[0] = MQTT_PACKET_TYPE_SET(MQTT_PACKET_TYPE_CONNECT_ACK);
231231
buf[1] = 0; /* remain_len = 0 */
232-
rc = MqttDecode_ConnectAck(buf, 2, NULL);
232+
XMEMSET(&ack, 0, sizeof(ack));
233+
rc = MqttDecode_ConnectAck(buf, 2, &ack);
233234
CHECK(rc == MQTT_CODE_ERROR_MALFORMED_DATA,
234235
"CONNACK remain_len=0: returns MALFORMED_DATA");
235236

236237
/* Malformed CONNACK: remain_len=1 */
237238
buf[0] = MQTT_PACKET_TYPE_SET(MQTT_PACKET_TYPE_CONNECT_ACK);
238239
buf[1] = 1; /* remain_len = 1 */
239240
buf[2] = 0; /* only flags, missing return_code */
240-
rc = MqttDecode_ConnectAck(buf, 3, NULL);
241+
XMEMSET(&ack, 0, sizeof(ack));
242+
rc = MqttDecode_ConnectAck(buf, 3, &ack);
241243
CHECK(rc == MQTT_CODE_ERROR_MALFORMED_DATA,
242244
"CONNACK remain_len=1: returns MALFORMED_DATA");
243245
}

‎wolfmqtt/mqtt_types.h‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,25 @@ enum MqttPacketResponseCodes {
358358
#define WOLFMQTT_NORETURN
359359
#endif
360360

361+
/* Secure memory zeroing - prevents compiler dead-store elimination */
362+
#ifndef WOLFMQTT_FORCE_ZERO
363+
#ifdef ENABLE_MQTT_TLS
364+
#include <wolfssl/wolfcrypt/memory.h>
365+
#define WOLFMQTT_FORCE_ZERO(mem, len) wc_ForceZero(mem, (word32)(len))
366+
#else
367+
static INLINE void wolfmqtt_force_zero(void* mem, word32 len)
368+
{
369+
volatile byte* p = (volatile byte*)mem;
370+
word32 i;
371+
for (i = 0; i < len; i++) {
372+
p[i] = 0;
373+
}
374+
}
375+
#define WOLFMQTT_FORCE_ZERO(mem, len) \
376+
wolfmqtt_force_zero(mem, (word32)(len))
377+
#endif
378+
#endif
379+
361380
/* Logging / Tracing */
362381
#ifdef WOLFMQTT_NO_STDIO
363382
#undef WOLFMQTT_DEBUG_CLIENT

0 commit comments

Comments
 (0)