Store secrets (apart from the pickle key) in the database

src/Cache.cpp

@@ -39,7 +39,7 @@
 
 //! Should be changed when a breaking change occurs in the cache format.
 //! This will reset client's data.
-static const std::string CURRENT_CACHE_FORMAT_VERSION{"2022.04.08"};
+static const std::string CURRENT_CACHE_FORMAT_VERSION{"2022.11.06"};
 
 //! Keys used for the DB
 static const std::string_view NEXT_BATCH_KEY("next_batch");
@@ -340,13 +340,13 @@ Cache::setup()
 
     txn.commit();
 
-    loadSecrets({
+    loadSecretsFromStore(
-      {mtx::secret_storage::secrets::cross_signing_master, false},
+      {
-      {mtx::secret_storage::secrets::cross_signing_self_signing, false},
+        {"pickle_secret", true},
-      {mtx::secret_storage::secrets::cross_signing_user_signing, false},
+      },
-      {mtx::secret_storage::secrets::megolm_backup_v1, false},
+      [this](const std::string &, bool, const std::string &value) {
-      {"pickle_secret", true},
+          this->pickle_secret_ = value;
-    });
+      });
 }
 
 static void
@@ -380,7 +380,9 @@ secretName(std::string name, bool internal)
 }
 
 void
-Cache::loadSecrets(std::vector<std::pair<std::string, bool>> toLoad)
+Cache::loadSecretsFromStore(
+  std::vector<std::pair<std::string, bool>> toLoad,
+  std::function<void(const std::string &name, bool internal, const std::string &value)> callback)
 {
     auto settings = UserSettings::instance()->qsettings();
 
@@ -398,12 +400,11 @@ Cache::loadSecrets(std::vector<std::pair<std::string, bool>> toLoad)
             if (value.isEmpty()) {
                 nhlog::db()->info("Restored empty secret '{}'.", name.toStdString());
             } else {
-                std::unique_lock lock(secret_storage.mtx);
+                callback(name_, internal, value.toStdString());
-                secret_storage.secrets[name.toStdString()] = value.toStdString();
             }
         }
         // if we emit the databaseReady signal directly it won't be received
-        QTimer::singleShot(0, this, [this] { loadSecrets({}); });
+        QTimer::singleShot(0, this, [this, callback] { loadSecretsFromStore({}, callback); });
         return;
     }
 
@@ -419,7 +420,7 @@ Cache::loadSecrets(std::vector<std::pair<std::string, bool>> toLoad)
     connect(job,
             &QKeychain::ReadPasswordJob::finished,
             this,
-            [this, name, toLoad, job](QKeychain::Job *) mutable {
+            [this, name, toLoad, job, name_, internal, callback](QKeychain::Job *) mutable {
                 nhlog::db()->debug("Finished reading '{}'", toLoad.begin()->first);
                 const QString secret = job->textData();
                 if (job->error() && job->error() != QKeychain::Error::EntryNotFound) {
@@ -433,40 +434,72 @@ Cache::loadSecrets(std::vector<std::pair<std::string, bool>> toLoad)
                 if (secret.isEmpty()) {
                     nhlog::db()->debug("Restored empty secret '{}'.", name.toStdString());
                 } else {
-                    std::unique_lock lock(secret_storage.mtx);
+                    callback(name_, internal, secret.toStdString());
-                    secret_storage.secrets[name.toStdString()] = secret.toStdString();
                 }
 
                 // load next secret
                 toLoad.erase(toLoad.begin());
 
                 // You can't start a job from the finish signal of a job.
-                QTimer::singleShot(0, this, [this, toLoad] { loadSecrets(toLoad); });
+                QTimer::singleShot(
+                  0, this, [this, toLoad, callback] { loadSecretsFromStore(toLoad, callback); });
             });
     nhlog::db()->debug("Reading '{}'", name_);
     job->start();
 }
 
 std::optional<std::string>
-Cache::secret(const std::string name_, bool internal)
+Cache::secret(const std::string &name_, bool internal)
 {
     auto name = secretName(name_, internal);
-    std::unique_lock lock(secret_storage.mtx);
+
-    if (auto secret = secret_storage.secrets.find(name.toStdString());
+    auto txn = ro_txn(env_);
-        secret != secret_storage.secrets.end())
+    std::string_view value;
-        return secret->second;
+    auto db_name = "secret." + name.toStdString();
-    else
+    if (!syncStateDb_.get(txn, db_name, value))
         return std::nullopt;
+
+    mtx::secret_storage::AesHmacSha2EncryptedData data = nlohmann::json::parse(value);
+
+    auto decrypted = mtx::crypto::decrypt(data, mtx::crypto::to_binary_buf(pickle_secret_), name_);
+    if (decrypted.empty())
+        return std::nullopt;
+    else
+        return decrypted;
 }
 
 void
-Cache::storeSecret(const std::string name_, const std::string secret, bool internal)
+Cache::storeSecret(const std::string &name_, const std::string &secret, bool internal)
 {
     auto name = secretName(name_, internal);
-    {
+
-        std::unique_lock lock(secret_storage.mtx);
+    auto txn = lmdb::txn::begin(env_);
-        secret_storage.secrets[name.toStdString()] = secret;
+
-    }
+    auto encrypted =
+      mtx::crypto::encrypt(secret, mtx::crypto::to_binary_buf(pickle_secret_), name_);
+
+    auto db_name = "secret." + name.toStdString();
+    syncStateDb_.put(txn, db_name, nlohmann::json(encrypted).dump());
+    txn.commit();
+    emit secretChanged(name_);
+}
+
+void
+Cache::deleteSecret(const std::string &name_, bool internal)
+{
+    auto name = secretName(name_, internal);
+
+    auto txn = lmdb::txn::begin(env_);
+    std::string_view value;
+    auto db_name = "secret." + name.toStdString();
+    syncStateDb_.del(txn, db_name, value);
+    txn.commit();
+}
+
+void
+Cache::storeSecretInStore(const std::string name_, const std::string secret)
+{
+    auto name = secretName(name_, true);
 
     auto settings = UserSettings::instance()->qsettings();
     if (settings->value(QStringLiteral("run_without_secure_secrets_service"), false).toBool()) {
@@ -507,13 +540,9 @@ Cache::storeSecret(const std::string name_, const std::string secret, bool inter
 }
 
 void
-Cache::deleteSecret(const std::string name, bool internal)
+Cache::deleteSecretFromStore(const std::string name, bool internal)
 {
     auto name_ = secretName(name, internal);
-    {
-        std::unique_lock lock(secret_storage.mtx);
-        secret_storage.secrets.erase(name_.toStdString());
-    }
 
     auto settings = UserSettings::instance()->qsettings();
     if (settings->value(QStringLiteral("run_without_secure_secrets_service"), false).toBool()) {
@@ -539,13 +568,8 @@ std::string
 Cache::pickleSecret()
 {
     if (pickle_secret_.empty()) {
-        auto s = secret("pickle_secret", true);
+        this->pickle_secret_ = mtx::client::utils::random_token(64, true);
-        if (!s) {
+        storeSecretInStore("pickle_secret", pickle_secret_);
-            this->pickle_secret_ = mtx::client::utils::random_token(64, true);
-            storeSecret("pickle_secret", pickle_secret_, true);
-        } else {
-            this->pickle_secret_ = *s;
-        }
     }
 
     return pickle_secret_;
@@ -1179,11 +1203,7 @@ Cache::deleteData()
             nhlog::db()->info("deleted cache files from disk");
         }
 
-        deleteSecret(mtx::secret_storage::secrets::megolm_backup_v1);
+        deleteSecretFromStore("pickle_secret", true);
-        deleteSecret(mtx::secret_storage::secrets::cross_signing_master);
-        deleteSecret(mtx::secret_storage::secrets::cross_signing_user_signing);
-        deleteSecret(mtx::secret_storage::secrets::cross_signing_self_signing);
-        deleteSecret("pickle_secret", true);
     }
 }
 
@@ -1392,7 +1412,8 @@ Cache::runMigrations()
        }},
       {"2021.08.31",
        [this]() {
-           storeSecret("pickle_secret", "secret", true);
+           storeSecretInStore("pickle_secret", "secret");
+           this->pickle_secret_ = "secret";
            return true;
        }},
       {"2022.04.08",
@@ -1448,6 +1469,22 @@ Cache::runMigrations()
                return false;
            }
        }},
+      {"2022.11.06",
+       [this]() {
+           loadSecretsFromStore(
+             {
+               {mtx::secret_storage::secrets::cross_signing_master, false},
+               {mtx::secret_storage::secrets::cross_signing_self_signing, false},
+               {mtx::secret_storage::secrets::cross_signing_user_signing, false},
+               {mtx::secret_storage::secrets::megolm_backup_v1, false},
+             },
+             [this](const std::string &name, bool internal, const std::string &value) {
+                 this->storeSecret(name, value, internal);
+                 QTimer::singleShot(
+                   0, this, [this, name, internal] { deleteSecretFromStore(name, internal); });
+             });
+           return true;
+       }},
     };
 
     nhlog::db()->info("Running migrations, this may take a while!");

src/Cache_p.h

@@ -291,9 +291,9 @@ public:
     void deleteBackupVersion();
     std::optional<OnlineBackupVersion> backupVersion();
 
-    void storeSecret(const std::string name, const std::string secret, bool internal = false);
+    void storeSecret(const std::string &name, const std::string &secret, bool internal = false);
-    void deleteSecret(const std::string name, bool internal = false);
+    void deleteSecret(const std::string &name, bool internal = false);
-    std::optional<std::string> secret(const std::string name, bool internal = false);
+    std::optional<std::string> secret(const std::string &name, bool internal = false);
 
     std::string pickleSecret();
 
@@ -324,7 +324,12 @@ signals:
     void databaseReady();
 
 private:
-    void loadSecrets(std::vector<std::pair<std::string, bool>> toLoad);
+    void loadSecretsFromStore(
+      std::vector<std::pair<std::string, bool>> toLoad,
+      std::function<void(const std::string &name, bool internal, const std::string &value)>
+        callback);
+    void storeSecretInStore(const std::string name, const std::string secret);
+    void deleteSecretFromStore(const std::string name, bool internal);
 
     //! Save an invited room.
     void saveInvite(lmdb::txn &txn,
@@ -685,7 +690,6 @@ private:
     std::string pickle_secret_;
 
     VerificationStorage verification_storage;
-    SecretsStorage secret_storage;
 
     bool databaseReady_ = false;
 };

src/ChatPage.cpp

@@ -384,7 +384,8 @@ ChatPage::dropToLoginPage(const QString &msg)
       tr("Because of the following reason Nheko wants to drop you to the login page:\n%1\nIf you "
          "think this is a mistake, you can close Nheko instead to possibly recover your encrpytion "
          "keys. After you have been dropped to the login page, you can sign in again using your "
-         "usual methods."),
+         "usual methods.")
+        .arg(msg),
       QMessageBox::StandardButton::Close | QMessageBox::StandardButton::Ok,
       QMessageBox::StandardButton::Ok);
     if (btn == QMessageBox::StandardButton::Close) {

src/encryption/Olm.cpp

@@ -6,6 +6,7 @@
 #include "Olm.h"
 
 #include <QObject>
+#include <QRandomGenerator>
 #include <QTimer>
 
 #include <nlohmann/json.hpp>
@@ -98,13 +99,19 @@ handle_secret_request(const mtx::events::DeviceEvent<mtx::events::msg::SecretReq
         return;
     secretSend.content.secret = secret.value();
 
-    send_encrypted_to_device_messages(
+    // Randomly delay reply to workaround olm session generation races
-      {{local_user.to_string(), {{e->content.requesting_device_id}}}}, secretSend);
+    QTimer::singleShot(QRandomGenerator::global()->bounded(0, 3000),
+                       ChatPage::instance(),
+                       [local_user, e = *e, secretSend] {
+                           send_encrypted_to_device_messages(
+                             {{local_user.to_string(), {{e.content.requesting_device_id}}}},
+                             secretSend);
 
-    nhlog::net()->info("Sent secret '{}' to ({},{})",
+                           nhlog::net()->info("Sent secret '{}' to ({},{})",
-                       e->content.name,
+                                              e.content.name,
-                       local_user.to_string(),
+                                              local_user.to_string(),
-                       e->content.requesting_device_id);
+                                              e.content.requesting_device_id);
+                       });
 }
 
 void