http: disallow unknown transfer encodings (envoyproxy#9316)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Prakhar <prakhar_au@yahoo.com>

docs/root/intro/version_history.rst

@@ -13,6 +13,7 @@ Version history
 * ext_authz: added :ref:`configurable ability<envoy_api_field_config.filter.http.ext_authz.v2.ExtAuthz.include_peer_certificate>` to send the :ref:`certificate<envoy_api_field_service.auth.v2.AttributeContext.Peer.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<envoy_api_field_config.filter.http.dynamic_forward_proxy.v2alpha.PerRouteConfig.auto_host_rewrite_header>` in the dynamic forward proxy.
 * jwt_authn: added :ref:`bypass_cors_preflight<envoy_api_field_config.filter.http.jwt_authn.v2alpha.JwtAuthentication.bypass_cors_preflight>` to allow bypassing the CORS preflight request.
 * lb_subset_config: new fallback policy for selectors: :ref:`KEYS_SUBSET<envoy_api_enum_value_Cluster.LbSubsetConfig.LbSubsetSelector.LbSubsetSelectorFallbackPolicy.KEYS_SUBSET>`

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 {

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;

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

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

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;

test/mocks/http/mocks.h

@@ -600,7 +600,15 @@ IsSupersetOfHeadersMatcher IsSupersetOfHeaders(const HeaderMap& expected_headers
 
 MATCHER_P(HeaderMapEqual, rhs, "") {
   Http::HeaderMapImpl& lhs = *dynamic_cast<Http::HeaderMapImpl*>(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, "") {