Ignore expired certificate during certificate validation

OpenSSL has a bug when validating a chain with two certificates.
If a certificate exists twice (which is a valid use case for renewed
CAs), and the first one it hits is expired (which depends on the order
on data structure internal to OpenSSL), it will fail to validate the
chain.

This is only a bandaid fix, which trades improved chain validation
for error reporting accuracy. However given that reissuing of CA certs
is a real problem that is only getting worse, this fix is needed.

See also: https://www.openssl.org/docs/ssl/SSL_CTX_load_verify_locations.html#WARNINGS

[ChangeLog][QtNetwork][QSslSocket] Added a workaround to an OpenSSL problem
that may cause errors when the trust store contains two certificates of the
issuing CA, one of which is expired.

Task-number: QTBUG-38896
Change-Id: I8f17972ac94555648098624e470fff0eff2e7940
Reviewed-by: Richard J. Moore <rich@kde.org>
Reviewed-by: Frederik Gladhorn <frederik.gladhorn@digia.com>

2 files changed, 24 insertions, 23 deletions

diff --git a/src/network/ssl/qsslcontext.cpp b/src/network/ssl/qsslcontext.cpp
index 9c68218062..f5e5352d5e 100644
--- a/src/network/ssl/qsslcontext.cpp
+++ b/src/network/ssl/qsslcontext.cpp
@@ -214,22 +214,23 @@ init_context:
     }
 
     // Add all our CAs to this store.
-    QList<QSslCertificate> expiredCerts;
     foreach (const QSslCertificate &caCertificate, sslContext->sslConfiguration.caCertificates()) {
-        // add expired certs later, so that the
-        // valid ones are used before the expired ones
-        if (caCertificate.expiryDate() < QDateTime::currentDateTime()) {
-            expiredCerts.append(caCertificate);
-        } else {
+        // From https://www.openssl.org/docs/ssl/SSL_CTX_load_verify_locations.html:
+        //
+        // If several CA certificates matching the name, key identifier, and
+        // serial number condition are available, only the first one will be
+        // examined. This may lead to unexpected results if the same CA
+        // certificate is available with different expiration dates. If a
+        // ``certificate expired'' verification error occurs, no other
+        // certificate will be searched. Make sure to not have expired
+        // certificates mixed with valid ones.
+        //
+        // See also: QSslSocketBackendPrivate::verify()
+        if (caCertificate.expiryDate() >= QDateTime::currentDateTime()) {
             q_X509_STORE_add_cert(sslContext->ctx->cert_store, (X509 *)caCertificate.handle());
         }
     }
 
-    // now add the expired certs
-    foreach (const QSslCertificate &caCertificate, expiredCerts) {
-        q_X509_STORE_add_cert(sslContext->ctx->cert_store, reinterpret_cast<X509 *>(caCertificate.handle()));
-    }
-
     if (QSslSocketPrivate::s_loadRootCertsOnDemand && allowRootCertOnDemandLoading) {
         // tell OpenSSL the directories where to look up the root certs on demand
         QList<QByteArray> unixDirs = QSslSocketPrivate::unixRootCertDirectories();
diff --git a/src/network/ssl/qsslsocket_openssl.cpp b/src/network/ssl/qsslsocket_openssl.cpp
index 1b3928fdfb..45e2fabfe9 100644
--- a/src/network/ssl/qsslsocket_openssl.cpp
+++ b/src/network/ssl/qsslsocket_openssl.cpp
@@ -1598,23 +1598,23 @@ QList<QSslError> QSslSocketBackendPrivate::verify(QList<QSslCertificate> certifi
         setDefaultCaCertificates(defaultCaCertificates() + systemCaCertificates());
     }
 
-    QList<QSslCertificate> expiredCerts;
-
     foreach (const QSslCertificate &caCertificate, QSslSocket::defaultCaCertificates()) {
-        // add expired certs later, so that the
-        // valid ones are used before the expired ones
-        if (caCertificate.expiryDate() < QDateTime::currentDateTime()) {
-            expiredCerts.append(caCertificate);
-        } else {
+        // From https://www.openssl.org/docs/ssl/SSL_CTX_load_verify_locations.html:
+        //
+        // If several CA certificates matching the name, key identifier, and
+        // serial number condition are available, only the first one will be
+        // examined. This may lead to unexpected results if the same CA
+        // certificate is available with different expiration dates. If a
+        // ``certificate expired'' verification error occurs, no other
+        // certificate will be searched. Make sure to not have expired
+        // certificates mixed with valid ones.
+        //
+        // See also: QSslContext::fromConfiguration()
+        if (caCertificate.expiryDate() >= QDateTime::currentDateTime()) {
             q_X509_STORE_add_cert(certStore, reinterpret_cast<X509 *>(caCertificate.handle()));
         }
     }
 
-    // now add the expired certs
-    foreach (const QSslCertificate &caCertificate, expiredCerts) {
-        q_X509_STORE_add_cert(certStore, reinterpret_cast<X509 *>(caCertificate.handle()));
-    }
-
     QMutexLocker sslErrorListMutexLocker(&_q_sslErrorList()->mutex);
 
     // Register a custom callback to get all verification errors.