codec: Raise max_request_headers_kb limit to 96 KiB (envoyproxy#5859)

Bump up max configurable max_request_headers_kb to 96 KiB. Add a check to http1/codec_impl.cc for headers size. Raise the default library limits in http_parser nghttp2 so we'll rely on our own codec check. Risk Level: Medium. Testing: Moved all the large request headers tests to ProtocolIntegrationTest. Part of envoyproxy#5626. Signed-off-by: Auni Ahsan <[email protected]>
jianjin · Mar 5, 2019 · df3d47f · df3d47f
1 parent a957da5
commit df3d47f
Show file tree

Hide file tree

Showing 21 changed files with 270 additions and 63 deletions.
diff --git a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto
@@ -137,14 +137,13 @@ message HttpConnectionManager {
   // header in responses. If not set, the default is *envoy*.
   string server_name = 10;
 
-  // The maximum request headers size for incoming connections. The default max
-  // is 60K, based on default settings for http codecs. For HTTP1, the current
-  // limit set by http_parser is 80K. for HTTP2, the default allowed header
-  // block in nghttp2 is 64K. The max configurable setting is 64K in order to
-  // stay under both codec limits.
-  // Requests that exceed this size will receive a 431 response.
+  // The maximum request headers size for incoming connections.
+  // If unconfigured, the default max request headers allowed is 60 KiB.
+  // Requests that exceed this limit will receive a 431 response.
+  // The max configurable limit is 96 KiB, based on current implementation
+  // constraints.
   google.protobuf.UInt32Value max_request_headers_kb = 29
-      [(validate.rules).uint32.gt = 0, (validate.rules).uint32.lte = 64];
+      [(validate.rules).uint32.gt = 0, (validate.rules).uint32.lte = 96];
 
   // The idle timeout for connections managed by the connection manager. The
   // idle timeout is defined as the period in which there are no active

diff --git a/bazel/external/http-parser.BUILD b/bazel/external/http-parser.BUILD
@@ -7,6 +7,10 @@ cc_library(
         "http_parser.h",
     ],
     hdrs = ["http_parser.h"],
+    # This compiler flag is set to an arbtitrarily high number so
+    # as to effectively disables the http_parser header limit, as
+    # we do our own checks in the conn manager and codec.
+    copts = ["-DHTTP_MAX_HEADER_SIZE=0x2000000"],
     includes = ["."],
     visibility = ["//visibility:public"],
 )
diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc
@@ -41,11 +41,11 @@ ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec(
     ServerConnectionCallbacks& callbacks, Stats::Scope& scope, const Http1Settings& http1_settings,
     const Http2Settings& http2_settings, const uint32_t max_request_headers_kb) {
   if (determineNextProtocol(connection, data) == Http2::ALPN_STRING) {
-    return ServerConnectionPtr{new Http2::ServerConnectionImpl(
-        connection, callbacks, scope, http2_settings, max_request_headers_kb)};
+    return std::make_unique<Http2::ServerConnectionImpl>(connection, callbacks, scope,
+                                                         http2_settings, max_request_headers_kb);
   } else {
-    return ServerConnectionPtr{
-        new Http1::ServerConnectionImpl(connection, callbacks, http1_settings)};
+    return std::make_unique<Http1::ServerConnectionImpl>(connection, callbacks, http1_settings,
+                                                         max_request_headers_kb);
   }
 }
 

diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc
@@ -316,9 +316,11 @@ const ToLowerTable& ConnectionImpl::toLowerTable() {
   return *table;
 }
 
-ConnectionImpl::ConnectionImpl(Network::Connection& connection, http_parser_type type)
+ConnectionImpl::ConnectionImpl(Network::Connection& connection, http_parser_type type,
+                               uint32_t max_headers_kb)
     : connection_(connection), output_buffer_([&]() -> void { this->onBelowLowWatermark(); },
-                                              [&]() -> void { this->onAboveHighWatermark(); }) {
+                                              [&]() -> void { this->onAboveHighWatermark(); }),
+      max_headers_kb_(max_headers_kb) {
   output_buffer_.setWatermarks(connection.bufferLimit());
   http_parser_init(&parser_, type);
   parser_.data = this;
@@ -419,6 +421,14 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) {
 
   header_parsing_state_ = HeaderParsingState::Value;
   current_header_value_.append(data, length);
+
+  const uint32_t total =
+      current_header_field_.size() + current_header_value_.size() + current_header_map_->byteSize();
+  if (total > (max_headers_kb_ * 1024)) {
+    error_code_ = Http::Code::RequestHeaderFieldsTooLarge;
+    sendProtocolError();
+    throw CodecProtocolException("headers size exceeds limit");
+  }
 }
 
 int ConnectionImpl::onHeadersCompleteBase() {
@@ -471,8 +481,9 @@ void ConnectionImpl::onResetStreamBase(StreamResetReason reason) {
 
 ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection,
                                            ServerConnectionCallbacks& callbacks,
-                                           Http1Settings settings)
-    : ConnectionImpl(connection, HTTP_REQUEST), callbacks_(callbacks), codec_settings_(settings) {}
+                                           Http1Settings settings, uint32_t max_request_headers_kb)
+    : ConnectionImpl(connection, HTTP_REQUEST, max_request_headers_kb), callbacks_(callbacks),
+      codec_settings_(settings) {}
 
 void ServerConnectionImpl::onEncodeComplete() {
   ASSERT(active_request_);
@@ -643,7 +654,7 @@ void ServerConnectionImpl::onBelowLowWatermark() {
 }
 
 ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks&)
-    : ConnectionImpl(connection, HTTP_RESPONSE) {}
+    : ConnectionImpl(connection, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB) {}
 
 bool ClientConnectionImpl::cannotHaveBody() {
   if ((!pending_responses_.empty() && pending_responses_.front().head_request_) ||

diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h
@@ -165,7 +165,8 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   bool maybeDirectDispatch(Buffer::Instance& data);
 
 protected:
-  ConnectionImpl(Network::Connection& connection, http_parser_type type);
+  ConnectionImpl(Network::Connection& connection, http_parser_type type,
+                 uint32_t max_request_headers_kb);
 
   bool resetStreamCalled() { return reset_stream_called_; }
 
@@ -273,6 +274,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   Buffer::RawSlice reserved_iovec_;
   char* reserved_current_{};
   Protocol protocol_{Protocol::Http11};
+  const uint32_t max_headers_kb_;
 };
 
 /**
@@ -281,7 +283,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
 class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
 public:
   ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks,
-                       Http1Settings settings);
+                       Http1Settings settings, uint32_t max_request_headers_kb);
 
   virtual bool supports_http_10() override { return codec_settings_.accept_http_10_; }
 
@@ -363,6 +365,9 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
   std::list<PendingResponse> pending_responses_;
   // Set true between receiving 100-Continue headers and receiving the spurious onMessageComplete.
   bool ignore_message_complete_for_100_continue_{};
+
+  // The default limit of 80 KiB is the vanilla http_parser behaviour.
+  static constexpr uint32_t MAX_RESPONSE_HEADERS_KB = 80;
 };
 
 } // namespace Http1

diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc
@@ -868,6 +868,10 @@ ConnectionImpl::Http2Options::Http2Options(const Http2Settings& http2_settings)
   nghttp2_option_set_no_closed_streams(options_, 1);
   nghttp2_option_set_no_auto_window_update(options_, 1);
 
+  // The max send header block length is configured to an arbitrarily high number so as to never
+  // trigger the check within nghttp2, as we check request headers length in codec_impl::saveHeader.
+  nghttp2_option_set_max_send_header_block_length(options_, 0x2000000);
+
   if (http2_settings.hpack_table_size_ != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) {
     nghttp2_option_set_max_deflate_dynamic_table_size(options_, http2_settings.hpack_table_size_);
   }

diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc
@@ -321,11 +321,11 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection,
                                          Http::ServerConnectionCallbacks& callbacks) {
   switch (codec_type_) {
   case CodecType::HTTP1:
-    return Http::ServerConnectionPtr{
-        new Http::Http1::ServerConnectionImpl(connection, callbacks, http1_settings_)};
+    return std::make_unique<Http::Http1::ServerConnectionImpl>(
+        connection, callbacks, http1_settings_, maxRequestHeadersKb());
   case CodecType::HTTP2:
-    return Http::ServerConnectionPtr{new Http::Http2::ServerConnectionImpl(
-        connection, callbacks, context_.scope(), http2_settings_, maxRequestHeadersKb())};
+    return std::make_unique<Http::Http2::ServerConnectionImpl>(
+        connection, callbacks, context_.scope(), http2_settings_, maxRequestHeadersKb());
   case CodecType::AUTO:
     return Http::ConnectionManagerUtility::autoCreateCodec(connection, data, callbacks,
                                                            context_.scope(), http1_settings_,

diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc
@@ -369,8 +369,8 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi
                                                                 max_request_headers_kb);
   } else {
     const Http1Settings server_http1settings{fromHttp1Settings(input.h1_settings().server())};
-    server = absl::make_unique<Http1::ServerConnectionImpl>(server_connection, server_callbacks,
-                                                            server_http1settings);
+    server = absl::make_unique<Http1::ServerConnectionImpl>(
+        server_connection, server_callbacks, server_http1settings, max_request_headers_kb);
   }
 
   ReorderBuffer client_write_buf{*server};

diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc
@@ -31,7 +31,8 @@ namespace Http1 {
 class Http1ServerConnectionImplTest : public testing::Test {
 public:
   void initialize() {
-    codec_ = std::make_unique<ServerConnectionImpl>(connection_, callbacks_, codec_settings_);
+    codec_ = std::make_unique<ServerConnectionImpl>(connection_, callbacks_, codec_settings_,
+                                                    max_request_headers_kb_);
   }
 
   NiceMock<Network::MockConnection> connection_;
@@ -42,6 +43,9 @@ class Http1ServerConnectionImplTest : public testing::Test {
   void expectHeadersTest(Protocol p, bool allow_absolute_url, Buffer::OwnedImpl& buffer,
                          TestHeaderMapImpl& expected_headers);
   void expect400(Protocol p, bool allow_absolute_url, Buffer::OwnedImpl& buffer);
+
+protected:
+  uint32_t max_request_headers_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_KB};
 };
 
 void Http1ServerConnectionImplTest::expect400(Protocol p, bool allow_absolute_url,
@@ -53,7 +57,8 @@ void Http1ServerConnectionImplTest::expect400(Protocol p, bool allow_absolute_ur
 
   if (allow_absolute_url) {
     codec_settings_.allow_absolute_url_ = allow_absolute_url;
-    codec_ = std::make_unique<ServerConnectionImpl>(connection_, callbacks_, codec_settings_);
+    codec_ = std::make_unique<ServerConnectionImpl>(connection_, callbacks_, codec_settings_,
+                                                    max_request_headers_kb_);
   }
 
   Http::MockStreamDecoder decoder;
@@ -72,7 +77,8 @@ void Http1ServerConnectionImplTest::expectHeadersTest(Protocol p, bool allow_abs
   // Make a new 'codec' with the right settings
   if (allow_absolute_url) {
     codec_settings_.allow_absolute_url_ = allow_absolute_url;
-    codec_ = std::make_unique<ServerConnectionImpl>(connection_, callbacks_, codec_settings_);
+    codec_ = std::make_unique<ServerConnectionImpl>(connection_, callbacks_, codec_settings_,
+                                                    max_request_headers_kb_);
   }
 
   Http::MockStreamDecoder decoder;
@@ -1006,9 +1012,8 @@ TEST_F(Http1ClientConnectionImplTest, HighwatermarkMultipleResponses) {
   static_cast<ClientConnection*>(codec_.get())
       ->onUnderlyingConnectionBelowWriteBufferLowWatermark();
 }
-
-// For issue #1421 regression test that Envoy's HTTP parser applies header limits early.
-TEST_F(Http1ServerConnectionImplTest, TestCodecHeaderLimits) {
+TEST_F(Http1ServerConnectionImplTest, TestLargeRequestHeadersRejected) {
+  // Default limit of 60 KiB
   initialize();
 
   std::string exception_reason;
@@ -1022,14 +1027,102 @@ TEST_F(Http1ServerConnectionImplTest, TestCodecHeaderLimits) {
 
   Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n");
   codec_->dispatch(buffer);
-  std::string long_string = "foo: " + std::string(1024, 'q') + "\r\n";
-  for (int i = 0; i < 79; ++i) {
-    buffer = Buffer::OwnedImpl(long_string);
+  std::string long_string = "big: " + std::string(60 * 1024, 'q') + "\r\n";
+  buffer = Buffer::OwnedImpl(long_string);
+  EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), EnvoyException, "headers size exceeds limit");
+}
+
+TEST_F(Http1ServerConnectionImplTest, TestLargeRequestHeadersSplitRejected) {
+  // Default limit of 60 KiB
+  initialize();
+
+  std::string exception_reason;
+  NiceMock<Http::MockStreamDecoder> decoder;
+  Http::StreamEncoder* response_encoder = nullptr;
+  EXPECT_CALL(callbacks_, newStream(_, _))
+      .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& {
+        response_encoder = &encoder;
+        return decoder;
+      }));
+  Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n");
+  codec_->dispatch(buffer);
+
+  std::string long_string = std::string(1024, 'q');
+  for (int i = 0; i < 59; i++) {
+    buffer = Buffer::OwnedImpl(fmt::format("big: {}\r\n", long_string));
     codec_->dispatch(buffer);
   }
+  // the 60th 1kb header should induce overflow
+  buffer = Buffer::OwnedImpl(fmt::format("big: {}\r\n", long_string));
+  EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), EnvoyException, "headers size exceeds limit");
+}
+
+TEST_F(Http1ServerConnectionImplTest, TestLargeRequestHeadersAccepted) {
+  max_request_headers_kb_ = 65;
+  initialize();
+
+  NiceMock<Http::MockStreamDecoder> decoder;
+  Http::StreamEncoder* response_encoder = nullptr;
+  EXPECT_CALL(callbacks_, newStream(_, _))
+      .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& {
+        response_encoder = &encoder;
+        return decoder;
+      }));
+
+  Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n");
+  codec_->dispatch(buffer);
+  std::string long_string = "big: " + std::string(64 * 1024, 'q') + "\r\n";
+  buffer = Buffer::OwnedImpl(long_string);
+  codec_->dispatch(buffer);
+}
+
+TEST_F(Http1ServerConnectionImplTest, TestLargeRequestHeadersAcceptedMaxConfigurable) {
+  max_request_headers_kb_ = 96;
+  initialize();
+
+  NiceMock<Http::MockStreamDecoder> decoder;
+  Http::StreamEncoder* response_encoder = nullptr;
+  EXPECT_CALL(callbacks_, newStream(_, _))
+      .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& {
+        response_encoder = &encoder;
+        return decoder;
+      }));
+
+  Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n");
+  codec_->dispatch(buffer);
+  std::string long_string = "big: " + std::string(95 * 1024, 'q') + "\r\n";
   buffer = Buffer::OwnedImpl(long_string);
-  EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), EnvoyException,
-                            "http/1.1 protocol error: HPE_HEADER_OVERFLOW");
+  codec_->dispatch(buffer);
+}
+
+TEST_F(Http1ClientConnectionImplTest, TestLargeResponseHeadersRejected) {
+  initialize();
+
+  NiceMock<Http::MockStreamDecoder> response_decoder;
+  Http::StreamEncoder& request_encoder = codec_->newStream(response_decoder);
+  TestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}};
+  request_encoder.encodeHeaders(headers, true);
+
+  Buffer::OwnedImpl buffer("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n");
+  codec_->dispatch(buffer);
+  std::string long_header = "big: " + std::string(80 * 1024, 'q') + "\r\n";
+  buffer = Buffer::OwnedImpl(long_header);
+  EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), EnvoyException, "headers size exceeds limit");
+}
+
+TEST_F(Http1ClientConnectionImplTest, TestLargeResponseHeadersAccepted) {
+  initialize();
+
+  NiceMock<Http::MockStreamDecoder> response_decoder;
+  Http::StreamEncoder& request_encoder = codec_->newStream(response_decoder);
+  TestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}};
+  request_encoder.encodeHeaders(headers, true);
+
+  Buffer::OwnedImpl buffer("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n");
+  codec_->dispatch(buffer);
+  std::string long_header = "big: " + std::string(79 * 1024, 'q') + "\r\n";
+  buffer = Buffer::OwnedImpl(long_header);
+  codec_->dispatch(buffer);
 }
 
 } // namespace Http1