whytofly
/
qt6-bb10
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

TLS socket: make verification callback lock-free (OpenSSL) 

When our QSslSocketBackendPrivate (OpenSSL backend) was developed,
the ancient versions of OpenSSL did not have an API needed to pass
an application-specific data into verification callback. Thus the
developers resorted to the use of global variables (a list with errors)
and locks. Some of our auto-tests use QNAM and in-process server.
Whenever the client (essentially qhttpthreadeddelegate) and the server
live in different threads, any use of 'https' is dead-lock prone,
which recent events demonstrated and which were previously observed
but not understood properly (rare occasions, not always easy to
reproduce). Now we fix this for good by removing locking.

There are two places (in 5.12) where these locks are needed:

1. Before calling SSL_connect/SSL_accept (handshake) - here
   we reuse the same trick we do in PSK callback ('SSL' has
   an external data set, and it's 'this', meaning an object
   of type QSslSocketBackendPrivate).

2. The static member function 'verify', here we do not have
   'SSL', but we have our temporary 'X509_STORE', to which
   we can directly attach an external data - a pointer to
   a vector to collect verification errors.

Note, this change assumes that OpenSSL Qt is build/linked
against is at least of version 1.0.1 - we set external data
on SSL unconditionally (no version checks).

Fixes: QTBUG-76157
Change-Id: I05c98e77dfd5fb0c2c260fb6c463732facf53ffc
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
Timur Pocheptsov 2019-06-14 11:34:08 +02:00
parent c086fc7341
commit fe6e54fb1f
5 changed files with 70 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

106
src/network/ssl/qsslsocket_openssl.cpp
View File

@ -70,6 +70,10 @@
#include "qwindowscarootfetcher_p.h"
#endif
#if !QT_CONFIG(opensslv11)
#include <openssl/x509_vfy.h>
#endif
#include <QtCore/qdatetime.h>
#include <QtCore/qdebug.h>
#include <QtCore/qdir.h>
@ -253,44 +257,41 @@ QSslErrorEntry QSslErrorEntry::fromStoreContext(X509_STORE_CTX *ctx)
};
}
// ### This list is shared between all threads, and protected by a
// mutex. Investigate using thread local storage instead.
struct QSslErrorList
{
QMutex mutex;
QVector<QSslErrorEntry> errors;
};
Q_GLOBAL_STATIC(QSslErrorList, _q_sslErrorList)
int q_X509Callback(int ok, X509_STORE_CTX *ctx)
{
if (!ok) {
// Store the error and at which depth the error was detected.
_q_sslErrorList()->errors << QSslErrorEntry::fromStoreContext(ctx);
#if !QT_CONFIG(opensslv11)
#ifdef QSSLSOCKET_DEBUG
qCDebug(lcSsl) << "verification error: dumping bad certificate";
qCDebug(lcSsl) << QSslCertificatePrivate::QSslCertificate_from_X509(q_X509_STORE_CTX_get_current_cert(ctx)).toPem();
qCDebug(lcSsl) << "dumping chain";
const auto certs = QSslSocketBackendPrivate::STACKOFX509_to_QSslCertificates(q_X509_STORE_CTX_get_chain(ctx));
for (const QSslCertificate &cert : certs) {
qCDebug(lcSsl) << "Issuer:" << "O=" << cert.issuerInfo(QSslCertificate::Organization)
<< "CN=" << cert.issuerInfo(QSslCertificate::CommonName)
<< "L=" << cert.issuerInfo(QSslCertificate::LocalityName)
<< "OU=" << cert.issuerInfo(QSslCertificate::OrganizationalUnitName)
<< "C=" << cert.issuerInfo(QSslCertificate::CountryName)
<< "ST=" << cert.issuerInfo(QSslCertificate::StateOrProvinceName);
qCDebug(lcSsl) << "Subject:" << "O=" << cert.subjectInfo(QSslCertificate::Organization)
<< "CN=" << cert.subjectInfo(QSslCertificate::CommonName)
<< "L=" << cert.subjectInfo(QSslCertificate::LocalityName)
<< "OU=" << cert.subjectInfo(QSslCertificate::OrganizationalUnitName)
<< "C=" << cert.subjectInfo(QSslCertificate::CountryName)
<< "ST=" << cert.subjectInfo(QSslCertificate::StateOrProvinceName);
qCDebug(lcSsl) << "Valid:" << cert.effectiveDate() << '-' << cert.expiryDate();
using ErrorListPtr = QVector<QSslErrorEntry>*;
ErrorListPtr errors = nullptr;
// Error list is attached to either 'SSL' or 'X509_STORE'.
if (X509_STORE *store = q_X509_STORE_CTX_get0_store(ctx)) { // We try store first:
#if QT_CONFIG(opensslv11)
errors = ErrorListPtr(q_X509_STORE_get_ex_data(store, 0));
#else
errors = ErrorListPtr(q_CRYPTO_get_ex_data(&store->ex_data, 0));
#endif // opensslv11
}
#endif // QSSLSOCKET_DEBUG
#endif // !QT_CONFIG(opensslv11)
if (!errors) {
// Not found on store? Try SSL and its external data then. According to the OpenSSL's
// documentation:
//
// "Whenever a X509_STORE_CTX object is created for the verification of the peers certificate
// during a handshake, a pointer to the SSL object is stored into the X509_STORE_CTX object
// to identify the connection affected. To retrieve this pointer the X509_STORE_CTX_get_ex_data()
// function can be used with the correct index."
if (SSL *ssl = static_cast<SSL *>(q_X509_STORE_CTX_get_ex_data(ctx, q_SSL_get_ex_data_X509_STORE_CTX_idx())))
errors = ErrorListPtr(q_SSL_get_ex_data(ssl, QSslSocketBackendPrivate::s_indexForSSLExtraData + 1));
}
if (!errors) {
qCWarning(lcSsl, "Neither X509_STORE, nor SSL contains error list, handshake failure");
return 0;
}
errors->append(QSslErrorEntry::fromStoreContext(ctx));
}
// Always return OK to allow verification to continue. We handle the
// errors gracefully after collecting all errors, after verification has
@ -445,11 +446,7 @@ bool QSslSocketBackendPrivate::initSslContext()
else
q_SSL_set_accept_state(ssl);
#if OPENSSL_VERSION_NUMBER >= 0x10001000L
// Save a pointer to this object into the SSL structure.
if (QSslSocket::sslLibraryVersionNumber() >= 0x10001000L)
q_SSL_set_ex_data(ssl, s_indexForSSLExtraData, this);
#endif
q_SSL_set_ex_data(ssl, s_indexForSSLExtraData, this);
#if OPENSSL_VERSION_NUMBER >= 0x10001000L && !defined(OPENSSL_NO_PSK)
// Set the client callback for PSK
@ -1002,14 +999,14 @@ bool QSslSocketBackendPrivate::startHandshake()
if (inSetAndEmitError)
return false;
QMutexLocker locker(&_q_sslErrorList()->mutex);
_q_sslErrorList()->errors.clear();
QVector<QSslErrorEntry> lastErrors;
q_SSL_set_ex_data(ssl, s_indexForSSLExtraData + 1, &lastErrors);
int result = (mode == QSslSocket::SslClientMode) ? q_SSL_connect(ssl) : q_SSL_accept(ssl);
q_SSL_set_ex_data(ssl, s_indexForSSLExtraData + 1, nullptr);
const auto &lastErrors = _q_sslErrorList()->errors;
if (!lastErrors.isEmpty())
storePeerCertificates();
for (const auto &currentError : lastErrors) {
for (const auto &currentError : qAsConst(lastErrors)) {
emit q->peerVerifyError(_q_OpenSSL_to_QSslError(currentError.code,
configuration.peerCertificateChain.value(currentError.depth)));
if (q->state() != QAbstractSocket::ConnectedState)
@ -1017,7 +1014,6 @@ bool QSslSocketBackendPrivate::startHandshake()
}
errorList << lastErrors;
locker.unlock();
// Connection aborted during handshake phase.
if (q->state() != QAbstractSocket::ConnectedState)
@ -1420,7 +1416,20 @@ QList<QSslError> QSslSocketBackendPrivate::verify(const QList<QSslCertificate> &
}
}
QMutexLocker sslErrorListMutexLocker(&_q_sslErrorList()->mutex);
QVector<QSslErrorEntry> lastErrors;
#if QT_CONFIG(opensslv11)
if (!q_X509_STORE_set_ex_data(certStore, 0, &lastErrors)) {
qCWarning(lcSsl) << "Unable to attach external data (error list) to a store";
errors << QSslError(QSslError::UnspecifiedError);
return errors;
}
#else
if (!q_CRYPTO_set_ex_data(&certStore->ex_data, 0, &lastErrors)) {
qCWarning(lcSsl) << "Unable to attach external data (error list) to a store";
errors << QSslError(QSslError::UnspecifiedError);
return errors;
}
#endif // opensslv11
// Register a custom callback to get all verification errors.
q_X509_STORE_set_verify_cb(certStore, q_X509Callback);
@ -1470,12 +1479,7 @@ QList<QSslError> QSslSocketBackendPrivate::verify(const QList<QSslCertificate> &
q_OPENSSL_sk_free((OPENSSL_STACK *)intermediates);
// Now process the errors
const auto errorList = std::move(_q_sslErrorList()->errors);
_q_sslErrorList()->errors.clear();
sslErrorListMutexLocker.unlock();
// Translate the errors
if (QSslCertificatePrivate::isBlacklisted(certificateChain[0])) {
QSslError error(QSslError::CertificateBlacklisted, certificateChain[0]);
errors << error;
@ -1489,8 +1493,8 @@ QList<QSslError> QSslSocketBackendPrivate::verify(const QList<QSslCertificate> &
}
// Translate errors from the error list into QSslErrors.
errors.reserve(errors.size() + errorList.size());
for (const auto &error : qAsConst(errorList))
errors.reserve(errors.size() + lastErrors.size());
for (const auto &error : qAsConst(lastErrors))
errors << _q_OpenSSL_to_QSslError(error.code, certificateChain.value(error.depth));
q_X509_STORE_free(certStore);

2
src/network/ssl/qsslsocket_openssl11_symbols_p.h
View File

@ -105,6 +105,8 @@ ASN1_TIME *q_X509_getm_notAfter(X509 *a);
long q_X509_get_version(X509 *a);
EVP_PKEY *q_X509_get_pubkey(X509 *a);
void q_X509_STORE_set_verify_cb(X509_STORE *ctx, X509_STORE_CTX_verify_cb verify_cb);
int q_X509_STORE_set_ex_data(X509_STORE *ctx, int idx, void *data);
void *q_X509_STORE_get_ex_data(X509_STORE *r, int idx);
STACK_OF(X509) *q_X509_STORE_CTX_get0_chain(X509_STORE_CTX *ctx);
void q_DH_get0_pqg(const DH *dh, const BIGNUM **p, const BIGNUM **q, const BIGNUM **g);
int q_DH_bits(DH *dh);

10
src/network/ssl/qsslsocket_openssl_symbols.cpp
View File

@ -178,6 +178,8 @@ DEFINEFUNC(ASN1_TIME *, X509_getm_notAfter, X509 *a, a, return nullptr, return)
DEFINEFUNC(long, X509_get_version, X509 *a, a, return -1, return)
DEFINEFUNC(EVP_PKEY *, X509_get_pubkey, X509 *a, a, return nullptr, return)
DEFINEFUNC2(void, X509_STORE_set_verify_cb, X509_STORE *a, a, X509_STORE_CTX_verify_cb verify_cb, verify_cb, return, DUMMYARG)
DEFINEFUNC3(int, X509_STORE_set_ex_data, X509_STORE *a, a, int idx, idx, void *data, data, return 0, return)
DEFINEFUNC2(void *, X509_STORE_get_ex_data, X509_STORE *r, r, int idx, idx, return nullptr, return)
DEFINEFUNC(STACK_OF(X509) *, X509_STORE_CTX_get0_chain, X509_STORE_CTX *a, a, return nullptr, return)
DEFINEFUNC3(void, CRYPTO_free, void *str, str, const char *file, file, int line, line, return, DUMMYARG)
DEFINEFUNC(long, OpenSSL_version_num, void, DUMMYARG, return 0, return)
@ -223,6 +225,8 @@ DEFINEFUNC(int, CRYPTO_num_locks, DUMMYARG, DUMMYARG, return 0, return)
DEFINEFUNC(void, CRYPTO_set_locking_callback, void (*a)(int, int, const char *, int), a, return, DUMMYARG)
DEFINEFUNC(void, CRYPTO_set_id_callback, unsigned long (*a)(), a, return, DUMMYARG)
DEFINEFUNC(void, CRYPTO_free, void *a, a, return, DUMMYARG)
DEFINEFUNC3(int, CRYPTO_set_ex_data, CRYPTO_EX_DATA *ad, ad, int idx, idx, void *val, val, return 0, return)
DEFINEFUNC2(void *, CRYPTO_get_ex_data, const CRYPTO_EX_DATA *ad, ad, int idx, idx, return nullptr, return)
DEFINEFUNC(unsigned long, ERR_peek_last_error, DUMMYARG, DUMMYARG, return 0, return)
DEFINEFUNC(void, ERR_free_strings, void, DUMMYARG, return, DUMMYARG)
DEFINEFUNC(void, EVP_CIPHER_CTX_cleanup, EVP_CIPHER_CTX *a, a, return, DUMMYARG)
@ -533,6 +537,7 @@ DEFINEFUNC2(int, X509_STORE_CTX_set_purpose, X509_STORE_CTX *a, a, int b, b, ret
DEFINEFUNC(int, X509_STORE_CTX_get_error, X509_STORE_CTX *a, a, return -1, return)
DEFINEFUNC(int, X509_STORE_CTX_get_error_depth, X509_STORE_CTX *a, a, return -1, return)
DEFINEFUNC(X509 *, X509_STORE_CTX_get_current_cert, X509_STORE_CTX *a, a, return nullptr, return)
DEFINEFUNC(X509_STORE *, X509_STORE_CTX_get0_store, X509_STORE_CTX *ctx, ctx, return nullptr, return)
DEFINEFUNC(X509_STORE_CTX *, X509_STORE_CTX_new, DUMMYARG, DUMMYARG, return nullptr, return)
DEFINEFUNC2(void *, X509_STORE_CTX_get_ex_data, X509_STORE_CTX *ctx, ctx, int idx, idx, return nullptr, return)
DEFINEFUNC(int, SSL_get_ex_data_X509_STORE_CTX_idx, DUMMYARG, DUMMYARG, return -1, return)
@ -998,6 +1003,8 @@ bool q_resolveOpenSslSymbols()
RESOLVEFUNC(X509_get_version)
RESOLVEFUNC(X509_get_pubkey)
RESOLVEFUNC(X509_STORE_set_verify_cb)
RESOLVEFUNC(X509_STORE_set_ex_data)
RESOLVEFUNC(X509_STORE_get_ex_data)
RESOLVEFUNC(CRYPTO_free)
RESOLVEFUNC(OpenSSL_version_num)
RESOLVEFUNC(OpenSSL_version)
@ -1046,6 +1053,8 @@ bool q_resolveOpenSslSymbols()
RESOLVEFUNC(CRYPTO_num_locks)
RESOLVEFUNC(CRYPTO_set_id_callback)
RESOLVEFUNC(CRYPTO_set_locking_callback)
RESOLVEFUNC(CRYPTO_set_ex_data)
RESOLVEFUNC(CRYPTO_get_ex_data)
RESOLVEFUNC(ERR_peek_last_error)
RESOLVEFUNC(ERR_free_strings)
RESOLVEFUNC(EVP_CIPHER_CTX_cleanup)
@ -1305,6 +1314,7 @@ bool q_resolveOpenSslSymbols()
RESOLVEFUNC(X509_STORE_CTX_get_error)
RESOLVEFUNC(X509_STORE_CTX_get_error_depth)
RESOLVEFUNC(X509_STORE_CTX_get_current_cert)
RESOLVEFUNC(X509_STORE_CTX_get0_store)
RESOLVEFUNC(X509_cmp)
RESOLVEFUNC(X509_STORE_CTX_get_ex_data)

1
src/network/ssl/qsslsocket_openssl_symbols_p.h
View File

@ -450,6 +450,7 @@ int q_X509_STORE_CTX_set_purpose(X509_STORE_CTX *ctx, int purpose);
int q_X509_STORE_CTX_get_error(X509_STORE_CTX *ctx);
int q_X509_STORE_CTX_get_error_depth(X509_STORE_CTX *ctx);
X509 *q_X509_STORE_CTX_get_current_cert(X509_STORE_CTX *ctx);
X509_STORE *q_X509_STORE_CTX_get0_store(X509_STORE_CTX *ctx);
// Diffie-Hellman support
DH *q_DH_new();

2
src/network/ssl/qsslsocket_opensslpre11_symbols_p.h
View File

@ -84,6 +84,8 @@ int q_CRYPTO_num_locks();
void q_CRYPTO_set_locking_callback(void (*a)(int, int, const char *, int));
void q_CRYPTO_set_id_callback(unsigned long (*a)());
void q_CRYPTO_free(void *a);
int q_CRYPTO_set_ex_data(CRYPTO_EX_DATA *ad, int idx, void *val);
void *q_CRYPTO_get_ex_data(const CRYPTO_EX_DATA *ad, int idx);
unsigned long q_ERR_peek_last_error();
void q_ERR_free_strings();
void q_EVP_CIPHER_CTX_cleanup(EVP_CIPHER_CTX *a);