Skip to content

Commit

Permalink
FIX(server): Build the servers certificate chain once instead of abus…
Browse files Browse the repository at this point in the history 
…ing 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 mumble-voip#3523 (partially)
  • Loading branch information
@EPNW-Eric
EPNW-Eric committed Jun 20, 2021
1 parent cda34be commit f211492
Show file tree
Hide file tree
Showing 6 changed files with 170 additions and 124 deletions.
161 changes: 134 additions & 27 deletions src/murmur/Cert.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,109 @@

#include <openssl/err.h>
#include <openssl/evp.h>
#include <openssl/ssl.h>
#include <openssl/x509.h>

#ifdef Q_OS_WIN
# include <winsock2.h>
#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;
Expand Down Expand Up @@ -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();
Expand All @@ -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()) {
Expand All @@ -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)
Expand All @@ -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());
}
}
Expand Down Expand Up @@ -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());
}
48 changes: 25 additions & 23 deletions src/murmur/Meta.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(" "));
Expand All @@ -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());
Expand All @@ -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() {
Expand All @@ -484,7 +487,6 @@ bool MetaParams::loadSSLSettings() {
qbaPassPhrase = qsSettings->value("sslPassPhrase").toByteArray();

QSslCertificate tmpCert;
QList< QSslCertificate > tmpCA;
QList< QSslCertificate > tmpIntermediates;
QSslKey tmpKey;
QByteArray tmpDHParams;
Expand All @@ -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));
Expand Down Expand Up @@ -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());
}
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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

Expand Down
17 changes: 1 addition & 16 deletions src/murmur/Meta.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
5 changes: 1 addition & 4 deletions src/murmur/Register.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit f211492

Please sign in to comment.