Fix various spec compliance gaps#504
Conversation
There was a problem hiding this comment.
Pull request overview
This PR targets MQTT v3.1.1 compliance for fixed-header reserved flags (notably SUBSCRIBE) by validating the first-byte flag nibble during decode/dispatch and ensuring malformed packets lead to connection close in the broker.
Changes:
- Add a public helper to validate fixed-header reserved flags (
MqttPacket_FixedHeaderFlagsValid) and use it fromMqttDecode_FixedHeader. - Update broker dispatch to (a) pre-validate fixed-header flags for packet types not run through decoders and (b) close connections when handlers report malformed/protocol errors.
- Add unit tests covering canonical/invalid fixed-header flag permutations and end-to-end decode rejection for SUBSCRIBE/UNSUBSCRIBE/PUBREL and malformed PUBLISH flags.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
wolfmqtt/mqtt_packet.h |
Exposes new fixed-header flag validation helper as public API. |
src/mqtt_packet.c |
Implements reserved-flag validation and enforces it in MqttDecode_FixedHeader. |
src/mqtt_broker.c |
Closes connections on malformed packets by validating flags pre-dispatch and honoring handler error returns. |
tests/test_mqtt_packet.c |
Adds coverage for valid/invalid reserved-flag nibbles and malformed PUBLISH QoS/DUP combinations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* [MQTT-2.2.2-2] Reject malformed fixed-header reserved flags. The | ||
| * per-type decoders also enforce this (see MqttDecode_FixedHeader), | ||
| * but PUBACK / PUBCOMP / PINGREQ / DISCONNECT are not run through a | ||
| * decoder here, so the broker enforces it directly before dispatch. */ | ||
| if (!MqttPacket_FixedHeaderFlagsValid(bc->rx_buf[0])) { | ||
| WBLOG_ERR(broker, | ||
| "broker: invalid fixed-header flags type=%u byte=0x%02X " | ||
| "sock=%d [MQTT-2.2.2-2]", |
There was a problem hiding this comment.
This check only validates the low-nibble fixed-header flags; it does not ensure the packet type itself is supported in the current session. As a result, packet types that the broker does not handle (e.g., AUTH, or RESERVED/unknown types) can pass this validation and then hit the switch default: (no-op), keeping the connection open and updating last_rx. Consider explicitly treating unsupported/unknown packet types as malformed here (and closing the connection), and additionally rejecting AUTH when bc->protocol_level < MQTT_CONNECT_PROTOCOL_LEVEL_5.
| TEST(fixed_header_flags_valid_canonical_values) | ||
| { | ||
| /* Canonical first-byte values for each fixed-flag packet type. */ | ||
| ASSERT_EQ(1, MqttPacket_FixedHeaderFlagsValid(0x10)); /* CONNECT */ | ||
| ASSERT_EQ(1, MqttPacket_FixedHeaderFlagsValid(0x20)); /* CONNACK */ | ||
| ASSERT_EQ(1, MqttPacket_FixedHeaderFlagsValid(0x40)); /* PUBACK */ | ||
| ASSERT_EQ(1, MqttPacket_FixedHeaderFlagsValid(0x50)); /* PUBREC */ | ||
| ASSERT_EQ(1, MqttPacket_FixedHeaderFlagsValid(0x62)); /* PUBREL */ | ||
| ASSERT_EQ(1, MqttPacket_FixedHeaderFlagsValid(0x70)); /* PUBCOMP */ | ||
| ASSERT_EQ(1, MqttPacket_FixedHeaderFlagsValid(0x82)); /* SUBSCRIBE */ | ||
| ASSERT_EQ(1, MqttPacket_FixedHeaderFlagsValid(0x90)); /* SUBACK */ | ||
| ASSERT_EQ(1, MqttPacket_FixedHeaderFlagsValid(0xA2)); /* UNSUBSCRIBE */ |
There was a problem hiding this comment.
The new fixed-header flag validator tests don’t cover reserved/unknown packet types (e.g., first byte 0x00 / packet type 0). Since the broker uses this helper as a gate before dispatch, it’s worth adding an assertion that reserved/unknown packet types are treated as invalid (or, if that’s intentionally out of scope for this helper, adding a separate broker-level test that reserved types trigger disconnect).
| return header_len; | ||
| } | ||
|
|
||
| /* [MQTT-2.2.2-1] Required fixed-header reserved-flag values per packet type. |
There was a problem hiding this comment.
The comment tag references [MQTT-2.2.2-1], but the reserved fixed-header flag requirement being implemented here is [MQTT-2.2.2-2] (as also referenced elsewhere in this PR). Please correct the spec reference to avoid confusion when auditing compliance behavior.
| /* [MQTT-2.2.2-1] Required fixed-header reserved-flag values per packet type. | |
| /* [MQTT-2.2.2-2] Required fixed-header reserved-flag values per packet type. |
| /* Unknown/reserved type: this helper validates the flag nibble only. | ||
| * Callers are responsible for rejecting unknown packet types. */ | ||
| return 1; |
There was a problem hiding this comment.
MqttPacket_FixedHeaderFlagsValid currently returns 1 for unknown/reserved packet types (including type=0). That makes it unsafe to use as a protocol-level malformed check (e.g., in the broker), because reserved/unknown types will pass this validation and can be silently ignored downstream. Consider returning 0 for MQTT_PACKET_TYPE_RESERVED and for any unrecognized packet type, or otherwise making callers explicitly reject reserved/unknown types before treating a packet as valid.
| /* Unknown/reserved type: this helper validates the flag nibble only. | |
| * Callers are responsible for rejecting unknown packet types. */ | |
| return 1; | |
| /* Reject reserved/unknown packet types so this helper is safe to use | |
| * as a protocol-level malformed packet check. */ | |
| return 0; |
…he Generic Encoder
Thanks to @LiD0209 for the excellent issue reports