From 0cccb540d9fef0c2990f930fd16f9d1d9ca34769 Mon Sep 17 00:00:00 2001 From: Danno Ferrin Date: Thu, 11 Oct 2018 11:43:14 -0600 Subject: [PATCH] NC-1026 - Validate Frame Length when receiving data (#30) * NC-1026 Make Framer#frame Throw on Excessively Large messages Note that this is * NC-1026 Make Framer#frame Throw on Excessively Large messages It is impossible to receive an uncompressed message < 16MiB because the length field in the header is 3 bytes. So this test will never pass. * NC-1026 Make Framer#frame Throw on Excessively Large messages review changes * NC-1026 Make Framer#frame Throw on Excessively Large messages Missed change in exception type. Added Null uncompressedLength test. --- .../ethereum/p2p/rlpx/framing/Compressor.java | 10 ++++++ .../ethereum/p2p/rlpx/framing/Framer.java | 11 +++++-- .../p2p/rlpx/framing/SnappyCompressor.java | 16 ++++++++-- .../ethereum/p2p/rlpx/framing/FramerTest.java | 31 ++++++++++++++++++- .../rlpx/framing/SnappyCompressorTest.java | 10 ++++-- 5 files changed, 69 insertions(+), 9 deletions(-) diff --git a/ethereum/p2p/src/main/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/Compressor.java b/ethereum/p2p/src/main/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/Compressor.java index 2f1c2940cd..58984d8668 100644 --- a/ethereum/p2p/src/main/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/Compressor.java +++ b/ethereum/p2p/src/main/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/Compressor.java @@ -22,4 +22,14 @@ public interface Compressor { * @return The original payload. */ byte[] decompress(byte[] compressed) throws CompressionException; + + /** + * Return the length when uncompressed + * + * @param compressed The compressed payload. + * @return The length of the payload when uncompressed. + * @throws CompressionException Thrown if the size cannot be calculated from the available data; + * expect to find the root cause inside; + */ + int uncompressedLength(byte[] compressed) throws CompressionException; } diff --git a/ethereum/p2p/src/main/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/Framer.java b/ethereum/p2p/src/main/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/Framer.java index 6c1ef9e5dd..eb3f21e8f9 100644 --- a/ethereum/p2p/src/main/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/Framer.java +++ b/ethereum/p2p/src/main/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/Framer.java @@ -15,6 +15,7 @@ import net.consensys.pantheon.util.bytes.BytesValue; import java.util.Arrays; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import io.netty.buffer.ByteBuf; import org.bouncycastle.crypto.BlockCipher; @@ -212,7 +213,6 @@ public class Framer { * * @param f The buffer containing * @param frameSize The expected - * @return */ private MessageData processFrame(final ByteBuf f, final int frameSize) { final int pad = padding16(frameSize); @@ -242,10 +242,14 @@ public class Framer { final int id = idbv.isZero() || idbv.size() == 0 ? 0 : idbv.get(0); // Write message data to ByteBuf, decompressing as necessary - ByteBuf data; + final ByteBuf data; if (compressionEnabled) { // Decompress data before writing to ByteBuf final byte[] compressedMessageData = Arrays.copyOfRange(frameData, 1, frameData.length - pad); + // Check message length + Preconditions.checkState( + compressor.uncompressedLength(compressedMessageData) < LENGTH_MAX_MESSAGE_FRAME, + "Message size in excess of maximum length."); final byte[] decompressedMessageData = compressor.decompress(compressedMessageData); data = NetworkMemoryPool.allocate(decompressedMessageData.length); data.writeBytes(decompressedMessageData); @@ -301,7 +305,8 @@ public class Framer { } } - private ByteBuf frameAndReleaseMessage(final MessageData message) { + @VisibleForTesting + ByteBuf frameAndReleaseMessage(final MessageData message) { try { final int frameSize = message.getSize() + LENGTH_MESSAGE_ID; final int pad = padding16(frameSize); diff --git a/ethereum/p2p/src/main/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/SnappyCompressor.java b/ethereum/p2p/src/main/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/SnappyCompressor.java index e4bb8cf78f..c40b3ff8f2 100644 --- a/ethereum/p2p/src/main/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/SnappyCompressor.java +++ b/ethereum/p2p/src/main/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/SnappyCompressor.java @@ -1,6 +1,6 @@ package net.consensys.pantheon.ethereum.p2p.rlpx.framing; -import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import java.io.IOException; @@ -15,7 +15,7 @@ public class SnappyCompressor implements Compressor { @Override public byte[] compress(final byte[] uncompressed) { - checkArgument(uncompressed != null, "input data must not be null"); + checkNotNull(uncompressed, "input data must not be null"); try { return Snappy.compress(uncompressed); } catch (final IOException e) { @@ -25,11 +25,21 @@ public class SnappyCompressor implements Compressor { @Override public byte[] decompress(final byte[] compressed) { - checkArgument(compressed != null, "input data must not be null"); + checkNotNull(compressed, "input data must not be null"); try { return Snappy.uncompress(compressed); } catch (final IOException e) { throw new CompressionException("Snappy decompression failed", e); } } + + @Override + public int uncompressedLength(final byte[] compressed) { + checkNotNull(compressed, "input data must not be null"); + try { + return Snappy.uncompressedLength(compressed); + } catch (final IOException e) { + throw new CompressionException("Snappy uncompressedLength failed", e); + } + } } diff --git a/ethereum/p2p/src/test/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/FramerTest.java b/ethereum/p2p/src/test/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/FramerTest.java index 38d13b076b..9ff5c17307 100644 --- a/ethereum/p2p/src/test/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/FramerTest.java +++ b/ethereum/p2p/src/test/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/FramerTest.java @@ -22,12 +22,13 @@ import com.fasterxml.jackson.databind.ObjectMapper; import io.netty.buffer.ByteBuf; import io.netty.buffer.Unpooled; import org.junit.Test; +import org.xerial.snappy.Snappy; public class FramerTest { private static final ObjectMapper MAPPER = new ObjectMapper(); @Test - public void shouldThrowExceptionWhenMessageTooLong() { + public void shouldThrowExceptionWhenFramingMessageTooLong() { final byte[] aes = { 0xa, 0x2, 0xa, 0x2, 0xa, 0x2, 0xa, 0x2, 0xa, 0x2, 0xa, 0x2, 0xa, 0x2, 0xa, 0x2, 0xa, 0x2, 0xa, 0x2, 0xa, 0x2, 0xa, 0x2, 0xa, 0x2, 0xa, 0x2, 0xa, 0x2, 0xa, 0x2 @@ -50,6 +51,34 @@ public class FramerTest { .withMessageContaining("Message size in excess of maximum length."); } + @Test + public void shouldThrowExceptionWhenDeframingCompressedMessageTooLong() throws IOException { + // This is a circular test. + // + // This test frames a too-large message; it then impersonates the sending end + // by swapping the ingress and egress MACs and frames the plaintext messages. + // The expected result is an exception on deframing not relating to encryption. + // + final JsonNode td = MAPPER.readTree(FramerTest.class.getResource("/peer1.json")); + final HandshakeSecrets secrets = secretsFrom(td, false); + final Framer framer = new Framer(secrets); + framer.enableCompression(); + + final byte[] byteArray = Snappy.compress(new byte[0x1000000]); + final ByteBuf buf = wrappedBuffer(byteArray); + + final MessageData ethMessage = new RawMessage(0x00, buf); + final ByteBuf framedMessage = framer.frameAndReleaseMessage(ethMessage); + + final HandshakeSecrets deframeSecrets = secretsFrom(td, true); + final Framer deframer = new Framer(deframeSecrets); + deframer.enableCompression(); + + assertThatExceptionOfType(IllegalStateException.class) + .isThrownBy(() -> deframer.deframe(framedMessage)) + .withMessageContaining("Message size in excess of maximum length."); + } + @Test public void deframeOne() throws IOException { // Load test data. diff --git a/ethereum/p2p/src/test/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/SnappyCompressorTest.java b/ethereum/p2p/src/test/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/SnappyCompressorTest.java index 101396323a..cf768cd457 100644 --- a/ethereum/p2p/src/test/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/SnappyCompressorTest.java +++ b/ethereum/p2p/src/test/java/net/consensys/pantheon/ethereum/p2p/rlpx/framing/SnappyCompressorTest.java @@ -25,18 +25,24 @@ public class SnappyCompressorTest { assertThat(snappy.decompress(snappy.compress(data))).isEqualTo(data); } - @Test(expected = IllegalArgumentException.class) + @Test(expected = NullPointerException.class) public void compressNull() { final SnappyCompressor snappy = new SnappyCompressor(); snappy.compress(null); } - @Test(expected = IllegalArgumentException.class) + @Test(expected = NullPointerException.class) public void decompressNull() { final SnappyCompressor snappy = new SnappyCompressor(); snappy.decompress(null); } + @Test(expected = NullPointerException.class) + public void uncompressedLengthNull() { + final SnappyCompressor snappy = new SnappyCompressor(); + snappy.uncompressedLength(null); + } + @Test public void roundTripEthereumData() { // First data set.