From f211492ed4d1d422732e06f8b19100bbc09cbede Mon Sep 17 00:00:00 2001 From: Eric Prokop Date: Mon, 3 May 2021 08:19:31 +0000 Subject: [PATCH] FIX(server): Build the servers certificate chain once instead of abusing the trust store While there are serveral options to configure, which ssl certificates a server should use, the way that they were processed was a bit hacky and messy. The original process, how the certificates were used is the following: 1. Read all certs from sslCA; these were Meta::mp.qlCA 2. If sslKey set and valid, use it as private key 3. If sslKey NOT set, try to get a private key from sslCert 4. If we have no key by now, fail, else we have found our key Meta::mp.qskKey 5. Read ALL certs from sslCert and ALL certs from sslKey to a cert list 6. Use the private key to find a matching cert in the cert list 7. If there is NO match, fail 8. If theres a match, remove it from the list and consider it our cert Meta::mp.qscCert 9. Treat all leftover certs in the list as intermediates; these were Meta::mp.qlIntermediates Now, when a client connection was made, the server sets the certificate itself presents to the client to qscCert. Secondly, it configured its own ssl context to trust all certificates from Meta::mp.qlCA and Meta::mp.qlIntermediates. But according to the wiki sslCA should be used to denote the servers intermediates, not certificates it want to trust. The reason why these certificates were added to the truststore is that, if they were added to the truststore Qt (or whatever is beneath Qt in ssl) will caltulate the servers chain of trust and present them to the client. So the server actually abused the truststore to do the chain calculation. This has the undesired side effect that the server will always trust all certificates in its own chain of trust itself (which for most usecases seem desirable, but not for all). This commit changes the chain calculation: Steps 2 to 9 from above are not altered, but instead of storing the leftover certificates from step 9 into Meta::mp.qlIntermediates, they are stored in a temperary list, called pool. And instead of doing step 1, the certificates form sslCA are feed directly to pool. Then, qscCert and pool are used to calculate the chain (using openssl), just the chain is stored and everything else is discarded. Calculating the chain once in advance and then only use it is actually better then building it every time it is needed. Since the certificates from sslCA and sslCert are no longer added to the trust store, separating the things the server presents to the client, and certificates the server wants to trust is possible now. Fixes #3523 (partially) --- src/murmur/Cert.cpp | 161 +++++++++++++++++++++++++++++++++------- src/murmur/Meta.cpp | 48 ++++++------ src/murmur/Meta.h | 17 +---- src/murmur/Register.cpp | 5 +- src/murmur/Server.cpp | 50 ++----------- src/murmur/Server.h | 13 +--- 6 files changed, 170 insertions(+), 124 deletions(-) diff --git a/src/murmur/Cert.cpp b/src/murmur/Cert.cpp index 12d68c4e0c0..173703c4512 100644 --- a/src/murmur/Cert.cpp +++ b/src/murmur/Cert.cpp @@ -15,12 +15,109 @@ #include #include +#include #include #ifdef Q_OS_WIN # include #endif +QList< QSslCertificate > Server::buildSslChain(const QSslCertificate &leaf, const QList< QSslCertificate > &pool) { + QList< QSslCertificate > chain; + if (leaf.isNull()) { + return chain; + } + chain << leaf; + if (pool.isEmpty()) { + return chain; + } + + // Convert the leaf to der format and create an openssl x509 from it + QByteArray qbaLeaf = leaf.toDer(); + int maxDerSize = qbaLeaf.size(); + BIO *mem = BIO_new_mem_buf(qbaLeaf.data(), maxDerSize); + Q_UNUSED(BIO_set_close(mem, BIO_NOCLOSE)); + X509 *leaf_x509 = d2i_X509_bio(mem, nullptr); + BIO_free(mem); + + // Prepare a ssl context, the method should not matter, so just go with TLS_method() + SSL_CTX *ctx = SSL_CTX_new(TLS_method()); + + // Add the leaf + SSL_CTX_use_certificate(ctx, leaf_x509); + + // Construct openssl x509 for the pool and add each to the ctx + foreach (const QSslCertificate &cert, pool) { + QByteArray qbaCert = cert.toDer(); + int s = qbaCert.size(); + maxDerSize = maxDerSize < s ? s : maxDerSize; + BIO *mem = BIO_new_mem_buf(qbaCert.data(), s); + Q_UNUSED(BIO_set_close(mem, BIO_NOCLOSE)); + X509 *x509 = d2i_X509_bio(mem, nullptr); + BIO_free(mem); + SSL_CTX_add0_chain_cert(ctx, x509); + } + + // Do the actual chain building + int flags = SSL_BUILD_CHAIN_FLAG_CHECK | SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR; // Think of the correct flags + int ret = SSL_CTX_build_cert_chain(ctx, flags); + + // Check if we were succesfull, since we use the SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR flag + // 2 is a acceptable return value, too + if (ret == 1 || ret == 2) { + // Retrieve the chain + STACK_OF(X509) *stack = nullptr; + SSL_CTX_get0_chain_certs(ctx, &stack); + + // Copy the chain back to qt + // Instead of allocating a new buffer every time i2d_X509 is called, we allocate a shared buffer + // and because we know the maxDerSize, we konw how big this needs to be + unsigned char *buffer = (unsigned char *) malloc(maxDerSize); + while (sk_X509_num(stack) > 0) { + X509 *next = sk_X509_shift(stack); + int actualSize = i2d_X509(next, &buffer); + X509_free(next); + if (actualSize == -1) { + // Failed to der encode certificate in openssl + chain.clear(); + break; + } + // i2d_X509 altered our buffer pointer, so we need to set it back manually + buffer -= actualSize; + QByteArray array = QByteArray::fromRawData((char *) buffer, actualSize); + QList< QSslCertificate > ql = QSslCertificate::fromData(array, QSsl::EncodingFormat::Der); + if (ql.size() == 1) { + chain << ql; + } else { + // Data from openssl must contain exactly one certificate! + chain.clear(); + break; + } + } + + // Clean up + free(buffer); + } else { + chain.clear(); + } + // pool certificates where added with add0 option (not add1), + // so they will automatically be freed if the ctx is freed. + // Same for the stack, which was obtain with the set0 verison. + SSL_CTX_free(ctx); + X509_free(leaf_x509); + + // Drain OpenSSL's per-thread error queue, see below! + ERR_clear_error(); + + return chain; +} + +QByteArray Server::chainToPem(const QList< QSslCertificate > &chain) { + QByteArrayList bytes; + foreach (const QSslCertificate &cert, chain) { bytes << cert.toPem(); } + return bytes.join(); +} + bool Server::isKeyForCert(const QSslKey &key, const QSslCertificate &cert) { if (key.isNull() || cert.isNull() || (key.type() != QSsl::PrivateKey)) return false; @@ -73,8 +170,7 @@ void Server::initializeCert() { // Clear all existing SSL settings // for this server. - qscCert.clear(); - qlIntermediates.clear(); + qlCertificateChain.clear(); qskKey.clear(); #if defined(USE_QSSLDIFFIEHELLMANPARAMETERS) qsdhpDHParams = QSslDiffieHellmanParameters(); @@ -85,7 +181,7 @@ void Server::initializeCert() { pass = getConf("passphrase", QByteArray()).toByteArray(); dhparams = getConf("sslDHParams", Meta::mp.qbaDHParams).toByteArray(); - QList< QSslCertificate > ql; + // Attempt to load the private key. if (!key.isEmpty()) { @@ -101,16 +197,20 @@ void Server::initializeCert() { // remove any certs for our key from the list, what's left is part of // the CA certificate chain. if (!qskKey.isNull()) { + QList< QSslCertificate > ql; ql << QSslCertificate::fromData(crt); ql << QSslCertificate::fromData(key); + QSslCertificate tmpCrt; for (int i = 0; i < ql.size(); ++i) { const QSslCertificate &c = ql.at(i); if (isKeyForCert(qskKey, c)) { - qscCert = c; + tmpCrt = c; ql.removeAt(i); } } - qlIntermediates = ql; + if (!tmpCrt.isNull()) { + qlCertificateChain = buildSslChain(tmpCrt, ql); + } } #if defined(USE_QSSLDIFFIEHELLMANPARAMETERS) @@ -132,57 +232,62 @@ void Server::initializeCert() { QString issuer; - QStringList issuerNames = qscCert.issuerInfo(QSslCertificate::CommonName); - if (!issuerNames.isEmpty()) { - issuer = issuerNames.first(); + if (!qlCertificateChain.isEmpty()) { + QStringList issuerNames = qlCertificateChain[0].issuerInfo(QSslCertificate::CommonName); + if (!issuerNames.isEmpty()) { + issuer = issuerNames.first(); + } } // Really old certs/keys are no good, throw them away so we can // generate a new one below. if (issuer == QString::fromUtf8("Murmur Autogenerated Certificate")) { log("Old autogenerated certificate is unusable for registration, invalidating it"); - qscCert = QSslCertificate(); - qskKey = QSslKey(); + qlCertificateChain.clear(); + qlCertificateChain << QSslCertificate(); + qskKey = QSslKey(); } // If we have a cert, and it's a self-signed one, but we're binding to // all the same addresses as the Meta server is, use it's cert instead. // This allows a self-signed certificate generated by Murmur to be // replaced by a CA-signed certificate in the .ini file. - if (!qscCert.isNull() && issuer.startsWith(QString::fromUtf8("Murmur Autogenerated Certificate")) - && !Meta::mp.qscCert.isNull() && !Meta::mp.qskKey.isNull() && (Meta::mp.qlBind == qlBind)) { - qscCert = Meta::mp.qscCert; - qskKey = Meta::mp.qskKey; - qlIntermediates = Meta::mp.qlIntermediates; - - if (!qscCert.isNull() && !qskKey.isNull()) { + if (!qlCertificateChain.isEmpty() && !qlCertificateChain[0].isNull() + && issuer.startsWith(QString::fromUtf8("Murmur Autogenerated Certificate")) + && !Meta::mp.qlCertificateChain.isEmpty() && !Meta::mp.qskKey.isNull() && (Meta::mp.qlBind == qlBind)) { + qlCertificateChain.clear(); + qlCertificateChain = Meta::mp.qlCertificateChain; + qskKey = Meta::mp.qskKey; + if (!qlCertificateChain.isEmpty() && !qlCertificateChain[0].isNull() && !qskKey.isNull()) { bUsingMetaCert = true; } } // If we still don't have a certificate by now, try to load the one from Meta - if (qscCert.isNull() || qskKey.isNull()) { + if (qlCertificateChain.isEmpty() || qlCertificateChain[0].isNull() || qskKey.isNull()) { if (!key.isEmpty() || !crt.isEmpty()) { log("Certificate specified, but failed to load."); } - qskKey = Meta::mp.qskKey; - qscCert = Meta::mp.qscCert; - qlIntermediates = Meta::mp.qlIntermediates; + qlCertificateChain.clear(); + qlCertificateChain = Meta::mp.qlCertificateChain; + qskKey = Meta::mp.qskKey; - if (!qscCert.isNull() && !qskKey.isNull()) { + if (!qlCertificateChain.isEmpty() && !qlCertificateChain[0].isNull() && !qskKey.isNull()) { bUsingMetaCert = true; } // If loading from Meta doesn't work, build+sign a new one - if (qscCert.isNull() || qskKey.isNull()) { + if (qlCertificateChain.isEmpty() || qlCertificateChain[0].isNull() || qskKey.isNull()) { log("Generating new server certificate."); - - if (!SelfSignedCertificate::generateMurmurV2Certificate(qscCert, qskKey)) { + if (qlCertificateChain.isEmpty()) { + qlCertificateChain << QSslCertificate(); + } + if (!SelfSignedCertificate::generateMurmurV2Certificate(qlCertificateChain[0], qskKey)) { log("Certificate or key generation failed"); } - setConf("certificate", qscCert.toPem()); + setConf("certificate", chainToPem(qlCertificateChain)); setConf("key", qskKey.toPem()); } } @@ -216,5 +321,7 @@ void Server::initializeCert() { } const QString Server::getDigest() const { - return QString::fromLatin1(qscCert.digest(QCryptographicHash::Sha1).toHex()); + return qlCertificateChain.isEmpty() + ? QString() + : QString::fromLatin1(qlCertificateChain[0].digest(QCryptographicHash::Sha1).toHex()); } diff --git a/src/murmur/Meta.cpp b/src/murmur/Meta.cpp index 7475de650c6..51dd56f5410 100644 --- a/src/murmur/Meta.cpp +++ b/src/murmur/Meta.cpp @@ -425,6 +425,13 @@ void MetaParams::read(QString fname) { qFatal("MetaParams: Failed to load SSL settings. See previous errors."); } + // This was done two times, once in loadSSLSettings(), and then a second time below + // Move it here to do it only once + qmConfig.insert(QLatin1String("certificate"), QString::fromUtf8(Server::chainToPem(qlCertificateChain))); + qmConfig.insert(QLatin1String("key"), QString::fromUtf8(qskKey.toPem())); + qmConfig.insert(QLatin1String("sslCiphers"), qsCiphers); + qmConfig.insert(QLatin1String("sslDHParams"), QString::fromLatin1(qbaDHParams.constData())); + QStringList hosts; foreach (const QHostAddress &qha, qlBind) { hosts << qha.toString(); } qmConfig.insert(QLatin1String("host"), hosts.join(" ")); @@ -449,8 +456,6 @@ void MetaParams::read(QString fname) { qmConfig.insert(QLatin1String("registerlocation"), qsRegLocation); qmConfig.insert(QLatin1String("registerurl"), qurlRegWeb.toString()); qmConfig.insert(QLatin1String("bonjour"), bBonjour ? QLatin1String("true") : QLatin1String("false")); - qmConfig.insert(QLatin1String("certificate"), QString::fromUtf8(qscCert.toPem())); - qmConfig.insert(QLatin1String("key"), QString::fromUtf8(qskKey.toPem())); qmConfig.insert(QLatin1String("obfuscate"), bObfuscate ? QLatin1String("true") : QLatin1String("false")); qmConfig.insert(QLatin1String("username"), qrUserName.pattern()); qmConfig.insert(QLatin1String("channelname"), qrChannelName.pattern()); @@ -466,8 +471,6 @@ void MetaParams::read(QString fname) { qmConfig.insert(QLatin1String("opusthreshold"), QString::number(iOpusThreshold)); qmConfig.insert(QLatin1String("channelnestinglimit"), QString::number(iChannelNestingLimit)); qmConfig.insert(QLatin1String("channelcountlimit"), QString::number(iChannelCountLimit)); - qmConfig.insert(QLatin1String("sslCiphers"), qsCiphers); - qmConfig.insert(QLatin1String("sslDHParams"), QString::fromLatin1(qbaDHParams.constData())); } bool MetaParams::loadSSLSettings() { @@ -484,7 +487,6 @@ bool MetaParams::loadSSLSettings() { qbaPassPhrase = qsSettings->value("sslPassPhrase").toByteArray(); QSslCertificate tmpCert; - QList< QSslCertificate > tmpCA; QList< QSslCertificate > tmpIntermediates; QSslKey tmpKey; QByteArray tmpDHParams; @@ -501,7 +503,7 @@ bool MetaParams::loadSSLSettings() { qCritical("MetaParams: Failed to parse any CA certificates from %s", qPrintable(qsSSLCA)); return false; } else { - tmpCA = ql; + tmpIntermediates << ql; } } else { qCritical("MetaParams: Failed to read %s", qPrintable(qsSSLCA)); @@ -561,7 +563,7 @@ bool MetaParams::loadSSLSettings() { return false; } if (ql.size() > 0) { - tmpIntermediates = ql; + tmpIntermediates << ql; qCritical("MetaParams: Adding %d intermediate certificates from certificate file.", ql.size()); } } @@ -650,18 +652,17 @@ bool MetaParams::loadSSLSettings() { qWarning("MetaParams: TLS cipher preference is \"%s\"", qPrintable(pref.join(QLatin1String(":")))); } - qscCert = tmpCert; - qlCA = tmpCA; - qlIntermediates = tmpIntermediates; - qskKey = tmpKey; - qbaDHParams = tmpDHParams; - qsCiphers = tmpCiphersStr; - qlCiphers = tmpCiphers; - - qmConfig.insert(QLatin1String("certificate"), QString::fromUtf8(qscCert.toPem())); - qmConfig.insert(QLatin1String("key"), QString::fromUtf8(qskKey.toPem())); - qmConfig.insert(QLatin1String("sslCiphers"), qsCiphers); - qmConfig.insert(QLatin1String("sslDHParams"), QString::fromLatin1(qbaDHParams.constData())); + if (!tmpCert.isNull()) { + qlCertificateChain = Server::buildSslChain(tmpCert, tmpIntermediates); + if (qlCertificateChain.isEmpty()) { + qCritical() << "Unable to calculate certificate chain"; + return false; + } + } + qskKey = tmpKey; + qbaDHParams = tmpDHParams; + qsCiphers = tmpCiphersStr; + qlCiphers = tmpCiphers; return true; } @@ -764,10 +765,11 @@ bool Meta::boot(int srvnum) { } } if (r.rlim_cur < sockets) - qCritical( - "Current booted servers require minimum %d file descriptors when all slots are full, but only %lu file " - "descriptors are allowed for this process. Your server will crash and burn; read the FAQ for details.", - sockets, static_cast< unsigned long >(r.rlim_cur)); + qCritical("Current booted servers require minimum %d file descriptors when all slots are full, but " + "only %lu file " + "descriptors are allowed for this process. Your server will crash and burn; read the FAQ for " + "details.", + sockets, static_cast< unsigned long >(r.rlim_cur)); } #endif diff --git a/src/murmur/Meta.h b/src/murmur/Meta.h index 91b48f53559..6dde2fafe19 100644 --- a/src/murmur/Meta.h +++ b/src/murmur/Meta.h @@ -107,23 +107,8 @@ class MetaParams { unsigned int iPluginMessageLimit; unsigned int iPluginMessageBurst; - QSslCertificate qscCert; QSslKey qskKey; - - /// qlIntermediates contains the certificates - /// from PEM bundle pointed to by murmur.ini's - /// sslCert option that do not match the key - /// pointed to by murmur.ini's sslKey option. - /// - /// Simply put: it contains any certificates - /// that aren't the main certificate, or "leaf" - /// certificate. - QList< QSslCertificate > qlIntermediates; - - /// qlCA contains all certificates read from - /// the PEM bundle pointed to by murmur.ini's - /// sslCA option. - QList< QSslCertificate > qlCA; + QList< QSslCertificate > qlCertificateChain; /// qlCiphers contains the list of supported /// cipher suites. diff --git a/src/murmur/Register.cpp b/src/murmur/Register.cpp index 06cee5b21ae..3c2d561c7e5 100644 --- a/src/murmur/Register.cpp +++ b/src/murmur/Register.cpp @@ -111,15 +111,12 @@ void Server::update() { qnr.setHeader(QNetworkRequest::ContentTypeHeader, QLatin1String("text/xml")); QSslConfiguration ssl = qnr.sslConfiguration(); - ssl.setLocalCertificate(qscCert); + ssl.setLocalCertificateChain(qlCertificateChain); ssl.setPrivateKey(qskKey); /* Work around bug in QSslConfiguration */ QList< QSslCertificate > calist = ssl.caCertificates(); calist << QSslConfiguration::defaultConfiguration().caCertificates(); - calist << Meta::mp.qlCA; - calist << Meta::mp.qlIntermediates; - calist << qscCert; ssl.setCaCertificates(calist); ssl.setCiphers(Meta::mp.qlCiphers); diff --git a/src/murmur/Server.cpp b/src/murmur/Server.cpp index 84f8334864a..5c62cf0618e 100644 --- a/src/murmur/Server.cpp +++ b/src/murmur/Server.cpp @@ -1407,47 +1407,9 @@ void Server::newClient() { EnvUtils::setenv("QT_SSL_USE_TEMPORARY_KEYCHAIN", "1"); #endif sock->setPrivateKey(qskKey); - sock->setLocalCertificate(qscCert); - - QSslConfiguration config; -#if QT_VERSION >= QT_VERSION_CHECK(5, 15, 0) - config = sock->sslConfiguration(); - // Qt 5.15 introduced QSslConfiguration::addCaCertificate(s) that should be preferred over the functions in - // QSslSocket - - // Treat the leaf certificate as a root. - // This shouldn't strictly be necessary, - // and is a left-over from early on. - // Perhaps it is necessary for self-signed - // certs? - config.addCaCertificate(qscCert); - - // Add CA certificates specified via - // murmur.ini's sslCA option. - config.addCaCertificates(Meta::mp.qlCA); - - // Add intermediate CAs found in the PEM - // bundle used for this server's certificate. - config.addCaCertificates(qlIntermediates); -#else - // Treat the leaf certificate as a root. - // This shouldn't strictly be necessary, - // and is a left-over from early on. - // Perhaps it is necessary for self-signed - // certs? - sock->addCaCertificate(qscCert); - - // Add CA certificates specified via - // murmur.ini's sslCA option. - sock->addCaCertificates(Meta::mp.qlCA); - - // Add intermediate CAs found in the PEM - // bundle used for this server's certificate. - sock->addCaCertificates(qlIntermediates); - - // Must not get config from socket before setting CA certificates - config = sock->sslConfiguration(); -#endif + sock->setLocalCertificateChain(qlCertificateChain); + + QSslConfiguration config = sock->sslConfiguration(); config.setCiphers(Meta::mp.qlCiphers); #if defined(USE_QSSLDIFFIEHELLMANPARAMETERS) @@ -1670,9 +1632,9 @@ void Server::connectionClosed(QAbstractSocket::SocketError err, const QString &r qhUsers.remove(u->uiSession); qhHostUsers[u->haAddress].remove(u); - quint16 port = (u->saiUdpAddress.ss_family == AF_INET6) - ? (reinterpret_cast< sockaddr_in6 * >(&u->saiUdpAddress)->sin6_port) - : (reinterpret_cast< sockaddr_in * >(&u->saiUdpAddress)->sin_port); + quint16 port = (u->saiUdpAddress.ss_family == AF_INET6) + ? (reinterpret_cast< sockaddr_in6 * >(&u->saiUdpAddress)->sin6_port) + : (reinterpret_cast< sockaddr_in * >(&u->saiUdpAddress)->sin_port); const QPair< HostAddress, quint16 > &key = QPair< HostAddress, quint16 >(u->haAddress, port); qhPeerUsers.remove(key); diff --git a/src/murmur/Server.h b/src/murmur/Server.h index f6e96bb5be6..0c6fcc013cd 100644 --- a/src/murmur/Server.h +++ b/src/murmur/Server.h @@ -149,18 +149,9 @@ class Server : public QThread { QVariant qvSuggestPushToTalk; bool bUsingMetaCert; - QSslCertificate qscCert; + QList< QSslCertificate > qlCertificateChain; QSslKey qskKey; - /// qlIntermediates contains the certificates - /// from this virtual server's certificate PEM - // bundle that do not match the virtual server's - // private key. - /// - /// Simply put: it contains any certificates - /// that aren't the main certificate, or "leaf" - /// certificate. - QList< QSslCertificate > qlIntermediates; #if defined(USE_QSSLDIFFIEHELLMANPARAMETERS) QSslDiffieHellmanParameters qsdhpDHParams; #endif @@ -204,6 +195,8 @@ public slots: /// If a valid RSA, DSA or EC key is found, it is returned. /// If no valid private key is found, a null QSslKey is returned. static QSslKey privateKeyFromPEM(const QByteArray &buf, const QByteArray &pass = QByteArray()); + static QList< QSslCertificate > buildSslChain(const QSslCertificate &leaf, const QList< QSslCertificate > &pool); + static QByteArray chainToPem(const QList< QSslCertificate > &chain); void initializeCert(); const QString getDigest() const;