From 65e2837b30becb745036f13d98a9472a0332b152 Mon Sep 17 00:00:00 2001 From: Timur Pocheptsov Date: Fri, 20 Nov 2020 10:34:15 +0100 Subject: QSslSocket::verify: do not alter the default configuration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (cherry picked from commit 1158ff67b492853b72199ed78bfcf24132e1c7ff) Reviewed-by: Qt Cherry-pick Bot --- src/network/ssl/qsslcertificate.cpp | 6 +-- src/network/ssl/qsslsocket_openssl.cpp | 12 ++++-- .../network/ssl/qsslsocket/certs/qtiochain.crt | 50 ++++++++++++++++++++++ .../auto/network/ssl/qsslsocket/tst_qsslsocket.cpp | 48 ++++++++++++++++++++- 4 files changed, 108 insertions(+), 8 deletions(-) create mode 100644 tests/auto/network/ssl/qsslsocket/certs/qtiochain.crt 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::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 QSslSocketBackendPrivate::STACKOFX509_to_QSslCertificates QList QSslSocketBackendPrivate::verify(const QList &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 QSslSocketBackendPrivate::verify(const QList &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 #include #include +#include #include #include #include @@ -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 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(); -- cgit v1.2.3