Verify we do not receive multiple content-length headers or a content-length and transfer-encoding: chunked header when using HTTP/1.1 (netty#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 netty#9861

Author: normanmaurer

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

@@ -600,23 +600,61 @@ private State readHeaders(ByteBuf buffer) {
         if (name != null) {
             headers.add(name, value);
         }
+
         // reset name and value fields
         name = null;
         value = null;
 
-        State nextState;
+        List<String> 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() {

codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java

@@ -323,29 +323,75 @@ public void testTooLargeHeaders() {
 
     @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());