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

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

@@ -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;
@@ -630,49 +629,16 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
         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:
-            //
+            contentLength = HttpUtil.normalizeAndGetContentLength(contentLengthFields,
-            // If a message is received that has multiple Content-Length header
+                    isHttp10OrEarlier, allowDuplicateContentLengths);
-            //   fields with field-values consisting of the same decimal value, or a
+            if (contentLength != -1) {
-            //   single Content-Length header field with a field value containing a
+                headers.set(HttpHeaderNames.CONTENT_LENGTH, contentLength);
-            //   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));
             }
         }
 

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

@@ -24,10 +24,14 @@ import java.util.ArrayList;
 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.
@@ -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() { }
 
@@ -530,4 +535,85 @@ public final class HttpUtil {
         }
         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);
+        }
+    }
 }

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

@@ -16,8 +16,11 @@ package io.netty.handler.codec.http2;
 
 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;
@@ -49,6 +52,8 @@ import static java.lang.Math.min;
  */
 @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;
@@ -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,
@@ -125,6 +131,7 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
             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");
@@ -223,6 +230,23 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
         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.
      */
@@ -232,7 +256,8 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
                               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 {
@@ -259,7 +284,6 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
                 // All bytes have been consumed.
                 return bytesToReturn;
             }
-
             Http2Exception error = null;
             switch (stream.state()) {
                 case OPEN:
@@ -287,6 +311,8 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
                     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);
@@ -367,16 +393,36 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
                             stream.state());
             }
 
+            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);
+                    }
+                }
+            }
+
             stream.headersReceived(isInformational);
+            try {
+                verifyContentLength(stream, 0, endOfStream);
                 encoder.flowController().updateDependencyTree(streamId, streamDependency, weight, exclusive);
-
+                listener.onHeadersRead(ctx, streamId, headers, streamDependency,
-            listener.onHeadersRead(ctx, streamId, headers, streamDependency, weight, exclusive, padding, endOfStream);
+                        weight, exclusive, padding, endOfStream);
-
+            } finally {
                 // If the headers completes this stream, close it.
                 if (endOfStream) {
                     lifecycleManager.closeStreamRemote(stream, ctx.newSucceededFuture());
                 }
             }
+        }
 
         @Override
         public void onPriorityRead(ChannelHandlerContext ctx, int streamId, int streamDependency, short weight,
@@ -736,4 +782,40 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
             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);
+                }
+            }
+        }
+    }
 }

codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java

@@ -21,17 +21,21 @@ import io.netty.channel.ChannelFuture;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelPromise;
 import io.netty.channel.DefaultChannelPromise;
+import io.netty.handler.codec.http.HttpHeaderNames;
 import io.netty.handler.codec.http.HttpResponseStatus;
 import junit.framework.AssertionFailedError;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
+import org.mockito.ArgumentMatchers;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
 
 import java.util.Collections;
+import java.util.IdentityHashMap;
+import java.util.Map;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import static io.netty.buffer.Unpooled.EMPTY_BUFFER;
@@ -134,6 +138,21 @@ public class DefaultHttp2ConnectionDecoderTest {
         when(stream.id()).thenReturn(STREAM_ID);
         when(stream.state()).thenReturn(OPEN);
         when(stream.open(anyBoolean())).thenReturn(stream);
+
+        final Map<Object, Object> properties = new IdentityHashMap<Object, Object>();
+        when(stream.getProperty(ArgumentMatchers.<Http2Connection.PropertyKey>any())).thenAnswer(new Answer<Object>() {
+            @Override
+            public Object answer(InvocationOnMock invocationOnMock) {
+                return properties.get(invocationOnMock.getArgument(0));
+            }
+        });
+        when(stream.setProperty(ArgumentMatchers.<Http2Connection.PropertyKey>any(), any())).then(new Answer<Object>() {
+            @Override
+            public Object answer(InvocationOnMock invocationOnMock) {
+                return properties.put(invocationOnMock.getArgument(0), invocationOnMock.getArgument(1));
+            }
+        });
+
         when(pushStream.id()).thenReturn(PUSH_STREAM_ID);
         doAnswer(new Answer<Boolean>() {
             @Override
@@ -774,6 +793,115 @@ public class DefaultHttp2ConnectionDecoderTest {
         verify(listener).onGoAwayRead(eq(ctx), eq(1), eq(2L), eq(EMPTY_BUFFER));
     }
 
+    @Test(expected = Http2Exception.StreamException.class)
+    public void dataContentLengthMissmatch() throws Exception {
+        dataContentLengthInvalid(false);
+    }
+
+    @Test(expected = Http2Exception.StreamException.class)
+    public void dataContentLengthInvalid() throws Exception {
+        dataContentLengthInvalid(true);
+    }
+
+    private void dataContentLengthInvalid(boolean negative) throws Exception {
+        final ByteBuf data = dummyData();
+        int padding = 10;
+        int processedBytes = data.readableBytes() + padding;
+        mockFlowControl(processedBytes);
+        try {
+            decode().onHeadersRead(ctx, STREAM_ID, new DefaultHttp2Headers()
+                    .setLong(HttpHeaderNames.CONTENT_LENGTH, negative ? -1L : 1L), padding, false);
+            decode().onDataRead(ctx, STREAM_ID, data, padding, true);
+            verify(localFlow).receiveFlowControlledFrame(eq(stream), eq(data), eq(padding), eq(true));
+            verify(localFlow).consumeBytes(eq(stream), eq(processedBytes));
+
+            verify(listener, times(1)).onHeadersRead(eq(ctx), anyInt(),
+                    any(Http2Headers.class), eq(0), eq(DEFAULT_PRIORITY_WEIGHT), eq(false),
+                    eq(padding), eq(false));
+            // Verify that the event was absorbed and not propagated to the observer.
+            verify(listener, never()).onDataRead(eq(ctx), anyInt(), any(ByteBuf.class), anyInt(), anyBoolean());
+        } finally {
+            data.release();
+        }
+    }
+
+    @Test(expected = Http2Exception.StreamException.class)
+    public void headersContentLengthPositiveSign() throws Exception {
+        headersContentLengthSign("+1");
+    }
+
+    @Test(expected = Http2Exception.StreamException.class)
+    public void headersContentLengthNegativeSign() throws Exception {
+        headersContentLengthSign("-1");
+    }
+
+    private void headersContentLengthSign(String length) throws Exception {
+        int padding = 10;
+        when(connection.isServer()).thenReturn(true);
+        decode().onHeadersRead(ctx, STREAM_ID, new DefaultHttp2Headers()
+                .set(HttpHeaderNames.CONTENT_LENGTH, length), padding, false);
+
+        // Verify that the event was absorbed and not propagated to the observer.
+        verify(listener, never()).onHeadersRead(eq(ctx), anyInt(),
+                any(Http2Headers.class), anyInt(), anyShort(), anyBoolean(), anyInt(), anyBoolean());
+    }
+
+    @Test(expected = Http2Exception.StreamException.class)
+    public void headersContentLengthMissmatch() throws Exception {
+        headersContentLength(false);
+    }
+
+    @Test(expected = Http2Exception.StreamException.class)
+    public void headersContentLengthInvalid() throws Exception {
+        headersContentLength(true);
+    }
+
+    private void headersContentLength(boolean negative) throws Exception {
+        int padding = 10;
+        when(connection.isServer()).thenReturn(true);
+        decode().onHeadersRead(ctx, STREAM_ID, new DefaultHttp2Headers()
+                .setLong(HttpHeaderNames.CONTENT_LENGTH, negative ? -1L : 1L), padding, true);
+
+        // Verify that the event was absorbed and not propagated to the observer.
+        verify(listener, never()).onHeadersRead(eq(ctx), anyInt(),
+                any(Http2Headers.class), anyInt(), anyShort(), anyBoolean(), anyInt(), anyBoolean());
+    }
+
+    @Test
+    public void multipleHeadersContentLengthSame() throws Exception {
+        multipleHeadersContentLength(true);
+    }
+
+    @Test(expected = Http2Exception.StreamException.class)
+    public void multipleHeadersContentLengthDifferent() throws Exception {
+        multipleHeadersContentLength(false);
+    }
+
+    private void multipleHeadersContentLength(boolean same) throws Exception {
+        int padding = 10;
+        when(connection.isServer()).thenReturn(true);
+        Http2Headers headers = new DefaultHttp2Headers();
+        if (same) {
+            headers.addLong(HttpHeaderNames.CONTENT_LENGTH, 0);
+            headers.addLong(HttpHeaderNames.CONTENT_LENGTH, 0);
+        } else {
+            headers.addLong(HttpHeaderNames.CONTENT_LENGTH, 0);
+            headers.addLong(HttpHeaderNames.CONTENT_LENGTH, 1);
+        }
+
+        decode().onHeadersRead(ctx, STREAM_ID, headers, padding, true);
+
+        if (same) {
+            verify(listener, times(1)).onHeadersRead(eq(ctx), anyInt(),
+                    any(Http2Headers.class), anyInt(), anyShort(), anyBoolean(), anyInt(), anyBoolean());
+            assertEquals(1, headers.getAll(HttpHeaderNames.CONTENT_LENGTH).size());
+        } else {
+            // Verify that the event was absorbed and not propagated to the observer.
+            verify(listener, never()).onHeadersRead(eq(ctx), anyInt(),
+                    any(Http2Headers.class), anyInt(), anyShort(), anyBoolean(), anyInt(), anyBoolean());
+        }
+    }
+
     private static ByteBuf dummyData() {
         // The buffer is purposely 8 bytes so it will even work for a ping frame.
         return wrappedBuffer("abcdefgh".getBytes(UTF_8));