From 00016ba4e1fa4098862aeebc8a66cc1979d7d7ff Mon Sep 17 00:00:00 2001 From: Shane Freeder Date: Wed, 7 May 2025 23:49:33 +0100 Subject: [PATCH] Validate handshake packet length early --- .../proxy/connection/MinecraftConnection.java | 6 ++ .../backend/BackendPlaySessionHandler.java | 2 + .../backend/ConfigSessionHandler.java | 2 + .../network/BackendChannelInitializer.java | 2 +- .../network/ServerChannelInitializer.java | 2 +- .../netty/MinecraftVarintFrameDecoder.java | 81 +++++++++++++++++++ .../protocol/packet/HandshakePacket.java | 12 +++ .../server/VelocityRegisteredServer.java | 2 +- 8 files changed, 106 insertions(+), 3 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/MinecraftConnection.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/MinecraftConnection.java index 3f34a54c..3d7ed448 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/MinecraftConnection.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/MinecraftConnection.java @@ -46,6 +46,7 @@ import com.velocitypowered.proxy.protocol.netty.MinecraftCompressDecoder; import com.velocitypowered.proxy.protocol.netty.MinecraftCompressorAndLengthEncoder; import com.velocitypowered.proxy.protocol.netty.MinecraftDecoder; import com.velocitypowered.proxy.protocol.netty.MinecraftEncoder; +import com.velocitypowered.proxy.protocol.netty.MinecraftVarintFrameDecoder; import com.velocitypowered.proxy.protocol.netty.MinecraftVarintLengthEncoder; import com.velocitypowered.proxy.protocol.netty.PlayPacketQueueInboundHandler; import com.velocitypowered.proxy.protocol.netty.PlayPacketQueueOutboundHandler; @@ -368,6 +369,11 @@ public class MinecraftConnection extends ChannelInboundHandlerAdapter { ensureInEventLoop(); this.state = state; + final MinecraftVarintFrameDecoder frameDecoder = this.channel.pipeline() + .get(MinecraftVarintFrameDecoder.class); + if (frameDecoder != null) { + frameDecoder.setState(state); + } // If the connection is LEGACY (<1.6), the decoder and encoder are not set. final MinecraftEncoder minecraftEncoder = this.channel.pipeline() .get(MinecraftEncoder.class); diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java index 128d5c37..7c89f659 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java @@ -47,6 +47,7 @@ import com.velocitypowered.proxy.connection.util.ConnectionMessages; import com.velocitypowered.proxy.protocol.MinecraftPacket; import com.velocitypowered.proxy.protocol.StateRegistry; import com.velocitypowered.proxy.protocol.netty.MinecraftDecoder; +import com.velocitypowered.proxy.protocol.netty.MinecraftVarintFrameDecoder; import com.velocitypowered.proxy.protocol.packet.AvailableCommandsPacket; import com.velocitypowered.proxy.protocol.packet.BossBarPacket; import com.velocitypowered.proxy.protocol.packet.BundleDelimiterPacket; @@ -149,6 +150,7 @@ public class BackendPlaySessionHandler implements MinecraftSessionHandler { MinecraftConnection smc = serverConn.ensureConnected(); smc.setAutoReading(false); // Even when not auto reading messages are still decoded. Decode them with the correct state + smc.getChannel().pipeline().get(MinecraftVarintFrameDecoder.class).setState(StateRegistry.CONFIG); smc.getChannel().pipeline().get(MinecraftDecoder.class).setState(StateRegistry.CONFIG); serverConn.getPlayer().switchToConfigState(); return true; diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/ConfigSessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/ConfigSessionHandler.java index 1860093d..fb0e48d1 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/ConfigSessionHandler.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/ConfigSessionHandler.java @@ -40,6 +40,7 @@ import com.velocitypowered.proxy.connection.util.ConnectionRequestResults.Impl; import com.velocitypowered.proxy.protocol.MinecraftPacket; import com.velocitypowered.proxy.protocol.StateRegistry; import com.velocitypowered.proxy.protocol.netty.MinecraftDecoder; +import com.velocitypowered.proxy.protocol.netty.MinecraftVarintFrameDecoder; import com.velocitypowered.proxy.protocol.packet.ClientboundCookieRequestPacket; import com.velocitypowered.proxy.protocol.packet.ClientboundStoreCookiePacket; import com.velocitypowered.proxy.protocol.packet.DisconnectPacket; @@ -232,6 +233,7 @@ public class ConfigSessionHandler implements MinecraftSessionHandler { final ConnectedPlayer player = serverConn.getPlayer(); final ClientConfigSessionHandler configHandler = (ClientConfigSessionHandler) player.getConnection().getActiveSessionHandler(); + smc.getChannel().pipeline().get(MinecraftVarintFrameDecoder.class).setState(StateRegistry.PLAY); smc.getChannel().pipeline().get(MinecraftDecoder.class).setState(StateRegistry.PLAY); //noinspection DataFlowIssue configHandler.handleBackendFinishUpdate(serverConn).thenRunAsync(() -> { diff --git a/proxy/src/main/java/com/velocitypowered/proxy/network/BackendChannelInitializer.java b/proxy/src/main/java/com/velocitypowered/proxy/network/BackendChannelInitializer.java index d60c6477..236394fb 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/network/BackendChannelInitializer.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/network/BackendChannelInitializer.java @@ -51,7 +51,7 @@ public class BackendChannelInitializer extends ChannelInitializer { @Override protected void initChannel(Channel ch) { ch.pipeline() - .addLast(FRAME_DECODER, new MinecraftVarintFrameDecoder()) + .addLast(FRAME_DECODER, new MinecraftVarintFrameDecoder(ProtocolUtils.Direction.CLIENTBOUND)) .addLast(READ_TIMEOUT, new ReadTimeoutHandler(server.getConfiguration().getReadTimeout(), TimeUnit.MILLISECONDS)) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/network/ServerChannelInitializer.java b/proxy/src/main/java/com/velocitypowered/proxy/network/ServerChannelInitializer.java index ef8e6b1c..0c22dcce 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/network/ServerChannelInitializer.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/network/ServerChannelInitializer.java @@ -58,7 +58,7 @@ public class ServerChannelInitializer extends ChannelInitializer { protected void initChannel(final Channel ch) { ch.pipeline() .addLast(LEGACY_PING_DECODER, new LegacyPingDecoder()) - .addLast(FRAME_DECODER, new MinecraftVarintFrameDecoder()) + .addLast(FRAME_DECODER, new MinecraftVarintFrameDecoder(ProtocolUtils.Direction.SERVERBOUND)) .addLast(READ_TIMEOUT, new ReadTimeoutHandler(this.server.getConfiguration().getReadTimeout(), TimeUnit.MILLISECONDS)) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftVarintFrameDecoder.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftVarintFrameDecoder.java index 84f35381..8215a787 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftVarintFrameDecoder.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftVarintFrameDecoder.java @@ -19,10 +19,16 @@ package com.velocitypowered.proxy.protocol.netty; import static io.netty.util.ByteProcessor.FIND_NON_NUL; +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 com.velocitypowered.proxy.util.except.QuietRuntimeException; import io.netty.buffer.ByteBuf; import io.netty.channel.ChannelHandlerContext; import io.netty.handler.codec.ByteToMessageDecoder; +import io.netty.handler.codec.CorruptedFrameException; import java.util.List; /** @@ -30,10 +36,31 @@ import java.util.List; */ public class MinecraftVarintFrameDecoder extends ByteToMessageDecoder { + private static final QuietRuntimeException FRAME_DECODER_FAILED = + new QuietRuntimeException("A packet frame decoder failed. For more information, launch " + + "Velocity with -Dvelocity.packet-decode-logging=true to see more."); private static final QuietDecoderException BAD_PACKET_LENGTH = new QuietDecoderException("Bad packet length"); private static final QuietDecoderException VARINT_TOO_BIG = new QuietDecoderException("VarInt too big"); + private static final QuietDecoderException UNKNOWN_PACKET = + new QuietDecoderException("Unknown packet"); + + private final ProtocolUtils.Direction direction; + private final StateRegistry.PacketRegistry.ProtocolRegistry registry; + private StateRegistry state; + + /** + * Creates a new {@code MinecraftVarintFrameDecoder} decoding packets from the specified {@code Direction}. + * + * @param direction the direction from which we decode from + */ + public MinecraftVarintFrameDecoder(ProtocolUtils.Direction direction) { + this.direction = direction; + this.registry = StateRegistry.HANDSHAKE.getProtocolRegistry( + direction, ProtocolVersion.MINIMUM_VERSION); + this.state = StateRegistry.HANDSHAKE; + } @Override protected void decode(ChannelHandlerContext ctx, ByteBuf in, List out) @@ -62,6 +89,38 @@ public class MinecraftVarintFrameDecoder extends ByteToMessageDecoder { throw BAD_PACKET_LENGTH; } + if (length > 0) { + if (state == StateRegistry.HANDSHAKE && direction == ProtocolUtils.Direction.SERVERBOUND) { + StateRegistry.PacketRegistry.ProtocolRegistry registry = + state.getProtocolRegistry(direction, ProtocolVersion.MINIMUM_VERSION); + + final int index = in.readerIndex(); + final int packetId = ProtocolUtils.readVarInt(in); + final int payloadLength = length - ProtocolUtils.varIntBytes(packetId); + + MinecraftPacket packet = registry.createPacket(packetId); + + // We handle every packet in this phase, if you said something we don't know, something is really wrong + if (packet == null) { + throw UNKNOWN_PACKET; + } + + // We 'technically' have the incoming bytes of a payload here, and so, these can actually parse + // the packet if needed, so, we'll take advantage of the existing methods + int expectedMinLen = packet.expectedMinLength(in, direction, registry.version); + int expectedMaxLen = packet.expectedMaxLength(in, direction, registry.version); + if (expectedMaxLen != -1 && payloadLength > expectedMaxLen) { + throw handleOverflow(packet, expectedMaxLen, in.readableBytes()); + } + if (payloadLength < expectedMinLen) { + throw handleUnderflow(packet, expectedMaxLen, in.readableBytes()); + } + + + in.readerIndex(index); + } + } + // note that zero-length packets are ignored if (length > 0) { if (in.readableBytes() < length) { @@ -141,4 +200,26 @@ public class MinecraftVarintFrameDecoder extends ByteToMessageDecoder { } return result | (tmp & 0x7F) << 14; } + + private Exception handleOverflow(MinecraftPacket packet, int expected, int actual) { + if (MinecraftDecoder.DEBUG) { + return new CorruptedFrameException("Packet sent for " + packet.getClass() + " was too " + + "big (expected " + expected + " bytes, got " + actual + " bytes)"); + } else { + return FRAME_DECODER_FAILED; + } + } + + private Exception handleUnderflow(MinecraftPacket packet, int expected, int actual) { + if (MinecraftDecoder.DEBUG) { + return new CorruptedFrameException("Packet sent for " + packet.getClass() + " was too " + + "small (expected " + expected + " bytes, got " + actual + " bytes)"); + } else { + return FRAME_DECODER_FAILED; + } + } + + public void setState(StateRegistry stateRegistry) { + this.state = stateRegistry; + } } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/HandshakePacket.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/HandshakePacket.java index 2edaed9e..493dfa5d 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/HandshakePacket.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/HandshakePacket.java @@ -106,4 +106,16 @@ public class HandshakePacket implements MinecraftPacket { public boolean handle(MinecraftSessionHandler handler) { return handler.handle(this); } + + @Override + public int expectedMinLength(ByteBuf buf, ProtocolUtils.Direction direction, + ProtocolVersion version) { + return 7; + } + + @Override + public int expectedMaxLength(ByteBuf buf, ProtocolUtils.Direction direction, + ProtocolVersion version) { + return 270; + } } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/server/VelocityRegisteredServer.java b/proxy/src/main/java/com/velocitypowered/proxy/server/VelocityRegisteredServer.java index 697135e3..e48881f3 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/server/VelocityRegisteredServer.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/server/VelocityRegisteredServer.java @@ -114,7 +114,7 @@ public class VelocityRegisteredServer implements RegisteredServer, ForwardingAud server.createBootstrap(loop).handler(new ChannelInitializer<>() { @Override protected void initChannel(Channel ch) { - ch.pipeline().addLast(FRAME_DECODER, new MinecraftVarintFrameDecoder()) + ch.pipeline().addLast(FRAME_DECODER, new MinecraftVarintFrameDecoder(ProtocolUtils.Direction.CLIENTBOUND)) .addLast(READ_TIMEOUT, new ReadTimeoutHandler( pingOptions.getTimeout() == 0 ? server.getConfiguration().getReadTimeout()