Reimplement packet length checks

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

@@ -398,14 +398,15 @@ public enum ProtocolUtils {
    * @param buf the buffer to read from
    * @return the read byte array
    */
-  public static byte[] readByteArray17(ByteBuf buf) {
+  public static byte[] readByteArray17(ByteBuf buf, int cap) {
     // Read in a 2 or 3 byte number that represents the length of the packet. (3 byte "shorts" for
     // Forge only)
     // No vanilla packet should give a 3 byte packet
     int len = readExtendedForgeShort(buf);
 
-    checkArgument(len <= FORGE_MAX_ARRAY_LENGTH,
-        "Cannot receive array longer than %s (got %s bytes)", FORGE_MAX_ARRAY_LENGTH, len);
+    int maximumCap = Math.min(FORGE_MAX_ARRAY_LENGTH, cap);
+    checkArgument(len <= maximumCap,
+        "Cannot receive array longer than %s (got %s bytes)", maximumCap, len);
 
     byte[] ret = new byte[len];
     buf.readBytes(ret);

proxy/src/main/java/com/velocitypowered/proxy/network/packet/AbstractStatusPingPacket.java

@@ -18,13 +18,28 @@
 package com.velocitypowered.proxy.network.packet;
 
 import com.google.common.base.MoreObjects;
+import com.velocitypowered.api.network.ProtocolVersion;
+import io.netty.buffer.ByteBuf;
 import java.util.function.LongFunction;
 
 public abstract class AbstractStatusPingPacket implements Packet {
   protected static <P extends AbstractStatusPingPacket> PacketReader<P> decoder(final LongFunction<P> factory) {
-    return (buf, version) -> {
+    return new PacketReader<P>() {
+      @Override
+      public P read(ByteBuf buf, ProtocolVersion version) {
         final long randomId = buf.readLong();
         return factory.apply(randomId);
+      }
+
+      @Override
+      public int expectedMaxLength(ByteBuf buf, ProtocolVersion version) {
+        return 8;
+      }
+
+      @Override
+      public int expectedMinLength(ByteBuf buf, ProtocolVersion version) {
+        return 8;
+      }
     };
   }
 

proxy/src/main/java/com/velocitypowered/proxy/network/packet/Packet.java

@@ -33,13 +33,4 @@ public interface Packet {
   }
 
   boolean handle(PacketHandler handler);
-
-  // TODO: Move this into decoder
-  default int expectedMinLength(ByteBuf buf, PacketDirection direction, ProtocolVersion version) {
-    return 0;
-  }
-
-  default int expectedMaxLength(ByteBuf buf, PacketDirection direction, ProtocolVersion version) {
-    return -1;
-  }
 }

proxy/src/main/java/com/velocitypowered/proxy/network/packet/PacketReader.java

@@ -24,11 +24,11 @@ import java.util.function.Supplier;
 public interface PacketReader<P extends Packet> {
   P read(final ByteBuf buf, final ProtocolVersion version);
 
-  default int expectedMinLength(ByteBuf buf, PacketDirection direction, ProtocolVersion version) {
+  default int expectedMinLength(ByteBuf buf, ProtocolVersion version) {
     return 0;
   }
 
-  default int expectedMaxLength(ByteBuf buf, PacketDirection direction, ProtocolVersion version) {
+  default int expectedMaxLength(ByteBuf buf, ProtocolVersion version) {
     return -1;
   }
 

proxy/src/main/java/com/velocitypowered/proxy/network/packet/clientbound/ClientboundEncryptionRequestPacket.java

@@ -60,8 +60,8 @@ public class ClientboundEncryptionRequestPacket implements Packet {
       publicKey = ProtocolUtils.readByteArray(buf, 256);
       verifyToken = ProtocolUtils.readByteArray(buf, 16);
     } else {
-      publicKey = ProtocolUtils.readByteArray17(buf);
-      verifyToken = ProtocolUtils.readByteArray17(buf);
+      publicKey = ProtocolUtils.readByteArray17(buf, 256);
+      verifyToken = ProtocolUtils.readByteArray17(buf, 16);
     }
   }
 

proxy/src/main/java/com/velocitypowered/proxy/network/packet/serverbound/ServerboundEncryptionResponsePacket.java

@@ -27,17 +27,34 @@ import com.velocitypowered.proxy.network.packet.PacketWriter;
 import io.netty.buffer.ByteBuf;
 
 public class ServerboundEncryptionResponsePacket implements Packet {
-  public static final PacketReader<ServerboundEncryptionResponsePacket> DECODER = (buf, version) -> {
+  public static final PacketReader<ServerboundEncryptionResponsePacket> DECODER = new PacketReader<>() {
+    @Override
+    public ServerboundEncryptionResponsePacket read(ByteBuf buf,
+        ProtocolVersion version) {
       final byte[] sharedSecret;
       final byte[] verifyToken;
       if (version.gte(ProtocolVersion.MINECRAFT_1_8)) {
-      sharedSecret = ProtocolUtils.readByteArray(buf, 256);
+        sharedSecret = ProtocolUtils.readByteArray(buf, 128);
         verifyToken = ProtocolUtils.readByteArray(buf, 128);
       } else {
-      sharedSecret = ProtocolUtils.readByteArray17(buf);
-      verifyToken = ProtocolUtils.readByteArray17(buf);
+        sharedSecret = ProtocolUtils.readByteArray17(buf, 128);
+        verifyToken = ProtocolUtils.readByteArray17(buf, 128);
       }
       return new ServerboundEncryptionResponsePacket(sharedSecret, verifyToken);
+    }
+
+    @Override
+    public int expectedMaxLength(ByteBuf buf, ProtocolVersion version) {
+      // Both arrays are always 128 bytes long (due to padding), and each array has 2 bytes of
+      // padding (which applies to 1.7 since the length is written as a short and applies to 1.8+
+      // as 128 encodes as a 2-byte VarInt).
+      return 260;
+    }
+
+    @Override
+    public int expectedMinLength(ByteBuf buf, ProtocolVersion version) {
+      return expectedMaxLength(buf, version);
+    }
   };
   public static final PacketWriter<ServerboundEncryptionResponsePacket> ENCODER = PacketWriter.deprecatedEncode();
 

proxy/src/main/java/com/velocitypowered/proxy/network/packet/serverbound/ServerboundServerLoginPacket.java

@@ -18,23 +18,35 @@
 package com.velocitypowered.proxy.network.packet.serverbound;
 
 import com.google.common.base.MoreObjects;
+import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.proxy.network.ProtocolUtils;
 import com.velocitypowered.proxy.network.packet.Packet;
 import com.velocitypowered.proxy.network.packet.PacketHandler;
 import com.velocitypowered.proxy.network.packet.PacketReader;
 import com.velocitypowered.proxy.network.packet.PacketWriter;
 import com.velocitypowered.proxy.util.except.QuietDecoderException;
+import io.netty.buffer.ByteBuf;
 import java.util.Objects;
 
 public class ServerboundServerLoginPacket implements Packet {
   private static final QuietDecoderException EMPTY_USERNAME = new QuietDecoderException("Empty username!");
 
-  public static final PacketReader<ServerboundServerLoginPacket> DECODER = (buf, version) -> {
+  public static final PacketReader<ServerboundServerLoginPacket> DECODER = new PacketReader<>() {
+    @Override
+    public ServerboundServerLoginPacket read(ByteBuf buf, ProtocolVersion version) {
       final String username = ProtocolUtils.readString(buf, 16);
       if (username.isEmpty()) {
         throw EMPTY_USERNAME;
       }
       return new ServerboundServerLoginPacket(username);
+    }
+
+    @Override
+    public int expectedMaxLength(ByteBuf buf, ProtocolVersion version) {
+      // Accommodate the rare (but likely malicious) use of UTF-8 usernames, since it is technically
+      // legal on the protocol level.
+      return 1 + (16 * 4);
+    }
   };
 
   public static final PacketWriter<ServerboundServerLoginPacket> ENCODER = (buf, packet, version) ->

proxy/src/main/java/com/velocitypowered/proxy/network/pipeline/MinecraftDecoder.java

@@ -22,6 +22,7 @@ import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.proxy.network.ProtocolUtils;
 import com.velocitypowered.proxy.network.packet.Packet;
 import com.velocitypowered.proxy.network.packet.PacketDirection;
+import com.velocitypowered.proxy.network.packet.PacketReader;
 import com.velocitypowered.proxy.network.registry.packet.PacketRegistryMap;
 import com.velocitypowered.proxy.network.registry.protocol.ProtocolRegistry;
 import com.velocitypowered.proxy.network.registry.state.ProtocolStates;
@@ -75,17 +76,18 @@ public class MinecraftDecoder extends ChannelInboundHandlerAdapter {
     int packetId = ProtocolUtils.readVarInt(buf);
     Packet packet;
     try {
-      packet = this.registry.readPacket(packetId, buf, this.version);
+      packet = this.readPacket(packetId, buf);
     } catch (Exception e) {
       throw handleDecodeFailure(e, packetId);
     }
+
     if (packet == null) {
       buf.readerIndex(originalReaderIndex);
       ctx.fireChannelRead(buf);
     } else {
       try {
         if (buf.isReadable()) {
-          throw handleOverflow(packetId, buf.readerIndex(), buf.writerIndex());
+          throw handleOverflow(buf.readerIndex(), buf.writerIndex());
         }
         ctx.fireChannelRead(packet);
       } finally {
@@ -94,33 +96,37 @@ public class MinecraftDecoder extends ChannelInboundHandlerAdapter {
     }
   }
 
-  // TODO: Reimplement this
-  private void doLengthSanityChecks(ByteBuf buf, int packetId, Packet packet) throws Exception {
-    int expectedMinLen = packet.expectedMinLength(buf, direction, version);
-    int expectedMaxLen = packet.expectedMaxLength(buf, direction, version);
-    if (expectedMaxLen != -1 && buf.readableBytes() > expectedMaxLen) {
-      throw handleOverflow(packetId, expectedMaxLen, buf.readableBytes());
-    }
-    if (buf.readableBytes() < expectedMinLen) {
-      throw handleUnderflow(packetId, expectedMaxLen, buf.readableBytes());
-    }
+  private Packet readPacket(int packetId, ByteBuf buf) throws Exception {
+    PacketReader<? extends Packet> reader = this.registry.lookupReader(packetId, this.version);
+    if (reader == null) {
+      return null;
     }
 
-  private Exception handleOverflow(int packetId, int expected, int actual) {
+    int expectedMinLen = reader.expectedMinLength(buf, version);
+    int expectedMaxLen = reader.expectedMaxLength(buf, version);
+    if (expectedMaxLen != -1 && buf.readableBytes() > expectedMaxLen) {
+      throw handleOverflow(expectedMaxLen, buf.readableBytes());
+    }
+    if (buf.readableBytes() < expectedMinLen) {
+      throw handleUnderflow(expectedMaxLen, buf.readableBytes());
+    }
+
+    return reader.read(buf, version);
+  }
+
+  private Exception handleOverflow(int expected, int actual) {
     if (DEBUG) {
-      Class<? extends Packet> packetClass = this.registry.lookupPacket(packetId);
-      return new CorruptedFrameException("Packet sent for " + packetClass + " was too "
-          + "big (expected " + expected + " bytes, got " + actual + " bytes)");
+      return new CorruptedFrameException("Packet sent was too big (expected "
+          + expected + " bytes, got " + actual + " bytes)");
     } else {
       return DECODE_FAILED;
     }
   }
 
-  private Exception handleUnderflow(int packetId, int expected, int actual) {
+  private Exception handleUnderflow(int expected, int actual) {
     if (DEBUG) {
-      Class<? extends Packet> packetClass = this.registry.lookupPacket(packetId);
-      return new CorruptedFrameException("Packet sent for " + packetClass + " was too "
-          + "small (expected " + expected + " bytes, got " + actual + " bytes)");
+      return new CorruptedFrameException("Packet was too small (expected " + expected
+          + " bytes, got " + actual + " bytes)");
     } else {
       return DECODE_FAILED;
     }

proxy/src/main/java/com/velocitypowered/proxy/network/registry/packet/DensePacketRegistryMap.java

@@ -97,12 +97,13 @@ public class DensePacketRegistryMap implements PacketRegistryMap {
   }
 
   @Override
-  public @Nullable Packet readPacket(int id, ByteBuf buf, ProtocolVersion version) {
+  public @Nullable PacketReader<? extends Packet> lookupReader(final int id,
+      ProtocolVersion version) {
     if (id < 0 || id >= this.readersById.length) {
       return null;
     }
 
-    return this.readersById[id].read(buf, version);
+    return this.readersById[id];
   }
 
   @Override

proxy/src/main/java/com/velocitypowered/proxy/network/registry/packet/EmptyPacketRegistryMap.java

@@ -19,6 +19,7 @@ package com.velocitypowered.proxy.network.registry.packet;
 
 import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.proxy.network.packet.Packet;
+import com.velocitypowered.proxy.network.packet.PacketReader;
 import io.netty.buffer.ByteBuf;
 import org.checkerframework.checker.nullness.qual.Nullable;
 
@@ -31,7 +32,8 @@ public class EmptyPacketRegistryMap implements PacketRegistryMap {
   }
 
   @Override
-  public @Nullable Packet readPacket(int id, ByteBuf buf, ProtocolVersion version) {
+  public @Nullable PacketReader<? extends Packet> lookupReader(final int id,
+      ProtocolVersion version) {
     return null;
   }
 

proxy/src/main/java/com/velocitypowered/proxy/network/registry/packet/PacketRegistryMap.java

@@ -19,11 +19,12 @@ package com.velocitypowered.proxy.network.registry.packet;
 
 import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.proxy.network.packet.Packet;
+import com.velocitypowered.proxy.network.packet.PacketReader;
 import io.netty.buffer.ByteBuf;
 import org.checkerframework.checker.nullness.qual.Nullable;
 
 public interface PacketRegistryMap {
-  @Nullable Packet readPacket(final int id, ByteBuf buf, ProtocolVersion version);
+  @Nullable PacketReader<? extends Packet> lookupReader(final int id, ProtocolVersion version);
 
   <P extends Packet> void writePacket(P packet, ByteBuf buf, ProtocolVersion version);
 

proxy/src/main/java/com/velocitypowered/proxy/network/registry/packet/RegularPacketRegistryMap.java

@@ -58,12 +58,8 @@ public class RegularPacketRegistryMap implements PacketRegistryMap {
   }
 
   @Override
-  public @Nullable Packet readPacket(int id, ByteBuf buf, ProtocolVersion version) {
-    PacketReader<?> reader = this.readersById.get(id);
-    if (reader == null) {
-      return null;
-    }
-    return reader.read(buf, version);
+  public @Nullable PacketReader<?> lookupReader(int id, ProtocolVersion version) {
+    return this.readersById.get(id);
   }
 
   @Override

proxy/src/main/java/com/velocitypowered/proxy/network/registry/state/PlayPacketRegistry.java

@@ -432,16 +432,12 @@ class PlayPacketRegistry implements ProtocolRegistry {
      * Attempts to create a packet from the specified {@code id}.
      *
      * @param id the packet ID
-     * @param buf the bytebuf
      * @return the packet instance, or {@code null} if the ID is not registered
      */
     @Override
-    public @Nullable Packet readPacket(final int id, ByteBuf buf, ProtocolVersion version) {
-      final PacketReader<? extends Packet> decoder = this.packetIdToReader.get(id);
-      if (decoder == null) {
-        return null;
-      }
-      return decoder.read(buf, version);
+    public @Nullable PacketReader<? extends Packet> lookupReader(final int id,
+        ProtocolVersion version) {
+      return this.packetIdToReader.get(id);
     }
 
     /**