Repository: netty/netty
Issue: netty-codec MqttDecoder should validate CONNECT reserved flag in variable header
Labels: improvement
feature

The MQTT 3.1.1 Protocol Specification says:

> The Server MUST validate that the reserved flag in the CONNECT Control Packet is set to zero and disconnect the Client if it is not zero.
> MqttDecoder should validate and throw a exception if it not zero.


Issue comments:
@longkerdandy we love contributions... Maybe you could submit a PR with a fix ?

Fixed by https://github.com/netty/netty/pull/4986


Pull request review comments:
Only read this flag if the version is 3.1.1


Patch that resolved the issue:
diff --git a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java
index 70f0a41e7ba..7103bed6416 100644
--- a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java
+++ b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java
@@ -221,6 +221,15 @@ private static Result<MqttConnectVariableHeader> decodeConnectionVariableHeader(
         final int willQos = (b1 & 0x18) >> 3;
         final boolean willFlag = (b1 & 0x04) == 0x04;
         final boolean cleanSession = (b1 & 0x02) == 0x02;
+        if (mqttVersion == MqttVersion.MQTT_3_1_1) {
+            final boolean zeroReservedFlag = (b1 & 0x01) == 0x0;
+            if (!zeroReservedFlag) {
+                // MQTT v3.1.1: The Server MUST validate that the reserved flag in the CONNECT Control Packet is
+                // set to zero and disconnect the Client if it is not zero.
+                // See http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc385349230
+                throw new DecoderException("non-zero reserved flag");
+            }
+        }
 
         final MqttConnectVariableHeader mqttConnectVariableHeader = new MqttConnectVariableHeader(
                 mqttVersion.protocolName(),


Test patch:
diff --git a/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java b/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java
index 705d071bfd8..5068a941390 100644
--- a/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java
+++ b/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java
@@ -21,6 +21,7 @@
 import io.netty.buffer.UnpooledByteBufAllocator;
 import io.netty.channel.Channel;
 import io.netty.channel.ChannelHandlerContext;
+import io.netty.handler.codec.DecoderException;
 import io.netty.util.CharsetUtil;
 import org.easymock.Mock;
 import org.junit.Before;
@@ -96,6 +97,28 @@ public void testConnectMessageForMqtt311() throws Exception {
         validateConnectPayload(message.payload(), decodedMessage.payload());
     }
 
+    @Test
+    public void testConnectMessageWithNonZeroReservedFlagForMqtt311() throws Exception {
+        final MqttConnectMessage message = createConnectMessage(MqttVersion.MQTT_3_1_1);
+        ByteBuf byteBuf = MqttEncoder.doEncode(ALLOCATOR, message);
+        try {
+            // Set the reserved flag in the CONNECT Packet to 1
+            byteBuf.setByte(9, byteBuf.getByte(9) | 0x1);
+            final List<Object> out = new LinkedList<Object>();
+            mqttDecoder.decode(ctx, byteBuf, out);
+
+            assertEquals("Expected one object bout got " + out.size(), 1, out.size());
+
+            final MqttMessage decodedMessage = (MqttMessage) out.get(0);
+            assertTrue(decodedMessage.decoderResult().isFailure());
+            Throwable cause = decodedMessage.decoderResult().cause();
+            assertTrue(cause instanceof DecoderException);
+            assertEquals("non-zero reserved flag", cause.getMessage());
+        } finally {
+            byteBuf.release();
+        }
+    }
+
     @Test
     public void testConnAckMessage() throws Exception {
         final MqttConnAckMessage message = createConnAckMessage();