From 1ec7774e2f814cdd8e1b0529cf17619a96701b14 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Fri, 16 Sep 2022 18:20:41 +0200 Subject: [PATCH 1/7] validate certificate for E2EE against private key Signed-off-by: Matthieu Gallien --- src/libsync/clientsideencryption.cpp | 33 +++++++++++++++++++++++++--- src/libsync/clientsideencryption.h | 6 ++++- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/libsync/clientsideencryption.cpp b/src/libsync/clientsideencryption.cpp index a9a81b1a218a5..29b56c4b81ced 100644 --- a/src/libsync/clientsideencryption.cpp +++ b/src/libsync/clientsideencryption.cpp @@ -1123,10 +1123,10 @@ void ClientSideEncryption::generateKeyPair(const AccountPtr &account) _privateKey = key; qCInfo(lcCse()) << "Keys generated correctly, sending to server."; - generateCSR(account, localKeyPair); + generateCSR(account, std::move(localKeyPair)); } -void ClientSideEncryption::generateCSR(const AccountPtr &account, EVP_PKEY *keyPair) +void ClientSideEncryption::generateCSR(const AccountPtr &account, PKey keyPair) { // OpenSSL expects const char. auto cnArray = account->davUser().toLocal8Bit(); @@ -1184,11 +1184,38 @@ void ClientSideEncryption::generateCSR(const AccountPtr &account, EVP_PKEY *keyP auto job = new SignPublicKeyApiJob(account, e2eeBaseUrl() + "public-key", this); job->setCsr(output); - connect(job, &SignPublicKeyApiJob::jsonReceived, [this, account](const QJsonDocument& json, int retCode) { + connect(job, &SignPublicKeyApiJob::jsonReceived, [this, account, keyPair = std::move(keyPair)](const QJsonDocument& json, int retCode) { if (retCode == 200) { QString cert = json.object().value("ocs").toObject().value("data").toObject().value("public-key").toString(); _certificate = QSslCertificate(cert.toLocal8Bit(), QSsl::Pem); _publicKey = _certificate.publicKey(); + + const auto publicKeyString = cert.toLocal8Bit(); + Bio serverPublicKeyBio; + BIO_write(serverPublicKeyBio, publicKeyString.constData(), publicKeyString.size()); + const auto serverPublicKey = PKey::readPrivateKey(serverPublicKeyBio); + + Bio certificateBio; + const auto certificatePem = _certificate.toPem(); + BIO_write(certificateBio, certificatePem.constData(), certificatePem.size()); + const auto x509Certificate = X509Certificate::readCertificate(certificateBio); + + if (auto certificateCheckResult = X509_check_private_key(x509Certificate, keyPair) ; !certificateCheckResult) { + std::array buffer; + qCInfo(lcCse()) << "X509_check_private_key" << certificateCheckResult; + + unsigned long lastError = 1; + while (lastError) { + lastError = ERR_get_error(); + qCInfo(lcCse()) << ERR_error_string(lastError, buffer.data()); + } + + forgetSensitiveData(account); + return; + } + + qCInfo(lcCse()) << "received a valid certificate"; + fetchAndValidatePublicKeyFromServer(account); } qCInfo(lcCse()) << retCode; diff --git a/src/libsync/clientsideencryption.h b/src/libsync/clientsideencryption.h index b628492b8f64a..b3f6079fe8ed2 100644 --- a/src/libsync/clientsideencryption.h +++ b/src/libsync/clientsideencryption.h @@ -113,6 +113,10 @@ class OWNCLOUDSYNC_EXPORT StreamingDecryptor }; } +namespace { +class PKey; +} + class OWNCLOUDSYNC_EXPORT ClientSideEncryption : public QObject { Q_OBJECT public: @@ -121,7 +125,7 @@ class OWNCLOUDSYNC_EXPORT ClientSideEncryption : public QObject { private: void generateKeyPair(const AccountPtr &account); - void generateCSR(const AccountPtr &account, EVP_PKEY *keyPair); + void generateCSR(const AccountPtr &account, PKey keyPair); void encryptPrivateKey(const AccountPtr &account); public: From 8ea75f49679224dbd4d3f0be999fe602c5b4751c Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 5 Oct 2022 11:20:02 +0200 Subject: [PATCH 2/7] address review comments from @allexzander Signed-off-by: Matthieu Gallien --- src/libsync/clientsideencryption.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/libsync/clientsideencryption.cpp b/src/libsync/clientsideencryption.cpp index 29b56c4b81ced..fd1f501a248de 100644 --- a/src/libsync/clientsideencryption.cpp +++ b/src/libsync/clientsideencryption.cpp @@ -1190,11 +1190,6 @@ void ClientSideEncryption::generateCSR(const AccountPtr &account, PKey keyPair) _certificate = QSslCertificate(cert.toLocal8Bit(), QSsl::Pem); _publicKey = _certificate.publicKey(); - const auto publicKeyString = cert.toLocal8Bit(); - Bio serverPublicKeyBio; - BIO_write(serverPublicKeyBio, publicKeyString.constData(), publicKeyString.size()); - const auto serverPublicKey = PKey::readPrivateKey(serverPublicKeyBio); - Bio certificateBio; const auto certificatePem = _certificate.toPem(); BIO_write(certificateBio, certificatePem.constData(), certificatePem.size()); From cd30d3645e2161f819827a0827140fc57db16a3d Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 5 Oct 2022 11:21:29 +0200 Subject: [PATCH 3/7] fix review comments from sonarcloud static analyzis Signed-off-by: Matthieu Gallien --- src/libsync/clientsideencryption.cpp | 36 +++++++++++++++------------- src/libsync/clientsideencryption.h | 4 +++- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/libsync/clientsideencryption.cpp b/src/libsync/clientsideencryption.cpp index fd1f501a248de..9bd5919ead9e6 100644 --- a/src/libsync/clientsideencryption.cpp +++ b/src/libsync/clientsideencryption.cpp @@ -196,7 +196,9 @@ namespace { EVP_PKEY_CTX* _ctx = nullptr; }; - class PKey { + } + + class ClientSideEncryption::PKey { public: ~PKey() { @@ -255,6 +257,8 @@ namespace { EVP_PKEY* _pkey = nullptr; }; + namespace + { class X509Certificate { public: ~X509Certificate() @@ -619,7 +623,7 @@ QByteArray decryptStringSymmetric(const QByteArray& key, const QByteArray& data) QByteArray privateKeyToPem(const QByteArray key) { Bio privateKeyBio; BIO_write(privateKeyBio, key.constData(), key.size()); - auto pkey = PKey::readPrivateKey(privateKeyBio); + auto pkey = ClientSideEncryption::PKey::readPrivateKey(privateKeyBio); Bio pemBio; PEM_write_bio_PKCS8PrivateKey(pemBio, pkey, nullptr, nullptr, 0, nullptr, nullptr); @@ -1181,12 +1185,17 @@ void ClientSideEncryption::generateCSR(const AccountPtr &account, PKey keyPair) qCInfo(lcCse()) << "Returning the certificate"; qCInfo(lcCse()) << output; + sendSignRequestCSR(account, std::move(keyPair), output); +} + +void ClientSideEncryption::sendSignRequestCSR(const AccountPtr &account, PKey keyPair, const QByteArray &csrContent) +{ auto job = new SignPublicKeyApiJob(account, e2eeBaseUrl() + "public-key", this); - job->setCsr(output); + job->setCsr(csrContent); connect(job, &SignPublicKeyApiJob::jsonReceived, [this, account, keyPair = std::move(keyPair)](const QJsonDocument& json, int retCode) { if (retCode == 200) { - QString cert = json.object().value("ocs").toObject().value("data").toObject().value("public-key").toString(); + const auto cert = json.object().value("ocs").toObject().value("data").toObject().value("public-key").toString(); _certificate = QSslCertificate(cert.toLocal8Bit(), QSsl::Pem); _publicKey = _certificate.publicKey(); @@ -1195,22 +1204,15 @@ void ClientSideEncryption::generateCSR(const AccountPtr &account, PKey keyPair) BIO_write(certificateBio, certificatePem.constData(), certificatePem.size()); const auto x509Certificate = X509Certificate::readCertificate(certificateBio); - if (auto certificateCheckResult = X509_check_private_key(x509Certificate, keyPair) ; !certificateCheckResult) { - std::array buffer; - qCInfo(lcCse()) << "X509_check_private_key" << certificateCheckResult; - - unsigned long lastError = 1; - while (lastError) { - lastError = ERR_get_error(); - qCInfo(lcCse()) << ERR_error_string(lastError, buffer.data()); + if (const auto certificateCheckResult = X509_check_private_key(x509Certificate, keyPair) ; !certificateCheckResult) { + auto lastError = 1; + while ((lastError= ERR_get_error())) { + qCInfo(lcCse()) << ERR_lib_error_string(lastError); } - forgetSensitiveData(account); return; } - qCInfo(lcCse()) << "received a valid certificate"; - fetchAndValidatePublicKeyFromServer(account); } qCInfo(lcCse()) << retCode; @@ -1497,7 +1499,7 @@ QByteArray FolderMetadata::encryptMetadataKey(const QByteArray& data) const Bio publicKeyBio; QByteArray publicKeyPem = _account->e2e()->_publicKey.toPem(); BIO_write(publicKeyBio, publicKeyPem.constData(), publicKeyPem.size()); - auto publicKey = PKey::readPublicKey(publicKeyBio); + auto publicKey = ClientSideEncryption::PKey::readPublicKey(publicKeyBio); // The metadata key is binary so base64 encode it first return EncryptionHelper::encryptStringAsymmetric(publicKey, data.toBase64()); @@ -1508,7 +1510,7 @@ QByteArray FolderMetadata::decryptMetadataKey(const QByteArray& encryptedMetadat Bio privateKeyBio; QByteArray privateKeyPem = _account->e2e()->_privateKey; BIO_write(privateKeyBio, privateKeyPem.constData(), privateKeyPem.size()); - auto key = PKey::readPrivateKey(privateKeyBio); + auto key = ClientSideEncryption::PKey::readPrivateKey(privateKeyBio); // Also base64 decode the result QByteArray decryptResult = EncryptionHelper::decryptStringAsymmetric( diff --git a/src/libsync/clientsideencryption.h b/src/libsync/clientsideencryption.h index b3f6079fe8ed2..7e7e6d05911b2 100644 --- a/src/libsync/clientsideencryption.h +++ b/src/libsync/clientsideencryption.h @@ -114,18 +114,20 @@ class OWNCLOUDSYNC_EXPORT StreamingDecryptor } namespace { -class PKey; } class OWNCLOUDSYNC_EXPORT ClientSideEncryption : public QObject { Q_OBJECT public: + class PKey; + ClientSideEncryption(); void initialize(const AccountPtr &account); private: void generateKeyPair(const AccountPtr &account); void generateCSR(const AccountPtr &account, PKey keyPair); + void sendSignRequestCSR(const AccountPtr &account, PKey keyPair, const QByteArray &csrContent); void encryptPrivateKey(const AccountPtr &account); public: From b19b69df02c3b7a6ed06434bc5a21159ba22e7c2 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 5 Oct 2022 15:41:31 +0200 Subject: [PATCH 4/7] remove useless lines: fix review comment Signed-off-by: Matthieu Gallien --- src/libsync/clientsideencryption.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/libsync/clientsideencryption.h b/src/libsync/clientsideencryption.h index 7e7e6d05911b2..ec51de61b0c91 100644 --- a/src/libsync/clientsideencryption.h +++ b/src/libsync/clientsideencryption.h @@ -113,9 +113,6 @@ class OWNCLOUDSYNC_EXPORT StreamingDecryptor }; } -namespace { -} - class OWNCLOUDSYNC_EXPORT ClientSideEncryption : public QObject { Q_OBJECT public: From 5e81ab821e932992ffc64cc8337cb79047250adf Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 5 Oct 2022 15:43:56 +0200 Subject: [PATCH 5/7] fix more issues reported by sonarcloud Signed-off-by: Matthieu Gallien --- src/libsync/clientsideencryption.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libsync/clientsideencryption.cpp b/src/libsync/clientsideencryption.cpp index 9bd5919ead9e6..e7b579d626185 100644 --- a/src/libsync/clientsideencryption.cpp +++ b/src/libsync/clientsideencryption.cpp @@ -1193,7 +1193,7 @@ void ClientSideEncryption::sendSignRequestCSR(const AccountPtr &account, PKey ke auto job = new SignPublicKeyApiJob(account, e2eeBaseUrl() + "public-key", this); job->setCsr(csrContent); - connect(job, &SignPublicKeyApiJob::jsonReceived, [this, account, keyPair = std::move(keyPair)](const QJsonDocument& json, int retCode) { + connect(job, &SignPublicKeyApiJob::jsonReceived, [this, account, keyPair = std::move(keyPair)](const QJsonDocument& json, const int retCode) { if (retCode == 200) { const auto cert = json.object().value("ocs").toObject().value("data").toObject().value("public-key").toString(); _certificate = QSslCertificate(cert.toLocal8Bit(), QSsl::Pem); @@ -1205,7 +1205,7 @@ void ClientSideEncryption::sendSignRequestCSR(const AccountPtr &account, PKey ke const auto x509Certificate = X509Certificate::readCertificate(certificateBio); if (const auto certificateCheckResult = X509_check_private_key(x509Certificate, keyPair) ; !certificateCheckResult) { - auto lastError = 1; + auto lastError = 1ul; while ((lastError= ERR_get_error())) { qCInfo(lcCse()) << ERR_lib_error_string(lastError); } From 7ffbd9563699bdec94784abe1eeffa9a746c752f Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 5 Oct 2022 17:27:45 +0200 Subject: [PATCH 6/7] fix more sonarcloud issues Signed-off-by: Matthieu Gallien --- src/libsync/clientsideencryption.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libsync/clientsideencryption.cpp b/src/libsync/clientsideencryption.cpp index e7b579d626185..8607f9866b498 100644 --- a/src/libsync/clientsideencryption.cpp +++ b/src/libsync/clientsideencryption.cpp @@ -1198,14 +1198,12 @@ void ClientSideEncryption::sendSignRequestCSR(const AccountPtr &account, PKey ke const auto cert = json.object().value("ocs").toObject().value("data").toObject().value("public-key").toString(); _certificate = QSslCertificate(cert.toLocal8Bit(), QSsl::Pem); _publicKey = _certificate.publicKey(); - Bio certificateBio; const auto certificatePem = _certificate.toPem(); BIO_write(certificateBio, certificatePem.constData(), certificatePem.size()); const auto x509Certificate = X509Certificate::readCertificate(certificateBio); - if (const auto certificateCheckResult = X509_check_private_key(x509Certificate, keyPair) ; !certificateCheckResult) { - auto lastError = 1ul; + auto lastError = 1UL; while ((lastError= ERR_get_error())) { qCInfo(lcCse()) << ERR_lib_error_string(lastError); } From b9d4e7045ea06ebf496d712036a7a07acdee04ba Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 12 Oct 2022 12:05:45 +0200 Subject: [PATCH 7/7] fix review comments from @allexzander Signed-off-by: Matthieu Gallien --- src/libsync/clientsideencryption.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libsync/clientsideencryption.cpp b/src/libsync/clientsideencryption.cpp index 8607f9866b498..2408b6adec9cd 100644 --- a/src/libsync/clientsideencryption.cpp +++ b/src/libsync/clientsideencryption.cpp @@ -1202,10 +1202,11 @@ void ClientSideEncryption::sendSignRequestCSR(const AccountPtr &account, PKey ke const auto certificatePem = _certificate.toPem(); BIO_write(certificateBio, certificatePem.constData(), certificatePem.size()); const auto x509Certificate = X509Certificate::readCertificate(certificateBio); - if (const auto certificateCheckResult = X509_check_private_key(x509Certificate, keyPair) ; !certificateCheckResult) { - auto lastError = 1UL; - while ((lastError= ERR_get_error())) { + if (!X509_check_private_key(x509Certificate, keyPair)) { + auto lastError = ERR_get_error(); + while (lastError) { qCInfo(lcCse()) << ERR_lib_error_string(lastError); + lastError = ERR_get_error(); } forgetSensitiveData(account); return;