ratelimit: fix a bug where response phase limit may result in crash

[CVE-2026-26330](GHSA-c23c-rp3m-vpg3)

Signed-off-by: wbpcode <wbphub@gmail.com>
Signed-off-by: Ryan Northey <ryan@synca.io>

Author: wbpcode

changelogs/current.yaml

@@ -181,6 +181,12 @@ bug_fixes:
     intentionally not parseable (for example ``goo.gle/debugonly`` prefixes). This change can
     be reverted by setting runtime guard
     ``envoy.reloadable_features.cel_message_serialize_text_format`` to ``false``.
+- area: ratelimit
+  change: |
+    Fixed a bug in the gRPC rate limit client where the client could get into a bad state if the
+    callbacks were not properly released after a request completion, leading to potential use-after-free
+    issues. The fix ensures that callbacks and request references are cleared after completion, and adds
+    assertions to enforce correct usage patterns.
 - area: ext_authz
   change: |
     Fixed a bug where headers from a denied authorization response (non-200) were not properly propagated

source/extensions/filters/common/ratelimit/ratelimit_impl.cc

@@ -37,7 +37,6 @@ void GrpcClientImpl::cancel() {
 }
 
 void GrpcClientImpl::detach() {
-  ASSERT(callbacks_ != nullptr);
   if (request_) {
     request_->detach();
     request_ = nullptr;
@@ -77,8 +76,11 @@ void GrpcClientImpl::limit(RequestCallbacks& callbacks, const std::string& domai
                            const std::vector<Envoy::RateLimit::Descriptor>& descriptors,
                            Tracing::Span& parent_span, const StreamInfo::StreamInfo& stream_info,
                            uint32_t hits_addend) {
+  // The client should only be used for one outstanding request at a time,
+  // so we assert that there is no existing request or callback.
   ASSERT(callbacks_ == nullptr);
   callbacks_ = &callbacks;
+  request_ = nullptr;
 
   envoy::service::ratelimit::v3::RateLimitRequest request;
   createRequest(request, domain, descriptors, hits_addend);
@@ -130,6 +132,7 @@ void GrpcClientImpl::onSuccess(
   // callback, so we release the callback here to make the destructor happy.
   auto call_backs = callbacks_;
   callbacks_ = nullptr;
+  request_ = nullptr;
   call_backs->complete(status, std::move(descriptor_statuses), std::move(response_headers_to_add),
                        std::move(request_headers_to_add), response->raw_body(),
                        std::move(dynamic_metadata));
@@ -144,6 +147,7 @@ void GrpcClientImpl::onFailure(Grpc::Status::GrpcStatus status, const std::strin
   // callback, so we release the callback here to make the destructor happy.
   auto call_backs = callbacks_;
   callbacks_ = nullptr;
+  request_ = nullptr;
   call_backs->complete(LimitStatus::Error, nullptr, nullptr, nullptr, EMPTY_STRING, nullptr);
 }
 

test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc

@@ -259,6 +259,31 @@ TEST_F(RateLimitGrpcClientTest, SendRequestAndDetach) {
     EXPECT_CALL(request_callbacks_, complete_(LimitStatus::OK, _, _, _, _, _));
     client_.onSuccess(std::move(response), span_);
   }
+
+  // Send request and then fail before detach, and than call the detach should be no-op.
+  {
+    envoy::service::ratelimit::v3::RateLimitRequest request;
+    GrpcClientImpl::createRequest(request, "foo", {{{{"foo", "bar"}}}}, 0);
+    EXPECT_CALL(*async_client_, sendRaw(_, _, Grpc::ProtoBufferEq(request), Ref(client_), _, _))
+        .WillOnce(
+            Invoke([this](absl::string_view service_full_name, absl::string_view method_name,
+                          Buffer::InstancePtr&&, Grpc::RawAsyncRequestCallbacks&, Tracing::Span&,
+                          const Http::AsyncClient::RequestOptions&) -> Grpc::AsyncRequest* {
+              std::string service_name = "envoy.service.ratelimit.v3.RateLimitService";
+              EXPECT_EQ(service_name, service_full_name);
+              EXPECT_EQ("ShouldRateLimit", method_name);
+              return &async_request_;
+            }));
+
+    client_.limit(request_callbacks_, "foo", {{{{"foo", "bar"}}}}, Tracing::NullSpan::instance(),
+                  stream_info_, 0);
+
+    EXPECT_CALL(request_callbacks_, complete_(LimitStatus::Error, _, _, _, _, _));
+    client_.onFailure(Grpc::Status::Unknown, "", span_);
+
+    // Detach should be no-op since the request has already failed.
+    client_.detach();
+  }
 }
 
 } // namespace