From 9510b1e224526e367d4b2dd77856336a5ed75493 Mon Sep 17 00:00:00 2001 From: "Five (Xer)" Date: Sat, 30 Jan 2021 18:04:14 +0100 Subject: [PATCH] Additional DoS mitigations Some stricter length checks and cached exceptions to withstand more illegitimate connections Courtesy of Tux and the Velocity Contributors. See: https://github.com/VelocityPowered/Velocity/commit/5ceac16a821ea35572ff11412ace8929fd06e278 diff --git a/protocol/src/main/java/net/md_5/bungee/protocol/DefinedPacket.java b/protocol/src/main/java/net/md_5/bungee/protocol/DefinedPacket.java index dffd3b7a..dafaba54 100644 --- a/protocol/src/main/java/net/md_5/bungee/protocol/DefinedPacket.java +++ b/protocol/src/main/java/net/md_5/bungee/protocol/DefinedPacket.java @@ -82,6 +82,7 @@ public abstract class DefinedPacket int len = readVarInt( buf ); if ( len > maxLen * 3 ) { + if(!MinecraftDecoder.DEBUG) throw STRING_TOO_MANY_BYTES_EXCEPTION; // Waterfall start: Additional DoS mitigations throw new OverflowPacketException( "Cannot receive string longer than " + maxLen * 3 + " (got " + len + " bytes)" ); } @@ -90,6 +91,7 @@ public abstract class DefinedPacket if ( s.length() > maxLen ) { + if(!MinecraftDecoder.DEBUG) throw STRING_TOO_LONG_EXCEPTION; // Waterfall start: Additional DoS mitigations throw new OverflowPacketException( "Cannot receive string longer than " + maxLen + " (got " + s.length() + " characters)" ); } @@ -567,4 +569,21 @@ public abstract class DefinedPacket @Override public abstract String toString(); + + // Waterfall start: Additional DoS mitigations, courtesy of Velocity + private static final OverflowPacketException STRING_TOO_LONG_EXCEPTION + = new OverflowPacketException("A string was longer than allowed. For more " + + "information, launch Waterfall with -Dwaterfall.packet-decode-logging=true"); + private static final OverflowPacketException STRING_TOO_MANY_BYTES_EXCEPTION + = new OverflowPacketException("A string had more data than allowed. For more " + + "information, launch Waterfall with -Dwaterfall.packet-decode-logging=true"); + + public int expectedMaxLength(ByteBuf buf, ProtocolConstants.Direction direction, int protocolVersion) { + return -1; + } + + public int expectedMinLength(ByteBuf buf, ProtocolConstants.Direction direction, int protocolVersion) { + return 0; + } + // Waterfall end } diff --git a/protocol/src/main/java/net/md_5/bungee/protocol/MinecraftDecoder.java b/protocol/src/main/java/net/md_5/bungee/protocol/MinecraftDecoder.java index 52f76cd9..3a4a735c 100644 --- a/protocol/src/main/java/net/md_5/bungee/protocol/MinecraftDecoder.java +++ b/protocol/src/main/java/net/md_5/bungee/protocol/MinecraftDecoder.java @@ -3,7 +3,7 @@ package net.md_5.bungee.protocol; import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufUtil; import io.netty.channel.ChannelHandlerContext; -import io.netty.handler.codec.DecoderException; +import io.netty.handler.codec.CorruptedFrameException; import io.netty.handler.codec.MessageToMessageDecoder; import java.util.List; import lombok.AllArgsConstructor; @@ -58,10 +58,16 @@ public class MinecraftDecoder extends MessageToMessageDecoder if ( packet != null ) { packetTypeInfo = packet.getClass(); + doLengthSanityChecks(in, packet, prot.getDirection(), packetId); // Waterfall: Additional DoS mitigations packet.read( in, protocol, prot.getDirection(), protocolVersion ); if ( in.isReadable() ) { + // Waterfall start: Additional DoS mitigations + if(!DEBUG) { + throw PACKET_NOT_READ_TO_END; + } + // Waterfall end throw new BadPacketException( "Packet " + protocol + ":" + prot.getDirection() + "/" + packetId + " (" + packet.getClass().getSimpleName() + ") larger than expected, extra bytes: " + in.readableBytes() ); } } else @@ -72,6 +78,25 @@ public class MinecraftDecoder extends MessageToMessageDecoder out.add( new PacketWrapper( packet, slice, protocol ) ); slice = null; } catch (BadPacketException | IndexOutOfBoundsException e) { + // Waterfall start: Additional DoS mitigations + if(!DEBUG) { + throw e; + } + // Waterfall end + final String packetTypeStr; + if (packetTypeInfo instanceof Integer) { + packetTypeStr = "id " + Integer.toHexString((Integer) packetTypeInfo); + } else if (packetTypeInfo instanceof Class) { + packetTypeStr = "class " + ((Class) packetTypeInfo).getSimpleName(); + } else { + packetTypeStr = "unknown"; + } + throw new FastDecoderException("Error decoding packet " + packetTypeStr + " with contents:\n" + ByteBufUtil.prettyHexDump(slice), e); // Waterfall + // Waterfall start + } catch (Exception e) { + if (!DEBUG) { + throw e; + } final String packetTypeStr; if (packetTypeInfo instanceof Integer) { packetTypeStr = "id " + Integer.toHexString((Integer) packetTypeInfo); @@ -81,6 +106,7 @@ public class MinecraftDecoder extends MessageToMessageDecoder packetTypeStr = "unknown"; } throw new FastDecoderException("Error decoding packet " + packetTypeStr + " with contents:\n" + ByteBufUtil.prettyHexDump(slice), e); // Waterfall + // Waterfall end } finally { if ( slice != null ) @@ -89,4 +115,52 @@ public class MinecraftDecoder extends MessageToMessageDecoder } } } + + // Waterfall start: Additional DoS mitigations, courtesy of Velocity + public static final boolean DEBUG = Boolean.getBoolean("waterfall.packet-decode-logging"); + + // Cached Exceptions: + private static final CorruptedFrameException PACKET_LENGTH_OVERSIZED = + new CorruptedFrameException("A packet could not be decoded because it was too large. For more " + + "information, launch Waterfall with -Dwaterfall.packet-decode-logging=true"); + private static final CorruptedFrameException PACKET_LENGTH_UNDERSIZED = + new CorruptedFrameException("A packet could not be decoded because it was smaller than allowed. For more " + + "information, launch Waterfall with -Dwaterfall.packet-decode-logging=true"); + private static final BadPacketException PACKET_NOT_READ_TO_END = + new BadPacketException("Couldn't read all bytes from a packet. For more " + + "information, launch Waterfall with -Dwaterfall.packet-decode-logging=true"); + + + private void doLengthSanityChecks(ByteBuf buf, DefinedPacket packet, + ProtocolConstants.Direction direction, int packetId) throws Exception { + int expectedMinLen = packet.expectedMinLength(buf, direction, protocolVersion); + int expectedMaxLen = packet.expectedMaxLength(buf, direction, protocolVersion); + if (expectedMaxLen != -1 && buf.readableBytes() > expectedMaxLen) { + throw handleOverflow(packet, expectedMaxLen, buf.readableBytes(), packetId); + } + if (buf.readableBytes() < expectedMinLen) { + throw handleUnderflow(packet, expectedMaxLen, buf.readableBytes(), packetId); + } + } + + private Exception handleOverflow(DefinedPacket packet, int expected, int actual, int packetId) { + if (DEBUG) { + throw new CorruptedFrameException( "Packet " + packet.getClass() + " " + packetId + + " Protocol " + protocolVersion + " was too big (expected " + + expected + " bytes, got " + actual + " bytes)"); + } else { + return PACKET_LENGTH_OVERSIZED; + } + } + + private Exception handleUnderflow(DefinedPacket packet, int expected, int actual, int packetId) { + if (DEBUG) { + throw new CorruptedFrameException( "Packet " + packet.getClass() + " " + packetId + + " Protocol " + protocolVersion + " was too small (expected " + + expected + " bytes, got " + actual + " bytes)"); + } else { + return PACKET_LENGTH_UNDERSIZED; + } + } + // Waterfall end } diff --git a/protocol/src/main/java/net/md_5/bungee/protocol/packet/EncryptionResponse.java b/protocol/src/main/java/net/md_5/bungee/protocol/packet/EncryptionResponse.java index 63e9d18d..545eec72 100644 --- a/protocol/src/main/java/net/md_5/bungee/protocol/packet/EncryptionResponse.java +++ b/protocol/src/main/java/net/md_5/bungee/protocol/packet/EncryptionResponse.java @@ -63,4 +63,17 @@ public class EncryptionResponse extends DefinedPacket private final long salt; private final byte[] signature; } + + // Waterfall start: Additional DoS mitigations, courtesy of Velocity + public int expectedMaxLength(ByteBuf buf, ProtocolConstants.Direction direction, int protocolVersion) { + // 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. + if (protocolVersion >= ProtocolConstants.MINECRAFT_1_19) return -1; + return 260; + } + + public int expectedMinLength(ByteBuf buf, ProtocolConstants.Direction direction, int protocolVersion) { + return expectedMaxLength(buf, direction, protocolVersion); + } + // Waterfall end } diff --git a/protocol/src/main/java/net/md_5/bungee/protocol/packet/LoginRequest.java b/protocol/src/main/java/net/md_5/bungee/protocol/packet/LoginRequest.java index e62a3a03..9789215c 100644 --- a/protocol/src/main/java/net/md_5/bungee/protocol/packet/LoginRequest.java +++ b/protocol/src/main/java/net/md_5/bungee/protocol/packet/LoginRequest.java @@ -71,4 +71,13 @@ public class LoginRequest extends DefinedPacket { handler.handle( this ); } + + // Waterfall start: Additional DoS mitigations, courtesy of Velocity + public int expectedMaxLength(ByteBuf buf, ProtocolConstants.Direction direction, int protocolVersion) { + // Accommodate the rare (but likely malicious) use of UTF-8 usernames, since it is technically + // legal on the protocol level. + if (protocolVersion >= ProtocolConstants.MINECRAFT_1_19) return -1; + return 1 + (16 * 3); + } + // Waterfall end } diff --git a/protocol/src/main/java/net/md_5/bungee/protocol/packet/PingPacket.java b/protocol/src/main/java/net/md_5/bungee/protocol/packet/PingPacket.java index 5f24d425..3163a771 100644 --- a/protocol/src/main/java/net/md_5/bungee/protocol/packet/PingPacket.java +++ b/protocol/src/main/java/net/md_5/bungee/protocol/packet/PingPacket.java @@ -7,6 +7,7 @@ import lombok.EqualsAndHashCode; import lombok.NoArgsConstructor; import net.md_5.bungee.protocol.AbstractPacketHandler; import net.md_5.bungee.protocol.DefinedPacket; +import net.md_5.bungee.protocol.ProtocolConstants; @Data @NoArgsConstructor @@ -34,4 +35,14 @@ public class PingPacket extends DefinedPacket { handler.handle( this ); } + + // Waterfall start: Additional DoS mitigations, courtesy of Velocity + public int expectedMaxLength(ByteBuf buf, ProtocolConstants.Direction direction, int protocolVersion) { + return 8; + } + + public int expectedMinLength(ByteBuf buf, ProtocolConstants.Direction direction, int protocolVersion) { + return 8; + } + // Waterfall end } diff --git a/protocol/src/main/java/net/md_5/bungee/protocol/packet/StatusRequest.java b/protocol/src/main/java/net/md_5/bungee/protocol/packet/StatusRequest.java index 738f0c92..ec33d337 100644 --- a/protocol/src/main/java/net/md_5/bungee/protocol/packet/StatusRequest.java +++ b/protocol/src/main/java/net/md_5/bungee/protocol/packet/StatusRequest.java @@ -6,6 +6,7 @@ import lombok.EqualsAndHashCode; import lombok.NoArgsConstructor; import net.md_5.bungee.protocol.AbstractPacketHandler; import net.md_5.bungee.protocol.DefinedPacket; +import net.md_5.bungee.protocol.ProtocolConstants; @Data @NoArgsConstructor @@ -28,4 +29,10 @@ public class StatusRequest extends DefinedPacket { handler.handle( this ); } + + // Waterfall start: Additional DoS mitigations, courtesy of Velocity + public int expectedMaxLength(ByteBuf buf, ProtocolConstants.Direction direction, int protocolVersion) { + return 0; + } + // Waterfall end } -- 2.44.0