105 lines
5.1 KiB
Diff
105 lines
5.1 KiB
Diff
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(-)
|
|
|
|
--- a/src/main/java/org/jboss/netty/handler/codec/http/HttpMessageDecoder.java
|
|
+++ b/src/main/java/org/jboss/netty/handler/codec/http/HttpMessageDecoder.java
|
|
@@ -568,23 +568,9 @@
|
|
// Because this decoder did not call HttpMessage.setChunked(true)
|
|
// yet, HttpMessage.isChunked() should return true only when
|
|
// 'Transfer-Encoding' is 'chunked'.
|
|
- // 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.getProtocolVersion() == 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 (HttpHeaders.getContentLength(message, -1) >= 0) {
|
|
return State.READ_FIXED_LENGTH_CONTENT;
|
|
@@ -593,6 +579,31 @@
|
|
}
|
|
}
|
|
|
|
+ /**
|
|
+ * 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(HttpHeaders.Names.CONTENT_LENGTH);
|
|
+ }
|
|
+
|
|
private HttpChunkTrailer readTrailingHeaders(ChannelBuffer buffer) throws TooLongFrameException {
|
|
headerSize = 0;
|
|
String line = readHeader(buffer);
|
|
|