FIX(server): Build the servers certificate chain once instead of abusing the trust store #5312

EP-u-NW · 2021-11-10T18:58:47Z

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.

…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]>

Krzmbrzl

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.

Krzmbrzl · 2022-01-08T12:40:58Z

src/murmur/Cert.cpp

+ 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));


Why did you mark this as unused? That seems a bit odd to me 👀

It's to explicitly ignore the return value.

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 👀

Krzmbrzl · 2022-01-08T12:42:23Z

src/murmur/Cert.cpp

+ for (const QSslCertificate &cert : pool) {
+ QByteArray qbaCert = cert.toDer();
+ int s = qbaCert.size();
+ maxDerSize = maxDerSize < s ? s : maxDerSize;


Suggested change

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

Krzmbrzl · 2022-01-08T12:48:34Z

src/murmur/Cert.cpp

+ // 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);


This might be more elegant:

Suggested change

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.

One-liner:

std::vector< uint8_t > buffer(maxDerSize);

Krzmbrzl · 2022-01-08T12:49:56Z

src/murmur/Cert.cpp

+ // i2d_X509() altered our buffer pointer, we need to set it back manually.
+ buffer -= actualSize;


yuck - is there no way to avoid this in the first place?

From OpenSSL's documentation page:

Krzmbrzl

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.

Krzmbrzl · 2022-01-08T12:52:09Z

src/murmur/Cert.cpp

+ SSL_CTX_free(ctx);
+ X509_free(leaf_x509);
+
+ // Drain OpenSSL's per-thread error queue, see below!


see where? and what?

Krzmbrzl · 2022-01-08T12:56:29Z

src/murmur/Cert.cpp

+ tmpCrt = c;
 ql.removeAt(i);


don't we want to break once we have found the cert?

Krzmbrzl · 2022-01-08T12:58:39Z

src/murmur/Cert.cpp

-
- if (!qscCert.isNull() && !qskKey.isNull()) {
+ if (!qlCertificateChain.isEmpty() && !qlCertificateChain[0].isNull()
+ && issuer.startsWith(QString::fromUtf8("Murmur Autogenerated Certificate"))


Suggested change

&& 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

Just for reference, QString::fromLatin1() or QLatin1String() should've been used in this case.

Krzmbrzl · 2022-01-08T13:01:36Z

src/murmur/Cert.cpp

 log("Certificate or key generation failed");
 }

- setConf("certificate", qscCert.toPem());
+ setConf("certificate", chainToPem(qlCertificateChain));


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

Krzmbrzl · 2022-01-08T13:04:45Z

src/murmur/Meta.cpp

- qlCiphers = tmpCiphers;
-
- qmConfig.insert(QLatin1String("certificate"), QString::fromUtf8(qscCert.toPem()));
+ if (!tmpCert.isNull()) {


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? 🤔

Krzmbrzl · 2022-01-08T13:05:23Z

src/murmur/Meta.cpp

+ if (!tmpCert.isNull()) {
+ qlCertificateChain = Server::buildSslChain(tmpCert, tmpIntermediates);
+ if (qlCertificateChain.isEmpty()) {
+ qCritical() << "Unable to calculate certificate chain";


Suggested change

qCritical() << "Unable to calculate certificate chain";

qCritical() << "Unable to build certificate chain";

Krzmbrzl · 2022-01-08T13:08:18Z

src/murmur/Register.cpp

 /* 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);


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.

Krzmbrzl · 2022-01-08T13:11:30Z

Note that I have force-pushed your branch to fix some minor formatting issues

Krzmbrzl force-pushed the build-server-chain-once branch from 5fd7088 to 0779084 Compare January 8, 2022 12:38

Krzmbrzl reviewed Jan 8, 2022

View reviewed changes

Krzmbrzl requested changes Jan 8, 2022

View reviewed changes

Krzmbrzl added bug A bug (error) in the software server labels Jan 8, 2022

Krzmbrzl marked this pull request as draft January 1, 2024 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX(server): Build the servers certificate chain once instead of abusing the trust store #5312

FIX(server): Build the servers certificate chain once instead of abusing the trust store #5312

EP-u-NW commented Nov 10, 2021

Krzmbrzl left a comment

Krzmbrzl Jan 8, 2022

davidebeatrici Jan 8, 2022

Krzmbrzl Jan 8, 2022

Krzmbrzl Jan 8, 2022

Krzmbrzl Jan 8, 2022

davidebeatrici Jan 8, 2022

Krzmbrzl Jan 8, 2022

davidebeatrici Jan 8, 2022

Krzmbrzl left a comment

Krzmbrzl Jan 8, 2022

Krzmbrzl Jan 8, 2022

Krzmbrzl Jan 8, 2022

davidebeatrici Jan 8, 2022

Krzmbrzl Jan 8, 2022

Krzmbrzl Jan 8, 2022

Krzmbrzl Jan 8, 2022

Krzmbrzl Jan 8, 2022

Krzmbrzl commented Jan 8, 2022

	maxDerSize = maxDerSize < s ? s : maxDerSize;
	maxDerSize = std::max(maxDerSize, s);

	unsigned char buffer = (unsigned char ) malloc(maxDerSize);
	std::vector< unsigned char > buffer;
	buffer.resize(maxDerSize);

		// i2d_X509() altered our buffer pointer, we need to set it back manually.
		buffer -= actualSize;

	&& issuer.startsWith(QString::fromUtf8("Murmur Autogenerated Certificate"))
	&& issuer.startsWith("Murmur Autogenerated Certificate")

	qCritical() << "Unable to calculate certificate chain";
	qCritical() << "Unable to build certificate chain";

FIX(server): Build the servers certificate chain once instead of abusing the trust store #5312

Are you sure you want to change the base?

FIX(server): Build the servers certificate chain once instead of abusing the trust store #5312

Conversation

EP-u-NW commented Nov 10, 2021

Krzmbrzl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krzmbrzl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krzmbrzl commented Jan 8, 2022