On 21/04/17 15:58, Alex Rousskov wrote:
On 04/20/2017 04:50 PM, Amos Jeffries wrote:
These two patches begin the refactoring work to replace OpenSSL
SSL_set/get_ex_data() API with a generic libsecurity API.
Thank you for working on this. I think the patch is on the right track
overall, but I found one bug, and I believe the proposed GetFrom() API
needs to be revised. I also object to adding any code that calls TLS
connections "sessions", especially when we already have code that
calls TLS sessions "sessions". If at all possible, please fix the
naming in the existing code (as already discussed elsewhere) before
adding any new stuff that makes the problem worse.
Lets not get bogged down in that again.
Details below.
+/// extra data which we attach to a TLS session
s/session/connection/
No this data is explicitly not part of the "TLS connection". It persists
long after the "TLS connection" is ended and may be shared across
multiple if and when the "TLS session" does so.
+/// extra data which we attach to a TLS session +class ExtraData
Please avoid names that end with State, Data, Info, Object, Status,
and similar noise/placeholder words without any specific meaning. In
this particular case, the class should be named ConnectionExtras and
described as "Squid-specific TLS connection details" or something like
that. The variables holding class objects should then become "extras"
instead of "data".
OpenSSL calls this "ex_data" and GnuTLS calls it a "datum pointer", in
both cases it is documented as being attached to a session. They are
definitely NOT destructed when the connection is ended.
+ /// \retval nullptr only if session is nullptr + static
Security::ExtraData *GetFrom(const SessionPointer &);
The patch contains: * code that asserts if GetFrom() returns nil *
code that crashes if GetFrom() returns nil * code that pointlessly
checks whether GetFrom() returns nil I did not see any code that
actually expects a nil connection pointer and can handle it correctly.
Specific alternatives would be useful. I'm looking at a mix of code that
can only occur when a SessionPointer is available [use without checks],
code that can occur with or without one [check with an if], and code
that I'm not sure of its status [check with an if where possible, or an
assert as alternative to risking segfault].
The method itself does not store the TLS connection so it should not
need a pointer to one.
Since when was it a requirement that all function parameters were
stored? it is being _used_.
Regardless of what that parameter is called it is the object that our
class is being attached to. So yes it is absolutely needed.
To avoid multiplying these problems beyond the patch, please change
the method to accept a reference to the connection (instead of a
pointer) and return a reference to ConnectionExtras (instead of a
pointer).
Er, did you read the pt1 code? The pointer received is passed on to the
underlying library API. Using a reference to an object instance of
unknown type is not doable. And we also agreed that mixing smart and raw
pointers in our API was a bad idea.
The connection is not accessible in most places this function is called.
What we have reliably is a pointer to the session state where our object
is (to be) associated. Sometimes that session has a connection, but half
of the calls are at times before the TLS session initiates a TLS connection.
Please s/GetFrom/Of/ or, if you insist, s/GetFrom/From/ to shorten the
name and emphasize that this is not just an existing extras retrieval
method. From the caller point of view, all connections have a valid
extras object (and that is a good thing!).
- if (hostName) - SSL_set_ex_data(serverSession.get(),
ssl_ex_index_server, (void*)hostName); + if (hostName.isEmpty()) {
You want the opposite condition. AFAICT, this bug implies that the
changes were essentially untested. Please explicitly disclose untested
patches.
Thanks for the catch.
That code path is untested by me, yes. I cannot test the OpenSSL code
changes, and thought I'd stated that enough by now. It is one of the
reasons I'm going about these patches so slowly and piece-wise.
- SBuf *hostName = NULL; + SBuf hostName;
Or perhaps we can do the following instead? SBuf &hostName = extras->sni;
Nice. Done.
+ Security::SessionPointer session(ssl, [](SSL *){/* do not destroy
*/});
This ugly hack will no longer be needed after the above changes AFAICT.
No such luck. At least it is part of the callback state preparation.
+class ExtraData +{ +public: + SBuf sni;
This member may not actually contain an SNI extension value (neither
sent nor received). Let's not lie to ourselves and call it serverName.
Please document new data members. Consider: /// suspected or actual
connection destination (from various sources) SBuf serverName;
Sigh. Ironicly it started as that. Then realized the value being stored
there was SNI or an SNI-to-be value.
- sniServer = !request->url.hostIsNumeric() ? request->url.host() :
NULL; + sniServer = !request->url.hostIsNumeric() ?
request->url.host() : nullptr;
An out-of-scope change.
// validationRequest disappears on return so no need to
cbdataReference validationRequest.errors = errs; + try {
An out-of-scope change. Thank you, Alex.
Removed those.
Amos
=== modified file 'src/security/Session.cc'
--- src/security/Session.cc 2017-02-05 06:12:19 +0000
+++ src/security/Session.cc 2017-04-21 12:06:49 +0000
@@ -122,6 +122,8 @@
gnutls_session_t tmp;
errCode = gnutls_init(&tmp, static_cast<unsigned int>(type) | GNUTLS_NONBLOCK);
Security::SessionPointer session(tmp, [](gnutls_session_t p) {
+ // free any Security::Extras object associated with this session
+ delete static_cast<Security::Extras *>(gnutls_session_get_ptr(p));
debugs(83, 5, "gnutls_deinit session=" << (void*)p);
gnutls_deinit(p);
});
@@ -201,6 +203,49 @@
}
}
+#if USE_OPENSSL
+/// "free" function for Security::Extras
+static void
+free_Extras(void *, void *ptr, CRYPTO_EX_DATA *, int, long, void *)
+{
+ delete static_cast<Security::Extras *>(ptr);
+}
+#endif
+
+Security::Extras *
+Security::Extras::From(const SessionPointer &session)
+{
+ if (!session)
+ return nullptr;
+
+ Security::Extras *extras = nullptr;
+
+ // get any data we attached to the session earlier
+#if USE_OPENSSL
+ // initialize extended-data registration with OpenSSL if needed
+ static int extras_index = -1;
+ if (extras_index == -1) {
+ extras_index = SSL_get_ex_new_index(0, const_cast<char *>("Security::Extras"), nullptr, nullptr, free_Extras);
+ }
+
+ extras = static_cast<Security::Extras *>(SSL_get_ex_data(session.get(), extras_index));
+#elif USE_GNUTLS
+ extras = static_cast<Security::Extras *>(gnutls_session_get_ptr(session.get()));
+#endif
+
+ // allocate and attach a new data object if missing
+ if (!extras) {
+ extras = new Security::Extras;
+#if USE_OPENSSL
+ SSL_set_ex_data(session.get(), extras_index, extras);
+#elif USE_GNUTLS
+ gnutls_session_set_ptr(session.get(), extras);
+#endif
+ }
+
+ return extras;
+}
+
bool
Security::SessionIsResumed(const Security::SessionPointer &s)
{
=== modified file 'src/security/Session.h'
--- src/security/Session.h 2017-02-10 13:35:05 +0000
+++ src/security/Session.h 2017-04-21 11:11:18 +0000
@@ -11,6 +11,7 @@
#include "base/HardFun.h"
#include "comm/forward.h"
+#include "sbuf/SBuf.h"
#include "security/LockingPointer.h"
#include <memory>
@@ -56,6 +57,20 @@
#endif
+/// extra data which we attach to a TLS session
+class Extras
+{
+public:
+ SBuf serverName;
+
+public:
+ /// The extra data (if any) associated with a session.
+ /// \note Guaranteed to return an object if the session pointer is set,
+ /// will create if necessary.
+ /// \retval nullptr only if session is nullptr
+ static Security::Extras *From(const SessionPointer &);
+};
+
/// send the shutdown/bye notice for an active TLS session.
void SessionSendGoodbye(const Security::SessionPointer &);
=== modified file 'src/tests/stub_libsecurity.cc'
--- src/tests/stub_libsecurity.cc 2017-02-05 06:12:19 +0000
+++ src/tests/stub_libsecurity.cc 2017-04-21 11:12:59 +0000
@@ -98,6 +98,7 @@
bool CreateClientSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, const char *) STUB_RETVAL(false)
bool CreateServerSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, const char *) STUB_RETVAL(false)
void SessionSendGoodbye(const Security::SessionPointer &) STUB
+Security::Extras *Security::Extras::From(const Security::SessionPointer &) STUB_RETVAL(nullptr)
bool SessionIsResumed(const Security::SessionPointer &) STUB_RETVAL(false)
void MaybeGetSessionResumeData(const Security::SessionPointer &, Security::SessionStatePointer &) STUB
void SetSessionResumeData(const Security::SessionPointer &, const Security::SessionStatePointer &) STUB
=== modified file 'src/adaptation/icap/Xaction.cc'
--- src/adaptation/icap/Xaction.cc 2017-02-16 11:51:56 +0000
+++ src/adaptation/icap/Xaction.cc 2017-04-21 11:13:20 +0000
@@ -712,13 +712,14 @@
return false;
assert(!icapService->cfg().secure.sslDomain.isEmpty());
+ auto *extras = Security::Extras::From(serverSession);
+ assert(extras);
+ extras->serverName = icapService->cfg().secure.sslDomain;
+
#if USE_OPENSSL
- SBuf *host = new SBuf(icapService->cfg().secure.sslDomain);
- SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, host);
-
ACLFilledChecklist *check = static_cast<ACLFilledChecklist *>(SSL_get_ex_data(serverSession.get(), ssl_ex_index_cert_error_check));
if (check)
- check->dst_peer_name = *host;
+ check->dst_peer_name = extras->serverName;
#endif
Security::SetSessionResumeData(serverSession, icapService->sslSession);
=== modified file 'src/globals.h'
--- src/globals.h 2017-01-01 00:12:22 +0000
+++ src/globals.h 2017-04-19 06:38:34 +0000
@@ -98,7 +98,6 @@
extern unsigned int WIN32_run_mode; /* _WIN_SQUID_RUN_MODE_INTERACTIVE */
#endif
-extern int ssl_ex_index_server; /* -1 */
extern int ssl_ctx_ex_index_dont_verify_domain; /* -1 */
extern int ssl_ex_index_cert_error_check; /* -1 */
extern int ssl_ex_index_ssl_error_detail; /* -1 */
=== modified file 'src/security/BlindPeerConnector.cc'
--- src/security/BlindPeerConnector.cc 2017-01-12 13:26:45 +0000
+++ src/security/BlindPeerConnector.cc 2017-04-21 11:13:35 +0000
@@ -37,22 +37,19 @@
return false;
}
+ auto *extras = Security::Extras::From(serverSession);
+ assert(extras);
+
if (const CachePeer *peer = serverConnection()->getPeer()) {
assert(peer);
// NP: domain may be a raw-IP but it is now always set
assert(!peer->secure.sslDomain.isEmpty());
-
-#if USE_OPENSSL
- // const loss is okay here, ssl_ex_index_server is only read and not assigned a destructor
- SBuf *host = new SBuf(peer->secure.sslDomain);
- SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, host);
+ extras->serverName = peer->secure.sslDomain;
Security::SetSessionResumeData(serverSession, peer->sslSession);
} else {
- SBuf *hostName = new SBuf(request->url.host());
- SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, (void*)hostName);
-#endif
+ extras->serverName = request->url.host();
}
debugs(83, 5, "success");
=== modified file 'src/security/PeerConnector.cc'
--- src/security/PeerConnector.cc 2017-02-15 03:10:55 +0000
+++ src/security/PeerConnector.cc 2017-04-22 10:49:44 +0000
@@ -233,8 +233,10 @@
// Ssl::CertValidationRequest object used only to pass data to
// Ssl::CertValidationHelper::submit method.
validationRequest.ssl = session;
- if (SBuf *dName = (SBuf *)SSL_get_ex_data(session.get(), ssl_ex_index_server))
- validationRequest.domainName = dName->c_str();
+ if (auto *extras = Security::Extras::From(session)) {
+ if (!extras->serverName.isEmpty())
+ validationRequest.domainName = extras->serverName.c_str();
+ }
if (Security::CertErrors *errs = static_cast<Security::CertErrors *>(SSL_get_ex_data(session.get(), ssl_ex_index_ssl_errors)))
// validationRequest disappears on return so no need to cbdataReference
validationRequest.errors = errs;
@@ -277,8 +279,8 @@
if (Debug::Enabled(83, 5)) {
Security::SessionPointer ssl(fd_table[serverConnection()->fd].ssl);
- SBuf *server = static_cast<SBuf *>(SSL_get_ex_data(ssl.get(), ssl_ex_index_server));
- debugs(83,5, RawPointer("host", server) << " cert validation result: " << validationResponse->resultCode);
+ const auto *extras = Security::Extras::From(ssl);
+ debugs(83,5, "host=" << extras->serverName << " cert validation result: " << validationResponse->resultCode);
}
if (validationResponse->resultCode == ::Helper::Error) {
=== modified file 'src/ssl/PeekingPeerConnector.cc'
--- src/ssl/PeekingPeerConnector.cc 2017-02-05 05:57:32 +0000
+++ src/ssl/PeekingPeerConnector.cc 2017-04-22 03:37:18 +0000
@@ -143,27 +143,26 @@
// client connection is required in the case we need to splice
// or terminate client and server connections
assert(clientConn != NULL);
- SBuf *hostName = NULL;
+
+ auto *extras = Security::Extras::From(serverSession);
+ SBuf &hostName = extras->serverName;
//Enable Status_request TLS extension, required to bump some clients
SSL_set_tlsext_status_type(serverSession.get(), TLSEXT_STATUSTYPE_ocsp);
const Security::TlsDetails::Pointer details = csd->tlsParser.details;
if (details && !details->serverName.isEmpty())
- hostName = new SBuf(details->serverName);
+ hostName = details->serverName;
- if (!hostName) {
+ if (hostName.isEmpty()) {
// While we are peeking at the certificate, we may not know the server
// name that the client will request (after interception or CONNECT)
// unless it was the CONNECT request with a user-typed address.
const bool isConnectRequest = !csd->port->flags.isIntercepted();
if (!request->flags.sslPeek || isConnectRequest)
- hostName = new SBuf(request->url.host());
+ hostName = request->url.host();
}
- if (hostName)
- SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, (void*)hostName);
-
Must(!csd->serverBump() || csd->serverBump()->step <= Ssl::bumpStep2);
if (csd->sslBumpMode == Ssl::bumpPeek || csd->sslBumpMode == Ssl::bumpStare) {
auto clientSession = fd_table[clientConn->fd].ssl.get();
@@ -189,10 +188,10 @@
// to the origin server and we know the server host name.
const char *sniServer = NULL;
const bool redirected = request->flags.redirected && ::Config.onoff.redir_rewrites_host;
- if (!hostName || redirected)
+ if (hostName.isEmpty() || redirected)
sniServer = !request->url.hostIsNumeric() ? request->url.host() : NULL;
else
- sniServer = hostName->c_str();
+ sniServer = hostName.c_str();
if (sniServer)
Ssl::setClientSNI(serverSession.get(), sniServer);
=== modified file 'src/ssl/support.cc'
--- src/ssl/support.cc 2017-02-05 05:57:32 +0000
+++ src/ssl/support.cc 2017-04-21 11:12:24 +0000
@@ -253,7 +253,8 @@
char buffer[256] = "";
SSL *ssl = (SSL *)X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
SSL_CTX *sslctx = SSL_get_SSL_CTX(ssl);
- SBuf *server = (SBuf *)SSL_get_ex_data(ssl, ssl_ex_index_server);
+ Security::SessionPointer session(ssl, [](SSL *){/* do not destroy */});
+ auto *extras = Security::Extras::From(session);
void *dont_verify_domain = SSL_CTX_get_ex_data(sslctx, ssl_ctx_ex_index_dont_verify_domain);
ACLChecklist *check = (ACLChecklist*)SSL_get_ex_data(ssl, ssl_ex_index_cert_error_check);
X509 *peeked_cert = (X509 *)SSL_get_ex_data(ssl, ssl_ex_index_ssl_peeked_cert);
@@ -283,9 +284,9 @@
debugs(83, 5, "SSL Certificate signature OK: " << buffer);
// Check for domain mismatch only if the current certificate is the peer certificate.
- if (!dont_verify_domain && server && peer_cert.get() == X509_STORE_CTX_get_current_cert(ctx)) {
- if (!Ssl::checkX509ServerValidity(peer_cert.get(), server->c_str())) {
- debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << buffer << " does not match domainname " << server);
+ if (!dont_verify_domain && !extras->serverName.isEmpty() && peer_cert.get() == X509_STORE_CTX_get_current_cert(ctx)) {
+ if (!Ssl::checkX509ServerValidity(peer_cert.get(), extras->serverName.c_str())) {
+ debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << buffer << " does not match domainname " << extras->serverName);
ok = 0;
error_no = SQUID_X509_V_ERR_DOMAIN_MISMATCH;
}
@@ -450,15 +451,6 @@
X509_free(cert);
}
-// "free" function for SBuf
-static void
-ssl_free_SBuf(void *, void *ptr, CRYPTO_EX_DATA *,
- int, long, void *)
-{
- SBuf *buf = static_cast <SBuf *>(ptr);
- delete buf;
-}
-
void
Ssl::Initialize(void)
{
@@ -491,7 +483,6 @@
if (!Ssl::DefaultSignHash)
fatalf("Sign hash '%s' is not supported\n", defName);
- ssl_ex_index_server = SSL_get_ex_new_index(0, (void *) "server", NULL, NULL, ssl_free_SBuf);
ssl_ctx_ex_index_dont_verify_domain = SSL_CTX_get_ex_new_index(0, (void *) "dont_verify_domain", NULL, NULL, NULL);
ssl_ex_index_cert_error_check = SSL_get_ex_new_index(0, (void *) "cert_error_check", NULL, &ssl_dupAclChecklist, &ssl_freeAclChecklist);
ssl_ex_index_ssl_error_detail = SSL_get_ex_new_index(0, (void *) "ssl_error_detail", NULL, NULL, &ssl_free_ErrorDetail);
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
