http2: fix stream flush timeout race with protocol error (#181)

Fixes envoyproxy/envoy-setec#180

Signed-off-by: Matt Klein <[email protected]>

docs/root/configuration/best_practices/edge.rst

@@ -30,8 +30,7 @@ HTTP proxies should additionally configure:
 The following is a YAML example of the above recommendation (taken from the :ref:`Google VRP
 <arch_overview_google_vrp>` edge server configuration):
 
-.. literalinclude:: envoy-edge.yaml
-  :language: yaml
+.. code-block:: yaml
 
   overload_manager:
     refresh_interval: 0.25s

docs/root/version_history/current.rst

@@ -39,10 +39,16 @@ Bug Fixes
 * adaptive concurrency: fixed a minRTT calculation bug where requests started before the concurrency
   limit was pinned to the minimum would skew the new minRTT value if the replies arrived after the
   start of the new minRTT window.
+* buffer: fixed CVE-2020-12603 by avoiding fragmentation, and tracking of HTTP/2 data and control frames in the output buffer.
 * grpc-json: fix a bug when in trailers only gRPC response (e.g. error) HTTP status code is not being re-written.
 * http: fixed a bug in the grpc_http1_reverse_bridge filter where header-only requests were forwarded with a non-zero content length.
 * http: fixed a bug where in some cases slash was moved from path to query string when :ref:`merging of adjacent slashes<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.merge_slashes>` is enabled.
+* http: fixed CVE-2020-12604 by changing :ref:`stream_idle_timeout <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.stream_idle_timeout>`
+  to also defend against an HTTP/2 peer that does not open stream window once an entire response has been buffered to be sent to a downstream client.
+* http: fixed CVE-2020-12605 by including request URL in request header size computation, and rejecting partial headers that exceed configured limits.
 * http: fixed several bugs with applying correct connection close behavior across the http connection manager, health checker, and connection pool. This behavior may be temporarily reverted by setting runtime feature `envoy.reloadable_features.fix_connection_close` to false.
+* listener: fixed CVE-2020-8663 by adding runtime support for :ref:`per-listener limits <config_listeners_runtime>` on active/accepted connections.
+* overload management: fixed CVE-2020-8663 by adding runtime support for :ref:`global limits <config_overload_manager>` on active/accepted connections.
 * prometheus stats: fix the sort order of output lines to comply with the standard.
 * udp: the :ref:`reuse_port <envoy_api_field_Listener.reuse_port>` listener option must now be
   specified for UDP listeners if concurrency is > 1. This previously crashed so is considered a

docs/root/version_history/v1.12.5.rst

@@ -0,0 +1,11 @@
+1.12.5 (June 30, 2020)
+======================
+
+Changes
+-------
+* buffer: fixed CVE-2020-12603 by avoiding fragmentation, and tracking of HTTP/2 data and control frames in the output buffer.
+* http: fixed CVE-2020-12604 by changing :ref:`stream_idle_timeout <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.stream_idle_timeout>`
+  to also defend against an HTTP/2 peer that does not open stream window once an entire response has been buffered to be sent to a downstream client.
+* http: fixed CVE-2020-12605 by including request URL in request header size computation, and rejecting partial headers that exceed configured limits.
+* listener: fixed CVE-2020-8663 by adding runtime support for :ref:`per-listener limits <config_listeners_runtime>` on active/accepted connections.
+* overload management: fixed CVE-2020-8663 by adding runtime support for :ref:`global limits <config_overload_manager>` on active/accepted connections.

docs/root/version_history/v1.13.3.rst

@@ -0,0 +1,12 @@
+1.13.3 (June 30, 2020)
+======================
+
+Changes
+-------
+
+* buffer: fixed CVE-2020-12603 by avoiding fragmentation, and tracking of HTTP/2 data and control frames in the output buffer.
+* http: fixed CVE-2020-12604 by changing :ref:`stream_idle_timeout <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.stream_idle_timeout>`
+  to also defend against an HTTP/2 peer that does not open stream window once an entire response has been buffered to be sent to a downstream client.
+* http: fixed CVE-2020-12605 by including request URL in request header size computation, and rejecting partial headers that exceed configured limits.
+* listener: fixed CVE-2020-8663 by adding runtime support for :ref:`per-listener limits <config_listeners_runtime>` on active/accepted connections.
+* overload management: fixed CVE-2020-8663 by adding runtime support for :ref:`global limits <config_overload_manager>` on active/accepted connections.

docs/root/version_history/v1.14.3.rst

@@ -0,0 +1,11 @@
+1.14.3 (June 30, 2020)
+======================
+
+Changes
+-------
+* buffer: fixed CVE-2020-12603 by avoiding fragmentation, and tracking of HTTP/2 data and control frames in the output buffer.
+* http: fixed CVE-2020-12604 by changing :ref:`stream_idle_timeout <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.stream_idle_timeout>`
+  to also defend against an HTTP/2 peer that does not open stream window once an entire response has been buffered to be sent to a downstream client.
+* http: fixed CVE-2020-12605 by including request URL in request header size computation, and rejecting partial headers that exceed configured limits.
+* listener: fixed CVE-2020-8663 by adding runtime support for :ref:`per-listener limits <config_listeners_runtime>` on active/accepted connections.
+* overload management: fixed CVE-2020-8663 by adding runtime support for :ref:`global limits <config_overload_manager>` on active/accepted connections.

docs/root/version_history/version_history.rst

@@ -7,12 +7,15 @@ Version history
   :titlesonly:
 
   current
+  v1.14.3
   v1.14.2
   v1.14.1
   v1.14.0
+  v1.13.3
   v1.13.2
   v1.13.1
   v1.13.0
+  v1.12.5
   v1.12.4
   v1.12.3
   v1.12.2

source/common/http/http2/codec_impl.cc

@@ -105,12 +105,7 @@ ConnectionImpl::StreamImpl::StreamImpl(ConnectionImpl& parent, uint32_t buffer_l
 ConnectionImpl::StreamImpl::~StreamImpl() { ASSERT(stream_idle_timer_ == nullptr); }
 
 void ConnectionImpl::StreamImpl::destroy() {
-  if (stream_idle_timer_ != nullptr) {
-    // To ease testing and the destructor assertion.
-    stream_idle_timer_->disableTimer();
-    stream_idle_timer_.reset();
-  }
-
+  disarmStreamIdleTimer();
   parent_.stats_.streams_active_.dec();
   parent_.stats_.pending_send_bytes_.sub(pending_send_data_.length());
 }
@@ -733,6 +728,15 @@ int ConnectionImpl::onFrameSend(const nghttp2_frame* frame) {
   case NGHTTP2_GOAWAY: {
     ENVOY_CONN_LOG(debug, "sent goaway code={}", connection_, frame->goaway.error_code);
     if (frame->goaway.error_code != NGHTTP2_NO_ERROR) {
+      // TODO(mattklein123): Returning this error code abandons standard nghttp2 frame accounting.
+      // As such, it is not reliable to call sendPendingFrames() again after this and we assume
+      // that the connection is going to get torn down immediately. One byproduct of this is that
+      // we need to cancel all pending flush stream timeouts since they can race with connection
+      // teardown. As part of the work to remove exceptions we should aim to clean up all of this
+      // error handling logic and only handle this type of case at the end of dispatch.
+      for (auto& stream : active_streams_) {
+        stream->disarmStreamIdleTimer();
+      }
       return NGHTTP2_ERR_CALLBACK_FAILURE;
     }
     break;

source/common/http/http2/codec_impl.h

@@ -173,6 +173,13 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
     // deferred delete lifetime issues that need sorting out if the destructor of the stream is
     // going to be able to refer to the parent connection.
     void destroy();
+    void disarmStreamIdleTimer() {
+      if (stream_idle_timer_ != nullptr) {
+        // To ease testing and the destructor assertion.
+        stream_idle_timer_->disableTimer();
+        stream_idle_timer_.reset();
+      }
+    }
 
     StreamImpl* base() { return this; }
     ssize_t onDataSourceRead(uint64_t length, uint32_t* data_flags);

test/common/http/http2/codec_impl_test.cc

@@ -1121,6 +1121,41 @@ TEST_P(Http2CodecImplFlowControlTest, LargeServerBodyFlushTimeout) {
   EXPECT_EQ(1, server_stats_store_.counter("http2.tx_flush_timeout").value());
 }
 
+// Verify that when an incoming protocol error races with a stream flush timeout we correctly
+// disable the flush timeout and do not attempt to reset the stream.
+TEST_P(Http2CodecImplFlowControlTest, LargeServerBodyFlushTimeoutAfterGoaway) {
+  initialize();
+
+  InSequence s;
+  MockStreamCallbacks client_stream_callbacks;
+  request_encoder_->getStream().addCallbacks(client_stream_callbacks);
+  TestRequestHeaderMapImpl request_headers;
+  HttpTestUtility::addDefaultHeaders(request_headers);
+  EXPECT_CALL(request_decoder_, decodeHeaders_(_, true));
+  request_encoder_->encodeHeaders(request_headers, true);
+
+  ON_CALL(client_connection_, write(_, _))
+      .WillByDefault(
+          Invoke([&](Buffer::Instance& data, bool) -> void { server_wrapper_.buffer_.add(data); }));
+  TestResponseHeaderMapImpl response_headers{{":status", "200"}};
+  EXPECT_CALL(response_decoder_, decodeHeaders_(_, false));
+  response_encoder_->encodeHeaders(response_headers, false);
+  EXPECT_CALL(response_decoder_, decodeData(_, false)).Times(AtLeast(1));
+  auto flush_timer = new Event::MockTimer(&server_connection_.dispatcher_);
+  EXPECT_CALL(*flush_timer, enableTimer(std::chrono::milliseconds(30000), _));
+  Buffer::OwnedImpl body(std::string(1024 * 1024, 'a'));
+  response_encoder_->encodeData(body, true);
+
+  // Force a protocol error.
+  Buffer::OwnedImpl garbage_data("this should cause a protocol error");
+  EXPECT_CALL(client_callbacks_, onGoAway(_));
+  EXPECT_CALL(*flush_timer, disableTimer());
+  EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(0);
+  auto status = server_wrapper_.dispatch(garbage_data, *server_);
+  EXPECT_FALSE(status.ok());
+  EXPECT_EQ(0, server_stats_store_.counter("http2.tx_flush_timeout").value());
+}
+
 TEST_P(Http2CodecImplTest, WatermarkUnderEndStream) {
   initialize();
   MockStreamCallbacks callbacks;

test/extensions/transport_sockets/alts/alts_integration_test.cc

@@ -332,7 +332,7 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, AltsIntegrationTestCapturingHandshaker,
 // Verifies that handshake request should include ALTS version.
 TEST_P(AltsIntegrationTestCapturingHandshaker, CheckAltsVersion) {
   initialize();
-  codec_client_ = makeRawHttpConnection(makeAltsConnection());
+  codec_client_ = makeRawHttpConnection(makeAltsConnection(), absl::nullopt);
   EXPECT_FALSE(codec_client_->connected());
   EXPECT_EQ(capturing_handshaker_service_->client_versions.max_rpc_version().major(),
             capturing_handshaker_service_->server_versions.max_rpc_version().major());

test/integration/drain_close_integration_test.cc

@@ -116,7 +116,7 @@ TEST_P(DrainCloseIntegrationTest, AdminGracefulDrain) {
   }
 
   // New connections can still be made.
-  auto second_codec_client_ = makeRawHttpConnection(makeClientConnection(http_port));
+  auto second_codec_client_ = makeRawHttpConnection(makeClientConnection(http_port), absl::nullopt);
   EXPECT_TRUE(second_codec_client_->connected());
 
   // Invoke /drain_listeners and shut down listeners.

test/integration/http_integration.cc

@@ -988,7 +988,7 @@ void HttpIntegrationTest::testLargeRequestUrl(uint32_t url_size, uint32_t max_he
     auto response = std::move(encoder_decoder.second);
 
     if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
-      codec_client_->waitForDisconnect();
+      ASSERT_TRUE(codec_client_->waitForDisconnect());
       EXPECT_TRUE(response->complete());
       EXPECT_EQ("431", response->headers().Status()->value().getStringView());
     } else {