http: Stricter validation of HTTP/1 headers (CVE-2019-18802) (#77)

* Stricter validation of HTTP/1 headers (CVE-2019-18802).

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

* Remove unnecessary nghttp2 patches after it was upgraded to 1.40.0

Signed-off-by: Yan Avlasov <yavlasov@google.com>

docs/root/intro/version_history.rst

@@ -84,9 +84,11 @@ Version history
 * http: :ref:`AUTO <envoy_api_enum_value_config.filter.network.http_connection_manager.v2.HttpConnectionManager.CodecType.AUTO>` codec protocol inference now requires the H2 magic bytes to be the first bytes transmitted by a downstream client.
 * http: remove h2c upgrade headers for HTTP/1 as h2c upgrades are currently not supported.
 * http: absolute URL support is now on by default. The prior behavior can be reinstated by setting :ref:`allow_absolute_url <envoy_api_field_core.Http1ProtocolOptions.allow_absolute_url>` to false.
+* http: added strict authority checking. This can be reversed temporarily by setting the runtime feature `envoy.reloadable_features.strict_authority_validation` to false.
 * http: support :ref:`host rewrite <envoy_api_msg_config.filter.http.dynamic_forward_proxy.v2alpha.PerRouteConfig>` in the dynamic forward proxy.
 * http: support :ref:`disabling the filter per route <envoy_api_msg_config.filter.http.grpc_http1_reverse_bridge.v2alpha1.FilterConfigPerRoute>` in the grpc http1 reverse bridge filter.
 * http: added the ability to :ref:`configure max connection duration <envoy_api_field_core.HttpProtocolOptions.max_connection_duration>` for downstream connections.
+* http: trim LWS at the end of header keys, for correct HTTP/1.1 header parsing.
 * listeners: added :ref:`continue_on_listener_filters_timeout <envoy_api_field_Listener.continue_on_listener_filters_timeout>` to configure whether a listener will still create a connection when listener filters time out.
 * listeners: added :ref:`HTTP inspector listener filter <config_listener_filters_http_inspector>`.
 * listeners: added :ref:`connection balancer <envoy_api_field_Listener.connection_balance_config>`

include/envoy/stream_info/stream_info.h

@@ -137,6 +137,8 @@ struct ResponseCodeDetailValues {
   // indicates that original "success" headers may have been sent downstream
   // despite the subsequent failure.
   const std::string LateUpstreamReset = "upstream_reset_after_response_started";
+  // The request was rejected due to invalid characters in the HOST/:authority header.
+  const std::string InvalidAuthority = "invalid_authority";
 };
 
 using ResponseCodeDetails = ConstSingleton<ResponseCodeDetailValues>;

source/common/http/conn_manager_impl.cc

@@ -766,6 +766,15 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
     }
   }
 
+  // Make sure the host is valid.
+  if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_authority_validation") &&
+      !HeaderUtility::authorityIsValid(request_headers_->Host()->value().getStringView())) {
+    sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "",
+                   nullptr, is_head_request_, absl::nullopt,
+                   StreamInfo::ResponseCodeDetails::get().InvalidAuthority);
+    return;
+  }
+
   // Currently we only support relative paths at the application layer. We expect the codec to have
   // broken the path into pieces if applicable. NOTE: Currently the HTTP/1.1 codec only does this
   // when the allow_absolute_url flag is enabled on the HCM.

source/common/http/header_utility.cc

@@ -135,6 +135,11 @@ bool HeaderUtility::headerIsValid(const absl::string_view header_value) {
                                      header_value.size()) != 0);
 }
 
+bool HeaderUtility::authorityIsValid(const absl::string_view header_value) {
+  return (nghttp2_check_authority(reinterpret_cast<const uint8_t*>(header_value.data()),
+                                  header_value.size()) != 0);
+}
+
 void HeaderUtility::addHeaders(HeaderMap& headers, const HeaderMap& headers_to_add) {
   headers_to_add.iterate(
       [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate {

source/common/http/header_utility.h

@@ -96,6 +96,12 @@ class HeaderUtility {
    */
   static bool headerIsValid(const absl::string_view header_value);
 
+  /**
+   * Validates that the characters in the authority are valid.
+   * @return bool true if the header values are valid, false otherwise.
+   */
+  static bool authorityIsValid(const absl::string_view authority_value);
+
   /**
    * Add headers from one HeaderMap to another
    * @param headers target where headers will be added

source/common/http/http1/codec_impl.cc

@@ -469,7 +469,9 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) {
     return;
   }
 
-  const absl::string_view header_value = absl::string_view(data, length);
+  // Work around a bug in http_parser where trailing whitespace is not trimmed
+  // as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4
+  const absl::string_view header_value = StringUtil::trim(absl::string_view(data, length));
 
   if (strict_header_validation_) {
     if (!Http::HeaderUtility::headerIsValid(header_value)) {
@@ -487,7 +489,7 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) {
   }
 
   header_parsing_state_ = HeaderParsingState::Value;
-  current_header_value_.append(data, length);
+  current_header_value_.append(header_value.data(), header_value.length());
 
   const uint32_t total =
       current_header_field_.size() + current_header_value_.size() + current_header_map_->byteSize();

source/common/runtime/runtime_features.cc

@@ -29,6 +29,8 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.buffer_filter_populate_content_length",
     "envoy.reloadable_features.outlier_detection_support_for_grpc_status",
     "envoy.reloadable_features.connection_header_sanitization",
+    "envoy.reloadable_features.strict_header_validation",
+    "envoy.reloadable_features.strict_authority_validation",
 };
 
 // This is a list of configuration fields which are disallowed by default in Envoy

test/common/http/header_utility_test.cc

@@ -464,6 +464,13 @@ TEST(HeaderIsValidTest, ValidHeaderValuesAreAccepted) {
   EXPECT_TRUE(HeaderUtility::headerIsValid("Some Other Value"));
 }
 
+TEST(HeaderIsValidTest, AuthIsValid) {
+  EXPECT_TRUE(HeaderUtility::authorityIsValid("strangebutlegal$-%&'"));
+  EXPECT_FALSE(HeaderUtility::authorityIsValid("illegal{}"));
+  // Full checks are done by Http2CodecImplTest.CheckAuthority, cross checking
+  // against nghttp2 compliance.
+}
+
 TEST(HeaderAddTest, HeaderAdd) {
   TestHeaderMapImpl headers{{"myheader1", "123value"}};
   TestHeaderMapImpl headers_to_add{{"myheader2", "456value"}};

test/common/http/http1/codec_impl_test.cc

@@ -61,6 +61,25 @@ class Http1ServerConnectionImplTest : public testing::Test {
   void testRequestHeadersExceedLimit(std::string header_string);
   void testRequestHeadersAccepted(std::string header_string);
 
+  // Send the request, and validate the received request headers.
+  // Then send a response just to clean up.
+  void sendAndValidateRequestAndSendResponse(absl::string_view raw_request,
+                                             const TestHeaderMapImpl& expected_request_headers) {
+    NiceMock<Http::MockStreamDecoder> decoder;
+    Http::StreamEncoder* response_encoder = nullptr;
+    EXPECT_CALL(callbacks_, newStream(_, _))
+        .Times(1)
+        .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& {
+          response_encoder = &encoder;
+          return decoder;
+        }));
+    EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_request_headers), true)).Times(1);
+    Buffer::OwnedImpl buffer(raw_request);
+    codec_->dispatch(buffer);
+    EXPECT_EQ(0U, buffer.length());
+    response_encoder->encodeHeaders(TestHeaderMapImpl{{":status", "200"}}, true);
+  }
+
 protected:
   uint32_t max_request_headers_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_KB};
   uint32_t max_request_headers_count_{Http::DEFAULT_MAX_HEADERS_COUNT};
@@ -167,6 +186,23 @@ TEST_F(Http1ServerConnectionImplTest, EmptyHeader) {
   EXPECT_EQ(0U, buffer.length());
 }
 
+TEST_F(Http1ServerConnectionImplTest, HostWithLWS) {
+  initialize();
+
+  TestHeaderMapImpl expected_headers{{":authority", "host"}, {":path", "/"}, {":method", "GET"}};
+
+  // Regression test spaces before and after the host header value.
+  sendAndValidateRequestAndSendResponse("GET / HTTP/1.1\r\nHost: host \r\n\r\n", expected_headers);
+
+  // Regression test tabs before and after the host header value.
+  sendAndValidateRequestAndSendResponse("GET / HTTP/1.1\r\nHost:	host	\r\n\r\n",
+                                        expected_headers);
+
+  // Regression test mixed spaces and tabs before and after the host header value.
+  sendAndValidateRequestAndSendResponse(
+      "GET / HTTP/1.1\r\nHost: 	 	  host		  	 \r\n\r\n", expected_headers);
+}
+
 TEST_F(Http1ServerConnectionImplTest, Http10) {
   initialize();
 

test/common/http/http2/BUILD

@@ -19,6 +19,7 @@ envoy_cc_test(
         "//source/common/event:dispatcher_lib",
         "//source/common/http:exception_lib",
         "//source/common/http:header_map_lib",
+        "//source/common/http:header_utility_lib",
         "//source/common/http/http2:codec_lib",
         "//source/common/stats:stats_lib",
         "//test/common/http:common_lib",

test/integration/integration_test.cc

@@ -446,13 +446,15 @@ TEST_P(IntegrationTest, TestInlineHeaders) {
 
 // Verify for HTTP/1.0 a keep-alive header results in no connection: close.
 // Also verify existing host headers are passed through for the HTTP/1.0 case.
-TEST_P(IntegrationTest, Http10WithHostandKeepAlive) {
+// This also regression tests proper handling of trailing whitespace after key
+// values, specifically the host header.
+TEST_P(IntegrationTest, Http10WithHostandKeepAliveAndLws) {
   autonomous_upstream_ = true;
   config_helper_.addConfigModifier(&setAllowHttp10WithDefaultHost);
   initialize();
   std::string response;
   sendRawHttpAndWaitForResponse(lookupPort("http"),
-                                "GET / HTTP/1.0\r\nHost: foo.com\r\nConnection:Keep-alive\r\n\r\n",
+                                "GET / HTTP/1.0\r\nHost: foo.com \r\nConnection:Keep-alive\r\n\r\n",
                                 &response, true);
   EXPECT_THAT(response, HasSubstr("HTTP/1.0 200 OK\r\n"));
   EXPECT_THAT(response, Not(HasSubstr("connection: close")));

test/integration/protocol_integration_test.cc

@@ -1467,6 +1467,32 @@ TEST_P(ProtocolIntegrationTest, ConnDurationTimeoutNoHttpRequest) {
   test_server_->waitForCounterGe("http.config_test.downstream_cx_max_duration_reached", 1);
 }
 
+// Make sure that invalid authority headers get blocked at or before the HCM.
+TEST_P(DownstreamProtocolIntegrationTest, InvalidAuth) {
+  initialize();
+  fake_upstreams_[0]->set_allow_unexpected_disconnects(true);
+
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+
+  Http::TestHeaderMapImpl request_headers{{":method", "POST"},
+                                          {":path", "/test/long/url"},
+                                          {":scheme", "http"},
+                                          {":authority", "ho|st|"}};
+
+  auto response = codec_client_->makeHeaderOnlyRequest(request_headers);
+  if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) {
+    // For HTTP/1 this is handled by the HCM, which sends a full 400 response.
+    response->waitForEndStream();
+    ASSERT_TRUE(response->complete());
+    EXPECT_EQ("400", response->headers().Status()->value().getStringView());
+  } else {
+    // For HTTP/2 this is handled by nghttp2 which resets the connection without
+    // sending an HTTP response.
+    codec_client_->waitForDisconnect();
+    ASSERT_FALSE(response->complete());
+  }
+}
+
 // For tests which focus on downstream-to-Envoy behavior, and don't need to be
 // run with both HTTP/1 and HTTP/2 upstreams.
 INSTANTIATE_TEST_SUITE_P(Protocols, DownstreamProtocolIntegrationTest,