diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 4fb77f3b1bef1..a19ffab252fdf 100644 --- a/source/common/http/utility.cc +++ b/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(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; diff --git a/source/common/http/utility.h b/source/common/http/utility.h index e2881b15181a1..f77cc3baf6c66 100644 --- a/source/common/http/utility.h +++ b/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. diff --git a/test/common/http/BUILD b/test/common/http/BUILD index a3dde8c5c7471..b27a3ac4612c5 100644 --- a/test/common/http/BUILD +++ b/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", diff --git a/test/common/http/common_fuzz.cc b/test/common/http/common_fuzz.cc new file mode 100644 index 0000000000000..30d57777d4011 --- /dev/null +++ b/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 diff --git a/test/common/http/common_fuzz.h b/test/common/http/common_fuzz.h new file mode 100644 index 0000000000000..c388cf84376ac --- /dev/null +++ b/test/common/http/common_fuzz.h @@ -0,0 +1,16 @@ +#include + +#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 diff --git a/test/common/http/header_map_impl_fuzz_test.cc b/test/common/http/header_map_impl_fuzz_test.cc index c7a7b478b11a8..39506775d37c4 100644 --- a/test/common/http/header_map_impl_fuzz_test.cc +++ b/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(); 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> 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(filterInvalidCharacters(add_reference.key()))); + std::make_unique(replaceInvalidCharacters(add_reference.key()))); strings.emplace_back( - std::make_unique(filterInvalidCharacters(add_reference.value()))); + std::make_unique(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( - 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(filterInvalidCharacters(set_reference.key()))); + std::make_unique(replaceInvalidCharacters(set_reference.key()))); strings.emplace_back( - std::make_unique(filterInvalidCharacters(set_reference.value()))); + std::make_unique(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( - 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,7 +119,7 @@ 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: @@ -155,7 +127,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::HeaderMapImplFuzzTestCase& input) break; case test::common::http::GetAndMutate::kSetReference: strings.emplace_back(std::make_unique( - 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: diff --git a/test/common/http/utility_corpus/clusterfuzz-testcase-utility_fuzz_test-5735325211557888 b/test/common/http/utility_corpus/clusterfuzz-testcase-utility_fuzz_test-5735325211557888 new file mode 100644 index 0000000000000..236c2da8db1e9 --- /dev/null +++ b/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" +} diff --git a/test/common/http/utility_fuzz.proto b/test/common/http/utility_fuzz.proto index f026ad9ca9b66..d958b60c850a5 100644 --- a/test/common/http/utility_fuzz.proto +++ b/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; } diff --git a/test/common/http/utility_fuzz_test.cc b/test/common/http/utility_fuzz_test.cc index 8892b79df26c7..0e03c1f601024 100644 --- a/test/common/http/utility_fuzz_test.cc +++ b/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; diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index e2e354c1da9a4..9bcbfaecfa316 100644 --- a/test/common/http/utility_test.cc +++ b/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)); diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index 7cc14ac27e390..341cf7e12c18c 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -90,6 +90,7 @@ FIN FIPS FQDN FUZZER +FUZZERS GB GC GCC