Skip to content

Fenrir fixes #566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aidangarske merged 3 commits into from
Aug 4, 2026
Merged

Fenrir fixes #566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
57c1e3c
Fenrir fixes
embhorn Aug 3, 2026
032326e
Fix from review
embhorn Aug 3, 2026
a0a0fc9
Fix from review
embhorn Aug 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 106 additions & 15 deletions src/mqtt_broker.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ static const char* BrokerLog_Sanitize(const char* src)
char* dst = pool[pool_idx];
word32 di = 0;

/* Clear the slot before reuse so no residue from a previous longer
* sanitized string survives past this call's NUL terminator. */
BROKER_FORCE_ZERO(dst, BROKER_LOG_SAN_SZ);

pool_idx = (pool_idx + 1) % BROKER_LOG_SAN_POOL;

if (src == NULL) {
Expand Down Expand Up @@ -1816,6 +1820,9 @@ static void BrokerClient_DrainOutQueue(BrokerClient* bc)
WBLOG_ERR(bc->broker,
"broker: drain encode failed sock=%d topic=%s rc=%d",
(int)bc->sock, BrokerLog_Sanitize(cur->topic), enc_rc);
/* Encode length is unknown on failure, so scrub the whole buffer
* in case a partial payload was written. */
BROKER_FORCE_ZERO(bc->tx_buf, BROKER_CLIENT_TX_SZ(bc));
/* Drop just this entry and continue. Encoding failure for
* a single message is not fatal to the connection. */
if (prev == NULL) {
Expand Down Expand Up @@ -1850,6 +1857,13 @@ static void BrokerClient_DrainOutQueue(BrokerClient* bc)
{
int wr_rc;
wr_rc = MqttPacket_Write(&bc->client, bc->tx_buf, enc_rc);
/* Scrub the forwarded PUBLISH (which may carry a replayed will or
* an application payload) once the buffer is idle. Skip only the
* MQTT_CODE_CONTINUE case, where a non-blocking or TLS-async send
* still references bc->tx_buf. */
if (wr_rc != MQTT_CODE_CONTINUE) {
BROKER_FORCE_ZERO(bc->tx_buf, enc_rc);
}
if (wr_rc < 0) {
/* Socket dropped (EPIPE/ECONNRESET/etc). Leave this
* entry in QUEUED state, do not advance, do not bump
Expand Down Expand Up @@ -2395,6 +2409,7 @@ WOLFMQTT_LOCAL void BrokerOrphan_DropFull(MqttBroker* broker,
broker->subs = next;
}
if (sp->filter) {
BROKER_FORCE_ZERO(sp->filter, XSTRLEN(sp->filter) + 1);
WOLFMQTT_FREE(sp->filter);
}
if (sp->client_id) {
Expand Down Expand Up @@ -2783,6 +2798,7 @@ static void BrokerSubs_RemoveClient(MqttBroker* broker, BrokerClient* bc)
broker->subs = next;
}
if (cur->filter) {
BROKER_FORCE_ZERO(cur->filter, XSTRLEN(cur->filter) + 1);
WOLFMQTT_FREE(cur->filter);
}
if (cur->client_id) {
Expand Down Expand Up @@ -2974,6 +2990,7 @@ static void BrokerSubs_Remove(MqttBroker* broker, BrokerClient* bc,
}
WBLOG_INFO(broker, "broker: sub remove sock=%d filter=%s",
(int)bc->sock, BrokerLog_Sanitize(cur->filter));
BROKER_FORCE_ZERO(cur->filter, XSTRLEN(cur->filter) + 1);
WOLFMQTT_FREE(cur->filter);
if (cur->client_id) {
WOLFMQTT_FREE(cur->client_id);
Expand Down Expand Up @@ -3097,6 +3114,7 @@ static void BrokerSubs_RemoveByClientId(MqttBroker* broker,
broker->subs = next;
}
if (cur->filter) {
BROKER_FORCE_ZERO(cur->filter, XSTRLEN(cur->filter) + 1);
WOLFMQTT_FREE(cur->filter);
}
if (cur->client_id) {
Expand Down Expand Up @@ -3901,8 +3919,7 @@ static void BrokerRetained_DeliverToClient(MqttBroker* broker,
* MQTT_CODE_CONTINUE with the send still referencing bc->tx_buf,
* so zeroing then would corrupt it (that residue is cleared by
* the next full write or by BrokerClient_Free). */
if (wr_rc == enc_rc ||
(wr_rc < 0 && wr_rc != MQTT_CODE_CONTINUE)) {
if (wr_rc != MQTT_CODE_CONTINUE) {
/* Scrub the retained (possibly retained-will) payload from
* the subscriber tx_buf, mirroring the will fan-out. */
BROKER_FORCE_ZERO(bc->tx_buf, enc_rc);
Expand Down Expand Up @@ -3982,8 +3999,7 @@ static void BrokerRetained_DeliverToClient(MqttBroker* broker,
* MQTT_CODE_CONTINUE with the send still referencing bc->tx_buf,
* so zeroing then would corrupt it (that residue is cleared by
* the next full write or by BrokerClient_Free). */
if (wr_rc == enc_rc ||
(wr_rc < 0 && wr_rc != MQTT_CODE_CONTINUE)) {
if (wr_rc != MQTT_CODE_CONTINUE) {
/* Scrub the retained (possibly retained-will) payload from
* the subscriber tx_buf, mirroring the will fan-out. */
BROKER_FORCE_ZERO(bc->tx_buf, enc_rc);
Expand Down Expand Up @@ -4148,8 +4164,7 @@ static void BrokerClient_PublishWillImmediate(MqttBroker* broker,
* MQTT_CODE_CONTINUE with the send still referencing tx_buf, so
* zeroing then would corrupt it. Scrubbing on the error path
* keeps the will payload from lingering after a failed send. */
if (wr_rc == enc_rc ||
(wr_rc < 0 && wr_rc != MQTT_CODE_CONTINUE)) {
if (wr_rc != MQTT_CODE_CONTINUE) {
BROKER_FORCE_ZERO(sub->client->tx_buf, enc_rc);
}
}
Expand Down Expand Up @@ -5550,18 +5565,24 @@ static int BrokerHandle_Publish(BrokerClient* bc, int rx_len,
wr = MqttPacket_Write(&sub->client->client,
sub->client->tx_buf, sub_rc);
/* Static fan-out has no per-subscriber resume queue, so
* a partial write leaves this subscriber's stream
* desynced and unrecoverable. Tear down its socket and
* clear connected; the main loop reaps it on the next
* read error, and the match guard above then skips this
* client's other matching subscriptions. */
* anything short of a complete write leaves this
* subscriber's stream desynced and unrecoverable. Tear
* down its socket and clear connected; the main loop
* reaps it on the next read error, and the match guard
* above then skips this client's other matching
* subscriptions. */
if (wr != sub_rc &&
sub->client->sock != BROKER_SOCKET_INVALID) {
broker->net.close(broker->net.ctx,
sub->client->sock);
sub->client->sock = BROKER_SOCKET_INVALID;
sub->client->connected = 0;
}
/* Scrub the forwarded payload from the reusable tx_buf.
* On a complete write the send is done; on any other
* outcome the send is abandoned above (no resume queue),
* so nothing references the buffer either way. */
BROKER_FORCE_ZERO(sub->client->tx_buf, sub_rc);
}
else {
WBLOG_ERR(broker,
Expand Down Expand Up @@ -6422,8 +6443,14 @@ int MqttBroker_Start(MqttBroker* broker)
#ifdef WOLFMQTT_BROKER_PERSIST
/* Restore persisted state (orphan subs, retained messages) before
* opening the listen sockets so reconnecting clients see the
* resumed session immediately. No-op when no hooks are installed. */
(void)BrokerPersist_Restore(broker);
* resumed session immediately. No-op when no hooks are installed.
* A nonzero result means the backing store is unhealthy, so refuse to
* start rather than come up with silently incomplete state. */
rc = BrokerPersist_Restore(broker);
if (rc != MQTT_CODE_SUCCESS) {
WBLOG_ERR(broker, "broker: persistence restore failed rc=%d", rc);
return rc;
}
#endif

#ifdef ENABLE_MQTT_TLS
Expand Down Expand Up @@ -6589,7 +6616,11 @@ int MqttBroker_Run(MqttBroker* broker)
}
}

return MQTT_CODE_SUCCESS;
/* Propagate a fatal step error; a clean stop leaves rc idle or success. */
if (rc == MQTT_CODE_CONTINUE || rc > 0) {
rc = MQTT_CODE_SUCCESS;
}
return rc;
}

int MqttBroker_Stop(MqttBroker* broker)
Expand Down Expand Up @@ -6627,6 +6658,8 @@ int MqttBroker_Free(MqttBroker* broker)
while (broker->subs) {
BrokerSub* next = broker->subs->next;
if (broker->subs->filter) {
BROKER_FORCE_ZERO(broker->subs->filter,
XSTRLEN(broker->subs->filter) + 1);
WOLFMQTT_FREE(broker->subs->filter);
}
if (broker->subs->client_id) {
Expand Down Expand Up @@ -6801,12 +6834,56 @@ static int wolfmqtt_broker_dev_derive_key(void* ctx, byte* out_key,
}
#endif

#ifdef WOLFMQTT_BROKER_AUTH
/* Copy a CLI -P password into broker-owned storage and wipe the source argv
* slot so the plaintext leaves /proc/<pid>/cmdline. The destination is cleared
* first so no residue from a previous -P (or a rejected too-long value)
* survives. Exposed as WOLFMQTT_LOCAL for unit tests. */
WOLFMQTT_LOCAL int wolfmqtt_broker_set_auth_pass(MqttBroker* broker,
char* pass_arg, char* auth_pass_buf, word32 auth_pass_buf_sz)
{
word32 pass_len;

if (broker == NULL || pass_arg == NULL || auth_pass_buf == NULL) {
return MQTT_CODE_ERROR_BAD_ARG;
}
pass_len = (word32)XSTRLEN(pass_arg);

/* Clear any residue from a previous -P before copying or rejecting. */
BROKER_FORCE_ZERO(auth_pass_buf, auth_pass_buf_sz);

if (pass_len >= auth_pass_buf_sz) {
PRINTF("broker: -P password too long (max %d)",
(int)auth_pass_buf_sz - 1);
BROKER_FORCE_ZERO(pass_arg, pass_len);
return MQTT_CODE_ERROR_BAD_ARG;
}
XMEMCPY(auth_pass_buf, pass_arg, pass_len);
auth_pass_buf[pass_len] = '\0';
broker->auth_pass = auth_pass_buf;
BROKER_FORCE_ZERO(pass_arg, pass_len);
return MQTT_CODE_SUCCESS;
}

/* Wipe the stack copy of the CLI password on every exit path (including the
* early error returns) so the plaintext does not outlive the call. Matters for
* NO_MAIN_DRIVER builds that reuse the stack frame across invocations. No-op
* when broker auth is disabled. */
#define BROKER_WIPE_AUTH_PASS() \
BROKER_FORCE_ZERO(auth_pass_buf, sizeof(auth_pass_buf))
#else
#define BROKER_WIPE_AUTH_PASS() do {} while (0)
#endif

int wolfmqtt_broker(int argc, char** argv)
{
int rc;
MqttBroker broker;
MqttBrokerNet net;
int i;
#ifdef WOLFMQTT_BROKER_AUTH
char auth_pass_buf[BROKER_MAX_PASSWORD_LEN] = {0};
#endif
#ifdef WOLFMQTT_BROKER_PERSIST
MqttBrokerPersistHooks persist_hooks;
const char* persist_dir = NULL;
Expand Down Expand Up @@ -6860,7 +6937,11 @@ int wolfmqtt_broker(int argc, char** argv)
broker.auth_user = argv[++i];
}
else if (XSTRCMP(argv[i], "-P") == 0 && i + 1 < argc) {
Comment thread
aidangarske marked this conversation as resolved.
broker.auth_pass = argv[++i];
rc = wolfmqtt_broker_set_auth_pass(&broker, argv[++i],
auth_pass_buf, (word32)sizeof(auth_pass_buf));
if (rc != MQTT_CODE_SUCCESS) {
return rc;
}
}
#endif
#ifdef ENABLE_MQTT_TLS
Expand Down Expand Up @@ -6909,10 +6990,12 @@ int wolfmqtt_broker(int argc, char** argv)
#endif
else if (XSTRCMP(argv[i], "-h") == 0) {
BrokerUsage(argv[0]);
BROKER_WIPE_AUTH_PASS();
return 0;
}
else {
BrokerUsage(argv[0]);
BROKER_WIPE_AUTH_PASS();
return MQTT_CODE_ERROR_BAD_ARG;
}
}
Expand All @@ -6934,11 +7017,13 @@ int wolfmqtt_broker(int argc, char** argv)
PRINTF("broker: ERROR persist+encrypt build needs -E <source> "
"(only \"dev\" is recognized; production deployments "
"must install MqttBrokerPersistHooks.derive_key)");
BROKER_WIPE_AUTH_PASS();
return MQTT_CODE_ERROR_BAD_ARG;
}
if (XSTRCMP(encrypt_key_source, "dev") != 0) {
PRINTF("broker: ERROR unknown -E source \"%s\" "
"(only \"dev\" is recognized)", encrypt_key_source);
BROKER_WIPE_AUTH_PASS();
return MQTT_CODE_ERROR_BAD_ARG;
}
#else
Expand All @@ -6948,13 +7033,15 @@ int wolfmqtt_broker(int argc, char** argv)
PRINTF("broker: ERROR persist+encrypt build has no built-in key "
"source (rebuild with --enable-broker-persist-encrypt-dev-key "
"for testing, or install MqttBrokerPersistHooks.derive_key)");
BROKER_WIPE_AUTH_PASS();
return MQTT_CODE_ERROR_BAD_ARG;
#endif
#endif
rc = MqttBrokerNet_PersistPosix_Init(&persist_hooks, persist_dir);
if (rc != 0) {
PRINTF("broker: persist init failed dir=%s rc=%d",
persist_dir, rc);
BROKER_WIPE_AUTH_PASS();
return rc;
}
persist_initialized = 1;
Expand Down Expand Up @@ -7018,6 +7105,9 @@ int wolfmqtt_broker(int argc, char** argv)

MqttBroker_Free(&broker);

/* Erase the broker-owned password copy before returning. */
BROKER_WIPE_AUTH_PASS();

#ifdef WOLFMQTT_BROKER_PERSIST
if (persist_initialized) {
MqttBrokerNet_PersistPosix_Free(&persist_hooks);
Expand All @@ -7026,6 +7116,7 @@ int wolfmqtt_broker(int argc, char** argv)

return rc;
}
#undef BROKER_WIPE_AUTH_PASS

#ifndef NO_MAIN_DRIVER
int main(int argc, char** argv)
Expand Down
18 changes: 18 additions & 0 deletions src/mqtt_broker_persist_posix.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,18 @@ static int wmqb_posix_iter(void* ctx, byte ns, MqttBrokerPersist_IterCb cb,
void* cb_ctx);
static int wmqb_posix_sync(void* ctx);

/* Secure zeroing via a volatile pointer so the compiler cannot elide the
* stores. Kept file-local because MqttClient_ForceZero has hidden linkage
* and is not reachable from the standalone broker program. */
static void wmqb_posix_force_zero(void* mem, word32 len)
{
volatile byte* p = (volatile byte*)mem;
word32 i;
for (i = 0; i < len; i++) {
p[i] = 0;
}
}

/* hex encode key bytes into out (must be 2*key_len+1). Lowercase. */
static void wmqb_hex_encode(char* out, const byte* in, word16 in_len)
{
Expand Down Expand Up @@ -439,10 +451,16 @@ static int wmqb_posix_iter(void* ctx, byte ns, MqttBrokerPersist_IterCb cb,
}
(void)close(fd);
if (read_total != blob_cap) {
/* Scrub the record before releasing the heap buffer. In
* encrypt-at-rest builds the blob is ciphertext, so the wipe is
* harmless; in plaintext builds it clears session and payload
* data from a buffer that would otherwise linger in the heap. */
wmqb_posix_force_zero(blob, blob_cap);
WOLFMQTT_FREE(blob);
continue;
}
stop = cb(key_buf, (word16)kn, blob, blob_cap, cb_ctx);
wmqb_posix_force_zero(blob, blob_cap);
WOLFMQTT_FREE(blob);
if (stop != 0) {
break;
Expand Down
20 changes: 5 additions & 15 deletions src/mqtt_packet.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3837,21 +3837,11 @@ int MqttDecode_Auth(byte *rx_buf, int rx_buf_len, MqttAuth *auth)
return tmp;
rx_payload += tmp;
}
else if (auth->reason_code != MQTT_REASON_SUCCESS) {
/* The Reason Code and Property Length can be omitted if
the Reason Code is 0x00 (Success) and there are no
Properties. In this case the AUTH has a Remaining
Length of 0. */
return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA);
}
if (auth->props != NULL) {
/* Must have Authentication Method */

/* Must have Authentication Data */

/* May have zero or more User Property pairs */
}
else {
if (auth->props == NULL) {
/* An AUTH that carries a reason code must also carry
properties (at minimum the Authentication Method). The
short form (reason 0x00 Success, no properties) has a
Remaining Length of 0 and never reaches this branch. */
return MQTT_TRACE_ERROR(MQTT_CODE_ERROR_MALFORMED_DATA);
}
}
Expand Down
Loading
Loading