From f88283f1271569de4d8256cab25c5b421ef7ad12 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Tue, 30 Mar 2021 12:07:46 -0400 Subject: [PATCH] Revert "Revert strict handshake hostname checks" This reverts commit 4f80d2b261d0a90d9076474e9e5d55f63fd171d9. Experience elsewhere (Waterfall PR) and confirmation from TCPShield means this ought to work. Let's hope. --- .../proxy/protocol/ProtocolUtils.java | 20 ++++++++++++++++++- .../proxy/protocol/packet/Handshake.java | 8 +++++++- 2 files changed, 26 insertions(+), 2 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 2c4f50a99..7da858d9f 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -97,12 +97,30 @@ public enum ProtocolUtils { } } + /** + * Reads a VarInt length-prefixed ASCII string from the {@code buf}, making sure to not go over + * {@code cap} size. This method is specialized for select parts of the Minecraft protocol where + * ASCII characters are guaranteed to be used. + * + * @param buf the buffer to read from + * @param cap the maximum size of the string, in UTF-8 character length + * @return the decoded string + */ + public static String readAsciiString(ByteBuf buf, int cap) { + int length = readVarInt(buf); + checkFrame(length >= 0, "Got a negative-length string (%s)", length); + checkFrame(length <= cap, "Bad string size (got %s, maximum is %s)", length, cap); + String str = buf.toString(buf.readerIndex(), length, StandardCharsets.US_ASCII); + buf.skipBytes(length); + return str; + } + public static String readString(ByteBuf buf) { return readString(buf, DEFAULT_MAX_STRING_SIZE); } /** - * Reads a VarInt length-prefixed string from the {@code buf}, making sure to not go over + * Reads a VarInt length-prefixed UTF-8 string from the {@code buf}, making sure to not go over * {@code cap} size. * @param buf the buffer to read from * @param cap the maximum size of the string, in UTF-8 character length diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/Handshake.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/Handshake.java index cf64b7b93..a1c902057 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/Handshake.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/Handshake.java @@ -1,13 +1,19 @@ package com.velocitypowered.proxy.protocol.packet; +import static com.velocitypowered.proxy.connection.forge.legacy.LegacyForgeConstants.HANDSHAKE_HOSTNAME_TOKEN; + import com.velocitypowered.api.network.ProtocolVersion; import com.velocitypowered.proxy.connection.MinecraftSessionHandler; +import com.velocitypowered.proxy.connection.forge.legacy.LegacyForgeConstants; import com.velocitypowered.proxy.protocol.MinecraftPacket; import com.velocitypowered.proxy.protocol.ProtocolUtils; import io.netty.buffer.ByteBuf; public class Handshake implements MinecraftPacket { + // This size was chosen to ensure Forge clients can still connect even with very long hostnames. + // While DNS technically allows any character to be used, in practice ASCII is used. + private static final int MAXIMUM_HOSTNAME_LENGTH = 255 + HANDSHAKE_HOSTNAME_TOKEN.length() + 1; private ProtocolVersion protocolVersion; private String serverAddress = ""; private int port; @@ -59,7 +65,7 @@ public class Handshake implements MinecraftPacket { public void decode(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion ignored) { int realProtocolVersion = ProtocolUtils.readVarInt(buf); this.protocolVersion = ProtocolVersion.getProtocolVersion(realProtocolVersion); - this.serverAddress = ProtocolUtils.readString(buf); + this.serverAddress = ProtocolUtils.readAsciiString(buf, MAXIMUM_HOSTNAME_LENGTH); this.port = buf.readUnsignedShort(); this.nextStatus = ProtocolUtils.readVarInt(buf); }