Projects
openEuler:22.03:LTS:LoongArch
netty
_service:tar_scm_kernel_repo:CVE-2021-21295.patch
Sign Up
Log In
Username
Password
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File _service:tar_scm_kernel_repo:CVE-2021-21295.patch of Package netty
From 89c241e3b1795ff257af4ad6eadc616cb2fb3dc4 Mon Sep 17 00:00:00 2001 From: Norman Maurer <norman_maurer@apple.com> Date: Tue, 9 Mar 2021 08:20:09 +0100 Subject: [PATCH] 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 --- .../handler/codec/http/HttpObjectDecoder.java | 48 +------ .../io/netty/handler/codec/http/HttpUtil.java | 87 ++++++++++++ .../http2/DefaultHttp2ConnectionDecoder.java | 100 ++++++++++++-- .../DefaultHttp2ConnectionDecoderTest.java | 128 ++++++++++++++++++ 4 files changed, 313 insertions(+), 50 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java index b52e36ac92..82b0c365c1 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java +++ b/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; @@ -638,49 +637,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: - // - // 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); } } diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java index 035cd754cf..3d1c57f5ac 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java @@ -25,6 +25,11 @@ import java.nio.charset.UnsupportedCharsetException; import java.util.Iterator; import java.util.List; +import io.netty.handler.codec.Headers; +import io.netty.util.internal.UnstableApi; + +import static io.netty.util.internal.StringUtil.COMMA; + /** * Utility methods useful in the HTTP context. */ @@ -40,6 +45,7 @@ public final class HttpUtil { static final EmptyHttpHeaders EMPTY_HEADERS = new EmptyHttpHeaders(); private static final AsciiString CHARSET_EQUALS = AsciiString.of(HttpHeaderValues.CHARSET + "="); private static final AsciiString SEMICOLON = AsciiString.of(";"); + private static final String COMMA_STRING = String.valueOf(COMMA); private HttpUtil() { } @@ -519,4 +525,85 @@ public final class HttpUtil { return contentTypeValue.length() > 0 ? contentTypeValue : null; } } + + /** + * 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); + } + } } diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java index 4027978651..f04a0b5a69 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java +++ b/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; @@ -57,6 +62,7 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder { private final Http2FrameReader frameReader; private Http2FrameListener listener; private final Http2PromisedRequestVerifier requestVerifier; + private final Http2Connection.PropertyKey contentLengthKey; public DefaultHttp2ConnectionDecoder(Http2Connection connection, Http2ConnectionEncoder encoder, @@ -69,6 +75,7 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder { Http2FrameReader frameReader, Http2PromisedRequestVerifier requestVerifier) { this.connection = checkNotNull(connection, "connection"); + contentLengthKey = this.connection.newKey(); this.frameReader = checkNotNull(frameReader, "frameReader"); this.encoder = checkNotNull(encoder, "encoder"); this.requestVerifier = checkNotNull(requestVerifier, "requestVerifier"); @@ -171,6 +178,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. */ @@ -180,7 +204,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 { @@ -207,7 +232,6 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder { // All bytes have been consumed. return bytesToReturn; } - Http2Exception error = null; switch (stream.state()) { case OPEN: @@ -235,6 +259,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); @@ -315,14 +341,34 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder { 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()); + } } } @@ -673,4 +719,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); + } + } + } + } } diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java index 3fcf560eff..467c6a9904 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java +++ b/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; @@ -130,6 +134,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 @@ -751,6 +770,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)); -- 2.23.0
Locations
Projects
Search
Status Monitor
Help
Open Build Service
OBS Manuals
API Documentation
OBS Portal
Reporting a Bug
Contact
Mailing List
Forums
Chat (IRC)
Twitter
Open Build Service (OBS)
is an
openSUSE project
.