-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX(server): Build the servers certificate chain once instead of abusing the trust store #5312
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -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; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This seems to also require an include of |
||||||||
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); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be more elegant:
Suggested change
and then access the raw buffer with Why use this instead of raw pointers? Mostly due to stylistic reasons (when possible we want to avoid using raw pointers) and also it will be easier for any future developer editing this code, as they won't have to worry about whether they might introduce a path (e.g. early return) that might cause a memory leak. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One-liner: std::vector< uint8_t > buffer(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; | ||||||||
Comment on lines
+85
to
+86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yuck - is there no way to avoid this in the first place? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From OpenSSL's documentation page: |
||||||||
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! | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see where? and what? |
||||||||
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; | ||||||||
|
@@ -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(); | ||||||||
|
@@ -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()) { | ||||||||
|
@@ -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); | ||||||||
Comment on lines
+209
to
210
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't we want to |
||||||||
} | ||||||||
} | ||||||||
qlIntermediates = ql; | ||||||||
if (!tmpCrt.isNull()) { | ||||||||
qlCertificateChain = buildSslChain(tmpCrt, ql); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
#if defined(USE_QSSLDIFFIEHELLMANPARAMETERS) | ||||||||
|
@@ -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")) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We allow direct conversion from string literals to QString in our codebase now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for reference, |
||||||||
&& !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)); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure if it is a good idea to reuse the same config-name for a different thing. Maybe it would be better if we used |
||||||||
setConf("key", qskKey.toPem()); | ||||||||
} | ||||||||
} | ||||||||
|
@@ -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()); | ||||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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()); | ||||||
|
@@ -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() { | ||||||
|
@@ -432,7 +428,6 @@ bool MetaParams::loadSSLSettings() { | |||||
qbaPassPhrase = qsSettings->value("sslPassPhrase").toByteArray(); | ||||||
|
||||||
QSslCertificate tmpCert; | ||||||
QList< QSslCertificate > tmpCA; | ||||||
QList< QSslCertificate > tmpIntermediates; | ||||||
QSslKey tmpKey; | ||||||
QByteArray tmpDHParams; | ||||||
|
@@ -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)); | ||||||
|
@@ -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()); | ||||||
} | ||||||
} | ||||||
|
@@ -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()) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I see it, |
||||||
qlCertificateChain = Server::buildSslChain(tmpCert, tmpIntermediates); | ||||||
if (qlCertificateChain.isEmpty()) { | ||||||
qCritical() << "Unable to calculate certificate chain"; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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())); | ||||||
|
@@ -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 | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
Comment on lines
117
to
120
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point, I have the feeling that the remaining code doesn't actually do anything anymore 👀 |
||
|
||
ssl.setCiphers(Meta::mp.qlCiphers); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you mark this as unused? That seems a bit odd to me 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's to explicitly ignore the return value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that is not necessary, is it? This is usually only required to mark an unused variable and we don't have a variable here 👀