Skip to content

Fix SSL-Bump X509 certificate leak on SSL_set_ex_data() failure #2240

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MegaManSec wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix SSL-Bump X509 certificate leak on SSL_set_ex_data() failure #2240

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2d519c
ssl-bump: fix X509 ref leak on SSL_set_ex_data() failure
MegaManSec Sep 9, 2025
591841b
Update src/ssl/PeekingPeerConnector.cc
MegaManSec Sep 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/ssl/PeekingPeerConnector.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,14 @@ Ssl::PeekingPeerConnector::initialize(Security::SessionPointer &serverSession)
serverBump->attachServerSession(serverSession);
// store peeked cert to check SQUID_X509_V_ERR_CERT_CHANGE
if (X509 *peeked_cert = serverBump->serverCert.get()) {
X509_up_ref(peeked_cert);
SSL_set_ex_data(serverSession.get(), ssl_ex_index_ssl_peeked_cert, peeked_cert);
if (!X509_up_ref(peeked_cert)) {
Copy link
Contributor

@rousskov rousskov Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we could just ignore these failures like current PR code does, but we cannot: If we cannot add this certificate, then we cannot check for SQUID_X509_V_ERR_CERT_CHANGE problems. If we cannot check for those problems, we must terminate the connection because it is unsafe to proceed.

There are several reasonable ways to fix this, but I will propose the simplest solution that I can think of: Agree that if X509_up_ref() fails, then there is a Squid bug somewhere. The only way that function can fail is if we overflow the lock or corrupt certificate metadata. Both are Squid bugs.

The corresponding solution would wrap X509_up_ref() into a Squid function called Ssl::Lock() that does something like this:

const auto locked = X509_up_ref(cert);
Assure(locked);

Ssl::Lock() will return void. We can place that function into generic Security namespace instead, but I would not do that (for now) because GnuTLS does not lock its certificates.

This code will then just call Ssl::Lock(peeked_cert). We should convert other callers as well, but that can wait for a dedicated PR if you prefer, especially if this PR provides a reusable function for that future conversion (rather than adding more technical debt by inlining that solution once).

Please test callers reaction on the thrown exception by injecting/faking an X509_up_ref() error.

Copy link
Contributor Author

@MegaManSec MegaManSec Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have the time to learn more about this unfortunately. Any chance this can be turned into a bug report?

debugs(83, DBG_IMPORTANT, "WARNING: X509_up_ref(peeked_cert) failed on server certificate");
Copy link
Contributor

@rousskov rousskov Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rule of thumb: Do not inform admins about transaction-specific problems they can do nothing about. I hope that addressing the other change request will eliminate the two new level-1 messages. If it does not, then they need to be converted to level 3.

} else if (!SSL_set_ex_data(serverSession.get(),
ssl_ex_index_ssl_peeked_cert,
peeked_cert)) {
debugs(83, DBG_IMPORTANT, "WARNING: SSL_set_ex_data(ssl_ex_index_ssl_peeked_cert) failed; dropping extra X509 ref");
Copy link
Contributor

@rousskov rousskov Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we could just ignore these failures like current PR code does, but we cannot: If we cannot add this certificate, then we cannot check for SQUID_X509_V_ERR_CERT_CHANGE problems. If we cannot check for those problems, we must terminate the connection because it is unsafe to proceed.

To quit, we should set an error object and return false. Something along these lines (untested but please do test by injecting this failure):

Suggested change
debugs(83, DBG_IMPORTANT, "WARNING: SSL_set_ex_data(ssl_ex_index_ssl_peeked_cert) failed; dropping extra X509 ref");
debugs(83, 3, "SSL_set_ex_data(ssl_ex_index_ssl_peeked_cert) failed" << Ssl::ReportAndForgetErrors);
const auto err = ErrorState::NewForwarding(ERR_SECURE_CONNECT_FAIL, request, al);
static const auto d = MakeNamedErrorDetail("TLS_SET_EX_PEEKED_CERT");
err->detailError(d);
noteNegotiationDone(err);
bail(err);
return false;

For Ssl::ReportAndForgetErrors() to work best, please call Ssl::ForgetErrors() before calling SSL_set_ex_data().

X509_free(peeked_cert);
}
}
}
}
Expand Down