From d7bbe7531a89100e10e5a082b6cdc445df9edd72 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 25 May 2020 15:58:52 -0400 Subject: [PATCH 1/3] Additional protocol hardening. --- .../proxy/protocol/ProtocolUtils.java | 83 +++++++++++-------- .../protocol/netty/MinecraftDecoder.java | 6 +- .../protocol/util/NettyPreconditions.java | 26 +++++- 3 files changed, 72 insertions(+), 43 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java index 555885575..a112c9681 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/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); - checkState(buf.isReadable(length), + checkFrame(length <= cap * 4, "Bad string size (got %s, maximum is %s)", length, cap); + 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); - checkArgument(length <= cap, "Bad array size (got %s, maximum is %s)", length, cap); - checkState(buf.isReadable(length), + checkFrame(length >= 0, "Got a negative-length array (%s)", length); + checkFrame(length <= cap, "Bad array size (got %s, maximum is %s)", length, cap); + 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 - .checkArgument(b.length <= (FORGE_MAX_ARRAY_LENGTH), - "Cannot send array longer than %s (got %s bytes)", FORGE_MAX_ARRAY_LENGTH, - b.length); + checkFrame(b.length <= (FORGE_MAX_ARRAY_LENGTH), + "Cannot send array longer than %s (got %s bytes)", FORGE_MAX_ARRAY_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 - .checkArgument(b.readableBytes() <= (FORGE_MAX_ARRAY_LENGTH), - "Cannot send array longer than %s (got %s bytes)", FORGE_MAX_ARRAY_LENGTH, - b.readableBytes()); + checkFrame(b.readableBytes() <= (FORGE_MAX_ARRAY_LENGTH), + "Cannot send array longer than %s (got %s bytes)", FORGE_MAX_ARRAY_LENGTH, + 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(); - 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; + return readString(buf, DEFAULT_MAX_STRING_SIZE, buf.readableBytes()); } public enum Direction { diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftDecoder.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftDecoder.java index 0204aefb3..c37fef224 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftDecoder.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftDecoder.java @@ -14,7 +14,7 @@ import java.util.List; public class MinecraftDecoder extends MessageToMessageDecoder { - 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 { */ public MinecraftDecoder(ProtocolUtils.Direction direction) { this.direction = Preconditions.checkNotNull(direction, "direction"); - this.registry = direction - .getProtocolRegistry(StateRegistry.HANDSHAKE, ProtocolVersion.MINIMUM_VERSION); + this.registry = direction.getProtocolRegistry(StateRegistry.HANDSHAKE, + ProtocolVersion.MINIMUM_VERSION); this.state = StateRegistry.HANDSHAKE; } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/util/NettyPreconditions.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/util/NettyPreconditions.java index 5c5cabde1..cd3d46452 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/util/NettyPreconditions.java +++ b/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; + } } } } From ebad3d1005bfba1dc0e9262ddd1fb79a095a03c9 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 25 May 2020 16:05:36 -0400 Subject: [PATCH 2/3] Use Integer.MIN_VALUE for the sentinel for readVarIntSafely() --- .../com/velocitypowered/proxy/protocol/ProtocolUtils.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java index a112c9681..5c602014f 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -28,7 +28,7 @@ public enum ProtocolUtils { */ public static int readVarInt(ByteBuf buf) { int read = readVarIntSafely(buf); - if (read == -1) { + if (read == Integer.MIN_VALUE) { throw MinecraftDecoder.DEBUG ? BAD_VARINT_CACHED : new CorruptedFrameException("Bad varint decoded"); } @@ -40,19 +40,19 @@ public enum ProtocolUtils { * 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 + * @return the decoded VarInt, or {@code Integer.MIN_VALUE} if the varint is invalid */ public static int readVarIntSafely(ByteBuf buf) { int i = 0; int j = 0; while (true) { if (!buf.isReadable()) { - return -1; + return Integer.MIN_VALUE; } int k = buf.readByte(); i |= (k & 0x7F) << j++ * 7; if (j > 5) { - return -1; + return Integer.MIN_VALUE; } if ((k & 0x80) != 128) { break; From d538516f4c014a431783da110e2a88290341d45f Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 25 May 2020 16:08:53 -0400 Subject: [PATCH 3/3] Bump Netty version --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index d3d19cdc9..8cb7655e4 100644 --- a/build.gradle +++ b/build.gradle @@ -24,7 +24,7 @@ allprojects { junitVersion = '5.3.0-M1' slf4jVersion = '1.7.25' log4jVersion = '2.11.2' - nettyVersion = '4.1.49.Final' + nettyVersion = '4.1.50.Final' guavaVersion = '25.1-jre' checkerFrameworkVersion = '2.7.0' configurateVersion = '3.6'