QSslSocket::verify: do not alter the default configuration

QSslCertificate::verify() has an undocumented and not very desirable property -
on some platorms it updates the default configuration, which can be surprising.
For example, we deprecated QSslSocket::setDefaultCaCertificates() and recommend
using QSslConfiguration::defaultConfiguration(), QSslConfiguration::setDefaultConfiguration(),
and QSslConfiguration::setCaCertificates(). If an application does this to select
CA roots it trusts explicitly, and then for some reason is calling verify, the
application can have its QSslSockets successfully connecting to a host, whose
root was not trusted by the application. Also, on Windows, defaultCaCertificates()
include system roots already, no need to have them twice.

[ChangeLog][QtCore][QtNetwork] QSslSocket::verify - do not change the default configuration

Fixes: QTBUG-88639
Change-Id: I1cd40b259d0a6dcd15c78d1e7c027ff10859595c
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
(cherry picked from commit 1158ff67b492853b72199ed78bfcf24132e1c7ff)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

4 files changed, 108 insertions, 8 deletions

diff --git a/src/network/ssl/qsslcertificate.cpp b/src/network/ssl/qsslcertificate.cpp
index 5dd3f49524..c63ef5d205 100644
--- a/src/network/ssl/qsslcertificate.cpp
+++ b/src/network/ssl/qsslcertificate.cpp
@@ -591,9 +591,9 @@ QList<QSslCertificate> QSslCertificate::fromData(const QByteArray &data, QSsl::E
     the specified host name.
 
     Note that the root (CA) certificate should not be included in the list to be verified,
-    this will be looked up automatically either using the CA list specified by
-    QSslSocket::defaultCaCertificates() or, if possible, it will be loaded on demand
-    on Unix.
+    this will be looked up automatically using the CA list specified in the
+    default QSslConfiguration, and, in addition, if possible, CA certificates loaded on
+    demand on Unix and Windows.
 
     \since 5.0
  */
diff --git a/src/network/ssl/qsslsocket_openssl.cpp b/src/network/ssl/qsslsocket_openssl.cpp
index 39d011d684..7a237ee7db 100644
--- a/src/network/ssl/qsslsocket_openssl.cpp
+++ b/src/network/ssl/qsslsocket_openssl.cpp
@@ -2319,10 +2319,16 @@ QList<QSslCertificate> QSslSocketBackendPrivate::STACKOFX509_to_QSslCertificates
 QList<QSslError> QSslSocketBackendPrivate::verify(const QList<QSslCertificate> &certificateChain,
                                                   const QString &hostName)
 {
+    auto roots = QSslConfiguration::defaultConfiguration().caCertificates();
+#ifndef Q_OS_WIN
+    // On Windows, system CA certificates are already set as default ones.
+    // No need to add them again (and again) and also, if the default configuration
+    // has its own set of CAs, this probably should not be amended by the ones
+    // from the 'ROOT' store, since it's not what an application chose to trust.
     if (s_loadRootCertsOnDemand)
-        setDefaultCaCertificates(defaultCaCertificates() + systemCaCertificates());
-
-    return verify(QSslConfiguration::defaultConfiguration().caCertificates(), certificateChain, hostName);
+        roots.append(systemCaCertificates());
+#endif // Q_OS_WIN
+    return verify(roots, certificateChain, hostName);
 }
 
 QList<QSslError> QSslSocketBackendPrivate::verify(const QList<QSslCertificate> &caCertificates,
diff --git a/tests/auto/network/ssl/qsslsocket/certs/qtiochain.crt b/tests/auto/network/ssl/qsslsocket/certs/qtiochain.crt
new file mode 100644
index 0000000000..9634bcef68
--- /dev/null
+++ b/tests/auto/network/ssl/qsslsocket/certs/qtiochain.crt
@@ -0,0 +1,50 @@
+-----BEGIN CERTIFICATE-----
+MIIEjjCCBDOgAwIBAgIQCQsKtxCf9ik3vIVQ+PMa5TAKBggqhkjOPQQDAjBKMQsw
+CQYDVQQGEwJVUzEZMBcGA1UEChMQQ2xvdWRmbGFyZSwgSW5jLjEgMB4GA1UEAxMX
+Q2xvdWRmbGFyZSBJbmMgRUNDIENBLTMwHhcNMjAwODE2MDAwMDAwWhcNMjEwODE2
+MTIwMDAwWjBhMQswCQYDVQQGEwJVUzELMAkGA1UECBMCQ0ExFjAUBgNVBAcTDVNh
+biBGcmFuY2lzY28xGTAXBgNVBAoTEENsb3VkZmxhcmUsIEluYy4xEjAQBgNVBAMT
+CXd3dy5xdC5pbzBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABP/r0xH22wdU8fLk
+RsXhxRj5fmUNUo7rxnUl3lyqYYp53cLvn3agQifXkegpE8Xv4pGmuyWZj85FtoeZ
+UZh8iyCjggLiMIIC3jAfBgNVHSMEGDAWgBSlzjfq67B1DpRniLRF+tkkEIeWHzAd
+BgNVHQ4EFgQU7qPYGi9VtC4/6MS+54LNEAXApBgwFAYDVR0RBA0wC4IJd3d3LnF0
+LmlvMA4GA1UdDwEB/wQEAwIHgDAdBgNVHSUEFjAUBggrBgEFBQcDAQYIKwYBBQUH
+AwIwewYDVR0fBHQwcjA3oDWgM4YxaHR0cDovL2NybDMuZGlnaWNlcnQuY29tL0Ns
+b3VkZmxhcmVJbmNFQ0NDQS0zLmNybDA3oDWgM4YxaHR0cDovL2NybDQuZGlnaWNl
+cnQuY29tL0Nsb3VkZmxhcmVJbmNFQ0NDQS0zLmNybDBMBgNVHSAERTBDMDcGCWCG
+SAGG/WwBATAqMCgGCCsGAQUFBwIBFhxodHRwczovL3d3dy5kaWdpY2VydC5jb20v
+Q1BTMAgGBmeBDAECAjB2BggrBgEFBQcBAQRqMGgwJAYIKwYBBQUHMAGGGGh0dHA6
+Ly9vY3NwLmRpZ2ljZXJ0LmNvbTBABggrBgEFBQcwAoY0aHR0cDovL2NhY2VydHMu
+ZGlnaWNlcnQuY29tL0Nsb3VkZmxhcmVJbmNFQ0NDQS0zLmNydDAMBgNVHRMBAf8E
+AjAAMIIBBAYKKwYBBAHWeQIEAgSB9QSB8gDwAHYA9lyUL9F3MCIUVBgIMJRWjuNN
+Exkzv98MLyALzE7xZOMAAAFz90PlSQAABAMARzBFAiAhrrtxdmuxpCy8HAJJ5Qkg
+WNvlo8nZqfe6pqGUcz0dmwIhAOMqDtd5ZhcfRk96GAJxPm8bH4hDnmqDP/zJG2Mq
+nFpMAHYAXNxDkv7mq0VEsV6a1FbmEDf71fpH3KFzlLJe5vbHDsoAAAFz90PlewAA
+BAMARzBFAiB/EkdY10LDdaRcf6eSc/QxucxU+2PI+3pWjh/21A8ZUAIhAK2Qz9Kw
+onlRNyHpV3E6qyVydkXihj3c3q5UclpURYcmMAoGCCqGSM49BAMCA0kAMEYCIQDz
+K/lzLb2Rbeg1HErRLLm2HkJUmfOGU2+tbROSTGK8ugIhAKA+MKqaZ8VjPxQ+Ho4v
+fuwccvZfkU/fg8tAHTOzX23v
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+MIIDzTCCArWgAwIBAgIQCjeHZF5ftIwiTv0b7RQMPDANBgkqhkiG9w0BAQsFADBa
+MQswCQYDVQQGEwJJRTESMBAGA1UEChMJQmFsdGltb3JlMRMwEQYDVQQLEwpDeWJl
+clRydXN0MSIwIAYDVQQDExlCYWx0aW1vcmUgQ3liZXJUcnVzdCBSb290MB4XDTIw
+MDEyNzEyNDgwOFoXDTI0MTIzMTIzNTk1OVowSjELMAkGA1UEBhMCVVMxGTAXBgNV
+BAoTEENsb3VkZmxhcmUsIEluYy4xIDAeBgNVBAMTF0Nsb3VkZmxhcmUgSW5jIEVD
+QyBDQS0zMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEua1NZpkUC0bsH4HRKlAe
+nQMVLzQSfS2WuIg4m4Vfj7+7Te9hRsTJc9QkT+DuHM5ss1FxL2ruTAUJd9NyYqSb
+16OCAWgwggFkMB0GA1UdDgQWBBSlzjfq67B1DpRniLRF+tkkEIeWHzAfBgNVHSME
+GDAWgBTlnVkwgkdYzKz6CFQ2hns6tQRN8DAOBgNVHQ8BAf8EBAMCAYYwHQYDVR0l
+BBYwFAYIKwYBBQUHAwEGCCsGAQUFBwMCMBIGA1UdEwEB/wQIMAYBAf8CAQAwNAYI
+KwYBBQUHAQEEKDAmMCQGCCsGAQUFBzABhhhodHRwOi8vb2NzcC5kaWdpY2VydC5j
+b20wOgYDVR0fBDMwMTAvoC2gK4YpaHR0cDovL2NybDMuZGlnaWNlcnQuY29tL09t
+bmlyb290MjAyNS5jcmwwbQYDVR0gBGYwZDA3BglghkgBhv1sAQEwKjAoBggrBgEF
+BQcCARYcaHR0cHM6Ly93d3cuZGlnaWNlcnQuY29tL0NQUzALBglghkgBhv1sAQIw
+CAYGZ4EMAQIBMAgGBmeBDAECAjAIBgZngQwBAgMwDQYJKoZIhvcNAQELBQADggEB
+AAUkHd0bsCrrmNaF4zlNXmtXnYJX/OvoMaJXkGUFvhZEOFp3ArnPEELG4ZKk40Un
++ABHLGioVplTVI+tnkDB0A+21w0LOEhsUCxJkAZbZB2LzEgwLt4I4ptJIsCSDBFe
+lpKU1fwg3FZs5ZKTv3ocwDfjhUkV+ivhdDkYD7fa86JXWGBPzI6UAPxGezQxPk1H
+goE6y/SJXQ7vTQ1unBuCJN0yJV0ReFEQPaA1IwQvZW+cwdFD19Ae8zFnWSfda9J1
+CZMRJCQUzym+5iPDuI9yP+kHyCREU3qzuWFloUwOxkgAyXVjBYdwRVKD05WdRerw
+6DEdfgkfCv4+3ao8XnTSrLE=
+-----END CERTIFICATE-----
diff --git a/tests/auto/network/ssl/qsslsocket/tst_qsslsocket.cpp b/tests/auto/network/ssl/qsslsocket/tst_qsslsocket.cpp
index d5035c5266..0d75d4dd33 100644
--- a/tests/auto/network/ssl/qsslsocket/tst_qsslsocket.cpp
+++ b/tests/auto/network/ssl/qsslsocket/tst_qsslsocket.cpp
@@ -31,6 +31,7 @@
 #include <QtCore/qthread.h>
 #include <QtCore/qelapsedtimer.h>
 #include <QtCore/qrandom.h>
+#include <QtCore/qscopeguard.h>
 #include <QtNetwork/qhostaddress.h>
 #include <QtNetwork/qhostinfo.h>
 #include <QtNetwork/qnetworkproxy.h>
@@ -51,10 +52,12 @@
 #include "../../../network-settings.h"
 
 #ifndef QT_NO_SSL
+
 #ifndef QT_NO_OPENSSL
 #include "private/qsslsocket_openssl_p.h"
 #include "private/qsslsocket_openssl_symbols_p.h"
-#endif
+#endif // QT_NO_OPENSSL
+
 #include "private/qsslsocket_p.h"
 #include "private/qsslconfiguration_p.h"
 
@@ -73,7 +76,8 @@ typedef QSharedPointer<QSslSocket> QSslSocketPtr;
 #define FLUKE_CERTIFICATE_ERROR QSslError::SelfSignedCertificate
 #else
 #define FLUKE_CERTIFICATE_ERROR QSslError::CertificateUntrusted
-#endif
+#endif // QT_NO_OPENSSL
+
 #endif // QT_NO_OPENSSL
 
 // Detect ALPN (Application-Layer Protocol Negotiation) support
@@ -218,6 +222,9 @@ private slots:
     void waitForMinusOne();
     void verifyMode();
     void verifyDepth();
+#ifndef QT_NO_OPENSSL
+    void verifyAndDefaultConfiguration();
+#endif // QT_NO_OPENSSL
     void disconnectFromHostWhenConnecting();
     void disconnectFromHostWhenConnected();
 #ifndef QT_NO_OPENSSL
@@ -2392,6 +2399,43 @@ void tst_QSslSocket::verifyDepth()
     QCOMPARE(socket.peerVerifyDepth(), 1);
 }
 
+#ifndef QT_NO_OPENSSL
+void tst_QSslSocket::verifyAndDefaultConfiguration()
+{
+    QFETCH_GLOBAL(const bool, setProxy);
+    if (setProxy)
+        return;
+    const auto defaultCACertificates = QSslConfiguration::defaultConfiguration().caCertificates();
+    const auto chainGuard = qScopeGuard([&defaultCACertificates]{
+        auto conf = QSslConfiguration::defaultConfiguration();
+        conf.setCaCertificates(defaultCACertificates);
+        QSslConfiguration::setDefaultConfiguration(conf);
+    });
+
+    auto chain = QSslCertificate::fromPath(testDataDir + QStringLiteral("certs/qtiochain.crt"), QSsl::Pem);
+    QCOMPARE(chain.size(), 2);
+    QVERIFY(!chain.at(0).isNull());
+    QVERIFY(!chain.at(1).isNull());
+    auto errors = QSslCertificate::verify(chain);
+    // At least, test that 'verify' did not alter the default configuration:
+    QCOMPARE(defaultCACertificates, QSslConfiguration::defaultConfiguration().caCertificates());
+    if (!errors.isEmpty())
+        QSKIP("The certificate for qt.io could not be trusted, skipping the rest of the test");
+#ifdef Q_OS_WINDOWS
+    const auto fakeCaChain = QSslCertificate::fromPath(testDataDir + QStringLiteral("certs/fluke.cert"));
+    QCOMPARE(fakeCaChain.size(), 1);
+    const auto caCert = fakeCaChain.at(0);
+    QVERIFY(!caCert.isNull());
+    auto conf = QSslConfiguration::defaultConfiguration();
+    conf.setCaCertificates({caCert});
+    QSslConfiguration::setDefaultConfiguration(conf);
+    errors = QSslCertificate::verify(chain);
+    QVERIFY(errors.size() > 0);
+    QCOMPARE(QSslConfiguration::defaultConfiguration().caCertificates(), QList{caCert});
+#endif
+}
+#endif // QT_NO_OPENSSL
+
 void tst_QSslSocket::disconnectFromHostWhenConnecting()
 {
     QSslSocketPtr socket = newSocket();