Broker, client, and MQTT v5 packet validation and reliability fixes by aidangarske · Pull Request #552 · wolfSSL/wolfMQTT

aidangarske · 2026-06-12T17:24:00Z

Description

F-3825, F-4050, F-4051, F-4052, F-4059, F-4244, F-4245, F-4246, F-4247, F-4249, F-4304, F-4305, F-4307,
F-4308, F-4529, F-4652, F-4653, F-4654, F-4655, F-4656, F-4657, F-4658, F-4722, F-4723, F-4724, F-4726,
F-4727, F-4729, F-4772, F-4773, F-4776, F-4777, F-4928, F-4929, F-4932, F-4933, F-4991, F-4992, F-4993,
F-4994, F-4996, F-4997,  F-5115, F-5116, F-5143, F-5148, F-5149, F-5512, F-5766, F-5767, F-5768, F-5769,
F-5771, F-5861, F-5862, F-5863, F-5865, F-4725, F-5144, F-4927

… sub/unsub property cleanup

…shot sub iterator

…tion identifier

… interval

…tize peer strings in broker log sinks

…n-state mutation

…essage list

…ty count

…y callbacks

…gs in example log output

…pass

…erification, and log injection

…ner policy

…5 props, fix packet-size comparison

…ll re-entrancy, scoped wolfSSL cleanup, fan-out write reset

… PUBACK on topic alloc failure

…et credential staging buffer

…roker publish, enforce retained cap on persist restore

…ed before scrub)

…cap at 1, count restore cap skips

…unsubscribe-reject test

… reap tombstones after delivery

…fication passes

…al-write handling

…for localhost

…fter-free

embhorn

Couple minor things to clean up

…e topic comment

…isconnect, and Auth decoders

embhorn

Skoll Code Review

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

Posted findings

[Medium] Per-client subscription cap default can be surprisingly small and silently rejects subscriptions — src/mqtt_broker.c:2806-2812 (cap check); wolfmqtt/mqtt_broker.h:96-106 (default)
[Medium] MqttClient_Auth rx_buf scrub runs unlocked, racing concurrent reads in MULTITHREAD — src/mqtt_client.c:2924
[Low] Static fan-out: misleading comment and write attempted to a just-closed peer's other matching sub — src/mqtt_broker.c:5312-5325

Skipped findings

[Low] ~~New v5 DISCONNECT-with-Will (reason 0x04) broker path has no dedicated test~~
[Low] ~~MQTT_MAX_PROPS cap may reject spec-legal v5 messages with many User Properties~~

Review generated by Skoll

embhorn · 2026-06-16T12:48:36Z

+     * delivered auth->props to the property callback and then freed them and
+     * set auth->props = NULL. So the bytes are consumed before this scrub and
+     * the caller has no live pointer into rx_buf. */
+    CLIENT_FORCE_ZERO(client->rx_buf, client->rx_buf_len);


🟠 [Medium] MqttClient_Auth rx_buf scrub runs unlocked, racing concurrent reads in MULTITHREAD

The new CLIENT_FORCE_ZERO(client->rx_buf, client->rx_buf_len) scrub is placed AFTER MqttClient_WaitType has returned and after lockClient was released (line ~2914), and it is not guarded by lockRecv. In a WOLFMQTT_MULTITHREAD build another thread can already be inside…

Fix: Wrap the rx_buf scrub in the receive lock in MULTITHREAD builds, mirroring the tx_buf scrub discipline.

embhorn · 2026-06-16T12:48:36Z

-                        (void)MqttPacket_Write(&sub->client->client,
+                            BrokerLog_Sanitize(topic), eff_qos,
+                            (unsigned)pub.total_len);
+                        wr = MqttPacket_Write(&sub->client->client,


🔵 [Low] Static fan-out: misleading comment and write attempted to a just-closed peer's other matching sub

Two minor issues in the new static-memory partial-write teardown: (1) The comment states "Removal is deferred so next_sub stays valid this iteration," but next_sub only exists in the dynamic (#ifndef WOLFMQTT_STATIC_MEMORY) path; in static mode iteration advances via the…

Fix: Correct the comment and guard the fan-out match on connection liveness.

aidangarske added 26 commits June 11, 2026 13:14

F-3825 F-4050 F-4051 F-4052 F-4059 - Harden v5 unsubscribe reject and…

ccce7eb

… sub/unsub property cleanup

F-4244 F-4307 - Defer WebSocket close during publish fan-out and snap…

60d20f6

…shot sub iterator

F-4246 F-4247 - Reject v5 empty topic without alias and zero subscrip…

57cfd36

…tion identifier

F-4245 F-4249 - Reject overlong static topics and clamp v5 will delay…

1ef1d06

… interval

F-4304 F-4654 F-4655 F-4722 F-4723 F-4727 F-4729 F-4776 F-4933 - Sani…

2581a2c

…tize peer strings in broker log sinks

F-4305 F-4652 - Validate broker CONNECT credentials before any sessio…

d46da08

…n-state mutation

F-4308 F-4653 - Reject v5 zero topic alias and cap dynamic retained-m…

b455240

…essage list

F-4529 - Add MqttClient_Subscribe broker-rejection regression test

5b97a8d

F-4657 F-4658 - Validate v5 response topic and cap per-message proper…

337f88d

…ty count

F-4656 - Cap per-client broker subscriptions to prevent table exhaustion

2c5ce32

F-4928 F-4929 F-4991 - Enforce TLS peer verification in example verif…

d9e79ce

…y callbacks

F-5766 F-5767 F-5768 F-5769 F-5861 - Sanitize broker-controlled strin…

087d9f0

…gs in example log output

F-4724 F-4772 - Fix firmware client length-overflow and topic-gate by…

c829dbc

…pass

F-5148 - Prevent broker client double-free on re-entrant takeover close

154bfae

F-4773 F-4992 F-5116 F-5143 F-5771 - Fix WebSocket fan-out UAF, TLS v…

be003af

…erification, and log injection

F-4726 F-5115 - Enforce WebSocket origin allowlist and TLS-only liste…

61870fb

…ner policy

F-4994 F-4996 F-5149 - Clamp publish payload copy, reject duplicate v…

dc8ab1e

…5 props, fix packet-size comparison

F-4997 F-5862 F-5863 F-5865 - Broker disconnect-with-will, pending-wi…

356fd47

…ll re-entrancy, scoped wolfSSL cleanup, fan-out write reset

F-4777 F-4932 - Reject client PUBLISH subscription id and avoid false…

0154f26

… PUBACK on topic alloc failure

F-4993 F-5512 - Add broker pre-CONNECT idle timeout and scrub WebSock…

dcbc2c0

…et credential staging buffer

Address skoll review: bound UNSUBACK reason codes, reject oversized b…

ab130c2

…roker publish, enforce retained cap on persist restore

Address skoll review: guard v5-only test registrations for non-v5 builds

0fb17bc

Clarify MqttClient_Auth rx_buf scrub ordering (skoll FP: props consum…

6bec2fb

…ed before scrub)

Address skoll review: free v5 DISCONNECT props, floor per-client sub …

41dabde

…cap at 1, count restore cap skips

Address skoll review: map UNSUBSCRIBE_REJECTED string and add client …

bd8104d

…unsubscribe-reject test

Address skoll review: cancel deferred retained delete on re-store and…

7e14290

… reap tombstones after delivery

Copilot AI review requested due to automatic review settings June 12, 2026 17:24

Copilot started reviewing on behalf of aidangarske June 12, 2026 17:24 View session

Add localhost SAN to broker test certs so WebSocket TLS hostname veri…

abb3dcc

…fication passes

This comment was marked as resolved.

Sign in to view

aidangarske added 2 commits June 12, 2026 10:48

Enforce example TLS peer verification only when a CA is configured

bf581a1

Harden publish encode clamp, firmware length prints, and broker parti…

9447505

…al-write handling

aidangarske requested a review from wolfSSL-Fenrir-bot June 12, 2026 17:55

This comment was marked as resolved.

Sign in to view

aidangarske added 2 commits June 12, 2026 11:06

F-4927 - Enforce curl TLS hostname verification instead of bypassing …

3004ba9

…for localhost

Defer nested retained-delivery reap to outermost depth to avoid use-a…

c138692

…fter-free

aidangarske requested a review from embhorn June 12, 2026 19:04

aidangarske assigned embhorn Jun 12, 2026

embhorn requested changes Jun 12, 2026

View reviewed changes

embhorn mentioned this pull request Jun 15, 2026

bound remain_len in publishresp, disconnect and auth decoders #553

Open

aidangarske added 2 commits June 15, 2026 09:38

Drop CWE and internal issue references from comments and trim firmwar…

8e5ee6c

…e topic comment

Regenerate broker test certs as X.509 v3 with localhost SAN

cc4de83

aidangarske requested a review from embhorn June 15, 2026 16:59

Bound reason-code read by received buffer length in v5 PublishResp, D…

408d499

…isconnect, and Auth decoders

embhorn reviewed Jun 16, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Broker, client, and MQTT v5 packet validation and reliability fixes#552

Broker, client, and MQTT v5 packet validation and reliability fixes#552
aidangarske wants to merge 34 commits into
masterfrom
fenrir-highmed-fixes

aidangarske commented Jun 12, 2026 •

edited

Loading

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

embhorn left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

embhorn left a comment

Uh oh!

embhorn Jun 16, 2026

Uh oh!

embhorn Jun 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

aidangarske commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

embhorn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

embhorn left a comment

Choose a reason for hiding this comment

Skoll Code Review

Posted findings

Skipped findings

Uh oh!

embhorn Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

embhorn Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aidangarske commented Jun 12, 2026 •

edited

Loading