Skip to content

Commit c8ac01d

Browse files
committed
router: disallow host header removal.
This is a followup to envoyproxy#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 <htuch@google.com>
1 parent 6ac936f commit c8ac01d

File tree

4 files changed

+13
-12
lines changed

4 files changed

+13
-12
lines changed

source/common/router/header_parser.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -243,8 +243,8 @@ HeaderParserPtr HeaderParser::configure(
243243
// We reject :-prefix (e.g. :path) removal here. This is dangerous, since other aspects of
244244
// request finalization assume their existence and they are needed for well-formedness in most
245245
// cases.
246-
if (header[0] == ':') {
247-
throw EnvoyException(":-prefixed headers may not be removed");
246+
if (header[0] == ':' || Http::LowerCaseString(header).get() == "host") {
247+
throw EnvoyException(":-prefixed or host headers may not be removed");
248248
}
249249
header_parser->headers_to_remove_.emplace_back(header);
250250
}

test/common/router/config_impl_test.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1010,7 +1010,7 @@ name: foo
10101010
// Validate that we can't remove :-prefixed request headers.
10111011
TEST(RouteMatcherTest, TestRequestHeadersToRemoveNoPseudoHeader) {
10121012
for (const std::string& header : {":path", ":authority", ":method", ":scheme", ":status",
1013-
":protocol", ":no-chunks", ":status"}) {
1013+
":protocol", ":no-chunks", ":status", "host"}) {
10141014
const std::string yaml = fmt::format(R"EOF(
10151015
name: foo
10161016
virtual_hosts:
@@ -1027,7 +1027,7 @@ name: foo
10271027
envoy::api::v2::RouteConfiguration route_config = parseRouteConfigurationFromV2Yaml(yaml);
10281028

10291029
EXPECT_THROW_WITH_MESSAGE(TestConfigImpl config(route_config, factory_context, true),
1030-
EnvoyException, ":-prefixed headers may not be removed");
1030+
EnvoyException, ":-prefixed or host headers may not be removed");
10311031
}
10321032
}
10331033

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
config { virtual_hosts { name: " " domains: "*" routes { match { path: "/" } route { cluster: " " host_rewrite: " " } } } request_headers_to_remove: "host" }

test/common/router/route_fuzz_test.cc

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,16 @@ DEFINE_PROTO_FUZZER(const test::common::router::RouteTestCase& input) {
1818
MessageUtil::validate(input.config());
1919
ConfigImpl config(input.config(), factory_context, true);
2020
Http::TestHeaderMapImpl headers = Fuzz::fromHeaders(input.headers());
21-
// It's a precondition of routing that {host, path:, x-fowarded-proto} headers exists, HCM
22-
// enforces this.
23-
if (!headers.has("host")) {
24-
headers.addCopy("host", "example.com");
21+
// It's a precondition of routing that {:authority, :path, x-forwarded-proto} headers exists,
22+
// HCM enforces this.
23+
if (headers.Host() == nullptr) {
24+
headers.insertHost().value(std::string("example.com"));
2525
}
26-
if (!headers.has(":path")) {
27-
headers.addCopy(":path", "/");
26+
if (headers.Path() == nullptr) {
27+
headers.insertPath().value(std::string("/"));
2828
}
29-
if (!headers.has("x-forwarded-proto")) {
30-
headers.addCopy("x-forwarded-proto", "http");
29+
if (headers.ForwardedProto() == nullptr) {
30+
headers.insertForwardedProto().value(std::string("http"));
3131
}
3232
auto route = config.route(headers, input.random_value());
3333
if (route != nullptr && route->routeEntry() != nullptr) {

0 commit comments

Comments
 (0)