From: Eduard Bagdasaryan Date: Fri, 9 Aug 2024 12:29:21 +0000 (+0000) Subject: Protect ACLFilledChecklist heap allocations from leaking (#1870) X-Git-Tag: SQUID_7_0_1~79 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=c56edb4a82a746c90e157382b2d44e2a0007f905;p=thirdparty%2Fsquid.git Protect ACLFilledChecklist heap allocations from leaking (#1870) Non-blocking ACL checks follow this code pattern: const auto ch = new ACLFilledChecklist(...); fillWithInformation(ch); // may throw ch->nonBlockingCheck(&throwingCallback); // may delete ch or throw! // ch may be a dangling raw pointer here The checklist object is leaked if an exception is thrown by fillWithInformation() (and the code it calls) or by nonBlockingCheck() (and the code it calls, including, in some cases, throwingCallback()). Use std::unique_ptr to avoid such leaks. --- diff --git a/src/acl/Checklist.h b/src/acl/Checklist.h index 664751a1fa..9e2d70fe66 100644 --- a/src/acl/Checklist.h +++ b/src/acl/Checklist.h @@ -39,28 +39,6 @@ public: ACLChecklist(); virtual ~ACLChecklist(); - /** - * Start a non-blocking (async) check for a list of allow/deny rules. - * Each rule comes with a list of ACLs. - * - * The callback specified will be called with the result of the check. - * - * The first rule where all ACLs match wins. If there is such a rule, - * the result becomes that rule keyword (ACCESS_ALLOWED or ACCESS_DENIED). - * - * If there are rules but all ACL lists mismatch, an implicit rule is used. - * Its result is the negation of the keyword of the last seen rule. - * - * Some ACLs may stop the check prematurely by setting an exceptional - * check result (e.g., ACCESS_AUTH_REQUIRED) instead of declaring a - * match or mismatch. - * - * If there are no rules to check at all, the result becomes ACCESS_DUNNO. - * Calling this method with no rules to check wastes a lot of CPU cycles - * and will result in a DBG_CRITICAL debugging message. - */ - void nonBlockingCheck(ACLCB * callback, void *callback_data); - /** * Perform a blocking (immediate) check for a list of allow/deny rules. * Each rule comes with a list of ACLs. @@ -149,6 +127,29 @@ public: /// remember the name of the last ACL being evaluated void setLastCheckedName(const SBuf &name) { lastCheckedName_ = name; } +protected: + /** + * Start a non-blocking (async) check for a list of allow/deny rules. + * Each rule comes with a list of ACLs. + * + * The callback specified will be called with the result of the check. + * + * The first rule where all ACLs match wins. If there is such a rule, + * the result becomes that rule keyword (ACCESS_ALLOWED or ACCESS_DENIED). + * + * If there are rules but all ACL lists mismatch, an implicit rule is used. + * Its result is the negation of the keyword of the last seen rule. + * + * Some ACLs may stop the check prematurely by setting an exceptional + * check result (e.g., ACCESS_AUTH_REQUIRED) instead of declaring a + * match or mismatch. + * + * If there are no rules to check at all, the result becomes ACCESS_DUNNO. + * Calling this method with no rules to check wastes a lot of CPU cycles + * and will result in a DBG_CRITICAL debugging message. + */ + void nonBlockingCheck(ACLCB * callback, void *callback_data); + private: /// Calls non-blocking check callback with the answer and destroys self. /// If abortReason is provided, sets the final answer to ACCESS_DUNNO. diff --git a/src/acl/FilledChecklist.cc b/src/acl/FilledChecklist.cc index e52c941cd6..4d8f932d77 100644 --- a/src/acl/FilledChecklist.cc +++ b/src/acl/FilledChecklist.cc @@ -186,16 +186,17 @@ ACLFilledChecklist::markSourceDomainChecked() /* * There are two common ACLFilledChecklist lifecycles paths: * - * A) Using aclCheckFast(): The caller creates an ACLFilledChecklist object - * on stack and calls aclCheckFast(). + * "Fast" (always synchronous or "blocking"): The user constructs an + * ACLFilledChecklist object on stack, configures it as needed, and calls one + * or both of its fastCheck() methods. * - * B) Using aclNBCheck() and callbacks: The caller allocates an - * ACLFilledChecklist object (via operator new) and passes it to - * aclNBCheck(). Control eventually passes to ACLChecklist::checkCallback(), - * which will invoke the callback function as requested by the - * original caller of aclNBCheck(). This callback function must - * *not* delete the list. After the callback function returns, - * checkCallback() will delete the list (i.e., self). + * "Slow" (usually asynchronous or "non-blocking"): The user allocates an + * ACLFilledChecklist object on heap (via Make()), configures it as needed, + * and passes it to NonBlockingCheck() while specifying the callback function + * to call with check results. NonBlockingCheck() calls the callback function + * (if the corresponding cbdata is still valid), either immediately/directly + * (XXX) or eventually/asynchronously. After this callback obligations are + * fulfilled, checkCallback() deletes the checklist object (i.e. "this"). */ ACLFilledChecklist::ACLFilledChecklist(const acl_access *A, HttpRequest *http_request): dst_rdns(nullptr), diff --git a/src/acl/FilledChecklist.h b/src/acl/FilledChecklist.h index 358792a2ec..cf5fa954b2 100644 --- a/src/acl/FilledChecklist.h +++ b/src/acl/FilledChecklist.h @@ -32,13 +32,27 @@ class ConnStateData; */ class ACLFilledChecklist: public ACLChecklist { - CBDATA_CLASS(ACLFilledChecklist); + CBDATA_CLASS_WITH_MAKE(ACLFilledChecklist); public: + /// Unlike regular Foo::Pointer types, this smart pointer is meant for use + /// during checklist configuration only, when it provides exception safety. + /// Any other/long-term checklist storage requires CbcPointer or equivalent. + using MakingPointer = std::unique_ptr; + ACLFilledChecklist(); ACLFilledChecklist(const acl_access *, HttpRequest *); ~ACLFilledChecklist() override; + /// Creates an ACLFilledChecklist object with given constructor arguments. + /// Callers are expected to eventually proceed with NonBlockingCheck(). + static MakingPointer Make(const acl_access *a, HttpRequest *r) { return MakingPointer(new ACLFilledChecklist(a, r)); } + + /// \copydoc ACLChecklist::nonBlockingCheck() + /// This public nonBlockingCheck() wrapper should be paired with Make(). The + /// pair prevents exception-caused Checklist memory leaks in caller code. + static void NonBlockingCheck(MakingPointer &&p, ACLCB *cb, void *data) { p->nonBlockingCheck(cb, data); (void)p.release(); } + /// configure client request-related fields for the first time void setRequest(HttpRequest *); diff --git a/src/adaptation/AccessCheck.cc b/src/adaptation/AccessCheck.cc index 475ecf2588..301a9971dc 100644 --- a/src/adaptation/AccessCheck.cc +++ b/src/adaptation/AccessCheck.cc @@ -128,11 +128,11 @@ Adaptation::AccessCheck::checkCandidates() while (!candidates.empty()) { if (AccessRule *r = FindRule(topCandidate())) { /* BUG 2526: what to do when r->acl is empty?? */ - const auto acl_checklist = new ACLFilledChecklist(r->acl, filter.request); + auto acl_checklist = ACLFilledChecklist::Make(r->acl, filter.request); acl_checklist->updateAle(filter.al); acl_checklist->updateReply(filter.reply); acl_checklist->syncAle(filter.request, nullptr); - acl_checklist->nonBlockingCheck(AccessCheckCallbackWrapper, this); + ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), AccessCheckCallbackWrapper, this); return; } diff --git a/src/cbdata.h b/src/cbdata.h index 7b67098826..edeac4fe6f 100644 --- a/src/cbdata.h +++ b/src/cbdata.h @@ -255,12 +255,12 @@ cbdata_type cbdataInternalAddType(cbdata_type type, const char *label, int size) /// declaration-generator used internally by CBDATA_CLASS() and CBDATA_CHILD() #define CBDATA_DECL_(type, methodSpecifiers) \ - public: \ void *operator new(size_t size) { \ assert(size == sizeof(type)); \ if (!CBDATA_##type) CBDATA_##type = cbdataInternalAddType(CBDATA_##type, #type, sizeof(type)); \ return (type *)cbdataInternalAlloc(CBDATA_##type); \ } \ + public: \ void operator delete (void *address) { \ if (address) cbdataInternalFree(address); \ } \ @@ -286,12 +286,17 @@ private: /// cbdata-enables a stand-alone class that is not a CbdataParent child /// sets the class declaration section to "private" /// use this at the start of your class declaration for consistency sake -#define CBDATA_CLASS(type) CBDATA_DECL_(type, noexcept) +#define CBDATA_CLASS(type) public: CBDATA_DECL_(type, noexcept) + +/// A CBDATA_CLASS() variant for classes that want to prevent accidental +/// operator new() calls by making that operator private and forcing external +/// users to call a Make() function instead. +#define CBDATA_CLASS_WITH_MAKE(type) private: CBDATA_DECL_(type, noexcept) /// cbdata-enables a final CbdataParent-derived class in a hierarchy /// sets the class declaration section to "private" /// use this at the start of your class declaration for consistency sake -#define CBDATA_CHILD(type) CBDATA_DECL_(type, final) \ +#define CBDATA_CHILD(type) public: CBDATA_DECL_(type, final) \ void finalizedInCbdataChild() final {} /// cbdata-enables a non-final CbdataParent-derived class T in a hierarchy. diff --git a/src/client_side.cc b/src/client_side.cc index 2a977e34d7..38cd088e17 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2484,7 +2484,7 @@ ConnStateData::postHttpsAccept() CodeContext::Reset(connectAle); // TODO: Use these request/ALE when waiting for new bumped transactions. - const auto acl_checklist = new ACLFilledChecklist(Config.accessList.ssl_bump, request); + auto acl_checklist = ACLFilledChecklist::Make(Config.accessList.ssl_bump, request); fillChecklist(*acl_checklist); // Build a local AccessLogEntry to allow requiresAle() acls work acl_checklist->al = connectAle; @@ -2501,7 +2501,7 @@ ConnStateData::postHttpsAccept() ClientHttpRequest *http = context ? context->http : nullptr; const char *log_uri = http ? http->log_uri : nullptr; acl_checklist->syncAle(request, log_uri); - acl_checklist->nonBlockingCheck(httpsSslBumpAccessCheckDone, this); + ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), httpsSslBumpAccessCheckDone, this); #else fatal("FATAL: SSL-Bump requires --with-openssl"); #endif @@ -2967,12 +2967,12 @@ ConnStateData::startPeekAndSplice() sslServerBump->step = XactionStep::tlsBump2; // Run a accessList check to check if want to splice or continue bumping - const auto acl_checklist = new ACLFilledChecklist(Config.accessList.ssl_bump, sslServerBump->request.getRaw()); + auto acl_checklist = ACLFilledChecklist::Make(Config.accessList.ssl_bump, sslServerBump->request.getRaw()); acl_checklist->banAction(Acl::Answer(ACCESS_ALLOWED, Ssl::bumpNone)); acl_checklist->banAction(Acl::Answer(ACCESS_ALLOWED, Ssl::bumpClientFirst)); acl_checklist->banAction(Acl::Answer(ACCESS_ALLOWED, Ssl::bumpServerFirst)); fillChecklist(*acl_checklist); - acl_checklist->nonBlockingCheck(httpsSslBumpStep2AccessCheckDone, this); + ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), httpsSslBumpStep2AccessCheckDone, this); return; } @@ -3111,7 +3111,7 @@ ConnStateData::initiateTunneledRequest(HttpRequest::Pointer const &cause, const // TLS handshakes on non-bumping https_port. TODO: Discover these // problems earlier so that they can be classified/detailed better. debugs(33, 2, "Not able to compute URL, abort request tunneling for " << reason); - // TODO: throw when nonBlockingCheck() callbacks gain job protections + // TODO: throw when NonBlockingCheck() callbacks gain job protections static const auto d = MakeNamedErrorDetail("TUNNEL_TARGET"); updateError(ERR_INVALID_REQ, d); return false; @@ -3448,10 +3448,10 @@ varyEvaluateMatch(StoreEntry * entry, HttpRequest * request) } } -ACLFilledChecklist * +ACLFilledChecklist::MakingPointer clientAclChecklistCreate(const acl_access * acl, ClientHttpRequest * http) { - const auto checklist = new ACLFilledChecklist(acl, nullptr); + auto checklist = ACLFilledChecklist::Make(acl, nullptr); clientAclChecklistFill(*checklist, http); return checklist; } diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 4754573dd2..3deab499dc 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -1835,10 +1835,10 @@ clientReplyContext::processReplyAccess () } /** Process http_reply_access lists */ - ACLFilledChecklist *replyChecklist = + auto replyChecklist = clientAclChecklistCreate(Config.accessList.reply, http); replyChecklist->updateReply(reply); - replyChecklist->nonBlockingCheck(ProcessReplyAccessResult, this); + ACLFilledChecklist::NonBlockingCheck(std::move(replyChecklist), ProcessReplyAccessResult, this); } void diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 1d432a8f52..8062bd1c2d 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -438,13 +438,13 @@ clientFollowXForwardedForCheck(Acl::Answer answer, void *data) if ((addr = asciiaddr)) { request->indirect_client_addr = addr; request->x_forwarded_for_iterator.cut(l); - const auto ch = clientAclChecklistCreate(Config.accessList.followXFF, http); + auto ch = clientAclChecklistCreate(Config.accessList.followXFF, http); if (!Config.onoff.acl_uses_indirect_client) { /* override the default src_addr tested if we have to go deeper than one level into XFF */ ch->src_addr = request->indirect_client_addr; } if (++calloutContext->currentXffHopNumber < SQUID_X_FORWARDED_FOR_HOP_MAX) { - ch->nonBlockingCheck(clientFollowXForwardedForCheck, data); + ACLFilledChecklist::NonBlockingCheck(std::move(ch), clientFollowXForwardedForCheck, data); return; } const auto headerName = Http::HeaderLookupTable.lookup(Http::HdrType::X_FORWARDED_FOR).name; @@ -666,15 +666,15 @@ ClientRequestContext::clientAccessCheck() http->request->x_forwarded_for_iterator = http->request->header.getList(Http::HdrType::X_FORWARDED_FOR); /* begin by checking to see if we trust direct client enough to walk XFF */ - const auto acl_checklist = clientAclChecklistCreate(Config.accessList.followXFF, http); - acl_checklist->nonBlockingCheck(clientFollowXForwardedForCheck, this); + auto acl_checklist = clientAclChecklistCreate(Config.accessList.followXFF, http); + ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), clientFollowXForwardedForCheck, this); return; } #endif if (Config.accessList.http) { - const auto acl_checklist = clientAclChecklistCreate(Config.accessList.http, http); - acl_checklist->nonBlockingCheck(clientAccessCheckDoneWrapper, this); + auto acl_checklist = clientAclChecklistCreate(Config.accessList.http, http); + ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), clientAccessCheckDoneWrapper, this); } else { debugs(0, DBG_CRITICAL, "No http_access configuration found. This will block ALL traffic"); clientAccessCheckDone(ACCESS_DENIED); @@ -690,8 +690,8 @@ void ClientRequestContext::clientAccessCheck2() { if (Config.accessList.adapted_http) { - const auto acl_checklist = clientAclChecklistCreate(Config.accessList.adapted_http, http); - acl_checklist->nonBlockingCheck(clientAccessCheckDoneWrapper, this); + auto acl_checklist = clientAclChecklistCreate(Config.accessList.adapted_http, http); + ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), clientAccessCheckDoneWrapper, this); } else { debugs(85, 2, "No adapted_http_access configuration. default: ALLOW"); clientAccessCheckDone(ACCESS_ALLOWED); @@ -835,8 +835,8 @@ ClientRequestContext::clientRedirectStart() debugs(33, 5, "'" << http->uri << "'"); http->al->syncNotes(http->request); if (Config.accessList.redirector) { - const auto acl_checklist = clientAclChecklistCreate(Config.accessList.redirector, http); - acl_checklist->nonBlockingCheck(clientRedirectAccessCheckDone, this); + auto acl_checklist = clientAclChecklistCreate(Config.accessList.redirector, http); + ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), clientRedirectAccessCheckDone, this); } else redirectStart(http, clientRedirectDoneWrapper, this); } @@ -871,8 +871,8 @@ ClientRequestContext::clientStoreIdStart() debugs(33, 5,"'" << http->uri << "'"); if (Config.accessList.store_id) { - const auto acl_checklist = clientAclChecklistCreate(Config.accessList.store_id, http); - acl_checklist->nonBlockingCheck(clientStoreIdAccessCheckDone, this); + auto acl_checklist = clientAclChecklistCreate(Config.accessList.store_id, http); + ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), clientStoreIdAccessCheckDone, this); } else storeIdStart(http, clientStoreIdDoneWrapper, this); } @@ -1310,8 +1310,8 @@ void ClientRequestContext::checkNoCache() { if (Config.accessList.noCache) { - const auto acl_checklist = clientAclChecklistCreate(Config.accessList.noCache, http); - acl_checklist->nonBlockingCheck(checkNoCacheDoneWrapper, this); + auto acl_checklist = clientAclChecklistCreate(Config.accessList.noCache, http); + ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), checkNoCacheDoneWrapper, this); } else { /* unless otherwise specified, we try to cache. */ checkNoCacheDone(ACCESS_ALLOWED); @@ -1405,8 +1405,8 @@ ClientRequestContext::sslBumpAccessCheck() debugs(85, 5, "SslBump possible, checking ACL"); - ACLFilledChecklist *aclChecklist = clientAclChecklistCreate(Config.accessList.ssl_bump, http); - aclChecklist->nonBlockingCheck(sslBumpAccessCheckDoneWrapper, this); + auto aclChecklist = clientAclChecklistCreate(Config.accessList.ssl_bump, http); + ACLFilledChecklist::NonBlockingCheck(std::move(aclChecklist), sslBumpAccessCheckDoneWrapper, this); return true; } diff --git a/src/client_side_request.h b/src/client_side_request.h index e35962564c..bff45673b3 100644 --- a/src/client_side_request.h +++ b/src/client_side_request.h @@ -10,6 +10,7 @@ #define SQUID_SRC_CLIENT_SIDE_REQUEST_H #include "AccessLogEntry.h" +#include "acl/FilledChecklist.h" #include "client_side.h" #include "clientStream.h" #include "http/forward.h" @@ -257,7 +258,7 @@ private: /* client http based routines */ char *clientConstructTraceEcho(ClientHttpRequest *); -ACLFilledChecklist *clientAclChecklistCreate(const acl_access *, ClientHttpRequest *); +ACLFilledChecklist::MakingPointer clientAclChecklistCreate(const acl_access *, ClientHttpRequest *); void clientAclChecklistFill(ACLFilledChecklist &, ClientHttpRequest *); void clientAccessCheck(ClientHttpRequest *); diff --git a/src/peer_select.cc b/src/peer_select.cc index ead91c0f99..632c2e8b76 100644 --- a/src/peer_select.cc +++ b/src/peer_select.cc @@ -613,18 +613,18 @@ PeerSelector::selectMore() if (always_direct == ACCESS_DUNNO) { debugs(44, 3, "direct = " << DirectStr[direct] << " (always_direct to be checked)"); /** check always_direct; */ - const auto ch = new ACLFilledChecklist(Config.accessList.AlwaysDirect, request); + auto ch = ACLFilledChecklist::Make(Config.accessList.AlwaysDirect, request); ch->al = al; ch->syncAle(request, nullptr); - ch->nonBlockingCheck(CheckAlwaysDirectDone, this); + ACLFilledChecklist::NonBlockingCheck(std::move(ch), CheckAlwaysDirectDone, this); return; } else if (never_direct == ACCESS_DUNNO) { debugs(44, 3, "direct = " << DirectStr[direct] << " (never_direct to be checked)"); /** check never_direct; */ - const auto ch = new ACLFilledChecklist(Config.accessList.NeverDirect, request); + auto ch = ACLFilledChecklist::Make(Config.accessList.NeverDirect, request); ch->al = al; ch->syncAle(request, nullptr); - ch->nonBlockingCheck(CheckNeverDirectDone, this); + ACLFilledChecklist::NonBlockingCheck(std::move(ch), CheckNeverDirectDone, this); return; } else if (request->flags.noDirect) { /** if we are accelerating, direct is not an option. */ diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index 48fbbf45c1..7fdffebf72 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -169,9 +169,9 @@ Security::PeerConnector::initialize(Security::SessionPointer &serverSession) // TODO: Remove ACLFilledChecklist::sslErrors and other pre-computed // state in favor of the ACLs accessing current/fresh info directly. if (acl_access *acl = ::Config.ssl_client.cert_error) { - const auto check = new ACLFilledChecklist(acl, request.getRaw()); + auto check = ACLFilledChecklist::Make(acl, request.getRaw()); fillChecklist(*check); - SSL_set_ex_data(serverSession.get(), ssl_ex_index_cert_error_check, check); + SSL_set_ex_data(serverSession.get(), ssl_ex_index_cert_error_check, check.release()); } } diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc index 2f08736812..87aaeb1cbb 100644 --- a/src/ssl/PeekingPeerConnector.cc +++ b/src/ssl/PeekingPeerConnector.cc @@ -69,7 +69,7 @@ Ssl::PeekingPeerConnector::checkForPeekAndSplice() { handleServerCertificate(); - const auto acl_checklist = new ACLFilledChecklist(::Config.accessList.ssl_bump, request.getRaw()); + auto acl_checklist = ACLFilledChecklist::Make(::Config.accessList.ssl_bump, request.getRaw()); acl_checklist->al = al; acl_checklist->banAction(Acl::Answer(ACCESS_ALLOWED, Ssl::bumpNone)); acl_checklist->banAction(Acl::Answer(ACCESS_ALLOWED, Ssl::bumpPeek)); @@ -84,7 +84,7 @@ Ssl::PeekingPeerConnector::checkForPeekAndSplice() if (!srvBio->canBump()) acl_checklist->banAction(Acl::Answer(ACCESS_ALLOWED, Ssl::bumpBump)); acl_checklist->syncAle(request.getRaw(), nullptr); - acl_checklist->nonBlockingCheck(Ssl::PeekingPeerConnector::cbCheckForPeekAndSpliceDone, this); + ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), Ssl::PeekingPeerConnector::cbCheckForPeekAndSpliceDone, this); } void