From 81f5a531bddf92900259760ad1c20b6abf4108a4 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Fri, 13 Dec 2019 12:17:17 -0500 Subject: [PATCH] http: disallow unknown transfer encodings (#9316) Signed-off-by: Alyssa Wilk --- docs/root/intro/version_history.rst | 1 + source/common/http/headers.h | 1 + source/common/http/http1/codec_impl.cc | 14 ++++++ source/common/runtime/runtime_features.cc | 1 + test/common/http/http1/codec_impl_test.cc | 58 +++++++++++++++++++++++ test/integration/integration_test.cc | 36 ++++++++++++++ test/mocks/http/mocks.h | 10 +++- 7 files changed, 120 insertions(+), 1 deletion(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 5725ce0bc8ce..d6f615483fb2 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -13,6 +13,7 @@ Version history * ext_authz: added :ref:`configurable ability` to send the :ref:`certificate` to the `ext_authz` service. * health check: gRPC health checker sets the gRPC deadline to the configured timeout duration. * http: added the ability to sanitize headers nominated by the Connection header. This new behavior is guarded by envoy.reloadable_features.connection_header_sanitization which defaults to true. +* http: blocks unsupported transfer-encodings. Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.reject_unsupported_transfer_encodings` to false. * http: support :ref:`auto_host_rewrite_header` in the dynamic forward proxy. * jwt_authn: added :ref:`bypass_cors_preflight` to allow bypassing the CORS preflight request. * lb_subset_config: new fallback policy for selectors: :ref:`KEYS_SUBSET` diff --git a/source/common/http/headers.h b/source/common/http/headers.h index 3ea47c5d1c0c..388a2dd3f801 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -248,6 +248,7 @@ class HeaderValues { const std::string Chunked{"chunked"}; const std::string Deflate{"deflate"}; const std::string Gzip{"gzip"}; + const std::string Identity{"identity"}; } TransferEncodingValues; struct { diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 0dddbc30b2bc..01a343d83e2d 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -555,6 +555,20 @@ int ConnectionImpl::onHeadersCompleteBase() { } } + // Per https://tools.ietf.org/html/rfc7230#section-3.3.1 Envoy should reject + // transfer-codings it does not understand. + if (current_header_map_->TransferEncoding()) { + absl::string_view encoding = current_header_map_->TransferEncoding()->value().getStringView(); + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.reject_unsupported_transfer_encodings") && + encoding != Headers::get().TransferEncodingValues.Identity && + encoding != Headers::get().TransferEncodingValues.Chunked) { + error_code_ = Http::Code::NotImplemented; + sendProtocolError(); + throw CodecProtocolException("http/1.1 protocol error: unsupported transfer encoding"); + } + } + int rc = onHeadersComplete(std::move(current_header_map_)); current_header_map_.reset(); header_parsing_state_ = HeaderParsingState::Done; diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 73af6426f37a..0f9411d52ee9 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -31,6 +31,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.connection_header_sanitization", "envoy.reloadable_features.strict_header_validation", "envoy.reloadable_features.strict_authority_validation", + "envoy.reloadable_features.reject_unsupported_transfer_encodings", }; // This is a section for officially sanctioned runtime features which are too diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 912d6c63656d..190464d72e26 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -200,6 +200,64 @@ TEST_F(Http1ServerConnectionImplTest, EmptyHeader) { EXPECT_EQ(0U, buffer.length()); } +TEST_F(Http1ServerConnectionImplTest, IdentityEncoding) { + initialize(); + + InSequence sequence; + + Http::MockStreamDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + TestHeaderMapImpl expected_headers{ + {":path", "/"}, + {":method", "GET"}, + {"transfer-encoding", "identity"}, + }; + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)).Times(1); + Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\ntransfer-encoding: identity\r\n\r\n"); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); +} + +TEST_F(Http1ServerConnectionImplTest, ChunkedBody) { + initialize(); + + InSequence sequence; + + Http::MockStreamDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + TestHeaderMapImpl expected_headers{ + {":path", "/"}, + {":method", "POST"}, + {"transfer-encoding", "chunked"}, + }; + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)).Times(1); + Buffer::OwnedImpl expected_data("Hello World"); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); + EXPECT_CALL(decoder, decodeData(_, true)).Times(1); + + Buffer::OwnedImpl buffer( + "POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\nb\r\nHello World\r\n0\r\n\r\n"); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); +} + +// Currently http_parser does not support chained transfer encodings. +TEST_F(Http1ServerConnectionImplTest, IdentityAndChunkedBody) { + initialize(); + + InSequence sequence; + + Http::MockStreamDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\ntransfer-encoding: " + "identity,chunked\r\n\r\nb\r\nHello World\r\n0\r\n\r\n"); + EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), CodecProtocolException, + "http/1.1 protocol error: unsupported transfer encoding"); +} + TEST_F(Http1ServerConnectionImplTest, HostWithLWS) { initialize(); diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 507baf1d7f49..ba592e487bbf 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -328,6 +328,42 @@ TEST_P(IntegrationTest, HittingGrpcFilterLimitBufferingHeaders) { HeaderValueOf(Headers::get().GrpcStatus, "2")); // Unknown gRPC error } +TEST_P(IntegrationTest, TestSmuggling) { + initialize(); + const std::string smuggled_request = "GET / HTTP/1.1\r\nHost: disallowed\r\n\r\n"; + ASSERT_EQ(smuggled_request.length(), 36); + // Make sure the http parser rejects having content-length and transfer-encoding: chunked + // on the same request, regardless of order and spacing. + { + std::string response; + const std::string full_request = + "GET / HTTP/1.1\r\nHost: host\r\ncontent-length: 36\r\ntransfer-encoding: chunked\r\n\r\n" + + smuggled_request; + sendRawHttpAndWaitForResponse(lookupPort("http"), full_request.c_str(), &response, false); + EXPECT_EQ("HTTP/1.1 400 Bad Request\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", + response); + } + { + std::string response; + const std::string request = "GET / HTTP/1.1\r\nHost: host\r\ntransfer-encoding: chunked " + "\r\ncontent-length: 36\r\n\r\n" + + smuggled_request; + sendRawHttpAndWaitForResponse(lookupPort("http"), request.c_str(), &response, false); + EXPECT_EQ("HTTP/1.1 400 Bad Request\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", + response); + } + // Make sure unsupported transfer encodings are rejected, lest they be abused. + { + std::string response; + const std::string request = "GET / HTTP/1.1\r\nHost: host\r\ntransfer-encoding: " + "identity,chunked \r\ncontent-length: 36\r\n\r\n" + + smuggled_request; + sendRawHttpAndWaitForResponse(lookupPort("http"), request.c_str(), &response, false); + EXPECT_EQ("HTTP/1.1 501 Not Implemented\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", + response); + } +} + TEST_P(IntegrationTest, BadFirstline) { initialize(); std::string response; diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 8c3995f1f189..4a0c208b77ac 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -600,7 +600,15 @@ IsSupersetOfHeadersMatcher IsSupersetOfHeaders(const HeaderMap& expected_headers MATCHER_P(HeaderMapEqual, rhs, "") { Http::HeaderMapImpl& lhs = *dynamic_cast(arg.get()); - return lhs == *rhs; + bool equal = (lhs == *rhs); + if (!equal) { + *result_listener << "\n" + << TestUtility::addLeftAndRightPadding("header map:") << "\n" + << *rhs << TestUtility::addLeftAndRightPadding("is not equal to:") << "\n" + << lhs << TestUtility::addLeftAndRightPadding("") // line full of padding + << "\n"; + } + return equal; } MATCHER_P(HeaderMapEqualRef, rhs, "") {