From 0fa61216e771cb5ff84c7a165d374476ac1f5c8a Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Tue, 19 Jan 2021 02:40:32 -0500 Subject: [PATCH 01/13] Bump to Adventure 4.4.0 --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 9becd93df..ebb19f20a 100644 --- a/build.gradle +++ b/build.gradle @@ -21,7 +21,7 @@ allprojects { ext { // dependency versions textVersion = '3.0.4' - adventureVersion = '4.3.0' + adventureVersion = '4.4.0' adventurePlatformVersion = '4.0.0-SNAPSHOT' junitVersion = '5.7.0' slf4jVersion = '1.7.30' From 892ac6f6263f00c85d0e358d6efaa4ee646f6c3f Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Thu, 21 Jan 2021 17:57:40 -0500 Subject: [PATCH 02/13] Accept changes to compression treshold on the fly. Vanilla allows this for some reason, and there has been one case where Velocity's strict behavior has caused a bug. Fix this. --- .../proxy/connection/MinecraftConnection.java | 27 ++++++++++++------- .../netty/MinecraftCompressDecoder.java | 6 ++++- .../netty/MinecraftCompressEncoder.java | 6 ++++- 3 files changed, 28 insertions(+), 11 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 3571f7a0f..42b3a766f 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/MinecraftConnection.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/MinecraftConnection.java @@ -382,16 +382,25 @@ public class MinecraftConnection extends ChannelInboundHandlerAdapter { if (threshold == -1) { channel.pipeline().remove(COMPRESSION_DECODER); channel.pipeline().remove(COMPRESSION_ENCODER); - return; + } else { + MinecraftCompressDecoder decoder = (MinecraftCompressDecoder) channel.pipeline() + .get(COMPRESSION_DECODER); + MinecraftCompressEncoder encoder = (MinecraftCompressEncoder) channel.pipeline() + .get(COMPRESSION_ENCODER); + if (decoder != null && encoder != null) { + decoder.setThreshold(threshold); + encoder.setThreshold(threshold); + } else { + int level = server.getConfiguration().getCompressionLevel(); + VelocityCompressor compressor = Natives.compress.get().create(level); + + encoder = new MinecraftCompressEncoder(threshold, compressor); + decoder = new MinecraftCompressDecoder(threshold, compressor); + + channel.pipeline().addBefore(MINECRAFT_DECODER, COMPRESSION_DECODER, decoder); + channel.pipeline().addBefore(MINECRAFT_ENCODER, COMPRESSION_ENCODER, encoder); + } } - - int level = server.getConfiguration().getCompressionLevel(); - VelocityCompressor compressor = Natives.compress.get().create(level); - MinecraftCompressEncoder encoder = new MinecraftCompressEncoder(threshold, compressor); - MinecraftCompressDecoder decoder = new MinecraftCompressDecoder(threshold, compressor); - - channel.pipeline().addBefore(MINECRAFT_DECODER, COMPRESSION_DECODER, decoder); - channel.pipeline().addBefore(MINECRAFT_ENCODER, COMPRESSION_ENCODER, encoder); } /** diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressDecoder.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressDecoder.java index a083a9291..21b958b05 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressDecoder.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressDecoder.java @@ -20,7 +20,7 @@ public class MinecraftCompressDecoder extends MessageToMessageDecoder { Boolean.getBoolean("velocity.increased-compression-cap") ? HARD_MAXIMUM_UNCOMPRESSED_SIZE : VANILLA_MAXIMUM_UNCOMPRESSED_SIZE; - private final int threshold; + private int threshold; private final VelocityCompressor compressor; public MinecraftCompressDecoder(int threshold, VelocityCompressor compressor) { @@ -60,4 +60,8 @@ public class MinecraftCompressDecoder extends MessageToMessageDecoder { public void handlerRemoved(ChannelHandlerContext ctx) throws Exception { compressor.close(); } + + public void setThreshold(int threshold) { + this.threshold = threshold; + } } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java index 23c67c024..aad607602 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java @@ -9,7 +9,7 @@ import io.netty.handler.codec.MessageToByteEncoder; public class MinecraftCompressEncoder extends MessageToByteEncoder { - private final int threshold; + private int threshold; private final VelocityCompressor compressor; public MinecraftCompressEncoder(int threshold, VelocityCompressor compressor) { @@ -54,4 +54,8 @@ public class MinecraftCompressEncoder extends MessageToByteEncoder { public void handlerRemoved(ChannelHandlerContext ctx) throws Exception { compressor.close(); } + + public void setThreshold(int threshold) { + this.threshold = threshold; + } } From 13ee59d20b5ead30210fb1504151c71e782b99bb Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Fri, 22 Jan 2021 19:49:38 -0500 Subject: [PATCH 03/13] Update CONTRIBUTING.md --- CONTRIBUTING.md | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e44a7026d..c650ff3af 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -28,9 +28,6 @@ sure that you are properly adhering to the code style. To reduce bugs and ensure code quality, we run the following tools on all commits and pull requests: -* [Checker Framework](https://checkerframework.org/): an enhancement to Java's type - system that is designed to help catch bugs. Velocity runs the _Nullness Checker_ - and the _Optional Checker_. The build will fail if Checker Framework notices an - issue. +* [Error Prone](https://errorprone.info/) * [Checkstyle](http://checkstyle.sourceforge.net/): ensures that your code is - correctly formatted. The build will fail if Checkstyle detects a problem. \ No newline at end of file + correctly formatted. The build will fail if Checkstyle detects a problem. From a2a799a8a7328546d762585cb35160ff187989f6 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Fri, 22 Jan 2021 19:50:31 -0500 Subject: [PATCH 04/13] Update CONTRIBUTING.md --- CONTRIBUTING.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c650ff3af..e6b626f81 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -28,6 +28,7 @@ sure that you are properly adhering to the code style. To reduce bugs and ensure code quality, we run the following tools on all commits and pull requests: -* [Error Prone](https://errorprone.info/) +* [SpotBugs](https://spotbugs.github.io/): ensures that common errors do not + get into the codebase. The build will fail if SpotBugs finds an issue. * [Checkstyle](http://checkstyle.sourceforge.net/): ensures that your code is correctly formatted. The build will fail if Checkstyle detects a problem. From ce7d1dcb2662e6c3f4416092d40dc226e344e441 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Sat, 23 Jan 2021 01:34:38 -0500 Subject: [PATCH 05/13] A new year, a new color for Velocity! --- .../proxy/command/builtin/VelocityCommand.java | 4 +++- .../velocitypowered/proxy/config/VelocityConfiguration.java | 2 +- proxy/src/main/resources/default-velocity.toml | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/command/builtin/VelocityCommand.java b/proxy/src/main/java/com/velocitypowered/proxy/command/builtin/VelocityCommand.java index 1b5e61919..3297f457b 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/command/builtin/VelocityCommand.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/command/builtin/VelocityCommand.java @@ -38,6 +38,7 @@ import net.kyori.adventure.text.TextComponent; import net.kyori.adventure.text.event.ClickEvent; import net.kyori.adventure.text.event.HoverEvent; import net.kyori.adventure.text.format.NamedTextColor; +import net.kyori.adventure.text.format.TextColor; import net.kyori.adventure.text.format.TextDecoration; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -188,6 +189,7 @@ public class VelocityCommand implements SimpleCommand { private static class Info implements SubCommand { + private static final TextColor VELOCITY_COLOR = TextColor.fromHexString("#09add3"); private final ProxyServer server; private Info(ProxyServer server) { @@ -205,7 +207,7 @@ public class VelocityCommand implements SimpleCommand { TextComponent velocity = Component.text().content(version.getName() + " ") .decoration(TextDecoration.BOLD, true) - .color(NamedTextColor.DARK_AQUA) + .color(VELOCITY_COLOR) .append(Component.text(version.getVersion()).decoration(TextDecoration.BOLD, false)) .build(); TextComponent copyright = Component diff --git a/proxy/src/main/java/com/velocitypowered/proxy/config/VelocityConfiguration.java b/proxy/src/main/java/com/velocitypowered/proxy/config/VelocityConfiguration.java index 5f849ff47..803312423 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/config/VelocityConfiguration.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/config/VelocityConfiguration.java @@ -458,7 +458,7 @@ public class VelocityConfiguration implements ProxyConfig { PingPassthroughMode.DISABLED); String bind = config.getOrElse("bind", "0.0.0.0:25577"); - String motd = config.getOrElse("motd", "&3A Velocity Server"); + String motd = config.getOrElse("motd", " add3A Velocity Server"); int maxPlayers = config.getIntOrElse("show-max-players", 500); Boolean onlineMode = config.getOrElse("online-mode", true); Boolean announceForge = config.getOrElse("announce-forge", true); diff --git a/proxy/src/main/resources/default-velocity.toml b/proxy/src/main/resources/default-velocity.toml index 050b9c1fe..fe57acad7 100644 --- a/proxy/src/main/resources/default-velocity.toml +++ b/proxy/src/main/resources/default-velocity.toml @@ -6,7 +6,7 @@ bind = "0.0.0.0:25577" # What should be the MOTD? This gets displayed when the player adds your server to # their server list. Legacy color codes and JSON are accepted. -motd = "&3A Velocity Server" +motd = "&3#09add3A Velocity Server" # What should we display for the maximum number of players? (Velocity does not support a cap # on the number of players online.) From fcffccf0d8d291be9ba0bdff9c7ed6717fce9db2 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Sat, 23 Jan 2021 01:35:49 -0500 Subject: [PATCH 06/13] Fix typo --- proxy/src/main/resources/default-velocity.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxy/src/main/resources/default-velocity.toml b/proxy/src/main/resources/default-velocity.toml index fe57acad7..f3b890926 100644 --- a/proxy/src/main/resources/default-velocity.toml +++ b/proxy/src/main/resources/default-velocity.toml @@ -6,7 +6,7 @@ bind = "0.0.0.0:25577" # What should be the MOTD? This gets displayed when the player adds your server to # their server list. Legacy color codes and JSON are accepted. -motd = "&3#09add3A Velocity Server" +motd = " add3A Velocity Server" # What should we display for the maximum number of players? (Velocity does not support a cap # on the number of players online.) From 03e9fa79d6bbf19ca7446c234d78bb4c400d1854 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Sat, 23 Jan 2021 12:55:28 -0500 Subject: [PATCH 07/13] Raise limit on JoinGame NBT reading to 2MiB. This is required for particularly large mod packs (think All of Fabric 3 for instance). --- .../velocitypowered/proxy/protocol/ProtocolUtils.java | 9 ++++++--- .../velocitypowered/proxy/protocol/packet/JoinGame.java | 6 ++++-- .../velocitypowered/proxy/protocol/packet/Respawn.java | 3 ++- 3 files changed, 12 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 a7e0bd437..2c4f50a99 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -16,6 +16,8 @@ import io.netty.handler.codec.CorruptedFrameException; import io.netty.handler.codec.DecoderException; import io.netty.handler.codec.EncoderException; +import java.io.DataInput; +import java.io.DataOutput; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.ArrayList; @@ -227,11 +229,12 @@ public enum ProtocolUtils { /** * Reads a {@link net.kyori.adventure.nbt.CompoundBinaryTag} from the {@code buf}. * @param buf the buffer to read from + * @param reader the NBT reader to use * @return {@link net.kyori.adventure.nbt.CompoundBinaryTag} the CompoundTag from the buffer */ - public static CompoundBinaryTag readCompoundTag(ByteBuf buf) { + public static CompoundBinaryTag readCompoundTag(ByteBuf buf, BinaryTagIO.Reader reader) { try { - return BinaryTagIO.readDataInput(new ByteBufInputStream(buf)); + return reader.read((DataInput) new ByteBufInputStream(buf)); } catch (IOException thrown) { throw new DecoderException( "Unable to parse NBT CompoundTag, full error: " + thrown.getMessage()); @@ -245,7 +248,7 @@ public enum ProtocolUtils { */ public static void writeCompoundTag(ByteBuf buf, CompoundBinaryTag compoundTag) { try { - BinaryTagIO.writeDataOutput(compoundTag, new ByteBufOutputStream(buf)); + BinaryTagIO.writer().write(compoundTag, (DataOutput) new ByteBufOutputStream(buf)); } catch (IOException e) { throw new EncoderException("Unable to encode NBT CompoundTag"); } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/JoinGame.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/JoinGame.java index 76c384584..84b4fd3ec 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/JoinGame.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/JoinGame.java @@ -8,6 +8,7 @@ import com.velocitypowered.proxy.connection.registry.DimensionInfo; import com.velocitypowered.proxy.connection.registry.DimensionRegistry; import com.velocitypowered.proxy.protocol.*; import io.netty.buffer.ByteBuf; +import net.kyori.adventure.nbt.BinaryTagIO; import net.kyori.adventure.nbt.BinaryTagTypes; import net.kyori.adventure.nbt.CompoundBinaryTag; import net.kyori.adventure.nbt.ListBinaryTag; @@ -15,6 +16,7 @@ import org.checkerframework.checker.nullness.qual.Nullable; public class JoinGame implements MinecraftPacket { + private static final BinaryTagIO.Reader JOINGAME_READER = BinaryTagIO.reader(2 * 1024 * 1024); private int entityId; private short gamemode; private int dimension; @@ -178,7 +180,7 @@ public class JoinGame implements MinecraftPacket { if (version.compareTo(ProtocolVersion.MINECRAFT_1_16) >= 0) { this.previousGamemode = buf.readByte(); ImmutableSet levelNames = ImmutableSet.copyOf(ProtocolUtils.readStringArray(buf)); - CompoundBinaryTag registryContainer = ProtocolUtils.readCompoundTag(buf); + CompoundBinaryTag registryContainer = ProtocolUtils.readCompoundTag(buf, JOINGAME_READER); ListBinaryTag dimensionRegistryContainer = null; if (version.compareTo(ProtocolVersion.MINECRAFT_1_16_2) >= 0) { dimensionRegistryContainer = registryContainer.getCompound("minecraft:dimension_type") @@ -192,7 +194,7 @@ public class JoinGame implements MinecraftPacket { DimensionRegistry.fromGameData(dimensionRegistryContainer, version); this.dimensionRegistry = new DimensionRegistry(readData, levelNames); if (version.compareTo(ProtocolVersion.MINECRAFT_1_16_2) >= 0) { - CompoundBinaryTag currentDimDataTag = ProtocolUtils.readCompoundTag(buf); + CompoundBinaryTag currentDimDataTag = ProtocolUtils.readCompoundTag(buf, JOINGAME_READER); dimensionIdentifier = ProtocolUtils.readString(buf); this.currentDimensionData = DimensionData.decodeBaseCompoundTag(currentDimDataTag, version) .annotateWith(dimensionIdentifier, null); diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/Respawn.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/Respawn.java index 9a5aee432..d8b0a674a 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/Respawn.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/Respawn.java @@ -7,6 +7,7 @@ import com.velocitypowered.proxy.connection.registry.DimensionInfo; import com.velocitypowered.proxy.protocol.MinecraftPacket; import com.velocitypowered.proxy.protocol.ProtocolUtils; import io.netty.buffer.ByteBuf; +import net.kyori.adventure.nbt.BinaryTagIO; import net.kyori.adventure.nbt.CompoundBinaryTag; public class Respawn implements MinecraftPacket { @@ -116,7 +117,7 @@ public class Respawn implements MinecraftPacket { String levelName = null; if (version.compareTo(ProtocolVersion.MINECRAFT_1_16) >= 0) { if (version.compareTo(ProtocolVersion.MINECRAFT_1_16_2) >= 0) { - CompoundBinaryTag dimDataTag = ProtocolUtils.readCompoundTag(buf); + CompoundBinaryTag dimDataTag = ProtocolUtils.readCompoundTag(buf, BinaryTagIO.reader()); dimensionIdentifier = ProtocolUtils.readString(buf); this.currentDimensionData = DimensionData.decodeBaseCompoundTag(dimDataTag, version) .annotateWith(dimensionIdentifier, null); From 7aed76ee3deab3b42051e11606173f1f7791174c Mon Sep 17 00:00:00 2001 From: 0x22 <0x22@futureclient.net> Date: Sat, 23 Jan 2021 18:35:14 -0500 Subject: [PATCH 08/13] Use keepalive queue instead of just the last keepalive. --- .../backend/BackendPlaySessionHandler.java | 2 +- .../backend/VelocityServerConnection.java | 22 +++++-------------- .../client/ClientPlaySessionHandler.java | 14 +++++++----- 3 files changed, 14 insertions(+), 24 deletions(-) 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 6dab7d8a5..667b1867e 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 @@ -83,7 +83,7 @@ public class BackendPlaySessionHandler implements MinecraftSessionHandler { @Override public boolean handle(KeepAlive packet) { - serverConn.setLastPingId(packet.getRandomId()); + serverConn.getPendingPings().put(packet.getRandomId(), System.currentTimeMillis()); return false; // forwards on } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/VelocityServerConnection.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/VelocityServerConnection.java index fe1144157..93958663e 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/VelocityServerConnection.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/VelocityServerConnection.java @@ -29,7 +29,9 @@ import io.netty.buffer.Unpooled; import io.netty.channel.ChannelFutureListener; import java.net.InetSocketAddress; import java.nio.charset.StandardCharsets; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.concurrent.CompletableFuture; import java.util.function.UnaryOperator; import org.checkerframework.checker.nullness.qual.MonotonicNonNull; @@ -44,8 +46,7 @@ public class VelocityServerConnection implements MinecraftConnectionAssociation, private boolean hasCompletedJoin = false; private boolean gracefulDisconnect = false; private BackendConnectionPhase connectionPhase = BackendConnectionPhases.UNKNOWN; - private long lastPingId; - private long lastPingSent; + private final Map pendingPings = new HashMap<>(); private @MonotonicNonNull DimensionRegistry activeDimensionRegistry; /** @@ -244,21 +245,8 @@ public class VelocityServerConnection implements MinecraftConnectionAssociation, return gracefulDisconnect; } - public long getLastPingId() { - return lastPingId; - } - - public long getLastPingSent() { - return lastPingSent; - } - - void setLastPingId(long lastPingId) { - this.lastPingId = lastPingId; - this.lastPingSent = System.currentTimeMillis(); - } - - public void resetLastPingId() { - this.lastPingId = -1; + public Map getPendingPings() { + return pendingPings; } /** diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java index b41a5ae32..414e2520b 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java @@ -99,12 +99,14 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler { @Override public boolean handle(KeepAlive packet) { VelocityServerConnection serverConnection = player.getConnectedServer(); - if (serverConnection != null && packet.getRandomId() == serverConnection.getLastPingId()) { - MinecraftConnection smc = serverConnection.getConnection(); - if (smc != null) { - player.setPing(System.currentTimeMillis() - serverConnection.getLastPingSent()); - smc.write(packet); - serverConnection.resetLastPingId(); + if (serverConnection != null) { + Long sentTime = serverConnection.getPendingPings().remove(packet.getRandomId()); + if (sentTime != null) { + MinecraftConnection smc = serverConnection.getConnection(); + if (smc != null) { + player.setPing(System.currentTimeMillis() - sentTime); + smc.write(packet); + } } } return true; From 4bc76b3012b5d8deeac4d007f30da44f649228ce Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 25 Jan 2021 01:50:56 -0500 Subject: [PATCH 09/13] Add JUL support to log4j Certain badly behaved plugins insist on logging with java.util.logging. Accommodate them. --- proxy/build.gradle | 1 + proxy/src/main/java/com/velocitypowered/proxy/Velocity.java | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/proxy/build.gradle b/proxy/build.gradle index 1ed87838e..25087f2e5 100644 --- a/proxy/build.gradle +++ b/proxy/build.gradle @@ -57,6 +57,7 @@ dependencies { implementation "org.apache.logging.log4j:log4j-core:${log4jVersion}" implementation "org.apache.logging.log4j:log4j-slf4j-impl:${log4jVersion}" implementation "org.apache.logging.log4j:log4j-iostreams:${log4jVersion}" + implementation "org.apache.logging.log4j:log4j-jul:${log4jVersion}" implementation 'net.sf.jopt-simple:jopt-simple:5.0.4' // command-line options implementation 'net.minecrell:terminalconsoleappender:1.2.0' diff --git a/proxy/src/main/java/com/velocitypowered/proxy/Velocity.java b/proxy/src/main/java/com/velocitypowered/proxy/Velocity.java index 9185a4f40..619cb4a01 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/Velocity.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/Velocity.java @@ -8,9 +8,12 @@ import org.apache.logging.log4j.Logger; public class Velocity { - private static final Logger logger = LogManager.getLogger(Velocity.class); + private static final Logger logger; static { + System.setProperty("java.util.logging.manager", "org.apache.logging.log4j.jul.LogManager"); + logger = LogManager.getLogger(Velocity.class); + // We use BufferedImage for favicons, and on macOS this puts the Java application in the dock. // How inconvenient. Force AWT to work with its head chopped off. System.setProperty("java.awt.headless", "true"); From 8d71ea7135dc14fa9654fbd84f30e23b7693004d Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 25 Jan 2021 01:54:50 -0500 Subject: [PATCH 10/13] Also provide a "proper" java.util.logger logger to plugins. --- .../proxy/plugin/loader/java/VelocityPluginModule.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/VelocityPluginModule.java b/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/VelocityPluginModule.java index 77e944ac8..2bfc33863 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/VelocityPluginModule.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/VelocityPluginModule.java @@ -31,6 +31,8 @@ class VelocityPluginModule implements Module { binder.bind(description.getMainClass()).in(Scopes.SINGLETON); binder.bind(Logger.class).toInstance(LoggerFactory.getLogger(description.getId())); + binder.bind(java.util.logging.Logger.class) + .toInstance(java.util.logging.Logger.getLogger(description.getId())); binder.bind(Path.class).annotatedWith(DataDirectory.class) .toInstance(basePluginPath.resolve(description.getId())); binder.bind(PluginDescription.class).toInstance(description); From 959e75d16db352924e679fb5be545ee9b264fbd2 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 25 Jan 2021 09:46:42 -0500 Subject: [PATCH 11/13] Revert "Also provide a "proper" java.util.logger logger to plugins." This reverts commit 8d71ea7135dc14fa9654fbd84f30e23b7693004d. Totally broken --- .../proxy/plugin/loader/java/VelocityPluginModule.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/VelocityPluginModule.java b/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/VelocityPluginModule.java index 2bfc33863..77e944ac8 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/VelocityPluginModule.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/VelocityPluginModule.java @@ -31,8 +31,6 @@ class VelocityPluginModule implements Module { binder.bind(description.getMainClass()).in(Scopes.SINGLETON); binder.bind(Logger.class).toInstance(LoggerFactory.getLogger(description.getId())); - binder.bind(java.util.logging.Logger.class) - .toInstance(java.util.logging.Logger.getLogger(description.getId())); binder.bind(Path.class).annotatedWith(DataDirectory.class) .toInstance(basePluginPath.resolve(description.getId())); binder.bind(PluginDescription.class).toInstance(description); From 5ceac16a821ea35572ff11412ace8929fd06e278 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Tue, 26 Jan 2021 12:33:35 -0500 Subject: [PATCH 12/13] 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; + } } From 0c90e94e85a521afd56ac83eb47b111675c827d2 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Tue, 26 Jan 2021 19:50:43 -0500 Subject: [PATCH 13/13] fix --- .../proxy/connection/client/LoginSessionHandler.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/LoginSessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/LoginSessionHandler.java index 2874ba137..f342cc15e 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/LoginSessionHandler.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/LoginSessionHandler.java @@ -36,6 +36,7 @@ import io.netty.buffer.ByteBuf; import java.net.InetSocketAddress; import java.security.GeneralSecurityException; import java.security.KeyPair; +import java.security.MessageDigest; import java.util.Arrays; import java.util.Optional; import java.util.UUID; @@ -90,7 +91,7 @@ public class LoginSessionHandler implements MinecraftSessionHandler { try { KeyPair serverKeyPair = server.getServerKeyPair(); byte[] decryptedVerifyToken = decryptRsa(serverKeyPair, packet.getVerifyToken()); - if (!Arrays.equals(verify, decryptedVerifyToken)) { + if (!MessageDigest.isEqual(verify, decryptedVerifyToken)) { throw new IllegalStateException("Unable to successfully decrypt the verification token."); }