router: disallow :path/host rewriting in request_headers_to_add. (#4220)

We have dedicated alternative mechanisms for this in RouteAction, it can
confuse other actions (e.g. prefix_rewrite).

Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9995.

Risk level: Low
Testing: Unit tests and corpus entry added.

Signed-off-by: Harvey Tuch <htuch@google.com>

Author: htuch

api/envoy/api/v2/core/base.proto

@@ -146,7 +146,7 @@ message HeaderValue {
 // Header name/value pair plus option to control append behavior.
 message HeaderValueOption {
   // Header name/value pair that this option applies to.
-  HeaderValue header = 1;
+  HeaderValue header = 1 [(validate.rules).message.required = true];
 
   // Should the value be appended? If true (default), the value is appended to
   // existing values.

docs/root/configuration/http_conn_man/headers.rst

@@ -438,6 +438,11 @@ route, virtual host, and/or global route configuration level. See the relevant :
 <config_http_conn_man_route_table>` and :ref:`v2 <envoy_api_msg_RouteConfiguration>` API
 documentation.
 
+No *:*-prefixed pseudo-header may be modified via this mechanism. The *:path*
+and *:authority* headers may instead be modified via mechanisms such as
+:ref:`prefix_rewrite <envoy_api_field_route.RouteAction.prefix_rewrite>` and
+:ref:`host_rewrite <envoy_api_field_route.RouteAction.host_rewrite>`.
+
 Headers are appended to requests/responses in the following order: weighted cluster level headers,
 route level headers, virtual host level headers and finally global level headers.
 
@@ -502,4 +507,4 @@ Supported variable names are:
           - header:
               key: "x-request-start"
               value: "%START_TIME(%s.%3f)%"
-            append: true
+            append: true

docs/root/intro/version_history.rst

@@ -48,6 +48,8 @@ Version history
   <envoy_api_field_route.Route.request_headers_to_add>` with *authorization:
   token2* applied.
 * http: response filters not applied to early error paths such as http_parser generated 400s.
+* http: restrictions added to reject *:*-prefixed pseudo-headers in :ref:`custom
+  request headers <config_http_conn_man_headers_custom_request_headers>`.
 * http: :ref:`hpack_table_size <envoy_api_field_core.Http2ProtocolOptions.hpack_table_size>` now controls
   dynamic table size of both: encoder and decoder.
 * listeners: added the ability to match :ref:`FilterChain <envoy_api_msg_listener.FilterChain>` using

source/common/router/BUILD

@@ -214,6 +214,7 @@ envoy_cc_library(
         ":header_formatter_lib",
         "//include/envoy/http:header_map_interface",
         "//source/common/config:base_json_lib",
+        "//source/common/http:headers_lib",
         "//source/common/protobuf:utility_lib",
     ],
 )

source/common/router/header_parser.cc

@@ -6,6 +6,7 @@
 #include <string>
 
 #include "common/common/assert.h"
+#include "common/http/headers.h"
 #include "common/protobuf/utility.h"
 
 #include "absl/strings/str_cat.h"
@@ -36,6 +37,18 @@ std::string unescape(absl::string_view sv) { return absl::StrReplaceAll(sv, {{"%
 // RequestInfoHeaderFormatter.
 HeaderFormatterPtr
 parseInternal(const envoy::api::v2::core::HeaderValueOption& header_value_option) {
+  const std::string& key = header_value_option.header().key();
+  // PGV constraints provide this guarantee.
+  ASSERT(!key.empty());
+  // We reject :path/:authority rewriting, there is already a well defined mechanism to
+  // perform this in the RouteAction, and doing this via request_headers_to_add
+  // will cause us to have to worry about interaction with other aspects of the
+  // RouteAction, e.g. prefix rewriting. We also reject other :-prefixed
+  // headers, since it seems dangerous and there doesn't appear a use case.
+  if (key[0] == ':') {
+    throw EnvoyException(":-prefixed headers may not be modified");
+  }
+
   const bool append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(header_value_option, append, true);
 
   absl::string_view format(header_value_option.header().value());

test/common/router/config_impl_test.cc

@@ -915,6 +915,33 @@ response_headers_to_remove: ["x-global-remove"]
               ContainerEq(config.internalOnlyHeaders()));
 }
 
+// Validate that we can't add :-prefixed request headers.
+TEST(RouteMatcherTest, TestRequestHeadersToAddNoPseudoHeader) {
+  for (const std::string& header : {":path", ":authority", ":method", ":scheme", ":status",
+                                    ":protocol", ":no-chunks", ":status"}) {
+    const std::string yaml = fmt::format(R"EOF(
+name: foo
+virtual_hosts:
+  - name: www2
+    domains: ["*"]
+    request_headers_to_add:
+      - header:
+          key: {}
+          value: vhost-www2
+        append: false
+)EOF",
+                                         header);
+
+    NiceMock<Server::Configuration::MockFactoryContext> factory_context;
+    NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;
+
+    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 modified");
+  }
+}
+
 TEST(RouteMatcherTest, Priority) {
   std::string json = R"EOF(
 {

test/common/router/route_corpus/clusterfuzz-testcase-route_fuzz_test-5647162250625024

@@ -0,0 +1,40 @@
+config {
+  virtual_hosts {
+    name: "regex"
+    domains: "bat.com"
+    domains: "*"
+    domains: "w.lyft.com"
+    routes {
+      match {
+        regex: "/"
+      }
+      route {
+        cluster: "regex"
+        prefix_rewrite: "ewrote"
+      }
+      request_headers_to_add {
+        header {
+          key: "\177\177\177\177\177\177\177\177"
+        }
+      }
+    }
+  }
+  request_headers_to_add {
+    header {
+      key: ":path"
+      value: "`"
+    }
+    append {
+    }
+  }
+  request_headers_to_add {
+    header {
+      key: "`"
+      value: "`"
+    }
+    append {
+    }
+  }
+  validate_clusters {
+  }
+}