Add more protocol safeguards (#1743)

* Add max length to more config phase packets

* Ensure all packets during non-play state are known

* Limit maximum size of play inbound packet queue

Co-authored-by: Dwarslooper <dwarslooper2910@gmx.de>

* Fix checkstyle

---------

Co-authored-by: Dwarslooper <dwarslooper2910@gmx.de>

proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java

@@ -161,6 +161,9 @@ public enum ProtocolUtils {
     VAR_INT_LENGTHS[32] = 1; // Special case for the number 0.
   }
 
+  public static final int DEFAULT_MAX_STRING_BYTES = varIntBytes(ByteBufUtil.utf8MaxBytes(DEFAULT_MAX_STRING_SIZE))
+          + ByteBufUtil.utf8MaxBytes(DEFAULT_MAX_STRING_SIZE);
+
   private static DecoderException badVarint() {
     return MinecraftDecoder.DEBUG ? new CorruptedFrameException("Bad VarInt decoded")
         : BAD_VARINT_CACHED;

proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftDecoder.java

@@ -74,6 +74,10 @@ public class MinecraftDecoder extends ChannelInboundHandlerAdapter {
     MinecraftPacket packet = this.registry.createPacket(packetId);
     if (packet == null) {
       buf.readerIndex(originalReaderIndex);
+      if (this.direction == ProtocolUtils.Direction.SERVERBOUND && this.state != StateRegistry.PLAY) {
+        buf.release();
+        throw this.handleInvalidPacketId(packetId);
+      }
       ctx.fireChannelRead(buf);
     } else {
       try {
@@ -133,6 +137,14 @@ public class MinecraftDecoder extends ChannelInboundHandlerAdapter {
     }
   }
 
+  private Exception handleInvalidPacketId(int packetId) {
+    if (DEBUG) {
+      return new CorruptedFrameException("Invalid packet " + getExtraConnectionDetail(packetId));
+    } else {
+      return DECODE_FAILED;
+    }
+  }
+
   private String getExtraConnectionDetail(int packetId) {
     return "Direction " + direction + " Protocol " + registry.version + " State " + state
         + " ID 0x" + Integer.toHexString(packetId);

proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/PlayPacketQueueInboundHandler.java

@@ -21,6 +21,9 @@ import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.proxy.protocol.MinecraftPacket;
 import com.velocitypowered.proxy.protocol.ProtocolUtils;
 import com.velocitypowered.proxy.protocol.StateRegistry;
+import com.velocitypowered.proxy.util.except.QuietDecoderException;
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.ByteBufHolder;
 import io.netty.channel.ChannelDuplexHandler;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.util.ReferenceCountUtil;
@@ -41,8 +44,13 @@ import org.jetbrains.annotations.NotNull;
  */
 public class PlayPacketQueueInboundHandler extends ChannelDuplexHandler {
 
+  private static final int MAXIMUM_SIZE = Integer.getInteger("velocity.maximum-play-queue-size", 128 * 1024 * 1024); // 128MiB by default
+  private static final QuietDecoderException QUEUE_LIMIT_FAILED = new QuietDecoderException(
+      "Queue too big (greater than " + MAXIMUM_SIZE + " bytes)");
+
   private final StateRegistry.PacketRegistry.ProtocolRegistry registry;
   private final Queue<Object> queue = new ArrayDeque<>();
+  private int queueSize = 0;
 
   /**
    * Provides registries for client &amp; server bound packets.
@@ -64,6 +72,20 @@ public class PlayPacketQueueInboundHandler extends ChannelDuplexHandler {
       }
     }
 
+    int length = 0;
+    if (msg instanceof ByteBuf) {
+      // keep track of raw packets
+      length = ((ByteBuf) msg).readableBytes();
+    } else if (msg instanceof ByteBufHolder) {
+      // keep track of bytebufs wrapped inside packets
+      length = ((ByteBufHolder) msg).content().readableBytes();
+    }
+    if (this.queueSize + length > MAXIMUM_SIZE) {
+      ReferenceCountUtil.release(msg);
+      throw QUEUE_LIMIT_FAILED;
+    }
+    this.queueSize += length;
+
     // Otherwise, queue the packet
     this.queue.offer(msg);
   }
@@ -90,5 +112,6 @@ public class PlayPacketQueueInboundHandler extends ChannelDuplexHandler {
         ReferenceCountUtil.release(msg);
       }
     }
+    this.queueSize = 0;
   }
 }

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ClientSettingsPacket.java

@@ -22,8 +22,8 @@ import com.velocitypowered.proxy.connection.MinecraftSessionHandler;
 import com.velocitypowered.proxy.protocol.MinecraftPacket;
 import com.velocitypowered.proxy.protocol.ProtocolUtils;
 import io.netty.buffer.ByteBuf;
+import io.netty.buffer.ByteBufUtil;
 import java.util.Objects;
-
 import org.checkerframework.checker.nullness.qual.Nullable;
 
 public class ClientSettingsPacket implements MinecraftPacket {
@@ -206,6 +206,16 @@ public class ClientSettingsPacket implements MinecraftPacket {
     return handler.handle(this);
   }
 
+  @Override
+  public int decodeExpectedMaxLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) {
+    return 1 + ByteBufUtil.utf8MaxBytes(16) + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1;
+  }
+
+  @Override
+  public int decodeExpectedMinLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) {
+    return 1 + 0 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1;
+  }
+
   @Override
   public boolean equals(@Nullable final Object o) {
     if (this == o) {

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/KeepAlivePacket.java

@@ -64,6 +64,28 @@ public class KeepAlivePacket implements MinecraftPacket {
     }
   }
 
+  @Override
+  public int decodeExpectedMaxLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) {
+    if (version.noLessThan(ProtocolVersion.MINECRAFT_1_12_2)) {
+      return Long.BYTES;
+    } else if (version.noLessThan(ProtocolVersion.MINECRAFT_1_8)) {
+      return 5;
+    } else {
+      return Integer.BYTES;
+    }
+  }
+
+  @Override
+  public int decodeExpectedMinLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) {
+    if (version.noLessThan(ProtocolVersion.MINECRAFT_1_12_2)) {
+      return Long.BYTES;
+    } else if (version.noLessThan(ProtocolVersion.MINECRAFT_1_8)) {
+      return 1;
+    } else {
+      return Integer.BYTES;
+    }
+  }
+
   @Override
   public boolean handle(MinecraftSessionHandler handler) {
     return handler.handle(this);

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PingIdentifyPacket.java

@@ -42,6 +42,16 @@ public class PingIdentifyPacket implements MinecraftPacket {
     buf.writeInt(id);
   }
 
+  @Override
+  public int decodeExpectedMaxLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) {
+    return Integer.BYTES;
+  }
+
+  @Override
+  public int decodeExpectedMinLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) {
+    return Integer.BYTES;
+  }
+
   @Override
   public boolean handle(MinecraftSessionHandler handler) {
     return handler.handle(this);

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PluginMessagePacket.java

@@ -31,6 +31,8 @@ import org.checkerframework.checker.nullness.qual.Nullable;
 
 public class PluginMessagePacket extends DeferredByteBufHolder implements MinecraftPacket {
 
+  private static final int MAX_PAYLOAD_SIZE = Integer.getInteger("velocity.max-plugin-message-payload-size", 32767);
+
   private @Nullable String channel;
 
   public PluginMessagePacket() {
@@ -100,6 +102,16 @@ public class PluginMessagePacket extends DeferredByteBufHolder implements Minecr
 
   }
 
+  @Override
+  public int decodeExpectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    return ProtocolUtils.DEFAULT_MAX_STRING_BYTES + MAX_PAYLOAD_SIZE;
+  }
+
+  @Override
+  public int decodeExpectedMinLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    return 1 + 0 + 0;
+  }
+
   @Override
   public boolean handle(MinecraftSessionHandler handler) {
     return handler.handle(this);

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ResourcePackResponsePacket.java

@@ -80,6 +80,26 @@ public class ResourcePackResponsePacket implements MinecraftPacket {
     ProtocolUtils.writeVarInt(buf, status.ordinal());
   }
 
+  @Override
+  public int decodeExpectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    if (version.noLessThan(ProtocolVersion.MINECRAFT_1_20_3)) {
+      return Long.BYTES * 2 + 1;
+    } else if (version.noGreaterThan(ProtocolVersion.MINECRAFT_1_9_4)) {
+      return ProtocolUtils.DEFAULT_MAX_STRING_BYTES + 1;
+    }
+    return 1;
+  }
+
+  @Override
+  public int decodeExpectedMinLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    if (version.noLessThan(ProtocolVersion.MINECRAFT_1_20_3)) {
+      return Long.BYTES * 2 + 1;
+    } else if (version.noGreaterThan(ProtocolVersion.MINECRAFT_1_9_4)) {
+      return 1 + 0 + 1;
+    }
+    return 1;
+  }
+
   @Override
   public boolean handle(MinecraftSessionHandler handler) {
     return handler.handle(this);

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerboundCookieResponsePacket.java

@@ -65,6 +65,16 @@ public class ServerboundCookieResponsePacket implements MinecraftPacket {
     }
   }
 
+  @Override
+  public int decodeExpectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    return ProtocolUtils.DEFAULT_MAX_STRING_BYTES + 1 + 2 + 5120;
+  }
+
+  @Override
+  public int decodeExpectedMinLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    return 1 + 0 + 0;
+  }
+
   @Override
   public boolean handle(MinecraftSessionHandler handler) {
     return handler.handle(this);

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerboundCustomClickActionPacket.java

@@ -27,6 +27,8 @@ import io.netty.buffer.ByteBuf;
 
 public class ServerboundCustomClickActionPacket extends DeferredByteBufHolder implements MinecraftPacket {
 
+  private static final int MAX_TAG_SIZE = 65536;
+
   public ServerboundCustomClickActionPacket() {
     super(null);
   }
@@ -41,6 +43,16 @@ public class ServerboundCustomClickActionPacket extends DeferredByteBufHolder im
     buf.writeBytes(content());
   }
 
+  @Override
+  public int decodeExpectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    return ProtocolUtils.DEFAULT_MAX_STRING_BYTES + ProtocolUtils.varIntBytes(MAX_TAG_SIZE) + MAX_TAG_SIZE;
+  }
+
+  @Override
+  public int decodeExpectedMinLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    return 1 + 0 + 1 + 0;
+  }
+
   @Override
   public boolean handle(MinecraftSessionHandler handler) {
     return handler.handle(this);

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/config/ClientboundCustomReportDetailsPacket.java

@@ -22,7 +22,6 @@ import com.velocitypowered.proxy.connection.MinecraftSessionHandler;
 import com.velocitypowered.proxy.protocol.MinecraftPacket;
 import com.velocitypowered.proxy.protocol.ProtocolUtils;
 import io.netty.buffer.ByteBuf;
-import java.util.HashMap;
 import java.util.Map;
 
 public class ClientboundCustomReportDetailsPacket implements MinecraftPacket {

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/config/CodeOfConductAcceptPacket.java

@@ -38,6 +38,11 @@ public class CodeOfConductAcceptPacket implements MinecraftPacket {
   public void encode(ByteBuf buf, Direction direction, ProtocolVersion protocolVersion) {
   }
 
+  @Override
+  public int decodeExpectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) {
+    return 0;
+  }
+
   @Override
   public boolean handle(MinecraftSessionHandler handler) {
     return handler.handle(this);