CVE-2022-23606 (C++, published 2022-02-22T23:15Z)
Description: value: Envoy is an open source edge and service proxy, designed for cloud-native applications. When a cluster is deleted via Cluster Discovery Service (CDS) all idle connections established to endpoints in that cluster are disconnected. A recursion was introduced in the procedure of disconnecting idle connections that can lead to stack exhaustion and abnormal process termination when a cluster has a large number of idle connections. This infinite recursion causes Envoy to crash. Users are advised to upgrade.
Severity: MEDIUM (CVSS v3 base score: 6.5)
Weakness: CWE-674 \u2014 Uncontrolled Recursion
The product does not properly control the amount of recursion that takes place,  consuming excessive resources, such as allocated memory or the program stack.
Repository: https://github.com/envoyproxy/envoy
Files changed:
docs/root/version_history/current.rst
source/common/conn_pool/BUILD
source/common/conn_pool/conn_pool_base.cc
source/common/conn_pool/conn_pool_base.h
test/config/utility.cc
test/config/utility.h
test/integration/BUILD
test/integration/cds_integration_test.cc
Commit message: CVE-2022-23606

Avoid closing other connections to prevent deep recursion when a large number of idle connections are closed at the start of a pool drain, when a connection is closed.

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


Vulnerable code:
void ConnPoolImplBase::checkForIdleAndCloseIdleConnsIfDraining() {
  if (is_draining_for_deletion_) {
    closeIdleConnectionsForDrainingPool();
  }

    checkForIdleAndCloseIdleConnsIfDraining();
                                    : "DELTA_GRPC")) {
    addFakeUpstream(Http::CodecType::HTTP2);
    addFakeUpstream(Http::CodecType::HTTP2);
    cluster1_ = ConfigHelper::buildStaticCluster(
        Network::Test::getLoopbackAddressString(ipVersion()));
    cluster2_ = ConfigHelper::buildStaticCluster(
        Network::Test::getLoopbackAddressString(ipVersion()));

Fixed code:
* upstream: fix stack overflow when a cluster with large number of idle connections is removed.
        "//source/common/common:debug_recursion_checker_lib",
#include "source/common/common/debug_recursion_checker.h"
  Common::AutoDebugRecursionChecker assert_not_in(recursion_checker_);

void ConnPoolImplBase::checkForIdleAndNotify() {
void ConnPoolImplBase::checkForIdleAndCloseIdleConnsIfDraining() {
  if (is_draining_for_deletion_) {
    closeIdleConnectionsForDrainingPool();
  }

  checkForIdleAndNotify();
}

    // Check if the pool transitioned to idle state after removing closed client
    // from one of the client tracking lists.
    // There is no need to check if other connections are idle in a draining pool
    // because the pool will close all idle connection when it is starting to
    // drain.
    // Trying to close other connections here can lead to deep recursion when
    // a large number idle connections are closed at the start of pool drain.
    // See CdsIntegrationTest.CdsClusterDownWithLotsOfIdleConnections for an example.
    checkForIdleAndNotify();
#include "source/common/common/debug_recursion_checker.h"
  // Check if the pool has gone idle and invoke idle notification callbacks.
  void checkForIdleAndNotify();

  Common::DebugRecursionChecker recursion_checker_;
envoy::config::cluster::v3::Cluster ConfigHelper::buildH1ClusterWithHighCircuitBreakersLimits(
    const std::string& name, int port, const std::string& address, const std::string& lb_policy) {
  return TestUtility::parseYaml<envoy::config::cluster::v3::Cluster>(
      fmt::format(R"EOF(
      name: {}
      connect_timeout: 50s
      type: STATIC
      circuit_breakers:
        thresholds:
        - priority: DEFAULT
          max_connections: 10000
          max_pending_requests: 10000
          max_requests: 10000
          max_retries: 10000
      load_assignment:
        cluster_name: {}
        endpoints:
        - lb_endpoints:
          - endpoint:
              address:
                socket_address:
                  address: {}
                  port_value: {}
      lb_policy: {}
    )EOF",
                  name, name, address, port, lb_policy));
}

  static envoy::config::cluster::v3::Cluster
  buildH1ClusterWithHighCircuitBreakersLimits(const std::string& name, int port,
                                              const std::string& address,
                                              const std::string& lb_policy = "ROUND_ROBIN");

    size = "large",
    shard_count = 4,
                                    : "DELTA_GRPC")),
        cluster_creator_(&ConfigHelper::buildStaticCluster) {
    addFakeUpstream(upstream_codec_type_);
    addFakeUpstream(upstream_codec_type_);
    cluster1_ = cluster_creator_(
        Network::Test::getLoopbackAddressString(ipVersion()), "ROUND_ROBIN");
    cluster2_ = cluster_creator_(
        Network::Test::getLoopbackAddressString(ipVersion()), "ROUND_ROBIN");
  Http::CodecType upstream_codec_type_{Http::CodecType::HTTP2};
  std::function<envoy::config::cluster::v3::Cluster(const std::string&, int, const std::string&,
                                                    const std::string&)>
      cluster_creator_;
// This test verifies that Envoy can delete a cluster with a lot of idle connections.
// The original problem was recursive closure of idle connections that can run out
// of stack when there are a lot of idle connections.
TEST_P(CdsIntegrationTest, CdsClusterDownWithLotsOfIdleConnections) {
  constexpr int num_requests = 2000;
  // Make upstream H/1 so it creates connection for each request
  upstream_codec_type_ = Http::CodecType::HTTP1;
  // Relax default circuit breaker limits and timeouts so Envoy can accumulate a lot of idle
  // connections
  cluster_creator_ = &ConfigHelper::buildH1ClusterWithHighCircuitBreakersLimits;
  config_helper_.addConfigModifier(
      [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
              hcm) -> void {
        hcm.mutable_route_config()
            ->mutable_virtual_hosts(0)
            ->mutable_routes(0)
            ->mutable_route()
            ->mutable_timeout()
            ->set_seconds(600);
        hcm.mutable_route_config()
            ->mutable_virtual_hosts(0)
            ->mutable_routes(0)
            ->mutable_route()
            ->mutable_idle_timeout()
            ->set_seconds(600);
      });
  initialize();
  std::vector<IntegrationStreamDecoderPtr> responses;
  std::vector<FakeHttpConnectionPtr> upstream_connections;
  std::vector<FakeStreamPtr> upstream_requests;
  codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http"))));
  // The first loop establishes a lot of open connections with active requests to upstream
  for (int i = 0; i < num_requests; ++i) {
    Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"},
                                                   {":path", "/cluster1"},
                                                   {":scheme", "http"},
                                                   {":authority", "host"},
                                                   {"x-lyft-user-id", absl::StrCat(i)}};

    auto response = codec_client_->makeHeaderOnlyRequest(request_headers);
    responses.push_back(std::move(response));

    FakeHttpConnectionPtr fake_upstream_connection;
    waitForNextUpstreamConnection({UpstreamIndex1}, TestUtility::DefaultTimeout,
                                  fake_upstream_connection);
    // Wait for the next stream on the upstream connection.
    FakeStreamPtr upstream_request;
    AssertionResult result =
        fake_upstream_connection->waitForNewStream(*dispatcher_, upstream_request);
    RELEASE_ASSERT(result, result.message());
    // Wait for the stream to be completely received.
    result = upstream_request->waitForEndStream(*dispatcher_);
    RELEASE_ASSERT(result, result.message());
    upstream_connections.push_back(std::move(fake_upstream_connection));
    upstream_requests.push_back(std::move(upstream_request));
  }

  // This loop completes all requests making the all upstream connections idle
  for (int i = 0; i < num_requests; ++i) {
    // Send response headers, and end_stream if there is no response body.
    upstream_requests[i]->encodeHeaders(default_response_headers_, true);
    // Wait for the response to be read by the codec client.
    RELEASE_ASSERT(responses[i]->waitForEndStream(), "unexpected timeout");
    ASSERT_TRUE(responses[i]->complete());
    EXPECT_EQ("200", responses[i]->headers().getStatusValue());
  }

  test_server_->waitForCounterGe("cluster_manager.cluster_added", 1);

  // Tell Envoy that cluster_1 is gone. Envoy will try to close all idle connections
  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "55", {}, {}, {}));
  sendDiscoveryResponse<envoy::config::cluster::v3::Cluster>(Config::TypeUrl::get().Cluster, {}, {},
                                                             {ClusterName1}, "42");
  // We can continue the test once we're sure that Envoy's ClusterManager has made use of
  // the DiscoveryResponse that says cluster_1 is gone.
  test_server_->waitForCounterGe("cluster_manager.cluster_removed", 1);

  // If we made it this far then everything is ok.
  for (int i = 0; i < num_requests; ++i) {
    AssertionResult result = upstream_connections[i]->close();
    RELEASE_ASSERT(result, result.message());
    result = upstream_connections[i]->waitForDisconnect();
    RELEASE_ASSERT(result, result.message());
  }
  upstream_connections.clear();
  cleanupUpstreamAndDownstream();
  ASSERT_TRUE(codec_client_->waitForDisconnect());
}

// This test verifies that Envoy can delete a cluster with a lot of connections in the connecting
// state and associated pending requests. The recursion guard in the
// ConnPoolImplBase::closeIdleConnectionsForDrainingPool() would fire if it was called recursively.
//
// Test is currently disabled as there is presently no reliable way of making upstream connections
// hang in connecting state.
TEST_P(CdsIntegrationTest, DISABLED_CdsClusterDownWithLotsOfConnectingConnections) {
  // Use low number of pending connections to prevent bumping into the default
  // limit of 128, since the upstream will be prevented below from
  // accepting connections.
  constexpr int num_requests = 64;
  // Make upstream H/1 so it creates connection for each request
  upstream_codec_type_ = Http::CodecType::HTTP1;
  cluster_creator_ = &ConfigHelper::buildH1ClusterWithHighCircuitBreakersLimits;
  config_helper_.addConfigModifier(
      [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
              hcm) -> void {
        hcm.mutable_route_config()
            ->mutable_virtual_hosts(0)
            ->mutable_routes(0)
            ->mutable_route()
            ->mutable_timeout()
            ->set_seconds(600);
        hcm.mutable_route_config()
            ->mutable_virtual_hosts(0)
            ->mutable_routes(0)
            ->mutable_route()
            ->mutable_idle_timeout()
            ->set_seconds(600);
      });
  initialize();
  test_server_->waitForCounterGe("cluster_manager.cluster_added", 1);
  std::vector<IntegrationStreamDecoderPtr> responses;
  codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http"))));
  // Stop upstream at UpstreamIndex1 dispatcher, to prevent it from accepting TCP connections.
  // This will cause Envoy's connections to that upstream hang in the connecting state.
  fake_upstreams_[UpstreamIndex1]->dispatcher()->exit();
  for (int i = 0; i < num_requests; ++i) {
    Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"},
                                                   {":path", "/cluster1"},
                                                   {":scheme", "http"},
                                                   {":authority", "host"},
                                                   {"x-lyft-user-id", absl::StrCat(i)}};

    auto response = codec_client_->makeHeaderOnlyRequest(request_headers);
    responses.push_back(std::move(response));
  }

  // Wait for Envoy to try to establish all expected connections
  test_server_->waitForCounterEq("cluster.cluster_1.upstream_cx_total", num_requests);

  // Tell Envoy that cluster_1 is gone. Envoy will try to close all pending connections
  EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "55", {}, {}, {}));
  sendDiscoveryResponse<envoy::config::cluster::v3::Cluster>(Config::TypeUrl::get().Cluster, {}, {},
                                                             {ClusterName1}, "42");
  // We can continue the test once we're sure that Envoy's ClusterManager has made use of
  // the DiscoveryResponse that says cluster_1 is gone.
  test_server_->waitForCounterGe("cluster_manager.cluster_removed", 1);

  cleanupUpstreamAndDownstream();
  ASSERT_TRUE(codec_client_->waitForDisconnect());
  // If we got here it means that the recursion guard in the
  // ConnPoolImplBase::closeIdleConnectionsForDrainingPool() did not fire, which is what this test
  // validates.
}