Revert "[1.7] Fix CVE-2020-25017 (#47)" (#48)

This reverts commit c336863.

VERSION

@@ -1 +1 @@
-1.15.1-dev
+1.15.0

docs/root/version_history/version_history.rst

@@ -7,8 +7,6 @@ Version history
   :titlesonly:
 
   current
-  v1.15.1
-  v1.15.0
   v1.14.3
   v1.14.2
   v1.14.1

include/envoy/http/header_map.h

@@ -513,31 +513,6 @@ 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 };
 

source/common/http/BUILD

@@ -275,7 +275,6 @@ 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

@@ -10,7 +10,6 @@
 #include "common/common/assert.h"
 #include "common/common/dump_state_utils.h"
 #include "common/common/empty_string.h"
-#include "common/runtime/runtime_features.h"
 #include "common/singleton/const_singleton.h"
 
 #include "absl/strings/match.h"
@@ -385,37 +384,39 @@ 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.empty()) {
-    const uint64_t added_size = appendToHeader(entry[0]->value(), value);
+  auto* entry = getExisting(key);
+  if (entry) {
+    const uint64_t added_size = appendToHeader(entry->value(), value);
     addSize(added_size);
   } else {
     addCopy(key, value);
   }
 }
 
 void HeaderMapImpl::setReference(const LowerCaseString& key, absl::string_view value) {
+  HeaderString ref_key(key);
+  HeaderString ref_value(value);
   remove(key);
-  addReference(key, value);
+  insertByKey(std::move(ref_key), std::move(ref_value));
 }
 
 void HeaderMapImpl::setReferenceKey(const LowerCaseString& key, absl::string_view value) {
+  HeaderString ref_key(key);
+  HeaderString new_value;
+  new_value.setCopy(value);
   remove(key);
-  addReferenceKey(key, value);
+  insertByKey(std::move(ref_key), std::move(new_value));
+  ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move)
 }
 
 void HeaderMapImpl::setCopy(const LowerCaseString& key, absl::string_view 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);
-    }
+  // 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);
   } else {
-    remove(key);
     addCopy(key, value);
   }
 }
@@ -433,26 +434,17 @@ void HeaderMapImpl::verifyByteSizeInternalForTest() const {
 }
 
 const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const {
-  const auto result = getAll(key);
-  return result.empty() ? nullptr : result[0];
+  return const_cast<HeaderMapImpl*>(this)->getExisting(key);
 }
 
-HeaderMap::GetResult HeaderMapImpl::getAll(const LowerCaseString& key) const {
-  return HeaderMap::GetResult(const_cast<HeaderMapImpl*>(this)->getExisting(key));
-}
-
-HeaderMap::NonConstGetResult HeaderMapImpl::getExisting(const LowerCaseString& key) {
+HeaderEntry* 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;
   auto lookup = staticLookup(key.get());
   if (lookup.has_value()) {
-    if (*lookup.value().entry_ != nullptr) {
-      ret.push_back(*lookup.value().entry_);
-    }
-    return ret;
+    return *lookup.value().entry_;
   }
 
   // If the requested header is not an O(1) header we do a full scan. Doing the trie lookup is
@@ -463,11 +455,11 @@ HeaderMap::NonConstGetResult HeaderMapImpl::getExisting(const LowerCaseString& k
   // 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 &header;
     }
   }
 
-  return ret;
+  return nullptr;
 }
 
 void HeaderMapImpl::iterate(HeaderMap::ConstIterateCb cb, void* context) const {

source/common/http/header_map_impl.h

@@ -86,7 +86,6 @@ class HeaderMapImpl : NonCopyable {
   void setCopy(const LowerCaseString& key, absl::string_view value);
   uint64_t byteSize() const;
   const HeaderEntry* get(const LowerCaseString& key) const;
-  HeaderMap::GetResult getAll(const LowerCaseString& key) const;
   void iterate(HeaderMap::ConstIterateCb cb, void* context) const;
   void iterateReverse(HeaderMap::ConstIterateCb cb, void* context) const;
   void clear();
@@ -241,7 +240,7 @@ class HeaderMapImpl : NonCopyable {
   HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key);
   HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key,
                                      HeaderString&& value);
-  HeaderMap::NonConstGetResult getExisting(const LowerCaseString& key);
+  HeaderEntry* getExisting(const LowerCaseString& key);
   size_t removeInline(HeaderEntryImpl** entry);
   void updateSize(uint64_t from_size, uint64_t to_size);
   void addSize(uint64_t size);
@@ -299,9 +298,6 @@ template <class Interface> class TypedHeaderMapImpl : public HeaderMapImpl, publ
   const HeaderEntry* get(const LowerCaseString& key) const override {
     return HeaderMapImpl::get(key);
   }
-  HeaderMap::GetResult getAll(const LowerCaseString& key) const override {
-    return HeaderMapImpl::getAll(key);
-  }
   void iterate(HeaderMap::ConstIterateCb cb, void* context) const override {
     HeaderMapImpl::iterate(cb, context);
   }

source/common/http/header_utility.cc

@@ -106,65 +106,36 @@ 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 auto header_value = getAllOfHeaderAsString(request_headers, header_data.name_);
+  const HeaderEntry* header = request_headers.get(header_data.name_);
 
-  if (!header_value.result().has_value()) {
+  if (header == nullptr) {
     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_value.result().value() == header_data.value_;
+    match = header_data.value_.empty() || header_view == header_data.value_;
     break;
   case HeaderMatchType::Regex:
-    match = header_data.regex_->match(header_value.result().value());
+    match = header_data.regex_->match(header_view);
     break;
   case HeaderMatchType::Range: {
-    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();
+    int64_t header_value = 0;
+    match = absl::SimpleAtoi(header_view, &header_value) &&
+            header_value >= header_data.range_.start() && header_value < header_data.range_.end();
     break;
   }
   case HeaderMatchType::Present:
     match = true;
     break;
   case HeaderMatchType::Prefix:
-    match = absl::StartsWith(header_value.result().value(), header_data.value_);
+    match = absl::StartsWith(header_view, header_data.value_);
     break;
   case HeaderMatchType::Suffix:
-    match = absl::EndsWith(header_value.result().value(), header_data.value_);
+    match = absl::EndsWith(header_view, header_data.value_);
     break;
   default:
     NOT_REACHED_GCOVR_EXCL_LINE;

source/common/http/header_utility.h

@@ -33,35 +33,6 @@ 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/runtime_features.cc

@@ -71,8 +71,6 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.fix_wildcard_matching",
     "envoy.reloadable_features.fixed_connection_close",
     "envoy.reloadable_features.http_default_alpn",
-    "envoy.reloadable_features.http_match_on_all_headers",
-    "envoy.reloadable_features.http_set_copy_replace_all_headers",
     "envoy.reloadable_features.listener_in_place_filterchain_update",
     "envoy.reloadable_features.preserve_query_string_in_path_redirects",
     "envoy.reloadable_features.preserve_upstream_date",

source/extensions/filters/common/expr/context.cc

@@ -23,19 +23,6 @@ absl::optional<CelValue> convertHeaderEntry(const Http::HeaderEntry* header) {
   return CelValue::CreateStringView(header->value().getStringView());
 }
 
-absl::optional<CelValue>
-convertHeaderEntry(Protobuf::Arena& arena,
-                   Http::HeaderUtility::GetAllOfHeaderAsStringResult&& result) {
-  if (!result.result().has_value()) {
-    return {};
-  } else if (!result.backingString().empty()) {
-    return CelValue::CreateString(
-        Protobuf::Arena::Create<std::string>(&arena, result.backingString()));
-  } else {
-    return CelValue::CreateStringView(result.result().value());
-  }
-}
-
 namespace {
 
 absl::optional<CelValue> extractSslInfo(const Ssl::ConnectionInfo& ssl_info,