Fix raciness of encryption API

And make it saner by not having one global password that has to be set before encrypting/decrypting, which is as racy and poorly designed as it gets

Fixes #1917 's immediate symtoms, which some potential for other regressions due to the mess that is encrypted persistence currently

src/core/core.cpp

@@ -67,7 +67,6 @@ Core::Core(QThread *CoreThread, Profile& profile) :
     Audio::getInstance();
 
     videobuf = nullptr;
-    encryptionKey = nullptr;
 
     toxTimer = new QTimer(this);
     toxTimer->setSingleShot(true);

src/core/core.h

@@ -81,10 +81,15 @@ public:
     ToxId getSelfId() const; ///< Returns our Tox ID
     QPair<QByteArray, QByteArray> getKeypair() const; ///< Returns our public and private keys
 
+    static std::unique_ptr<TOX_PASS_KEY> createPasskey(const QString &password, uint8_t* salt = nullptr);
+    static QByteArray encryptData(const QByteArray& data, const TOX_PASS_KEY& encryptionKey);
+    static QByteArray encryptData(const QByteArray& data); ///< Uses the default profile's key
+    static QByteArray decryptData(const QByteArray& data, const TOX_PASS_KEY &encryptionKey);
+    static QByteArray decryptData(const QByteArray& data); ///< Uses the default profile's key
+
     VideoSource* getVideoSourceFromCall(int callNumber); ///< Get a call's video source
 
     static bool anyActiveCalls(); ///< true is any calls are currently active (note: a call about to start is not yet active)
-    bool isPasswordSet();
     bool isReady(); ///< Most of the API shouldn't be used until Core is ready, call start() first
 
     void resetCallSources(); ///< Forces to regenerate each call's audio sources
@@ -148,11 +153,6 @@ public slots:
     static bool isGroupCallMicEnabled(int groupId);
     static bool isGroupCallVolEnabled(int groupId);
 
-    void setPassword(const QString &password, uint8_t* salt = nullptr);
-    void clearPassword();
-    QByteArray encryptData(const QByteArray& data);
-    QByteArray decryptData(const QByteArray& data);
-
 signals:
     void connected();
     void disconnected();
@@ -303,8 +303,6 @@ private:
     QMutex messageSendMutex;
     bool ready;
 
-    static TOX_PASS_KEY* encryptionKey; // use the pw's hash as the "pw"
-
     static const int videobufsize;
     static uint8_t* videobuf;
 

src/core/coreencryption.cpp

@@ -38,40 +38,29 @@
 #include <algorithm>
 #include <cassert>
 
-TOX_PASS_KEY* Core::encryptionKey = nullptr;
+std::unique_ptr<TOX_PASS_KEY> Core::createPasskey(const QString& password, uint8_t* salt)
-
-void Core::setPassword(const QString& password, uint8_t* salt)
 {
-    clearPassword();
+    std::unique_ptr<TOX_PASS_KEY> encryptionKey(new TOX_PASS_KEY);
-    if (password.isEmpty())
-        return;
-
-    encryptionKey = new TOX_PASS_KEY;
 
     CString str(password);
     if (salt)
-        tox_derive_key_with_salt(str.data(), str.size(), salt, encryptionKey, nullptr);
+        tox_derive_key_with_salt(str.data(), str.size(), salt, encryptionKey.get(), nullptr);
     else
-        tox_derive_key_from_pass(str.data(), str.size(), encryptionKey, nullptr);
+        tox_derive_key_from_pass(str.data(), str.size(), encryptionKey.get(), nullptr);
+
+    return encryptionKey;
 }
 
-void Core::clearPassword()
+QByteArray Core::encryptData(const QByteArray &data)
 {
-    delete encryptionKey;
+    return encryptData(data, Nexus::getProfile()->getPasskey());
-    encryptionKey = nullptr;
 }
 
-QByteArray Core::encryptData(const QByteArray& data)
+QByteArray Core::encryptData(const QByteArray& data, const TOX_PASS_KEY& encryptionKey)
 {
-    if (!encryptionKey)
-    {
-        qWarning() << "No encryption key set";
-        return QByteArray();
-    }
-
     uint8_t encrypted[data.size() + TOX_PASS_ENCRYPTION_EXTRA_LENGTH];
     if (!tox_pass_key_encrypt(reinterpret_cast<const uint8_t*>(data.data()), data.size(),
-                            encryptionKey, encrypted, nullptr))
+                            &encryptionKey, encrypted, nullptr))
     {
         qWarning() << "Encryption failed";
         return QByteArray();
@@ -79,18 +68,17 @@ QByteArray Core::encryptData(const QByteArray& data)
     return QByteArray(reinterpret_cast<char*>(encrypted), data.size() + TOX_PASS_ENCRYPTION_EXTRA_LENGTH);
 }
 
-QByteArray Core::decryptData(const QByteArray& data)
+QByteArray Core::decryptData(const QByteArray &data)
 {
-    if (!encryptionKey)
+    return decryptData(data, Nexus::getProfile()->getPasskey());
-    {
+}
-        qWarning() << "No encryption key set";
-        return QByteArray();
-    }
 
+QByteArray Core::decryptData(const QByteArray& data, const TOX_PASS_KEY& encryptionKey)
+{
     int sz = data.size() - TOX_PASS_ENCRYPTION_EXTRA_LENGTH;
     uint8_t decrypted[sz];
     if (!tox_pass_key_decrypt(reinterpret_cast<const uint8_t*>(data.data()), data.size(),
-                              encryptionKey, decrypted, nullptr))
+                              &encryptionKey, decrypted, nullptr))
     {
         qWarning() << "Decryption failed";
         return QByteArray();
@@ -98,11 +86,6 @@ QByteArray Core::decryptData(const QByteArray& data)
     return QByteArray(reinterpret_cast<char*>(decrypted), sz);
 }
 
-bool Core::isPasswordSet()
-{
-    return static_cast<bool>(encryptionKey);
-}
-
 QByteArray Core::getSaltFromFile(QString filename)
 {
     QFile file(filename);
@@ -139,7 +122,7 @@ void Core::checkEncryptedHistory()
         return;
     }
 
-    setPassword(Nexus::getProfile()->getPassword(), reinterpret_cast<uint8_t*>(salt.data()));
+    auto passkey = createPasskey(Nexus::getProfile()->getPassword(), reinterpret_cast<uint8_t*>(salt.data()));
 
     QString a(tr("Please enter the password for the chat history for the profile \"%1\".", "used in load() when no hist pw set").arg(Nexus::getProfile()->getName()));
     QString b(tr("The previous password is incorrect; please try again:", "used on retries in load()"));
@@ -147,7 +130,7 @@ void Core::checkEncryptedHistory()
     QString dialogtxt;
 
 
-    if (!exists || HistoryKeeper::checkPassword())
+    if (!exists || HistoryKeeper::checkPassword(*passkey))
         return;
 
     dialogtxt = tr("The chat history password failed. Please try another?", "used only when pw set before load() doesn't work");
@@ -160,17 +143,16 @@ void Core::checkEncryptedHistory()
 
         if (pw.isEmpty())
         {
-            clearPassword();
             Settings::getInstance().setEnableLogging(false);
             HistoryKeeper::resetInstance();
             return;
         }
         else
         {
-            setPassword(pw, reinterpret_cast<uint8_t*>(salt.data()));
+            passkey = createPasskey(pw, reinterpret_cast<uint8_t*>(salt.data()));
         }
 
-        error = exists && !HistoryKeeper::checkPassword();
+        error = exists && !HistoryKeeper::checkPassword(*passkey);
         dialogtxt = a + "\n" + c + "\n" + b;
     } while (error);
 }

src/persistence/db/encrypteddb.cpp

@@ -20,6 +20,8 @@
 #include "encrypteddb.h"
 #include "src/persistence/settings.h"
 #include "src/core/core.h"
+#include "src/nexus.h"
+#include "src/persistence/profile.h"
 
 #include <tox/toxencryptsave.h>
 
@@ -30,6 +32,8 @@
 qint64 EncryptedDb::encryptedChunkSize = 4096;
 qint64 EncryptedDb::plainChunkSize = EncryptedDb::encryptedChunkSize - TOX_PASS_ENCRYPTION_EXTRA_LENGTH;
 
+TOX_PASS_KEY EncryptedDb::decryptionKey;
+
 EncryptedDb::EncryptedDb(const QString &fname, QList<QString> initList) :
     PlainDb(":memory:", initList), fileName(fname)
 {
@@ -90,7 +94,7 @@ bool EncryptedDb::pullFileContent(const QString &fname, QByteArray &buf)
     while (!dbFile.atEnd())
     {
         QByteArray encrChunk = dbFile.read(encryptedChunkSize);
-        buf = Core::getInstance()->decryptData(encrChunk);
+        buf = Core::getInstance()->decryptData(encrChunk, decryptionKey);
         if (buf.size() > 0)
         {
             fileContent += buf;
@@ -154,7 +158,7 @@ void EncryptedDb::appendToEncrypted(const QString &sql)
     encrFile.flush();
 }
 
-bool EncryptedDb::check(const QString &fname)
+bool EncryptedDb::check(const TOX_PASS_KEY &passkey, const QString &fname)
 {
     QFile file(fname);
     file.open(QIODevice::ReadOnly);
@@ -163,9 +167,11 @@ bool EncryptedDb::check(const QString &fname)
     if (file.size() > 0)
     {
         QByteArray encrChunk = file.read(encryptedChunkSize);
-        QByteArray buf = Core::getInstance()->decryptData(encrChunk);
+        QByteArray buf = Core::getInstance()->decryptData(encrChunk, passkey);
         if (buf.size() == 0)
             state = false;
+        else
+            decryptionKey = passkey;
     }
     else
     {

src/persistence/db/encrypteddb.h

@@ -21,6 +21,7 @@
 #define ENCRYPTEDDB_H
 
 #include "plaindb.h"
+#include <tox/toxencryptsave.h>
 
 #include <QList>
 #include <QFile>
@@ -32,7 +33,7 @@ public:
     virtual ~EncryptedDb();
 
     virtual QSqlQuery exec(const QString &query);
-    static bool check(const QString &fname);
+    static bool check(const TOX_PASS_KEY &passkey, const QString &fname);
 
 private:
     bool pullFileContent(const QString& fname, QByteArray &buf);
@@ -45,6 +46,8 @@ private:
     static qint64 plainChunkSize;
     static qint64 encryptedChunkSize;
 
+    static TOX_PASS_KEY decryptionKey; ///< When importing, the decryption key may not be the same as the profile key
+
     qint64 chunkPosition;
     QByteArray buffer;
 };

src/persistence/historykeeper.cpp

@@ -73,13 +73,13 @@ HistoryKeeper *HistoryKeeper::getInstance()
     return historyInstance;
 }
 
-bool HistoryKeeper::checkPassword(int encrypted)
+bool HistoryKeeper::checkPassword(const TOX_PASS_KEY &passkey, int encrypted)
 {
     if (!Settings::getInstance().getEnableLogging() && (encrypted == -1))
         return true;
 
     if ((encrypted == 1) || (encrypted == -1 && Nexus::getProfile()->isEncrypted()))
-        return EncryptedDb::check(getHistoryPath(Nexus::getProfile()->getName(), encrypted));
+        return EncryptedDb::check(passkey, getHistoryPath(Nexus::getProfile()->getName(), encrypted));
 
     return true;
 }

src/persistence/historykeeper.h

@@ -23,6 +23,7 @@
 #include <QMap>
 #include <QList>
 #include <QDateTime>
+#include <tox/toxencryptsave.h>
 
 class GenericDdInterface;
 namespace Db { enum class syncType; }
@@ -51,7 +52,7 @@ public:
     static void resetInstance();
 
     static QString getHistoryPath(QString currentProfile = QString(), int encrypted = -1); // -1 defaults to checking settings, 0 or 1 to specify
-    static bool checkPassword(int encrypted = -1);
+    static bool checkPassword(const TOX_PASS_KEY& passkey, int encrypted = -1);
     static bool isFileExist();
     static void renameHistory(QString from, QString to);
     static bool removeHistory(int encrypted = -1);

src/persistence/profile.cpp

@@ -40,6 +40,8 @@ Profile::Profile(QString name, QString password, bool isNewProfile)
     : name{name}, password{password},
       newProfile{isNewProfile}, isRemoved{false}
 {
+    passkey = *core->createPasskey(password);
+
     Settings& s = Settings::getInstance();
     s.setCurrentProfile(name);
     s.saveGlobal();
@@ -66,6 +68,63 @@ Profile* Profile::loadProfile(QString name, QString password)
         return nullptr;
     }
 
+    // Check password
+    {
+        QString path = Settings::getInstance().getSettingsDirPath() + name + ".tox";
+        QFile saveFile(path);
+        qDebug() << "Loading tox save "<<path;
+
+        if (!saveFile.exists())
+        {
+            qWarning() << "The tox save file "<<path<<" was not found";
+            ProfileLocker::unlock();
+            return nullptr;
+        }
+
+        if (!saveFile.open(QIODevice::ReadOnly))
+        {
+            qCritical() << "The tox save file " << path << " couldn't' be opened";
+            ProfileLocker::unlock();
+            return nullptr;
+        }
+
+        qint64 fileSize = saveFile.size();
+        if (fileSize <= 0)
+        {
+            qWarning() << "The tox save file"<<path<<" is empty!";
+            ProfileLocker::unlock();
+            return nullptr;
+        }
+
+        QByteArray data = saveFile.readAll();
+        if (tox_is_data_encrypted((uint8_t*)data.data()))
+        {
+            if (password.isEmpty())
+            {
+                qCritical() << "The tox save file is encrypted, but we don't have a password!";
+                ProfileLocker::unlock();
+                return nullptr;
+            }
+
+            uint8_t salt[TOX_PASS_SALT_LENGTH];
+            tox_get_salt(reinterpret_cast<uint8_t *>(data.data()), salt);
+            auto tmpkey = *Core::createPasskey(password, salt);
+
+            data = Core::decryptData(data, tmpkey);
+            if (data.isEmpty())
+            {
+                qCritical() << "Failed to decrypt the tox save file";
+                ProfileLocker::unlock();
+                return nullptr;
+            }
+        }
+        else
+        {
+            if (!password.isEmpty())
+                qWarning() << "We have a password, but the tox save file is not encrypted";
+        }
+    }
+
     return new Profile(name, password, false);
 }
 
@@ -205,9 +264,9 @@ QByteArray Profile::loadToxSave()
 
         uint8_t salt[TOX_PASS_SALT_LENGTH];
         tox_get_salt(reinterpret_cast<uint8_t *>(data.data()), salt);
-        core->setPassword(password, salt);
+        passkey = *core->createPasskey(password, salt);
 
-        data = core->decryptData(data);
+        data = core->decryptData(data, passkey);
         if (data.isEmpty())
             qCritical() << "Failed to decrypt the tox save file";
     }
@@ -247,8 +306,8 @@ void Profile::saveToxSave(QByteArray data)
 
     if (!password.isEmpty())
     {
-        core->setPassword(password);
+        passkey = *core->createPasskey(password);
-        data = core->encryptData(data);
+        data = core->encryptData(data, passkey);
         if (data.isEmpty())
         {
             qCritical() << "Failed to encrypt, can't save!";
@@ -350,6 +409,11 @@ QString Profile::getPassword()
     return password;
 }
 
+const TOX_PASS_KEY& Profile::getPasskey()
+{
+    return passkey;
+}
+
 void Profile::restartCore()
 {
     GUI::setEnabled(false); // Core::reset re-enables it
@@ -363,7 +427,7 @@ void Profile::setPassword(QString newPassword)
     QList<HistoryKeeper::HistMessage> oldMessages = HistoryKeeper::exportMessagesDeleteFile();
 
     password = newPassword;
-    core->setPassword(password);
+    passkey = *core->createPasskey(password);
     saveToxSave();
 
     HistoryKeeper::getInstance()->importMessages(oldMessages);

src/persistence/profile.h

@@ -24,6 +24,7 @@
 #include <QVector>
 #include <QString>
 #include <QByteArray>
+#include <tox/toxencryptsave.h>
 
 class Core;
 class QThread;
@@ -51,6 +52,7 @@ public:
     bool checkPassword(); ///< Checks whether the password is valid
     QString getPassword();
     void setPassword(QString newPassword); ///< Changes the encryption password and re-saves everything with it
+    const TOX_PASS_KEY& getPasskey();
 
     QByteArray loadToxSave(); ///< Loads the profile's .tox save from file, unencrypted
     void saveToxSave(); ///< Saves the profile's .tox save, encrypted if needed. Invalid on deleted profiles.
@@ -85,6 +87,7 @@ private:
     Core* core;
     QThread* coreThread;
     QString name, password;
+    TOX_PASS_KEY passkey;
     bool newProfile; ///< True if this is a newly created profile, with no .tox save file yet.
     bool isRemoved; ///< True if the profile has been removed by remove()
     static QVector<QString> profiles;

src/persistence/settingsserializer.cpp

@@ -287,8 +287,8 @@ void SettingsSerializer::save()
     if (!password.isEmpty())
     {
         Core* core = Nexus::getCore();
-        core->setPassword(password);
+        auto passkey = core->createPasskey(password);
-        data = core->encryptData(data);
+        data = core->encryptData(data, *passkey);
     }
 
     f.write(data);
@@ -319,11 +319,14 @@ void SettingsSerializer::readSerialized()
 
         uint8_t salt[TOX_PASS_SALT_LENGTH];
         tox_get_salt(reinterpret_cast<uint8_t *>(data.data()), salt);
-        core->setPassword(password, salt);
+        auto passkey = core->createPasskey(password, salt);
 
-        data = core->decryptData(data);
+        data = core->decryptData(data, *passkey);
         if (data.isEmpty())
+        {
             qCritical() << "Failed to decrypt the settings file";
+            return;
+        }
     }
     else
     {

src/widget/loginscreen.cpp

@@ -179,15 +179,16 @@ void LoginScreen::onLogin()
     Profile* profile = Profile::loadProfile(name, pass);
     if (!profile)
     {
-        // Unknown error
+        if (!ProfileLocker::isLockable(name))
-        QMessageBox::critical(this, tr("Couldn't load this profile"), tr("Couldn't load this profile."));
+        {
-        return;
+            QMessageBox::critical(this, tr("Couldn't load this profile"), tr("Profile already in use. Close other clients."));
-    }
+            return;
-    if (!profile->checkPassword())
+        }
-    {
+        else
-        QMessageBox::critical(this, tr("Couldn't load this profile"), tr("Wrong password."));
+        {
-        delete profile;
+            QMessageBox::critical(this, tr("Couldn't load this profile"), tr("Wrong password."));
-        return;
+            return;
+        }
     }
 
     Nexus& nexus = Nexus::getInstance();