[http1 codec] Allow requests with Transfer-Encoding and Content-Length headers set.

api/envoy/config/core/v3/protocol.proto

@@ -100,7 +100,7 @@ message HttpProtocolOptions {
   HeadersWithUnderscoresAction headers_with_underscores_action = 5;
 }
 
-// [#next-free-field: 6]
+// [#next-free-field: 7]
 message Http1ProtocolOptions {
   option (udpa.annotations.versioning).previous_message_type =
       "envoy.api.v2.core.Http1ProtocolOptions";
@@ -157,6 +157,16 @@ message Http1ProtocolOptions {
   //   - Not a response to a HEAD request.
   //   - The content length header is not present.
   bool enable_trailers = 5;
+
+  // Allows Envoy to process requests/responses with both `Content-Length` and `Transfer-Encoding`
+  // headers set. By default such messages are rejected, but if option is enabled - Envoy will
+  // remove Content-Length header and process message.
+  // See `RFC7230, sec. 3.3.3 <https://tools.ietf.org/html/rfc7230#section-3.3.3>` for details.
+  //
+  // .. attention::
+  //   Enabling this option might lead to request smuggling vulnerability, especially if traffic
+  //   is proxied via multiple layers of proxies.
+  bool allow_chunked_length = 6;
 }
 
 // [#next-free-field: 15]

bazel/repository_locations.bzl

@@ -283,9 +283,12 @@ DEPENDENCY_REPOSITORIES = dict(
         cpe = "N/A",
     ),
     com_github_nodejs_http_parser = dict(
-        sha256 = "8fa0ab8770fd8425a9b431fdbf91623c4d7a9cdb842b9339289bd2b0b01b0d3d",
-        strip_prefix = "http-parser-2.9.3",
-        urls = ["https://github.com/nodejs/http-parser/archive/v2.9.3.tar.gz"],
+        sha256 = "6a12896313ce1ca630cf516a0ee43a79b5f13f5a5d8143f56560ac0b21c98fac",
+        strip_prefix = "http-parser-4f15b7d510dc7c6361a26a7c6d2f7c3a17f8d878",
+        # 2020-07-10
+        # This SHA includes fix for https://github.com/nodejs/http-parser/issues/517 which allows (opt-in) to serve
+        # requests with both Content-Legth and Transfer-Encoding: chunked headers set.
+        urls = ["https://github.com/nodejs/http-parser/archive/4f15b7d510dc7c6361a26a7c6d2f7c3a17f8d878.tar.gz"],
         use_category = ["dataplane"],
         cpe = "cpe:2.3:a:nodejs:node.js:*",
     ),

docs/root/version_history/current.rst

@@ -47,6 +47,7 @@ New Features
 * ext_authz filter: added support for emitting dynamic metadata for both :ref:`HTTP <config_http_filters_ext_authz_dynamic_metadata>` and :ref:`network <config_network_filters_ext_authz_dynamic_metadata>` filters.
 * grpc-json: support specifying `response_body` field in for `google.api.HttpBody` message.
 * http: added support for :ref:`%DOWNSTREAM_PEER_FINGERPRINT_1% <config_http_conn_man_headers_custom_request_headers>` as custom header.
+* http: added :ref:`allow_chunked_length <envoy_v3_api_field_config.core.v3.Http1ProtocolOptions.allow_chunked_length>` configuration option for HTTP/1 codec to allow processing requests/responses with both Content-Length and Transfer-Encoding: chunked headers. If such message is served and option is enabled - per RFC Content-Length is ignored and removed.
 * http: introduced new HTTP/1 and HTTP/2 codec implementations that will remove the use of exceptions for control flow due to high risk factors and instead use error statuses. The old behavior is used by default, but the new codecs can be enabled for testing by setting the runtime feature `envoy.reloadable_features.new_codec_behavior` to true. The new codecs will be in development for one month, and then enabled by default while the old codecs are deprecated.
 * load balancer: added a :ref:`configuration<envoy_v3_api_msg_config.cluster.v3.Cluster.LeastRequestLbConfig>` option to specify the active request bias used by the least request load balancer.
 * lua: added Lua APIs to access :ref:`SSL connection info <config_http_filters_lua_ssl_socket_info>` object.

include/envoy/http/codec.h

@@ -374,6 +374,10 @@ struct Http1Settings {
   //  - Is neither a HEAD only request nor a HTTP Upgrade
   //  - Not a HEAD request
   bool enable_trailers_{false};
+  // Allows Envoy to process requests/responses with both `Content-Length` and `Transfer-Encoding`
+  // headers set. By default such messages are rejected, but if option is enabled - Envoy will
+  // remove Content-Length header and process message.
+  bool allow_chunked_length_{false};
 
   enum class HeaderKeyFormat {
     // By default no formatting is performed, presenting all headers in lowercase (as Envoy

source/common/http/http1/codec_impl.cc

@@ -442,15 +442,14 @@ http_parser_settings ConnectionImpl::settings_{
 };
 
 ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stats,
-                               http_parser_type type, uint32_t max_headers_kb,
-                               const uint32_t max_headers_count,
-                               HeaderKeyFormatterPtr&& header_key_formatter, bool enable_trailers)
-    : connection_(connection), stats_(stats),
+                               const Http1Settings& settings, http_parser_type type,
+                               uint32_t max_headers_kb, const uint32_t max_headers_count,
+                               HeaderKeyFormatterPtr&& header_key_formatter)
+    : connection_(connection), stats_(stats), codec_settings_(settings),
       header_key_formatter_(std::move(header_key_formatter)), processing_trailers_(false),
       handling_upgrade_(false), reset_stream_called_(false), deferred_end_stream_headers_(false),
       connection_header_sanitization_(Runtime::runtimeFeatureEnabled(
           "envoy.reloadable_features.connection_header_sanitization")),
-      enable_trailers_(enable_trailers),
       strict_1xx_and_204_headers_(Runtime::runtimeFeatureEnabled(
           "envoy.reloadable_features.strict_1xx_and_204_response_headers")),
       output_buffer_([&]() -> void { this->onBelowLowWatermark(); },
@@ -459,6 +458,7 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat
       max_headers_kb_(max_headers_kb), max_headers_count_(max_headers_count) {
   output_buffer_.setWatermarks(connection.bufferLimit());
   http_parser_init(&parser_, type);
+  parser_.allow_chunked_length = 1;
   parser_.data = this;
 }
 
@@ -580,7 +580,7 @@ void ConnectionImpl::onHeaderField(const char* data, size_t length) {
   // We previously already finished up the headers, these headers are
   // now trailers.
   if (header_parsing_state_ == HeaderParsingState::Done) {
-    if (!enable_trailers_) {
+    if (!enableTrailers()) {
       // Ignore trailers.
       return;
     }
@@ -598,7 +598,7 @@ void ConnectionImpl::onHeaderField(const char* data, size_t length) {
 }
 
 void ConnectionImpl::onHeaderValue(const char* data, size_t length) {
-  if (header_parsing_state_ == HeaderParsingState::Done && !enable_trailers_) {
+  if (header_parsing_state_ == HeaderParsingState::Done && !enableTrailers()) {
     // Ignore trailers.
     return;
   }
@@ -674,6 +674,29 @@ int ConnectionImpl::onHeadersCompleteBase() {
     handling_upgrade_ = true;
   }
 
+  // https://tools.ietf.org/html/rfc7230#section-3.3.3
+  // If a message is received with both a Transfer-Encoding and a
+  // Content-Length header field, the Transfer-Encoding overrides the
+  // Content-Length. Such a message might indicate an attempt to
+  // perform request smuggling (Section 9.5) or response splitting
+  // (Section 9.4) and ought to be handled as an error. A sender MUST
+  // remove the received Content-Length field prior to forwarding such
+  // a message.
+
+  // Reject message with Http::Code::BadRequest if both Transfer-Encoding and Content-Length
+  // headers are present or if allowed by http1 codec settings and 'Transfer-Encoding'
+  // is chunked - remove Content-Length and serve request.
+  if (parser_.uses_transfer_encoding != 0 && request_or_response_headers.ContentLength()) {
+    if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) {
+      request_or_response_headers.removeContentLength();
+    } else {
+      error_code_ = Http::Code::BadRequest;
+      sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed);
+      throw CodecProtocolException(
+          "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set.");
+    }
+  }
+
   // Per https://tools.ietf.org/html/rfc7230#section-3.3.1 Envoy should reject
   // transfer-codings it does not understand.
   // Per https://tools.ietf.org/html/rfc7231#section-4.3.6 a payload with a
@@ -762,9 +785,9 @@ ServerConnectionImpl::ServerConnectionImpl(
     const uint32_t max_request_headers_count,
     envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
         headers_with_underscores_action)
-    : ConnectionImpl(connection, stats, HTTP_REQUEST, max_request_headers_kb,
-                     max_request_headers_count, formatter(settings), settings.enable_trailers_),
-      callbacks_(callbacks), codec_settings_(settings),
+    : ConnectionImpl(connection, stats, settings, HTTP_REQUEST, max_request_headers_kb,
+                     max_request_headers_count, formatter(settings)),
+      callbacks_(callbacks),
       response_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) {
         releaseOutboundResponse(fragment);
       }),
@@ -1055,8 +1078,8 @@ void ServerConnectionImpl::checkHeaderNameForUnderscores() {
 ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, CodecStats& stats,
                                            ConnectionCallbacks&, const Http1Settings& settings,
                                            const uint32_t max_response_headers_count)
-    : ConnectionImpl(connection, stats, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB,
-                     max_response_headers_count, formatter(settings), settings.enable_trailers_) {}
+    : ConnectionImpl(connection, stats, settings, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB,
+                     max_response_headers_count, formatter(settings)) {}
 
 bool ClientConnectionImpl::cannotHaveBody() {
   if (pending_response_.has_value() && pending_response_.value().encoder_.headRequest()) {

source/common/http/http1/codec_impl.h

@@ -204,7 +204,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   bool maybeDirectDispatch(Buffer::Instance& data);
   virtual void maybeAddSentinelBufferFragment(Buffer::WatermarkBuffer&) {}
   CodecStats& stats() { return stats_; }
-  bool enableTrailers() const { return enable_trailers_; }
+  bool enableTrailers() const { return codec_settings_.enable_trailers_; }
 
   // Http::Connection
   Http::Status dispatch(Buffer::Instance& data) override;
@@ -218,9 +218,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   bool strict1xxAnd204Headers() { return strict_1xx_and_204_headers_; }
 
 protected:
-  ConnectionImpl(Network::Connection& connection, CodecStats& stats, http_parser_type type,
-                 uint32_t max_headers_kb, const uint32_t max_headers_count,
-                 HeaderKeyFormatterPtr&& header_key_formatter, bool enable_trailers);
+  ConnectionImpl(Network::Connection& connection, CodecStats& stats, const Http1Settings& settings,
+                 http_parser_type type, uint32_t max_headers_kb, const uint32_t max_headers_count,
+                 HeaderKeyFormatterPtr&& header_key_formatter);
 
   bool resetStreamCalled() { return reset_stream_called_; }
   void onMessageBeginBase();
@@ -241,6 +241,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
 
   Network::Connection& connection_;
   CodecStats& stats_;
+  const Http1Settings codec_settings_;
   http_parser parser_;
   Http::Code error_code_{Http::Code::BadRequest};
   const HeaderKeyFormatterPtr header_key_formatter_;
@@ -254,7 +255,6 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   // block with end stream set to true with no further protocol data remaining.
   bool deferred_end_stream_headers_ : 1;
   const bool connection_header_sanitization_ : 1;
-  const bool enable_trailers_ : 1;
   const bool strict_1xx_and_204_headers_ : 1;
 
 private:
@@ -500,7 +500,6 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
 
   ServerConnectionCallbacks& callbacks_;
   absl::optional<ActiveRequest> active_request_;
-  Http1Settings codec_settings_;
   const Buffer::OwnedBufferFragmentImpl::Releasor response_buffer_releasor_;
   uint32_t outbound_responses_{};
   // This defaults to 2, which functionally disables pipelining. If any users
@@ -527,7 +526,6 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
   ClientConnectionImpl(Network::Connection& connection, CodecStats& stats,
                        ConnectionCallbacks& callbacks, const Http1Settings& settings,
                        const uint32_t max_response_headers_count);
-
   // Http::ClientConnection
   RequestEncoder& newStream(ResponseDecoder& response_decoder) override;