Skip to content

Commit 89c241e

Browse files
authored
Merge pull request from GHSA-wm47-8v5p-wjpj
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
1 parent bf9b90c commit 89c241e

File tree

4 files changed

+312
-50
lines changed

4 files changed

+312
-50
lines changed

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

+7-41
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
package io.netty.handler.codec.http;
1717

1818
import static io.netty.util.internal.ObjectUtil.checkPositive;
19-
import static io.netty.util.internal.StringUtil.COMMA;
2019

2120
import io.netty.buffer.ByteBuf;
2221
import io.netty.buffer.Unpooled;
@@ -630,49 +629,16 @@ private State readHeaders(ByteBuf buffer) {
630629
value = null;
631630

632631
List<String> contentLengthFields = headers.getAll(HttpHeaderNames.CONTENT_LENGTH);
633-
634632
if (!contentLengthFields.isEmpty()) {
633+
HttpVersion version = message.protocolVersion();
634+
boolean isHttp10OrEarlier = version.majorVersion() < 1 || (version.majorVersion() == 1
635+
&& version.minorVersion() == 0);
635636
// Guard against multiple Content-Length headers as stated in
636637
// https://tools.ietf.org/html/rfc7230#section-3.3.2:
637-
//
638-
// If a message is received that has multiple Content-Length header
639-
// fields with field-values consisting of the same decimal value, or a
640-
// single Content-Length header field with a field value containing a
641-
// list of identical decimal values (e.g., "Content-Length: 42, 42"),
642-
// indicating that duplicate Content-Length header fields have been
643-
// generated or combined by an upstream message processor, then the
644-
// recipient MUST either reject the message as invalid or replace the
645-
// duplicated field-values with a single valid Content-Length field
646-
// containing that decimal value prior to determining the message body
647-
// length or forwarding the message.
648-
boolean multipleContentLengths =
649-
contentLengthFields.size() > 1 || contentLengthFields.get(0).indexOf(COMMA) >= 0;
650-
if (multipleContentLengths && message.protocolVersion() == HttpVersion.HTTP_1_1) {
651-
if (allowDuplicateContentLengths) {
652-
// Find and enforce that all Content-Length values are the same
653-
String firstValue = null;
654-
for (String field : contentLengthFields) {
655-
String[] tokens = COMMA_PATTERN.split(field, -1);
656-
for (String token : tokens) {
657-
String trimmed = token.trim();
658-
if (firstValue == null) {
659-
firstValue = trimmed;
660-
} else if (!trimmed.equals(firstValue)) {
661-
throw new IllegalArgumentException(
662-
"Multiple Content-Length values found: " + contentLengthFields);
663-
}
664-
}
665-
}
666-
// Replace the duplicated field-values with a single valid Content-Length field
667-
headers.set(HttpHeaderNames.CONTENT_LENGTH, firstValue);
668-
contentLength = Long.parseLong(firstValue);
669-
} else {
670-
// Reject the message as invalid
671-
throw new IllegalArgumentException(
672-
"Multiple Content-Length values found: " + contentLengthFields);
673-
}
674-
} else {
675-
contentLength = Long.parseLong(contentLengthFields.get(0));
638+
contentLength = HttpUtil.normalizeAndGetContentLength(contentLengthFields,
639+
isHttp10OrEarlier, allowDuplicateContentLengths);
640+
if (contentLength != -1) {
641+
headers.set(HttpHeaderNames.CONTENT_LENGTH, contentLength);
676642
}
677643
}
678644

Diff for: codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java

+86
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,14 @@
2424
import java.util.Iterator;
2525
import java.util.List;
2626

27+
import io.netty.handler.codec.Headers;
2728
import io.netty.util.AsciiString;
2829
import io.netty.util.CharsetUtil;
2930
import io.netty.util.NetUtil;
3031
import io.netty.util.internal.ObjectUtil;
32+
import io.netty.util.internal.UnstableApi;
33+
34+
import static io.netty.util.internal.StringUtil.COMMA;
3135

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

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

4045
private HttpUtil() { }
4146

@@ -530,4 +535,85 @@ public static String formatHostnameForHttp(InetSocketAddress addr) {
530535
}
531536
return hostString;
532537
}
538+
539+
/**
540+
* Validates, and optionally extracts the content length from headers. This method is not intended for
541+
* general use, but is here to be shared between HTTP/1 and HTTP/2 parsing.
542+
*
543+
* @param contentLengthFields the content-length header fields.
544+
* @param isHttp10OrEarlier {@code true} if we are handling HTTP/1.0 or earlier
545+
* @param allowDuplicateContentLengths {@code true} if multiple, identical-value content lengths should be allowed.
546+
* @return the normalized content length from the headers or {@code -1} if the fields were empty.
547+
* @throws IllegalArgumentException if the content-length fields are not valid
548+
*/
549+
@UnstableApi
550+
public static long normalizeAndGetContentLength(
551+
List<? extends CharSequence> contentLengthFields, boolean isHttp10OrEarlier,
552+
boolean allowDuplicateContentLengths) {
553+
if (contentLengthFields.isEmpty()) {
554+
return -1;
555+
}
556+
557+
// Guard against multiple Content-Length headers as stated in
558+
// https://tools.ietf.org/html/rfc7230#section-3.3.2:
559+
//
560+
// If a message is received that has multiple Content-Length header
561+
// fields with field-values consisting of the same decimal value, or a
562+
// single Content-Length header field with a field value containing a
563+
// list of identical decimal values (e.g., "Content-Length: 42, 42"),
564+
// indicating that duplicate Content-Length header fields have been
565+
// generated or combined by an upstream message processor, then the
566+
// recipient MUST either reject the message as invalid or replace the
567+
// duplicated field-values with a single valid Content-Length field
568+
// containing that decimal value prior to determining the message body
569+
// length or forwarding the message.
570+
String firstField = contentLengthFields.get(0).toString();
571+
boolean multipleContentLengths =
572+
contentLengthFields.size() > 1 || firstField.indexOf(COMMA) >= 0;
573+
574+
if (multipleContentLengths && !isHttp10OrEarlier) {
575+
if (allowDuplicateContentLengths) {
576+
// Find and enforce that all Content-Length values are the same
577+
String firstValue = null;
578+
for (CharSequence field : contentLengthFields) {
579+
String[] tokens = field.toString().split(COMMA_STRING, -1);
580+
for (String token : tokens) {
581+
String trimmed = token.trim();
582+
if (firstValue == null) {
583+
firstValue = trimmed;
584+
} else if (!trimmed.equals(firstValue)) {
585+
throw new IllegalArgumentException(
586+
"Multiple Content-Length values found: " + contentLengthFields);
587+
}
588+
}
589+
}
590+
// Replace the duplicated field-values with a single valid Content-Length field
591+
firstField = firstValue;
592+
} else {
593+
// Reject the message as invalid
594+
throw new IllegalArgumentException(
595+
"Multiple Content-Length values found: " + contentLengthFields);
596+
}
597+
}
598+
// Ensure we not allow sign as part of the content-length:
599+
// See https://github.com/squid-cache/squid/security/advisories/GHSA-qf3v-rc95-96j5
600+
if (!Character.isDigit(firstField.charAt(0))) {
601+
// Reject the message as invalid
602+
throw new IllegalArgumentException(
603+
"Content-Length value is not a number: " + firstField);
604+
}
605+
try {
606+
final long value = Long.parseLong(firstField);
607+
if (value < 0) {
608+
// Reject the message as invalid
609+
throw new IllegalArgumentException(
610+
"Content-Length value must be >=0: " + value);
611+
}
612+
return value;
613+
} catch (NumberFormatException e) {
614+
// Reject the message as invalid
615+
throw new IllegalArgumentException(
616+
"Content-Length value is not a number: " + firstField, e);
617+
}
618+
}
533619
}

Diff for: codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java

+91-9
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,11 @@
1616

1717
import io.netty.buffer.ByteBuf;
1818
import io.netty.channel.ChannelHandlerContext;
19+
import io.netty.handler.codec.http.HttpHeaderNames;
1920
import io.netty.handler.codec.http.HttpStatusClass;
21+
import io.netty.handler.codec.http.HttpUtil;
2022
import io.netty.handler.codec.http2.Http2Connection.Endpoint;
23+
import io.netty.util.internal.SystemPropertyUtil;
2124
import io.netty.util.internal.UnstableApi;
2225
import io.netty.util.internal.logging.InternalLogger;
2326
import io.netty.util.internal.logging.InternalLoggerFactory;
@@ -49,6 +52,8 @@
4952
*/
5053
@UnstableApi
5154
public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
55+
private static final boolean VALIDATE_CONTENT_LENGTH =
56+
SystemPropertyUtil.getBoolean("io.netty.http2.validateContentLength", true);
5257
private static final InternalLogger logger = InternalLoggerFactory.getInstance(DefaultHttp2ConnectionDecoder.class);
5358
private Http2FrameListener internalFrameListener = new PrefaceFrameListener();
5459
private final Http2Connection connection;
@@ -59,6 +64,7 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
5964
private final Http2PromisedRequestVerifier requestVerifier;
6065
private final Http2SettingsReceivedConsumer settingsReceivedConsumer;
6166
private final boolean autoAckPing;
67+
private final Http2Connection.PropertyKey contentLengthKey;
6268

6369
public DefaultHttp2ConnectionDecoder(Http2Connection connection,
6470
Http2ConnectionEncoder encoder,
@@ -125,6 +131,7 @@ public DefaultHttp2ConnectionDecoder(Http2Connection connection,
125131
settingsReceivedConsumer = (Http2SettingsReceivedConsumer) encoder;
126132
}
127133
this.connection = checkNotNull(connection, "connection");
134+
contentLengthKey = this.connection.newKey();
128135
this.frameReader = checkNotNull(frameReader, "frameReader");
129136
this.encoder = checkNotNull(encoder, "encoder");
130137
this.requestVerifier = checkNotNull(requestVerifier, "requestVerifier");
@@ -223,6 +230,23 @@ void onUnknownFrame0(ChannelHandlerContext ctx, byte frameType, int streamId, Ht
223230
listener.onUnknownFrame(ctx, frameType, streamId, flags, payload);
224231
}
225232

233+
// See https://tools.ietf.org/html/rfc7540#section-8.1.2.6
234+
private void verifyContentLength(Http2Stream stream, int data, boolean isEnd) throws Http2Exception {
235+
if (!VALIDATE_CONTENT_LENGTH) {
236+
return;
237+
}
238+
ContentLength contentLength = stream.getProperty(contentLengthKey);
239+
if (contentLength != null) {
240+
try {
241+
contentLength.increaseReceivedBytes(connection.isServer(), stream.id(), data, isEnd);
242+
} finally {
243+
if (isEnd) {
244+
stream.removeProperty(contentLengthKey);
245+
}
246+
}
247+
}
248+
}
249+
226250
/**
227251
* Handles all inbound frames from the network.
228252
*/
@@ -232,7 +256,8 @@ public int onDataRead(final ChannelHandlerContext ctx, int streamId, ByteBuf dat
232256
boolean endOfStream) throws Http2Exception {
233257
Http2Stream stream = connection.stream(streamId);
234258
Http2LocalFlowController flowController = flowController();
235-
int bytesToReturn = data.readableBytes() + padding;
259+
int readable = data.readableBytes();
260+
int bytesToReturn = readable + padding;
236261

237262
final boolean shouldIgnore;
238263
try {
@@ -259,7 +284,6 @@ public int onDataRead(final ChannelHandlerContext ctx, int streamId, ByteBuf dat
259284
// All bytes have been consumed.
260285
return bytesToReturn;
261286
}
262-
263287
Http2Exception error = null;
264288
switch (stream.state()) {
265289
case OPEN:
@@ -287,6 +311,8 @@ public int onDataRead(final ChannelHandlerContext ctx, int streamId, ByteBuf dat
287311
throw error;
288312
}
289313

314+
verifyContentLength(stream, readable, endOfStream);
315+
290316
// Call back the application and retrieve the number of bytes that have been
291317
// immediately processed.
292318
bytesToReturn = listener.onDataRead(ctx, streamId, data, padding, endOfStream);
@@ -367,14 +393,34 @@ public void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers
367393
stream.state());
368394
}
369395

370-
stream.headersReceived(isInformational);
371-
encoder.flowController().updateDependencyTree(streamId, streamDependency, weight, exclusive);
372-
373-
listener.onHeadersRead(ctx, streamId, headers, streamDependency, weight, exclusive, padding, endOfStream);
396+
if (!stream.isHeadersReceived()) {
397+
// extract the content-length header
398+
List<? extends CharSequence> contentLength = headers.getAll(HttpHeaderNames.CONTENT_LENGTH);
399+
if (contentLength != null && !contentLength.isEmpty()) {
400+
try {
401+
long cLength = HttpUtil.normalizeAndGetContentLength(contentLength, false, true);
402+
if (cLength != -1) {
403+
headers.setLong(HttpHeaderNames.CONTENT_LENGTH, cLength);
404+
stream.setProperty(contentLengthKey, new ContentLength(cLength));
405+
}
406+
} catch (IllegalArgumentException e) {
407+
throw streamError(stream.id(), PROTOCOL_ERROR,
408+
"Multiple content-length headers received", e);
409+
}
410+
}
411+
}
374412

375-
// If the headers completes this stream, close it.
376-
if (endOfStream) {
377-
lifecycleManager.closeStreamRemote(stream, ctx.newSucceededFuture());
413+
stream.headersReceived(isInformational);
414+
try {
415+
verifyContentLength(stream, 0, endOfStream);
416+
encoder.flowController().updateDependencyTree(streamId, streamDependency, weight, exclusive);
417+
listener.onHeadersRead(ctx, streamId, headers, streamDependency,
418+
weight, exclusive, padding, endOfStream);
419+
} finally {
420+
// If the headers completes this stream, close it.
421+
if (endOfStream) {
422+
lifecycleManager.closeStreamRemote(stream, ctx.newSucceededFuture());
423+
}
378424
}
379425
}
380426

@@ -736,4 +782,40 @@ public void onUnknownFrame(ChannelHandlerContext ctx, byte frameType, int stream
736782
onUnknownFrame0(ctx, frameType, streamId, flags, payload);
737783
}
738784
}
785+
786+
private static final class ContentLength {
787+
private final long expected;
788+
private long seen;
789+
790+
ContentLength(long expected) {
791+
this.expected = expected;
792+
}
793+
794+
void increaseReceivedBytes(boolean server, int streamId, int bytes, boolean isEnd) throws Http2Exception {
795+
seen += bytes;
796+
// Check for overflow
797+
if (seen < 0) {
798+
throw streamError(streamId, PROTOCOL_ERROR,
799+
"Received amount of data did overflow and so not match content-length header %d", expected);
800+
}
801+
// Check if we received more data then what was advertised via the content-length header.
802+
if (seen > expected) {
803+
throw streamError(streamId, PROTOCOL_ERROR,
804+
"Received amount of data %d does not match content-length header %d", seen, expected);
805+
}
806+
807+
if (isEnd) {
808+
if (seen == 0 && !server) {
809+
// This may be a response to a HEAD request, let's just allow it.
810+
return;
811+
}
812+
813+
// Check that we really saw what was told via the content-length header.
814+
if (expected > seen) {
815+
throw streamError(streamId, PROTOCOL_ERROR,
816+
"Received amount of data %d does not match content-length header %d", seen, expected);
817+
}
818+
}
819+
}
820+
}
739821
}

0 commit comments

Comments
 (0)