[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

@@ -389,10 +389,13 @@ DEPENDENCY_REPOSITORIES_SPEC = dict(
     com_github_nodejs_http_parser = dict(
         project_name = "HTTP Parser",
         project_url = "https://github.com/nodejs/http-parser",
-        version = "2.9.3",
-        sha256 = "8fa0ab8770fd8425a9b431fdbf91623c4d7a9cdb842b9339289bd2b0b01b0d3d",
+        # 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.
+        version = "4f15b7d510dc7c6361a26a7c6d2f7c3a17f8d878",
+        sha256 = "6a12896313ce1ca630cf516a0ee43a79b5f13f5a5d8143f56560ac0b21c98fac",
         strip_prefix = "http-parser-{version}",
-        urls = ["https://github.com/nodejs/http-parser/archive/v{version}.tar.gz"],
+        urls = ["https://github.com/nodejs/http-parser/archive/{version}.tar.gz"],
         use_category = ["dataplane"],
         cpe = "cpe:2.3:a:nodejs:node.js:*",
     ),

docs/root/version_history/current.rst

@@ -69,6 +69,7 @@ New Features
 * grpc-json: support specifying `response_body` field in for `google.api.HttpBody` message.
 * hds: added :ref:`cluster_endpoints_health <envoy_v3_api_field_service.health.v3.EndpointHealthResponse.cluster_endpoints_health>` to HDS responses, keeping endpoints in the same groupings as they were configured in the HDS specifier by cluster and locality instead of as a flat list.
 * 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

@@ -42,6 +42,7 @@ struct Http1ResponseCodeDetailValues {
   const absl::string_view TransferEncodingNotAllowed = "http1.transfer_encoding_not_allowed";
   const absl::string_view ContentLengthNotAllowed = "http1.content_length_not_allowed";
   const absl::string_view InvalidUnderscore = "http1.unexpected_underscore";
+  const absl::string_view ChunkedContentLength = "http1.content_length_and_chunked_not_allowed";
 };
 
 struct Http1HeaderTypesValues {
@@ -471,13 +472,12 @@ 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),
-      enable_trailers_(enable_trailers),
       strict_1xx_and_204_headers_(Runtime::runtimeFeatureEnabled(
           "envoy.reloadable_features.strict_1xx_and_204_response_headers")),
       dispatching_(false),
@@ -487,6 +487,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;
 }
 
@@ -632,7 +633,7 @@ Status 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 okStatus();
     }
@@ -651,7 +652,7 @@ Status ConnectionImpl::onHeaderField(const char* data, size_t length) {
 
 Status ConnectionImpl::onHeaderValue(const char* data, size_t length) {
   ASSERT(dispatching_);
-  if (header_parsing_state_ == HeaderParsingState::Done && !enable_trailers_) {
+  if (header_parsing_state_ == HeaderParsingState::Done && !enableTrailers()) {
     // Ignore trailers.
     return okStatus();
   }
@@ -728,6 +729,29 @@ Envoy::StatusOr<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;
+      RETURN_IF_ERROR(sendProtocolError(Http1ResponseCodeDetails::get().ChunkedContentLength));
+      return codecProtocolError(
+          "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
@@ -822,9 +846,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);
       }),
@@ -1123,15 +1147,16 @@ Status 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()) {
     ASSERT(!pending_response_done_);
     return true;
   } else if (parser_.status_code == 204 || parser_.status_code == 304 ||
-             (parser_.status_code >= 200 && parser_.content_length == 0)) {
+             (parser_.status_code >= 200 && parser_.content_length == 0 &&
+              !(parser_.flags & F_CHUNKED))) {
     return true;
   } else {
     return false;

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;
@@ -225,9 +225,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   Envoy::Http::Status codec_status_;
 
 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);
 
   // The following define special return values for http_parser callbacks. See:
   // https://github.com/nodejs/http-parser/blob/5c5b3ac62662736de9e71640a8dc16da45b32503/http_parser.h#L72
@@ -266,6 +266,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_;
@@ -278,7 +279,6 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   // HTTP/1 message has been flushed from the parser. This allows raising an HTTP/2 style headers
   // block with end stream set to true with no further protocol data remaining.
   bool deferred_end_stream_headers_ : 1;
-  const bool enable_trailers_ : 1;
   const bool strict_1xx_and_204_headers_ : 1;
   bool dispatching_ : 1;
 
@@ -531,7 +531,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
@@ -558,7 +557,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;