From a97fd919809d2284cdc6a5d68168c99bfe795d9c 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 40281aa4..a310844d 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 @@ -44,6 +44,7 @@ public abstract class DefinedPacket int len = readVarInt( buf ); if ( len > maxLen * 4 ) { + if(!MinecraftDecoder.DEBUG) throw STRING_TOO_MANY_BYTES_EXCEPTION; // Waterfall start: Additional DoS mitigations throw new OverflowPacketException( "Cannot receive string longer than " + maxLen * 4 + " (got " + len + " bytes)" ); } @@ -53,6 +54,7 @@ public abstract class DefinedPacket String s = new String( b, Charsets.UTF_8 ); 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)" ); } @@ -284,4 +286,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 f5e414c1..ac83e325 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; @@ -56,10 +56,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, 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 @@ -70,6 +76,11 @@ public class MinecraftDecoder extends MessageToMessageDecoder out.add( new PacketWrapper( packet, slice ) ); 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); @@ -87,4 +98,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 388f6cdb..53575ce0 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 @@ -38,4 +38,16 @@ public class EncryptionResponse extends DefinedPacket { handler.handle( this ); } + + // 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. + 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 08ee376e..cfd8e64e 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 @@ -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,12 @@ 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. + return 1 + (16 * 4); + } + // 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.34.1