From c8ac01d4efc41f6f3a703a90c8e5b521ed1f93f8 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 16 Oct 2018 17:11:09 -0400 Subject: [PATCH] router: disallow host header removal. This is a followup to #4576. It turns out that we have both the ability to refer to the host header via "host" and ":authority" in HeaderMapImpl, see https://github.com/envoyproxy/envoy/blob/6ac936f2750c39a8b4fb232d6ddc4802f4e6aeee/source/common/http/header_map_impl.cc#L276. Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10771. Risk Level: Low Testing: Modified existing unit test and corpus entry added. Signed-off-by: Harvey Tuch --- source/common/router/header_parser.cc | 4 ++-- test/common/router/config_impl_test.cc | 4 ++-- ...se-minimized-route_fuzz_test-5654717359718400 | 1 + test/common/router/route_fuzz_test.cc | 16 ++++++++-------- 4 files changed, 13 insertions(+), 12 deletions(-) create mode 100644 test/common/router/route_corpus/clusterfuzz-testcase-minimized-route_fuzz_test-5654717359718400 diff --git a/source/common/router/header_parser.cc b/source/common/router/header_parser.cc index d0383e28f6ea7..b9cd484e56dca 100644 --- a/source/common/router/header_parser.cc +++ b/source/common/router/header_parser.cc @@ -243,8 +243,8 @@ HeaderParserPtr HeaderParser::configure( // We reject :-prefix (e.g. :path) removal here. This is dangerous, since other aspects of // request finalization assume their existence and they are needed for well-formedness in most // cases. - if (header[0] == ':') { - throw EnvoyException(":-prefixed headers may not be removed"); + if (header[0] == ':' || Http::LowerCaseString(header).get() == "host") { + throw EnvoyException(":-prefixed or host headers may not be removed"); } header_parser->headers_to_remove_.emplace_back(header); } diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index de56e04b7a445..20c61538154d2 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -1010,7 +1010,7 @@ name: foo // Validate that we can't remove :-prefixed request headers. TEST(RouteMatcherTest, TestRequestHeadersToRemoveNoPseudoHeader) { for (const std::string& header : {":path", ":authority", ":method", ":scheme", ":status", - ":protocol", ":no-chunks", ":status"}) { + ":protocol", ":no-chunks", ":status", "host"}) { const std::string yaml = fmt::format(R"EOF( name: foo virtual_hosts: @@ -1027,7 +1027,7 @@ name: foo envoy::api::v2::RouteConfiguration route_config = parseRouteConfigurationFromV2Yaml(yaml); EXPECT_THROW_WITH_MESSAGE(TestConfigImpl config(route_config, factory_context, true), - EnvoyException, ":-prefixed headers may not be removed"); + EnvoyException, ":-prefixed or host headers may not be removed"); } } diff --git a/test/common/router/route_corpus/clusterfuzz-testcase-minimized-route_fuzz_test-5654717359718400 b/test/common/router/route_corpus/clusterfuzz-testcase-minimized-route_fuzz_test-5654717359718400 new file mode 100644 index 0000000000000..34324eec3424f --- /dev/null +++ b/test/common/router/route_corpus/clusterfuzz-testcase-minimized-route_fuzz_test-5654717359718400 @@ -0,0 +1 @@ +config { virtual_hosts { name: " " domains: "*" routes { match { path: "/" } route { cluster: " " host_rewrite: " " } } } request_headers_to_remove: "host" } diff --git a/test/common/router/route_fuzz_test.cc b/test/common/router/route_fuzz_test.cc index 45f8101eb68d0..19aedb7313647 100644 --- a/test/common/router/route_fuzz_test.cc +++ b/test/common/router/route_fuzz_test.cc @@ -18,16 +18,16 @@ DEFINE_PROTO_FUZZER(const test::common::router::RouteTestCase& input) { MessageUtil::validate(input.config()); ConfigImpl config(input.config(), factory_context, true); Http::TestHeaderMapImpl headers = Fuzz::fromHeaders(input.headers()); - // It's a precondition of routing that {host, path:, x-fowarded-proto} headers exists, HCM - // enforces this. - if (!headers.has("host")) { - headers.addCopy("host", "example.com"); + // It's a precondition of routing that {:authority, :path, x-forwarded-proto} headers exists, + // HCM enforces this. + if (headers.Host() == nullptr) { + headers.insertHost().value(std::string("example.com")); } - if (!headers.has(":path")) { - headers.addCopy(":path", "/"); + if (headers.Path() == nullptr) { + headers.insertPath().value(std::string("/")); } - if (!headers.has("x-forwarded-proto")) { - headers.addCopy("x-forwarded-proto", "http"); + if (headers.ForwardedProto() == nullptr) { + headers.insertForwardedProto().value(std::string("http")); } auto route = config.route(headers, input.random_value()); if (route != nullptr && route->routeEntry() != nullptr) {