CVE-2019-15226-for-istio-envoy-release-1-1

Tracking header size and add limit for the number of headers

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

api/envoy/api/v2/cds.proto

@@ -232,8 +232,8 @@ message Cluster {
 
   reserved 12;
 
-  // Additional options when handling HTTP requests. These options will be applicable to both
-  // HTTP1 and HTTP2 requests.
+  // Additional options when handling HTTP requests upstream. These options will be applicable to
+  // both HTTP1 and HTTP2 requests.
   core.HttpProtocolOptions common_http_protocol_options = 29;
 
   // Additional options when handling HTTP1 requests.

api/envoy/api/v2/core/protocol.proto

@@ -23,11 +23,19 @@ message TcpProtocolOptions {
 }
 
 message HttpProtocolOptions {
-  // The idle timeout for upstream connection pool connections. The idle timeout is defined as the
+  // The idle timeout for connections. The idle timeout is defined as the
   // period in which there are no active requests. If not set, there is no idle timeout. When the
-  // idle timeout is reached the connection will be closed. Note that request based timeouts mean
-  // that HTTP/2 PINGs will not keep the connection alive.
+  // idle timeout is reached the connection will be closed. If the connection is an HTTP/2
+  // downstream connection a drain sequence will occur prior to closing the connection, see
+  // :ref:`drain_timeout
+  // <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.drain_timeout>`.
+  // Note that request based timeouts mean that HTTP/2 PINGs will not keep the connection alive.
   google.protobuf.Duration idle_timeout = 1 [(gogoproto.stdduration) = true];
+
+  // The maximum number of headers. If unconfigured, the default
+  // maximum number of request headers allowed is 100. Requests that exceed this limit will receive
+  // a 431 response for HTTP/1.x and cause a stream reset for HTTP/2.
+  google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}];
 }
 
 message Http1ProtocolOptions {

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

@@ -24,7 +24,7 @@ import "gogoproto/gogo.proto";
 // [#protodoc-title: HTTP connection manager]
 // HTTP connection manager :ref:`configuration overview <config_http_conn_man>`.
 
-// [#comment:next free field: 31]
+// [#comment:next free field: 36]
 message HttpConnectionManager {
   enum CodecType {
     option (gogoproto.goproto_enum_prefix) = false;
@@ -127,6 +127,10 @@ message HttpConnectionManager {
   // <envoy_api_msg_config.trace.v2.Tracing>`.
   Tracing tracing = 7;
 
+  // Additional settings for HTTP requests handled by the connection manager. These will be
+  // applicable to both HTTP1 and HTTP2 requests.
+  envoy.api.v2.core.HttpProtocolOptions common_http_protocol_options = 35;
+
   // Additional HTTP/1 settings that are passed to the HTTP/1 codec.
   envoy.api.v2.core.Http1ProtocolOptions http_protocol_options = 8;
 
@@ -149,10 +153,11 @@ message HttpConnectionManager {
   // idle timeout is defined as the period in which there are no active
   // requests. If not set, there is no idle timeout. When the idle timeout is
   // reached the connection will be closed. If the connection is an HTTP/2
-  // connection a drain sequence will occur prior to closing the connection. See
-  // :ref:`drain_timeout
-  // <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.drain_timeout>`.
-  google.protobuf.Duration idle_timeout = 11 [(gogoproto.stdduration) = true];
+  // connection a drain sequence will occur prior to closing the connection.
+  // This field is deprecated. Use :ref:`idle_timeout
+  // <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.common_http_protocol_options>`
+  // instead.
+  google.protobuf.Duration idle_timeout = 11 [(gogoproto.stdduration) = true, deprecated = true];
 
   // The stream idle timeout for connections managed by the connection manager.
   // If not specified, this defaults to 5 minutes. The default value was selected

include/envoy/http/codec.h

@@ -15,6 +15,13 @@ namespace Http {
 
 // Legacy default value of 60K is safely under both codec default limits.
 static const uint32_t DEFAULT_MAX_REQUEST_HEADERS_KB = 60;
+// Default maximum number of headers.
+static const uint32_t DEFAULT_MAX_HEADERS_COUNT = 100;
+
+const char MaxRequestHeadersCountOverrideKey[] =
+    "envoy.reloadable_features.max_request_headers_count";
+const char MaxResponseHeadersCountOverrideKey[] =
+    "envoy.reloadable_features.max_response_headers_count";
 
 class Stream;
 

include/envoy/http/header_map.h

@@ -445,9 +445,41 @@ class HeaderMap {
   virtual void setReferenceKey(const LowerCaseString& key, const std::string& value) PURE;
 
   /**
+   * HeaderMap contains an internal byte size count, updated as entries are added, removed, or
+   * modified through the HeaderMap interface. However, HeaderEntries can be accessed and modified
+   * by reference so that the HeaderMap can no longer accurately update the internal byte size
+   * count.
+   *
+   * Calling byteSize before a HeaderEntry is accessed will return the internal byte size count. The
+   * value is cleared when a HeaderEntry is accessed, and the value is updated and set again when
+   * refreshByteSize is called.
+   *
+   * To guarantee an accurate byte size count, call refreshByteSize.
+   *
+   * @return uint64_t the approximate size of the header map in bytes if valid.
+   */
+  virtual absl::optional<uint64_t> byteSize() const PURE;
+
+  /**
+   * This returns the sum of the byte sizes of the keys and values in the HeaderMap. This also
+   * updates and sets the byte size count.
+   *
+   * To guarantee an accurate byte size count, use this. If it is known HeaderEntries have not been
+   * manipulated since a call to refreshByteSize, it is safe to use byteSize.
+   *
+   * @return uint64_t the approximate size of the header map in bytes.
+   */
+  virtual uint64_t refreshByteSize() PURE;
+
+  /**
+   * This returns the sum of the byte sizes of the keys and values in the HeaderMap.
+   *
+   * This iterates over the HeaderMap to calculate size and should only be called directly when the
+   * user wants an explicit recalculation of the byte size.
+   *
    * @return uint64_t the approximate size of the header map in bytes.
    */
-  virtual uint64_t byteSize() const PURE;
+  virtual uint64_t byteSizeInternal() const PURE;
 
   /**
    * Get a header by key.

include/envoy/upstream/upstream.h

@@ -694,6 +694,12 @@ class ClusterInfo {
    */
   virtual uint64_t maxRequestsPerConnection() const PURE;
 
+  /**
+   * @return uint32_t the maximum number of response headers. The default value is 100. Results in a
+   * reset if the number of headers exceeds this value.
+   */
+  virtual uint32_t maxResponseHeadersCount() const PURE;
+
   /**
    * @return the human readable name of the cluster.
    */

source/common/http/codec_client.cc

@@ -140,13 +140,14 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne
     : 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, host->cluster().maxResponseHeadersCount());
     break;
   }
   case Type::HTTP2: {
     codec_ = std::make_unique<Http2::ClientConnectionImpl>(
         *connection_, *this, host->cluster().statsScope(), host->cluster().http2Settings(),
-        Http::DEFAULT_MAX_REQUEST_HEADERS_KB);
+        Http::DEFAULT_MAX_REQUEST_HEADERS_KB, host->cluster().maxResponseHeadersCount());
     break;
   }
   }

source/common/http/conn_manager_config.h

@@ -228,6 +228,11 @@ class ConnectionManagerConfig {
    */
   virtual uint32_t maxRequestHeadersKb() const PURE;
 
+  /**
+   * @return maximum number of request headers the codecs will accept.
+   */
+  virtual uint32_t maxRequestHeadersCount() const PURE;
+
   /**
    * @return per-stream idle timeout for incoming connection manager connections. Zero indicates a
    *         disabled idle timeout.

source/common/http/conn_manager_impl.cc

@@ -469,6 +469,18 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() {
   }
 
   connection_manager_.stats_.named_.downstream_rq_active_.dec();
+  // Refresh byte sizes of the HeaderMaps before logging.
+  // TODO(asraa): Remove this when entries in HeaderMap can no longer be modified by reference and
+  // HeaderMap holds an accurate internal byte size count.
+  if (request_headers_ != nullptr) {
+    request_headers_->refreshByteSize();
+  }
+  if (response_headers_ != nullptr) {
+    response_headers_->refreshByteSize();
+  }
+  if (response_trailers_ != nullptr) {
+    response_trailers_->refreshByteSize();
+  }
   for (const AccessLog::InstanceSharedPtr& access_log : connection_manager_.config_.accessLogs()) {
     access_log->log(request_headers_.get(), response_headers_.get(), response_trailers_.get(),
                     stream_info_);

source/common/http/conn_manager_utility.cc

@@ -40,13 +40,15 @@ 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,
+    const uint32_t max_request_headers_count) {
   if (determineNextProtocol(connection, data) == Http2::ALPN_STRING) {
     return std::make_unique<Http2::ServerConnectionImpl>(connection, callbacks, scope,
-                                                         http2_settings, max_request_headers_kb);
+                                                         http2_settings, max_request_headers_kb,
+                                                         max_request_headers_count);
   } 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, max_request_headers_count);
   }
 }
 

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);
+                  uint32_t max_request_headers_kb, uint32_t max_request_headers_count);
 
   /**
    * Mutates request headers in various ways. This functionality is broken out because of its

source/common/http/header_map_impl.cc

@@ -295,14 +295,17 @@ struct HeaderMapImpl::StaticLookupTable : public TrieLookupTable<EntryCb> {
   }
 };
 
-void HeaderMapImpl::appendToHeader(HeaderString& header, absl::string_view data) {
+uint64_t HeaderMapImpl::appendToHeader(HeaderString& header, absl::string_view data) {
   if (data.empty()) {
-    return;
+    return 0;
   }
+  uint64_t byte_size = 0;
   if (!header.empty()) {
     header.append(",", 1);
+    byte_size += 1;
   }
   header.append(data.data(), data.size());
+  return data.size() + byte_size;
 }
 
 HeaderMapImpl::HeaderMapImpl() { memset(&inline_headers_, 0, sizeof(inline_headers_)); }
@@ -319,6 +322,20 @@ HeaderMapImpl::HeaderMapImpl(
   }
 }
 
+void HeaderMapImpl::addSize(uint64_t size) {
+  // Adds size to cached_byte_size_ if it exists.
+  if (cached_byte_size_.has_value()) {
+    cached_byte_size_.value() += size;
+  }
+}
+
+void HeaderMapImpl::subtractSize(uint64_t size) {
+  if (cached_byte_size_.has_value()) {
+    ASSERT(cached_byte_size_ >= size);
+    cached_byte_size_.value() -= size;
+  }
+}
+
 void HeaderMapImpl::copyFrom(const HeaderMap& header_map) {
   header_map.iterate(
       [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate {
@@ -359,10 +376,13 @@ void HeaderMapImpl::insertByKey(HeaderString&& key, HeaderString&& value) {
     if (*ref_lookup_response.entry_ == nullptr) {
       maybeCreateInline(ref_lookup_response.entry_, *ref_lookup_response.key_, std::move(value));
     } else {
-      appendToHeader((*ref_lookup_response.entry_)->value(), value.c_str());
+      const uint64_t added_size =
+          appendToHeader((*ref_lookup_response.entry_)->value(), value.getStringView());
+      addSize(added_size);
       value.clear();
     }
   } else {
+    addSize(key.size() + value.size());
     std::list<HeaderEntryImpl>::iterator i = headers_.insert(std::move(key), std::move(value));
     i->entry_ = i;
   }
@@ -373,7 +393,8 @@ void HeaderMapImpl::addViaMove(HeaderString&& key, HeaderString&& value) {
   // the existing value.
   auto* entry = getExistingInline(key.c_str());
   if (entry != nullptr) {
-    appendToHeader(entry->value(), value.c_str());
+    const uint64_t added_size = appendToHeader(entry->value(), value.getStringView());
+    addSize(added_size);
     key.clear();
     value.clear();
   } else {
@@ -408,7 +429,8 @@ void HeaderMapImpl::addCopy(const LowerCaseString& key, uint64_t value) {
   if (entry != nullptr) {
     char buf[32];
     StringUtil::itoa(buf, sizeof(buf), value);
-    appendToHeader(entry->value(), buf);
+    const uint64_t added_size = appendToHeader(entry->value(), buf);
+    addSize(added_size);
     return;
   }
   HeaderString new_key;
@@ -423,7 +445,8 @@ void HeaderMapImpl::addCopy(const LowerCaseString& key, uint64_t value) {
 void HeaderMapImpl::addCopy(const LowerCaseString& key, const std::string& value) {
   auto* entry = getExistingInline(key.get().c_str());
   if (entry != nullptr) {
-    appendToHeader(entry->value(), value);
+    const uint64_t added_size = appendToHeader(entry->value(), value);
+    addSize(added_size);
     return;
   }
   HeaderString new_key;
@@ -451,13 +474,24 @@ void HeaderMapImpl::setReferenceKey(const LowerCaseString& key, const std::strin
   ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move)
 }
 
-uint64_t HeaderMapImpl::byteSize() const {
+absl::optional<uint64_t> HeaderMapImpl::byteSize() const { return cached_byte_size_; }
+
+uint64_t HeaderMapImpl::refreshByteSize() {
+  if (!cached_byte_size_.has_value()) {
+    // In this case, the cached byte size is not valid, and the byte size is computed via an
+    // iteration over the HeaderMap. The cached byte size is updated.
+    cached_byte_size_ = byteSizeInternal();
+  }
+  return cached_byte_size_.value();
+}
+
+uint64_t HeaderMapImpl::byteSizeInternal() const {
+  // Computes the total byte size by summing the byte size of the keys and values.
   uint64_t byte_size = 0;
   for (const HeaderEntryImpl& header : headers_) {
     byte_size += header.key().size();
     byte_size += header.value().size();
   }
-
   return byte_size;
 }
 
@@ -474,6 +508,7 @@ const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const {
 HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) {
   for (HeaderEntryImpl& header : headers_) {
     if (header.key() == key.get().c_str()) {
+      cached_byte_size_.reset();
       return &header;
     }
   }
@@ -528,6 +563,7 @@ void HeaderMapImpl::remove(const LowerCaseString& key) {
   } else {
     for (auto i = headers_.begin(); i != headers_.end();) {
       if (i->key() == key.get().c_str()) {
+        subtractSize(i->key().size() + i->value().size());
         i = headers_.erase(i);
       } else {
         ++i;
@@ -537,7 +573,7 @@ void HeaderMapImpl::remove(const LowerCaseString& key) {
 }
 
 void HeaderMapImpl::removePrefix(const LowerCaseString& prefix) {
-  headers_.remove_if([&](const HeaderEntryImpl& entry) {
+  headers_.remove_if([&prefix, this](const HeaderEntryImpl& entry) {
     bool to_remove = absl::StartsWith(entry.key().getStringView(), prefix.get());
     if (to_remove) {
       // If this header should be removed, make sure any references in the
@@ -546,8 +582,13 @@ void HeaderMapImpl::removePrefix(const LowerCaseString& prefix) {
       if (cb) {
         StaticLookupResponse ref_lookup_response = cb(*this);
         if (ref_lookup_response.entry_) {
+          const uint32_t key_value_size = (*ref_lookup_response.entry_)->key().size() +
+                                          (*ref_lookup_response.entry_)->value().size();
+          subtractSize(key_value_size);
           *ref_lookup_response.entry_ = nullptr;
         }
+      } else {
+        subtractSize(entry.key().size() + entry.value().size());
       }
     }
     return to_remove;
@@ -556,6 +597,7 @@ void HeaderMapImpl::removePrefix(const LowerCaseString& prefix) {
 
 HeaderMapImpl::HeaderEntryImpl& HeaderMapImpl::maybeCreateInline(HeaderEntryImpl** entry,
                                                                  const LowerCaseString& key) {
+  cached_byte_size_.reset();
   if (*entry) {
     return **entry;
   }
@@ -574,6 +616,7 @@ HeaderMapImpl::HeaderEntryImpl& HeaderMapImpl::maybeCreateInline(HeaderEntryImpl
     return **entry;
   }
 
+  addSize(key.get().size() + value.size());
   std::list<HeaderEntryImpl>::iterator i = headers_.insert(key, std::move(value));
   i->entry_ = i;
   *entry = &(*i);
@@ -595,6 +638,8 @@ void HeaderMapImpl::removeInline(HeaderEntryImpl** ptr_to_entry) {
   }
 
   HeaderEntryImpl* entry = *ptr_to_entry;
+  const uint64_t size_to_subtract = entry->entry_->key().size() + entry->entry_->value().size();
+  subtractSize(size_to_subtract);
   *ptr_to_entry = nullptr;
   headers_.erase(entry->entry_);
 }