From 5ceac16a821ea35572ff11412ace8929fd06e278 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Tue, 26 Jan 2021 12:33:35 -0500 Subject: [PATCH] Add upfront size checks for some packets. This is simply a further protection against certain attacks which send malformed packets to the proxy. --- .../proxy/protocol/MinecraftPacket.java | 8 +++++ .../protocol/netty/MinecraftDecoder.java | 30 ++++++++++++++++--- .../protocol/packet/EncryptionResponse.java | 15 +++++++++- .../proxy/protocol/packet/ServerLogin.java | 8 +++++ .../proxy/protocol/packet/StatusPing.java | 11 +++++++ .../proxy/protocol/packet/StatusRequest.java | 6 ++++ 6 files changed, 73 insertions(+), 5 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/MinecraftPacket.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/MinecraftPacket.java index 11bed9297..6f1705765 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/MinecraftPacket.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/MinecraftPacket.java @@ -11,4 +11,12 @@ public interface MinecraftPacket { void encode(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion protocolVersion); boolean handle(MinecraftSessionHandler handler); + + default int expectedMaxLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) { + return -1; + } + + default int expectedMinLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) { + return 0; + } } 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 9e442e493..aaf1a58e3 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 @@ -58,6 +58,8 @@ public class MinecraftDecoder extends ChannelInboundHandlerAdapter { ctx.fireChannelRead(buf); } else { try { + doLengthSanityChecks(buf, packet); + try { packet.decode(buf, direction, registry.version); } catch (Exception e) { @@ -65,7 +67,7 @@ public class MinecraftDecoder extends ChannelInboundHandlerAdapter { } if (buf.isReadable()) { - throw handleNotReadEnough(packet, packetId); + throw handleOverflow(packet, buf.readerIndex(), buf.writerIndex()); } ctx.fireChannelRead(packet); } finally { @@ -74,10 +76,30 @@ public class MinecraftDecoder extends ChannelInboundHandlerAdapter { } } - private Exception handleNotReadEnough(MinecraftPacket packet, int packetId) { + private void doLengthSanityChecks(ByteBuf buf, MinecraftPacket packet) throws Exception { + int expectedMinLen = packet.expectedMinLength(buf, direction, registry.version); + int expectedMaxLen = packet.expectedMaxLength(buf, direction, registry.version); + if (expectedMaxLen != -1 && buf.readableBytes() > expectedMaxLen) { + throw handleOverflow(packet, expectedMaxLen, buf.readableBytes()); + } + if (buf.readableBytes() < expectedMinLen) { + throw handleUnderflow(packet, expectedMaxLen, buf.readableBytes()); + } + } + + private Exception handleOverflow(MinecraftPacket packet, int expected, int actual) { if (DEBUG) { - return new CorruptedFrameException("Did not read full packet for " + packet.getClass() + " " - + getExtraConnectionDetail(packetId)); + return new CorruptedFrameException("Packet sent for " + packet.getClass() + " was too " + + "big (expected " + expected + " bytes, got " + actual + " bytes)"); + } else { + return DECODE_FAILED; + } + } + + private Exception handleUnderflow(MinecraftPacket packet, int expected, int actual) { + if (DEBUG) { + return new CorruptedFrameException("Packet sent for " + packet.getClass() + " was too " + + "small (expected " + expected + " bytes, got " + actual + " bytes)"); } else { return DECODE_FAILED; } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/EncryptionResponse.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/EncryptionResponse.java index 5022f5c18..b59b46c61 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/EncryptionResponse.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/EncryptionResponse.java @@ -6,6 +6,7 @@ import com.velocitypowered.api.network.ProtocolVersion; import com.velocitypowered.proxy.connection.MinecraftSessionHandler; import com.velocitypowered.proxy.protocol.MinecraftPacket; import com.velocitypowered.proxy.protocol.ProtocolUtils; +import com.velocitypowered.proxy.protocol.ProtocolUtils.Direction; import io.netty.buffer.ByteBuf; import java.util.Arrays; @@ -33,7 +34,7 @@ public class EncryptionResponse implements MinecraftPacket { @Override public void decode(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) { if (version.compareTo(ProtocolVersion.MINECRAFT_1_8) >= 0) { - this.sharedSecret = ProtocolUtils.readByteArray(buf, 256); + this.sharedSecret = ProtocolUtils.readByteArray(buf, 128); this.verifyToken = ProtocolUtils.readByteArray(buf, 128); } else { this.sharedSecret = ProtocolUtils.readByteArray17(buf); @@ -56,4 +57,16 @@ public class EncryptionResponse implements MinecraftPacket { public boolean handle(MinecraftSessionHandler handler) { return handler.handle(this); } + + @Override + public int expectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) { + // It turns out these come out to the same length, whether we're talking >=1.8 or not. + // The length prefix always winds up being 2 bytes. + return 260; + } + + @Override + public int expectedMinLength(ByteBuf buf, Direction direction, ProtocolVersion version) { + return expectedMaxLength(buf, direction, version); + } } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerLogin.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerLogin.java index ebd7489e9..cef02eb0c 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerLogin.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerLogin.java @@ -5,6 +5,7 @@ import com.velocitypowered.api.network.ProtocolVersion; import com.velocitypowered.proxy.connection.MinecraftSessionHandler; import com.velocitypowered.proxy.protocol.MinecraftPacket; import com.velocitypowered.proxy.protocol.ProtocolUtils; +import com.velocitypowered.proxy.protocol.ProtocolUtils.Direction; import com.velocitypowered.proxy.util.except.QuietDecoderException; import io.netty.buffer.ByteBuf; import org.checkerframework.checker.nullness.qual.Nullable; @@ -52,6 +53,13 @@ public class ServerLogin implements MinecraftPacket { ProtocolUtils.writeString(buf, username); } + @Override + public int expectedMaxLength(ByteBuf buf, Direction direction, 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); + } + @Override public boolean handle(MinecraftSessionHandler handler) { return handler.handle(this); diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/StatusPing.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/StatusPing.java index 7d61e6996..16a6fcd73 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/StatusPing.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/StatusPing.java @@ -4,6 +4,7 @@ import com.velocitypowered.api.network.ProtocolVersion; import com.velocitypowered.proxy.connection.MinecraftSessionHandler; import com.velocitypowered.proxy.protocol.MinecraftPacket; import com.velocitypowered.proxy.protocol.ProtocolUtils; +import com.velocitypowered.proxy.protocol.ProtocolUtils.Direction; import io.netty.buffer.ByteBuf; public class StatusPing implements MinecraftPacket { @@ -24,4 +25,14 @@ public class StatusPing implements MinecraftPacket { public boolean handle(MinecraftSessionHandler handler) { return handler.handle(this); } + + @Override + public int expectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) { + return 8; + } + + @Override + public int expectedMinLength(ByteBuf buf, Direction direction, ProtocolVersion version) { + return 8; + } } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/StatusRequest.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/StatusRequest.java index 7d1b9bc57..e0aaef5e2 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/StatusRequest.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/StatusRequest.java @@ -4,6 +4,7 @@ import com.velocitypowered.api.network.ProtocolVersion; import com.velocitypowered.proxy.connection.MinecraftSessionHandler; import com.velocitypowered.proxy.protocol.MinecraftPacket; import com.velocitypowered.proxy.protocol.ProtocolUtils; +import com.velocitypowered.proxy.protocol.ProtocolUtils.Direction; import io.netty.buffer.ByteBuf; public class StatusRequest implements MinecraftPacket { @@ -33,4 +34,9 @@ public class StatusRequest implements MinecraftPacket { public boolean handle(MinecraftSessionHandler handler) { return handler.handle(this); } + + @Override + public int expectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) { + return 0; + } }