From b1bb31343bb6d573761423ad649b7580d9817577 Mon Sep 17 00:00:00 2001 From: yawkat Date: Fri, 15 Dec 2023 15:05:21 +0100 Subject: [PATCH 1/6] Fix exception on HTTP chunk size overflow Motivation: If there is an overflow in the chunk size calculation, it would throw an exception on the pipeline instead of an invalid chunk like other number format errors. In particular: ``` Caused by: java.lang.IllegalArgumentException: minimumReadableBytes : -1932735284 (expected: >= 0) at io.netty.util.internal.ObjectUtil.checkPositiveOrZero(ObjectUtil.java:144) at io.netty.buffer.AbstractByteBuf.checkReadableBytes(AbstractByteBuf.java:1428) at io.netty.buffer.AbstractByteBuf.readRetainedSlice(AbstractByteBuf.java:887) at io.netty.handler.codec.http.HttpObjectDecoder.decode(HttpObjectDecoder.java:480) at io.netty.handler.codec.http.HttpServerCodec$HttpServerRequestDecoder.decode(HttpServerCodec.java:167) at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:529) at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:468) ... 134 common frames omitted ``` Modification: Check for negative chunk size, and throw a NumberFormatException early. Result: This invalid input is treated like any other invalid chunk. --- .../handler/codec/http/HttpObjectDecoder.java | 3 +++ .../handler/codec/http/HttpRequestDecoderTest.java | 14 ++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java index 18dfde41da62..5355ae44e11a 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java @@ -913,6 +913,9 @@ private static int getChunkSize(byte[] hex, int start, int length) { result *= 16; result += digit; } + if (result < 0) { + throw new NumberFormatException(); + } return result; } diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java index 35a0def4303f..0033e8830b44 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java @@ -639,6 +639,20 @@ void headerValuesMayBeBracketedByZeroOrMoreWhitespace() throws Exception { assertFalse(channel.finish()); } + @Test + public void testChunkSizeOverflow() { + String requestStr = "PUT /some/path HTTP/1.1\r\n" + + "Transfer-Encoding: chunked\r\n\r\n" + + "8ccccccc\r\n"; + EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestDecoder()); + assertTrue(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII))); + HttpRequest request = channel.readInbound(); + assertTrue(request.decoderResult().isSuccess()); + HttpContent c = channel.readInbound(); + c.release(); + assertFalse(channel.finish()); + } + private static void testInvalidHeaders0(String requestStr) { testInvalidHeaders0(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII)); } From 2e6a44aaec7c18b057462f213224bd81628e1afe Mon Sep 17 00:00:00 2001 From: yawkat Date: Fri, 15 Dec 2023 15:54:12 +0100 Subject: [PATCH 2/6] Add message --- .../java/io/netty/handler/codec/http/HttpObjectDecoder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java index 5355ae44e11a..3b784d1ee3a0 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java @@ -914,7 +914,7 @@ private static int getChunkSize(byte[] hex, int start, int length) { result += digit; } if (result < 0) { - throw new NumberFormatException(); + throw new NumberFormatException("Chunk size overflow: " + result); } return result; } From 8062eed628df05f02988f2570ebce5e0ab887856 Mon Sep 17 00:00:00 2001 From: yawkat Date: Tue, 19 Dec 2023 11:09:05 +0100 Subject: [PATCH 3/6] Fix the fix --- .../handler/codec/http/HttpObjectDecoder.java | 2 +- .../handler/codec/http/HttpRequestDecoderTest.java | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java index 3b784d1ee3a0..d79aef1a9ba8 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java @@ -905,7 +905,7 @@ private static int getChunkSize(byte[] hex, int start, int length) { // empty case throw new NumberFormatException(); } - return result; + break; } // non-hex char fail-fast path throw new NumberFormatException(); diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java index 0033e8830b44..7f66e677cc4f 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java @@ -653,6 +653,20 @@ public void testChunkSizeOverflow() { assertFalse(channel.finish()); } + @Test + public void testChunkSizeOverflow2() { + String requestStr = "PUT /some/path HTTP/1.1\r\n" + + "Transfer-Encoding: chunked\r\n\r\n" + + "bbbbbbbe;\n\r\n"; + EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestDecoder()); + assertTrue(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII))); + HttpRequest request = channel.readInbound(); + assertTrue(request.decoderResult().isSuccess()); + HttpContent c = channel.readInbound(); + c.release(); + assertFalse(channel.finish()); + } + private static void testInvalidHeaders0(String requestStr) { testInvalidHeaders0(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII)); } From 7ccdc7014bb839072e12925dd39e796f22b12899 Mon Sep 17 00:00:00 2001 From: yawkat Date: Wed, 20 Dec 2023 09:43:46 +0100 Subject: [PATCH 4/6] move into loop --- .../java/io/netty/handler/codec/http/HttpObjectDecoder.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java index d79aef1a9ba8..3d9d75f42d07 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java @@ -912,9 +912,9 @@ private static int getChunkSize(byte[] hex, int start, int length) { } result *= 16; result += digit; - } - if (result < 0) { - throw new NumberFormatException("Chunk size overflow: " + result); + if (result < 0) { + throw new NumberFormatException("Chunk size overflow: " + result); + } } return result; } From 868a7794f17c9534f3bebeb0e40c11999043259a Mon Sep 17 00:00:00 2001 From: yawkat Date: Thu, 21 Dec 2023 08:58:12 +0100 Subject: [PATCH 5/6] revert break change now that the if is inside the loop --- .../java/io/netty/handler/codec/http/HttpObjectDecoder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java index 3d9d75f42d07..04e615ed52da 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java @@ -905,7 +905,7 @@ private static int getChunkSize(byte[] hex, int start, int length) { // empty case throw new NumberFormatException(); } - break; + return result; } // non-hex char fail-fast path throw new NumberFormatException(); From 1bf5af2faf3029563d1b18be303a4ec21e073355 Mon Sep 17 00:00:00 2001 From: yawkat Date: Thu, 21 Dec 2023 09:00:19 +0100 Subject: [PATCH 6/6] address review --- .../java/io/netty/handler/codec/http/HttpObjectDecoder.java | 4 ++-- .../io/netty/handler/codec/http/HttpRequestDecoderTest.java | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java index 04e615ed52da..282796f465b6 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java @@ -903,12 +903,12 @@ private static int getChunkSize(byte[] hex, int start, int length) { if (b == ';' || isControlOrWhitespaceAsciiChar(b)) { if (i == 0) { // empty case - throw new NumberFormatException(); + throw new NumberFormatException("Empty chunk size"); } return result; } // non-hex char fail-fast path - throw new NumberFormatException(); + throw new NumberFormatException("Invalid character in chunk size"); } result *= 16; result += digit; diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java index 7f66e677cc4f..6a6d3589e139 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java @@ -35,6 +35,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -650,6 +651,8 @@ public void testChunkSizeOverflow() { assertTrue(request.decoderResult().isSuccess()); HttpContent c = channel.readInbound(); c.release(); + assertTrue(c.decoderResult().isFailure()); + assertInstanceOf(NumberFormatException.class, c.decoderResult().cause()); assertFalse(channel.finish()); } @@ -664,6 +667,8 @@ public void testChunkSizeOverflow2() { assertTrue(request.decoderResult().isSuccess()); HttpContent c = channel.readInbound(); c.release(); + assertTrue(c.decoderResult().isFailure()); + assertInstanceOf(NumberFormatException.class, c.decoderResult().cause()); assertFalse(channel.finish()); }