From 8e1e275f8f45fab9d035e86c9caba1f03db43373 Mon Sep 17 00:00:00 2001 From: Timur Pocheptsov Date: Thu, 12 Jul 2018 16:31:20 +0200 Subject: [PATCH] SecureTransport - implement renegotiations After a handshake was completed, TLS socket is in 'connectionEncrypted' state. So on a read notification, in 'transmit', we call 'SSLRead' to read supposedly encrypted application data or TLS internal messages. In case SSLRead finds either ClientHello or HelloRequest from a server, it attempts in a rather sneaky manner to renegotiate. And as it happens here and there with SecureTransport, SSLRead fails and the work is only half-done, since we have kSSLSessionOptionBreakOnServerAuth and kSSLSessionOptionBreakOnCertRequested options set to 'true'. We end up with completely unexpected errors like errSSLClientCertRequested or errSSLPeerAuthCompleted (yes, this is so normal and totally expected for 'SSLRead' function to verify certificates and WRITE messages, no need to document this at all!). If SecureTransport is sneaky, so can be us: - in a read callback SecureTransport is probing the type of record and we can notice a sudden session state change - it goes from kSSLConnected (which is set upon handshake completion) to kSSLHandshake (which means a (re)handshake is ongoing); - if this is the case - we lie to SecureTransport about the amount of data available (0 bytes), set 'renegotiating' to 'true', return errSSLWouldBlock; - in 'transmit', if SSLRead returns errSSLWouldBlock and 'renegotiating' was set, we call 'startHandshake' until isHandshakeComplete() == true or some error encountered. [ChangeLog][QtNetwork][QSslSocket] Implement renegotiation for SecureTransport backend Task-number: QTBUG-69420 Change-Id: Iaab1336aa3abf3f6ac94b358f3142d2738a18ee9 Reviewed-by: Edward Welbourne --- src/network/ssl/qsslsocket_mac.cpp | 64 +++++++++++++++++++++++++----- src/network/ssl/qsslsocket_mac_p.h | 7 ++++ 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/src/network/ssl/qsslsocket_mac.cpp b/src/network/ssl/qsslsocket_mac.cpp index 406748d9f50..8aa01deee7b 100644 --- a/src/network/ssl/qsslsocket_mac.cpp +++ b/src/network/ssl/qsslsocket_mac.cpp @@ -48,6 +48,7 @@ #include #include +#include #include #include #include @@ -231,12 +232,35 @@ static const uint8_t dhparam[] = "\x90\x0b\x35\x64\xff\xd9\xe3\xac\xf2\xf2\xeb\x3a\x63\x02\x01\x02"; #endif -static OSStatus _q_SSLRead(QTcpSocket *plainSocket, char *data, size_t *dataLength) +OSStatus QSslSocketBackendPrivate::ReadCallback(QSslSocketBackendPrivate *socket, + char *data, size_t *dataLength) { - Q_ASSERT(plainSocket); + Q_ASSERT(socket); Q_ASSERT(data); Q_ASSERT(dataLength); + QTcpSocket *plainSocket = socket->plainSocket; + Q_ASSERT(plainSocket); + + if (socket->isHandshakeComplete()) { + // Check if it's a renegotiation attempt, when the handshake is complete, the + // session state is 'kSSLConnected': + SSLSessionState currentState = kSSLConnected; + const OSStatus result = SSLGetSessionState(socket->context, ¤tState); + if (result != noErr) { + *dataLength = 0; + return result; + } + + if (currentState == kSSLHandshake) { + // Renegotiation detected, don't allow read more yet - 'transmit' + // will notice this and will call 'startHandshake': + *dataLength = 0; + socket->renegotiating = true; + return errSSLWouldBlock; + } + } + const qint64 bytes = plainSocket->read(data, *dataLength); #ifdef QSSLSOCKET_DEBUG qCDebug(lcSsl) << plainSocket << "read" << bytes; @@ -252,12 +276,16 @@ static OSStatus _q_SSLRead(QTcpSocket *plainSocket, char *data, size_t *dataLeng return err; } -static OSStatus _q_SSLWrite(QTcpSocket *plainSocket, const char *data, size_t *dataLength) +OSStatus QSslSocketBackendPrivate::WriteCallback(QSslSocketBackendPrivate *socket, + const char *data, size_t *dataLength) { - Q_ASSERT(plainSocket); + Q_ASSERT(socket); Q_ASSERT(data); Q_ASSERT(dataLength); + QTcpSocket *plainSocket = socket->plainSocket; + Q_ASSERT(plainSocket); + const qint64 bytes = plainSocket->write(data, *dataLength); #ifdef QSSLSOCKET_DEBUG qCDebug(lcSsl) << plainSocket << "write" << bytes; @@ -407,7 +435,9 @@ void QSslSocketBackendPrivate::continueHandshake() } #endif // QT_DARWIN_PLATFORM_SDK_EQUAL_OR_ABOVE - emit q->encrypted(); + if (!renegotiating) + emit q->encrypted(); + if (autoStartHandshake && pendingClose) { pendingClose = false; q->disconnectFromHost(); @@ -505,10 +535,10 @@ void QSslSocketBackendPrivate::transmit() if (!context || shutdown) return; - if (!connectionEncrypted) + if (!isHandshakeComplete()) startHandshake(); - if (connectionEncrypted && !writeBuffer.isEmpty()) { + if (isHandshakeComplete() && !writeBuffer.isEmpty()) { qint64 totalBytesWritten = 0; while (writeBuffer.nextDataBlockSize() > 0 && context) { const size_t nextDataBlockSize = writeBuffer.nextDataBlockSize(); @@ -543,7 +573,7 @@ void QSslSocketBackendPrivate::transmit() } } - if (connectionEncrypted) { + if (isHandshakeComplete()) { QVarLengthArray data; while (context && (!readBufferMaxSize || buffer.size() < readBufferMaxSize)) { size_t readBytes = 0; @@ -563,6 +593,11 @@ void QSslSocketBackendPrivate::transmit() break; } + if (err == errSSLWouldBlock && renegotiating) { + startHandshake(); + break; + } + if (readBytes) { buffer.append(data.constData(), readBytes); if (readyReadEmittedPointer) @@ -845,8 +880,9 @@ bool QSslSocketBackendPrivate::initSslContext() return false; } - const OSStatus err = SSLSetIOFuncs(context, reinterpret_cast(&_q_SSLRead), - reinterpret_cast(&_q_SSLWrite)); + const OSStatus err = SSLSetIOFuncs(context, + reinterpret_cast(&QSslSocketBackendPrivate::ReadCallback), + reinterpret_cast(&QSslSocketBackendPrivate::WriteCallback)); if (err != errSecSuccess) { destroySslContext(); setErrorAndEmit(QAbstractSocket::SslInternalError, @@ -854,7 +890,7 @@ bool QSslSocketBackendPrivate::initSslContext() return false; } - SSLSetConnection(context, plainSocket); + SSLSetConnection(context, this); if (mode == QSslSocket::SslServerMode && !configuration.localCertificateChain.isEmpty()) { @@ -1406,6 +1442,7 @@ bool QSslSocketBackendPrivate::startHandshake() // Failure means a real error (invalid certificate, no private key, etc). if (!setSessionCertificate(errorDescription, errorCode)) { setErrorAndEmit(errorCode, errorDescription); + renegotiating = false; return false; } else { // We try to resume a handshake, even if have no @@ -1420,6 +1457,7 @@ bool QSslSocketBackendPrivate::startHandshake() return startHandshake(); } + renegotiating = false; setErrorAndEmit(QAbstractSocket::SslHandshakeFailedError, QStringLiteral("SSLHandshake failed: %1").arg(err)); plainSocket->disconnectFromHost(); @@ -1429,6 +1467,7 @@ bool QSslSocketBackendPrivate::startHandshake() // Connection aborted during handshake phase. if (q->state() != QAbstractSocket::ConnectedState) { qCDebug(lcSsl) << "connection aborted"; + renegotiating = false; return false; } @@ -1437,13 +1476,16 @@ bool QSslSocketBackendPrivate::startHandshake() if (!verifySessionProtocol()) { setErrorAndEmit(QAbstractSocket::SslHandshakeFailedError, QStringLiteral("Protocol version mismatch")); plainSocket->disconnectFromHost(); + renegotiating = false; return false; } if (verifyPeerTrust()) { continueHandshake(); + renegotiating = false; return true; } else { + renegotiating = false; return false; } } diff --git a/src/network/ssl/qsslsocket_mac_p.h b/src/network/ssl/qsslsocket_mac_p.h index 34e30ebb16d..e37171e56a6 100644 --- a/src/network/ssl/qsslsocket_mac_p.h +++ b/src/network/ssl/qsslsocket_mac_p.h @@ -120,7 +120,14 @@ private: bool checkSslErrors(); bool startHandshake(); + bool isHandshakeComplete() const {return connectionEncrypted && !renegotiating;} + + // IO callbacks: + static OSStatus ReadCallback(QSslSocketBackendPrivate *socket, char *data, size_t *dataLength); + static OSStatus WriteCallback(QSslSocketBackendPrivate *plainSocket, const char *data, size_t *dataLength); + QSecureTransportContext context; + bool renegotiating = false; Q_DISABLE_COPY(QSslSocketBackendPrivate) };