[1.12] http: header map security fixes for duplicate headers (#197) (#…

…204)

Previously header matching did not match on all headers for
non-inline headers. This patch changes the default behavior to
always logically match on all headers. Multiple individual
headers will be logically concatenated with ',' similar to what
is done with inline headers. This makes the behavior effectively
consistent. This behavior can be temporary reverted by setting
the runtime value "envoy.reloadable_features.header_match_on_all_headers"
to "false".

Targeted fixes have been additionally performed on the following
extensions which make them consider all duplicate headers by default as
a comma concatenated list:
1) Any extension using CEL matching on headers.
2) The header to metadata filter.
3) The JWT filter.
4) The Lua filter.
Like primary header matching used in routing, RBAC, etc. this behavior
can be disabled by setting the runtime value
"envoy.reloadable_features.header_match_on_all_headers" to false.

Finally, the setCopy() header map API previously only set the first
header in the case of duplicate non-inline headers. setCopy() now
behaves similiarly to the other set*() APIs and replaces all found
headers with a single value. This may have had security implications
in the extauth filter which uses this API. This behavior can be disabled
by setting the runtime value
"envoy.reloadable_features.http_set_copy_replace_all_headers" to false.

Fixes https://github.com/envoyproxy/envoy-setec/issues/188

Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

docs/root/intro/version_history.rst

@@ -3,6 +3,24 @@ Version history
 
 1.12.7 (Pending)
 ================
+Changes
+-------
+* http: fixed CVE-2020-25017. Previously header matching did not match on all headers for non-inline headers. This patch
+  changes the default behavior to always logically match on all headers. Multiple individual
+  headers will be logically concatenated with ',' similar to what is done with inline headers. This
+  makes the behavior effectively consistent. This behavior can be temporary reverted by setting
+  the runtime value "envoy.reloadable_features.header_match_on_all_headers" to "false".
+
+  Targeted fixes have been additionally performed on the following extensions which make them
+  consider all duplicate headers by default as a comma concatenated list:
+
+    1. Any extension using CEL matching on headers.
+    2. The header to metadata filter.
+    3. The JWT filter.
+    4. The Lua filter.
+
+  Like primary header matching used in routing, RBAC, etc. this behavior can be disabled by setting
+  the runtime value "envoy.reloadable_features.header_match_on_all_headers" to false.
 
 1.12.6 (July 7, 2020)
 =====================

include/envoy/http/BUILD

@@ -86,6 +86,9 @@ envoy_cc_library(
 envoy_cc_library(
     name = "header_map_interface",
     hdrs = ["header_map.h"],
+    external_deps = [
+        "abseil_inlined_vector",
+    ],
     deps = [
         "//source/common/common:assert_lib",
         "//source/common/common:hash_lib",

include/envoy/http/header_map.h

@@ -15,6 +15,7 @@
 #include "common/common/hash.h"
 #include "common/common/macros.h"
 
+#include "absl/container/inlined_vector.h"
 #include "absl/strings/string_view.h"
 
 namespace Envoy {
@@ -507,6 +508,31 @@ class HeaderMap {
   virtual const HeaderEntry* get(const LowerCaseString& key) const PURE;
   virtual HeaderEntry* get(const LowerCaseString& key) PURE;
 
+  /**
+   * This is a wrapper for the return result from getAll(). It avoids a copy when translating from
+   * non-const HeaderEntry to const HeaderEntry and only provides const access to the result.
+   */
+  using NonConstGetResult = absl::InlinedVector<HeaderEntry*, 1>;
+  class GetResult {
+  public:
+    GetResult() = default;
+    explicit GetResult(NonConstGetResult&& result) : result_(std::move(result)) {}
+
+    bool empty() const { return result_.empty(); }
+    size_t size() const { return result_.size(); }
+    const HeaderEntry* operator[](size_t i) const { return result_[i]; }
+
+  private:
+    NonConstGetResult result_;
+  };
+
+  /**
+   * Get a header by key.
+   * @param key supplies the header key.
+   * @return all header entries matching the key.
+   */
+  virtual GetResult getAll(const LowerCaseString& key) const PURE;
+
   // aliases to make iterate() and iterateReverse() callbacks easier to read
   enum class Iterate { Continue, Break };
 

source/common/http/BUILD

@@ -236,6 +236,7 @@ envoy_cc_library(
         "//source/common/common:empty_string",
         "//source/common/common:non_copyable",
         "//source/common/common:utility_lib",
+        "//source/common/runtime:runtime_features_lib",
         "//source/common/singleton:const_singleton",
     ],
 )

source/common/http/header_map_impl.cc

@@ -505,13 +505,12 @@ uint64_t HeaderMapImpl::byteSizeInternal() const {
 }
 
 const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const {
-  for (const HeaderEntryImpl& header : headers_) {
-    if (header.key() == key.get().c_str()) {
-      return &header;
-    }
-  }
+  const auto result = getAll(key);
+  return result.empty() ? nullptr : result[0];
+}
 
-  return nullptr;
+HeaderMap::GetResult HeaderMapImpl::getAll(const LowerCaseString& key) const {
+  return HeaderMap::GetResult(const_cast<HeaderMapImpl*>(this)->getExisting(key));
 }
 
 HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) {
@@ -521,10 +520,38 @@ HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) {
       return &header;
     }
   }
-
   return nullptr;
 }
 
+HeaderMap::NonConstGetResult HeaderMapImpl::getExisting(const LowerCaseString& key) {
+  // Attempt a trie lookup first to see if the user is requesting an O(1) header. This may be
+  // relatively common in certain header matching / routing patterns.
+  // TODO(mattklein123): Add inline handle support directly to the header matcher code to support
+  // this use case more directly.
+  HeaderMap::NonConstGetResult ret;
+
+  EntryCb cb = ConstSingleton<StaticLookupTable>::get().find(key.get());
+  if (cb) {
+    StaticLookupResponse ref_lookup_response = cb(*this);
+    if (*ref_lookup_response.entry_) {
+      ret.push_back(*ref_lookup_response.entry_);
+    }
+    return ret;
+  }
+  // If the requested header is not an O(1) header we do a full scan. Doing the trie lookup is
+  // wasteful in the miss case, but is present for code consistency with other functions that do
+  // similar things.
+  // TODO(mattklein123): The full scan here and in remove() are the biggest issues with this
+  // implementation for certain use cases. We can either replace this with a totally different
+  // implementation or potentially create a lazy map if the size of the map is above a threshold.
+  for (HeaderEntryImpl& header : headers_) {
+    if (header.key() == key.get().c_str()) {
+      ret.push_back(&header);
+    }
+  }
+  return ret;
+}
+
 void HeaderMapImpl::iterate(ConstIterateCb cb, void* context) const {
   for (const HeaderEntryImpl& header : headers_) {
     if (cb(header, context) == HeaderMap::Iterate::Break) {

source/common/http/header_map_impl.h

@@ -85,6 +85,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable {
   uint64_t byteSizeInternal() const override;
   const HeaderEntry* get(const LowerCaseString& key) const override;
   HeaderEntry* get(const LowerCaseString& key) override;
+  HeaderMap::GetResult getAll(const LowerCaseString& key) const override;
   void iterate(ConstIterateCb cb, void* context) const override;
   void iterateReverse(ConstIterateCb cb, void* context) const override;
   Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const override;
@@ -203,6 +204,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable {
   HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key);
   HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key,
                                      HeaderString&& value);
+  HeaderMap::NonConstGetResult getExisting(const LowerCaseString& key);
   HeaderEntryImpl* getExistingInline(absl::string_view key);
 
   void removeInline(HeaderEntryImpl** entry);

source/common/http/header_utility.cc

@@ -4,8 +4,10 @@
 #include "common/common/utility.h"
 #include "common/http/header_map_impl.h"
 #include "common/protobuf/utility.h"
+#include "common/runtime/runtime_features.h"
 
 #include "absl/strings/match.h"
+#include "absl/strings/str_join.h"
 #include "nghttp2/nghttp2.h"
 
 namespace Envoy {
@@ -92,36 +94,65 @@ bool HeaderUtility::matchHeaders(const HeaderMap& request_headers,
   return true;
 }
 
+HeaderUtility::GetAllOfHeaderAsStringResult
+HeaderUtility::getAllOfHeaderAsString(const HeaderMap& headers, const Http::LowerCaseString& key) {
+  GetAllOfHeaderAsStringResult result;
+  const auto header_value = headers.getAll(key);
+
+  if (header_value.empty()) {
+    // Empty for clarity. Avoid handling the empty case in the block below if the runtime feature
+    // is disabled.
+  } else if (header_value.size() == 1 ||
+             !Runtime::runtimeFeatureEnabled(
+                 "envoy.reloadable_features.http_match_on_all_headers")) {
+    result.result_ = header_value[0]->value().getStringView();
+  } else {
+    // In this case we concatenate all found headers using a ',' delimiter before performing the
+    // final match. We use an InlinedVector of absl::string_view to invoke the optimized join
+    // algorithm. This requires a copying phase before we invoke join. The 3 used as the inline
+    // size has been arbitrarily chosen.
+    // TODO(mattklein123): Do we need to normalize any whitespace here?
+    absl::InlinedVector<absl::string_view, 3> string_view_vector;
+    string_view_vector.reserve(header_value.size());
+    for (size_t i = 0; i < header_value.size(); i++) {
+      string_view_vector.push_back(header_value[i]->value().getStringView());
+    }
+    result.result_backing_string_ = absl::StrJoin(string_view_vector, ",");
+  }
+
+  return result;
+}
+
 bool HeaderUtility::matchHeaders(const HeaderMap& request_headers, const HeaderData& header_data) {
-  const HeaderEntry* header = request_headers.get(header_data.name_);
+  const auto header_value = getAllOfHeaderAsString(request_headers, header_data.name_);
 
-  if (header == nullptr) {
+  if (!header_value.result().has_value()) {
     return header_data.invert_match_ && header_data.header_match_type_ == HeaderMatchType::Present;
   }
 
   bool match;
-  const absl::string_view header_view = header->value().getStringView();
   switch (header_data.header_match_type_) {
   case HeaderMatchType::Value:
-    match = header_data.value_.empty() || header_view == header_data.value_;
+    match = header_data.value_.empty() || header_value.result().value() == header_data.value_;
     break;
   case HeaderMatchType::Regex:
-    match = header_data.regex_->match(header_view);
+    match = header_data.regex_->match(header_value.result().value());
     break;
   case HeaderMatchType::Range: {
-    int64_t header_value = 0;
-    match = absl::SimpleAtoi(header_view, &header_value) &&
-            header_value >= header_data.range_.start() && header_value < header_data.range_.end();
+    int64_t header_int_value = 0;
+    match = absl::SimpleAtoi(header_value.result().value(), &header_int_value) &&
+            header_int_value >= header_data.range_.start() &&
+            header_int_value < header_data.range_.end();
     break;
   }
   case HeaderMatchType::Present:
     match = true;
     break;
   case HeaderMatchType::Prefix:
-    match = absl::StartsWith(header_view, header_data.value_);
+    match = absl::StartsWith(header_value.result().value(), header_data.value_);
     break;
   case HeaderMatchType::Suffix:
-    match = absl::EndsWith(header_view, header_data.value_);
+    match = absl::EndsWith(header_value.result().value(), header_data.value_);
     break;
   default:
     NOT_REACHED_GCOVR_EXCL_LINE;

source/common/http/header_utility.h

@@ -32,6 +32,35 @@ class HeaderUtility {
   static void getAllOfHeader(const HeaderMap& headers, absl::string_view key,
                              std::vector<absl::string_view>& out);
 
+  /**
+   * Get all header values as a single string. Multiple headers are concatenated with ','.
+   */
+  class GetAllOfHeaderAsStringResult {
+  public:
+    // The ultimate result of the concatenation. If absl::nullopt, no header values were found.
+    // If the final string required a string allocation, the memory is held in
+    // backingString(). This allows zero allocation in the common case of a single header
+    // value.
+    absl::optional<absl::string_view> result() const {
+      // This is safe for move/copy of this class as the backing string will be moved or copied.
+      // Otherwise result_ is valid. The assert verifies that both are empty or only 1 is set.
+      ASSERT((!result_.has_value() && result_backing_string_.empty()) ||
+             (result_.has_value() ^ !result_backing_string_.empty()));
+      return !result_backing_string_.empty() ? result_backing_string_ : result_;
+    }
+
+    const std::string& backingString() const { return result_backing_string_; }
+
+  private:
+    absl::optional<absl::string_view> result_;
+    // Valid only if result_ relies on memory allocation that must live beyond the call. See above.
+    std::string result_backing_string_;
+
+    friend class HeaderUtility;
+  };
+  static GetAllOfHeaderAsStringResult getAllOfHeaderAsString(const HeaderMap& headers,
+                                                             const Http::LowerCaseString& key);
+
   // A HeaderData specifies one of exact value or regex or range element
   // to match in a request's header, specified in the header_match_type_ member.
   // It is the runtime equivalent of the HeaderMatchSpecifier proto in RDS API.

source/common/runtime/BUILD

@@ -9,13 +9,32 @@ load(
 envoy_package()
 
 envoy_cc_library(
-    name = "runtime_lib",
+    name = "runtime_features_lib",
     srcs = [
         "runtime_features.cc",
-        "runtime_impl.cc",
     ],
     hdrs = [
         "runtime_features.h",
+    ],
+    deps = [
+        "//include/envoy/runtime:runtime_interface",
+        "//source/common/common:assert_lib",
+        "//source/common/common:hash_lib",
+        "//source/common/protobuf:message_validator_lib",
+        "//source/common/protobuf:utility_lib",
+        "//source/common/singleton:const_singleton",
+        "@envoy_api//envoy/api/v2/core:pkg_cc_proto",
+        "@envoy_api//envoy/config/bootstrap/v2:pkg_cc_proto",
+        "@envoy_api//envoy/service/discovery/v2:pkg_cc_proto",
+    ],
+)
+
+envoy_cc_library(
+    name = "runtime_lib",
+    srcs = [
+        "runtime_impl.cc",
+    ],
+    hdrs = [
         "runtime_impl.h",
     ],
     external_deps = ["ssl"],
@@ -37,6 +56,7 @@ envoy_cc_library(
         "//source/common/init:target_lib",
         "//source/common/protobuf:message_validator_lib",
         "//source/common/protobuf:utility_lib",
+        "//source/common/runtime:runtime_features_lib",
         "@envoy_api//envoy/api/v2/core:pkg_cc_proto",
         "@envoy_api//envoy/config/bootstrap/v2:pkg_cc_proto",
         "@envoy_api//envoy/service/discovery/v2:pkg_cc_proto",

source/common/runtime/runtime_features.cc

@@ -1,8 +1,31 @@
 #include "common/runtime/runtime_features.h"
 
+#include "common/common/assert.h"
+
 namespace Envoy {
 namespace Runtime {
 
+bool runtimeFeatureEnabled(absl::string_view feature) {
+  if (Runtime::LoaderSingleton::getExisting()) {
+    return Runtime::LoaderSingleton::getExisting()->threadsafeSnapshot()->runtimeFeatureEnabled(
+        feature);
+  }
+  ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::runtime), warn,
+                      "Unable to use runtime singleton for feature {}", feature);
+  return RuntimeFeaturesDefaults::get().enabledByDefault(feature);
+}
+
+uint64_t getInteger(absl::string_view feature, uint64_t default_value) {
+  ASSERT(absl::StartsWith(feature, "envoy."));
+  if (Runtime::LoaderSingleton::getExisting()) {
+    return Runtime::LoaderSingleton::getExisting()->threadsafeSnapshot()->getInteger(
+        std::string(feature), default_value);
+  }
+  ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::runtime), warn,
+                      "Unable to use runtime singleton for feature {}", feature);
+  return default_value;
+}
+
 // Add additional features here to enable the new code paths by default.
 //
 // Per documentation in CONTRIBUTING.md is expected that new high risk code paths be guarded
@@ -32,6 +55,7 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.strict_header_validation",
     "envoy.reloadable_features.strict_authority_validation",
     "envoy.reloadable_features.fix_wildcard_matching",
+    "envoy.reloadable_features.http_match_on_all_headers",
 };
 
 // This is a list of configuration fields which are disallowed by default in Envoy

source/common/runtime/runtime_features.h

@@ -7,12 +7,17 @@
 
 #include "common/protobuf/utility.h"
 #include "common/singleton/const_singleton.h"
+#include "common/singleton/threadsafe_singleton.h"
 
 #include "absl/container/flat_hash_set.h"
+#include "absl/strings/match.h"
 
 namespace Envoy {
 namespace Runtime {
 
+bool runtimeFeatureEnabled(absl::string_view feature);
+uint64_t getInteger(absl::string_view feature, uint64_t default_value);
+
 class RuntimeFeatures {
 public:
   RuntimeFeatures();