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

bazel/repository_locations.bzl

@@ -283,9 +283,9 @@ 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",
+        urls = ["https://github.com/nodejs/http-parser/archive/4f15b7d510dc7c6361a26a7c6d2f7c3a17f8d878.tar.gz"],
         use_category = ["dataplane"],
         cpe = "cpe:2.3:a:nodejs:node.js:*",
     ),

include/envoy/http/codec.h

@@ -376,6 +376,10 @@ struct Http1Settings {
   //  - Is neither a HEAD only request nor a HTTP Upgrade
   //  - Not a HEAD request
   bool enable_trailers_{false};
+  // Allow serving requests with both Content-Length and Transfer-Encoding: chunked.
+  // In enabled and such request served - Content-Length is ignored and removed from request
+  // headers.
+  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

@@ -459,6 +459,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;
 }
 
@@ -878,6 +879,30 @@ int ServerConnectionImpl::onHeadersComplete() {
           "http/1.1 protocol error: request headers failed spec compliance checks");
     }
 
+    // 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 downstream.
+
+    // Reject request with Http::Code::BadRequest by default or remove Content-Length header
+    // and serve request if allowed by http1 codec settings.
+    if (parser_.uses_transfer_encoding != 0 &&
+        (parser_.content_length > 0 && parser_.content_length != ULLONG_MAX)) {
+      if ((parser_.flags & F_CHUNKED) && allowChunkedLength()) {
+        headers->removeContentLength();
+      } else {
+        ENVOY_CONN_LOG(error, "Both 'Content-Length' and 'Transfer-Encdoding' are set.",
+                       connection_);
+        error_code_ = Http::Code::BadRequest;
+        sendProtocolError(Http1ResponseCodeDetails::get().ContentLengthNotAllowed);
+        throw CodecProtocolException("Both Content-Length and Transfer-Encdoding headers are set.");
+      }
+    }
+
     // Determine here whether we have a body or not. This uses the new RFC semantics where the
     // presence of content-length or chunked transfer-encoding indicates a body vs. a particular
     // method. If there is no body, we defer raising decodeHeaders() until the parser is flushed
@@ -887,7 +912,6 @@ int ServerConnectionImpl::onHeadersComplete() {
     if (parser_.flags & F_CHUNKED ||
         (parser_.content_length > 0 && parser_.content_length != ULLONG_MAX) || handling_upgrade_) {
       active_request.request_decoder_->decodeHeaders(std::move(headers), false);
-
       // If the connection has been closed (or is closing) after decoding headers, pause the parser
       // so we return control to the caller.
       if (connection_.state() != Network::Connection::State::Open) {

source/common/http/http1/codec_impl.h

@@ -201,6 +201,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   }
   uint32_t bufferLimit() { return connection_.bufferLimit(); }
   virtual bool supportsHttp10() { return false; }
+  virtual bool allowChunkedLength() { return false; }
   bool maybeDirectDispatch(Buffer::Instance& data);
   virtual void maybeAddSentinelBufferFragment(Buffer::WatermarkBuffer&) {}
   CodecStats& stats() { return stats_; }
@@ -426,6 +427,7 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
                        envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
                            headers_with_underscores_action);
   bool supportsHttp10() override { return codec_settings_.accept_http_10_; }
+  bool allowChunkedLength() override { return codec_settings_.allow_chunked_length; }
 
 protected:
   /**

test/integration/integration_test.cc

@@ -425,7 +425,8 @@ TEST_P(IntegrationTest, TestSmuggling) {
                                 "identity,chunked \r\ncontent-length: 36\r\n\r\n" +
                                 smuggled_request;
     sendRawHttpAndWaitForResponse(lookupPort("http"), request.c_str(), &response, false);
-    EXPECT_THAT(response, HasSubstr("HTTP/1.1 400 Bad Request\r\n"));
+    // 'identity' encoding is not supported
+    EXPECT_THAT(response, HasSubstr("HTTP/1.1 501 Not Implemented\r\n"));
   }
 }