Cap pre-join plugin-message queue size (prevents arbitrary growth/OOM) (#1800)

* Cap pre-join plugin-message queue size (prevents arbitrary growth/OOM)

* Clear counters once as the entire queue will have been processed

Author: WouterGritter

proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java

@@ -86,6 +86,8 @@
 import java.util.UUID;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicLong;
 import net.kyori.adventure.key.Key;
 import net.kyori.adventure.text.Component;
 import net.kyori.adventure.text.ComponentLike;
@@ -102,12 +104,23 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler {
   private static final boolean BACKPRESSURE_LOG =
       Boolean.getBoolean("velocity.log-server-backpressure");
 
+  // Caps the per-connection queue used while the FML/login phases are not yet "complete". Without
+  // these caps, a client that never completes its handshake phase can spam plugin messages (each up
+  // to ~32 KiB serverbound) and grow the queue without bound.
+  private static final long MAX_QUEUED_LOGIN_PLUGIN_MESSAGE_BYTES =
+      Long.getLong("velocity.max-queued-login-plugin-message-bytes", 4L * 1024 * 1024);
+  private static final int MAX_QUEUED_LOGIN_PLUGIN_MESSAGES =
+      Integer.getInteger("velocity.max-queued-login-plugin-messages", 1024);
+
   private static final Logger logger = LogManager.getLogger(ClientPlaySessionHandler.class);
 
   private final ConnectedPlayer player;
   private boolean spawned = false;
   private final List<UUID> serverBossBars = new ArrayList<>();
   private final Queue<PluginMessagePacket> loginPluginMessages = new ConcurrentLinkedQueue<>();
+  private final AtomicLong loginPluginMessagesBytes = new AtomicLong();
+  private final AtomicInteger loginPluginMessagesCount = new AtomicInteger();
+  private volatile boolean loginPluginMessagesOverflowed;
   private final VelocityServer server;
   private @Nullable TabCompleteRequestPacket outstandingTabComplete;
   private final ChatHandler<? extends MinecraftPacket> chatHandler;
@@ -176,9 +189,38 @@ public void activated() {
   @Override
   public void deactivated() {
     player.discardChatQueue();
-    for (PluginMessagePacket message : loginPluginMessages) {
+    PluginMessagePacket message;
+    while ((message = loginPluginMessages.poll()) != null) {
       ReferenceCountUtil.release(message);
     }
+    loginPluginMessagesBytes.set(0);
+    loginPluginMessagesCount.set(0);
+  }
+
+  /**
+   * Adds a retained plugin message to the queue used while the FML/login phases are still in
+   * progress, enforcing the per-connection byte and count caps. Returns {@code true} if queued,
+   * {@code false} if the packet was released (and the player disconnected on overflow).
+   */
+  private boolean enqueueLoginPluginMessage(PluginMessagePacket packet) {
+    if (loginPluginMessagesOverflowed) {
+      ReferenceCountUtil.release(packet);
+      return false;
+    }
+    int packetSize = packet.content().readableBytes();
+    long newBytes = loginPluginMessagesBytes.addAndGet(packetSize);
+    int newCount = loginPluginMessagesCount.incrementAndGet();
+    if (newBytes > MAX_QUEUED_LOGIN_PLUGIN_MESSAGE_BYTES
+        || newCount > MAX_QUEUED_LOGIN_PLUGIN_MESSAGES) {
+      loginPluginMessagesOverflowed = true;
+      ReferenceCountUtil.release(packet);
+      logger.warn("Disconnecting {}: pre-join plugin-message queue exceeded its limits "
+              + "({} messages, {} bytes).", player, newCount, newBytes);
+      player.disconnect(Component.translatable("velocity.error.plugin-message-overflow"));
+      return false;
+    }
+    loginPluginMessages.add(packet);
+    return true;
   }
 
   @Override
@@ -359,7 +401,7 @@ public boolean handle(PluginMessagePacket packet) {
               //
               // We also need to make sure to retain these packets, so they can be flushed
               // appropriately.
-              loginPluginMessages.add(packet.retain());
+              enqueueLoginPluginMessage(packet.retain());
             } else {
               // The connection is ready, send the packet now.
               backendConn.write(packet.retain());
@@ -374,7 +416,7 @@ public boolean handle(PluginMessagePacket packet) {
                 if (!player.getPhase().consideredComplete() || !serverConn.getPhase()
                     .consideredComplete()) {
                   // We're still processing the connection (see above), enqueue the packet for now.
-                  loginPluginMessages.add(message.retain());
+                  enqueueLoginPluginMessage(message.retain());
                 } else {
                   backendConn.write(message);
                 }
@@ -637,6 +679,8 @@ public void handleBackendJoinGame(JoinGamePacket joinGame, VelocityServerConnect
     while ((pm = loginPluginMessages.poll()) != null) {
       serverMc.delayedWrite(pm);
     }
+    loginPluginMessagesBytes.set(0);
+    loginPluginMessagesCount.set(0);
 
     // Clear any title from the previous server.
     if (player.getProtocolVersion().noLessThan(ProtocolVersion.MINECRAFT_1_8)) {
@@ -869,6 +913,8 @@ public void flushQueuedMessages() {
         while ((pm = loginPluginMessages.poll()) != null) {
           connection.write(pm);
         }
+        loginPluginMessagesBytes.set(0);
+        loginPluginMessagesCount.set(0);
       }
     }
   }

proxy/src/main/resources/com/velocitypowered/proxy/l10n/messages.properties

@@ -25,6 +25,7 @@ velocity.error.internal-server-connection-error=An internal server connection er
 velocity.error.logging-in-too-fast=You are logging in too fast, try again later.
 velocity.error.online-mode-only=You are not logged into your Minecraft account. If you are logged into your Minecraft account, try restarting your Minecraft client.
 velocity.error.player-connection-error=An internal error occurred in your connection.
+velocity.error.plugin-message-overflow=You sent too many plugin messages before completing the connection.
 velocity.error.modern-forwarding-needs-new-client=This server is only compatible with Minecraft 1.13 and above.
 velocity.error.modern-forwarding-failed=Your server did not send a forwarding request to the proxy. Make sure the server is configured for Velocity forwarding.
 velocity.error.moved-to-new-server=You were kicked from <arg:0>: <arg:1>