netty/CVE-2019-20445-2.patch

From 629034624626b722128e0fcc6b3ec9d406cb3706 Mon Sep 17 00:00:00 2001
From: Bennett Lynch <bennett.lynch@gmail.com>
Date: Mon, 10 Feb 2020 01:41:57 -0800
Subject: [PATCH] =?UTF-8?q?Remove=20"Content-Length"=20when=20decoding=20H?=
 =?UTF-8?q?TTP/1.1=20message=20with=20both=20"Tra=E2=80=A6=20(#10003)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Motivation

As part of a recent commit for issue
https://github.com/netty/netty/issues/9861 the HttpObjectDecoder was
changed to throw an IllegalArgumentException (and produce a failed
decoder result) when decoding a message with both "Transfer-Encoding:
chunked" and "Content-Length".

While it seems correct for Netty to try to sanitize these types of
messages, the spec explicitly mentions that the Content-Length header
should be *removed* in this scenario.

Both Nginx 1.15.9 and Tomcat 9.0.31 also opt to remove the header:
https://github.com/apache/tomcat/blob/b693d7c1981fa7f51e58bc8c8e72e3fe80b7b773/java/org/apache/coyote/http11/Http11Processor.java#L747-L755
https://github.com/nginx/nginx/blob/0ad4393e30c119d250415cb769e3d8bc8dce5186/src/http/ngx_http_request.c#L1946-L1953

Modifications

* Change the default behavior from throwing an IllegalArgumentException
to removing the "Content-Length" header
* Extract the behavior to a new protected method,
handleChunkedEncodingWithContentLength(), that can be overridden to
change this behavior (or capture metrics)

Result

Messages of this nature will now be successfully decoded and have their
"Content-Length" header removed, rather than creating invalid messages
(decoder result failures). Users will be allowed to override and
configure this behavior.
---
 .../handler/codec/http/HttpObjectDecoder.java | 42 ++++++++++++-------
 .../codec/http/HttpRequestDecoderTest.java    | 10 ++++-
 2 files changed, 36 insertions(+), 16 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 768bd3b26f5..04861043590 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
@@ -632,23 +632,9 @@ private State readHeaders(ByteBuf buffer) {
             HttpUtil.setTransferEncodingChunked(message, false);
             return State.SKIP_CONTROL_CHARS;
         } else if (HttpUtil.isTransferEncodingChunked(message)) {
-            // 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");
+                handleTransferEncodingChunkedWithContentLength(message);
             }
-
             return State.READ_CHUNK_SIZE;
         } else if (contentLength() >= 0) {
             return State.READ_FIXED_LENGTH_CONTENT;
@@ -657,6 +643,32 @@ private State readHeaders(ByteBuf buffer) {
         }
     }
 
+    /**
+     * Invoked when a message with both a "Transfer-Encoding: chunked" and a "Content-Length" header field is detected.
+     * The default behavior is to <i>remove</i> the Content-Length field, but this method could be overridden
+     * to change the behavior (to, e.g., throw an exception and produce an invalid message).
+     * <p>
+     * See: https://tools.ietf.org/html/rfc7230#section-3.3.3
+     * <pre>
+     *     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.
+     * </pre>
+     * Also see:
+     * https://github.com/apache/tomcat/blob/b693d7c1981fa7f51e58bc8c8e72e3fe80b7b773/
+     * java/org/apache/coyote/http11/Http11Processor.java#L747-L755
+     * https://github.com/nginx/nginx/blob/0ad4393e30c119d250415cb769e3d8bc8dce5186/
+     * src/http/ngx_http_request.c#L1946-L1953
+     */
+    protected void handleTransferEncodingChunkedWithContentLength(HttpMessage message) {
+        message.headers().remove(HttpHeaderNames.CONTENT_LENGTH);
+        contentLength = Long.MIN_VALUE;
+    }
+
     private long contentLength() {
         if (contentLength == Long.MIN_VALUE) {
             contentLength = HttpUtil.getContentLength(message, -1L);
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 2548af0e2af..f92a32d0ad9 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
@@ -408,7 +408,15 @@ public void testContentLengthHeaderAndChunked() {
                 "Content-Length: 5\r\n" +
                 "Transfer-Encoding: chunked\r\n\r\n" +
                 "0\r\n\r\n";
-        testInvalidHeaders0(requestStr);
+
+        EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestDecoder());
+        assertTrue(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII)));
+        HttpRequest request = channel.readInbound();
+        assertFalse(request.decoderResult().isFailure());
+        assertTrue(request.headers().contains("Transfer-Encoding", "chunked", false));
+        assertFalse(request.headers().contains("Content-Length"));
+        LastHttpContent c = channel.readInbound();
+        assertFalse(channel.finish());
     }
 
     private static void testInvalidHeaders0(String requestStr) {