CVE-2021-21295 (Java, published 2021-03-09T19:15Z)
Description: value: Netty is an open-source, asynchronous event-driven network application framework for rapid development of maintainable high performance protocol servers & clients. In Netty (io.netty:netty-codec-http2) before version 4.1.60.Final there is a vulnerability that enables request smuggling. If a Content-Length header is present in the original HTTP/2 request, the field is not validated by `Http2MultiplexHandler` as it is propagated up. This is fine as long as the request is not proxied through as HTTP/1.1. If the request comes in as an HTTP/2 stream, gets converted into the HTTP/1.1 domain objects (`HttpRequest`, `HttpContent`, etc.) via `Http2StreamFrameToHttpObjectCodec `and then sent up to the child channel's pipeline and proxied through a remote peer as HTTP/1.1 this may result in request smuggling. In a proxy case, users may assume the content-length is validated somehow, which is not the case. If the request is forwarded to a backend channel that is a HTTP/1.1 connection, the Content-Length now has meaning and needs to be checked. An attacker can smuggle requests inside the body as it gets downgraded from HTTP/2 to HTTP/1.1. For an example attack refer to the linked GitHub Advisory. Users are only affected if all of this is true: `HTTP2MultiplexCodec` or `Http2FrameCodec` is used, `Http2StreamFrameToHttpObjectCodec` is used to convert to HTTP/1.1 objects, and these HTTP/1.1 objects are forwarded to another remote peer. This has been patched in 4.1.60.Final As a workaround, the user can do the validation by themselves by implementing a custom `ChannelInboundHandler` that is put in the `ChannelPipeline` behind `Http2StreamFrameToHttpObjectCodec`.
Severity: LOW (CVSS v3 base score: 5.9)
Weakness: CWE-444 \u2014 Inconsistent Interpretation of HTTP Requests ('HTTP Request/Response Smuggling')
The product acts as an intermediary HTTP agent
         (such as a proxy or firewall) in the data flow between two
         entities such as a client and server, but it does not
         interpret malformed HTTP requests or responses in ways that
         are consistent with how the messages will be processed by
         those entities that are at the ultimate destination.
Repository: https://github.com/netty/netty
Files changed:
codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java
codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java
codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java
codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java
Commit message: 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

Vulnerable code:
import static io.netty.util.internal.StringUtil.COMMA;

            //
            // 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));
            int bytesToReturn = data.readableBytes() + padding;

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

            listener.onHeadersRead(ctx, streamId, headers, streamDependency, weight, exclusive, padding, endOfStream);
            // If the headers completes this stream, close it.
            if (endOfStream) {
                lifecycleManager.closeStreamRemote(stream, ctx.newSucceededFuture());

Fixed code:
            HttpVersion version = message.protocolVersion();
            boolean isHttp10OrEarlier = version.majorVersion() < 1 || (version.majorVersion() == 1
                    && version.minorVersion() == 0);
            contentLength = HttpUtil.normalizeAndGetContentLength(contentLengthFields,
                    isHttp10OrEarlier, allowDuplicateContentLengths);
            if (contentLength != -1) {
                headers.set(HttpHeaderNames.CONTENT_LENGTH, contentLength);
import io.netty.handler.codec.Headers;
import io.netty.util.internal.UnstableApi;

import static io.netty.util.internal.StringUtil.COMMA;
    private static final String COMMA_STRING = String.valueOf(COMMA);

    /**
     * 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);
        }
    }
import io.netty.handler.codec.http.HttpHeaderNames;
import io.netty.handler.codec.http.HttpUtil;
import io.netty.util.internal.SystemPropertyUtil;
    private static final boolean VALIDATE_CONTENT_LENGTH =
            SystemPropertyUtil.getBoolean("io.netty.http2.validateContentLength", true);
    private final Http2Connection.PropertyKey contentLengthKey;
        contentLengthKey = this.connection.newKey();
    // 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);
                }
            }
        }
    }

            int readable = data.readableBytes();
            int bytesToReturn = readable + padding;
                verifyContentLength(stream, readable, 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);
                    }
                }
            }
            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());
                }

    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);
                }
            }
        }
    }
import io.netty.handler.codec.http.HttpHeaderNames;
import org.mockito.ArgumentMatchers;
import java.util.IdentityHashMap;
import java.util.Map;

        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));
            }
        });

    @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());
        }
    }