http/1.1 codec: Rejects requests w/ invalid HTTP header values (#7306)

Description: This PR addresses the behavior described in #7061 by modifying the HTTP/1.1 codec, such that it verifies the value of each HTTP header when onHeaderValue is called. The codec raises an exception, which results in a 400 response being returned if an invalid header value (per RFC 7230, section 3.2) is found. The actual validation is done via the nghttp2_check_header_value function, which is wrapped in Envoy::Http::HeaderUtility::headerIsValid. (NOTE: this has been discussed as something that nghttp2 could do itself, but the issue seems to have languished. Also note that Go handles this: Go uses the httpguts.ValidHeaderFieldValue function (which is analogous to nghttp2_check_header_value) to ensure that all the HTTP header values conform to the relevant RFC specs before an http.Transport instance will round-trip the request. Risk Level: Low/medium This stricter validation semantics are controlled with the envoy.reloadable_features.validate_header_values runtime-guarded feature. Originally, the PR used a new boolean on the HTTP connection manager proto to toggle the behavior, but during the course of PR review, it was decided that this would be more appropriate for a runtime guard. Testing: new unit tests, manual tests Release Notes: Updated Fixes #7061 Signed-off-by: Dylan Carney <dcarney@gmail.com>
envoyproxy · Jul 15, 2019 · 2e8ddf3 · 2e8ddf3
1 parent e442ba5
commit 2e8ddf3
Show file tree

Hide file tree

Showing 29 changed files with 261 additions and 52 deletions.
diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst
@@ -3,6 +3,7 @@ Version history
 
 1.12.0 (pending)
 ================
+* http: added the ability to reject HTTP/1.1 requests with invalid HTTP header values, using the runtime feature `envoy.reloadable_features.strict_header_validation`.
 
 1.11.0 (July 11, 2019)
 ======================

diff --git a/source/common/http/BUILD b/source/common/http/BUILD
@@ -142,6 +142,7 @@ envoy_cc_library(
         ":conn_manager_config_interface",
         ":exception_lib",
         ":header_map_lib",
+        ":header_utility_lib",
         ":headers_lib",
         ":path_utility_lib",
         ":user_agent_lib",
@@ -313,6 +314,9 @@ envoy_cc_library(
     name = "header_utility_lib",
     srcs = ["header_utility.cc"],
     hdrs = ["header_utility.h"],
+    external_deps = [
+        "nghttp2",
+    ],
     deps = [
         "//include/envoy/http:header_map_interface",
         "//include/envoy/json:json_object_interface",

diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc
@@ -136,11 +136,12 @@ void CodecClient::onData(Buffer::Instance& data) {
 
 CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& connection,
                                  Upstream::HostDescriptionConstSharedPtr host,
-                                 Event::Dispatcher& dispatcher)
+                                 Event::Dispatcher& dispatcher, bool strict_header_validation)
     : CodecClient(type, std::move(connection), host, dispatcher) {
   switch (type) {
   case Type::HTTP1: {
-    codec_ = std::make_unique<Http1::ClientConnectionImpl>(*connection_, *this);
+    codec_ = std::make_unique<Http1::ClientConnectionImpl>(*connection_, *this,
+                                                           strict_header_validation);
     break;
   }
   case Type::HTTP2: {

diff --git a/source/common/http/codec_client.h b/source/common/http/codec_client.h
@@ -241,7 +241,8 @@ using CodecClientPtr = std::unique_ptr<CodecClient>;
 class CodecClientProd : public CodecClient {
 public:
   CodecClientProd(Type type, Network::ClientConnectionPtr&& connection,
-                  Upstream::HostDescriptionConstSharedPtr host, Event::Dispatcher& dispatcher);
+                  Upstream::HostDescriptionConstSharedPtr host, Event::Dispatcher& dispatcher,
+                  bool strict_header_validation);
 };
 
 } // namespace Http

diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h
@@ -184,11 +184,15 @@ class ConnectionManagerConfig {
    * @param connection supplies the owning connection.
    * @param data supplies the currently available read data.
    * @param callbacks supplies the callbacks to install into the codec.
+   * @param strict_header_validation indicates whether or not the codec should validate the values
+   * of each HTTP header (NOTE: this argument only affects the H/1.1 codec; the H/2 codec always
+   * does this)
    * @return a codec or nullptr if no codec can be created.
    */
   virtual ServerConnectionPtr createCodec(Network::Connection& connection,
                                           const Buffer::Instance& data,
-                                          ServerConnectionCallbacks& callbacks) PURE;
+                                          ServerConnectionCallbacks& callbacks,
+                                          const bool strict_header_validation) PURE;
 
   /**
    * @return DateProvider& the date provider to use for

diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc
@@ -33,6 +33,7 @@
 #include "common/http/path_utility.h"
 #include "common/http/utility.h"
 #include "common/network/utility.h"
+#include "common/runtime/runtime_impl.h"
 
 #include "absl/strings/escaping.h"
 #include "absl/strings/match.h"
@@ -111,7 +112,8 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config,
           overload_manager ? overload_manager->getThreadLocalOverloadState().getState(
                                  Server::OverloadActionNames::get().DisableHttpKeepAlive)
                            : Server::OverloadManager::getInactiveState()),
-      time_source_(time_source) {}
+      time_source_(time_source), strict_header_validation_(Runtime::runtimeFeatureEnabled(
+                                     "envoy.reloadable_features.strict_header_validation")) {}
 
 const HeaderMapImpl& ConnectionManagerImpl::continueHeader() {
   CONSTRUCT_ON_FIRST_USE(HeaderMapImpl,
@@ -259,7 +261,8 @@ StreamDecoder& ConnectionManagerImpl::newStream(StreamEncoder& response_encoder,
 
 Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool) {
   if (!codec_) {
-    codec_ = config_.createCodec(read_callbacks_->connection(), data, *this);
+    codec_ =
+        config_.createCodec(read_callbacks_->connection(), data, *this, strict_header_validation_);
     if (codec_->protocol() == Protocol::Http2) {
       stats_.named_.downstream_cx_http2_total_.inc();
       stats_.named_.downstream_cx_http2_active_.inc();

diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h
@@ -678,6 +678,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
   const Server::OverloadActionState& overload_stop_accepting_requests_ref_;
   const Server::OverloadActionState& overload_disable_keepalive_ref_;
   TimeSource& time_source_;
+
+  const bool strict_header_validation_;
 };
 
 } // namespace Http

diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc
@@ -41,13 +41,14 @@ std::string ConnectionManagerUtility::determineNextProtocol(Network::Connection&
 ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec(
     Network::Connection& connection, const Buffer::Instance& data,
     ServerConnectionCallbacks& callbacks, Stats::Scope& scope, const Http1Settings& http1_settings,
-    const Http2Settings& http2_settings, const uint32_t max_request_headers_kb) {
+    const Http2Settings& http2_settings, const uint32_t max_request_headers_kb,
+    bool strict_header_validation) {
   if (determineNextProtocol(connection, data) == Http2::ALPN_STRING) {
     return std::make_unique<Http2::ServerConnectionImpl>(connection, callbacks, scope,
                                                          http2_settings, max_request_headers_kb);
   } else {
-    return std::make_unique<Http1::ServerConnectionImpl>(connection, callbacks, http1_settings,
-                                                         max_request_headers_kb);
+    return std::make_unique<Http1::ServerConnectionImpl>(
+        connection, callbacks, http1_settings, max_request_headers_kb, strict_header_validation);
   }
 }
 

diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h
@@ -37,7 +37,7 @@ class ConnectionManagerUtility {
   autoCreateCodec(Network::Connection& connection, const Buffer::Instance& data,
                   ServerConnectionCallbacks& callbacks, Stats::Scope& scope,
                   const Http1Settings& http1_settings, const Http2Settings& http2_settings,
-                  const uint32_t max_request_headers_kb);
+                  const uint32_t max_request_headers_kb, bool strict_header_validation);
 
   /**
    * Mutates request headers in various ways. This functionality is broken out because of its

diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc
@@ -6,6 +6,7 @@
 #include "common/protobuf/utility.h"
 
 #include "absl/strings/match.h"
+#include "nghttp2/nghttp2.h"
 
 namespace Envoy {
 namespace Http {
@@ -117,6 +118,11 @@ bool HeaderUtility::matchHeaders(const Http::HeaderMap& request_headers,
   return match != header_data.invert_match_;
 }
 
+bool HeaderUtility::headerIsValid(const absl::string_view header_value) {
+  return (nghttp2_check_header_value(reinterpret_cast<const uint8_t*>(header_value.data()),
+                                     header_value.size()) != 0);
+}
+
 void HeaderUtility::addHeaders(Http::HeaderMap& headers, const Http::HeaderMap& headers_to_add) {
   headers_to_add.iterate(
       [](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate {

diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h
@@ -45,6 +45,13 @@ class HeaderUtility {
 
   static bool matchHeaders(const Http::HeaderMap& request_headers, const HeaderData& config_header);
 
+  /**
+   * Validates that a header value is valid, according to RFC 7230, section 3.2.
+   * http://tools.ietf.org/html/rfc7230#section-3.2
+   * @return bool true if the header values are valid, according to the aforementioned RFC.
+   */
+  static bool headerIsValid(const absl::string_view header_value);
+
   /**
    * Add headers from one HeaderMap to another
    * @param headers target where headers will be added

diff --git a/source/common/http/http1/BUILD b/source/common/http/http1/BUILD
@@ -27,6 +27,7 @@ envoy_cc_library(
         "//source/common/http:codes_lib",
         "//source/common/http:exception_lib",
         "//source/common/http:header_map_lib",
+        "//source/common/http:header_utility_lib",
         "//source/common/http:headers_lib",
         "//source/common/http:utility_lib",
     ],
@@ -55,6 +56,7 @@ envoy_cc_library(
         "//source/common/http:conn_pool_base_lib",
         "//source/common/http:headers_lib",
         "//source/common/network:utility_lib",
+        "//source/common/runtime:runtime_lib",
         "//source/common/upstream:upstream_lib",
     ],
 )
diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc
@@ -13,6 +13,7 @@
 #include "common/common/stack_array.h"
 #include "common/common/utility.h"
 #include "common/http/exception.h"
+#include "common/http/header_utility.h"
 #include "common/http/headers.h"
 #include "common/http/utility.h"
 
@@ -319,11 +320,20 @@ const ToLowerTable& ConnectionImpl::toLowerTable() {
   return *table;
 }
 
+// TODO(alyssawilk) The overloaded constructors here and on {Client,Server}ConnectionImpl
+// can be cleaned up once "strict_header_validation" becomes the default behavior, rather
+// than a runtime-guarded one. The overloads were a workaround for the fact that Runtime
+// doesn't work from integration test call sites in scenarios where the required
+// thread-local storage is not available.
 ConnectionImpl::ConnectionImpl(Network::Connection& connection, http_parser_type type,
                                uint32_t max_headers_kb)
+    : ConnectionImpl::ConnectionImpl(connection, type, max_headers_kb, false) {}
+
+ConnectionImpl::ConnectionImpl(Network::Connection& connection, http_parser_type type,
+                               uint32_t max_headers_kb, bool strict_header_validation)
     : connection_(connection), output_buffer_([&]() -> void { this->onBelowLowWatermark(); },
                                               [&]() -> void { this->onAboveHighWatermark(); }),
-      max_headers_kb_(max_headers_kb) {
+      max_headers_kb_(max_headers_kb), strict_header_validation_(strict_header_validation) {
   output_buffer_.setWatermarks(connection.bufferLimit());
   http_parser_init(&parser_, type);
   parser_.data = this;
@@ -421,11 +431,21 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) {
     // Ignore trailers.
     return;
   }
-  // http-parser should filter for this
-  // (https://tools.ietf.org/html/rfc7230#section-3.2.6), but it doesn't today. HeaderStrings
-  // have an invariant that they must not contain embedded zero characters
-  // (NUL, ASCII 0x0).
-  if (absl::string_view(data, length).find('\0') != absl::string_view::npos) {
+
+  const absl::string_view header_value = absl::string_view(data, length);
+
+  if (strict_header_validation_) {
+    if (!Http::HeaderUtility::headerIsValid(header_value)) {
+      ENVOY_CONN_LOG(debug, "invalid header value: {}", connection_, header_value);
+      error_code_ = Http::Code::BadRequest;
+      sendProtocolError();
+      throw CodecProtocolException("http/1.1 protocol error: header value contains invalid chars");
+    }
+  } else if (header_value.find('\0') != absl::string_view::npos) {
+    // http-parser should filter for this
+    // (https://tools.ietf.org/html/rfc7230#section-3.2.6), but it doesn't today. HeaderStrings
+    // have an invariant that they must not contain embedded zero characters
+    // (NUL, ASCII 0x0).
     throw CodecProtocolException("http/1.1 protocol error: header value contains NUL");
   }
 
@@ -496,8 +516,15 @@ void ConnectionImpl::onResetStreamBase(StreamResetReason reason) {
 ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection,
                                            ServerConnectionCallbacks& callbacks,
                                            Http1Settings settings, uint32_t max_request_headers_kb)
-    : ConnectionImpl(connection, HTTP_REQUEST, max_request_headers_kb), callbacks_(callbacks),
-      codec_settings_(settings) {}
+    : ServerConnectionImpl::ServerConnectionImpl(connection, callbacks, settings,
+                                                 max_request_headers_kb, false) {}
+
+ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection,
+                                           ServerConnectionCallbacks& callbacks,
+                                           Http1Settings settings, uint32_t max_request_headers_kb,
+                                           bool strict_header_validation)
+    : ConnectionImpl(connection, HTTP_REQUEST, max_request_headers_kb, strict_header_validation),
+      callbacks_(callbacks), codec_settings_(settings) {}
 
 void ServerConnectionImpl::onEncodeComplete() {
   ASSERT(active_request_);
@@ -668,8 +695,14 @@ void ServerConnectionImpl::onBelowLowWatermark() {
   }
 }
 
-ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks&)
-    : ConnectionImpl(connection, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB) {}
+ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection,
+                                           ConnectionCallbacks& callbacks)
+    : ClientConnectionImpl::ClientConnectionImpl(connection, callbacks, false) {}
+
+ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks&,
+                                           bool strict_header_validation)
+    : ConnectionImpl(connection, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, strict_header_validation) {
+}
 
 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
@@ -174,6 +174,8 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
 protected:
   ConnectionImpl(Network::Connection& connection, http_parser_type type,
                  uint32_t max_request_headers_kb);
+  ConnectionImpl(Network::Connection& connection, http_parser_type type,
+                 uint32_t max_request_headers_kb, bool strict_header_validation);
 
   bool resetStreamCalled() { return reset_stream_called_; }
 
@@ -282,6 +284,8 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   char* reserved_current_{};
   Protocol protocol_{Protocol::Http11};
   const uint32_t max_headers_kb_;
+
+  bool strict_header_validation_;
 };
 
 /**
@@ -292,6 +296,10 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
   ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks,
                        Http1Settings settings, uint32_t max_request_headers_kb);
 
+  ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks,
+                       Http1Settings settings, uint32_t max_request_headers_kb,
+                       bool strict_header_validation);
+
   virtual bool supports_http_10() override { return codec_settings_.accept_http_10_; }
 
 private:
@@ -342,6 +350,9 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
 public:
   ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks& callbacks);
 
+  ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks& callbacks,
+                       bool strict_header_validation);
+
   // Http::ClientConnection
   StreamEncoder& newStream(StreamDecoder& response_decoder) override;
 

diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc
@@ -14,6 +14,7 @@
 #include "common/http/codes.h"
 #include "common/http/headers.h"
 #include "common/network/utility.h"
+#include "common/runtime/runtime_impl.h"
 #include "common/upstream/upstream_impl.h"
 
 #include "absl/strings/match.h"
@@ -341,8 +342,13 @@ void ConnPoolImpl::ActiveClient::onConnectTimeout() {
 }
 
 CodecClientPtr ProdConnPoolImpl::createCodecClient(Upstream::Host::CreateConnectionData& data) {
+
+  const bool strict_header_validation =
+      Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_header_validation");
+
   CodecClientPtr codec{new CodecClientProd(CodecClient::Type::HTTP1, std::move(data.connection_),
-                                           data.host_description_, dispatcher_)};
+                                           data.host_description_, dispatcher_,
+                                           strict_header_validation)};
   return codec;
 }
 

diff --git a/source/common/http/http2/conn_pool.cc b/source/common/http/http2/conn_pool.cc
@@ -304,7 +304,7 @@ ConnPoolImpl::ActiveClient::~ActiveClient() {
 
 CodecClientPtr ProdConnPoolImpl::createCodecClient(Upstream::Host::CreateConnectionData& data) {
   CodecClientPtr codec{new CodecClientProd(CodecClient::Type::HTTP2, std::move(data.connection_),
-                                           data.host_description_, dispatcher_)};
+                                           data.host_description_, dispatcher_, false)};
   return codec;
 }
 

diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc
@@ -12,6 +12,7 @@
 #include "common/http/header_map_impl.h"
 #include "common/network/address_impl.h"
 #include "common/router/router.h"
+#include "common/runtime/runtime_impl.h"
 #include "common/upstream/host_utility.h"
 
 // TODO(dio): Remove dependency to extension health checkers when redis_health_check is removed.
@@ -335,8 +336,10 @@ Http::CodecClient::Type HttpHealthCheckerImpl::codecClientType(bool use_http2) {
 
 Http::CodecClient*
 ProdHttpHealthCheckerImpl::createCodecClient(Upstream::Host::CreateConnectionData& data) {
+  const bool strict_header_validation =
+      Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_header_validation");
   return new Http::CodecClientProd(codec_client_type_, std::move(data.connection_),
-                                   data.host_description_, dispatcher_);
+                                   data.host_description_, dispatcher_, strict_header_validation);
 }
 
 TcpHealthCheckMatcher::MatchSegments TcpHealthCheckMatcher::loadProtoBytes(
@@ -744,7 +747,7 @@ Http::CodecClientPtr
 ProdGrpcHealthCheckerImpl::createCodecClient(Upstream::Host::CreateConnectionData& data) {
   return std::make_unique<Http::CodecClientProd>(Http::CodecClient::Type::HTTP2,
                                                  std::move(data.connection_),
-                                                 data.host_description_, dispatcher_);
+                                                 data.host_description_, dispatcher_, false);
 }
 
 std::ostream& operator<<(std::ostream& out, HealthState state) {