From f7fa9fce72dd423200af1131adf6f089b311128d Mon Sep 17 00:00:00 2001 From: Bennett Lynch Date: Mon, 6 Jul 2020 01:25:13 -0700 Subject: [PATCH] Add option to HttpObjectDecoder to allow duplicate Content-Lengths (#10349) Motivation: Since https://github.com/netty/netty/pull/9865 (Netty 4.1.44) the default behavior of the HttpObjectDecoder has been to reject any HTTP message that is found to have multiple Content-Length headers when decoding. This behavior is well-justified as per the risks outlined in https://github.com/netty/netty/issues/9861, however, we can see from the cited RFC section that there are multiple possible options offered for responding to this scenario: > 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. https://tools.ietf.org/html/rfc7230#section-3.3.2 Netty opted for the first option (rejecting as invalid), which seems like the safest, but the second option (replacing duplicate values with a single value) is also valid behavior. Modifications: * Introduce "allowDuplicateContentLengths" parameter to HttpObjectDecoder (defaulting to false). * When set to true, will allow multiple Content-Length headers only if they are all the same value. The duplicated field-values will be replaced with a single valid Content-Length field. * Add new parameterized test class for testing different variations of multiple Content-Length headers. Result: This is a backwards-compatible change with no functional change to the existing behavior. Note that the existing logic would result in NumberFormatExceptions for header values like "Content-Length: 42, 42". The new logic correctly reports these as IllegalArgumentException with the proper error message. Additionally note that this behavior is only applied to HTTP/1.1, but I suspect that we may want to expand that to include HTTP/1.0 as well... That behavior is not modified here to minimize the scope of this change. --- .../handler/codec/http/HttpClientCodec.java | 40 +++-- .../handler/codec/http/HttpObjectDecoder.java | 77 ++++++++-- .../codec/http/HttpRequestDecoder.java | 7 + .../codec/http/HttpResponseDecoder.java | 7 + .../handler/codec/http/HttpServerCodec.java | 16 ++ .../MultipleContentLengthHeadersTest.java | 142 ++++++++++++++++++ 6 files changed, 265 insertions(+), 24 deletions(-) create mode 100644 codec-http/src/test/java/io/netty/handler/codec/http/MultipleContentLengthHeadersTest.java diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java index ee7ebe06e6..a104eba006 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java @@ -28,8 +28,10 @@ import java.util.List; import java.util.Queue; import java.util.concurrent.atomic.AtomicLong; +import static io.netty.handler.codec.http.HttpObjectDecoder.DEFAULT_ALLOW_DUPLICATE_CONTENT_LENGTHS; import static io.netty.handler.codec.http.HttpObjectDecoder.DEFAULT_MAX_HEADER_SIZE; import static io.netty.handler.codec.http.HttpObjectDecoder.DEFAULT_MAX_INITIAL_LINE_LENGTH; +import static io.netty.handler.codec.http.HttpObjectDecoder.DEFAULT_VALIDATE_HEADERS; /** * A combination of {@link HttpRequestEncoder} and {@link HttpResponseDecoder} @@ -47,6 +49,8 @@ import static io.netty.handler.codec.http.HttpObjectDecoder.DEFAULT_MAX_INITIAL_ */ public final class HttpClientCodec extends CombinedChannelDuplexHandler implements HttpClientUpgradeHandler.SourceCodec { + public static final boolean DEFAULT_FAIL_ON_MISSING_RESPONSE = false; + public static final boolean DEFAULT_PARSE_HTTP_AFTER_CONNECT_REQUEST = false; /** A queue that is used for correlating a request and a response. */ private final Queue queue = new ArrayDeque<>(); @@ -64,14 +68,14 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandlerParameters that prevents excessive memory consumption * * - * + * * * * + * * * * + * * * + *
NameMeaningNameDefault valueMeaning
{@code maxInitialLineLength}{@value #DEFAULT_MAX_INITIAL_LINE_LENGTH}The maximum length of the initial line * (e.g. {@code "GET / HTTP/1.0"} or {@code "HTTP/1.0 200 OK"}) * If the length of the initial line exceeds this value, a @@ -48,15 +51,24 @@ import java.util.List; *
{@code maxHeaderSize}{@value #DEFAULT_MAX_HEADER_SIZE}The maximum length of all headers. If the sum of the length of each * header exceeds this value, a {@link TooLongFrameException} will be raised.
+ * + *

Parameters that control parsing behavior

+ * * - * - * + * + * + * + * + * + * * *
{@code maxChunkSize}The maximum length of the content or each chunk. If the content length - * (or the length of each chunk) exceeds this value, the content or chunk - * will be split into multiple {@link HttpContent}s whose length is - * {@code maxChunkSize} at maximum.NameDefault valueMeaning
{@code allowDuplicateContentLengths}{@value #DEFAULT_ALLOW_DUPLICATE_CONTENT_LENGTHS}When set to {@code false}, will reject any messages that contain multiple Content-Length header fields. + * When set to {@code true}, will allow multiple Content-Length headers only if they are all the same decimal value. + * The duplicated field-values will be replaced with a single valid Content-Length field. + * See RFC 7230, Section 3.3.2.
* @@ -107,11 +119,14 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder { public static final boolean DEFAULT_CHUNKED_SUPPORTED = true; public static final boolean DEFAULT_VALIDATE_HEADERS = true; public static final int DEFAULT_INITIAL_BUFFER_SIZE = 128; + public static final boolean DEFAULT_ALLOW_DUPLICATE_CONTENT_LENGTHS = false; private static final String EMPTY_VALUE = ""; + private static final Pattern COMMA_PATTERN = Pattern.compile(","); private final boolean chunkedSupported; protected final boolean validateHeaders; + private final boolean allowDuplicateContentLengths; private final HeaderParser headerParser; private final LineParser lineParser; @@ -172,9 +187,20 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder { this(maxInitialLineLength, maxHeaderSize, chunkedSupported, validateHeaders, DEFAULT_INITIAL_BUFFER_SIZE); } + /** + * Creates a new instance with the specified parameters. + */ protected HttpObjectDecoder( int maxInitialLineLength, int maxHeaderSize, boolean chunkedSupported, boolean validateHeaders, int initialBufferSize) { + this(maxInitialLineLength, maxHeaderSize, chunkedSupported, validateHeaders, initialBufferSize, + DEFAULT_ALLOW_DUPLICATE_CONTENT_LENGTHS); + } + + protected HttpObjectDecoder( + int maxInitialLineLength, int maxHeaderSize, + boolean chunkedSupported, boolean validateHeaders, int initialBufferSize, + boolean allowDuplicateContentLengths) { checkPositive(maxInitialLineLength, "maxInitialLineLength"); checkPositive(maxHeaderSize, "maxHeaderSize"); AppendableCharSequence seq = new AppendableCharSequence(initialBufferSize); @@ -182,6 +208,7 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder { headerParser = new HeaderParser(seq, maxHeaderSize); this.chunkedSupported = chunkedSupported; this.validateHeaders = validateHeaders; + this.allowDuplicateContentLengths = allowDuplicateContentLengths; } @Override @@ -587,10 +614,9 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder { name = null; value = null; - List values = headers.getAll(HttpHeaderNames.CONTENT_LENGTH); - int contentLengthValuesCount = values.size(); + List contentLengthFields = headers.getAll(HttpHeaderNames.CONTENT_LENGTH); - if (contentLengthValuesCount > 0) { + if (!contentLengthFields.isEmpty()) { // Guard against multiple Content-Length headers as stated in // https://tools.ietf.org/html/rfc7230#section-3.3.2: // @@ -604,17 +630,42 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder { // 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"); + boolean multipleContentLengths = + contentLengthFields.size() > 1 || contentLengthFields.get(0).indexOf(COMMA) >= 0; + if (multipleContentLengths && message.protocolVersion() == HttpVersion.HTTP_1_1) { + if (allowDuplicateContentLengths) { + // Find and enforce that all Content-Length values are the same + String firstValue = null; + for (String field : contentLengthFields) { + String[] tokens = COMMA_PATTERN.split(field, -1); + for (String token : tokens) { + String trimmed = token.trim(); + if (firstValue == null) { + firstValue = trimmed; + } else if (!trimmed.equals(firstValue)) { + throw new IllegalArgumentException( + "Multiple Content-Length values found: " + contentLengthFields); + } + } + } + // Replace the duplicated field-values with a single valid Content-Length field + headers.set(HttpHeaderNames.CONTENT_LENGTH, firstValue); + contentLength = Long.parseLong(firstValue); + } else { + // Reject the message as invalid + throw new IllegalArgumentException( + "Multiple Content-Length values found: " + contentLengthFields); + } + } else { + contentLength = Long.parseLong(contentLengthFields.get(0)); } - contentLength = Long.parseLong(values.get(0)); } if (isContentAlwaysEmpty(message)) { HttpUtil.setTransferEncodingChunked(message, false); return State.SKIP_CONTROL_CHARS; } else if (HttpUtil.isTransferEncodingChunked(message)) { - if (contentLengthValuesCount > 0 && message.protocolVersion() == HttpVersion.HTTP_1_1) { + if (!contentLengthFields.isEmpty() && message.protocolVersion() == HttpVersion.HTTP_1_1) { handleTransferEncodingChunkedWithContentLength(message); } return State.READ_CHUNK_SIZE; diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java index 0dffb9105c..37b0ca31af 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java @@ -81,6 +81,13 @@ public class HttpRequestDecoder extends HttpObjectDecoder { super(maxInitialLineLength, maxHeaderSize, DEFAULT_CHUNKED_SUPPORTED, validateHeaders, initialBufferSize); } + public HttpRequestDecoder( + int maxInitialLineLength, int maxHeaderSize, boolean validateHeaders, + int initialBufferSize, boolean allowDuplicateContentLengths) { + super(maxInitialLineLength, maxHeaderSize, DEFAULT_CHUNKED_SUPPORTED, validateHeaders, + initialBufferSize, allowDuplicateContentLengths); + } + @Override protected HttpMessage createMessage(String[] initialLine) throws Exception { return new DefaultHttpRequest( diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java index 76aad841b6..ad033d4486 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java @@ -112,6 +112,13 @@ public class HttpResponseDecoder extends HttpObjectDecoder { super(maxInitialLineLength, maxHeaderSize, DEFAULT_CHUNKED_SUPPORTED, validateHeaders, initialBufferSize); } + public HttpResponseDecoder( + int maxInitialLineLength, int maxHeaderSize, boolean validateHeaders, + int initialBufferSize, boolean allowDuplicateContentLengths) { + super(maxInitialLineLength, maxHeaderSize, DEFAULT_CHUNKED_SUPPORTED, validateHeaders, + initialBufferSize, allowDuplicateContentLengths); + } + @Override protected HttpMessage createMessage(String[] initialLine) { return new DefaultHttpResponse( diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerCodec.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerCodec.java index 7a977b50d7..74c7ef7989 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerCodec.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerCodec.java @@ -73,6 +73,16 @@ public final class HttpServerCodec extends CombinedChannelDuplexHandler parameters() { + return Arrays.asList(new Object[][] { + { false, false, false }, + { false, false, true }, + { false, true, false }, + { false, true, true }, + { true, false, false }, + { true, false, true }, + { true, true, false }, + { true, true, true } + }); + } + + public MultipleContentLengthHeadersTest( + boolean allowDuplicateContentLengths, boolean sameValue, boolean singleField) { + this.allowDuplicateContentLengths = allowDuplicateContentLengths; + this.sameValue = sameValue; + this.singleField = singleField; + } + + @Before + public void setUp() { + HttpRequestDecoder decoder = new HttpRequestDecoder( + DEFAULT_MAX_INITIAL_LINE_LENGTH, + DEFAULT_MAX_HEADER_SIZE, + DEFAULT_VALIDATE_HEADERS, + DEFAULT_INITIAL_BUFFER_SIZE, + allowDuplicateContentLengths); + channel = new EmbeddedChannel(decoder); + } + + @Test + public void testMultipleContentLengthHeadersBehavior() { + String requestStr = setupRequestString(); + assertThat(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII)), is(true)); + HttpRequest request = channel.readInbound(); + + if (allowDuplicateContentLengths) { + if (sameValue) { + assertValid(request); + List contentLengths = request.headers().getAll(HttpHeaderNames.CONTENT_LENGTH); + assertThat(contentLengths, contains("1")); + LastHttpContent body = channel.readInbound(); + assertThat(body.content().readableBytes(), is(1)); + assertThat(body.content().readCharSequence(1, CharsetUtil.US_ASCII).toString(), is("a")); + } else { + assertInvalid(request); + } + } else { + assertInvalid(request); + } + assertThat(channel.finish(), is(false)); + } + + private String setupRequestString() { + String firstValue = "1"; + String secondValue = sameValue ? firstValue : "2"; + String contentLength; + if (singleField) { + contentLength = "Content-Length: " + firstValue + ", " + secondValue + "\r\n\r\n"; + } else { + contentLength = "Content-Length: " + firstValue + "\r\n" + + "Content-Length: " + secondValue + "\r\n\r\n"; + } + return "PUT /some/path HTTP/1.1\r\n" + + contentLength + + "ab"; + } + + @Test + public void testDanglingComma() { + String requestStr = "GET /some/path HTTP/1.1\r\n" + + "Content-Length: 1,\r\n" + + "Connection: close\n\n" + + "ab"; + assertThat(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII)), is(true)); + HttpRequest request = channel.readInbound(); + assertInvalid(request); + assertThat(channel.finish(), is(false)); + } + + private static void assertValid(HttpRequest request) { + assertThat(request.decoderResult().isFailure(), is(false)); + } + + private static void assertInvalid(HttpRequest request) { + assertThat(request.decoderResult().isFailure(), is(true)); + assertThat(request.decoderResult().cause(), instanceOf(IllegalArgumentException.class)); + assertThat(request.decoderResult().cause().getMessage(), + containsString("Multiple Content-Length values found")); + } +}