diff --git a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpObjectDecoder.java b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpObjectDecoder.java index 4e6d89e825..006debbd28 100644 --- a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpObjectDecoder.java +++ b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpObjectDecoder.java @@ -545,6 +545,12 @@ private void parseHeaderLine(HttpHeaders headers, ByteBuf buffer, final int lfIn // obs-fold = CRLF 1*( SP / HTAB ) // ; obsolete line folding // ; see Section 3.2.4 + // OWS = *( SP / HTAB ) + // ; optional whitespace + // https://tools.ietf.org/html/rfc7230#section-3.2.4 + // No whitespace is allowed between the header field-name and colon. In + // the past, differences in the handling of such whitespace have led to + // security vulnerabilities in request routing and response handling. final int nonControlIndex = lfIndex - 2; int headerStart = buffer.forEachByte(buffer.readerIndex(), nonControlIndex - buffer.readerIndex(), FIND_NON_LINEAR_WHITESPACE); @@ -558,15 +564,13 @@ private void parseHeaderLine(HttpHeaders headers, ByteBuf buffer, final int lfIn throw new IllegalArgumentException("unable to find end of header name"); } + if (buffer.getByte(headerEnd) != COLON_BYTE) { + throw new IllegalArgumentException("No whitespace is allowed between the header field-name and colon."); + } + int valueStart = headerEnd + 1; // We assume the allocator will not leak memory, and so we retain + slice to avoid copying data. CharSequence name = newAsciiString(newBufferFrom(buffer.retainedSlice(headerStart, headerEnd - headerStart))); - if (buffer.getByte(headerEnd) != COLON_BYTE) { - valueStart = buffer.forEachByte(headerEnd + 1, nonControlIndex - headerEnd, FIND_COLON) + 1; - if (valueStart < 0) { - throw new IllegalArgumentException("unable to find colon"); - } - } if (nonControlIndex < valueStart) { headers.add(name, emptyAsciiString()); } else { diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpRequestDecoderTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpRequestDecoderTest.java index 43efd06959..ddae2ab6eb 100644 --- a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpRequestDecoderTest.java +++ b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpRequestDecoderTest.java @@ -101,9 +101,9 @@ public void contentLengthNoTrailersHeaderWhiteSpace() { byte[] content = new byte[128]; ThreadLocalRandom.current().nextBytes(content); byte[] beforeContentBytes = ("GET /some/path?foo=bar&baz=yyy HTTP/1.1" + "\r\n" + - " Connection : keep-alive " + "\r\n" + - " User-Agent : unit-test " + "\r\n" + - " Content-Length : " + content.length + "\r\n" + "\r\n").getBytes(US_ASCII); + " Connection: keep-alive " + "\r\n" + + " User-Agent: unit-test " + "\r\n" + + " Content-Length: " + content.length + "\r\n" + "\r\n").getBytes(US_ASCII); assertTrue(channel.writeInbound(wrappedBuffer(beforeContentBytes))); assertTrue(channel.writeInbound(wrappedBuffer(content))); @@ -134,8 +134,8 @@ public void contentLengthNoTrailersHeaderMixedWhiteSpace() { byte[] content = new byte[128]; ThreadLocalRandom.current().nextBytes(content); byte[] beforeContentBytes = ("GET /some/path?foo=bar&baz=yyy HTTP/1.1" + "\r\n" + - "Connection :keep-alive" + "\r\n" + - " User-Agent :unit-test" + "\r\n" + + "Connection:keep-alive" + "\r\n" + + " User-Agent:unit-test" + "\r\n" + "Empty:" + "\r\n" + "EmptyWhitespace: " + "\r\n" + "SingleCharacterNoWhiteSpace: a" + "\r\n" + @@ -329,6 +329,21 @@ public void variableNoTrailersWithInvalidContent() { channel.writeInbound(wrappedBuffer(content)); } + @Test(expected = DecoderException.class) + public void testWhitespaceNotAllowedBetweenHeaderFieldNameAndColon() { + EmbeddedChannel channel = newEmbeddedChannel(); + try { + byte[] beforeContentBytes = ("GET /some/path HTTP/1.1\r\n" + + "Transfer-Encoding : chunked\r\n" + + "Host: servicetalk.io\r\n\r\n").getBytes(US_ASCII); + + assertTrue(channel.writeInbound(wrappedBuffer(beforeContentBytes))); + channel.readInbound(); + } finally { + channel.finishAndReleaseAll(); + } + } + @Test(expected = DecoderException.class) public void variableWithTrailers() { EmbeddedChannel channel = newEmbeddedChannel(); diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpResponseDecoderTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpResponseDecoderTest.java index 0c86c4e4bf..291dd66db4 100644 --- a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpResponseDecoderTest.java +++ b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpResponseDecoderTest.java @@ -108,9 +108,9 @@ public void contentLengthNoTrailersHeaderWhiteSpace() { byte[] content = new byte[128]; ThreadLocalRandom.current().nextBytes(content); byte[] beforeContentBytes = ("HTTP/1.1 200 OK" + "\r\n" + - " Connection : keep-alive " + "\r\n" + - " Server : unit-test " + "\r\n" + - " Content-Length : " + content.length + "\r\n" + "\r\n").getBytes(US_ASCII); + " Connection: keep-alive " + "\r\n" + + " Server: unit-test " + "\r\n" + + " Content-Length: " + content.length + "\r\n" + "\r\n").getBytes(US_ASCII); assertTrue(channel.writeInbound(wrappedBuffer(beforeContentBytes))); assertTrue(channel.writeInbound(wrappedBuffer(content))); @@ -141,8 +141,8 @@ public void contentLengthNoTrailersHeaderMixedWhiteSpace() { byte[] content = new byte[128]; ThreadLocalRandom.current().nextBytes(content); byte[] beforeContentBytes = ("HTTP/1.1 200 OK" + "\r\n" + - "Connection :keep-alive" + "\r\n" + - " Server :unit-test" + "\r\n" + + "Connection:keep-alive" + "\r\n" + + " Server:unit-test" + "\r\n" + "Empty:" + "\r\n" + "EmptyWhitespace: " + "\r\n" + "SingleCharacterNoWhiteSpace: a" + "\r\n" + @@ -353,6 +353,21 @@ public void variableNoTrailersNoContent() { assertFalse(channel.finishAndReleaseAll()); } + @Test(expected = DecoderException.class) + public void testWhitespaceNotAllowedBetweenHeaderFieldNameAndColon() { + EmbeddedChannel channel = newEmbeddedChannel(); + try { + byte[] beforeContentBytes = ("HTTP/1.1 200 OK\r\n" + + "Transfer-Encoding : chunked\r\n" + + "Host: servicetalk.io\r\n\r\n").getBytes(US_ASCII); + + assertTrue(channel.writeInbound(wrappedBuffer(beforeContentBytes))); + channel.readInbound(); + } finally { + channel.finishAndReleaseAll(); + } + } + @Test public void variableWithTrailers() { EmbeddedChannel channel = newEmbeddedChannel();