Re: [squid-dev] Squid 5.6 leaking memory when peeking for an origin with an invalid certificate
Hi Alex,
Thanks for your reply. I will try your patch.
FWIW, we eventually arrived at a very different fix. See attached.
Best.
On Wed, May 10, 2023 at 2:09 PM Alex Rousskov <
rouss...@measurement-factory.com> wrote:
> On 3/23/23 19:56, Hamilton Coutinho wrote:
>
> > 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 https://slscr.update.microsoft.com
>
> Thanks a lot for sharing those details! Using them, I was able to
> reproduce one memory leak. Please try the following fix:
> https://github.com/squid-cache/squid/pull/1346
>
> I do not know whether that PR changes port to v5 cleanly or whether the
> same bug exists in v5. I can only speculate that v5 has a similar leak.
>
>
> > squidclient mgr:mem should show ever increasing HttpRequest instances.
>
> > As far as I can tell, the HttpRequest object created
> > in ConnStateData::buildFakeRequest() is never freed because its refcount
> > > 0.
>
> > Any ideas where an HTTPMSGUNLOCK() might be missing?
>
> Sorry, I do not know. In my master/v7-based tests, HttpRequest objects
> are not leaking. However, I am not testing an intercepting Squid. I
> still have one red flag to investigate, so perhaps I will be able to
> reproduce and fix that HttpRequest leak as well.
>
>
> HTH,
>
> Alex.
>
>
> > On Wed, Jan 18, 2023 at 11:28 AM Hamilton Coutinho wrote:
> >
> > Hi Alex,
> >
> > Thanks for the prompt reply! Thanks also for the clarifications.
> >
> > Agreed, I just realized the requests seem to be failing
> > with Http::scServiceUnavailable, so my focus turned
> > to Security::PeerConnector::sslCrtvdHandleReply() and friends.
> >
> > Best.
> >
> > On Wed, Jan 18, 2023 at 11:11 AM Alex Rousskov
> > > <mailto:rouss...@measurement-factory.com>> wrote:
> >
> > On 1/18/23 13:46, Hamilton Coutinho wrote:
> > > Hi all,
> > >
> > > We are observing what seems to be several objects leaking in
> > the output
> > > mgr:mem, to the tune of 10s of 1000s
> > >
> >
> of HttpRequest, HttpHeaderEntry, Comm::Connection, Security::ErrorDetail,
> cbdata
> PeekingPeerConnector (31), etc.
> > >
> > > We dumped a core and managed to find some HttpRequest objects
> > and they
> > > all seem to have failed in the same way, with an
> > ERR_SECURE_CONNECT_FAIL
> > > category, for a site that has a certificate signed by a CA
> > authority not
> > > available to squid.
> > >
> > > If I would guess, the origin of the problem might be in
> > > Ssl::PeekingPeerConnector::checkForPeekAndSpliceMatched():
> > >
> > > if (finalAction == Ssl::bumpTerminate) {
> > > bail(new ErrorState(ERR_SECURE_CONNECT_FAIL,
> > Http::scForbidden,
> > > request.getRaw(), al));
> > > clientConn->close();
> > > clientConn = nullptr;
> > >
> > > Wondering if assigning null to clientConn there would be
> > premature.
> >
> >
> > FWIW, that connection pointer reset itself looks OK to me.
> > ConnStateData
> > and/or others should have a connection closure handler attached
> > to the
> > clientConn descriptor. That handler should be notified by Comm
> and
> > initiate cleanup of the objects responsible for client-Squid
> > communication.
> >
> > The bail() call above should inform the requestor about the
> > error/termination and terminate this AsyncJob. That requestor
> > should
> > then close the Squid-server connection and clean up associated
> > state.
> >
> > While there may be bugs in those "should..." sequences, please
> > note that
> > the pasted code is not related to handling of untrusted origin
> > servers
> > (unless your ssl_bump rules specifically activate the terminate
> > action
> > upon discovering such an origin server). The pasted code is
> > reacting to
> >
Re: [squid-dev] Squid 5.6 leaking memory when peeking for an origin with an invalid certificate
Hello all, We moved one step closer to the root cause. We bisected the code and found out that the commit that introduced the leak is: $ git show --stat 800967afde7921eb22a2711c5a75b5cd9c9d7178 commit 800967afde7921eb22a2711c5a75b5cd9c9d7178 (HEAD) Author: Christos Tsantilas Date: Mon Feb 22 21:15:32 2021 + Handling missing issuer certificates for TLSv1.3 (#766) Prior to TLS v1.3 Squid could detect and fetch missing intermediate server certificates by parsing TLS ServerHello. TLS v1.3 encrypts the relevant part of the handshake, making such "prefetch" impossible. Instead of looking for certificates in TLS ServerHello, Squid now waits for the OpenSSL built-in certificate validation to fail with an X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY error. Squid-supplied verify_callback function tells OpenSSL to ignore that error. Squid SSL_connect()-calling code detects that the error was ignored and, if possible, fetches the missing certificates and orchestrates certificate chain validation outside the SSL_connect() sequence. If that validation is successful, Squid continues with SSL_connect(). See comments inside Security::PeerConnector::negotiate() for low-level details. In some cases, OpenSSL is able to complete SSL_connect() with an ignored X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY error. If Squid validation fails afterwards, the TLS connection is closed (before any payload is exchanged). Ideally, the negotiation with an untrusted server should not complete, but complexity BIO changes required to prevent such premature completion is probably not worth reaching that ideal, especially since all of this is just a workaround. The long-term solution is adding SSL_ERROR_WANT_RETRY_VERIFY to OpenSSL, giving an application a chance to download the missing certificates during SSL_connect() negotiations. We assist OpenSSL team with that change, but it will not be available at least until OpenSSL v3.0. This description and changes are not specific to SslBump code paths. This is a Measurement Factory project. acinclude/lib-checks.m4 | 6 ++ compat/openssl.h | 30 + src/security/Handshake.cc | 46 -- src/security/Handshake.h | 5 +- src/security/Io.cc| 27 src/security/Io.h | 14 +++- src/security/PeerConnector.cc | 211 - src/security/PeerConnector.h | 46 ++ src/security/Session.h| 4 ++ src/ssl/bio.cc| 7 -- src/ssl/bio.h | 8 --- src/ssl/gadgets.h | 1 + src/ssl/support.cc| 355 ++ src/ssl/support.h | 57 +++-- src/tests/stub_libsecurity.cc | 4 +- 15 files changed, 582 insertions(+), 239 deletions(-) On Thu, Mar 23, 2023 at 4:56 PM Hamilton Coutinho < hamilton.couti...@gmail.com> wrote: > Hi all, > > 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 https://slscr.update.microsoft.com. > squidclient mgr:mem should show ever increasing HttpRequest instances. > > As far as I can tell, the HttpRequest object created > in ConnStateData::buildFakeRequest() is never freed because its refcount > > 0. > > Any ideas where an HTTPMSGUNLOCK() might be missing? > > Thanks! > > On Wed, Jan 18, 2023 at 11:28 AM Hamilton Coutinho < > hamilton.couti...@gmail.com> wrote: > >> Hi Alex, >> >> Thanks for the prompt reply! Thanks also for the clarifications. >> >> Agreed, I just realized the requests seem to be failing >> with Http::scServiceUnavailable, so my focus turned >> to Security::PeerConnector::sslCrtvdHandleReply() and friends. >> >> Best. >> >> On Wed, Jan 18, 2023 at 11:11 AM Alex Rousskov < >> rouss...@measurement-factory.com> wrote: >> >>> On 1/18/23 13:46, Hamilton Coutinho wrote: >>> > Hi all, >>> > >>> > We are observing what seems to be several objects leaking in the >>> output >>> > mgr:mem, to the tune of 10s of 1000s >>> > >>> of HttpRequest, HttpHeaderEntry, Comm::Connection, Security::ErrorDetail, >>> cbdata >>> PeekingPeerConnector (31), etc. >>> > >>> > We dumped a core and managed to find some HttpRequest objects and they >>> > all seem to have failed in the same way, with an >>> ERR_SECURE_CONNECT_FAIL >>> >
Re: [squid-dev] Squid 5.6 leaking memory when peeking for an origin with an invalid certificate
Hi all,
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 https://slscr.update.microsoft.com.
squidclient mgr:mem should show ever increasing HttpRequest instances.
As far as I can tell, the HttpRequest object created
in ConnStateData::buildFakeRequest() is never freed because its refcount >
0.
Any ideas where an HTTPMSGUNLOCK() might be missing?
Thanks!
On Wed, Jan 18, 2023 at 11:28 AM Hamilton Coutinho <
hamilton.couti...@gmail.com> wrote:
> Hi Alex,
>
> Thanks for the prompt reply! Thanks also for the clarifications.
>
> Agreed, I just realized the requests seem to be failing
> with Http::scServiceUnavailable, so my focus turned
> to Security::PeerConnector::sslCrtvdHandleReply() and friends.
>
> Best.
>
> On Wed, Jan 18, 2023 at 11:11 AM Alex Rousskov <
> rouss...@measurement-factory.com> wrote:
>
>> On 1/18/23 13:46, Hamilton Coutinho wrote:
>> > Hi all,
>> >
>> > We are observing what seems to be several objects leaking in the output
>> > mgr:mem, to the tune of 10s of 1000s
>> >
>> of HttpRequest, HttpHeaderEntry, Comm::Connection, Security::ErrorDetail,
>> cbdata
>> PeekingPeerConnector (31), etc.
>> >
>> > We dumped a core and managed to find some HttpRequest objects and they
>> > all seem to have failed in the same way, with an
>> ERR_SECURE_CONNECT_FAIL
>> > category, for a site that has a certificate signed by a CA authority
>> not
>> > available to squid.
>> >
>> > If I would guess, the origin of the problem might be in
>> > Ssl::PeekingPeerConnector::checkForPeekAndSpliceMatched():
>> >
>> > if (finalAction == Ssl::bumpTerminate) {
>> > bail(new ErrorState(ERR_SECURE_CONNECT_FAIL,
>> Http::scForbidden,
>> > request.getRaw(), al));
>> > clientConn->close();
>> > clientConn = nullptr;
>> >
>> > Wondering if assigning null to clientConn there would be premature.
>>
>>
>> FWIW, that connection pointer reset itself looks OK to me. ConnStateData
>> and/or others should have a connection closure handler attached to the
>> clientConn descriptor. That handler should be notified by Comm and
>> initiate cleanup of the objects responsible for client-Squid
>> communication.
>>
>> The bail() call above should inform the requestor about the
>> error/termination and terminate this AsyncJob. That requestor should
>> then close the Squid-server connection and clean up associated state.
>>
>> While there may be bugs in those "should..." sequences, please note that
>> the pasted code is not related to handling of untrusted origin servers
>> (unless your ssl_bump rules specifically activate the terminate action
>> upon discovering such an origin server). The pasted code is reacting to
>> an "ssl_bump terminate" rule matching.
>>
>>
>> Cheers,
>>
>> Alex.
>>
>> ___
>> squid-dev mailing list
>> squid-dev@lists.squid-cache.org
>> http://lists.squid-cache.org/listinfo/squid-dev
>>
>
>
> --
> Hamilton
>
--
Hamilton
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] Squid 5.6 leaking memory when peeking for an origin with an invalid certificate
Hi Alex,
Thanks for the prompt reply! Thanks also for the clarifications.
Agreed, I just realized the requests seem to be failing
with Http::scServiceUnavailable, so my focus turned
to Security::PeerConnector::sslCrtvdHandleReply() and friends.
Best.
On Wed, Jan 18, 2023 at 11:11 AM Alex Rousskov <
rouss...@measurement-factory.com> wrote:
> On 1/18/23 13:46, Hamilton Coutinho wrote:
> > Hi all,
> >
> > We are observing what seems to be several objects leaking in the output
> > mgr:mem, to the tune of 10s of 1000s
> >
> of HttpRequest, HttpHeaderEntry, Comm::Connection, Security::ErrorDetail,
> cbdata
> PeekingPeerConnector (31), etc.
> >
> > We dumped a core and managed to find some HttpRequest objects and they
> > all seem to have failed in the same way, with an ERR_SECURE_CONNECT_FAIL
> > category, for a site that has a certificate signed by a CA authority not
> > available to squid.
> >
> > If I would guess, the origin of the problem might be in
> > Ssl::PeekingPeerConnector::checkForPeekAndSpliceMatched():
> >
> > if (finalAction == Ssl::bumpTerminate) {
> > bail(new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scForbidden,
> > request.getRaw(), al));
> > clientConn->close();
> > clientConn = nullptr;
> >
> > Wondering if assigning null to clientConn there would be premature.
>
>
> FWIW, that connection pointer reset itself looks OK to me. ConnStateData
> and/or others should have a connection closure handler attached to the
> clientConn descriptor. That handler should be notified by Comm and
> initiate cleanup of the objects responsible for client-Squid communication.
>
> The bail() call above should inform the requestor about the
> error/termination and terminate this AsyncJob. That requestor should
> then close the Squid-server connection and clean up associated state.
>
> While there may be bugs in those "should..." sequences, please note that
> the pasted code is not related to handling of untrusted origin servers
> (unless your ssl_bump rules specifically activate the terminate action
> upon discovering such an origin server). The pasted code is reacting to
> an "ssl_bump terminate" rule matching.
>
>
> Cheers,
>
> Alex.
>
> ___
> squid-dev mailing list
> squid-dev@lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>
--
Hamilton
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
[squid-dev] Squid 5.6 leaking memory when peeking for an origin with an invalid certificate
Hi all,
We are observing what seems to be several objects leaking in the output
mgr:mem, to the tune of 10s of 1000s
of HttpRequest, HttpHeaderEntry, Comm::Connection,
Security::ErrorDetail, cbdata
PeekingPeerConnector (31), etc.
We dumped a core and managed to find some HttpRequest objects and they all
seem to have failed in the same way, with an ERR_SECURE_CONNECT_FAIL
category, for a site that has a certificate signed by a CA authority not
available to squid.
If I would guess, the origin of the problem might be in
Ssl::PeekingPeerConnector::checkForPeekAndSpliceMatched():
if (finalAction == Ssl::bumpTerminate) {
bail(new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scForbidden,
request.getRaw(), al));
clientConn->close();
clientConn = nullptr;
Wondering if assigning null to clientConn there would be premature.
Any thoughts?
Thanks!
--
Hamilton
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
