From 7d67da15ee7bd082144b1cf9490d8f81bed9ead9 Mon Sep 17 00:00:00 2001 From: Eric Prokop Date: Mon, 3 May 2021 08:15:32 +0000 Subject: FIX(server,client): Validate/use the correct certifiacte from a peers chain Currently, obtaining and validating the peer's certificate chain is a mess. While obtaining the chain (which is ordered, starting with the peer's immediate certificate and ending with the CA's certificate), the peers immediate certificate is added again, as last certificate. Then, while validating, the last certificate is checked. This approach works (since the validated certificate is the one that is expliticly added), but puts the whole concept of a ordered certificate chain to absurdity. This commit fixes that. First, the chain it is returned unaltered in its original form (ordered and starting with the peer's immediate certificate and ending with the CA's certificate). Then, while validating, the first certificate in this chain is checked. Fixes #3523 (partially) --- src/Connection.cpp | 12 +++++++----- src/Connection.h | 3 ++- src/mumble/ServerHandler.cpp | 3 ++- src/murmur/Server.cpp | 3 ++- 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/Connection.cpp b/src/Connection.cpp index 88e0bcd00..e3a8c1d3b 100644 --- a/src/Connection.cpp +++ b/src/Connection.cpp @@ -228,11 +228,13 @@ quint16 Connection::localPort() const { } QList< QSslCertificate > Connection::peerCertificateChain() const { - const QSslCertificate cert = qtsSocket->peerCertificate(); - if (cert.isNull()) - return QList< QSslCertificate >(); - else - return qtsSocket->peerCertificateChain() << cert; + // The documentation of QSslSocket::peerCertificateChain() actually says nothing + // about the order of the certificates in the chain. The sentence in this functions + // documentation is taken from QSslConfiguration::peerCertificateChain(). + // Through tests and by looking into Qt's source code it was validated, + // that these two functions do the same thing. + // See mumble-voip/mumble#5280 for more information. + return qtsSocket->peerCertificateChain(); } QSslCipher Connection::sessionCipher() const { diff --git a/src/Connection.h b/src/Connection.h index 9bb333719..cfba240b7 100644 --- a/src/Connection.h +++ b/src/Connection.h @@ -74,7 +74,8 @@ public: QMutex qmCrypt; #endif std::unique_ptr< CryptState > csCrypt; - + /// Returns the peer's chain of digital certificates, starting with the peer's immediate certificate + /// and ending with the CA's certificate. QList< QSslCertificate > peerCertificateChain() const; QSslCipher sessionCipher() const; QSsl::SslProtocol sessionProtocol() const; diff --git a/src/mumble/ServerHandler.cpp b/src/mumble/ServerHandler.cpp index 399797193..83b569a6e 100644 --- a/src/mumble/ServerHandler.cpp +++ b/src/mumble/ServerHandler.cpp @@ -745,7 +745,8 @@ void ServerHandler::serverConnectionConnected() { qscCipher = connection->sessionCipher(); if (!qscCert.isEmpty()) { - const QSslCertificate &qsc = qscCert.last(); + // Get the server's immediate SSL certificate + const QSslCertificate &qsc = qscCert.first(); qbaDigest = sha1(qsc.publicKey().toDer()); bUdp = database->getUdp(qbaDigest); } else { diff --git a/src/murmur/Server.cpp b/src/murmur/Server.cpp index 28f7cf1f7..debda6a6e 100644 --- a/src/murmur/Server.cpp +++ b/src/murmur/Server.cpp @@ -1473,7 +1473,8 @@ void Server::encrypted() { QList< QSslCertificate > certs = uSource->peerCertificateChain(); if (!certs.isEmpty()) { - const QSslCertificate &cert = certs.last(); + // Get the client's immediate SSL certificate + const QSslCertificate &cert = certs.first(); uSource->qslEmail = cert.subjectAlternativeNames().values(QSsl::EmailEntry); uSource->qsHash = QString::fromLatin1(cert.digest(QCryptographicHash::Sha1).toHex()); if (!uSource->qslEmail.isEmpty() && uSource->bVerified) { -- cgit v1.2.3