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 #3523 (partially)

Co-authored-by: Davide Beatrici <[email protected]>
  • Loading branch information
@EPNW-Eric @davidebeatrici
EPNW-Eric and davidebeatrici committed Nov 10, 2021
1 parent 7d67da1 commit 5fd7088
Show file tree
Hide file tree
Showing 6 changed files with 171 additions and 121 deletions.
163 changes: 136 additions & 27 deletions src/murmur/Cert.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,111 @@

#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 object 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 an 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 an OpenSSL X509 object for the pool and add each to the context.
for (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 the operation is successful.
// Since we use the SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR flag, a return value of 2 is acceptable.
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 of
// "maxDerSize" size.
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 encode certificate in DER format.
chain.clear();
break;
}
// i2d_X509() altered our buffer pointer, 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);
// Data from OpenSSL must correspond to a single certificate!
if (ql.size() == 1) {
chain << ql;
} else {
chain.clear();
break;
}
}

// Clean up
free(buffer);
} else {
chain.clear();
}
// Pool certificates were added with the "add0" function (as opposed to "add1"),
// meaning that they are freed when ctx is.
// Same for the stack, which was obtained with "get0".
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;
for (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 +172,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 +183,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 +199,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 +234,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 +323,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());
}
44 changes: 24 additions & 20 deletions src/murmur/Meta.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,8 +397,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 @@ -414,8 +412,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 @@ -432,7 +428,6 @@ bool MetaParams::loadSSLSettings() {
qbaPassPhrase = qsSettings->value("sslPassPhrase").toByteArray();

QSslCertificate tmpCert;
QList< QSslCertificate > tmpCA;
QList< QSslCertificate > tmpIntermediates;
QSslKey tmpKey;
QByteArray tmpDHParams;
Expand All @@ -449,7 +444,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 @@ -509,7 +504,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 @@ -598,15 +593,23 @@ 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()));
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;

// Inserting these data was previously done here and additionally in read().
// Since loadSSLSettings() is always called from read(), it should be sufficient
// to do the insertions only here. It is not sufficient to do it only in read(),
// because loadSSLSettings() might be called from reloadSSLSettings().
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()));
Expand Down Expand Up @@ -712,10 +715,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
Loading

0 comments on commit 5fd7088

Please sign in to comment.