Skip to content
Permalink
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
yanavlasov authored and RyanTheOptimist committed Feb 22, 2022
1 parent d13f9c2 commit 4b6dd3b
Show file tree
Hide file tree
Showing 8 changed files with 230 additions and 13 deletions.
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
Expand Up @@ -28,6 +28,7 @@ Bug Fixes
* data plane: fixing error handling where writing to a socket failed while under the stack of processing. This should genreally affect HTTP/3. This behavioral change can be reverted by setting ``envoy.reloadable_features.allow_upstream_inline_write`` to false.
* eds: fix the eds cluster update by allowing update on the locality of the cluster endpoints. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.support_locality_update_on_eds_cluster_endpoints`` to false.
* tls: fix a bug while matching a certificate SAN with an exact value in ``match_typed_subject_alt_names`` of a listener where wildcard ``*`` character is not the only character of the dns label. Example, ``baz*.example.net`` and ``*baz.example.net`` and ``b*z.example.net`` will match ``baz1.example.net`` and ``foobaz.example.net`` and ``buzz.example.net``, respectively.
* upstream: fix stack overflow when a cluster with large number of idle connections is removed.
* xray: fix the AWS X-Ray tracer extension to not sample the trace if ``sampled=`` keyword is not present in the header ``x-amzn-trace-id``.

Removed Config or Runtime
Expand Down
1 change: 1 addition & 0 deletions source/common/conn_pool/BUILD
Expand Up @@ -14,6 +14,7 @@ envoy_cc_library(
hdrs = ["conn_pool_base.h"],
deps = [
"//envoy/stats:timespan_interface",
"//source/common/common:debug_recursion_checker_lib",
"//source/common/common:linked_object",
"//source/common/stats:timespan_lib",
"//source/common/upstream:upstream_lib",
Expand Down
27 changes: 21 additions & 6 deletions source/common/conn_pool/conn_pool_base.cc
@@ -1,6 +1,7 @@
#include "source/common/conn_pool/conn_pool_base.h"

#include "source/common/common/assert.h"
#include "source/common/common/debug_recursion_checker.h"
#include "source/common/network/transport_socket_options_impl.h"
#include "source/common/runtime/runtime_features.h"
#include "source/common/stats/timespan_impl.h"
Expand Down Expand Up @@ -349,6 +350,8 @@ void ConnPoolImplBase::transitionActiveClientState(ActiveClient& client,
void ConnPoolImplBase::addIdleCallbackImpl(Instance::IdleCb cb) { idle_callbacks_.push_back(cb); }

void ConnPoolImplBase::closeIdleConnectionsForDrainingPool() {
Common::AutoDebugRecursionChecker assert_not_in(recursion_checker_);

// Create a separate list of elements to close to avoid mutate-while-iterating problems.
std::list<ActiveClient*> to_close;

Expand Down Expand Up @@ -403,11 +406,7 @@ bool ConnPoolImplBase::isIdleImpl() const {
connecting_clients_.empty();
}

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

void ConnPoolImplBase::checkForIdleAndNotify() {
if (isIdleImpl()) {
ENVOY_LOG(debug, "invoking idle callbacks - is_draining_for_deletion_={}",
is_draining_for_deletion_);
Expand All @@ -417,6 +416,14 @@ void ConnPoolImplBase::checkForIdleAndCloseIdleConnsIfDraining() {
}
}

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

checkForIdleAndNotify();
}

void ConnPoolImplBase::onConnectionEvent(ActiveClient& client, absl::string_view failure_reason,
Network::ConnectionEvent event) {
if (client.state() == ActiveClient::State::CONNECTING) {
Expand Down Expand Up @@ -487,7 +494,15 @@ void ConnPoolImplBase::onConnectionEvent(ActiveClient& client, absl::string_view

dispatcher_.deferredDelete(client.removeFromList(owningList(client.state())));

checkForIdleAndCloseIdleConnsIfDraining();
// 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();

client.setState(ActiveClient::State::CLOSED);

Expand Down
5 changes: 5 additions & 0 deletions source/common/conn_pool/conn_pool_base.h
Expand Up @@ -6,6 +6,7 @@
#include "envoy/stats/timespan.h"
#include "envoy/upstream/cluster_manager.h"

#include "source/common/common/debug_recursion_checker.h"
#include "source/common/common/dump_state_utils.h"
#include "source/common/common/linked_object.h"

Expand Down Expand Up @@ -222,6 +223,9 @@ class ConnPoolImplBase : protected Logger::Loggable<Logger::Id::pool> {
void onConnectionEvent(ActiveClient& client, absl::string_view failure_reason,
Network::ConnectionEvent event);

// Check if the pool has gone idle and invoke idle notification callbacks.
void checkForIdleAndNotify();

// See if the pool has gone idle. If we're draining, this will also close idle connections.
void checkForIdleAndCloseIdleConnsIfDraining();

Expand Down Expand Up @@ -371,6 +375,7 @@ class ConnPoolImplBase : protected Logger::Loggable<Logger::Id::pool> {
bool deferred_deleting_{false};

Event::SchedulableCallbackPtr upstream_ready_cb_;
Common::DebugRecursionChecker recursion_checker_;
};

} // namespace ConnectionPool
Expand Down
28 changes: 28 additions & 0 deletions test/config/utility.cc
Expand Up @@ -442,6 +442,34 @@ envoy::config::cluster::v3::Cluster ConfigHelper::buildStaticCluster(const std::
name, name, address, port, lb_policy));
}

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));
}

envoy::config::cluster::v3::Cluster ConfigHelper::buildCluster(const std::string& name,
const std::string& lb_policy) {
API_NO_BOOST(envoy::config::cluster::v3::Cluster) cluster;
Expand Down
5 changes: 5 additions & 0 deletions test/config/utility.h
Expand Up @@ -153,6 +153,11 @@ class ConfigHelper {
buildStaticCluster(const std::string& name, int port, const std::string& address,
const std::string& lb_policy = "ROUND_ROBIN");

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

// ADS configurations
static envoy::config::cluster::v3::Cluster
buildCluster(const std::string& name, const std::string& lb_policy = "ROUND_ROBIN");
Expand Down
2 changes: 2 additions & 0 deletions test/integration/BUILD
Expand Up @@ -110,10 +110,12 @@ envoy_proto_library(

envoy_cc_test(
name = "cds_integration_test",
size = "large",
srcs = ["cds_integration_test.cc"],
data = [
"//test/config/integration/certs",
],
shard_count = 4,
deps = [
":http_integration_lib",
"//source/common/config:protobuf_link_hacks",
Expand Down
174 changes: 167 additions & 7 deletions test/integration/cds_integration_test.cc
Expand Up @@ -37,7 +37,8 @@ class CdsIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, public Ht
sotwOrDelta() == Grpc::SotwOrDelta::Sotw ||
sotwOrDelta() == Grpc::SotwOrDelta::UnifiedSotw
? "GRPC"
: "DELTA_GRPC")) {
: "DELTA_GRPC")),
cluster_creator_(&ConfigHelper::buildStaticCluster) {
if (sotwOrDelta() == Grpc::SotwOrDelta::UnifiedSotw ||
sotwOrDelta() == Grpc::SotwOrDelta::UnifiedDelta) {
config_helper_.addRuntimeOverride("envoy.reloadable_features.unified_mux", "true");
Expand Down Expand Up @@ -79,14 +80,14 @@ class CdsIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, public Ht
// Create the regular (i.e. not an xDS server) upstreams. We create them manually here after
// initialize() because finalize() expects all fake_upstreams_ to correspond to a static
// cluster in the bootstrap config - which we don't want since we're testing dynamic CDS!
addFakeUpstream(Http::CodecType::HTTP2);
addFakeUpstream(Http::CodecType::HTTP2);
cluster1_ = ConfigHelper::buildStaticCluster(
addFakeUpstream(upstream_codec_type_);
addFakeUpstream(upstream_codec_type_);
cluster1_ = cluster_creator_(
ClusterName1, fake_upstreams_[UpstreamIndex1]->localAddress()->ip()->port(),
Network::Test::getLoopbackAddressString(ipVersion()));
cluster2_ = ConfigHelper::buildStaticCluster(
Network::Test::getLoopbackAddressString(ipVersion()), "ROUND_ROBIN");
cluster2_ = cluster_creator_(
ClusterName2, fake_upstreams_[UpstreamIndex2]->localAddress()->ip()->port(),
Network::Test::getLoopbackAddressString(ipVersion()));
Network::Test::getLoopbackAddressString(ipVersion()), "ROUND_ROBIN");

// Let Envoy establish its connection to the CDS server.
acceptXdsConnection();
Expand Down Expand Up @@ -133,6 +134,10 @@ class CdsIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, public Ht
envoy::config::cluster::v3::Cluster cluster2_;
// True if we decided not to run the test after all.
bool test_skipped_{true};
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_;
};

INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, CdsIntegrationTest,
Expand Down Expand Up @@ -343,5 +348,160 @@ TEST_P(CdsIntegrationTest, VersionsRememberedAfterReconnect) {
ASSERT_TRUE(codec_client_->waitForDisconnect());
}

// 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.
}

} // namespace
} // namespace Envoy

0 comments on commit 4b6dd3b

Please sign in to comment.