252 lines
12 KiB
Diff
252 lines
12 KiB
Diff
From b1bb31343bb6d573761423ad649b7580d9817577 Mon Sep 17 00:00:00 2001
|
|
From: yawkat <jonas.konrad@oracle.com>
|
|
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 <jonas.konrad@oracle.com>
|
|
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 <jonas.konrad@oracle.com>
|
|
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 <jonas.konrad@oracle.com>
|
|
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 <jonas.konrad@oracle.com>
|
|
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 <jonas.konrad@oracle.com>
|
|
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());
|
|
}
|
|
|