Additional protocol hardening.

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

@@ -1,13 +1,16 @@
 package com.velocitypowered.proxy.protocol;
 
 import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Preconditions.checkState;
+import static com.velocitypowered.proxy.protocol.util.NettyPreconditions.checkFrame;
 
 import com.google.common.base.Preconditions;
 import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.api.util.GameProfile;
+import com.velocitypowered.proxy.protocol.netty.MinecraftDecoder;
+import com.velocitypowered.proxy.util.except.QuietException;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.ByteBufUtil;
+import io.netty.handler.codec.CorruptedFrameException;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.List;
@@ -16,6 +19,7 @@ import java.util.UUID;
 public enum ProtocolUtils {
   ;
   private static final int DEFAULT_MAX_STRING_SIZE = 65536; // 64KiB
+  private static final QuietException BAD_VARINT_CACHED = new QuietException("Bad varint decoded");
 
   /**
    * Reads a Minecraft-style VarInt from the specified {@code buf}.
@@ -23,13 +27,32 @@ public enum ProtocolUtils {
    * @return the decoded VarInt
    */
   public static int readVarInt(ByteBuf buf) {
+    int read = readVarIntSafely(buf);
+    if (read == -1) {
+      throw MinecraftDecoder.DEBUG ? BAD_VARINT_CACHED
+          : new CorruptedFrameException("Bad varint decoded");
+    }
+    return read;
+  }
+
+  /**
+   * Reads a Minecraft-style VarInt from the specified {@code buf}. The difference between this
+   * method and {@link #readVarInt(ByteBuf)} is that this function returns a sentinel value if the
+   * varint is invalid.
+   * @param buf the buffer to read from
+   * @return the decoded VarInt, or {@code -1} if the varint is invalid
+   */
+  public static int readVarIntSafely(ByteBuf buf) {
     int i = 0;
     int j = 0;
     while (true) {
+      if (!buf.isReadable()) {
+        return -1;
+      }
       int k = buf.readByte();
       i |= (k & 0x7F) << j++ * 7;
       if (j > 5) {
-        throw new RuntimeException("VarInt too big");
+        return -1;
       }
       if ((k & 0x80) != 128) {
         break;
@@ -68,17 +91,21 @@ public enum ProtocolUtils {
    */
   public static String readString(ByteBuf buf, int cap) {
     int length = readVarInt(buf);
-    checkArgument(length >= 0, "Got a negative-length string (%s)", length);
+    return readString(buf, cap, length);
+  }
+
+  private static String readString(ByteBuf buf, int cap, int length) {
+    checkFrame(length >= 0, "Got a negative-length string (%s)", length);
     // `cap` is interpreted as a UTF-8 character length. To cover the full Unicode plane, we must
-    // consider the length of a UTF-8 character, which can be up to a 4 bytes. We do an initial
+    // consider the length of a UTF-8 character, which can be up to 4 bytes. We do an initial
     // sanity check and then check again to make sure our optimistic guess was good.
-    checkArgument(length <= cap * 4, "Bad string size (got %s, maximum is %s)", length, cap);
+    checkFrame(length <= cap * 4, "Bad string size (got %s, maximum is %s)", length, cap);
-    checkState(buf.isReadable(length),
+    checkFrame(buf.isReadable(length),
         "Trying to read a string that is too long (wanted %s, only have %s)", length,
         buf.readableBytes());
     String str = buf.toString(buf.readerIndex(), length, StandardCharsets.UTF_8);
     buf.skipBytes(length);
-    checkState(str.length() <= cap, "Got a too-long string (got %s, max %s)",
+    checkFrame(str.length() <= cap, "Got a too-long string (got %s, max %s)",
         str.length(), cap);
     return str;
   }
@@ -107,9 +134,9 @@ public enum ProtocolUtils {
    */
   public static byte[] readByteArray(ByteBuf buf, int cap) {
     int length = readVarInt(buf);
-    checkArgument(length >= 0, "Got a negative-length array (%s)", length);
+    checkFrame(length >= 0, "Got a negative-length array (%s)", length);
-    checkArgument(length <= cap, "Bad array size (got %s, maximum is %s)", length, cap);
+    checkFrame(length <= cap, "Bad array size (got %s, maximum is %s)", length, cap);
-    checkState(buf.isReadable(length),
+    checkFrame(buf.isReadable(length),
         "Trying to read an array that is too long (wanted %s, only have %s)", length,
         buf.readableBytes());
     byte[] array = new byte[length];
@@ -228,7 +255,7 @@ public enum ProtocolUtils {
     // No vanilla packet should give a 3 byte packet
     int len = readExtendedForgeShort(buf);
 
-    Preconditions.checkArgument(len <= (FORGE_MAX_ARRAY_LENGTH),
+    checkFrame(len <= (FORGE_MAX_ARRAY_LENGTH),
         "Cannot receive array longer than %s (got %s bytes)", FORGE_MAX_ARRAY_LENGTH, len);
 
     return buf.readRetainedSlice(len);
@@ -243,12 +270,11 @@ public enum ProtocolUtils {
    */
   public static void writeByteArray17(byte[] b, ByteBuf buf, boolean allowExtended) {
     if (allowExtended) {
-      Preconditions
+      checkFrame(b.length <= (FORGE_MAX_ARRAY_LENGTH),
-          .checkArgument(b.length <= (FORGE_MAX_ARRAY_LENGTH),
+          "Cannot send array longer than %s (got %s bytes)", FORGE_MAX_ARRAY_LENGTH,
-              "Cannot send array longer than %s (got %s bytes)", FORGE_MAX_ARRAY_LENGTH,
+          b.length);
-              b.length);
     } else {
-      Preconditions.checkArgument(b.length <= Short.MAX_VALUE,
+      checkFrame(b.length <= Short.MAX_VALUE,
           "Cannot send array longer than Short.MAX_VALUE (got %s bytes)", b.length);
     }
     // Write a 2 or 3 byte number that represents the length of the packet. (3 byte "shorts" for
@@ -268,12 +294,11 @@ public enum ProtocolUtils {
    */
   public static void writeByteBuf17(ByteBuf b, ByteBuf buf, boolean allowExtended) {
     if (allowExtended) {
-      Preconditions
+      checkFrame(b.readableBytes() <= (FORGE_MAX_ARRAY_LENGTH),
-          .checkArgument(b.readableBytes() <= (FORGE_MAX_ARRAY_LENGTH),
+          "Cannot send array longer than %s (got %s bytes)", FORGE_MAX_ARRAY_LENGTH,
-              "Cannot send array longer than %s (got %s bytes)", FORGE_MAX_ARRAY_LENGTH,
+          b.readableBytes());
-              b.readableBytes());
     } else {
-      Preconditions.checkArgument(b.readableBytes() <= Short.MAX_VALUE,
+      checkFrame(b.readableBytes() <= Short.MAX_VALUE,
           "Cannot send array longer than Short.MAX_VALUE (got %s bytes)", b.readableBytes());
     }
     // Write a 2 or 3 byte number that represents the length of the packet. (3 byte "shorts" for
@@ -326,21 +351,7 @@ public enum ProtocolUtils {
    * @return the decoded string
    */
   public static String readStringWithoutLength(ByteBuf buf) {
-    int length = buf.readableBytes();
+    return readString(buf, DEFAULT_MAX_STRING_SIZE, buf.readableBytes());
-    int cap = DEFAULT_MAX_STRING_SIZE;
-    checkArgument(length >= 0, "Got a negative-length string (%s)", length);
-    // `cap` is interpreted as a UTF-8 character length. To cover the full Unicode plane, we must
-    // consider the length of a UTF-8 character, which can be up to a 4 bytes. We do an initial
-    // sanity check and then check again to make sure our optimistic guess was good.
-    checkArgument(length <= cap * 4, "Bad string size (got %s, maximum is %s)", length, cap);
-    checkState(buf.isReadable(length),
-        "Trying to read a string that is too long (wanted %s, only have %s)", length,
-        buf.readableBytes());
-    String str = buf.toString(buf.readerIndex(), length, StandardCharsets.UTF_8);
-    buf.skipBytes(length);
-    checkState(str.length() <= cap, "Got a too-long string (got %s, max %s)",
-        str.length(), cap);
-    return str;
   }
 
   public enum Direction {

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

@@ -14,7 +14,7 @@ import java.util.List;
 
 public class MinecraftDecoder extends MessageToMessageDecoder<ByteBuf> {
 
-  private static final boolean DEBUG = Boolean.getBoolean("velocity.packet-decode-logging");
+  public static final boolean DEBUG = Boolean.getBoolean("velocity.packet-decode-logging");
   private static final QuietException DECODE_FAILED =
       new QuietException("A packet did not decode successfully (invalid data). If you are a "
           + "developer, launch Velocity with -Dvelocity.packet-decode-logging=true to see more.");
@@ -30,8 +30,8 @@ public class MinecraftDecoder extends MessageToMessageDecoder<ByteBuf> {
    */
   public MinecraftDecoder(ProtocolUtils.Direction direction) {
     this.direction = Preconditions.checkNotNull(direction, "direction");
-    this.registry = direction
+    this.registry = direction.getProtocolRegistry(StateRegistry.HANDSHAKE,
-        .getProtocolRegistry(StateRegistry.HANDSHAKE, ProtocolVersion.MINIMUM_VERSION);
+        ProtocolVersion.MINIMUM_VERSION);
     this.state = StateRegistry.HANDSHAKE;
   }
 

proxy/src/main/java/com/velocitypowered/proxy/protocol/util/NettyPreconditions.java

@@ -1,12 +1,18 @@
 package com.velocitypowered.proxy.protocol.util;
 
 import com.google.common.base.Strings;
+import com.velocitypowered.proxy.protocol.netty.MinecraftDecoder;
+import com.velocitypowered.proxy.util.except.QuietException;
 import io.netty.handler.codec.CorruptedFrameException;
 
 /**
  * Extends {@link com.google.common.base.Preconditions} for Netty's {@link CorruptedFrameException}.
  */
 public class NettyPreconditions {
+  private static final QuietException BAD = new QuietException(
+      "Invalid packet received. Launch Velocity with -Dvelocity.packet-decode-logging=true "
+          + "to see more.");
+
   private NettyPreconditions() {
     throw new AssertionError();
   }
@@ -18,7 +24,7 @@ public class NettyPreconditions {
    */
   public static void checkFrame(boolean b, String message) {
     if (!b) {
-      throw new CorruptedFrameException(message);
+      throw MinecraftDecoder.DEBUG ? new CorruptedFrameException(message) : BAD;
     }
   }
 
@@ -32,7 +38,11 @@ public class NettyPreconditions {
    */
   public static void checkFrame(boolean b, String message, Object arg1) {
     if (!b) {
-      throw new CorruptedFrameException(Strings.lenientFormat(message, arg1));
+      if (MinecraftDecoder.DEBUG) {
+        throw new CorruptedFrameException(Strings.lenientFormat(message, arg1));
+      } else {
+        throw BAD;
+      }
     }
   }
 
@@ -47,7 +57,11 @@ public class NettyPreconditions {
    */
   public static void checkFrame(boolean b, String message, Object arg1, Object arg2) {
     if (!b) {
-      throw new CorruptedFrameException(Strings.lenientFormat(message, arg1, arg2));
+      if (MinecraftDecoder.DEBUG) {
+        throw new CorruptedFrameException(Strings.lenientFormat(message, arg1, arg2));
+      } else {
+        throw BAD;
+      }
     }
   }
 
@@ -61,7 +75,11 @@ public class NettyPreconditions {
    */
   public static void checkFrame(boolean b, String message, Object... args) {
     if (!b) {
-      throw new CorruptedFrameException(Strings.lenientFormat(message, args));
+      if (MinecraftDecoder.DEBUG) {
+        throw new CorruptedFrameException(Strings.lenientFormat(message, args));
+      } else {
+        throw BAD;
+      }
     }
   }
 }