fuzz: add header string filter in utility_fuzz_test and cleanup dead code

source/common/http/utility.cc

@@ -209,42 +209,6 @@ std::string Utility::makeSetCookieValue(const std::string& key, const std::strin
   return cookie_value;
 }
 
-bool Utility::hasSetCookie(const HeaderMap& headers, const std::string& key) {
-
-  struct State {
-    std::string key_;
-    bool ret_;
-  };
-
-  State state;
-  state.key_ = key;
-  state.ret_ = false;
-
-  headers.iterate(
-      [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate {
-        // Find the set-cookie headers in the request
-        if (header.key() == Http::Headers::get().SetCookie.get()) {
-          const absl::string_view value{header.value().getStringView()};
-          const size_t equals_index = value.find('=');
-
-          if (equals_index == absl::string_view::npos) {
-            // The cookie is malformed if it does not have an `=`.
-            return HeaderMap::Iterate::Continue;
-          }
-          absl::string_view k = value.substr(0, equals_index);
-          State* state = static_cast<State*>(context);
-          if (k == state->key_) {
-            state->ret_ = true;
-            return HeaderMap::Iterate::Break;
-          }
-        }
-        return HeaderMap::Iterate::Continue;
-      },
-      &state);
-
-  return state.ret_;
-}
-
 uint64_t Utility::getResponseStatus(const HeaderMap& headers) {
   const HeaderEntry* header = headers.Status();
   uint64_t response_code;

source/common/http/utility.h

@@ -99,14 +99,6 @@ absl::string_view findQueryStringStart(const HeaderString& path);
  **/
 std::string parseCookieValue(const HeaderMap& headers, const std::string& key);
 
-/**
- * Check whether a Set-Cookie header for the given cookie name exists
- * @param headers supplies the headers to search for the cookie
- * @param key the name of the cookie to search for
- * @return bool true if the cookie is set, false otherwise
- */
-bool hasSetCookie(const HeaderMap& headers, const std::string& key);
-
 /**
  * Produce the value for a Set-Cookie header with the given parameters.
  * @param key is the name of the cookie that is being set.

test/common/http/BUILD

@@ -129,6 +129,15 @@ envoy_cc_test_library(
     ],
 )
 
+envoy_cc_test_library(
+    name = "common_fuzz_lib",
+    srcs = ["common_fuzz.cc"],
+    hdrs = ["common_fuzz.h"],
+    deps = [
+        "//source/common/common:macros",
+    ],
+)
+
 envoy_proto_library(
     name = "conn_manager_impl_fuzz_proto",
     srcs = ["conn_manager_impl_fuzz.proto"],
@@ -264,6 +273,7 @@ envoy_cc_fuzz_test(
     srcs = ["header_map_impl_fuzz_test.cc"],
     corpus = "header_map_impl_corpus",
     deps = [
+        ":common_fuzz_lib",
         ":header_map_impl_fuzz_proto_cc",
         "//source/common/http:header_map_lib",
     ],
@@ -300,6 +310,7 @@ envoy_cc_fuzz_test(
     srcs = ["utility_fuzz_test.cc"],
     corpus = "utility_corpus",
     deps = [
+        ":common_fuzz_lib",
         ":utility_fuzz_proto_cc",
         "//source/common/http:utility_lib",
         "//test/test_common:utility_lib",

test/common/http/common_fuzz.cc

@@ -0,0 +1,26 @@
+#include "common_fuzz.h"
+
+#include "common/common/macros.h"
+
+namespace Envoy {
+
+std::string replaceInvalidCharacters(absl::string_view string) {
+  std::string filtered;
+  filtered.reserve(string.length());
+  for (const char& c : string) {
+    switch (c) {
+    case '\0':
+      FALLTHRU;
+    case '\r':
+      FALLTHRU;
+    case '\n':
+      filtered.push_back(' ');
+      break;
+    default:
+      filtered.push_back(c);
+    }
+  }
+  return filtered;
+}
+
+} // namespace Envoy

test/common/http/common_fuzz.h

@@ -0,0 +1,16 @@
+#include <functional>
+
+#include "absl/strings/string_view.h"
+
+namespace Envoy {
+
+// The HeaderMap code assumes that input does not contain certain characters, and
+// this is validated by the HTTP parser. Some fuzzers will create strings with
+// these characters, however, and this creates not very interesting fuzz test
+// failures as an assertion is rapidly hit in the LowerCaseString constructor
+// before we get to anything interesting.
+//
+// This method will replace any of those characters found with spaces.
+std::string replaceInvalidCharacters(absl::string_view string);
+
+} // namespace Envoy

test/common/http/header_map_impl_fuzz_test.cc

@@ -4,46 +4,18 @@
 #include "common/common/logger.h"
 #include "common/http/header_map_impl.h"
 
+#include "test/common/http/common_fuzz.h"
 #include "test/common/http/header_map_impl_fuzz.pb.h"
 #include "test/fuzz/fuzz_runner.h"
 
 namespace Envoy {
 
-namespace {
-// The HeaderMap code assumes that input does not contain certain characters, and
-// this is validated by the HTTP parser. The fuzzer will create strings with
-// these characters, however, and this creates not very interesting fuzz test
-// failures as an assertion is rapidly hit in the LowerCaseString constructor
-// before we get to anything interesting.
-//
-// This method will replace any of those characters found with spaces.
-std::string filterInvalidCharacters(absl::string_view string) {
-  std::string filtered;
-  filtered.reserve(string.length());
-  for (const char& c : string) {
-    switch (c) {
-    case '\0':
-      FALLTHRU;
-    case '\r':
-      FALLTHRU;
-    case '\n':
-      filtered.push_back(' ');
-      break;
-    default:
-      filtered.push_back(c);
-    }
-  }
-  return filtered;
-}
-
-} // namespace
-
 // Fuzz the header map implementation.
 DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) {
   Http::HeaderMapImplPtr header_map = std::make_unique<Http::HeaderMapImpl>();
   const auto predefined_exists = [&header_map](const std::string& s) -> bool {
     const Http::HeaderEntry* entry;
-    return header_map->lookup(Http::LowerCaseString(filterInvalidCharacters(s)), &entry) ==
+    return header_map->lookup(Http::LowerCaseString(replaceInvalidCharacters(s)), &entry) ==
            Http::HeaderMap::Lookup::Found;
   };
   std::vector<std::unique_ptr<Http::LowerCaseString>> lower_case_strings;
@@ -60,9 +32,9 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input)
         continue;
       }
       lower_case_strings.emplace_back(
-          std::make_unique<Http::LowerCaseString>(filterInvalidCharacters(add_reference.key())));
+          std::make_unique<Http::LowerCaseString>(replaceInvalidCharacters(add_reference.key())));
       strings.emplace_back(
-          std::make_unique<std::string>(filterInvalidCharacters(add_reference.value())));
+          std::make_unique<std::string>(replaceInvalidCharacters(add_reference.value())));
       header_map->addReference(*lower_case_strings.back(), *strings.back());
       break;
     }
@@ -73,11 +45,11 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input)
         continue;
       }
       lower_case_strings.emplace_back(std::make_unique<Http::LowerCaseString>(
-          filterInvalidCharacters(add_reference_key.key())));
+          replaceInvalidCharacters(add_reference_key.key())));
       switch (add_reference_key.value_selector_case()) {
       case test::common::http::AddReferenceKey::kStringValue:
         header_map->addReferenceKey(*lower_case_strings.back(),
-                                    filterInvalidCharacters(add_reference_key.string_value()));
+                                    replaceInvalidCharacters(add_reference_key.string_value()));
         break;
       case test::common::http::AddReferenceKey::kUint64Value:
         header_map->addReferenceKey(*lower_case_strings.back(), add_reference_key.uint64_value());
@@ -93,10 +65,10 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input)
       if (predefined_exists(add_copy.key())) {
         continue;
       }
-      const Http::LowerCaseString key{filterInvalidCharacters(add_copy.key())};
+      const Http::LowerCaseString key{replaceInvalidCharacters(add_copy.key())};
       switch (add_copy.value_selector_case()) {
       case test::common::http::AddCopy::kStringValue:
-        header_map->addCopy(key, filterInvalidCharacters(add_copy.string_value()));
+        header_map->addCopy(key, replaceInvalidCharacters(add_copy.string_value()));
         break;
       case test::common::http::AddCopy::kUint64Value:
         header_map->addCopy(key, add_copy.uint64_value());
@@ -109,24 +81,24 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input)
     case test::common::http::Action::kSetReference: {
       const auto& set_reference = action.set_reference();
       lower_case_strings.emplace_back(
-          std::make_unique<Http::LowerCaseString>(filterInvalidCharacters(set_reference.key())));
+          std::make_unique<Http::LowerCaseString>(replaceInvalidCharacters(set_reference.key())));
       strings.emplace_back(
-          std::make_unique<std::string>(filterInvalidCharacters(set_reference.value())));
+          std::make_unique<std::string>(replaceInvalidCharacters(set_reference.value())));
       header_map->setReference(*lower_case_strings.back(), *strings.back());
       break;
     }
     case test::common::http::Action::kSetReferenceKey: {
       const auto& set_reference_key = action.set_reference_key();
       lower_case_strings.emplace_back(std::make_unique<Http::LowerCaseString>(
-          filterInvalidCharacters(set_reference_key.key())));
+          replaceInvalidCharacters(set_reference_key.key())));
       header_map->setReferenceKey(*lower_case_strings.back(),
-                                  filterInvalidCharacters(set_reference_key.value()));
+                                  replaceInvalidCharacters(set_reference_key.value()));
       break;
     }
     case test::common::http::Action::kGetAndMutate: {
       const auto& get_and_mutate = action.get_and_mutate();
       auto* header_entry =
-          header_map->get(Http::LowerCaseString(filterInvalidCharacters(get_and_mutate.key())));
+          header_map->get(Http::LowerCaseString(replaceInvalidCharacters(get_and_mutate.key())));
       if (header_entry != nullptr) {
         // Do some read-only stuff.
         (void)strlen(std::string(header_entry->key().getStringView()).c_str());
@@ -137,7 +109,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input)
         // Do some mutation or parameterized action.
         switch (get_and_mutate.mutate_selector_case()) {
         case test::common::http::GetAndMutate::kAppend:
-          header_entry->value().append(filterInvalidCharacters(get_and_mutate.append()).c_str(),
+          header_entry->value().append(replaceInvalidCharacters(get_and_mutate.append()).c_str(),
                                        get_and_mutate.append().size());
           break;
         case test::common::http::GetAndMutate::kClear:
@@ -147,15 +119,15 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input)
           header_entry->value().getStringView().find(get_and_mutate.find());
           break;
         case test::common::http::GetAndMutate::kSetCopy:
-          header_entry->value().setCopy(filterInvalidCharacters(get_and_mutate.set_copy()).c_str(),
+          header_entry->value().setCopy(replaceInvalidCharacters(get_and_mutate.set_copy()).c_str(),
                                         get_and_mutate.set_copy().size());
           break;
         case test::common::http::GetAndMutate::kSetInteger:
           header_entry->value().setInteger(get_and_mutate.set_integer());
           break;
         case test::common::http::GetAndMutate::kSetReference:
           strings.emplace_back(std::make_unique<std::string>(
-              filterInvalidCharacters(get_and_mutate.set_reference())));
+              replaceInvalidCharacters(get_and_mutate.set_reference())));
           header_entry->value().setReference(*strings.back());
           break;
         default:
@@ -171,17 +143,17 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input)
     }
     case test::common::http::Action::kLookup: {
       const Http::HeaderEntry* header_entry;
-      header_map->lookup(Http::LowerCaseString(filterInvalidCharacters(action.lookup())),
+      header_map->lookup(Http::LowerCaseString(replaceInvalidCharacters(action.lookup())),
                          &header_entry);
       break;
     }
     case test::common::http::Action::kRemove: {
-      header_map->remove(Http::LowerCaseString(filterInvalidCharacters(action.remove())));
+      header_map->remove(Http::LowerCaseString(replaceInvalidCharacters(action.remove())));
       break;
     }
     case test::common::http::Action::kRemovePrefix: {
       header_map->removePrefix(
-          Http::LowerCaseString(filterInvalidCharacters(action.remove_prefix())));
+          Http::LowerCaseString(replaceInvalidCharacters(action.remove_prefix())));
       break;
     }
     default:

test/common/http/utility_corpus/clusterfuzz-testcase-utility_fuzz_test-5735325211557888

@@ -0,0 +1,3 @@
+parse_cookie_value {
+  cookies: "\027\000\000\000\000\000\000\000"
+}

test/common/http/utility_fuzz.proto

@@ -15,10 +15,11 @@ message GetLastAddressFromXff {
 }
 
 message UtilityTestCase {
+  reserved 3; // formerly has_set_cookie
+
   oneof utility_selector {
     string parse_query_string = 1;
     CookiesKey parse_cookie_value = 2;
-    CookiesKey has_set_cookie = 3;
     GetLastAddressFromXff get_last_address_from_xff = 4;
     string extract_host_path_from_uri = 5;
   }

test/common/http/utility_fuzz_test.cc

@@ -1,5 +1,6 @@
 #include "common/http/utility.h"
 
+#include "test/common/http/common_fuzz.h"
 #include "test/common/http/utility_fuzz.pb.h"
 #include "test/fuzz/fuzz_runner.h"
 #include "test/test_common/utility.h"
@@ -18,20 +19,11 @@ DEFINE_PROTO_FUZZER(const test::common::http::UtilityTestCase& input) {
     const auto& parse_cookie_value = input.parse_cookie_value();
     Http::TestHeaderMapImpl headers;
     for (const std::string& cookie : parse_cookie_value.cookies()) {
-      headers.addCopy("cookie", cookie);
+      headers.addCopy("cookie", replaceInvalidCharacters(cookie));
     }
     Http::Utility::parseCookieValue(headers, parse_cookie_value.key());
     break;
   }
-  case test::common::http::UtilityTestCase::kHasSetCookie: {
-    const auto& has_set_cookie = input.has_set_cookie();
-    Http::TestHeaderMapImpl headers;
-    for (const std::string& cookie : has_set_cookie.cookies()) {
-      headers.addCopy("set-cookie", cookie);
-    }
-    Http::Utility::hasSetCookie(headers, has_set_cookie.key());
-    break;
-  }
   case test::common::http::UtilityTestCase::kGetLastAddressFromXff: {
     const auto& get_last_address_from_xff = input.get_last_address_from_xff();
     Http::TestHeaderMapImpl headers;

test/common/http/utility_test.cc

@@ -410,27 +410,6 @@ TEST(HttpUtility, TestParseCookieWithQuotes) {
   EXPECT_EQ(Utility::parseCookieValue(headers, "leadingdquote"), "\"foobar");
 }
 
-TEST(HttpUtility, TestHasSetCookie) {
-  TestHeaderMapImpl headers{{"someheader", "10.0.0.1"},
-                            {"set-cookie", "somekey=somevalue"},
-                            {"set-cookie", "abc=def; Expires=Wed, 09 Jun 2021 10:18:14 GMT"},
-                            {"set-cookie", "key2=value2; Secure"}};
-
-  EXPECT_TRUE(Utility::hasSetCookie(headers, "abc"));
-  EXPECT_TRUE(Utility::hasSetCookie(headers, "somekey"));
-  EXPECT_FALSE(Utility::hasSetCookie(headers, "ghi"));
-}
-
-TEST(HttpUtility, TestHasSetCookieBadValues) {
-  TestHeaderMapImpl headers{{"someheader", "10.0.0.1"},
-                            {"set-cookie", "somekey =somevalue"},
-                            {"set-cookie", "abc"},
-                            {"set-cookie", "key2=value2; Secure"}};
-
-  EXPECT_FALSE(Utility::hasSetCookie(headers, "abc"));
-  EXPECT_TRUE(Utility::hasSetCookie(headers, "key2"));
-}
-
 TEST(HttpUtility, TestMakeSetCookieValue) {
   EXPECT_EQ("name=\"value\"; Max-Age=10",
             Utility::makeSetCookieValue("name", "value", "", std::chrono::seconds(10), false));

tools/spelling_dictionary.txt

@@ -90,6 +90,7 @@ FIN
 FIPS
 FQDN
 FUZZER
+FUZZERS
 GB
 GC
 GCC