From ca5d53abc2e5b66b0674480270eb62b08f982c6c Mon Sep 17 00:00:00 2001
From: Timur Pocheptsov <timur.pocheptsov@qt.io>
Date: Fri, 3 Aug 2018 10:36:28 +0200
Subject: [PATCH] SecureTransport - fix verifyPeerTrust for the renegotiation
 case
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Also remove unneeded and now wrong check (was marked with TODO)
which was a copy and paste from OpenSSL counterpart. There, testing
if peerCertificateChain.isEmpty() makes sense, since there we
potentially call storePeerCertificates() twice during the handshake.

Change-Id: I946e6876adb3f9504e93c06ac90ff36dd44aca4c
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
---
 src/network/ssl/qsslsocket_mac.cpp | 45 +++++++++++++++---------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/src/network/ssl/qsslsocket_mac.cpp b/src/network/ssl/qsslsocket_mac.cpp
index 8aa01deee7..bcc65874bd 100644
--- a/src/network/ssl/qsslsocket_mac.cpp
+++ b/src/network/ssl/qsslsocket_mac.cpp
@@ -1239,29 +1239,30 @@ bool QSslSocketBackendPrivate::verifyPeerTrust()
     QList<QSslError> errors;
     // store certificates
     const int certCount = SecTrustGetCertificateCount(trust);
-    // TODO: why this test depends on configuration.peerCertificateChain not being empty????
-    if (configuration.peerCertificateChain.isEmpty()) {
-        // Apple's docs say SetTrustEvaluate must be called before
-        // SecTrustGetCertificateAtIndex, but this results
-        // in 'kSecTrustResultRecoverableTrustFailure', so
-        // here we just ignore 'res' (later we'll use SetAnchor etc.
-        // and evaluate again).
-        SecTrustResultType res = kSecTrustResultInvalid;
-        err = SecTrustEvaluate(trust, &res);
-        if (err != errSecSuccess) {
-            // We can not ignore this, it's not even about trust verification
-            // probably ...
-            setErrorAndEmit(QAbstractSocket::SslHandshakeFailedError,
-                            QStringLiteral("SecTrustEvaluate failed: %1").arg(err));
-            plainSocket->disconnectFromHost();
-            return false;
-        }
 
-        for (int i = 0; i < certCount; ++i) {
-            SecCertificateRef cert = SecTrustGetCertificateAtIndex(trust, i);
-            QCFType<CFDataRef> derData = SecCertificateCopyData(cert);
-            configuration.peerCertificateChain << QSslCertificate(QByteArray::fromCFData(derData), QSsl::Der);
-        }
+    // Apple's docs say SetTrustEvaluate must be called before
+    // SecTrustGetCertificateAtIndex, but this results
+    // in 'kSecTrustResultRecoverableTrustFailure', so
+    // here we just ignore 'res' (later we'll use SetAnchor etc.
+    // and evaluate again).
+    SecTrustResultType res = kSecTrustResultInvalid;
+    err = SecTrustEvaluate(trust, &res);
+    if (err != errSecSuccess) {
+        // We can not ignore this, it's not even about trust verification
+        // probably ...
+        setErrorAndEmit(QAbstractSocket::SslHandshakeFailedError,
+                        QStringLiteral("SecTrustEvaluate failed: %1").arg(err));
+        plainSocket->disconnectFromHost();
+        return false;
+    }
+
+    configuration.peerCertificate.clear();
+    configuration.peerCertificateChain.clear();
+
+    for (int i = 0; i < certCount; ++i) {
+        SecCertificateRef cert = SecTrustGetCertificateAtIndex(trust, i);
+        QCFType<CFDataRef> derData = SecCertificateCopyData(cert);
+        configuration.peerCertificateChain << QSslCertificate(QByteArray::fromCFData(derData), QSsl::Der);
     }
 
     if (certCount > 0) {