Skip to content

Commit

Permalink
Chunked HTTP length decoding should account for whitespaces/ctrl chars (
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@franz1981 @normanmaurer
franz1981 and normanmaurer committed Mar 14, 2023
1 parent ab5c411 commit 0e8af54
Show file tree
Hide file tree
Showing 2 changed files with 258 additions and 2 deletions.
33 changes: 31 additions & 2 deletions codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java
View file
Expand Up @@ -799,16 +799,41 @@ private LastHttpContent readTrailingHeaders(ByteBuf buffer) {
protected abstract HttpMessage createMessage(String[] initialLine) throws Exception;
protected abstract HttpMessage createInvalidMessage();

/**
* It skips any whitespace char and return the number of skipped bytes.
*/
private static int skipWhiteSpaces(byte[] hex, int start, int length) {
for (int i = 0; i < length; i++) {
if (!isWhitespace(hex[start + i])) {
return i;
}
}
return length;
}

private static int getChunkSize(byte[] hex, int start, int length) {
// byte[] is produced by LineParse::parseLine that already skip ISO CTRL and Whitespace chars
// trim the leading bytes if white spaces, if any
final int skipped = skipWhiteSpaces(hex, start, length);
if (skipped == length) {
// empty case
throw new NumberFormatException();
}
start += skipped;
length -= skipped;
int result = 0;
for (int i = 0; i < length; i++) {
final int digit = StringUtil.decodeHexNibble(hex[start + i]);
if (digit == -1) {
// uncommon path
if (hex[start + i] == ';') {
final byte b = hex[start + i];
if (b == ';' || isControlOrWhitespaceAsciiChar(b)) {
if (i == 0) {
// empty case
throw new NumberFormatException();
}
return result;
}
// non-hex char fail-fast path
throw new NumberFormatException();
}
result *= 16;
Expand Down Expand Up @@ -1127,4 +1152,8 @@ public boolean process(byte value) {
return ISO_CONTROL_OR_WHITESPACE[128 + value];
}
};

private static boolean isControlOrWhitespaceAsciiChar(byte b) {
return ISO_CONTROL_OR_WHITESPACE[128 + b];
}
}
227 changes: 227 additions & 0 deletions codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java
View file
Expand Up @@ -147,6 +147,118 @@ public void testResponseChunked() {
assertNull(ch.readInbound());
}

@Test
public void testResponseChunkedWithValidUncommonPatterns() {
EmbeddedChannel ch = new EmbeddedChannel(new HttpResponseDecoder());
ch.writeInbound(Unpooled.copiedBuffer("HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n",
CharsetUtil.US_ASCII));

HttpResponse res = ch.readInbound();
assertThat(res.protocolVersion(), sameInstance(HttpVersion.HTTP_1_1));
assertThat(res.status(), is(HttpResponseStatus.OK));

byte[] data = new byte[1];
for (int i = 0; i < data.length; i++) {
data[i] = (byte) i;
}

// leading whitespace, trailing whitespace

assertFalse(ch.writeInbound(Unpooled.copiedBuffer(" " + Integer.toHexString(data.length) + " \r\n",
CharsetUtil.US_ASCII)));
assertTrue(ch.writeInbound(Unpooled.copiedBuffer(data)));
HttpContent content = ch.readInbound();
assertEquals(data.length, content.content().readableBytes());

byte[] decodedData = new byte[data.length];
content.content().readBytes(decodedData);
assertArrayEquals(data, decodedData);
content.release();

assertFalse(ch.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));

// leading whitespace, trailing control char

assertFalse(ch.writeInbound(Unpooled.copiedBuffer(" " + Integer.toHexString(data.length) + "\0\r\n",
CharsetUtil.US_ASCII)));
assertTrue(ch.writeInbound(Unpooled.copiedBuffer(data)));
content = ch.readInbound();
assertEquals(data.length, content.content().readableBytes());

decodedData = new byte[data.length];
content.content().readBytes(decodedData);
assertArrayEquals(data, decodedData);
content.release();

assertFalse(ch.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));

// leading whitespace, trailing semicolon

assertFalse(ch.writeInbound(Unpooled.copiedBuffer(" " + Integer.toHexString(data.length) + ";\r\n",
CharsetUtil.US_ASCII)));
assertTrue(ch.writeInbound(Unpooled.copiedBuffer(data)));
content = ch.readInbound();
assertEquals(data.length, content.content().readableBytes());

decodedData = new byte[data.length];
content.content().readBytes(decodedData);
assertArrayEquals(data, decodedData);
content.release();

assertFalse(ch.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));

// Write the last chunk.
ch.writeInbound(Unpooled.copiedBuffer("0\r\n\r\n", CharsetUtil.US_ASCII));

// Ensure the last chunk was decoded.
LastHttpContent lastContent = ch.readInbound();
assertFalse(lastContent.content().isReadable());
lastContent.release();

ch.finish();
assertNull(ch.readInbound());
}

@Test
public void testResponseChunkedWithControlChars() {
EmbeddedChannel ch = new EmbeddedChannel(new HttpResponseDecoder());
ch.writeInbound(Unpooled.copiedBuffer("HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n",
CharsetUtil.US_ASCII));

HttpResponse res = ch.readInbound();
assertThat(res.protocolVersion(), sameInstance(HttpVersion.HTTP_1_1));
assertThat(res.status(), is(HttpResponseStatus.OK));

byte[] data = new byte[1];
for (int i = 0; i < data.length; i++) {
data[i] = (byte) i;
}

assertFalse(ch.writeInbound(Unpooled.copiedBuffer(" " + Integer.toHexString(data.length) + " \r\n",
CharsetUtil.US_ASCII)));
assertTrue(ch.writeInbound(Unpooled.copiedBuffer(data)));
HttpContent content = ch.readInbound();
assertEquals(data.length, content.content().readableBytes());

byte[] decodedData = new byte[data.length];
content.content().readBytes(decodedData);
assertArrayEquals(data, decodedData);
content.release();

assertFalse(ch.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));

// Write the last chunk.
ch.writeInbound(Unpooled.copiedBuffer("0\r\n\r\n", CharsetUtil.US_ASCII));

// Ensure the last chunk was decoded.
LastHttpContent lastContent = ch.readInbound();
assertFalse(lastContent.content().isReadable());
lastContent.release();

assertFalse(ch.finish());
assertNull(ch.readInbound());
}

@Test
public void testResponseDisallowPartialChunks() {
HttpResponseDecoder decoder = new HttpResponseDecoder(
Expand Down Expand Up @@ -741,6 +853,121 @@ public void testGarbageChunk() {
assertThat(channel.finish(), is(false));
}

@Test
public void testWhiteSpaceGarbageChunk() {
EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
String responseWithIllegalChunk =
"HTTP/1.1 200 OK\r\n" +
"Transfer-Encoding: chunked\r\n\r\n" +
" \r\n";

channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));

// Ensure that the decoder generates the last chunk with correct decoder result.
LastHttpContent invalidChunk = channel.readInbound();
assertThat(invalidChunk.decoderResult().isFailure(), is(true));
invalidChunk.release();

// And no more messages should be produced by the decoder.
assertThat(channel.readInbound(), is(nullValue()));

// .. even after the connection is closed.
assertThat(channel.finish(), is(false));
}

@Test
public void testLeadingWhiteSpacesSemiColonGarbageChunk() {
EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
String responseWithIllegalChunk =
"HTTP/1.1 200 OK\r\n" +
"Transfer-Encoding: chunked\r\n\r\n" +
" ;\r\n";

channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));

// Ensure that the decoder generates the last chunk with correct decoder result.
LastHttpContent invalidChunk = channel.readInbound();
assertThat(invalidChunk.decoderResult().isFailure(), is(true));
invalidChunk.release();

// And no more messages should be produced by the decoder.
assertThat(channel.readInbound(), is(nullValue()));

// .. even after the connection is closed.
assertThat(channel.finish(), is(false));
}

@Test
public void testControlCharGarbageChunk() {
EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
String responseWithIllegalChunk =
"HTTP/1.1 200 OK\r\n" +
"Transfer-Encoding: chunked\r\n\r\n" +
"\0\r\n";

channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));

// Ensure that the decoder generates the last chunk with correct decoder result.
LastHttpContent invalidChunk = channel.readInbound();
assertThat(invalidChunk.decoderResult().isFailure(), is(true));
invalidChunk.release();

// And no more messages should be produced by the decoder.
assertThat(channel.readInbound(), is(nullValue()));

// .. even after the connection is closed.
assertThat(channel.finish(), is(false));
}

@Test
public void testLeadingWhiteSpacesControlCharGarbageChunk() {
EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
String responseWithIllegalChunk =
"HTTP/1.1 200 OK\r\n" +
"Transfer-Encoding: chunked\r\n\r\n" +
" \0\r\n";

channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));

// Ensure that the decoder generates the last chunk with correct decoder result.
LastHttpContent invalidChunk = channel.readInbound();
assertThat(invalidChunk.decoderResult().isFailure(), is(true));
invalidChunk.release();

// And no more messages should be produced by the decoder.
assertThat(channel.readInbound(), is(nullValue()));

// .. even after the connection is closed.
assertThat(channel.finish(), is(false));
}

@Test
public void testGarbageChunkAfterWhiteSpaces() {
EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
String responseWithIllegalChunk =
"HTTP/1.1 200 OK\r\n" +
"Transfer-Encoding: chunked\r\n\r\n" +
" 12345N1 ;\r\n";

channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));

// Ensure that the decoder generates the last chunk with correct decoder result.
LastHttpContent invalidChunk = channel.readInbound();
assertThat(invalidChunk.decoderResult().isFailure(), is(true));
invalidChunk.release();

// And no more messages should be produced by the decoder.
assertThat(channel.readInbound(), is(nullValue()));

// .. even after the connection is closed.
assertThat(channel.finish(), is(false));
}

@Test
public void testConnectionClosedBeforeHeadersReceived() {
EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
Expand Down

0 comments on commit 0e8af54

Please sign in to comment.