Skip to content

Commit

Permalink
Merge pull request from GHSA-wm47-8v5p-wjpj
Browse files Browse the repository at this point in the history 
Motivation:

As stated by https://tools.ietf.org/html/rfc7540#section-8.1.2.6 we should report a stream error if the content-length does not match the sum of all data frames.

Modifications:

- Verify that the sum of data frames match if a content-length header was send.
- Handle multiple content-length headers and also handle negative values
- Add io.netty.http2.validateContentLength system property which allows to disable the more strict validation
- Add unit tests

Result:

Correctly handle the case when the content-length header was included but not match what is send and also when content-length header is invalid
  • Loading branch information
@normanmaurer
normanmaurer committed Mar 9, 2021
1 parent bf9b90c commit 89c241e
Show file tree
Hide file tree
Showing 4 changed files with 312 additions and 50 deletions.
48 changes: 7 additions & 41 deletions codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java
View file
Expand Up @@ -16,7 +16,6 @@
package io.netty.handler.codec.http;

import static io.netty.util.internal.ObjectUtil.checkPositive;
import static io.netty.util.internal.StringUtil.COMMA;

import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
Expand Down Expand Up @@ -630,49 +629,16 @@ private State readHeaders(ByteBuf buffer) {
value = null;

List<String> contentLengthFields = headers.getAll(HttpHeaderNames.CONTENT_LENGTH);

if (!contentLengthFields.isEmpty()) {
HttpVersion version = message.protocolVersion();
boolean isHttp10OrEarlier = version.majorVersion() < 1 || (version.majorVersion() == 1
&& version.minorVersion() == 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.
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 = HttpUtil.normalizeAndGetContentLength(contentLengthFields,
isHttp10OrEarlier, allowDuplicateContentLengths);
if (contentLength != -1) {
headers.set(HttpHeaderNames.CONTENT_LENGTH, contentLength);
}
}

Expand Down
86 changes: 86 additions & 0 deletions codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java
View file
Expand Up @@ -24,10 +24,14 @@
import java.util.Iterator;
import java.util.List;

import io.netty.handler.codec.Headers;
import io.netty.util.AsciiString;
import io.netty.util.CharsetUtil;
import io.netty.util.NetUtil;
import io.netty.util.internal.ObjectUtil;
import io.netty.util.internal.UnstableApi;

import static io.netty.util.internal.StringUtil.COMMA;

/**
* Utility methods useful in the HTTP context.
Expand All @@ -36,6 +40,7 @@ public final class HttpUtil {

private static final AsciiString CHARSET_EQUALS = AsciiString.of(HttpHeaderValues.CHARSET + "=");
private static final AsciiString SEMICOLON = AsciiString.cached(";");
private static final String COMMA_STRING = String.valueOf(COMMA);

private HttpUtil() { }

Expand Down Expand Up @@ -530,4 +535,85 @@ public static String formatHostnameForHttp(InetSocketAddress addr) {
}
return hostString;
}

/**
* Validates, and optionally extracts the content length from headers. This method is not intended for
* general use, but is here to be shared between HTTP/1 and HTTP/2 parsing.
*
* @param contentLengthFields the content-length header fields.
* @param isHttp10OrEarlier {@code true} if we are handling HTTP/1.0 or earlier
* @param allowDuplicateContentLengths {@code true} if multiple, identical-value content lengths should be allowed.
* @return the normalized content length from the headers or {@code -1} if the fields were empty.
* @throws IllegalArgumentException if the content-length fields are not valid
*/
@UnstableApi
public static long normalizeAndGetContentLength(
List<? extends CharSequence> contentLengthFields, boolean isHttp10OrEarlier,
boolean allowDuplicateContentLengths) {
if (contentLengthFields.isEmpty()) {
return -1;
}

// 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.
String firstField = contentLengthFields.get(0).toString();
boolean multipleContentLengths =
contentLengthFields.size() > 1 || firstField.indexOf(COMMA) >= 0;

if (multipleContentLengths && !isHttp10OrEarlier) {
if (allowDuplicateContentLengths) {
// Find and enforce that all Content-Length values are the same
String firstValue = null;
for (CharSequence field : contentLengthFields) {
String[] tokens = field.toString().split(COMMA_STRING, -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
firstField = firstValue;
} else {
// Reject the message as invalid
throw new IllegalArgumentException(
"Multiple Content-Length values found: " + contentLengthFields);
}
}
// Ensure we not allow sign as part of the content-length:
// See https://github.com/squid-cache/squid/security/advisories/GHSA-qf3v-rc95-96j5
if (!Character.isDigit(firstField.charAt(0))) {
// Reject the message as invalid
throw new IllegalArgumentException(
"Content-Length value is not a number: " + firstField);
}
try {
final long value = Long.parseLong(firstField);
if (value < 0) {
// Reject the message as invalid
throw new IllegalArgumentException(
"Content-Length value must be >=0: " + value);
}
return value;
} catch (NumberFormatException e) {
// Reject the message as invalid
throw new IllegalArgumentException(
"Content-Length value is not a number: " + firstField, e);
}
}
}
100 changes: 91 additions & 9 deletions codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java
View file
Expand Up @@ -16,8 +16,11 @@

import io.netty.buffer.ByteBuf;
import io.netty.channel.ChannelHandlerContext;
import io.netty.handler.codec.http.HttpHeaderNames;
import io.netty.handler.codec.http.HttpStatusClass;
import io.netty.handler.codec.http.HttpUtil;
import io.netty.handler.codec.http2.Http2Connection.Endpoint;
import io.netty.util.internal.SystemPropertyUtil;
import io.netty.util.internal.UnstableApi;
import io.netty.util.internal.logging.InternalLogger;
import io.netty.util.internal.logging.InternalLoggerFactory;
Expand Down Expand Up @@ -49,6 +52,8 @@
*/
@UnstableApi
public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
private static final boolean VALIDATE_CONTENT_LENGTH =
SystemPropertyUtil.getBoolean("io.netty.http2.validateContentLength", true);
private static final InternalLogger logger = InternalLoggerFactory.getInstance(DefaultHttp2ConnectionDecoder.class);
private Http2FrameListener internalFrameListener = new PrefaceFrameListener();
private final Http2Connection connection;
Expand All @@ -59,6 +64,7 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
private final Http2PromisedRequestVerifier requestVerifier;
private final Http2SettingsReceivedConsumer settingsReceivedConsumer;
private final boolean autoAckPing;
private final Http2Connection.PropertyKey contentLengthKey;

public DefaultHttp2ConnectionDecoder(Http2Connection connection,
Http2ConnectionEncoder encoder,
Expand Down Expand Up @@ -125,6 +131,7 @@ public DefaultHttp2ConnectionDecoder(Http2Connection connection,
settingsReceivedConsumer = (Http2SettingsReceivedConsumer) encoder;
}
this.connection = checkNotNull(connection, "connection");
contentLengthKey = this.connection.newKey();
this.frameReader = checkNotNull(frameReader, "frameReader");
this.encoder = checkNotNull(encoder, "encoder");
this.requestVerifier = checkNotNull(requestVerifier, "requestVerifier");
Expand Down Expand Up @@ -223,6 +230,23 @@ void onUnknownFrame0(ChannelHandlerContext ctx, byte frameType, int streamId, Ht
listener.onUnknownFrame(ctx, frameType, streamId, flags, payload);
}

// See https://tools.ietf.org/html/rfc7540#section-8.1.2.6
private void verifyContentLength(Http2Stream stream, int data, boolean isEnd) throws Http2Exception {
if (!VALIDATE_CONTENT_LENGTH) {
return;
}
ContentLength contentLength = stream.getProperty(contentLengthKey);
if (contentLength != null) {
try {
contentLength.increaseReceivedBytes(connection.isServer(), stream.id(), data, isEnd);
} finally {
if (isEnd) {
stream.removeProperty(contentLengthKey);
}
}
}
}

/**
* Handles all inbound frames from the network.
*/
Expand All @@ -232,7 +256,8 @@ public int onDataRead(final ChannelHandlerContext ctx, int streamId, ByteBuf dat
boolean endOfStream) throws Http2Exception {
Http2Stream stream = connection.stream(streamId);
Http2LocalFlowController flowController = flowController();
int bytesToReturn = data.readableBytes() + padding;
int readable = data.readableBytes();
int bytesToReturn = readable + padding;

final boolean shouldIgnore;
try {
Expand All @@ -259,7 +284,6 @@ public int onDataRead(final ChannelHandlerContext ctx, int streamId, ByteBuf dat
// All bytes have been consumed.
return bytesToReturn;
}

Http2Exception error = null;
switch (stream.state()) {
case OPEN:
Expand Down Expand Up @@ -287,6 +311,8 @@ public int onDataRead(final ChannelHandlerContext ctx, int streamId, ByteBuf dat
throw error;
}

verifyContentLength(stream, readable, endOfStream);

// Call back the application and retrieve the number of bytes that have been
// immediately processed.
bytesToReturn = listener.onDataRead(ctx, streamId, data, padding, endOfStream);
Expand Down Expand Up @@ -367,14 +393,34 @@ public void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers
stream.state());
}

stream.headersReceived(isInformational);
encoder.flowController().updateDependencyTree(streamId, streamDependency, weight, exclusive);

listener.onHeadersRead(ctx, streamId, headers, streamDependency, weight, exclusive, padding, endOfStream);
if (!stream.isHeadersReceived()) {
// extract the content-length header
List<? extends CharSequence> contentLength = headers.getAll(HttpHeaderNames.CONTENT_LENGTH);
if (contentLength != null && !contentLength.isEmpty()) {
try {
long cLength = HttpUtil.normalizeAndGetContentLength(contentLength, false, true);
if (cLength != -1) {
headers.setLong(HttpHeaderNames.CONTENT_LENGTH, cLength);
stream.setProperty(contentLengthKey, new ContentLength(cLength));
}
} catch (IllegalArgumentException e) {
throw streamError(stream.id(), PROTOCOL_ERROR,
"Multiple content-length headers received", e);
}
}
}

// If the headers completes this stream, close it.
if (endOfStream) {
lifecycleManager.closeStreamRemote(stream, ctx.newSucceededFuture());
stream.headersReceived(isInformational);
try {
verifyContentLength(stream, 0, endOfStream);
encoder.flowController().updateDependencyTree(streamId, streamDependency, weight, exclusive);
listener.onHeadersRead(ctx, streamId, headers, streamDependency,
weight, exclusive, padding, endOfStream);
} finally {
// If the headers completes this stream, close it.
if (endOfStream) {
lifecycleManager.closeStreamRemote(stream, ctx.newSucceededFuture());
}
}
}

Expand Down Expand Up @@ -736,4 +782,40 @@ public void onUnknownFrame(ChannelHandlerContext ctx, byte frameType, int stream
onUnknownFrame0(ctx, frameType, streamId, flags, payload);
}
}

private static final class ContentLength {
private final long expected;
private long seen;

ContentLength(long expected) {
this.expected = expected;
}

void increaseReceivedBytes(boolean server, int streamId, int bytes, boolean isEnd) throws Http2Exception {
seen += bytes;
// Check for overflow
if (seen < 0) {
throw streamError(streamId, PROTOCOL_ERROR,
"Received amount of data did overflow and so not match content-length header %d", expected);
}
// Check if we received more data then what was advertised via the content-length header.
if (seen > expected) {
throw streamError(streamId, PROTOCOL_ERROR,
"Received amount of data %d does not match content-length header %d", seen, expected);
}

if (isEnd) {
if (seen == 0 && !server) {
// This may be a response to a HEAD request, let's just allow it.
return;
}

// Check that we really saw what was told via the content-length header.
if (expected > seen) {
throw streamError(streamId, PROTOCOL_ERROR,
"Received amount of data %d does not match content-length header %d", seen, expected);
}
}
}
}
}

0 comments on commit 89c241e

Please sign in to comment.