Skip to content

Commit 0e8af54

Browse files
franz1981normanmaurer
franz1981
and
normanmaurer
authored
Chunked HTTP length decoding should account for whitespaces/ctrl chars (Fixes #13273) (#13274)
 Motivation: #12321 introduces the assumption that whitespaces/ctrl chars are already stripped out hex chars of HTTP chunk length. It was a wrong assumption. Modifications: Correctly check for whitespace and ctrl chars and re-introduce whitespace (leading) trim Result: Fixes #13273 Co-authored-by: Norman Maurer <norman_maurer@apple.com>
1 parent ab5c411 commit 0e8af54

2 files changed

Lines changed: 258 additions & 2 deletions

File tree

‎codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java‎

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -799,16 +799,41 @@ private LastHttpContent readTrailingHeaders(ByteBuf buffer) {
799799
protected abstract HttpMessage createMessage(String[] initialLine) throws Exception;
800800
protected abstract HttpMessage createInvalidMessage();
801801

802+
/**
803+
* It skips any whitespace char and return the number of skipped bytes.
804+
*/
805+
private static int skipWhiteSpaces(byte[] hex, int start, int length) {
806+
for (int i = 0; i < length; i++) {
807+
if (!isWhitespace(hex[start + i])) {
808+
return i;
809+
}
810+
}
811+
return length;
812+
}
813+
802814
private static int getChunkSize(byte[] hex, int start, int length) {
803-
// byte[] is produced by LineParse::parseLine that already skip ISO CTRL and Whitespace chars
815+
// trim the leading bytes if white spaces, if any
816+
final int skipped = skipWhiteSpaces(hex, start, length);
817+
if (skipped == length) {
818+
// empty case
819+
throw new NumberFormatException();
820+
}
821+
start += skipped;
822+
length -= skipped;
804823
int result = 0;
805824
for (int i = 0; i < length; i++) {
806825
final int digit = StringUtil.decodeHexNibble(hex[start + i]);
807826
if (digit == -1) {
808827
// uncommon path
809-
if (hex[start + i] == ';') {
828+
final byte b = hex[start + i];
829+
if (b == ';' || isControlOrWhitespaceAsciiChar(b)) {
830+
if (i == 0) {
831+
// empty case
832+
throw new NumberFormatException();
833+
}
810834
return result;
811835
}
836+
// non-hex char fail-fast path
812837
throw new NumberFormatException();
813838
}
814839
result *= 16;
@@ -1127,4 +1152,8 @@ public boolean process(byte value) {
11271152
return ISO_CONTROL_OR_WHITESPACE[128 + value];
11281153
}
11291154
};
1155+
1156+
private static boolean isControlOrWhitespaceAsciiChar(byte b) {
1157+
return ISO_CONTROL_OR_WHITESPACE[128 + b];
1158+
}
11301159
}

‎codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java‎

Lines changed: 227 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,118 @@ public void testResponseChunked() {
147147
assertNull(ch.readInbound());
148148
}
149149

150+
@Test
151+
public void testResponseChunkedWithValidUncommonPatterns() {
152+
EmbeddedChannel ch = new EmbeddedChannel(new HttpResponseDecoder());
153+
ch.writeInbound(Unpooled.copiedBuffer("HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n",
154+
CharsetUtil.US_ASCII));
155+
156+
HttpResponse res = ch.readInbound();
157+
assertThat(res.protocolVersion(), sameInstance(HttpVersion.HTTP_1_1));
158+
assertThat(res.status(), is(HttpResponseStatus.OK));
159+
160+
byte[] data = new byte[1];
161+
for (int i = 0; i < data.length; i++) {
162+
data[i] = (byte) i;
163+
}
164+
165+
// leading whitespace, trailing whitespace
166+
167+
assertFalse(ch.writeInbound(Unpooled.copiedBuffer(" " + Integer.toHexString(data.length) + " \r\n",
168+
CharsetUtil.US_ASCII)));
169+
assertTrue(ch.writeInbound(Unpooled.copiedBuffer(data)));
170+
HttpContent content = ch.readInbound();
171+
assertEquals(data.length, content.content().readableBytes());
172+
173+
byte[] decodedData = new byte[data.length];
174+
content.content().readBytes(decodedData);
175+
assertArrayEquals(data, decodedData);
176+
content.release();
177+
178+
assertFalse(ch.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));
179+
180+
// leading whitespace, trailing control char
181+
182+
assertFalse(ch.writeInbound(Unpooled.copiedBuffer(" " + Integer.toHexString(data.length) + "\0\r\n",
183+
CharsetUtil.US_ASCII)));
184+
assertTrue(ch.writeInbound(Unpooled.copiedBuffer(data)));
185+
content = ch.readInbound();
186+
assertEquals(data.length, content.content().readableBytes());
187+
188+
decodedData = new byte[data.length];
189+
content.content().readBytes(decodedData);
190+
assertArrayEquals(data, decodedData);
191+
content.release();
192+
193+
assertFalse(ch.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));
194+
195+
// leading whitespace, trailing semicolon
196+
197+
assertFalse(ch.writeInbound(Unpooled.copiedBuffer(" " + Integer.toHexString(data.length) + ";\r\n",
198+
CharsetUtil.US_ASCII)));
199+
assertTrue(ch.writeInbound(Unpooled.copiedBuffer(data)));
200+
content = ch.readInbound();
201+
assertEquals(data.length, content.content().readableBytes());
202+
203+
decodedData = new byte[data.length];
204+
content.content().readBytes(decodedData);
205+
assertArrayEquals(data, decodedData);
206+
content.release();
207+
208+
assertFalse(ch.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));
209+
210+
// Write the last chunk.
211+
ch.writeInbound(Unpooled.copiedBuffer("0\r\n\r\n", CharsetUtil.US_ASCII));
212+
213+
// Ensure the last chunk was decoded.
214+
LastHttpContent lastContent = ch.readInbound();
215+
assertFalse(lastContent.content().isReadable());
216+
lastContent.release();
217+
218+
ch.finish();
219+
assertNull(ch.readInbound());
220+
}
221+
222+
@Test
223+
public void testResponseChunkedWithControlChars() {
224+
EmbeddedChannel ch = new EmbeddedChannel(new HttpResponseDecoder());
225+
ch.writeInbound(Unpooled.copiedBuffer("HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n",
226+
CharsetUtil.US_ASCII));
227+
228+
HttpResponse res = ch.readInbound();
229+
assertThat(res.protocolVersion(), sameInstance(HttpVersion.HTTP_1_1));
230+
assertThat(res.status(), is(HttpResponseStatus.OK));
231+
232+
byte[] data = new byte[1];
233+
for (int i = 0; i < data.length; i++) {
234+
data[i] = (byte) i;
235+
}
236+
237+
assertFalse(ch.writeInbound(Unpooled.copiedBuffer(" " + Integer.toHexString(data.length) + " \r\n",
238+
CharsetUtil.US_ASCII)));
239+
assertTrue(ch.writeInbound(Unpooled.copiedBuffer(data)));
240+
HttpContent content = ch.readInbound();
241+
assertEquals(data.length, content.content().readableBytes());
242+
243+
byte[] decodedData = new byte[data.length];
244+
content.content().readBytes(decodedData);
245+
assertArrayEquals(data, decodedData);
246+
content.release();
247+
248+
assertFalse(ch.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));
249+
250+
// Write the last chunk.
251+
ch.writeInbound(Unpooled.copiedBuffer("0\r\n\r\n", CharsetUtil.US_ASCII));
252+
253+
// Ensure the last chunk was decoded.
254+
LastHttpContent lastContent = ch.readInbound();
255+
assertFalse(lastContent.content().isReadable());
256+
lastContent.release();
257+
258+
assertFalse(ch.finish());
259+
assertNull(ch.readInbound());
260+
}
261+
150262
@Test
151263
public void testResponseDisallowPartialChunks() {
152264
HttpResponseDecoder decoder = new HttpResponseDecoder(
@@ -741,6 +853,121 @@ public void testGarbageChunk() {
741853
assertThat(channel.finish(), is(false));
742854
}
743855

856+
@Test
857+
public void testWhiteSpaceGarbageChunk() {
858+
EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
859+
String responseWithIllegalChunk =
860+
"HTTP/1.1 200 OK\r\n" +
861+
"Transfer-Encoding: chunked\r\n\r\n" +
862+
" \r\n";
863+
864+
channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
865+
assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));
866+
867+
// Ensure that the decoder generates the last chunk with correct decoder result.
868+
LastHttpContent invalidChunk = channel.readInbound();
869+
assertThat(invalidChunk.decoderResult().isFailure(), is(true));
870+
invalidChunk.release();
871+
872+
// And no more messages should be produced by the decoder.
873+
assertThat(channel.readInbound(), is(nullValue()));
874+
875+
// .. even after the connection is closed.
876+
assertThat(channel.finish(), is(false));
877+
}
878+
879+
@Test
880+
public void testLeadingWhiteSpacesSemiColonGarbageChunk() {
881+
EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
882+
String responseWithIllegalChunk =
883+
"HTTP/1.1 200 OK\r\n" +
884+
"Transfer-Encoding: chunked\r\n\r\n" +
885+
" ;\r\n";
886+
887+
channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
888+
assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));
889+
890+
// Ensure that the decoder generates the last chunk with correct decoder result.
891+
LastHttpContent invalidChunk = channel.readInbound();
892+
assertThat(invalidChunk.decoderResult().isFailure(), is(true));
893+
invalidChunk.release();
894+
895+
// And no more messages should be produced by the decoder.
896+
assertThat(channel.readInbound(), is(nullValue()));
897+
898+
// .. even after the connection is closed.
899+
assertThat(channel.finish(), is(false));
900+
}
901+
902+
@Test
903+
public void testControlCharGarbageChunk() {
904+
EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
905+
String responseWithIllegalChunk =
906+
"HTTP/1.1 200 OK\r\n" +
907+
"Transfer-Encoding: chunked\r\n\r\n" +
908+
"\0\r\n";
909+
910+
channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
911+
assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));
912+
913+
// Ensure that the decoder generates the last chunk with correct decoder result.
914+
LastHttpContent invalidChunk = channel.readInbound();
915+
assertThat(invalidChunk.decoderResult().isFailure(), is(true));
916+
invalidChunk.release();
917+
918+
// And no more messages should be produced by the decoder.
919+
assertThat(channel.readInbound(), is(nullValue()));
920+
921+
// .. even after the connection is closed.
922+
assertThat(channel.finish(), is(false));
923+
}
924+
925+
@Test
926+
public void testLeadingWhiteSpacesControlCharGarbageChunk() {
927+
EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
928+
String responseWithIllegalChunk =
929+
"HTTP/1.1 200 OK\r\n" +
930+
"Transfer-Encoding: chunked\r\n\r\n" +
931+
" \0\r\n";
932+
933+
channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
934+
assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));
935+
936+
// Ensure that the decoder generates the last chunk with correct decoder result.
937+
LastHttpContent invalidChunk = channel.readInbound();
938+
assertThat(invalidChunk.decoderResult().isFailure(), is(true));
939+
invalidChunk.release();
940+
941+
// And no more messages should be produced by the decoder.
942+
assertThat(channel.readInbound(), is(nullValue()));
943+
944+
// .. even after the connection is closed.
945+
assertThat(channel.finish(), is(false));
946+
}
947+
948+
@Test
949+
public void testGarbageChunkAfterWhiteSpaces() {
950+
EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
951+
String responseWithIllegalChunk =
952+
"HTTP/1.1 200 OK\r\n" +
953+
"Transfer-Encoding: chunked\r\n\r\n" +
954+
" 12345N1 ;\r\n";
955+
956+
channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
957+
assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));
958+
959+
// Ensure that the decoder generates the last chunk with correct decoder result.
960+
LastHttpContent invalidChunk = channel.readInbound();
961+
assertThat(invalidChunk.decoderResult().isFailure(), is(true));
962+
invalidChunk.release();
963+
964+
// And no more messages should be produced by the decoder.
965+
assertThat(channel.readInbound(), is(nullValue()));
966+
967+
// .. even after the connection is closed.
968+
assertThat(channel.finish(), is(false));
969+
}
970+
744971
@Test
745972
public void testConnectionClosedBeforeHeadersReceived() {
746973
EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());

0 commit comments

Comments
 (0)