-
-
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?
Conversation
…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) Co-authored-by: Davide Beatrici <[email protected]>
5fd7088
to
0779084
Compare
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.
The error branches in the new code in Cert.cpp should probably log a warning stating what went wrong. Otherwise it might be hard to figure out why something didn't end up working as expected.
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)); |
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 👀
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 comment
The reason will be displayed to describe this comment to others. Learn more.
maxDerSize = maxDerSize < s ? s : maxDerSize; | |
maxDerSize = std::max(maxDerSize, s); |
This seems to also require an include of <algorithm>
at the top of the file
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
This might be more elegant:
unsigned char *buffer = (unsigned char *) malloc(maxDerSize); | |
std::vector< unsigned char > buffer; | |
buffer.resize(maxDerSize); |
and then access the raw buffer with buffer.data()
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 comment
The reason will be displayed to describe this comment to others. Learn more.
One-liner:
std::vector< uint8_t > buffer(maxDerSize);
// i2d_X509() altered our buffer pointer, we need to set it back manually. | ||
buffer -= actualSize; |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
From OpenSSL's documentation page:
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.
Did you choose to store the chain in a QList
instead of a QVector
for a particular reason (e.g. because the Qt API always uses that)? If not, the latter might generally be preferrable to the former.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
see where? and what?
tmpCrt = c; | ||
ql.removeAt(i); |
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.
don't we want to break
once we have found the cert?
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
&& issuer.startsWith(QString::fromUtf8("Murmur Autogenerated Certificate")) | |
&& issuer.startsWith("Murmur Autogenerated Certificate") |
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Just for reference, QString::fromLatin1()
or QLatin1String()
should've been used in this case.
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 comment
The 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 certificate-chain
as the config key
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 comment
The reason will be displayed to describe this comment to others. Learn more.
As I see it, tmpCert
will always be null at this point, because it has only been default constructed but never used in any way (that I can see) before this check. Could this be a bug? 🤔
if (!tmpCert.isNull()) { | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
qCritical() << "Unable to calculate certificate chain"; | |
qCritical() << "Unable to build certificate chain"; |
/* 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); |
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.
At this point, I have the feeling that the remaining code doesn't actually do anything anymore 👀
If that is indeed the case, let's completely remove it.
Note that I have force-pushed your branch to fix some minor formatting issues |
The commit in this PR is an exceed from #5119 and part of the ssl overhault; for an in deep discussion, see there. For more information about this commit itself, see the commit's message.