http2: enable strict validation of HTTP/2 headers. (#25)

Fixes CVE-2019-9516.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

api/envoy/api/v2/core/protocol.proto

@@ -49,7 +49,7 @@ message Http1ProtocolOptions {
   string default_host_for_http_10 = 3;
 }
 
-// [#comment:next free field: 12]
+// [#comment:next free field: 13]
 message Http2ProtocolOptions {
   // `Maximum table size <https://httpwg.org/specs/rfc7541.html#rfc.section.4.2>`_
   // (in octets) that the encoder is permitted to use for the dynamic HPACK table. Valid values
@@ -142,6 +142,13 @@ message Http2ProtocolOptions {
   // [#comment:TODO: implement same limits for upstream inbound frames as well.]
   google.protobuf.UInt32Value max_inbound_window_update_frames_per_data_frame_sent = 11
       [(validate.rules).uint32 = {gte: 1}];
+
+  // Allows invalid HTTP messaging and headers. When this option is disabled (default), then
+  // the whole HTTP/2 connection is terminated upon receiving invalid HEADERS frame. However,
+  // when this option is enabled, only the offending stream is terminated.
+  //
+  // See [RFC7540, sec. 8.1](https://tools.ietf.org/html/rfc7540#section-8.1) for details.
+  bool stream_error_on_invalid_http_messaging = 12;
 }
 
 // [#not-implemented-hide:]

docs/root/intro/version_history.rst

@@ -34,6 +34,7 @@ Version history
 * http: added :ref:`inbound_window_update_frames_flood <config_http_conn_man_stats_per_codec>` counter stat to the HTTP/2 codec stats, for tracking number of connections terminated for exceeding the limit on inbound WINDOW_UPDATE frames. The limit is configured by setting the :ref:`max_inbound_window_update_frames_per_data_frame_sent config setting <envoy_api_field_core.Http2ProtocolOptions.max_inbound_window_update_frames_per_data_frame_sent>`.
 * http: added :ref:`outbound_flood <config_http_conn_man_stats_per_codec>` counter stat to the HTTP/2 codec stats, for tracking number of connections terminated for exceeding the outbound queue limit. The limit is configured by setting the :ref:`max_outbound_frames config setting <envoy_api_field_core.Http2ProtocolOptions.max_outbound_frames>`
 * http: added :ref:`outbound_control_flood <config_http_conn_man_stats_per_codec>` counter stat to the HTTP/2 codec stats, for tracking number of connections terminated for exceeding the outbound queue limit for PING, SETTINGS and RST_STREAM frames. The limit is configured by setting the :ref:`max_outbound_control_frames config setting <envoy_api_field_core.Http2ProtocolOptions.max_outbound_control_frames>`.
+* http: enabled strict validation of HTTP/2 messaging. Previous behavior can be restored using :ref:`stream_error_on_invalid_http_messaging config setting <envoy_api_field_core.Http2ProtocolOptions.stream_error_on_invalid_http_messaging>`.
 
 1.11.0 (July 11, 2019)
 ======================

include/envoy/http/codec.h

@@ -235,6 +235,7 @@ struct Http2Settings {
   uint32_t initial_connection_window_size_{DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE};
   bool allow_connect_{DEFAULT_ALLOW_CONNECT};
   bool allow_metadata_{DEFAULT_ALLOW_METADATA};
+  bool stream_error_on_invalid_http_messaging_{DEFAULT_STREAM_ERROR_ON_INVALID_HTTP_MESSAGING};
   uint32_t max_outbound_frames_{DEFAULT_MAX_OUTBOUND_FRAMES};
   uint32_t max_outbound_control_frames_{DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES};
   uint32_t max_consecutive_inbound_frames_with_empty_payload_{
@@ -279,6 +280,8 @@ struct Http2Settings {
   static const bool DEFAULT_ALLOW_CONNECT = false;
   // By default Envoy does not allow METADATA support.
   static const bool DEFAULT_ALLOW_METADATA = false;
+  // By default Envoy does not allow invalid headers.
+  static const bool DEFAULT_STREAM_ERROR_ON_INVALID_HTTP_MESSAGING = false;
 
   // Default limit on the number of outbound frames of all types.
   static const uint32_t DEFAULT_MAX_OUTBOUND_FRAMES = 10000;

source/common/http/http2/codec_impl.cc

@@ -579,16 +579,19 @@ int ConnectionImpl::onInvalidFrame(int32_t stream_id, int error_code) {
   ENVOY_CONN_LOG(debug, "invalid frame: {} on stream {}", connection_, nghttp2_strerror(error_code),
                  stream_id);
 
-  // The stream is about to be closed due to an invalid header or messaging. Don't kill the
-  // entire connection if one stream has bad headers or messaging.
   if (error_code == NGHTTP2_ERR_HTTP_HEADER || error_code == NGHTTP2_ERR_HTTP_MESSAGING) {
     stats_.rx_messaging_error_.inc();
-    StreamImpl* stream = getStream(stream_id);
-    if (stream != nullptr) {
-      // See comment below in onStreamClose() for why we do this.
-      stream->reset_due_to_messaging_error_ = true;
+
+    if (stream_error_on_invalid_http_messaging_) {
+      // The stream is about to be closed due to an invalid header or messaging. Don't kill the
+      // entire connection if one stream has bad headers or messaging.
+      StreamImpl* stream = getStream(stream_id);
+      if (stream != nullptr) {
+        // See comment below in onStreamClose() for why we do this.
+        stream->reset_due_to_messaging_error_ = true;
+      }
+      return 0;
     }
-    return 0;
   }
 
   // Cause dispatch to return with an error code.

source/common/http/http2/codec_impl.h

@@ -82,6 +82,8 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
       : stats_{ALL_HTTP2_CODEC_STATS(POOL_COUNTER_PREFIX(stats, "http2."))},
         connection_(connection), max_request_headers_kb_(max_request_headers_kb),
         per_stream_buffer_limit_(http2_settings.initial_stream_window_size_),
+        stream_error_on_invalid_http_messaging_(
+            http2_settings.stream_error_on_invalid_http_messaging_),
         flood_detected_(false), max_outbound_frames_(http2_settings.max_outbound_frames_),
         frame_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) {
           releaseOutboundFrame(fragment);
@@ -308,6 +310,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
   const uint32_t max_request_headers_kb_;
   uint32_t per_stream_buffer_limit_;
   bool allow_metadata_;
+  const bool stream_error_on_invalid_http_messaging_;
   bool flood_detected_;
 
   // Set if the type of frame that is about to be sent is PING or SETTINGS with the ACK flag set, or

source/common/http/utility.cc

@@ -266,6 +266,7 @@ Utility::parseHttp2Settings(const envoy::api::v2::core::Http2ProtocolOptions& co
       Http::Http2Settings::DEFAULT_MAX_INBOUND_WINDOW_UPDATE_FRAMES_PER_DATA_FRAME_SENT);
   ret.allow_connect_ = config.allow_connect();
   ret.allow_metadata_ = config.allow_metadata();
+  ret.stream_error_on_invalid_http_messaging_ = config.stream_error_on_invalid_http_messaging();
   return ret;
 }
 

test/common/http/http2/codec_impl_test.cc

@@ -97,6 +97,7 @@ class Http2CodecImplTestFixture {
     setting.initial_stream_window_size_ = ::testing::get<2>(tp);
     setting.initial_connection_window_size_ = ::testing::get<3>(tp);
     setting.allow_metadata_ = allow_metadata_;
+    setting.stream_error_on_invalid_http_messaging_ = stream_error_on_invalid_http_messaging_;
     setting.max_outbound_frames_ = max_outbound_frames_;
     setting.max_outbound_control_frames_ = max_outbound_control_frames_;
     setting.max_consecutive_inbound_frames_with_empty_payload_ =
@@ -128,6 +129,7 @@ class Http2CodecImplTestFixture {
   const Http2SettingsTuple client_settings_;
   const Http2SettingsTuple server_settings_;
   bool allow_metadata_ = false;
+  bool stream_error_on_invalid_http_messaging_ = false;
   Stats::IsolatedStoreImpl stats_store_;
   Http2Settings client_http2settings_;
   NiceMock<Network::MockConnection> client_connection_;
@@ -202,6 +204,20 @@ TEST_P(Http2CodecImplTest, ContinueHeaders) {
 TEST_P(Http2CodecImplTest, InvalidContinueWithFin) {
   initialize();
 
+  TestHeaderMapImpl request_headers;
+  HttpTestUtility::addDefaultHeaders(request_headers);
+  EXPECT_CALL(request_decoder_, decodeHeaders_(_, true));
+  request_encoder_->encodeHeaders(request_headers, true);
+
+  TestHeaderMapImpl continue_headers{{":status", "100"}};
+  EXPECT_THROW(response_encoder_->encodeHeaders(continue_headers, true), CodecProtocolException);
+  EXPECT_EQ(1, stats_store_.counter("http2.rx_messaging_error").value());
+}
+
+TEST_P(Http2CodecImplTest, InvalidContinueWithFinAllowed) {
+  stream_error_on_invalid_http_messaging_ = true;
+  initialize();
+
   MockStreamCallbacks request_callbacks;
   request_encoder_->getStream().addCallbacks(request_callbacks);
 
@@ -229,6 +245,23 @@ TEST_P(Http2CodecImplTest, InvalidContinueWithFin) {
 TEST_P(Http2CodecImplTest, InvalidRepeatContinue) {
   initialize();
 
+  TestHeaderMapImpl request_headers;
+  HttpTestUtility::addDefaultHeaders(request_headers);
+  EXPECT_CALL(request_decoder_, decodeHeaders_(_, true));
+  request_encoder_->encodeHeaders(request_headers, true);
+
+  TestHeaderMapImpl continue_headers{{":status", "100"}};
+  EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_));
+  response_encoder_->encode100ContinueHeaders(continue_headers);
+
+  EXPECT_THROW(response_encoder_->encodeHeaders(continue_headers, true), CodecProtocolException);
+  EXPECT_EQ(1, stats_store_.counter("http2.rx_messaging_error").value());
+};
+
+TEST_P(Http2CodecImplTest, InvalidRepeatContinueAllowed) {
+  stream_error_on_invalid_http_messaging_ = true;
+  initialize();
+
   MockStreamCallbacks request_callbacks;
   request_encoder_->getStream().addCallbacks(request_callbacks);
 
@@ -280,6 +313,28 @@ TEST_P(Http2CodecImplTest, Invalid103) {
 TEST_P(Http2CodecImplTest, Invalid204WithContentLength) {
   initialize();
 
+  TestHeaderMapImpl request_headers;
+  HttpTestUtility::addDefaultHeaders(request_headers);
+  EXPECT_CALL(request_decoder_, decodeHeaders_(_, true));
+  request_encoder_->encodeHeaders(request_headers, true);
+
+  TestHeaderMapImpl response_headers{{":status", "204"}, {"content-length", "3"}};
+  // What follows is a hack to get headers that should span into continuation frames. The default
+  // maximum frame size is 16K. We will add 3,000 headers that will take us above this size and
+  // not easily compress with HPACK. (I confirmed this generates 26,468 bytes of header data
+  // which should contain a continuation.)
+  for (uint i = 1; i < 3000; i++) {
+    response_headers.addCopy(std::to_string(i), std::to_string(i));
+  }
+
+  EXPECT_THROW(response_encoder_->encodeHeaders(response_headers, false), CodecProtocolException);
+  EXPECT_EQ(1, stats_store_.counter("http2.rx_messaging_error").value());
+};
+
+TEST_P(Http2CodecImplTest, Invalid204WithContentLengthAllowed) {
+  stream_error_on_invalid_http_messaging_ = true;
+  initialize();
+
   MockStreamCallbacks request_callbacks;
   request_encoder_->getStream().addCallbacks(request_callbacks);
 
@@ -329,7 +384,15 @@ TEST_P(Http2CodecImplTest, RefusedStreamReset) {
   response_encoder_->getStream().resetStream(StreamResetReason::LocalRefusedStreamReset);
 }
 
-TEST_P(Http2CodecImplTest, InvalidFrame) {
+TEST_P(Http2CodecImplTest, InvalidHeadersFrame) {
+  initialize();
+
+  EXPECT_THROW(request_encoder_->encodeHeaders(TestHeaderMapImpl{}, true), CodecProtocolException);
+  EXPECT_EQ(1, stats_store_.counter("http2.rx_messaging_error").value());
+}
+
+TEST_P(Http2CodecImplTest, InvalidHeadersFrameAllowed) {
+  stream_error_on_invalid_http_messaging_ = true;
   initialize();
 
   MockStreamCallbacks request_callbacks;

test/common/http/http2/http2_frame.cc

@@ -104,6 +104,12 @@ void Http2Frame::appendHeaderWithoutIndexing(StaticHeaderIndex index, absl::stri
   appendData(value);
 }
 
+void Http2Frame::appendEmptyHeader() {
+  data_.push_back(0x40);
+  data_.push_back(0x00);
+  data_.push_back(0x00);
+}
+
 Http2Frame Http2Frame::makePingFrame(absl::string_view data) {
   static constexpr size_t kPingPayloadSize = 8;
   Http2Frame frame;
@@ -169,6 +175,21 @@ Http2Frame Http2Frame::makeMalformedRequest(uint32_t stream_index) {
   return frame;
 }
 
+Http2Frame Http2Frame::makeMalformedRequestWithZerolenHeader(uint32_t stream_index,
+                                                             absl::string_view host,
+                                                             absl::string_view path) {
+  Http2Frame frame;
+  frame.buildHeader(Type::HEADERS, 0, orFlags(HeadersFlags::END_STREAM, HeadersFlags::END_HEADERS),
+                    makeRequestStreamId(stream_index));
+  frame.appendStaticHeader(StaticHeaderIndex::METHOD_GET);
+  frame.appendStaticHeader(StaticHeaderIndex::SCHEME_HTTPS);
+  frame.appendHeaderWithoutIndexing(StaticHeaderIndex::PATH, path);
+  frame.appendHeaderWithoutIndexing(StaticHeaderIndex::HOST, host);
+  frame.appendEmptyHeader();
+  frame.adjustPayloadSize();
+  return frame;
+}
+
 Http2Frame Http2Frame::makeRequest(uint32_t stream_index, absl::string_view host,
                                    absl::string_view path) {
   Http2Frame frame;

test/common/http/http2/http2_frame.h

@@ -79,6 +79,9 @@ class Http2Frame {
   static Http2Frame makePriorityFrame(uint32_t stream_index, uint32_t dependent_index);
   static Http2Frame makeWindowUpdateFrame(uint32_t stream_index, uint32_t increment);
   static Http2Frame makeMalformedRequest(uint32_t stream_index);
+  static Http2Frame makeMalformedRequestWithZerolenHeader(uint32_t stream_index,
+                                                          absl::string_view host,
+                                                          absl::string_view path);
   static Http2Frame makeRequest(uint32_t stream_index, absl::string_view host,
                                 absl::string_view path);
   static Http2Frame makePostRequest(uint32_t stream_index, absl::string_view host,
@@ -126,6 +129,7 @@ class Http2Frame {
   // Headers are directly encoded
   void appendStaticHeader(StaticHeaderIndex index);
   void appendHeaderWithoutIndexing(StaticHeaderIndex index, absl::string_view value);
+  void appendEmptyHeader();
 
   // This method updates payload length in the HTTP2 header based on the size of the data_
   void adjustPayloadSize() {

test/integration/http2_integration_test.cc

@@ -1584,6 +1584,12 @@ TEST_P(Http2FloodMitigationTest, Data) {
 
 // Verify that the server can detect flood of RST_STREAM frames.
 TEST_P(Http2FloodMitigationTest, RST_STREAM) {
+  // Use invalid HTTP headers to trigger sending RST_STREAM frames.
+  config_helper_.addConfigModifier(
+      [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm)
+          -> void {
+        hcm.mutable_http2_protocol_options()->set_stream_error_on_invalid_http_messaging(true);
+      });
   beginSession();
 
   int i = 0;
@@ -1717,4 +1723,54 @@ TEST_P(Http2FloodMitigationTest, WindowUpdate) {
               "http2.inbound_window_update_frames_flood");
 }
 
+// Verify that the HTTP/2 connection is terminated upon receiving invalid HEADERS frame.
+TEST_P(Http2FloodMitigationTest, ZerolenHeader) {
+  beginSession();
+
+  // Send invalid request.
+  uint32_t request_idx = 0;
+  auto request = Http2Frame::makeMalformedRequestWithZerolenHeader(request_idx, "host", "/");
+  sendFame(request);
+
+  tcp_client_->waitForDisconnect();
+
+  EXPECT_EQ(1, test_server_->counter("http2.rx_messaging_error")->value());
+  EXPECT_EQ(1,
+            test_server_->counter("http.config_test.downstream_cx_delayed_close_timeout")->value());
+}
+
+// Verify that only the offending stream is terminated upon receiving invalid HEADERS frame.
+TEST_P(Http2FloodMitigationTest, ZerolenHeaderAllowed) {
+  config_helper_.addConfigModifier(
+      [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm)
+          -> void {
+        hcm.mutable_http2_protocol_options()->set_stream_error_on_invalid_http_messaging(true);
+      });
+  autonomous_upstream_ = true;
+  beginSession();
+  fake_upstreams_[0]->set_allow_unexpected_disconnects(true);
+
+  // Send invalid request.
+  uint32_t request_idx = 0;
+  auto request = Http2Frame::makeMalformedRequestWithZerolenHeader(request_idx, "host", "/");
+  sendFame(request);
+  // Make sure we've got RST_STREAM from the server.
+  auto response = readFrame();
+  EXPECT_EQ(Http2Frame::Type::RST_STREAM, response.type());
+
+  // Send valid request using the same connection.
+  request_idx++;
+  request = Http2Frame::makeRequest(request_idx, "host", "/");
+  sendFame(request);
+  response = readFrame();
+  EXPECT_EQ(Http2Frame::Type::HEADERS, response.type());
+  EXPECT_EQ(Http2Frame::ResponseStatus::_200, response.responseStatus());
+
+  tcp_client_->close();
+
+  EXPECT_EQ(1, test_server_->counter("http2.rx_messaging_error")->value());
+  EXPECT_EQ(0,
+            test_server_->counter("http.config_test.downstream_cx_delayed_close_timeout")->value());
+}
+
 } // namespace Envoy

test/integration/protocol_integration_test.cc

@@ -592,6 +592,36 @@ TEST_P(DownstreamProtocolIntegrationTest, InvalidContentLength) {
                                                           {"content-length", "-1"}});
   auto response = std::move(encoder_decoder.second);
 
+  codec_client_->waitForDisconnect();
+
+  if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
+    ASSERT_TRUE(response->complete());
+    EXPECT_EQ("400", response->headers().Status()->value().getStringView());
+  } else {
+    ASSERT_TRUE(response->reset());
+    EXPECT_EQ(Http::StreamResetReason::ConnectionTermination, response->reset_reason());
+  }
+}
+
+// TODO(PiotrSikora): move this HTTP/2 only variant to http2_integration_test.cc.
+TEST_P(DownstreamProtocolIntegrationTest, InvalidContentLengthAllowed) {
+  config_helper_.addConfigModifier(
+      [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm)
+          -> void {
+        hcm.mutable_http2_protocol_options()->set_stream_error_on_invalid_http_messaging(true);
+      });
+
+  initialize();
+
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+
+  auto encoder_decoder =
+      codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", "POST"},
+                                                          {":path", "/test/long/url"},
+                                                          {":authority", "host"},
+                                                          {"content-length", "-1"}});
+  auto response = std::move(encoder_decoder.second);
+
   if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
     codec_client_->waitForDisconnect();
   } else {
@@ -618,6 +648,34 @@ TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengths) {
                                                           {"content-length", "3,2"}});
   auto response = std::move(encoder_decoder.second);
 
+  codec_client_->waitForDisconnect();
+
+  if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
+    ASSERT_TRUE(response->complete());
+    EXPECT_EQ("400", response->headers().Status()->value().getStringView());
+  } else {
+    ASSERT_TRUE(response->reset());
+    EXPECT_EQ(Http::StreamResetReason::ConnectionTermination, response->reset_reason());
+  }
+}
+
+// TODO(PiotrSikora): move this HTTP/2 only variant to http2_integration_test.cc.
+TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengthsAllowed) {
+  config_helper_.addConfigModifier(
+      [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm)
+          -> void {
+        hcm.mutable_http2_protocol_options()->set_stream_error_on_invalid_http_messaging(true);
+      });
+
+  initialize();
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto encoder_decoder =
+      codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", "POST"},
+                                                          {":path", "/test/long/url"},
+                                                          {":authority", "host"},
+                                                          {"content-length", "3,2"}});
+  auto response = std::move(encoder_decoder.second);
+
   if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
     codec_client_->waitForDisconnect();
   } else {