From 5017f8c9f2d5aa4ed2edda4b2bd4786280bbe009 Mon Sep 17 00:00:00 2001 From: booky Date: Wed, 18 Mar 2026 13:04:31 +0100 Subject: [PATCH] Add more protocol safeguards (#1743) * Add max length to more config phase packets * Ensure all packets during non-play state are known * Limit maximum size of play inbound packet queue Co-authored-by: Dwarslooper * Fix checkstyle --------- Co-authored-by: Dwarslooper --- .../proxy/protocol/ProtocolUtils.java | 5 +++- .../protocol/netty/MinecraftDecoder.java | 12 ++++++++++ .../netty/PlayPacketQueueInboundHandler.java | 23 +++++++++++++++++++ .../protocol/packet/ClientSettingsPacket.java | 16 ++++++++++--- .../protocol/packet/KeepAlivePacket.java | 22 ++++++++++++++++++ .../protocol/packet/PingIdentifyPacket.java | 10 ++++++++ .../protocol/packet/PluginMessagePacket.java | 12 ++++++++++ .../packet/ResourcePackResponsePacket.java | 22 +++++++++++++++++- .../ServerboundCookieResponsePacket.java | 10 ++++++++ .../ServerboundCustomClickActionPacket.java | 12 ++++++++++ .../ClientboundCustomReportDetailsPacket.java | 1 - .../config/CodeOfConductAcceptPacket.java | 5 ++++ 12 files changed, 144 insertions(+), 6 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 21d3fb981..826df5b3c 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -161,6 +161,9 @@ public enum ProtocolUtils { VAR_INT_LENGTHS[32] = 1; // Special case for the number 0. } + public static final int DEFAULT_MAX_STRING_BYTES = varIntBytes(ByteBufUtil.utf8MaxBytes(DEFAULT_MAX_STRING_SIZE)) + + ByteBufUtil.utf8MaxBytes(DEFAULT_MAX_STRING_SIZE); + private static DecoderException badVarint() { return MinecraftDecoder.DEBUG ? new CorruptedFrameException("Bad VarInt decoded") : BAD_VARINT_CACHED; @@ -886,4 +889,4 @@ public enum ProtocolUtils { SERVERBOUND, CLIENTBOUND } -} \ No newline at end of file +} 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 35ddccd5e..ae84e7495 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 @@ -74,6 +74,10 @@ public class MinecraftDecoder extends ChannelInboundHandlerAdapter { MinecraftPacket packet = this.registry.createPacket(packetId); if (packet == null) { buf.readerIndex(originalReaderIndex); + if (this.direction == ProtocolUtils.Direction.SERVERBOUND && this.state != StateRegistry.PLAY) { + buf.release(); + throw this.handleInvalidPacketId(packetId); + } ctx.fireChannelRead(buf); } else { try { @@ -133,6 +137,14 @@ public class MinecraftDecoder extends ChannelInboundHandlerAdapter { } } + private Exception handleInvalidPacketId(int packetId) { + if (DEBUG) { + return new CorruptedFrameException("Invalid packet " + getExtraConnectionDetail(packetId)); + } else { + return DECODE_FAILED; + } + } + private String getExtraConnectionDetail(int packetId) { return "Direction " + direction + " Protocol " + registry.version + " State " + state + " ID 0x" + Integer.toHexString(packetId); diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/PlayPacketQueueInboundHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/PlayPacketQueueInboundHandler.java index 1affc13bc..899768d27 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/PlayPacketQueueInboundHandler.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/PlayPacketQueueInboundHandler.java @@ -21,6 +21,9 @@ 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 io.netty.buffer.ByteBuf; +import io.netty.buffer.ByteBufHolder; import io.netty.channel.ChannelDuplexHandler; import io.netty.channel.ChannelHandlerContext; import io.netty.util.ReferenceCountUtil; @@ -41,8 +44,13 @@ import org.jetbrains.annotations.NotNull; */ public class PlayPacketQueueInboundHandler extends ChannelDuplexHandler { + private static final int MAXIMUM_SIZE = Integer.getInteger("velocity.maximum-play-queue-size", 128 * 1024 * 1024); // 128MiB by default + private static final QuietDecoderException QUEUE_LIMIT_FAILED = new QuietDecoderException( + "Queue too big (greater than " + MAXIMUM_SIZE + " bytes)"); + private final StateRegistry.PacketRegistry.ProtocolRegistry registry; private final Queue queue = new ArrayDeque<>(); + private int queueSize = 0; /** * Provides registries for client & server bound packets. @@ -64,6 +72,20 @@ public class PlayPacketQueueInboundHandler extends ChannelDuplexHandler { } } + int length = 0; + if (msg instanceof ByteBuf) { + // keep track of raw packets + length = ((ByteBuf) msg).readableBytes(); + } else if (msg instanceof ByteBufHolder) { + // keep track of bytebufs wrapped inside packets + length = ((ByteBufHolder) msg).content().readableBytes(); + } + if (this.queueSize + length > MAXIMUM_SIZE) { + ReferenceCountUtil.release(msg); + throw QUEUE_LIMIT_FAILED; + } + this.queueSize += length; + // Otherwise, queue the packet this.queue.offer(msg); } @@ -90,5 +112,6 @@ public class PlayPacketQueueInboundHandler extends ChannelDuplexHandler { ReferenceCountUtil.release(msg); } } + this.queueSize = 0; } } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ClientSettingsPacket.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ClientSettingsPacket.java index 39e6fde02..b8b60a9f9 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ClientSettingsPacket.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ClientSettingsPacket.java @@ -22,8 +22,8 @@ import com.velocitypowered.proxy.connection.MinecraftSessionHandler; import com.velocitypowered.proxy.protocol.MinecraftPacket; import com.velocitypowered.proxy.protocol.ProtocolUtils; import io.netty.buffer.ByteBuf; +import io.netty.buffer.ByteBufUtil; import java.util.Objects; - import org.checkerframework.checker.nullness.qual.Nullable; public class ClientSettingsPacket implements MinecraftPacket { @@ -135,7 +135,7 @@ public class ClientSettingsPacket implements MinecraftPacket { return "ClientSettings{" + "locale='" + locale + '\'' + ", viewDistance=" + viewDistance + ", chatVisibility=" + chatVisibility + ", chatColors=" + chatColors + ", skinParts=" + skinParts + ", mainHand=" + mainHand + ", chatFilteringEnabled=" + textFilteringEnabled + - ", clientListingAllowed=" + clientListingAllowed + ", particleStatus=" + particleStatus + '}'; + ", clientListingAllowed=" + clientListingAllowed + ", particleStatus=" + particleStatus + '}'; } @Override @@ -206,6 +206,16 @@ public class ClientSettingsPacket implements MinecraftPacket { return handler.handle(this); } + @Override + public int decodeExpectedMaxLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) { + return 1 + ByteBufUtil.utf8MaxBytes(16) + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1; + } + + @Override + public int decodeExpectedMinLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) { + return 1 + 0 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1; + } + @Override public boolean equals(@Nullable final Object o) { if (this == o) { @@ -237,7 +247,7 @@ public class ClientSettingsPacket implements MinecraftPacket { difficulty, skinParts, mainHand, - textFilteringEnabled, + textFilteringEnabled, clientListingAllowed, particleStatus); } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/KeepAlivePacket.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/KeepAlivePacket.java index a44e50eea..932dd47a2 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/KeepAlivePacket.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/KeepAlivePacket.java @@ -64,6 +64,28 @@ public class KeepAlivePacket implements MinecraftPacket { } } + @Override + public int decodeExpectedMaxLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) { + if (version.noLessThan(ProtocolVersion.MINECRAFT_1_12_2)) { + return Long.BYTES; + } else if (version.noLessThan(ProtocolVersion.MINECRAFT_1_8)) { + return 5; + } else { + return Integer.BYTES; + } + } + + @Override + public int decodeExpectedMinLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) { + if (version.noLessThan(ProtocolVersion.MINECRAFT_1_12_2)) { + return Long.BYTES; + } else if (version.noLessThan(ProtocolVersion.MINECRAFT_1_8)) { + return 1; + } else { + return Integer.BYTES; + } + } + @Override public boolean handle(MinecraftSessionHandler handler) { return handler.handle(this); diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PingIdentifyPacket.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PingIdentifyPacket.java index 27c1351d5..ed1e3feb1 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PingIdentifyPacket.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PingIdentifyPacket.java @@ -42,6 +42,16 @@ public class PingIdentifyPacket implements MinecraftPacket { buf.writeInt(id); } + @Override + public int decodeExpectedMaxLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) { + return Integer.BYTES; + } + + @Override + public int decodeExpectedMinLength(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) { + return Integer.BYTES; + } + @Override public boolean handle(MinecraftSessionHandler handler) { return handler.handle(this); diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PluginMessagePacket.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PluginMessagePacket.java index ecf2887fd..e2da28de1 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PluginMessagePacket.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PluginMessagePacket.java @@ -31,6 +31,8 @@ import org.checkerframework.checker.nullness.qual.Nullable; public class PluginMessagePacket extends DeferredByteBufHolder implements MinecraftPacket { + private static final int MAX_PAYLOAD_SIZE = Integer.getInteger("velocity.max-plugin-message-payload-size", 32767); + private @Nullable String channel; public PluginMessagePacket() { @@ -100,6 +102,16 @@ public class PluginMessagePacket extends DeferredByteBufHolder implements Minecr } + @Override + public int decodeExpectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) { + return ProtocolUtils.DEFAULT_MAX_STRING_BYTES + MAX_PAYLOAD_SIZE; + } + + @Override + public int decodeExpectedMinLength(ByteBuf buf, Direction direction, ProtocolVersion version) { + return 1 + 0 + 0; + } + @Override public boolean handle(MinecraftSessionHandler handler) { return handler.handle(this); diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ResourcePackResponsePacket.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ResourcePackResponsePacket.java index 020c3530d..4d9a83d28 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ResourcePackResponsePacket.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ResourcePackResponsePacket.java @@ -80,6 +80,26 @@ public class ResourcePackResponsePacket implements MinecraftPacket { ProtocolUtils.writeVarInt(buf, status.ordinal()); } + @Override + public int decodeExpectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) { + if (version.noLessThan(ProtocolVersion.MINECRAFT_1_20_3)) { + return Long.BYTES * 2 + 1; + } else if (version.noGreaterThan(ProtocolVersion.MINECRAFT_1_9_4)) { + return ProtocolUtils.DEFAULT_MAX_STRING_BYTES + 1; + } + return 1; + } + + @Override + public int decodeExpectedMinLength(ByteBuf buf, Direction direction, ProtocolVersion version) { + if (version.noLessThan(ProtocolVersion.MINECRAFT_1_20_3)) { + return Long.BYTES * 2 + 1; + } else if (version.noGreaterThan(ProtocolVersion.MINECRAFT_1_9_4)) { + return 1 + 0 + 1; + } + return 1; + } + @Override public boolean handle(MinecraftSessionHandler handler) { return handler.handle(this); @@ -93,4 +113,4 @@ public class ResourcePackResponsePacket implements MinecraftPacket { ", status=" + status + '}'; } -} \ No newline at end of file +} diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerboundCookieResponsePacket.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerboundCookieResponsePacket.java index bee12b802..0731cc2d9 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerboundCookieResponsePacket.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerboundCookieResponsePacket.java @@ -65,6 +65,16 @@ public class ServerboundCookieResponsePacket implements MinecraftPacket { } } + @Override + public int decodeExpectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) { + return ProtocolUtils.DEFAULT_MAX_STRING_BYTES + 1 + 2 + 5120; + } + + @Override + public int decodeExpectedMinLength(ByteBuf buf, Direction direction, ProtocolVersion version) { + return 1 + 0 + 0; + } + @Override public boolean handle(MinecraftSessionHandler handler) { return handler.handle(this); diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerboundCustomClickActionPacket.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerboundCustomClickActionPacket.java index 6b846c234..7b2c92569 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerboundCustomClickActionPacket.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/ServerboundCustomClickActionPacket.java @@ -27,6 +27,8 @@ import io.netty.buffer.ByteBuf; public class ServerboundCustomClickActionPacket extends DeferredByteBufHolder implements MinecraftPacket { + private static final int MAX_TAG_SIZE = 65536; + public ServerboundCustomClickActionPacket() { super(null); } @@ -41,6 +43,16 @@ public class ServerboundCustomClickActionPacket extends DeferredByteBufHolder im buf.writeBytes(content()); } + @Override + public int decodeExpectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) { + return ProtocolUtils.DEFAULT_MAX_STRING_BYTES + ProtocolUtils.varIntBytes(MAX_TAG_SIZE) + MAX_TAG_SIZE; + } + + @Override + public int decodeExpectedMinLength(ByteBuf buf, Direction direction, ProtocolVersion version) { + return 1 + 0 + 1 + 0; + } + @Override public boolean handle(MinecraftSessionHandler handler) { return handler.handle(this); diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/config/ClientboundCustomReportDetailsPacket.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/config/ClientboundCustomReportDetailsPacket.java index c51c5ff20..05f535d42 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/config/ClientboundCustomReportDetailsPacket.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/config/ClientboundCustomReportDetailsPacket.java @@ -22,7 +22,6 @@ import com.velocitypowered.proxy.connection.MinecraftSessionHandler; import com.velocitypowered.proxy.protocol.MinecraftPacket; import com.velocitypowered.proxy.protocol.ProtocolUtils; import io.netty.buffer.ByteBuf; -import java.util.HashMap; import java.util.Map; public class ClientboundCustomReportDetailsPacket implements MinecraftPacket { diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/config/CodeOfConductAcceptPacket.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/config/CodeOfConductAcceptPacket.java index e9811f9fc..6cd338c41 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/config/CodeOfConductAcceptPacket.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/config/CodeOfConductAcceptPacket.java @@ -38,6 +38,11 @@ public class CodeOfConductAcceptPacket implements MinecraftPacket { public void encode(ByteBuf buf, Direction direction, ProtocolVersion protocolVersion) { } + @Override + public int decodeExpectedMaxLength(ByteBuf buf, Direction direction, ProtocolVersion version) { + return 0; + } + @Override public boolean handle(MinecraftSessionHandler handler) { return handler.handle(this);