HCM: protect against removal of critical response headers by a filter…

… chain. (envoyproxy#16745) This is the rework of the previous PR (envoyproxy#15658). Additional Description: Previously, Envoy hasn't been guarded from the removal of critical response headers like :status. This happens when there's misbehaving filter chan, and especially this protection is important when users run their own extensions through Wasm. Resolves envoyproxy#13756 and ref: envoyproxy#15487. Testing: new integration tests Risk: low Signed-off-by: Takeshi Yoneda <[email protected]>
leyao-daily · Sep 30, 2021 · 2090e02 · 2090e02
1 parent 1739086
commit 2090e02
Show file tree

Hide file tree

Showing 16 changed files with 169 additions and 17 deletions.
diff --git a/docs/root/configuration/http/http_conn_man/response_code_details.rst b/docs/root/configuration/http/http_conn_man/response_code_details.rst
@@ -26,7 +26,8 @@ Below are the list of reasons the HttpConnectionManager or Router filter may sen
    duration_timeout, The max connection duration was exceeded.
    direct_response, A direct response was generated by the router filter.
    filter_chain_not_found, The request was rejected due to no matching filter chain.
-   filter_removed_required_headers, The request was rejected in the filter manager because a configured filter removed required headers.
+   filter_removed_required_request_headers, The request was rejected in the filter manager because a configured filter removed required request headers.
+   filter_removed_required_response_headers, The response was rejected in the filter manager because a configured filter removed required response headers or these values were invalid (e.g. overflown status).
    internal_redirect, The original stream was replaced with an internal redirect.
    low_version, The HTTP/1.0 or HTTP/0.9 request was rejected due to HTTP/1.0 support not being configured.
    maintenance_mode, The request was rejected by the router filter because the cluster was in maintenance mode.

diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h
@@ -179,8 +179,13 @@ struct ResponseCodeDetailValues {
   const std::string AdminFilterResponse = "admin_filter_response";
   // The original stream was replaced with an internal redirect.
   const std::string InternalRedirect = "internal_redirect";
-  // The request was rejected because configured filters erroneously removed required headers.
-  const std::string FilterRemovedRequiredHeaders = "filter_removed_required_headers";
+  // The request was rejected because configured filters erroneously removed required request
+  // headers.
+  const std::string FilterRemovedRequiredRequestHeaders = "filter_removed_required_request_headers";
+  // The request was rejected because configured filters erroneously removed required response
+  // headers.
+  const std::string FilterRemovedRequiredResponseHeaders =
+      "filter_removed_required_response_headers";
   // The request was rejected because the original IP couldn't be detected.
   const std::string OriginalIPDetectionFailed = "rejecting because detection failed";
   // Changes or additions to details should be reflected in

diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc
@@ -1084,6 +1084,21 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea
     }
   }
 
+  // Check if the filter chain above did not remove critical headers or set malformed header values.
+  // We could do this at the codec in order to prevent other places than the filter chain from
+  // removing critical headers, but it will come with the implementation complexity.
+  // See the previous attempt (#15658) for detail, and for now we choose to protect only against
+  // filter chains.
+  const auto status = HeaderUtility::checkRequiredResponseHeaders(headers);
+  if (!status.ok()) {
+    // If the check failed, then we reply with BadGateway, and stop the further processing.
+    sendLocalReply(
+        Http::Code::BadGateway, status.message(), nullptr, absl::nullopt,
+        absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredResponseHeaders,
+                     "{", status.message(), "}"));
+    return;
+  }
+
   const bool modified_end_stream = (end_stream && continue_data_entry == encoder_filters_.end());
   state_.non_100_response_headers_encoded_ = true;
   filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream);

diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc
@@ -316,7 +316,7 @@ bool HeaderUtility::shouldCloseConnection(Http::Protocol protocol,
   return false;
 }
 
-Http::Status HeaderUtility::checkRequiredHeaders(const Http::RequestHeaderMap& headers) {
+Http::Status HeaderUtility::checkRequiredRequestHeaders(const Http::RequestHeaderMap& headers) {
   if (!headers.Method()) {
     return absl::InvalidArgumentError(
         absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Method.get()));
@@ -338,6 +338,15 @@ Http::Status HeaderUtility::checkRequiredHeaders(const Http::RequestHeaderMap& h
   return Http::okStatus();
 }
 
+Http::Status HeaderUtility::checkRequiredResponseHeaders(const Http::ResponseHeaderMap& headers) {
+  const absl::optional<uint64_t> status = Utility::getResponseStatusNoThrow(headers);
+  if (!status.has_value()) {
+    return absl::InvalidArgumentError(
+        absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Status.get()));
+  }
+  return Http::okStatus();
+}
+
 bool HeaderUtility::isRemovableHeader(absl::string_view header) {
   return (header.empty() || header[0] != ':') &&
          !absl::EqualsIgnoreCase(header, Headers::get().HostLegacy.get());

diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h
@@ -198,13 +198,20 @@ class HeaderUtility {
    */
   static absl::string_view::size_type getPortStart(absl::string_view host);
 
-  /* Does a common header check ensuring required headers are present.
+  /* Does a common header check ensuring required request headers are present.
    * Required request headers include :method header, :path for non-CONNECT requests, and
    * host/authority for HTTP/1.1 or CONNECT requests.
    * @return Status containing the result. If failed, message includes details on which header was
    * missing.
    */
-  static Http::Status checkRequiredHeaders(const Http::RequestHeaderMap& headers);
+  static Http::Status checkRequiredRequestHeaders(const Http::RequestHeaderMap& headers);
+
+  /* Does a common header check ensuring required response headers are present.
+   * Current required response headers only includes :status.
+   * @return Status containing the result. If failed, message includes details on which header was
+   * missing.
+   */
+  static Http::Status checkRequiredResponseHeaders(const Http::ResponseHeaderMap& headers);
 
   /**
    * Returns true if a header may be safely removed without causing additional

diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc
@@ -402,7 +402,7 @@ static const char REQUEST_POSTFIX[] = " HTTP/1.1\r\n";
 Status RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end_stream) {
   // Required headers must be present. This can only happen by some erroneous processing after the
   // downstream codecs decode.
-  RETURN_IF_ERROR(HeaderUtility::checkRequiredHeaders(headers));
+  RETURN_IF_ERROR(HeaderUtility::checkRequiredRequestHeaders(headers));
 
   const HeaderEntry* method = headers.Method();
   const HeaderEntry* path = headers.Path();

diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc
@@ -210,7 +210,7 @@ Status ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& h
                                                        bool end_stream) {
   // Required headers must be present. This can only happen by some erroneous processing after the
   // downstream codecs decode.
-  RETURN_IF_ERROR(HeaderUtility::checkRequiredHeaders(headers));
+  RETURN_IF_ERROR(HeaderUtility::checkRequiredRequestHeaders(headers));
   // This must exist outside of the scope of isUpgrade as the underlying memory is
   // needed until encodeHeadersBase has been called.
   std::vector<nghttp2_nv> final_headers;

diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc
@@ -58,7 +58,7 @@ Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap&
                                                   bool end_stream) {
   // Required headers must be present. This can only happen by some erroneous processing after the
   // downstream codecs decode.
-  RETURN_IF_ERROR(Http::HeaderUtility::checkRequiredHeaders(headers));
+  RETURN_IF_ERROR(Http::HeaderUtility::checkRequiredRequestHeaders(headers));
 
   ENVOY_STREAM_LOG(debug, "encodeHeaders: (end_stream={}) {}.", *this, end_stream, headers);
   local_end_stream_ = end_stream;

diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc
@@ -449,8 +449,8 @@ void UpstreamRequest::onPoolReady(
     // erroneously remove required headers.
     stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError);
     const std::string details =
-        absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{",
-                     status.message(), "}");
+        absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredRequestHeaders,
+                     "{", status.message(), "}");
     parent_.callbacks()->sendLocalReply(Http::Code::ServiceUnavailable, status.message(), nullptr,
                                         absl::nullopt, details);
     return;

diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc
@@ -1467,5 +1467,30 @@ TEST(PercentEncoding, Encoding) {
   EXPECT_EQ(Utility::PercentEncoding::encode("too%!large/", "%!/"), "too%25%21large%2F");
 }
 
+TEST(CheckRequiredHeaders, Request) {
+  EXPECT_EQ(Http::okStatus(), HeaderUtility::checkRequiredRequestHeaders(
+                                  TestRequestHeaderMapImpl{{":method", "GET"}, {":path", "/"}}));
+  EXPECT_EQ(Http::okStatus(), HeaderUtility::checkRequiredRequestHeaders(TestRequestHeaderMapImpl{
+                                  {":method", "CONNECT"}, {":authority", "localhost:1234"}}));
+  EXPECT_EQ(absl::InvalidArgumentError("missing required header: :method"),
+            HeaderUtility::checkRequiredRequestHeaders(TestRequestHeaderMapImpl{}));
+  EXPECT_EQ(
+      absl::InvalidArgumentError("missing required header: :path"),
+      HeaderUtility::checkRequiredRequestHeaders(TestRequestHeaderMapImpl{{":method", "GET"}}));
+  EXPECT_EQ(
+      absl::InvalidArgumentError("missing required header: :authority"),
+      HeaderUtility::checkRequiredRequestHeaders(TestRequestHeaderMapImpl{{":method", "CONNECT"}}));
+}
+
+TEST(CheckRequiredHeaders, Response) {
+  EXPECT_EQ(Http::okStatus(), HeaderUtility::checkRequiredResponseHeaders(
+                                  TestResponseHeaderMapImpl{{":status", "200"}}));
+  EXPECT_EQ(absl::InvalidArgumentError("missing required header: :status"),
+            HeaderUtility::checkRequiredResponseHeaders(TestResponseHeaderMapImpl{}));
+  EXPECT_EQ(
+      absl::InvalidArgumentError("missing required header: :status"),
+      HeaderUtility::checkRequiredResponseHeaders(TestResponseHeaderMapImpl{{":status", "abcd"}}));
+}
+
 } // namespace Http
 } // namespace Envoy
diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc
@@ -165,12 +165,13 @@ TEST_F(RouterTest, MissingRequiredHeaders) {
 
   EXPECT_CALL(encoder, encodeHeaders(_, _))
       .WillOnce(Invoke([](const Http::RequestHeaderMap& headers, bool) -> Http::Status {
-        return Http::HeaderUtility::checkRequiredHeaders(headers);
+        return Http::HeaderUtility::checkRequiredRequestHeaders(headers);
       }));
-  EXPECT_CALL(callbacks_,
-              sendLocalReply(Http::Code::ServiceUnavailable,
-                             testing::Eq("missing required header: :method"), _, _,
-                             "filter_removed_required_headers{missing required header: :method}"))
+  EXPECT_CALL(
+      callbacks_,
+      sendLocalReply(Http::Code::ServiceUnavailable,
+                     testing::Eq("missing required header: :method"), _, _,
+                     "filter_removed_required_request_headers{missing required header: :method}"))
       .WillOnce(testing::InvokeWithoutArgs([] {}));
   router_.decodeHeaders(headers, true);
   router_.onDestroy();

diff --git a/test/integration/BUILD b/test/integration/BUILD
@@ -486,6 +486,7 @@ envoy_cc_test_library(
         "//test/integration/filters:local_reply_during_encoding_filter_lib",
         "//test/integration/filters:local_reply_with_metadata_filter_lib",
         "//test/integration/filters:random_pause_filter_lib",
+        "//test/integration/filters:remove_response_headers_lib",
         "//test/test_common:logging_lib",
         "//test/test_common:utility_lib",
         "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",

diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD
@@ -527,3 +527,18 @@ envoy_cc_test_library(
         "//test/test_common:delegating_route_utility_lib",
     ],
 )
+
+envoy_cc_test_library(
+    name = "remove_response_headers_lib",
+    srcs = [
+        "remove_response_headers.cc",
+    ],
+    deps = [
+        ":common_lib",
+        "//include/envoy/http:filter_interface",
+        "//include/envoy/registry",
+        "//include/envoy/server:filter_config_interface",
+        "//source/extensions/filters/http/common:pass_through_filter_lib",
+        "//test/extensions/filters/http/common:empty_http_filter_config_lib",
+    ],
+)
diff --git a/test/integration/filters/remove_response_headers.cc b/test/integration/filters/remove_response_headers.cc
@@ -0,0 +1,33 @@
+#include "envoy/registry/registry.h"
+#include "envoy/server/filter_config.h"
+
+#include "extensions/filters/http/common/pass_through_filter.h"
+
+#include "test/extensions/filters/http/common/empty_http_filter_config.h"
+#include "test/integration/filters/common.h"
+
+namespace Envoy {
+
+// Registers the misbehaving filter which removes all response headers.
+class RemoveResponseHeadersFilter : public Http::PassThroughFilter {
+public:
+  constexpr static char name[] = "remove-response-headers-filter";
+  Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers, bool) override {
+    std::vector<std::string> keys;
+    headers.iterate([&keys](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate {
+      keys.push_back(std::string(header.key().getStringView()));
+      return Http::HeaderMap::Iterate::Continue;
+    });
+    for (auto& k : keys) {
+      const Http::LowerCaseString lower_key{k};
+      headers.remove(lower_key);
+    }
+    return Http::FilterHeadersStatus::Continue;
+  }
+};
+
+static Registry::RegisterFactory<SimpleFilterConfig<RemoveResponseHeadersFilter>,
+                                 Server::Configuration::NamedHttpFilterConfigFactory>
+    register_;
+
+} // namespace Envoy
diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc
@@ -2396,4 +2396,44 @@ TEST_P(DownstreamProtocolIntegrationTest, DisableStripTrailingHostDot) {
   EXPECT_EQ("404", response->headers().getStatusValue());
 }
 
+static std::string remove_response_headers_filter = R"EOF(
+name: remove-response-headers-filter
+typed_config:
+  "@type": type.googleapis.com/google.protobuf.Empty
+)EOF";
+
+TEST_P(ProtocolIntegrationTest, HeadersOnlyRequestWithRemoveResponseHeadersFilter) {
+  config_helper_.addFilter(remove_response_headers_filter);
+  initialize();
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+
+  IntegrationStreamDecoderPtr response =
+      codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+  waitForNextUpstreamRequest();
+  upstream_request_->encodeHeaders(default_response_headers_, false);
+  ASSERT_TRUE(response->waitForEndStream());
+  // If a filter chain removes :status from the response headers, then Envoy must reply with
+  // BadGateway and must not crash.
+  ASSERT_TRUE(codec_client_->connected());
+  EXPECT_EQ("502", response->headers().getStatusValue());
+  EXPECT_THAT(response->body(), HasSubstr("missing required header: :status"));
+}
+
+TEST_P(ProtocolIntegrationTest, RemoveResponseHeadersFilter) {
+  config_helper_.addFilter(remove_response_headers_filter);
+  initialize();
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+
+  IntegrationStreamDecoderPtr response =
+      codec_client_->makeRequestWithBody(default_request_headers_, 10);
+  waitForNextUpstreamRequest();
+  upstream_request_->encodeHeaders(default_response_headers_, false);
+  ASSERT_TRUE(response->waitForEndStream());
+  // If a filter chain removes :status from the response headers, then Envoy must reply with
+  // BadGateway and not crash.
+  ASSERT_TRUE(codec_client_->connected());
+  EXPECT_EQ("502", response->headers().getStatusValue());
+  EXPECT_THAT(response->body(), HasSubstr("missing required header: :status"));
+}
+
 } // namespace Envoy
diff --git a/test/mocks/http/stream_encoder.cc b/test/mocks/http/stream_encoder.cc
@@ -17,7 +17,7 @@ MockRequestEncoder::MockRequestEncoder() {
       .WillByDefault(Invoke([](const RequestHeaderMap& headers, bool) -> Status {
         // Check to see that method is not-null. Path can be null for CONNECT and authority can be
         // null at the codec level.
-        ASSERT(HeaderUtility::checkRequiredHeaders(headers).ok());
+        ASSERT(HeaderUtility::checkRequiredRequestHeaders(headers).ok());
         return okStatus();
       }));
 }