Skip to content

Commit 30f8f28

Browse files
netty-project-botdaguimu
netty-project-bot
and
daguimu
authored
Auto-port 4.1: Fix MQTT decoder size check after variable header replay (#16838)
Auto-port of #16787 to 4.1 Cherry-picked commit: 72df658 --- ## Problem The MQTT decoder can reject valid packets after the CVE-2026-44248 fix when several MQTT packets are present in the same cumulation buffer. If the current packet's variable header needs a replay, the decoder compares the total readable bytes in the buffer against `maxBytesInMessage`, so later packets can make the current in-limit packet look too large. ## Root Cause `READ_VARIABLE_HEADER` recorded `buffer.readableBytes()` before decoding the variable header. That value is the cumulation's total readable bytes, not the current MQTT packet's declared remaining length. When `decodeVariableHeader` throws `Signal.REPLAY`, the too-large decision must be based on `bytesRemainingBeforeVariableHeader` for the current packet. ## Fix - Use `bytesRemainingBeforeVariableHeader` when deciding whether to swallow `Signal.REPLAY` and raise `TooLongFrameException`. - Continue replaying when the current packet is within `maxBytesInMessage`, even if the cumulation contains additional bytes for following packets. ## Tests Added | Change Point | Test | |-------------|------| | Variable-header replay uses the current packet size instead of total cumulation bytes for the too-long check | `testPublishMessageIncompleteVariableHeaderDoesNotUseCumulationSizeForTooLongCheck()` verifies an incomplete in-limit PUBLISH variable header with extra cumulated PINGREQ packets does not emit an invalid message | | Oversized current packets still fail during variable-header replay | `testPublishMessageIncompleteVariableHeaderStillFailsWhenCurrentPacketTooLarge()` verifies an incomplete PUBLISH whose own remaining length exceeds `maxBytesInMessage` still emits a `TooLongFrameException` | ## Impact This restores decoding for valid in-limit MQTT packets batched in the same buffer while preserving the CVE fix: packets whose declared remaining length exceeds `maxBytesInMessage` still fail with `TooLongFrameException` even if variable-header decoding requests replay. Fixes #16776 Co-authored-by: Guimu <30684111+daguimu@users.noreply.github.com>
1 parent 4c1cb1d commit 30f8f28

2 files changed

Lines changed: 77 additions & 5 deletions

File tree

‎codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -97,19 +97,18 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf buffer, List<Object> ou
9797

9898
case READ_VARIABLE_HEADER: try {
9999
int bytesRemainingBeforeVariableHeader = bytesRemainingInVariablePart;
100-
int initialAvailableBytes = buffer.readableBytes();
101100
boolean bailOut = false;
102101
try {
103102
variableHeader = decodeVariableHeader(ctx, buffer, mqttFixedHeader);
104103
} catch (Signal signal) {
105-
if (initialAvailableBytes < maxBytesInMessage) {
106-
// Ask for REPLAY if the buffer was less than maxBytesInMessage
107-
throw signal;
108-
} else {
104+
if (bytesRemainingBeforeVariableHeader > maxBytesInMessage) {
109105
// We couldn't parse the complete message, and it's already too large.
110106
// Swallow the Signal (we don't need more data) and instead bail out
111107
// and throw the TooLongFrameException below.
112108
bailOut = true;
109+
} else {
110+
// Ask for REPLAY if the current message is within maxBytesInMessage.
111+
throw signal;
113112
}
114113
}
115114
if (bailOut || bytesRemainingBeforeVariableHeader > maxBytesInMessage) {

‎codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import io.netty.buffer.UnpooledByteBufAllocator;
2222
import io.netty.channel.Channel;
2323
import io.netty.channel.ChannelHandlerContext;
24+
import io.netty.channel.embedded.EmbeddedChannel;
2425
import io.netty.handler.codec.DecoderException;
2526
import io.netty.handler.codec.EncoderException;
2627
import io.netty.handler.codec.TooLongFrameException;
@@ -311,6 +312,78 @@ public void testPublishMessage() throws Exception {
311312
validatePublishPayload(message.payload(), decodedMessage.payload());
312313
}
313314

315+
@Test
316+
public void testPublishMessageIncompleteVariableHeaderDoesNotUseCumulationSizeForTooLongCheck() throws Exception {
317+
// The leading PUBLISH is hand-crafted rather than going through MqttEncoder because the
318+
// bug under test only triggers when variable-header decoding asks for REPLAY mid-message,
319+
// which in turn requires a deliberately malformed packet (topic-name length prefix larger
320+
// than the bytes we actually supply). MqttEncoder only produces well-formed messages.
321+
final int maxBytesInMessage = 16;
322+
// bytes after the fixed header; < 128 so it fits in a 1-byte Variable Byte Integer.
323+
final int currentPacketRemainingLength = 10;
324+
// > the bytes we write below, so the decoder must REPLAY mid-variable-header.
325+
final int claimedTopicNameLength = 32;
326+
final int followingPingReqPackets = 3;
327+
EmbeddedChannel channel = new EmbeddedChannel(new MqttDecoder(maxBytesInMessage));
328+
ByteBuf byteBuf = ALLOCATOR.buffer();
329+
// Leading PUBLISH packet (incomplete - missing most of the topic name):
330+
// Fixed header byte 1: PUBLISH (type 3), DUP=0, QoS=0, RETAIN=0.
331+
byteBuf.writeByte(0x30);
332+
// Fixed header remaining-length, encoded as a Variable Byte Integer (single byte for values < 128).
333+
byteBuf.writeByte(currentPacketRemainingLength);
334+
// Variable header: 2-byte topic-name length prefix.
335+
byteBuf.writeShort(claimedTopicNameLength);
336+
// ... + only 8 of the 32 topic-name bytes the prefix claims (so the decoder will ask for REPLAY).
337+
byteBuf.writeZero(currentPacketRemainingLength - 2);
338+
// Trailing PINGREQ packets - the cumulation bytes that the buggy size check used to look at.
339+
// Each PINGREQ is just a 2-byte fixed header: 0xC0 (type 12, flags 0) and remaining-length 0.
340+
for (int i = 0; i < followingPingReqPackets; i++) {
341+
byteBuf.writeByte(0xC0);
342+
byteBuf.writeByte(0);
343+
}
344+
345+
try {
346+
assertFalse(channel.writeInbound(byteBuf));
347+
assertNull(channel.readInbound());
348+
} finally {
349+
channel.finishAndReleaseAll();
350+
}
351+
}
352+
353+
@Test
354+
public void testPublishMessageIncompleteVariableHeaderStillFailsWhenCurrentPacketTooLarge() throws Exception {
355+
// Same hand-crafting rationale as the test above: a malformed (incomplete-topic) PUBLISH
356+
// is needed so variable-header decoding asks for REPLAY, which is the code path under test.
357+
final int maxBytesInMessage = 16;
358+
// Declared packet size already exceeds the limit; the in-flight check must still flag it.
359+
final int currentPacketRemainingLength = maxBytesInMessage + 1;
360+
// > the bytes we write below, so the decoder still asks for REPLAY mid-variable-header.
361+
final int claimedTopicNameLength = 32;
362+
EmbeddedChannel channel = new EmbeddedChannel(new MqttDecoder(maxBytesInMessage));
363+
ByteBuf byteBuf = ALLOCATOR.buffer();
364+
// Fixed header byte 1: PUBLISH (type 3), all flags 0.
365+
byteBuf.writeByte(0x30);
366+
// Fixed header remaining-length Variable Byte Integer: 17 (still a single byte since < 128).
367+
byteBuf.writeByte(currentPacketRemainingLength);
368+
// Variable header: 2-byte topic-name length prefix claiming 32 bytes.
369+
byteBuf.writeShort(claimedTopicNameLength);
370+
// ... + 14 zero bytes - fewer than the 32 claimed, so the decoder will ask for REPLAY.
371+
byteBuf.writeZero(maxBytesInMessage - 2);
372+
373+
try {
374+
assertTrue(channel.writeInbound(byteBuf));
375+
MqttMessage decodedMessage = channel.readInbound();
376+
try {
377+
assertTrue(decodedMessage.decoderResult().isFailure());
378+
assertInstanceOf(TooLongFrameException.class, decodedMessage.decoderResult().cause());
379+
} finally {
380+
ReferenceCountUtil.release(decodedMessage);
381+
}
382+
} finally {
383+
channel.finishAndReleaseAll();
384+
}
385+
}
386+
314387
@Test
315388
public void testPubAckMessage() throws Exception {
316389
testMessageWithOnlyFixedHeaderAndMessageIdVariableHeader(MqttMessageType.PUBACK);

0 commit comments

Comments
 (0)