Fenrir fixes

[embhorn]

• f-7478: check and return the BrokerPersist_Restore error in MqttBroker_Start instead of discarding it
• f-7479: re-read the full datagram from offset 0 on the non-DTLS MQTT-SN peek path instead of short-reading the header
• f-7485: copy the -P broker password into private storage and scrub the argv slot so it leaves /proc//cmdline
• f-7476: same -P argv password exposure as f-7485, resolved by the same copy and scrub
• f-7480: remove the unreachable EWOULDBLOCK/EAGAIN branch in MqttSocket_Read and MqttSocket_Write
• f-7481: reject a non-zero willMsgLen with a NULL willMsg buffer in SN_Encode_WillMsg and SN_Encode_WillMsgUpdate
• f-7482: propagate a fatal MqttBroker_Step error from MqttBroker_Run instead of always returning success
• f-7486: zeroize the subscriber tx_buf after forwarding a PUBLISH in the drain and static fan-out paths
• f-7487: clear the BrokerLog_Sanitize pool slot before reuse so no credential residue survives past a rotation
• f-7488: zeroize restored persist records before freeing them in the POSIX backend
• f-7477: zeroize subscription topic filters before freeing them
• f-7483: drop the redundant reason-code guard in MqttDecode_Auth
• f-7484: validate rx_buf_len in SN_Decode_Publish before reading the length byte

[Copilot]

Pull request overview

This PR applies a set of broker, socket, and MQTT-SN correctness/security fixes across the wolfMQTT embedded MQTT client/broker codebase, and adds regression tests to prevent reintroducing the issues.

Changes:

• Fix MQTT-SN packet handling regressions (buffer-length validation and non-DTLS “peek then re-read” datagram reads).
• Harden broker and persistence handling (propagate critical errors; scrub sensitive buffers and subscription filters; reduce credential residue).
• Add targeted unit tests covering new/changed behaviors and edge cases.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/test_mqtt_sn.c	Adds regression tests for short-buffer rejection in SN_Decode_Publish and NULL will-message buffer rejection in will-message encoders.
tests/test_mqtt_sn_client.c	Adds regression test ensuring non-DTLS MQTT-SN peek path re-reads full datagrams (CONNACK/SUBACK).
src/mqtt_socket.c	Removes unreachable EWOULDBLOCK/EAGAIN handling branches in read/write helpers.
src/mqtt_sn_packet.c	Adds will-message NULL-buffer guard, strengthens publish decode argument validation, and fixes non-DTLS datagram read length handling in SN_Packet_Read.
src/mqtt_packet.c	Simplifies/clarifies AUTH decode malformed-data guard behavior around required properties.
src/mqtt_broker.c	Improves persistence error propagation, broker run error propagation, and scrubs sensitive broker buffers/filters and CLI password handling.
src/mqtt_broker_persist_posix.c	Scrubs restored persistence blobs before freeing in the POSIX backend.

Suppressed comments (1)

src/mqtt_broker.c:6923

• The -P password too long early-return path wipes argv, but does not wipe auth_pass_buf. If -P was provided earlier in the same invocation (or the stack slot contains a prior password in NO_MAIN_DRIVER builds), this return can still leave plaintext credentials on the stack. Call BROKER_WIPE_AUTH_PASS() before returning here.

            if (pass_len >= (word32)sizeof(auth_pass_buf)) {
                PRINTF("broker: -P password too long (max %d)",
                    (int)sizeof(auth_pass_buf) - 1);
                BROKER_FORCE_ZERO(pass_arg, pass_len);
                return MQTT_CODE_ERROR_BAD_ARG;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #566

Scan targets checked: wolfmqtt-bugs, wolfmqtt-src

No new issues found in the changed files. ✅

[aidangarske]

Skoll Code Review

Scan type: review-security
Overall recommendation: APPROVE
Findings: 3 total — 3 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Low] -P too-long error path returns without wiping a previously copied password — src/mqtt_broker.c:6919-6923
• [Info] Drain-path tx_buf scrub condition misses a zero return from MqttPacket_Write — src/mqtt_broker.c:1864-1867
• [Info] New -P copy-and-scrub CLI path has no test coverage — src/mqtt_broker.c:6913-6929

Review generated by Skoll

[aidangarske]

🔵 [Low] -P too-long error path returns without wiping a previously copied password · Zeroization

This PR introduces auth_pass_buf plus the BROKER_WIPE_AUTH_PASS() macro, whose comment states the stack copy is wiped 'on every exit path (including the early error returns)' because NO_MAIN_DRIVER builds reuse the stack frame across invocations. The wipe was added to the -h return, the unknown-argument return, the three persist/-E error returns, and the normal end of the function - but NOT to the -P password too long error return inside the argument loop. If -P appears twice (e.g. -P shortpass -P <64+ char value>), the first -P copies the plaintext password into auth_pass_buf; the second hits the too-long branch, which zeroes only pass_arg (the argv slot) and returns MQTT_CODE_ERROR_BAD_ARG leaving the first password in the abandoned stack frame. This is the exact residue scenario the PR's own macro comment says it defends against, so the hardening added by f-7485/f-7476 is incomplete on this one path. Not exploitable in default main-driver builds (the process exits immediately), hence Low.

Fix: Add BROKER_WIPE_AUTH_PASS() before the return MQTT_CODE_ERROR_BAD_ARG; in the too-long branch, matching every other post-parse error return in this function.

[aidangarske]

⚪ [Info] Drain-path tx_buf scrub condition misses a zero return from MqttPacket_Write · Zeroization

The new scrub condition if (wr_rc == enc_rc || (wr_rc < 0 && wr_rc != MQTT_CODE_CONTINUE)) covers complete writes and hard errors, and correctly skips MQTT_CODE_CONTINUE. However, in blocking (non-WOLFMQTT_NONBLOCK) builds, MqttSocket_Write breaks its send loop and returns 0 when a custom net write callback returns 0. In that case wr_rc == 0 matches neither arm: the buffer is not scrubbed, and the pre-existing if (wr_rc < 0) check then treats the entry as sent, so the encoded PUBLISH payload (potentially a will or application payload - the data f-7486 targets) lingers in bc->tx_buf until the next encode. Unreachable with the built-in POSIX net (TCP send() with len>0 never returns 0); only custom-net embedders with a nonconforming callback are affected. The as-sent mishandling of 0 is pre-existing; only the scrub gap is new. By contrast, the static fan-out path handles this correctly since wr != sub_rc catches 0.

Fix: Tighten the condition to scrub on any terminal outcome, e.g. if (wr_rc != MQTT_CODE_CONTINUE) guarded to skip only the in-progress case, or explicitly include wr_rc == 0 alongside the negative-error arm.

[aidangarske]

⚪ [Info] New -P copy-and-scrub CLI path has no test coverage · Missing Tests

The SN changes in this PR ship with thorough new regression tests (sn_decode_publish_short_buffer_rejected, sn_encode_willmsg_null_msg_buffer_rejected, sn_encode_willmsgupd_null_msg_buffer_rejected, sn_nondtls_reads_full_frames - all verified passing in blocking and non-blocking builds). The broker-side changes have none: the -P copy/length-check/argv-scrub logic, the length-limit rejection (pass_len >= 64), and the MqttBroker_Start persist-restore failure propagation are untested. The length check interacts with MqttBroker_Start's own XSTRLEN(auth_pass) >= BROKER_MAX_PASSWORD_LEN startup check (both use the same bound, so behavior is consistent), but nothing exercises the too-long rejection or the repeated -P case where the wipe gap above lives.

Fix: Add a test (or extend tests/test_broker_connect.c) that invokes wolfmqtt_broker argument parsing with -P values at, below, and above BROKER_MAX_PASSWORD_LEN-1, and with duplicate -P flags, asserting the argv slot is zeroed and the error paths return BAD_ARG.