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

[MegaManSec]

No description provided.

[rousskov]

I agree that we should check OpenSSL function calls above for failures and that this X509_free() should be added to avoid leaking the certificate as stated in the PR title. Thank you for working on fixing this code!

The rest of these changes need more work as detailed in specific change requests.

[rousskov]

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.

[rousskov]

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.

[MegaManSec]

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

[rousskov]

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().