From: Amos Jeffries Date: Sat, 3 Dec 2016 15:08:28 +0000 (+1300) Subject: Implement GnuTLS session creation X-Git-Tag: M-staged-PR71~284^2~32 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=9c8549cf83d7380e38992ce1aa06671032d4f31e;p=thirdparty%2Fsquid.git Implement GnuTLS session creation Convert SessionPointer to use std::shared_ptr so that gnuts_deinit() can be run at the correct time. Also, improve debugging in PeerConnector and BlindPeerConnector negotiation methods. Also, add error handling for TLS connection handshake failures. --- diff --git a/src/security/BlindPeerConnector.cc b/src/security/BlindPeerConnector.cc index 1a1a3a3bfc..b27c772735 100644 --- a/src/security/BlindPeerConnector.cc +++ b/src/security/BlindPeerConnector.cc @@ -32,8 +32,10 @@ Security::BlindPeerConnector::getTlsContext() bool Security::BlindPeerConnector::initialize(Security::SessionPointer &serverSession) { - if (!Security::PeerConnector::initialize(serverSession)) + if (!Security::PeerConnector::initialize(serverSession)) { + debugs(83, 5, "Security::PeerConnector::initialize failed"); return false; + } if (const CachePeer *peer = serverConnection()->getPeer()) { assert(peer); @@ -52,6 +54,8 @@ Security::BlindPeerConnector::initialize(Security::SessionPointer &serverSession SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, (void*)hostName); #endif } + + debugs(83, 5, "success"); return true; } @@ -59,6 +63,7 @@ void Security::BlindPeerConnector::noteNegotiationDone(ErrorState *error) { if (error) { + debugs(83, 5, "error=" << (void*)error); // XXX: forward.cc calls peerConnectSucceeded() after an OK TCP connect but // we call peerConnectFailed() if SSL failed afterwards. Is that OK? // It is not clear whether we should call peerConnectSucceeded/Failed() diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index 3677c5d7b8..3f5a256d61 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -58,6 +58,7 @@ void Security::PeerConnector::start() { AsyncJob::start(); + debugs(83, 5, "this=" << (void*)this); Security::SessionPointer tmp; if (prepareSocket() && initialize(tmp)) @@ -76,6 +77,7 @@ Security::PeerConnector::commCloseHandler(const CommCloseCbParams ¶ms) void Security::PeerConnector::connectionClosed(const char *reason) { + debugs(83, 5, reason << " socket closed/closing. this=" << (void*)this); mustStop(reason); callback = NULL; } @@ -83,16 +85,18 @@ Security::PeerConnector::connectionClosed(const char *reason) bool Security::PeerConnector::prepareSocket() { - const int fd = serverConnection()->fd; - if (!Comm::IsConnOpen(serverConn) || fd_table[serverConn->fd].closing()) { + debugs(83, 5, serverConnection() << ", this=" << (void*)this); + if (!Comm::IsConnOpen(serverConnection()) || fd_table[serverConnection()->fd].closing()) { connectionClosed("Security::PeerConnector::prepareSocket"); return false; } + debugs(83, 5, serverConnection()); + // watch for external connection closures typedef CommCbMemFunT Dialer; closeHandler = JobCallback(9, 5, Dialer, this, Security::PeerConnector::commCloseHandler); - comm_add_close_handler(fd, closeHandler); + comm_add_close_handler(serverConnection()->fd, closeHandler); return true; } @@ -100,6 +104,7 @@ bool Security::PeerConnector::initialize(Security::SessionPointer &serverSession) { Security::ContextPointer ctx(getTlsContext()); + debugs(83, 5, serverConnection() << ", ctx=" << (void*)ctx.get()); if (!ctx || !Security::CreateClientSession(ctx, serverConnection(), "server https start")) { const auto xerrno = errno; @@ -115,6 +120,7 @@ Security::PeerConnector::initialize(Security::SessionPointer &serverSession) // A TLS/SSL session has now been created for the connection and stored in fd_table serverSession = fd_table[serverConnection()->fd].ssl; + debugs(83, 5, serverConnection() << ", session=" << (void*)serverSession.get()); #if USE_OPENSSL // If CertValidation Helper used do not lookup checklist for errors, @@ -177,13 +183,19 @@ Security::PeerConnector::negotiate() return; #if USE_OPENSSL - const int result = SSL_connect(fd_table[fd].ssl.get()); + auto session = fd_table[fd].ssl.get(); + debugs(83, 5, "SSL_connect session=" << (void*)session); + const int result = SSL_connect(session); + if (result <= 0) { #elif USE_GNUTLS - const int result = gnutls_handshake(fd_table[fd].ssl.get()); + auto session = fd_table[fd].ssl.get(); + debugs(83, 5, "gnutls_handshake session=" << (void*)session); + const int result = gnutls_handshake(session); + if (result != GNUTLS_E_SUCCESS) { + debugs(83, 5, "gnutls_handshake session=" << (void*)session << ", result=" << result); #else - const int result = -1; + if (const int result = -1) { #endif - if (result <= 0) { handleNegotiateError(result); return; // we might be gone by now } @@ -205,11 +217,11 @@ Security::PeerConnector::sslFinalized() Security::SessionPointer session(fd_table[fd].ssl); Ssl::CertValidationRequest validationRequest; - // WARNING: Currently we do not use any locking for any of the - // members of the Ssl::CertValidationRequest class. In this code the + // WARNING: Currently we do not use any locking for 'errors' member + // of the Ssl::CertValidationRequest class. In this code the // Ssl::CertValidationRequest object used only to pass data to // Ssl::CertValidationHelper::submit method. - validationRequest.ssl = session.get(); + validationRequest.ssl = session; if (SBuf *dName = (SBuf *)SSL_get_ex_data(session.get(), ssl_ex_index_server)) validationRequest.domainName = dName->c_str(); if (Security::CertErrors *errs = static_cast(SSL_get_ex_data(session.get(), ssl_ex_index_ssl_errors))) @@ -360,10 +372,11 @@ Security::PeerConnector::NegotiateSsl(int, void *data) void Security::PeerConnector::handleNegotiateError(const int ret) { -#if USE_OPENSSL const int fd = serverConnection()->fd; - unsigned long ssl_lib_error = SSL_ERROR_NONE; - Security::SessionPointer session(fd_table[fd].ssl); + const Security::SessionPointer session(fd_table[fd].ssl); + unsigned long ssl_lib_error = ret; + +#if USE_OPENSSL const int ssl_error = SSL_get_error(session.get(), ret); switch (ssl_error) { @@ -382,20 +395,42 @@ Security::PeerConnector::handleNegotiateError(const int ret) break; default: // no special error handling for all other errors + ssl_lib_error = SSL_ERROR_NONE; break; } - // Log connection details, if any - recordNegotiationDetails(); - noteNegotiationError(ret, ssl_error, ssl_lib_error); - #elif USE_GNUTLS - // TODO: handle specific errors individually like above - if (gnutls_error_is_fatal(ret) == 0) { - noteWantRead(); + const int ssl_error = ret; + + switch (ret) { + case GNUTLS_E_WARNING_ALERT_RECEIVED: { + auto alert = gnutls_alert_get(session.get()); + debugs(83, DBG_IMPORTANT, "TLS ALERT: " << gnutls_alert_get_name(alert)); + } + // drop through to next case + + case GNUTLS_E_AGAIN: + case GNUTLS_E_INTERRUPTED: + if (gnutls_record_get_direction(session.get()) == 0) + noteWantWrite(); + else + noteWantRead(); return; + + default: + // no special error handling for all other errors + break; } + +#else + // this avoids unused variable compiler warnings. + Must(!session); + const int ssl_error = ret; #endif + + // Log connection details, if any + recordNegotiationDetails(); + noteNegotiationError(ret, ssl_error, ssl_lib_error); } void @@ -439,19 +474,20 @@ Security::PeerConnector::noteWantWrite() void Security::PeerConnector::noteNegotiationError(const int ret, const int ssl_error, const int ssl_lib_error) { -#if USE_OPENSSL // not used unless OpenSSL enabled. #if defined(EPROTO) int sysErrNo = EPROTO; #else int sysErrNo = EACCES; #endif +#if USE_OPENSSL // store/report errno when ssl_error is SSL_ERROR_SYSCALL, ssl_lib_error is 0, and ret is -1 if (ssl_error == SSL_ERROR_SYSCALL && ret == -1 && ssl_lib_error == 0) sysErrNo = errno; +#endif const int fd = serverConnection()->fd; - debugs(83, DBG_IMPORTANT, "Error negotiating SSL on FD " << fd << + debugs(83, DBG_IMPORTANT, "ERROR: negotiating TLS on FD " << fd << ": " << Security::ErrorString(ssl_lib_error) << " (" << ssl_error << "/" << ret << "/" << errno << ")"); @@ -462,6 +498,7 @@ Security::PeerConnector::noteNegotiationError(const int ret, const int ssl_error anErr = new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scServiceUnavailable, NULL); anErr->xerrno = sysErrNo; +#if USE_OPENSSL Security::SessionPointer session(fd_table[fd].ssl); Ssl::ErrorDetail *errFromFailure = static_cast(SSL_get_ex_data(session.get(), ssl_ex_index_ssl_error_detail)); if (errFromFailure != NULL) { @@ -478,10 +515,10 @@ Security::PeerConnector::noteNegotiationError(const int ret, const int ssl_error if (ssl_lib_error != SSL_ERROR_NONE) anErr->detail->setLibError(ssl_lib_error); +#endif noteNegotiationDone(anErr); bail(anErr); -#endif } void @@ -500,11 +537,20 @@ Security::PeerConnector::bail(ErrorState *error) // the recepient before the fd-closure notification), but we would rather // minimize the number of fd-closure notifications and let the recepient // manage the TCP state of the connection. + +#if USE_GNUTLS + // but we do need to release the bad TLS related details in fd_table + // ... or GnuTLS will SEGFAULT. + const int fd = serverConnection()->fd; + Security::SessionClose(fd_table[fd].ssl, fd); +#endif } void Security::PeerConnector::callBack() { + debugs(83, 5, "TLS setup ended for " << serverConnection()); + AsyncCall::Pointer cb = callback; // Do this now so that if we throw below, swanSong() assert that we _tried_ // to call back holds. diff --git a/src/security/Session.cc b/src/security/Session.cc index 3dcf102b26..0d66a6361c 100644 --- a/src/security/Session.cc +++ b/src/security/Session.cc @@ -41,6 +41,7 @@ tls_read_method(int fd, char *buf, int len) int i = gnutls_record_recv(session, buf, len); #endif if (i > 0) { + debugs(83, 8, "TLS FD " << fd << " session=" << (void*)session << " " << i << " bytes"); (void)VALGRIND_MAKE_MEM_DEFINED(buf, i); } @@ -75,6 +76,9 @@ tls_write_method(int fd, const char *buf, int len) int i = gnutls_record_send(session, buf, len); #endif + if (i > 0) { + debugs(83, 8, "TLS FD " << fd << " session=" << (void*)session << " " << i << " bytes"); + } return i; } #endif @@ -92,7 +96,11 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer const char *errAction = "with no TLS/SSL library"; int errCode = 0; #if USE_OPENSSL - Security::SessionPointer session(SSL_new(ctx.get())); + Security::SessionPointer session(SSL_new(ctx.get()), [](SSL *p) { + debugs(83, 5, "SSL_free session=" << (void*)p); + SSL_free(p); + }); + debugs(83, 5, "SSL_new session=" << (void*)session.get()); if (!session) { errCode = ERR_get_error(); errAction = "failed to allocate handle"; @@ -100,7 +108,11 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer #elif USE_GNUTLS gnutls_session_t tmp; errCode = gnutls_init(&tmp, static_cast(type)); - Security::SessionPointer session(tmp); + Security::SessionPointer session(tmp, [](gnutls_session_t p) { + debugs(83, 5, "gnutls_deinit session=" << (void*)p); + gnutls_deinit(p); } + }); + debugs(83, 5, "gnutls_init session=" << (void*)session.get()); if (errCode != GNUTLS_E_SUCCESS) { session.reset(); errAction = "failed to initialize session"; @@ -119,6 +131,7 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer if (errCode == GNUTLS_E_SUCCESS) { #endif + debugs(83, 5, "link FD " << fd << " to TLS session=" << (void*)session.get()); fd_table[fd].ssl = session; fd_table[fd].read_method = &tls_read_method; fd_table[fd].write_method = &tls_write_method; @@ -153,16 +166,27 @@ Security::CreateServerSession(const Security::ContextPointer &ctx, const Comm::C } void -Security::SessionClose(const Security::SessionPointer &s) +Security::SessionClose(const Security::SessionPointer &s, const int fdOnError) { debugs(83, 5, "session=" << (void*)s.get()); - if (s) { + if (s && fdOnError == -1) { #if USE_OPENSSL SSL_shutdown(s.get()); #elif USE_GNUTLS gnutls_bye(s.get(), GNUTLS_SHUT_RDWR); #endif } + +#if USE_GNUTLS + // XXX: should probably be done for OpenSSL too, but that needs testing. + if (fdOnError != -1) { + debugs(83, 5, "unlink FD " << fdOnError << " from TLS session=" << (void*)fd_table[fdOnError].ssl.get()); + fd_table[fdOnError].ssl.reset(); + fd_table[fdOnError].read_method = &default_read_method; + fd_table[fdOnError].write_method = &default_write_method; + fd_note(fdOnError, "TLS error"); + } +#endif } bool diff --git a/src/security/Session.h b/src/security/Session.h index 182ca8da57..6dbb6db9c5 100644 --- a/src/security/Session.h +++ b/src/security/Session.h @@ -38,36 +38,28 @@ bool CreateClientSession(const Security::ContextPointer &, const Comm::Connectio bool CreateServerSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, const char *squidCtx); #if USE_OPENSSL -CtoCpp1(SSL_free, SSL *); -#if defined(CRYPTO_LOCK_SSL) // OpenSSL 1.0 -inline int SSL_up_ref(SSL *t) {if (t) CRYPTO_add(&t->references, 1, CRYPTO_LOCK_SSL); return 0;} -#endif -typedef Security::LockingPointer > SessionPointer; +typedef std::shared_ptr SessionPointer; typedef std::unique_ptr> SessionStatePointer; #elif USE_GNUTLS -// Locks can be implemented attaching locks counter to gnutls_session_t -// objects using the gnutls_session_set_ptr()/gnutls_session_get_ptr () -// library functions -CtoCpp1(gnutls_deinit, gnutls_session_t); -typedef Security::LockingPointer SessionPointer; +typedef std::shared_ptr SessionPointer; // wrapper function to get around gnutls_free being a typedef inline void squid_gnutls_free(void *d) {gnutls_free(d);} typedef std::unique_ptr> SessionStatePointer; #else -// use void* so we can check against NULL -CtoCpp1(xfree, void *); -typedef Security::LockingPointer SessionPointer; +typedef std::shared_ptr SessionPointer; typedef std::unique_ptr SessionStatePointer; #endif -/// close an active TLS session -void SessionClose(const Security::SessionPointer &); +/// close an active TLS session. +/// set fdOnError to the connection FD when the session is being closed +/// due to an encryption error, otherwise omit. +void SessionClose(const Security::SessionPointer &, int fdOnError = -1); /// whether the session is a resumed one bool SessionIsResumed(const Security::SessionPointer &);