FIX(server): Build the servers certificate chain once instead of abusing the trust store #5312
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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.
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. 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. 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. yuck - is there no way to avoid this in the first place? There was a problem hiding this comment. 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! | ||||||||
|
|
||||||||
| 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. | ||||||||
| 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(); | ||||||||
|
|
||||||||
|
|
||||||||
|
|
||||||||
| // 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)) { | ||||||||
| tmpCrt = c; | ||||||||
| ql.removeAt(i); | ||||||||
|
Comment on lines
+209
to
210
There was a problem hiding this comment. don't we want to |
||||||||
| } | ||||||||
| } | ||||||||
| if (!tmpCrt.isNull()) { | ||||||||
| qlCertificateChain = buildSslChain(tmpCrt, ql); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| #if defined(USE_QSSLDIFFIEHELLMANPARAMETERS) | ||||||||
|
|
@@ -132,57 +234,62 @@ void Server::initializeCert() { | |||||||
|
|
||||||||
| QString issuer; | ||||||||
|
|
||||||||
| 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"); | ||||||||
| 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 (!qlCertificateChain.isEmpty() && !qlCertificateChain[0].isNull() | ||||||||
| && issuer.startsWith(QString::fromUtf8("Murmur Autogenerated Certificate")) | ||||||||
|
There was a problem hiding this comment.
Suggested change
We allow direct conversion from string literals to QString in our codebase now There was a problem hiding this comment. 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 (qlCertificateChain.isEmpty() || qlCertificateChain[0].isNull() || qskKey.isNull()) { | ||||||||
| if (!key.isEmpty() || !crt.isEmpty()) { | ||||||||
| log("Certificate specified, but failed to load."); | ||||||||
| } | ||||||||
|
|
||||||||
| qlCertificateChain.clear(); | ||||||||
| qlCertificateChain = Meta::mp.qlCertificateChain; | ||||||||
| qskKey = Meta::mp.qskKey; | ||||||||
|
|
||||||||
| if (!qlCertificateChain.isEmpty() && !qlCertificateChain[0].isNull() && !qskKey.isNull()) { | ||||||||
| bUsingMetaCert = true; | ||||||||
| } | ||||||||
|
|
||||||||
| // If loading from Meta doesn't work, build+sign a new one | ||||||||
| if (qlCertificateChain.isEmpty() || qlCertificateChain[0].isNull() || qskKey.isNull()) { | ||||||||
| log("Generating new server certificate."); | ||||||||
| if (qlCertificateChain.isEmpty()) { | ||||||||
| qlCertificateChain << QSslCertificate(); | ||||||||
| } | ||||||||
| if (!SelfSignedCertificate::generateMurmurV2Certificate(qlCertificateChain[0], qskKey)) { | ||||||||
| log("Certificate or key generation failed"); | ||||||||
| } | ||||||||
|
|
||||||||
| setConf("certificate", chainToPem(qlCertificateChain)); | ||||||||
|
There was a problem hiding this comment. 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 qlCertificateChain.isEmpty() | ||||||||
| ? QString() | ||||||||
| : QString::fromLatin1(qlCertificateChain[0].digest(QCryptographicHash::Sha1).toHex()); | ||||||||
| } | ||||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,23 +107,8 @@ class MetaParams { | |
| unsigned int iPluginMessageLimit; | ||
| unsigned int iPluginMessageBurst; | ||
|
|
||
| QSslKey qskKey; | ||
| QList< QSslCertificate > qlCertificateChain; | ||
|
|
||
| /// qlCiphers contains the list of supported | ||
| /// cipher suites. | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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.setLocalCertificateChain(qlCertificateChain); | ||
| ssl.setPrivateKey(qskKey); | ||
|
|
||
| /* Work around bug in QSslConfiguration */ | ||
| QList< QSslCertificate > calist = ssl.caCertificates(); | ||
| calist << QSslConfiguration::defaultConfiguration().caCertificates(); | ||
| ssl.setCaCertificates(calist); | ||
|
Comment on lines
117
to
120
There was a problem hiding this comment. At this point, I have the feeling that the remaining code doesn't actually do anything anymore 👀 |
||
|
|
||
| ssl.setCiphers(Meta::mp.qlCiphers); | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 👀