QSslSocketBackendPrivate - avoid recursion while handing errors
The logic seems to be simple - if client code on error signal tries to close TLS socket and this socket has buffered data, it calls 'flush' and 'transmit' or even 'startHandshake' as a result, which in turn will set and emit error again. To auto- test this, we initiate a handshake with pre-shared key hint on a server side and both client/server sockets incorrectly configured (missing PSK signals). We also do early write into the client socket to make sure it has some data buffered by the moment we call 'close'. Task-number: QTBUG-68089 Task-number: QTBUG-56476 Change-Id: I6ba6435bd572ad85d9209c4c81774a397081b34f Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
This commit is contained in:
parent
e40f23f098
commit
86632bd377
@ -81,6 +81,7 @@
|
||||
#include <QtCore/qthread.h>
|
||||
#include <QtCore/qurl.h>
|
||||
#include <QtCore/qvarlengtharray.h>
|
||||
#include <QtCore/qscopedvaluerollback.h>
|
||||
|
||||
#include <string.h>
|
||||
|
||||
@ -644,6 +645,11 @@ void QSslSocketBackendPrivate::transmit()
|
||||
{
|
||||
Q_Q(QSslSocket);
|
||||
|
||||
using ScopedBool = QScopedValueRollback<bool>;
|
||||
|
||||
if (inSetAndEmitError)
|
||||
return;
|
||||
|
||||
// If we don't have any SSL context, don't bother transmitting.
|
||||
if (!ssl)
|
||||
return;
|
||||
@ -671,6 +677,7 @@ void QSslSocketBackendPrivate::transmit()
|
||||
break;
|
||||
} else {
|
||||
// ### Better error handling.
|
||||
const ScopedBool bg(inSetAndEmitError, true);
|
||||
setErrorAndEmit(QAbstractSocket::SslInternalError,
|
||||
QSslSocket::tr("Unable to write data: %1").arg(
|
||||
getErrorsFromOpenSsl()));
|
||||
@ -716,6 +723,7 @@ void QSslSocketBackendPrivate::transmit()
|
||||
#endif
|
||||
if (actualWritten < 0) {
|
||||
//plain socket write fails if it was in the pending close state.
|
||||
const ScopedBool bg(inSetAndEmitError, true);
|
||||
setErrorAndEmit(plainSocket->error(), plainSocket->errorString());
|
||||
return;
|
||||
}
|
||||
@ -741,6 +749,7 @@ void QSslSocketBackendPrivate::transmit()
|
||||
plainSocket->skip(writtenToBio);
|
||||
} else {
|
||||
// ### Better error handling.
|
||||
const ScopedBool bg(inSetAndEmitError, true);
|
||||
setErrorAndEmit(QAbstractSocket::SslInternalError,
|
||||
QSslSocket::tr("Unable to decrypt data: %1").arg(
|
||||
getErrorsFromOpenSsl()));
|
||||
@ -818,15 +827,21 @@ void QSslSocketBackendPrivate::transmit()
|
||||
qCDebug(lcSsl) << "QSslSocketBackendPrivate::transmit: remote disconnect";
|
||||
#endif
|
||||
shutdown = true; // the other side shut down, make sure we do not send shutdown ourselves
|
||||
setErrorAndEmit(QAbstractSocket::RemoteHostClosedError,
|
||||
QSslSocket::tr("The TLS/SSL connection has been closed"));
|
||||
{
|
||||
const ScopedBool bg(inSetAndEmitError, true);
|
||||
setErrorAndEmit(QAbstractSocket::RemoteHostClosedError,
|
||||
QSslSocket::tr("The TLS/SSL connection has been closed"));
|
||||
}
|
||||
return;
|
||||
case SSL_ERROR_SYSCALL: // some IO error
|
||||
case SSL_ERROR_SSL: // error in the SSL library
|
||||
// we do not know exactly what the error is, nor whether we can recover from it,
|
||||
// so just return to prevent an endless loop in the outer "while" statement
|
||||
setErrorAndEmit(QAbstractSocket::SslInternalError,
|
||||
QSslSocket::tr("Error while reading: %1").arg(getErrorsFromOpenSsl()));
|
||||
{
|
||||
const ScopedBool bg(inSetAndEmitError, true);
|
||||
setErrorAndEmit(QAbstractSocket::SslInternalError,
|
||||
QSslSocket::tr("Error while reading: %1").arg(getErrorsFromOpenSsl()));
|
||||
}
|
||||
return;
|
||||
default:
|
||||
// SSL_ERROR_WANT_CONNECT, SSL_ERROR_WANT_ACCEPT: can only happen with a
|
||||
@ -834,8 +849,11 @@ void QSslSocketBackendPrivate::transmit()
|
||||
// SSL_ERROR_WANT_X509_LOOKUP: can only happen with a
|
||||
// SSL_CTX_set_client_cert_cb(), which we do not call.
|
||||
// So this default case should never be triggered.
|
||||
setErrorAndEmit(QAbstractSocket::SslInternalError,
|
||||
QSslSocket::tr("Error while reading: %1").arg(getErrorsFromOpenSsl()));
|
||||
{
|
||||
const ScopedBool bg(inSetAndEmitError, true);
|
||||
setErrorAndEmit(QAbstractSocket::SslInternalError,
|
||||
QSslSocket::tr("Error while reading: %1").arg(getErrorsFromOpenSsl()));
|
||||
}
|
||||
break;
|
||||
}
|
||||
} while (ssl && readBytes > 0);
|
||||
@ -903,6 +921,12 @@ bool QSslSocketBackendPrivate::startHandshake()
|
||||
|
||||
// Check if the connection has been established. Get all errors from the
|
||||
// verification stage.
|
||||
|
||||
using ScopedBool = QScopedValueRollback<bool>;
|
||||
|
||||
if (inSetAndEmitError)
|
||||
return false;
|
||||
|
||||
QMutexLocker locker(&_q_sslErrorList()->mutex);
|
||||
_q_sslErrorList()->errors.clear();
|
||||
int result = (mode == QSslSocket::SslClientMode) ? q_SSL_connect(ssl) : q_SSL_accept(ssl);
|
||||
@ -936,7 +960,10 @@ bool QSslSocketBackendPrivate::startHandshake()
|
||||
#ifdef QSSLSOCKET_DEBUG
|
||||
qCDebug(lcSsl) << "QSslSocketBackendPrivate::startHandshake: error!" << errorString;
|
||||
#endif
|
||||
setErrorAndEmit(QAbstractSocket::SslHandshakeFailedError, errorString);
|
||||
{
|
||||
const ScopedBool bg(inSetAndEmitError, true);
|
||||
setErrorAndEmit(QAbstractSocket::SslHandshakeFailedError, errorString);
|
||||
}
|
||||
q->abort();
|
||||
}
|
||||
return false;
|
||||
|
@ -131,6 +131,8 @@ public:
|
||||
static int s_indexForSSLExtraData; // index used in SSL_get_ex_data to get the matching QSslSocketBackendPrivate
|
||||
#endif
|
||||
|
||||
bool inSetAndEmitError = false;
|
||||
|
||||
// Platform specific functions
|
||||
void startClientEncryption() override;
|
||||
void startServerEncryption() override;
|
||||
|
@ -202,6 +202,9 @@ private slots:
|
||||
void verifyDepth();
|
||||
void disconnectFromHostWhenConnecting();
|
||||
void disconnectFromHostWhenConnected();
|
||||
#ifndef QT_NO_OPENSSL
|
||||
void closeWhileEmittingSocketError();
|
||||
#endif
|
||||
void resetProxy();
|
||||
void ignoreSslErrorsList_data();
|
||||
void ignoreSslErrorsList();
|
||||
@ -2336,6 +2339,66 @@ void tst_QSslSocket::disconnectFromHostWhenConnected()
|
||||
QCOMPARE(socket->bytesToWrite(), qint64(0));
|
||||
}
|
||||
|
||||
#ifndef QT_NO_OPENSSL
|
||||
|
||||
class BrokenPskHandshake : public QTcpServer
|
||||
{
|
||||
public:
|
||||
void socketError(QAbstractSocket::SocketError error)
|
||||
{
|
||||
Q_UNUSED(error);
|
||||
QSslSocket *clientSocket = qobject_cast<QSslSocket *>(sender());
|
||||
Q_ASSERT(clientSocket);
|
||||
clientSocket->close();
|
||||
QTestEventLoop::instance().exitLoop();
|
||||
}
|
||||
private:
|
||||
|
||||
void incomingConnection(qintptr handle) override
|
||||
{
|
||||
if (!socket.setSocketDescriptor(handle))
|
||||
return;
|
||||
|
||||
QSslConfiguration serverConfig(QSslConfiguration::defaultConfiguration());
|
||||
serverConfig.setPreSharedKeyIdentityHint("abcdefghijklmnop");
|
||||
socket.setSslConfiguration(serverConfig);
|
||||
socket.startServerEncryption();
|
||||
}
|
||||
|
||||
QSslSocket socket;
|
||||
};
|
||||
|
||||
void tst_QSslSocket::closeWhileEmittingSocketError()
|
||||
{
|
||||
QFETCH_GLOBAL(bool, setProxy);
|
||||
if (setProxy)
|
||||
return;
|
||||
|
||||
BrokenPskHandshake handshake;
|
||||
if (!handshake.listen())
|
||||
QSKIP("failed to start TLS server");
|
||||
|
||||
QSslSocket clientSocket;
|
||||
QSslConfiguration clientConfig(QSslConfiguration::defaultConfiguration());
|
||||
clientConfig.setPeerVerifyMode(QSslSocket::VerifyNone);
|
||||
clientSocket.setSslConfiguration(clientConfig);
|
||||
|
||||
QSignalSpy socketErrorSpy(&clientSocket, SIGNAL(error(QAbstractSocket::SocketError)));
|
||||
void (QSslSocket::*errorSignal)(QAbstractSocket::SocketError) = &QSslSocket::error;
|
||||
connect(&clientSocket, errorSignal, &handshake, &BrokenPskHandshake::socketError);
|
||||
|
||||
clientSocket.connectToHostEncrypted(QStringLiteral("127.0.0.1"), handshake.serverPort());
|
||||
// Make sure we have some data buffered so that close will try to flush:
|
||||
clientSocket.write(QByteArray(1000000, Qt::Uninitialized));
|
||||
|
||||
QTestEventLoop::instance().enterLoopMSecs(1000);
|
||||
QVERIFY(!QTestEventLoop::instance().timeout());
|
||||
|
||||
QCOMPARE(socketErrorSpy.count(), 1);
|
||||
}
|
||||
|
||||
#endif // QT_NO_OPENSSL
|
||||
|
||||
void tst_QSslSocket::resetProxy()
|
||||
{
|
||||
#ifndef QT_NO_NETWORKPROXY
|
||||
|
Loading…
x
Reference in New Issue
Block a user