From 260ba0be469353fbf9eef5a47e0bd6c002be8815 Mon Sep 17 00:00:00 2001 From: alex-z Date: Wed, 11 Jan 2023 18:25:01 +0100 Subject: [PATCH] Fix security vulnerability when receiving empty metadataKeys from the server. Signed-off-by: alex-z --- src/libsync/clientsideencryption.cpp | 49 ++++++++++++------- src/libsync/clientsideencryption.h | 1 + src/libsync/propagatedownloadencrypted.cpp | 20 ++++---- .../propagateremotedeleteencrypted.cpp | 5 ++ ...opagateremotedeleteencryptedrootfolder.cpp | 5 ++ src/libsync/propagateuploadencrypted.cpp | 17 +++++-- src/libsync/vfs/cfapi/hydrationjob.cpp | 29 ++++++----- 7 files changed, 80 insertions(+), 46 deletions(-) diff --git a/src/libsync/clientsideencryption.cpp b/src/libsync/clientsideencryption.cpp index f3b0af87b..762dd5b2a 100644 --- a/src/libsync/clientsideencryption.cpp +++ b/src/libsync/clientsideencryption.cpp @@ -1419,6 +1419,12 @@ void FolderMetadata::setupExistingMetadata(const QByteArray& metadata) QJsonDocument metaDataDoc = QJsonDocument::fromJson(metaDataStr.toLocal8Bit()); QJsonObject metadataObj = metaDataDoc.object()["metadata"].toObject(); QJsonObject metadataKeys = metadataObj["metadataKeys"].toObject(); + + if (metadataKeys.isEmpty()) { + qCDebug(lcCse()) << "Could not setup existing metadata with missing metadataKeys!"; + return; + } + QByteArray sharing = metadataObj["sharing"].toString().toLocal8Bit(); QJsonObject files = metaDataDoc.object()["files"].toObject(); @@ -1447,14 +1453,19 @@ void FolderMetadata::setupExistingMetadata(const QByteArray& metadata) // Cool, We actually have the key, we can decrypt the rest of the metadata. qCDebug(lcCse) << "Sharing: " << sharing; if (sharing.size()) { - auto sharingDecrypted = decryptJsonObject(sharing, _metadataKeys.last()); - qCDebug(lcCse) << "Sharing Decrypted" << sharingDecrypted; + const auto metaDataKey = !_metadataKeys.isEmpty() ? _metadataKeys.last() : QByteArray{}; + if (metaDataKey.isEmpty()) { + qCDebug(lcCse) << "Failed to decrypt sharing! Empty metadata key!"; + } else { + auto sharingDecrypted = decryptJsonObject(sharing, metaDataKey); + qCDebug(lcCse) << "Sharing Decrypted" << sharingDecrypted; - //Sharing is also a JSON object, so extract it and populate. - auto sharingDoc = QJsonDocument::fromJson(sharingDecrypted); - auto sharingObj = sharingDoc.object(); - for (auto it = sharingObj.constBegin(), end = sharingObj.constEnd(); it != end; it++) { - _sharing.push_back({it.key(), it.value().toString()}); + // Sharing is also a JSON object, so extract it and populate. + auto sharingDoc = QJsonDocument::fromJson(sharingDecrypted); + auto sharingObj = sharingDoc.object(); + for (auto it = sharingObj.constBegin(), end = sharingObj.constEnd(); it != end; it++) { + _sharing.push_back({it.key(), it.value().toString()}); + } } } else { qCDebug(lcCse) << "Skipping sharing section since it is empty"; @@ -1470,9 +1481,9 @@ void FolderMetadata::setupExistingMetadata(const QByteArray& metadata) file.initializationVector = QByteArray::fromBase64(fileObj["initializationVector"].toString().toLocal8Bit()); //Decrypt encrypted part - QByteArray key = _metadataKeys[file.metadataKey]; + const auto key = _metadataKeys.value(file.metadataKey, {}); auto encryptedFile = fileObj["encrypted"].toString().toLocal8Bit(); - auto decryptedFile = decryptJsonObject(encryptedFile, key); + auto decryptedFile = !key.isEmpty() ? decryptJsonObject(encryptedFile, key) : QByteArray{}; auto decryptedFileDoc = QJsonDocument::fromJson(decryptedFile); auto decryptedFileObj = decryptedFileDoc.object(); @@ -1532,6 +1543,11 @@ QByteArray FolderMetadata::decryptJsonObject(const QByteArray& encryptedMetadata return EncryptionHelper::decryptStringSymmetric(pass, encryptedMetadata); } +bool FolderMetadata::isMetadataSetup() const +{ + return !_metadataKeys.isEmpty(); +} + void FolderMetadata::setupEmptyMetadata() { qCDebug(lcCse) << "Settint up empty metadata"; QByteArray newMetadataPass = EncryptionHelper::generateRandom(16); @@ -1546,6 +1562,11 @@ void FolderMetadata::setupEmptyMetadata() { QByteArray FolderMetadata::encryptedMetadata() { qCDebug(lcCse) << "Generating metadata"; + if (_metadataKeys.isEmpty()) { + qCDebug(lcCse) << "Metadata generation failed! Empty metadata key!"; + return {}; + } + QJsonObject metadataKeys; for (auto it = _metadataKeys.constBegin(), end = _metadataKeys.constEnd(); it != end; it++) { /* @@ -1556,16 +1577,6 @@ QByteArray FolderMetadata::encryptedMetadata() { metadataKeys.insert(QString::number(it.key()), QString(encryptedKey)); } - /* NO SHARING IN V1 - QJsonObject recepients; - for (auto it = _sharing.constBegin(), end = _sharing.constEnd(); it != end; it++) { - recepients.insert(it->first, it->second); - } - QJsonDocument recepientDoc; - recepientDoc.setObject(recepients); - QString sharingEncrypted = encryptJsonObject(recepientDoc.toJson(QJsonDocument::Compact), _metadataKeys.last()); - */ - QJsonObject metadata = { {"metadataKeys", metadataKeys}, // {"sharing", sharingEncrypted}, diff --git a/src/libsync/clientsideencryption.h b/src/libsync/clientsideencryption.h index 552282e38..664e92ad3 100644 --- a/src/libsync/clientsideencryption.h +++ b/src/libsync/clientsideencryption.h @@ -183,6 +183,7 @@ public: void removeEncryptedFile(const EncryptedFile& f); void removeAllEncryptedFiles(); [[nodiscard]] QVector files() const; + [[nodiscard]] bool isMetadataSetup() const; private: diff --git a/src/libsync/propagatedownloadencrypted.cpp b/src/libsync/propagatedownloadencrypted.cpp index 74f671cd7..6543c58ad 100644 --- a/src/libsync/propagatedownloadencrypted.cpp +++ b/src/libsync/propagatedownloadencrypted.cpp @@ -74,17 +74,19 @@ void PropagateDownloadEncrypted::checkFolderEncryptedMetadata(const QJsonDocumen << _item->_instruction << _item->_file << _item->_encryptedFileName; const QString filename = _info.fileName(); auto meta = new FolderMetadata(_propagator->account(), json.toJson(QJsonDocument::Compact)); - const QVector files = meta->files(); + if (meta->isMetadataSetup()) { + const QVector files = meta->files(); - const QString encryptedFilename = _item->_encryptedFileName.section(QLatin1Char('/'), -1); - for (const EncryptedFile &file : files) { - if (encryptedFilename == file.encryptedFilename) { - _encryptedInfo = file; + const QString encryptedFilename = _item->_encryptedFileName.section(QLatin1Char('/'), -1); + for (const EncryptedFile &file : files) { + if (encryptedFilename == file.encryptedFilename) { + _encryptedInfo = file; - qCDebug(lcPropagateDownloadEncrypted) << "Found matching encrypted metadata for file, starting download"; - emit fileMetadataFound(); - return; - } + qCDebug(lcPropagateDownloadEncrypted) << "Found matching encrypted metadata for file, starting download"; + emit fileMetadataFound(); + return; + } + } } emit failed(); diff --git a/src/libsync/propagateremotedeleteencrypted.cpp b/src/libsync/propagateremotedeleteencrypted.cpp index c648d79e6..1d7ae8aa6 100644 --- a/src/libsync/propagateremotedeleteencrypted.cpp +++ b/src/libsync/propagateremotedeleteencrypted.cpp @@ -53,6 +53,11 @@ void PropagateRemoteDeleteEncrypted::slotFolderEncryptedMetadataReceived(const Q FolderMetadata metadata(_propagator->account(), json.toJson(QJsonDocument::Compact), statusCode); + if (!metadata.isMetadataSetup()) { + taskFailed(); + return; + } + qCDebug(PROPAGATE_REMOVE_ENCRYPTED) << "Metadata Received, preparing it for removal of the file"; const QFileInfo info(_propagator->fullLocalPath(_item->_file)); diff --git a/src/libsync/propagateremotedeleteencryptedrootfolder.cpp b/src/libsync/propagateremotedeleteencryptedrootfolder.cpp index 3985f5308..e8d86e308 100644 --- a/src/libsync/propagateremotedeleteencryptedrootfolder.cpp +++ b/src/libsync/propagateremotedeleteencryptedrootfolder.cpp @@ -83,6 +83,11 @@ void PropagateRemoteDeleteEncryptedRootFolder::slotFolderEncryptedMetadataReceiv FolderMetadata metadata(_propagator->account(), json.toJson(QJsonDocument::Compact), statusCode); + if (!metadata.isMetadataSetup()) { + taskFailed(); + return; + } + qCDebug(PROPAGATE_REMOVE_ENCRYPTED_ROOTFOLDER) << "It's a root encrypted folder. Let's remove nested items first."; metadata.removeAllEncryptedFiles(); diff --git a/src/libsync/propagateuploadencrypted.cpp b/src/libsync/propagateuploadencrypted.cpp index b81f45b15..de61df307 100644 --- a/src/libsync/propagateuploadencrypted.cpp +++ b/src/libsync/propagateuploadencrypted.cpp @@ -120,10 +120,19 @@ void PropagateUploadEncrypted::slotFolderEncryptedMetadataError(const QByteArray void PropagateUploadEncrypted::slotFolderEncryptedMetadataReceived(const QJsonDocument &json, int statusCode) { qCDebug(lcPropagateUploadEncrypted) << "Metadata Received, Preparing it for the new file." << json.toVariant(); - // Encrypt File! _metadata = new FolderMetadata(_propagator->account(), json.toJson(QJsonDocument::Compact), statusCode); + if (!_metadata->isMetadataSetup()) { + if (_isFolderLocked) { + connect(this, &PropagateUploadEncrypted::folderUnlocked, this, &PropagateUploadEncrypted::error); + unlockFolder(); + } else { + emit error(); + } + return; + } + QFileInfo info(_propagator->fullLocalPath(_item->_file)); const QString fileName = info.fileName(); @@ -197,15 +206,13 @@ void PropagateUploadEncrypted::slotFolderEncryptedMetadataReceived(const QJsonDo if (statusCode == 404) { auto job = new StoreMetaDataApiJob(_propagator->account(), - _folderId, - _metadata->encryptedMetadata()); + _folderId, _metadata->encryptedMetadata()); connect(job, &StoreMetaDataApiJob::success, this, &PropagateUploadEncrypted::slotUpdateMetadataSuccess); connect(job, &StoreMetaDataApiJob::error, this, &PropagateUploadEncrypted::slotUpdateMetadataError); job->start(); } else { auto job = new UpdateMetadataApiJob(_propagator->account(), - _folderId, - _metadata->encryptedMetadata(), + _folderId, _metadata->encryptedMetadata(), _folderToken); connect(job, &UpdateMetadataApiJob::success, this, &PropagateUploadEncrypted::slotUpdateMetadataSuccess); diff --git a/src/libsync/vfs/cfapi/hydrationjob.cpp b/src/libsync/vfs/cfapi/hydrationjob.cpp index 26c8a19e7..754c2183b 100644 --- a/src/libsync/vfs/cfapi/hydrationjob.cpp +++ b/src/libsync/vfs/cfapi/hydrationjob.cpp @@ -200,23 +200,26 @@ void OCC::HydrationJob::slotCheckFolderEncryptedMetadata(const QJsonDocument &js qCDebug(lcHydration) << "Metadata Received reading" << e2eMangledName(); const QString filename = e2eMangledName(); auto meta = new FolderMetadata(_account, json.toJson(QJsonDocument::Compact)); - const QVector files = meta->files(); - EncryptedFile encryptedInfo = {}; + if (meta->isMetadataSetup()) { + const QVector files = meta->files(); - const QString encryptedFileExactName = e2eMangledName().section(QLatin1Char('/'), -1); - for (const EncryptedFile &file : files) { - if (encryptedFileExactName == file.encryptedFilename) { - EncryptedFile encryptedInfo = file; - encryptedInfo = file; + EncryptedFile encryptedInfo = {}; - qCDebug(lcHydration) << "Found matching encrypted metadata for file, starting download" << _requestId << _folderPath; - _transferDataSocket = _transferDataServer->nextPendingConnection(); - _job = new GETEncryptedFileJob(_account, _remotePath + e2eMangledName(), _transferDataSocket, {}, {}, 0, encryptedInfo, this); + const QString encryptedFileExactName = e2eMangledName().section(QLatin1Char('/'), -1); + for (const EncryptedFile &file : files) { + if (encryptedFileExactName == file.encryptedFilename) { + EncryptedFile encryptedInfo = file; + encryptedInfo = file; - connect(qobject_cast(_job), &GETEncryptedFileJob::finishedSignal, this, &HydrationJob::onGetFinished); - _job->start(); - return; + qCDebug(lcHydration) << "Found matching encrypted metadata for file, starting download" << _requestId << _folderPath; + _transferDataSocket = _transferDataServer->nextPendingConnection(); + _job = new GETEncryptedFileJob(_account, _remotePath + e2eMangledName(), _transferDataSocket, {}, {}, 0, encryptedInfo, this); + + connect(qobject_cast(_job), &GETEncryptedFileJob::finishedSignal, this, &HydrationJob::onGetFinished); + _job->start(); + return; + } } }