[1.5] Fix CVE-2020-25017 (#45) (#271)

* docs: kick-off 1.13.5 release. (envoyproxy#12164) Signed-off-by: Piotr Sikora <[email protected]> Signed-off-by: Yuchen Dai <[email protected]> * Preserve LWS from the middle of HTTP1 header values that requ… (envoyproxy#12320) [http1] Preserve LWS from the middle of HTTP1 header values that require multiple dispatch calls to process (envoyproxy#10886) Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice. Signed-off-by: Antonio Vicente <[email protected]> Signed-off-by: Yuchen Dai <[email protected]> * http: header map security fixes for duplicate headers (#197) (#203) 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 <[email protected]> Signed-off-by: Yuchen Dai <[email protected]> * fixed compile Signed-off-by: Yuchen Dai <[email protected]> * fix test Signed-off-by: Yuchen Dai <[email protected]> Co-authored-by: Piotr Sikora <[email protected]> Co-authored-by: Yuchen Dai <[email protected]> Co-authored-by: Piotr Sikora <[email protected]>
istio · Oct 8, 2020 · 936fdef · 936fdef
1 parent 7e40bc3
commit 936fdef
Show file tree

Hide file tree

Showing 38 changed files with 748 additions and 128 deletions.
diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-1.13.4
+1.13.5-dev
diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst
@@ -1,6 +1,32 @@
 Version history
 ---------------
 
+1.13.5 (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.
+* http: fixed CVE-2020-25017. The setCopy() header map API previously only set the first header in the case of duplicate
+  non-inline headers. setCopy() now behaves similarly 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.
+
 1.13.4 (July 7, 2020)
 =====================
 * tls: fixed a bug where wilcard matching for "\*.foo.com" also matched domains of the form "a.b.foo.com". This behavior can be temporarily reverted by setting runtime feature `envoy.reloadable_features.fix_wildcard_matching` to false.

diff --git a/include/envoy/http/BUILD b/include/envoy/http/BUILD
@@ -93,6 +93,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",

diff --git a/include/envoy/http/header_map.h b/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 {
@@ -129,6 +130,12 @@ class HeaderString {
    */
   char* buffer() { return buffer_.dynamic_; }
 
+  /**
+   * Trim trailing whitespaces from the HeaderString.
+   * v1.13 supports both Inline and Dynamic, but not Reference type.
+   */
+  void rtrim();
+
   /**
    * Get an absl::string_view. It will NOT be NUL terminated!
    *
@@ -503,6 +510,31 @@ class HeaderMap {
    */
   virtual const HeaderEntry* get(const LowerCaseString& key) const 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 };
 

diff --git a/source/common/http/BUILD b/source/common/http/BUILD
@@ -251,6 +251,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",
     ],
 )

diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc
@@ -9,6 +9,7 @@
 #include "common/common/dump_state_utils.h"
 #include "common/common/empty_string.h"
 #include "common/common/utility.h"
+#include "common/runtime/runtime_features.h"
 #include "common/singleton/const_singleton.h"
 
 #include "absl/strings/match.h"
@@ -150,6 +151,15 @@ void HeaderString::append(const char* data, uint32_t size) {
   string_length_ += size;
 }
 
+void HeaderString::rtrim() {
+  ASSERT(type() == Type::Inline || type() == Type::Dynamic);
+  absl::string_view original = getStringView();
+  absl::string_view rtrimmed = StringUtil::rtrim(original);
+  if (original.size() != rtrimmed.size()) {
+    string_length_ = rtrimmed.size();
+  }
+}
+
 void HeaderString::clear() {
   switch (type_) {
   case Type::Reference: {
@@ -462,9 +472,9 @@ void HeaderMapImpl::addCopy(const LowerCaseString& key, absl::string_view value)
 
 void HeaderMapImpl::appendCopy(const LowerCaseString& key, absl::string_view value) {
   // TODO(#9221): converge on and document a policy for coalescing multiple headers.
-  auto* entry = getExisting(key);
-  if (entry) {
-    const uint64_t added_size = appendToHeader(entry->value(), value);
+  auto entry = getExisting(key);
+  if (!entry.empty()) {
+    const uint64_t added_size = appendToHeader(entry[0]->value(), value);
     addSize(added_size);
   } else {
     addCopy(key, value);
@@ -474,31 +484,27 @@ void HeaderMapImpl::appendCopy(const LowerCaseString& key, absl::string_view val
 }
 
 void HeaderMapImpl::setReference(const LowerCaseString& key, absl::string_view value) {
-  HeaderString ref_key(key);
-  HeaderString ref_value(value);
   remove(key);
-  insertByKey(std::move(ref_key), std::move(ref_value));
-  verifyByteSize();
+  addReference(key, value);
 }
 
 void HeaderMapImpl::setReferenceKey(const LowerCaseString& key, absl::string_view value) {
-  HeaderString ref_key(key);
-  HeaderString new_value;
-  new_value.setCopy(value);
   remove(key);
-  insertByKey(std::move(ref_key), std::move(new_value));
-  ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move)
-  verifyByteSize();
+  addReferenceKey(key, value);
 }
 
 void HeaderMapImpl::setCopy(const LowerCaseString& key, absl::string_view value) {
-  // Replaces the first occurrence of a header if it exists, otherwise adds by copy.
-  // TODO(#9221): converge on and document a policy for coalescing multiple headers.
-  auto* entry = getExisting(key);
-  if (entry) {
-    updateSize(entry->value().size(), value.size());
-    entry->value(value);
+  if (!Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.http_set_copy_replace_all_headers")) {
+    auto entry = getExisting(key);
+    if (!entry.empty()) {
+      updateSize(entry[0]->value().size(), value.size());
+      entry[0]->value(value);
+    } else {
+      addCopy(key, value);
+    }
   } else {
+    remove(key);
     addCopy(key, value);
   }
   verifyByteSize();
@@ -517,23 +523,41 @@ 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::getExisting(const LowerCaseString& key) {
+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()) {
-      return &header;
+      ret.push_back(&header);
     }
   }
-
-  return nullptr;
+  return ret;
 }
 
 void HeaderMapImpl::iterate(ConstIterateCb cb, void* context) const {

diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h
@@ -88,6 +88,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable {
   void setCopy(const LowerCaseString& key, absl::string_view value) override;
   uint64_t byteSize() const override;
   const HeaderEntry* get(const LowerCaseString& key) const 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;
@@ -213,7 +214,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable {
   HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key);
   HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key,
                                      HeaderString&& value);
-  HeaderEntry* getExisting(const LowerCaseString& key);
+  HeaderMap::NonConstGetResult getExisting(const LowerCaseString& key);
   HeaderEntryImpl* getExistingInline(absl::string_view key);
 
   void removeInline(HeaderEntryImpl** entry);

diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc
@@ -105,36 +105,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;

diff --git a/source/common/http/header_utility.h b/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.