From eb7d1cf098df56f8ebf62f02af611a627435a4a1 Mon Sep 17 00:00:00 2001
From: Timur Pocheptsov <timur.pocheptsov@qt.io>
Date: Wed, 19 Aug 2020 11:10:11 +0200
Subject: [PATCH] QSslContext - do a little cleanup (OpenSSL)

1. Remove a useless forward declaration of a non-existing class.
2. Simplify the cipher filtering.
3. A missing private key (when local cert is present) found
   by the Qt, not OpenSSL, so no need in asking OpenSSL for errors
   in  queue.
3. Fix a potential double-free (for opaque keys).
4. read/write BIOs normally owned by SSL object, but if
   we fail to allocate any of them, we return early,
   potentially failing to free the one that was allocated.

Change-Id: Ifb52fbc9fd1a38f101bd7ff02e79b82d6eb7e5b0
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>
---
 src/network/ssl/qsslcontext_openssl.cpp | 16 +++++++---------
 src/network/ssl/qsslcontext_openssl_p.h |  2 --
 src/network/ssl/qsslsocket_openssl.cpp  |  4 ++++
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/network/ssl/qsslcontext_openssl.cpp b/src/network/ssl/qsslcontext_openssl.cpp
index feb403974d5..057f74b82a7 100644
--- a/src/network/ssl/qsslcontext_openssl.cpp
+++ b/src/network/ssl/qsslcontext_openssl.cpp
@@ -437,16 +437,13 @@ init_context:
     auto filterCiphers = [](const QList<QSslCipher> &ciphers, bool selectTls13)
     {
         QByteArray cipherString;
-        bool first = true;
 
-        for (const QSslCipher &cipher : qAsConst(ciphers)) {
+        for (const QSslCipher &cipher : ciphers) {
             const bool isTls13Cipher = cipher.protocol() == QSsl::TlsV1_3 || cipher.protocol() == QSsl::TlsV1_3OrLater;
             if (selectTls13 != isTls13Cipher)
                 continue;
 
-            if (first)
-                first = false;
-            else
+            if (cipherString.size())
                 cipherString.append(':');
             cipherString.append(cipher.name().toLatin1());
         }
@@ -530,7 +527,7 @@ init_context:
     if (!sslContext->sslConfiguration.localCertificate().isNull()) {
         // Require a private key as well.
         if (sslContext->sslConfiguration.privateKey().isNull()) {
-            sslContext->errorStr = QSslSocket::tr("Cannot provide a certificate with no key, %1").arg(QSslSocketBackendPrivate::getErrorsFromOpenSsl());
+            sslContext->errorStr = QSslSocket::tr("Cannot provide a certificate with no key");
             sslContext->errorCode = QSslError::UnspecifiedError;
             return;
         }
@@ -559,14 +556,15 @@ init_context:
                 q_EVP_PKEY_set1_EC_KEY(sslContext->pkey, reinterpret_cast<EC_KEY *>(configuration.d->privateKey.handle()));
 #endif
         }
+        auto pkey = sslContext->pkey;
+        if (configuration.d->privateKey.algorithm() == QSsl::Opaque)
+            sslContext->pkey = nullptr; // Don't free the private key, it belongs to QSslKey
 
-        if (!q_SSL_CTX_use_PrivateKey(sslContext->ctx, sslContext->pkey)) {
+        if (!q_SSL_CTX_use_PrivateKey(sslContext->ctx, pkey)) {
             sslContext->errorStr = QSslSocket::tr("Error loading private key, %1").arg(QSslSocketBackendPrivate::getErrorsFromOpenSsl());
             sslContext->errorCode = QSslError::UnspecifiedError;
             return;
         }
-        if (configuration.d->privateKey.algorithm() == QSsl::Opaque)
-            sslContext->pkey = nullptr; // Don't free the private key, it belongs to QSslKey
 
         // Check if the certificate matches the private key.
         if (!q_SSL_CTX_check_private_key(sslContext->ctx)) {
diff --git a/src/network/ssl/qsslcontext_openssl_p.h b/src/network/ssl/qsslcontext_openssl_p.h
index 70cb97aad82..5385c42240e 100644
--- a/src/network/ssl/qsslcontext_openssl_p.h
+++ b/src/network/ssl/qsslcontext_openssl_p.h
@@ -63,8 +63,6 @@ QT_BEGIN_NAMESPACE
 
 #ifndef QT_NO_SSL
 
-class QSslContextPrivate;
-
 class QSslContext
 {
 public:
diff --git a/src/network/ssl/qsslsocket_openssl.cpp b/src/network/ssl/qsslsocket_openssl.cpp
index f421df875c5..c5f82502fcf 100644
--- a/src/network/ssl/qsslsocket_openssl.cpp
+++ b/src/network/ssl/qsslsocket_openssl.cpp
@@ -747,6 +747,10 @@ bool QSslSocketBackendPrivate::initSslContext()
     if (!readBio || !writeBio) {
         setErrorAndEmit(QAbstractSocket::SslInternalError,
                         QSslSocket::tr("Error creating SSL session: %1").arg(getErrorsFromOpenSsl()));
+        if (readBio)
+            q_BIO_free(readBio);
+        if (writeBio)
+            q_BIO_free(writeBio);
         return false;
     }