From 69a43c6c3e3fcaf10480544c1638f6446ed25d00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Tue, 27 Aug 2019 01:14:39 +0200 Subject: Schannel refactoring This moves some repeated code into functions (namely readToBuffer and retainExtraData) while also changing how the intermediateBuffer is handled to avoid deallocating and reallocating repeatedly. Change-Id: I49e6cee641f961565051a67123c56b1c8f3c0259 Reviewed-by: Edward Welbourne Reviewed-by: Timur Pocheptsov --- src/network/ssl/qsslsocket_schannel.cpp | 105 +++++++++++++++++++------------- 1 file changed, 61 insertions(+), 44 deletions(-) (limited to 'src/network') diff --git a/src/network/ssl/qsslsocket_schannel.cpp b/src/network/ssl/qsslsocket_schannel.cpp index d7fb080b49..3166cda4c9 100644 --- a/src/network/ssl/qsslsocket_schannel.cpp +++ b/src/network/ssl/qsslsocket_schannel.cpp @@ -431,6 +431,42 @@ QByteArray createAlpnString(const QByteArrayList &nextAllowedProtocols) return alpnString; } #endif // SUPPORTS_ALPN + +qint64 readToBuffer(QByteArray &buffer, QTcpSocket *plainSocket) +{ + Q_ASSERT(plainSocket); + static const qint64 shrinkCutoff = 1024 * 12; + static const qint64 defaultRead = 1024 * 16; + qint64 bytesRead = 0; + + const auto toRead = std::min(defaultRead, plainSocket->bytesAvailable()); + if (toRead > 0) { + const auto bufferSize = buffer.size(); + buffer.reserve(bufferSize + toRead); // avoid growth strategy kicking in + buffer.resize(bufferSize + toRead); + bytesRead = plainSocket->read(buffer.data() + bufferSize, toRead); + buffer.resize(bufferSize + bytesRead); + // In case of excessive memory usage we shrink: + if (buffer.size() < shrinkCutoff && buffer.capacity() > defaultRead) + buffer.shrink_to_fit(); + } + + return bytesRead; +} + +void retainExtraData(QByteArray &buffer, const SecBuffer &secBuffer) +{ + Q_ASSERT(secBuffer.BufferType == SECBUFFER_EXTRA); + if (int(secBuffer.cbBuffer) >= buffer.size()) + return; + +#ifdef QSSLSOCKET_DEBUG + qCDebug(lcSsl, "We got SECBUFFER_EXTRA, will retain %lu bytes", secBuffer.cbBuffer); +#endif + std::move(buffer.end() - secBuffer.cbBuffer, buffer.end(), buffer.begin()); + buffer.resize(secBuffer.cbBuffer); +} + } // anonymous namespace bool QSslSocketPrivate::s_loadRootCertsOnDemand = true; @@ -619,8 +655,8 @@ bool QSslSocketBackendPrivate::acquireCredentialsHandle() nullptr); if (!chainContext) { const QString message = isClient - ? QSslSocket::tr("The certificate provided cannot be used for a client.") - : QSslSocket::tr("The certificate provided cannot be used for a server."); + ? QSslSocket::tr("The certificate provided cannot be used for a client.") + : QSslSocket::tr("The certificate provided cannot be used for a server."); setErrorAndEmit(QAbstractSocket::SocketError::SslInvalidUserDataError, message); return false; } @@ -774,7 +810,7 @@ bool QSslSocketBackendPrivate::acceptContext() Q_ASSERT(mode == QSslSocket::SslServerMode); ULONG contextReq = getContextRequirements(); - intermediateBuffer += plainSocket->read(16384); + readToBuffer(intermediateBuffer, plainSocket); if (intermediateBuffer.isEmpty()) return true; // definitely need more data.. @@ -837,9 +873,9 @@ bool QSslSocketBackendPrivate::acceptContext() // https://docs.microsoft.com/en-us/windows/desktop/secauthn/extra-buffers-returned-by-schannel // inBuffers[1].cbBuffer indicates the amount of bytes _NOT_ processed, the rest need to // be stored. - intermediateBuffer = intermediateBuffer.right(int(inBuffers[1].cbBuffer)); + retainExtraData(intermediateBuffer, inBuffers[1]); } else { /* No 'extra' data, message not incomplete */ - intermediateBuffer.clear(); + intermediateBuffer.resize(0); } if (status != SEC_I_CONTINUE_NEEDED) { @@ -865,11 +901,11 @@ bool QSslSocketBackendPrivate::performHandshake() Q_ASSERT(schannelState == SchannelState::PerformHandshake); #ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << "Bytes available from socket:" << plainSocket->bytesAvailable(); - qCDebug(lcSsl) << "intermediateBuffer size:" << intermediateBuffer.size(); + qCDebug(lcSsl, "Bytes available from socket: %lld", plainSocket->bytesAvailable()); + qCDebug(lcSsl, "intermediateBuffer size: %d", intermediateBuffer.size()); #endif - intermediateBuffer += plainSocket->read(16384); + readToBuffer(intermediateBuffer, plainSocket); if (intermediateBuffer.isEmpty()) return true; // no data, will fail @@ -918,11 +954,10 @@ bool QSslSocketBackendPrivate::performHandshake() // https://docs.microsoft.com/en-us/windows/desktop/secauthn/extra-buffers-returned-by-schannel // inputBuffers[1].cbBuffer indicates the amount of bytes _NOT_ processed, the rest need to // be stored. - intermediateBuffer = intermediateBuffer.right(int(inputBuffers[1].cbBuffer)); - } else { + retainExtraData(intermediateBuffer, inputBuffers[1]); + } else if (status != SEC_E_INCOMPLETE_MESSAGE) { // Clear the buffer if we weren't asked for more data - if (status != SEC_E_INCOMPLETE_MESSAGE) - intermediateBuffer.clear(); + intermediateBuffer.resize(0); } switch (status) { case SEC_E_OK: @@ -1228,8 +1263,7 @@ void QSslSocketBackendPrivate::transmit() fullMessage.resize(inputBuffers[0].cbBuffer + inputBuffers[1].cbBuffer + inputBuffers[2].cbBuffer); const qint64 bytesWritten = plainSocket->write(fullMessage); #ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << "Wrote" << bytesWritten << "of total" - << fullMessage.length() << "bytes"; + qCDebug(lcSsl, "Wrote %lld of total %d bytes", bytesWritten, fullMessage.length()); #endif if (bytesWritten >= 0) { totalBytesWritten += bytesWritten; @@ -1254,36 +1288,24 @@ void QSslSocketBackendPrivate::transmit() int totalRead = 0; bool hadIncompleteData = false; while (!readBufferMaxSize || buffer.size() < readBufferMaxSize) { - QByteArray ciphertext; - if (intermediateBuffer.length()) { + const qint64 bytesRead = readToBuffer(intermediateBuffer, plainSocket); #ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << "Restoring data from intermediateBuffer:" - << intermediateBuffer.length() << "bytes"; + qCDebug(lcSsl, "Read %lld encrypted bytes from the socket", bytesRead); #endif - ciphertext.swap(intermediateBuffer); - } - int initialLength = ciphertext.length(); - ciphertext += plainSocket->read(16384); + if (intermediateBuffer.length() == 0 || (hadIncompleteData && bytesRead == 0)) { #ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << "Read" << ciphertext.length() - initialLength - << "encrypted bytes from the socket"; + qCDebug(lcSsl, (hadIncompleteData ? "No new data received, leaving loop!" + : "Nothing to decrypt, leaving loop!")); #endif - if (ciphertext.length() == 0 || (hadIncompleteData && initialLength == ciphertext.length())) { -#ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << (hadIncompleteData ? "No new data received, leaving loop!" - : "Nothing to decrypt, leaving loop!"); -#endif - if (ciphertext.length()) // We have data, it came from intermediateBuffer, swap back - intermediateBuffer.swap(ciphertext); break; } hadIncompleteData = false; #ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << "Total amount of bytes to decrypt:" << ciphertext.length(); + qCDebug(lcSsl, "Total amount of bytes to decrypt: %d", intermediateBuffer.length()); #endif SecBuffer dataBuffer[4]{ - createSecBuffer(ciphertext, SECBUFFER_DATA), + createSecBuffer(intermediateBuffer, SECBUFFER_DATA), createSecBuffer(nullptr, 0, SECBUFFER_EMPTY), createSecBuffer(nullptr, 0, SECBUFFER_EMPTY), createSecBuffer(nullptr, 0, SECBUFFER_EMPTY) @@ -1299,34 +1321,29 @@ void QSslSocketBackendPrivate::transmit() if (dataBuffer[1].cbBuffer > 0) { // It is always decrypted in-place. // But [0] is the STREAM_HEADER, [1] is the DATA and [2] is the STREAM_TRAILER. - // The pointers in all of those still point into the 'ciphertext' byte array. + // The pointers in all of those still point into 'intermediateBuffer'. buffer.append(static_cast(dataBuffer[1].pvBuffer), dataBuffer[1].cbBuffer); totalRead += dataBuffer[1].cbBuffer; #ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << "Decrypted" << dataBuffer[1].cbBuffer - << "bytes. New read buffer size:" << buffer.size(); + qCDebug(lcSsl, "Decrypted %lu bytes. New read buffer size: %d", + dataBuffer[1].cbBuffer, buffer.size()); #endif } if (dataBuffer[3].BufferType == SECBUFFER_EXTRA) { // https://docs.microsoft.com/en-us/windows/desktop/secauthn/extra-buffers-returned-by-schannel // dataBuffer[3].cbBuffer indicates the amount of bytes _NOT_ processed, // the rest need to be stored. -#ifdef QSSLSOCKET_DEBUG - qCDebug(lcSsl) << "We've got excess data, moving it to the intermediate buffer:" - << dataBuffer[3].cbBuffer << "bytes"; -#endif - intermediateBuffer = ciphertext.right(int(dataBuffer[3].cbBuffer)); + retainExtraData(intermediateBuffer, dataBuffer[3]); + } else { + intermediateBuffer.resize(0); } } if (status == SEC_E_INCOMPLETE_MESSAGE) { - // Need more data before we can decrypt.. to the buffer it goes! #ifdef QSSLSOCKET_DEBUG qCDebug(lcSsl, "We didn't have enough data to decrypt anything, will try again!"); #endif - Q_ASSERT(intermediateBuffer.isEmpty()); - intermediateBuffer.swap(ciphertext); // We try again, but if we don't get any more data then we leave hadIncompleteData = true; } else if (status == SEC_E_INVALID_HANDLE) { -- cgit v1.2.3 From 559b563d711db0760a51b0dce26536dbc8766a9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Sat, 7 Sep 2019 02:22:21 +0200 Subject: Use Schannel's incomplete data guesstimation feature It tells us how many bytes we will need before the call succeeds. It's not accurate but will reduce the amount of calls to their slow functions Change-Id: I82393d5acd68b84c6e6f3377ba40bb1d5c51ca8a Reviewed-by: Edward Welbourne Reviewed-by: Timur Pocheptsov --- src/network/ssl/qsslsocket_schannel.cpp | 32 ++++++++++++++++++++++++++++++++ src/network/ssl/qsslsocket_schannel_p.h | 1 + 2 files changed, 33 insertions(+) (limited to 'src/network') diff --git a/src/network/ssl/qsslsocket_schannel.cpp b/src/network/ssl/qsslsocket_schannel.cpp index 3166cda4c9..78d807106b 100644 --- a/src/network/ssl/qsslsocket_schannel.cpp +++ b/src/network/ssl/qsslsocket_schannel.cpp @@ -467,6 +467,17 @@ void retainExtraData(QByteArray &buffer, const SecBuffer &secBuffer) buffer.resize(secBuffer.cbBuffer); } +qint64 checkIncompleteData(const SecBuffer &secBuffer) +{ + if (secBuffer.BufferType == SECBUFFER_MISSING) { +#ifdef QSSLSOCKET_DEBUG + qCDebug(lcSsl, "Need %lu more bytes.", secBuffer.cbBuffer); +#endif + return secBuffer.cbBuffer; +} + return 0; +} + } // anonymous namespace bool QSslSocketPrivate::s_loadRootCertsOnDemand = true; @@ -810,6 +821,10 @@ bool QSslSocketBackendPrivate::acceptContext() Q_ASSERT(mode == QSslSocket::SslServerMode); ULONG contextReq = getContextRequirements(); + if (missingData > plainSocket->bytesAvailable()) + return true; + + missingData = 0; readToBuffer(intermediateBuffer, plainSocket); if (intermediateBuffer.isEmpty()) return true; // definitely need more data.. @@ -866,6 +881,7 @@ bool QSslSocketBackendPrivate::acceptContext() if (status == SEC_E_INCOMPLETE_MESSAGE) { // Need more data + missingData = checkIncompleteData(outBuffers[0]); return true; } @@ -905,6 +921,10 @@ bool QSslSocketBackendPrivate::performHandshake() qCDebug(lcSsl, "intermediateBuffer size: %d", intermediateBuffer.size()); #endif + if (missingData > plainSocket->bytesAvailable()) + return true; + + missingData = 0; readToBuffer(intermediateBuffer, plainSocket); if (intermediateBuffer.isEmpty()) return true; // no data, will fail @@ -990,6 +1010,7 @@ bool QSslSocketBackendPrivate::performHandshake() return true; case SEC_E_INCOMPLETE_MESSAGE: // Simply incomplete, wait for more data + missingData = checkIncompleteData(outBuffers[0]); return true; case SEC_E_ALGORITHM_MISMATCH: setErrorAndEmit(QAbstractSocket::SslHandshakeFailedError, @@ -1192,6 +1213,8 @@ void QSslSocketBackendPrivate::reset() connectionEncrypted = false; shutdown = false; renegotiating = false; + + missingData = 0; } void QSslSocketBackendPrivate::startClientEncryption() @@ -1288,6 +1311,14 @@ void QSslSocketBackendPrivate::transmit() int totalRead = 0; bool hadIncompleteData = false; while (!readBufferMaxSize || buffer.size() < readBufferMaxSize) { + if (missingData > plainSocket->bytesAvailable()) { +#ifdef QSSLSOCKET_DEBUG + qCDebug(lcSsl, "We're still missing %lld bytes, will check later.", missingData); +#endif + break; + } + + missingData = 0; const qint64 bytesRead = readToBuffer(intermediateBuffer, plainSocket); #ifdef QSSLSOCKET_DEBUG qCDebug(lcSsl, "Read %lld encrypted bytes from the socket", bytesRead); @@ -1341,6 +1372,7 @@ void QSslSocketBackendPrivate::transmit() } if (status == SEC_E_INCOMPLETE_MESSAGE) { + missingData = checkIncompleteData(dataBuffer[0]); #ifdef QSSLSOCKET_DEBUG qCDebug(lcSsl, "We didn't have enough data to decrypt anything, will try again!"); #endif diff --git a/src/network/ssl/qsslsocket_schannel_p.h b/src/network/ssl/qsslsocket_schannel_p.h index 6ab200e1f9..a184deef49 100644 --- a/src/network/ssl/qsslsocket_schannel_p.h +++ b/src/network/ssl/qsslsocket_schannel_p.h @@ -145,6 +145,7 @@ private: const CERT_CONTEXT *localCertContext = nullptr; ULONG contextAttributes = 0; + qint64 missingData = 0; bool renegotiating = false; }; -- cgit v1.2.3