http: Fix memory leak in nghttp2 codec

Fix memory leak in nghttp2 when it processes pending requests after
receiving the GOAWAY frame.

Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Ryan Northey <ryan@synca.io>

bazel/foreign_cc/nghttp2.patch

@@ -15,3 +15,37 @@ diff -u -r a/CMakeLists.txt b/CMakeLists.txt
  # AC_TYPE_UINT8_T
  # AC_TYPE_UINT16_T
 Only in b: CMakeLists.txt.orig
+--- a/lib/nghttp2_session.c	2023-06-14 00:17:10.311464189 +0000
++++ b/lib/nghttp2_session.c	2023-06-14 00:18:49.551376483 +0000
+@@ -3294,6 +3294,7 @@
+         break;
+       }
+       if (rv < 0) {
++        int rv2 = 0;
+         int32_t opened_stream_id = 0;
+         uint32_t error_code = NGHTTP2_INTERNAL_ERROR;
+
+@@ -3338,19 +3339,18 @@
+         }
+         if (opened_stream_id) {
+           /* careful not to override rv */
+-          int rv2;
+           rv2 = nghttp2_session_close_stream(session, opened_stream_id,
+                                              error_code);
+-
+-          if (nghttp2_is_fatal(rv2)) {
+-            return rv2;
+-          }
+         }
+
+         nghttp2_outbound_item_free(item, mem);
+         nghttp2_mem_free(mem, item);
+         active_outbound_item_reset(aob, mem);
+
++        if (nghttp2_is_fatal(rv2)) {
++          return rv2;
++        }
++        
+         if (rv == NGHTTP2_ERR_HEADER_COMP) {
+           /* If header compression error occurred, should terminiate
+              connection. */

changelogs/current.yaml

@@ -12,6 +12,10 @@ bug_fixes:
   change: |
     Fixes an issue with the ORIGINAL_DST cluster cleanup timer lifetime, which
     can occur if the cluster is removed while the timer is armed.
+- area: http2
+  change: |
+    Fix memory leak in nghttp2 when scheduled requests are cancelled due to the ``GOAWAY`` frame being received from the
+    upstream service. Fix `CVE-2023-35945 <https://nvd.nist.gov/vuln/detail/CVE-2023-35945>`_.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

test/integration/filters/test_socket_interface.cc

@@ -58,6 +58,16 @@ IoHandlePtr TestIoSocketHandle::duplicate() {
                                               domain_);
 }
 
+Api::SysCallIntResult TestIoSocketHandle::connect(Address::InstanceConstSharedPtr address) {
+  if (write_override_) {
+    auto result = write_override_(this, nullptr, 0);
+    if (result.has_value()) {
+      return Api::SysCallIntResult{-1, EINPROGRESS};
+    }
+  }
+  return Test::IoSocketHandlePlatformImpl::connect(address);
+}
+
 IoHandlePtr TestSocketInterface::makeSocket(int socket_fd, bool socket_v6only,
                                             absl::optional<int> domain) const {
   return std::make_unique<TestIoSocketHandle>(write_override_proc_, socket_fd, socket_v6only,

test/integration/filters/test_socket_interface.h

@@ -61,6 +61,7 @@ class TestIoSocketHandle : public Test::IoSocketHandlePlatformImpl {
 
 private:
   IoHandlePtr accept(struct sockaddr* addr, socklen_t* addrlen) override;
+  Api::SysCallIntResult connect(Address::InstanceConstSharedPtr address) override;
   Api::IoCallUint64Result writev(const Buffer::RawSlice* slices, uint64_t num_slice) override;
   Api::IoCallUint64Result sendmsg(const Buffer::RawSlice* slices, uint64_t num_slice, int flags,
                                   const Address::Ip* self_ip,

test/integration/http2_flood_integration_test.cc

@@ -1552,4 +1552,59 @@ TEST_P(Http2FloodMitigationTest, UpstreamRstStreamStormOnDownstreamCloseRegressi
   EXPECT_EQ(2, test_server_->gauge("cluster.cluster_0.upstream_cx_active")->value());
 }
 
+// This test validates fix for memory leak in nghttp2 that occurs when
+// nghttp2 processes RST_STREAM and GO_AWAY in the same I/O operation.
+// Max concurrent request should be limited to 1.
+// nghttp2 leaks HEADERS frame and bookkeeping structure.
+// The failure occurs in the ASAN build only due to leak checker.
+#ifndef WIN32
+// Windows does not support override of the socket connect() call
+TEST_P(Http2FloodMitigationTest, GoAwayAfterRequestReset) {
+  setDownstreamProtocol(Http::CodecType::HTTP2);
+  setUpstreamProtocol(Http::CodecType::HTTP2);
+  // Limit concurrent requests to the upstream to 1
+  config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
+    ConfigHelper::HttpProtocolOptions protocol_options;
+    auto* http_protocol_options =
+        protocol_options.mutable_explicit_http_config()->mutable_http2_protocol_options();
+    http_protocol_options->mutable_max_concurrent_streams()->set_value(1);
+    ConfigHelper::setProtocolOptions(*bootstrap.mutable_static_resources()->mutable_clusters(0),
+                                     protocol_options);
+  });
+
+  initialize();
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto response_1 = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+  waitForNextUpstreamRequest();
+
+  // After we've got the first request at the upstream, make the connect() call return EAGAIN (and
+  // never connect). This will make the second request to hang in Envoy waiting for either the first
+  // connection to free up capacity (i.e. finish with the first request) or the second connection to
+  // connect, which will never happen.
+  auto* upstream = fake_upstreams_.front().get();
+  write_matcher_->setDestinationPort(upstream->localAddress()->ip()->port());
+  write_matcher_->setConnectBlock(true);
+
+  auto response_2 = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+
+  // The RST_STREAM will cause the capacity to free up on the first connection and the second
+  // request to be scheduled on the first connection.
+  // The GOAWAY frame will cause nghttp2 to not send any more requests. Without the fix this
+  // leaks scheduled request and it is detected by ASAN run.
+  const auto rst_stream = Http2Frame::makeResetStreamFrame(Http2Frame::makeClientStreamId(0),
+                                                           Http2Frame::ErrorCode::NoError);
+  const auto go_away = Http2Frame::makeEmptyGoAwayFrame(0, Http2Frame::ErrorCode::NoError);
+  std::string buf(static_cast<std::string>(rst_stream));
+  buf.append(static_cast<std::string>(go_away));
+  ASSERT_TRUE(upstream->rawWriteConnection(0, buf));
+
+  ASSERT_TRUE(response_1->waitForEndStream());
+  ASSERT_TRUE(response_2->waitForEndStream());
+  ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
+
+  EXPECT_EQ("503", response_1->headers().getStatusValue());
+  EXPECT_EQ("503", response_2->headers().getStatusValue());
+}
+#endif
+
 } // namespace Envoy

test/integration/socket_interface_swap.cc

@@ -8,12 +8,22 @@ SocketInterfaceSwap::SocketInterfaceSwap() {
   Envoy::Network::SocketInterfaceSingleton::clear();
   test_socket_interface_loader_ = std::make_unique<Envoy::Network::SocketInterfaceLoader>(
       std::make_unique<Envoy::Network::TestSocketInterface>(
-          [write_matcher = write_matcher_](Envoy::Network::TestIoSocketHandle* io_handle,
-                                           const Buffer::RawSlice*,
-                                           uint64_t) -> absl::optional<Api::IoCallUint64Result> {
-            Network::IoSocketError* error_override = write_matcher->returnOverride(io_handle);
-            if (error_override) {
-              return Api::IoCallUint64Result(0, Api::IoErrorPtr(error_override, preserveIoError));
+          [write_matcher = write_matcher_](
+              Envoy::Network::TestIoSocketHandle* io_handle, const Buffer::RawSlice* slices,
+              uint64_t size) -> absl::optional<Api::IoCallUint64Result> {
+            // TODO(yanavlasov): refactor into separate method after CVE is public.
+            if (slices == nullptr && size == 0) {
+              // This is connect override check
+              Network::IoSocketError* error_override =
+                  write_matcher->returnConnectOverride(io_handle);
+              if (error_override) {
+                return Api::IoCallUint64Result(0, Api::IoErrorPtr(error_override, preserveIoError));
+              }
+            } else {
+              Network::IoSocketError* error_override = write_matcher->returnOverride(io_handle);
+              if (error_override) {
+                return Api::IoCallUint64Result(0, Api::IoErrorPtr(error_override, preserveIoError));
+              }
             }
             return absl::nullopt;
           }));

test/integration/socket_interface_swap.h

@@ -26,6 +26,15 @@ class SocketInterfaceSwap {
       return nullptr;
     }
 
+    Network::IoSocketError* returnConnectOverride(Envoy::Network::TestIoSocketHandle* io_handle) {
+      absl::MutexLock lock(&mutex_);
+      if (block_connect_ && (io_handle->localAddress()->ip()->port() == src_port_ ||
+                             (dst_port_ && io_handle->peerAddress()->ip()->port() == dst_port_))) {
+        return Network::IoSocketError::getIoSocketEagainInstance();
+      }
+      return nullptr;
+    }
+
     // Source port to match. The port specified should be associated with a listener.
     void setSourcePort(uint32_t port) {
       absl::WriterMutexLock lock(&mutex_);
@@ -51,6 +60,12 @@ class SocketInterfaceSwap {
       error_ = error;
     }
 
+    void setConnectBlock(bool block) {
+      absl::WriterMutexLock lock(&mutex_);
+      ASSERT(src_port_ != 0 || dst_port_ != 0);
+      block_connect_ = block;
+    }
+
     void setResumeWrites();
 
   private:
@@ -59,6 +74,7 @@ class SocketInterfaceSwap {
     uint32_t dst_port_ ABSL_GUARDED_BY(mutex_) = 0;
     Network::IoSocketError* error_ ABSL_GUARDED_BY(mutex_) = nullptr;
     Network::TestIoSocketHandle* matched_iohandle_{};
+    bool block_connect_ ABSL_GUARDED_BY(mutex_) = false;
   };
 
   SocketInterfaceSwap();