From ac76a24ff6d3cd0f59a2833c2f83eb82184ecaeb Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 13 Dec 2019 08:53:19 +0100 Subject: [PATCH] Verify we do not receive multiple content-length headers or a content-length and transfer-encoding: chunked header when using HTTP/1.1 (#9865) Motivation: RFC7230 states that we should not accept multiple content-length headers and also should not accept a content-length header in combination with transfer-encoding: chunked Modifications: - Check for multiple content-length headers and if found mark message as invalid - Check if we found a content-length header and also a transfer-encoding: chunked and if so mark the message as invalid - Add unit test Result: Fixes https://github.com/netty/netty/issues/9861 --- .../handler/codec/http/HttpObjectDecoder.java | 50 +++++++++++++-- .../codec/http/HttpRequestDecoderTest.java | 64 ++++++++++++++++--- 2 files changed, 99 insertions(+), 15 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 01ae62bda0..cdd5e2316e 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 @@ -596,23 +596,61 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder { if (name != null) { headers.add(name, value); } + // reset name and value fields name = null; value = null; - State nextState; + List values = headers.getAll(HttpHeaderNames.CONTENT_LENGTH); + int contentLengthValuesCount = values.size(); + + if (contentLengthValuesCount > 0) { + // Guard against multiple Content-Length headers as stated in + // https://tools.ietf.org/html/rfc7230#section-3.3.2: + // + // If a message is received that has multiple Content-Length header + // fields with field-values consisting of the same decimal value, or a + // single Content-Length header field with a field value containing a + // list of identical decimal values (e.g., "Content-Length: 42, 42"), + // indicating that duplicate Content-Length header fields have been + // generated or combined by an upstream message processor, then the + // recipient MUST either reject the message as invalid or replace the + // duplicated field-values with a single valid Content-Length field + // containing that decimal value prior to determining the message body + // length or forwarding the message. + if (contentLengthValuesCount > 1 && message.protocolVersion() == HttpVersion.HTTP_1_1) { + throw new IllegalArgumentException("Multiple Content-Length headers found"); + } + contentLength = Long.parseLong(values.get(0)); + } if (isContentAlwaysEmpty(message)) { HttpUtil.setTransferEncodingChunked(message, false); - nextState = State.SKIP_CONTROL_CHARS; + return State.SKIP_CONTROL_CHARS; } else if (HttpUtil.isTransferEncodingChunked(message)) { - nextState = State.READ_CHUNK_SIZE; + // See https://tools.ietf.org/html/rfc7230#section-3.3.3 + // + // If a message is received with both a Transfer-Encoding and a + // Content-Length header field, the Transfer-Encoding overrides the + // Content-Length. Such a message might indicate an attempt to + // perform request smuggling (Section 9.5) or response splitting + // (Section 9.4) and ought to be handled as an error. A sender MUST + // remove the received Content-Length field prior to forwarding such + // a message downstream. + // + // This is also what http_parser does: + // https://github.com/nodejs/http-parser/blob/v2.9.2/http_parser.c#L1769 + if (contentLengthValuesCount > 0 && message.protocolVersion() == HttpVersion.HTTP_1_1) { + throw new IllegalArgumentException( + "Both 'Content-Length: " + contentLength + "' and 'Transfer-Encoding: chunked' found"); + } + + return State.READ_CHUNK_SIZE; } else if (contentLength() >= 0) { - nextState = State.READ_FIXED_LENGTH_CONTENT; + return State.READ_FIXED_LENGTH_CONTENT; } else { - nextState = State.READ_VARIABLE_LENGTH_CONTENT; + return State.READ_VARIABLE_LENGTH_CONTENT; } - return nextState; } private long contentLength() { 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 4117f521d8..94d704ef13 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 @@ -323,29 +323,75 @@ public class HttpRequestDecoderTest { @Test public void testWhitespace() { - EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestDecoder()); String requestStr = "GET /some/path HTTP/1.1\r\n" + "Transfer-Encoding : chunked\r\n" + "Host: netty.io\n\r\n"; - - assertTrue(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII))); - HttpRequest request = channel.readInbound(); - assertTrue(request.decoderResult().isFailure()); - assertTrue(request.decoderResult().cause() instanceof IllegalArgumentException); - assertFalse(channel.finish()); + testInvalidHeaders0(requestStr); } @Test public void testHeaderWithNoValueAndMissingColon() { - EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestDecoder()); String requestStr = "GET /some/path HTTP/1.1\r\n" + "Content-Length: 0\r\n" + "Host:\r\n" + "netty.io\r\n\r\n"; + testInvalidHeaders0(requestStr); + } + @Test + public void testMultipleContentLengthHeaders() { + String requestStr = "GET /some/path HTTP/1.1\r\n" + + "Content-Length: 1\r\n" + + "Content-Length: 0\r\n\r\n" + + "b"; + testInvalidHeaders0(requestStr); + } + + @Test + public void testMultipleContentLengthHeaders2() { + String requestStr = "GET /some/path HTTP/1.1\r\n" + + "Content-Length: 1\r\n" + + "Connection: close\r\n" + + "Content-Length: 0\r\n\r\n" + + "b"; + testInvalidHeaders0(requestStr); + } + + @Test + public void testContentLengthHeaderWithCommaValue() { + String requestStr = "GET /some/path HTTP/1.1\r\n" + + "Content-Length: 1,1\r\n\r\n" + + "b"; + testInvalidHeaders0(requestStr); + } + + @Test + public void testMultipleContentLengthHeadersWithFolding() { + String requestStr = "POST / HTTP/1.1\r\n" + + "Host: example.com\r\n" + + "Connection: close\r\n" + + "Content-Length: 5\r\n" + + "Content-Length:\r\n" + + "\t6\r\n\r\n" + + "123456"; + testInvalidHeaders0(requestStr); + } + + @Test + public void testContentLengthHeaderAndChunked() { + String requestStr = "POST / HTTP/1.1\r\n" + + "Host: example.com\r\n" + + "Connection: close\r\n" + + "Content-Length: 5\r\n" + + "Transfer-Encoding: chunked\r\n\r\n" + + "0\r\n\r\n"; + testInvalidHeaders0(requestStr); + } + + private static void testInvalidHeaders0(String requestStr) { + EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestDecoder()); assertTrue(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII))); HttpRequest request = channel.readInbound(); - System.err.println(request.headers().names().toString()); assertTrue(request.decoderResult().isFailure()); assertTrue(request.decoderResult().cause() instanceof IllegalArgumentException); assertFalse(channel.finish());