<div dir="ltr"><div class="gmail_default" style="font-size:small">Hello all,</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">We moved one step closer to the root cause. We bisected the code and found out that the commit that introduced the leak is:</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">$ git show --stat 800967afde7921eb22a2711c5a75b5cd9c9d7178<br>commit 800967afde7921eb22a2711c5a75b5cd9c9d7178 (HEAD)<br>Author: Christos Tsantilas <<a href="mailto:christos@chtsanti.net">christos@chtsanti.net</a>><br>Date: Mon Feb 22 21:15:32 2021 +0000<br><br> Handling missing issuer certificates for TLSv1.3 (#766)<br> <br> Prior to TLS v1.3 Squid could detect and fetch missing intermediate<br> server certificates by parsing TLS ServerHello. TLS v1.3 encrypts the<br> relevant part of the handshake, making such "prefetch" impossible.<br> <br> Instead of looking for certificates in TLS ServerHello, Squid now waits<br> for the OpenSSL built-in certificate validation to fail with an<br> X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY error. Squid-supplied<br> verify_callback function tells OpenSSL to ignore that error. Squid<br> SSL_connect()-calling code detects that the error was ignored and, if<br> possible, fetches the missing certificates and orchestrates certificate<br> chain validation outside the SSL_connect() sequence. If that validation<br> is successful, Squid continues with SSL_connect(). See comments inside<br> Security::PeerConnector::negotiate() for low-level details.<br> <br> In some cases, OpenSSL is able to complete SSL_connect() with an ignored<br> X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY error. If Squid validation<br> fails afterwards, the TLS connection is closed (before any payload is<br> exchanged). Ideally, the negotiation with an untrusted server should not<br> complete, but complexity BIO changes required to prevent such premature<br> completion is probably not worth reaching that ideal, especially since<br> all of this is just a workaround.<br> <br> The long-term solution is adding SSL_ERROR_WANT_RETRY_VERIFY to OpenSSL,<br> giving an application a chance to download the missing certificates<br> during SSL_connect() negotiations. We assist OpenSSL team with that<br> change, but it will not be available at least until OpenSSL v3.0.<br> <br> This description and changes are not specific to SslBump code paths.<br> <br> This is a Measurement Factory project.<br><br> acinclude/lib-checks.m4 | 6 ++<br> compat/openssl.h | 30 +++++++++<br> src/security/Handshake.cc | 46 --------------<br> src/security/Handshake.h | 5 +-<br> src/security/Io.cc | 27 ++++++++<br> src/security/Io.h | 14 +++-<br> src/security/PeerConnector.cc | 211 ++++++++++++++++++++++++++++++++++++++++++++-----------------<br> src/security/PeerConnector.h | 46 ++++++++++----<br> src/security/Session.h | 4 ++<br> src/ssl/bio.cc | 7 --<br> src/ssl/bio.h | 8 ---<br> src/ssl/gadgets.h | 1 +<br> src/ssl/support.cc | 355 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------<br> src/ssl/support.h | 57 +++++++++++++++--<br> src/tests/stub_libsecurity.cc | 4 +-<br> 15 files changed, 582 insertions(+), 239 deletions(-)<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 23, 2023 at 4:56 PM Hamilton Coutinho <<a href="mailto:hamilton.coutinho@gmail.com">hamilton.coutinho@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_default" style="font-size:small">Hi all,</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">We found a way to reproduce the leak: set up squid to run in intercept mode + SSL description + memory pools off (to ease identifying the leak) and then generate requests to sites with invalid certs (eg, CA not installed), for instance: curl -k <a href="https://slscr.update.microsoft.com" target="_blank">https://slscr.update.microsoft.com</a>. squidclient mgr:mem should show ever increasing HttpRequest instances.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">As far as I can tell, the HttpRequest object created in ConnStateData::buildFakeRequest() is never freed because its refcount > 0.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">Any ideas where an HTTPMSGUNLOCK() might be missing?</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">Thanks!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jan 18, 2023 at 11:28 AM Hamilton Coutinho <<a href="mailto:hamilton.coutinho@gmail.com" target="_blank">hamilton.coutinho@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_default" style="font-size:small">Hi Alex,</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">Thanks for the prompt reply! Thanks also for the clarifications.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">Agreed, I just realized the requests seem to be failing with Http::scServiceUnavailable, so my focus turned to Security::PeerConnector::sslCrtvdHandleReply() and friends.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">Best.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jan 18, 2023 at 11:11 AM Alex Rousskov <<a href="mailto:rousskov@measurement-factory.com" target="_blank">rousskov@measurement-factory.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 1/18/23 13:46, Hamilton Coutinho wrote:<br>
> Hi all,<br>
> <br>
> We are observing what seems to be several objects leaking in the output <br>
> mgr:mem, to the tune of 10s of 1000s <br>
> of HttpRequest, HttpHeaderEntry, Comm::Connection, Security::ErrorDetail, cbdata PeekingPeerConnector (31), etc.<br>
> <br>
> We dumped a core and managed to find some HttpRequest objects and they <br>
> all seem to have failed in the same way, with an ERR_SECURE_CONNECT_FAIL <br>
> category, for a site that has a certificate signed by a CA authority not <br>
> available to squid.<br>
> <br>
> If I would guess, the origin of the problem might be in <br>
> Ssl::PeekingPeerConnector::checkForPeekAndSpliceMatched():<br>
> <br>
> if (finalAction == Ssl::bumpTerminate) {<br>
> bail(new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scForbidden, <br>
> request.getRaw(), al));<br>
> clientConn->close();<br>
> clientConn = nullptr;<br>
> <br>
> Wondering if assigning null to clientConn there would be premature.<br>
<br>
<br>
FWIW, that connection pointer reset itself looks OK to me. ConnStateData <br>
and/or others should have a connection closure handler attached to the <br>
clientConn descriptor. That handler should be notified by Comm and <br>
initiate cleanup of the objects responsible for client-Squid communication.<br>
<br>
The bail() call above should inform the requestor about the <br>
error/termination and terminate this AsyncJob. That requestor should <br>
then close the Squid-server connection and clean up associated state.<br>
<br>
While there may be bugs in those "should..." sequences, please note that <br>
the pasted code is not related to handling of untrusted origin servers <br>
(unless your ssl_bump rules specifically activate the terminate action <br>
upon discovering such an origin server). The pasted code is reacting to <br>
an "ssl_bump terminate" rule matching.<br>
<br>
<br>
Cheers,<br>
<br>
Alex.<br>
<br>
_______________________________________________<br>
squid-dev mailing list<br>
<a href="mailto:squid-dev@lists.squid-cache.org" target="_blank">squid-dev@lists.squid-cache.org</a><br>
<a href="http://lists.squid-cache.org/listinfo/squid-dev" rel="noreferrer" target="_blank">http://lists.squid-cache.org/listinfo/squid-dev</a><br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr"><div dir="ltr"><div><div dir="ltr"><div><font size="2">Hamilton</font></div></div></div></div></div>
</blockquote></div><br clear="all"><div><br></div><span>-- </span><br><div dir="ltr"><div dir="ltr"><div><div dir="ltr"><div><font size="2">Hamilton</font></div></div></div></div></div>
</blockquote></div><br clear="all"><div><br></div><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><font size="2">Hamilton</font></div></div></div></div></div>