Merge branch 'release/2.3.1' into develop

src/CMakeLists.txt

@@ -152,6 +152,7 @@ set(keepassx_SOURCES
     gui/group/EditGroupWidget.cpp
     gui/group/GroupModel.cpp
     gui/group/GroupView.cpp
+    keys/ChallengeResponseKey.h
     keys/CompositeKey.cpp
     keys/drivers/YubiKey.h
     keys/FileKey.cpp

src/core/Database.cpp

@@ -320,7 +320,12 @@ bool Database::verifyKey(const CompositeKey& key) const
     return (m_data.key.rawKey() == key.rawKey());
 }
 
-QVariantMap Database::publicCustomData() const
+QVariantMap& Database::publicCustomData()
+{
+    return m_data.publicCustomData;
+}
+
+const QVariantMap& Database::publicCustomData() const
 {
     return m_data.publicCustomData;
 }

src/core/Database.h

@@ -105,7 +105,8 @@ public:
                 bool updateTransformSalt = false);
     bool hasKey() const;
     bool verifyKey(const CompositeKey& key) const;
-    QVariantMap publicCustomData() const;
+    QVariantMap& publicCustomData();
+    const QVariantMap& publicCustomData() const;
     void setPublicCustomData(const QVariantMap& customData);
     void recycleEntry(Entry* entry);
     void recycleGroup(Group* group);

src/core/MacPasteboard.h

@@ -20,8 +20,9 @@
 
 #include <QMacPasteboardMime>
 #include <QTextCodec>
+#include <QObject>
 
-class MacPasteboard : public QMacPasteboardMime
+class MacPasteboard : public QObject, public QMacPasteboardMime
 {
 public:
     explicit MacPasteboard() : QMacPasteboardMime(MIME_ALL) {}

src/gui/Clipboard.cpp

@@ -24,14 +24,19 @@
 #include "core/Config.h"
 
 Clipboard* Clipboard::m_instance(nullptr);
+#ifdef Q_OS_MAC
+QPointer<MacPasteboard> Clipboard::m_pasteboard(nullptr);
+#endif
 
 Clipboard::Clipboard(QObject* parent)
     : QObject(parent)
     , m_timer(new QTimer(this))
-#ifdef Q_OS_MAC
-    , m_pasteboard(new MacPasteboard)
-#endif
 {
+#ifdef Q_OS_MAC
+    if (!m_pasteboard) {
+        m_pasteboard = new MacPasteboard();
+    }
+#endif
     m_timer->setSingleShot(true);
     connect(m_timer, SIGNAL(timeout()), SLOT(clearClipboard()));
     connect(qApp, SIGNAL(aboutToQuit()), SLOT(clearCopiedText()));

src/gui/Clipboard.h

@@ -21,6 +21,7 @@
 #include <QObject>
 #ifdef Q_OS_MAC
 #include "core/MacPasteboard.h"
+#include <QPointer>
 #endif
 
 class QTimer;
@@ -47,7 +48,9 @@ private:
 
     QTimer* m_timer;
 #ifdef Q_OS_MAC
-    QScopedPointer<MacPasteboard> m_pasteboard;
+    // This object lives for the whole program lifetime and we cannot delete it on exit,
+    // so ignore leak warnings. See https://bugreports.qt.io/browse/QTBUG-54832
+    static QPointer<MacPasteboard> m_pasteboard;
 #endif
     QString m_lastCopied;
 };

src/keys/YkChallengeResponseKey.cpp

@@ -83,7 +83,7 @@ bool YkChallengeResponseKey::challenge(const QByteArray& challenge, unsigned ret
         }
 
         // if challenge failed, retry to detect YubiKeys in the event the YubiKey was un-plugged and re-plugged
-        if (retries > 0 && YubiKey::instance()->init() != true) {
+        if (retries > 0 && !YubiKey::instance()->init()) {
             continue;
         }
 

src/keys/YkChallengeResponseKey.h

@@ -1,18 +1,18 @@
 /*
-*  Copyright (C) 2017 KeePassXC Team <team@keepassxc.org>
+ *  Copyright (C) 2017 KeePassXC Team <team@keepassxc.org>
-*
+ *
-*  This program is free software: you can redistribute it and/or modify
+ *  This program is free software: you can redistribute it and/or modify
-*  it under the terms of the GNU General Public License as published by
+ *  it under the terms of the GNU General Public License as published by
-*  the Free Software Foundation, either version 2 or (at your option)
+ *  the Free Software Foundation, either version 2 or (at your option)
-*  version 3 of the License.
+ *  version 3 of the License.
-*
+ *
-*  This program is distributed in the hope that it will be useful,
+ *  This program is distributed in the hope that it will be useful,
-*  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
-*  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-*  GNU General Public License for more details.
+ *  GNU General Public License for more details.
-*
+ *
-*  You should have received a copy of the GNU General Public License
+ *  You should have received a copy of the GNU General Public License
-*  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */
 
 #ifndef KEEPASSX_YK_CHALLENGERESPONSEKEY_H

tests/CMakeLists.txt

@@ -112,10 +112,10 @@ add_unit_test(NAME testkdbx2 SOURCES TestKdbx2.cpp
 add_unit_test(NAME testkdbx3 SOURCES TestKeePass2Format.cpp FailDevice.cpp TestKdbx3.cpp
         LIBS ${TEST_LIBRARIES})
 
-add_unit_test(NAME testkdbx4 SOURCES TestKeePass2Format.cpp FailDevice.cpp TestKdbx4.cpp
+add_unit_test(NAME testkdbx4 SOURCES TestKeePass2Format.cpp FailDevice.cpp mock/MockChallengeResponseKey.cpp TestKdbx4.cpp
         LIBS ${TEST_LIBRARIES})
 
-add_unit_test(NAME testkeys SOURCES TestKeys.cpp
+add_unit_test(NAME testkeys SOURCES TestKeys.cpp mock/MockChallengeResponseKey.cpp
         LIBS ${TEST_LIBRARIES})
 
 add_unit_test(NAME testgroupmodel SOURCES TestGroupModel.cpp

tests/TestKdbx4.cpp

@@ -20,6 +20,8 @@
 
 #include "core/Metadata.h"
 #include "keys/PasswordKey.h"
+#include "keys/FileKey.h"
+#include "mock/MockChallengeResponseKey.h"
 #include "format/KeePass2.h"
 #include "format/KeePass2Reader.h"
 #include "format/KeePass2Writer.h"
@@ -211,6 +213,106 @@ void TestKdbx4::testFormat400Upgrade_data()
     QTest::newRow("AES-KDF (legacy) + Twofish  + CustomData") << KeePass2::KDF_AES_KDBX3 << KeePass2::CIPHER_TWOFISH   << true  << kdbx4;
 }
 
+void TestKdbx4::testUpgradeMasterKeyIntegrity()
+{
+    QFETCH(QString, upgradeAction);
+    QFETCH(quint32, expectedVersion);
+
+    // prepare composite key
+    PasswordKey passwordKey("turXpGMQiUE6CkPvWacydAKsnp4cxz");
+
+    QByteArray fileKeyBytes("Ma6hHov98FbPeyAL22XhcgmpJk8xjQ");
+    QBuffer fileKeyBuffer(&fileKeyBytes);
+    fileKeyBuffer.open(QBuffer::ReadOnly);
+    FileKey fileKey;
+    fileKey.load(&fileKeyBuffer);
+
+    auto crKey = QSharedPointer<MockChallengeResponseKey>::create(QByteArray("azdJnbVCFE76vV6t9RJ2DS6xvSS93k"));
+
+    CompositeKey compositeKey;
+    compositeKey.addKey(passwordKey);
+    compositeKey.addKey(fileKey);
+    compositeKey.addChallengeResponseKey(crKey);
+
+    QScopedPointer<Database> db(new Database());
+    db->setKey(compositeKey);
+
+    // upgrade the database by a specific method
+    if (upgradeAction == "none") {
+        // do nothing
+    } else if (upgradeAction == "meta-customdata") {
+        db->metadata()->customData()->set("abc", "def");
+    } else if (upgradeAction == "kdf-aes-kdbx3") {
+        db->changeKdf(KeePass2::uuidToKdf(KeePass2::KDF_AES_KDBX3));
+    } else if (upgradeAction == "kdf-argon2") {
+        db->changeKdf(KeePass2::uuidToKdf(KeePass2::KDF_ARGON2));
+    } else if (upgradeAction == "kdf-aes-kdbx4") {
+        db->changeKdf(KeePass2::uuidToKdf(KeePass2::KDF_AES_KDBX4));
+    } else if (upgradeAction == "public-customdata") {
+        db->publicCustomData().insert("abc", "def");
+    } else if (upgradeAction == "rootgroup-customdata") {
+        db->rootGroup()->customData()->set("abc", "def");
+    } else if (upgradeAction == "group-customdata") {
+        auto group = new Group();
+        group->setParent(db->rootGroup());
+        group->setUuid(Uuid::random());
+        group->customData()->set("abc", "def");
+    } else if (upgradeAction == "rootentry-customdata") {
+        auto entry = new Entry();
+        entry->setGroup(db->rootGroup());
+        entry->setUuid(Uuid::random());
+        entry->customData()->set("abc", "def");
+    } else if (upgradeAction == "entry-customdata") {
+        auto group = new Group();
+        group->setParent(db->rootGroup());
+        group->setUuid(Uuid::random());
+        auto entry = new Entry();
+        entry->setGroup(group);
+        entry->setUuid(Uuid::random());
+        entry->customData()->set("abc", "def");
+    } else {
+        QFAIL(qPrintable(QString("Unknown action: %s").arg(upgradeAction)));
+    }
+
+    QBuffer buffer;
+    buffer.open(QBuffer::ReadWrite);
+    KeePass2Writer writer;
+    QVERIFY(writer.writeDatabase(&buffer, db.data()));
+
+    // paranoid check that we cannot decrypt the database without a key
+    buffer.seek(0);
+    KeePass2Reader reader;
+    QScopedPointer<Database> db2;
+    db2.reset(reader.readDatabase(&buffer, CompositeKey()));
+    QVERIFY(reader.hasError());
+
+    // check that we can read back the database with the original composite key,
+    // i.e., no components have been lost on the way
+    buffer.seek(0);
+    db2.reset(reader.readDatabase(&buffer, compositeKey));
+    if (reader.hasError()) {
+        QFAIL(qPrintable(reader.errorString()));
+    }
+    QCOMPARE(reader.version(), expectedVersion & KeePass2::FILE_VERSION_CRITICAL_MASK);
+}
+
+void TestKdbx4::testUpgradeMasterKeyIntegrity_data()
+{
+    QTest::addColumn<QString>("upgradeAction");
+    QTest::addColumn<quint32>("expectedVersion");
+
+    QTest::newRow("Upgrade: none")                            << QString("none")                 << KeePass2::FILE_VERSION_3;
+    QTest::newRow("Upgrade: none (meta-customdata)")          << QString("meta-customdata")      << KeePass2::FILE_VERSION_3;
+    QTest::newRow("Upgrade: none (explicit kdf-aes-kdbx3)")   << QString("kdf-aes-kdbx3")        << KeePass2::FILE_VERSION_3;
+    QTest::newRow("Upgrade (explicit): kdf-argon2")           << QString("kdf-argon2")           << KeePass2::FILE_VERSION_4;
+    QTest::newRow("Upgrade (explicit): kdf-aes-kdbx4")        << QString("kdf-aes-kdbx4")        << KeePass2::FILE_VERSION_4;
+    QTest::newRow("Upgrade (implicit): public-customdata")    << QString("public-customdata")    << KeePass2::FILE_VERSION_4;
+    QTest::newRow("Upgrade (implicit): rootgroup-customdata") << QString("rootgroup-customdata") << KeePass2::FILE_VERSION_4;
+    QTest::newRow("Upgrade (implicit): group-customdata")     << QString("group-customdata")     << KeePass2::FILE_VERSION_4;
+    QTest::newRow("Upgrade (implicit): rootentry-customdata") << QString("rootentry-customdata") << KeePass2::FILE_VERSION_4;
+    QTest::newRow("Upgrade (implicit): entry-customdata")     << QString("entry-customdata")     << KeePass2::FILE_VERSION_4;
+}
+
 void TestKdbx4::testCustomData()
 {
     Database db;
@@ -220,8 +322,9 @@ void TestKdbx4::testCustomData()
     publicCustomData.insert("CD1", 123);
     publicCustomData.insert("CD2", true);
     publicCustomData.insert("CD3", "abcäöü");
-    publicCustomData.insert("CD4", QByteArray::fromHex("ababa123ff"));
     db.setPublicCustomData(publicCustomData);
+    publicCustomData.insert("CD4", QByteArray::fromHex("ababa123ff"));
+    db.publicCustomData().insert("CD4", publicCustomData.value("CD4"));
     QCOMPARE(db.publicCustomData(), publicCustomData);
 
     const QString customDataKey1 = "CD1";
@@ -229,7 +332,7 @@ void TestKdbx4::testCustomData()
     const QString customData1 = "abcäöü";
     const QString customData2 = "Hello World";
     const int dataSize = customDataKey1.toUtf8().size() + customDataKey1.toUtf8().size() +
-            customData1.toUtf8().size() + customData2.toUtf8().size();
+        customData1.toUtf8().size() + customData2.toUtf8().size();
 
     // test custom database data
     db.metadata()->customData()->set(customDataKey1, customData1);

tests/TestKdbx4.h

@@ -28,6 +28,8 @@ private slots:
     void testFormat400();
     void testFormat400Upgrade();
     void testFormat400Upgrade_data();
+    void testUpgradeMasterKeyIntegrity();
+    void testUpgradeMasterKeyIntegrity_data();
     void testCustomData();
 
 protected:

tests/TestKeys.cpp

@@ -32,6 +32,7 @@
 #include "format/KeePass2Writer.h"
 #include "keys/FileKey.h"
 #include "keys/PasswordKey.h"
+#include "mock/MockChallengeResponseKey.h"
 
 QTEST_GUILESS_MAIN(TestKeys)
 Q_DECLARE_METATYPE(FileKey::Type);
@@ -232,3 +233,85 @@ void TestKeys::benchmarkTransformKey()
         Q_UNUSED(compositeKey.transform(kdf, result));
     };
 }
+
+void TestKeys::testCompositeKeyComponents()
+{
+    PasswordKey passwordKeyEnc("password");
+    FileKey fileKeyEnc;
+    QString error;
+    fileKeyEnc.load(QString("%1/%2").arg(QString(KEEPASSX_TEST_DATA_DIR), "FileKeyHashed.key"), &error);
+    if (!error.isNull()) {
+        QFAIL(qPrintable(error));
+    }
+    auto challengeResponseKeyEnc = QSharedPointer<MockChallengeResponseKey>::create(QByteArray(16, 0x10));
+
+    CompositeKey compositeKeyEnc;
+    compositeKeyEnc.addKey(passwordKeyEnc);
+    compositeKeyEnc.addKey(fileKeyEnc);
+    compositeKeyEnc.addChallengeResponseKey(challengeResponseKeyEnc);
+
+    QScopedPointer<Database> db1(new Database());
+    db1->setKey(compositeKeyEnc);
+
+    KeePass2Writer writer;
+    QBuffer buffer;
+    buffer.open(QBuffer::ReadWrite);
+    QVERIFY(writer.writeDatabase(&buffer, db1.data()));
+
+    buffer.seek(0);
+    QScopedPointer<Database> db2;
+    KeePass2Reader reader;
+    CompositeKey compositeKeyDec1;
+
+    // try decryption and subsequently add key components until decryption is successful
+    db2.reset(reader.readDatabase(&buffer, compositeKeyDec1));
+    QVERIFY(reader.hasError());
+
+    compositeKeyDec1.addKey(passwordKeyEnc);
+    buffer.seek(0);
+    db2.reset(reader.readDatabase(&buffer, compositeKeyDec1));
+    QVERIFY(reader.hasError());
+
+    compositeKeyDec1.addKey(fileKeyEnc);
+    buffer.seek(0);
+    db2.reset(reader.readDatabase(&buffer, compositeKeyDec1));
+    QVERIFY(reader.hasError());
+
+    compositeKeyDec1.addChallengeResponseKey(challengeResponseKeyEnc);
+    buffer.seek(0);
+    db2.reset(reader.readDatabase(&buffer, compositeKeyDec1));
+    // now we should be able to open the database
+    if (reader.hasError()) {
+        QFAIL(qPrintable(reader.errorString()));
+    }
+
+    // try the same again, but this time with one wrong key component each time
+    CompositeKey compositeKeyDec2;
+    compositeKeyDec2.addKey(PasswordKey("wrong password"));
+    compositeKeyDec2.addKey(fileKeyEnc);
+    compositeKeyDec2.addChallengeResponseKey(challengeResponseKeyEnc);
+    buffer.seek(0);
+    db2.reset(reader.readDatabase(&buffer, compositeKeyDec2));
+    QVERIFY(reader.hasError());
+
+    CompositeKey compositeKeyDec3;
+    compositeKeyDec3.addKey(passwordKeyEnc);
+    FileKey fileKeyWrong;
+    fileKeyWrong.load(QString("%1/%2").arg(QString(KEEPASSX_TEST_DATA_DIR), "FileKeyHashed2.key"), &error);
+    if (!error.isNull()) {
+        QFAIL(qPrintable(error));
+    }
+    compositeKeyDec3.addKey(fileKeyWrong);
+    compositeKeyDec3.addChallengeResponseKey(challengeResponseKeyEnc);
+    buffer.seek(0);
+    db2.reset(reader.readDatabase(&buffer, compositeKeyDec3));
+    QVERIFY(reader.hasError());
+
+    CompositeKey compositeKeyDec4;
+    compositeKeyDec4.addKey(passwordKeyEnc);
+    compositeKeyDec4.addKey(fileKeyEnc);
+    compositeKeyDec4.addChallengeResponseKey(QSharedPointer<MockChallengeResponseKey>::create(QByteArray(16, 0x20)));
+    buffer.seek(0);
+    db2.reset(reader.readDatabase(&buffer, compositeKeyDec4));
+    QVERIFY(reader.hasError());
+}

tests/TestKeys.h

@@ -34,6 +34,7 @@ private slots:
     void testCreateAndOpenFileKey();
     void testFileKeyHash();
     void testFileKeyError();
+    void testCompositeKeyComponents();
     void benchmarkTransformKey();
 };
 

tests/mock/MockChallengeResponseKey.cpp

@@ -0,0 +1,38 @@
+/*
+ *  Copyright (C) 2018 KeePassXC Team <team@keepassxc.org>
+ *
+ *  This program is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 2 or (at your option)
+ *  version 3 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include "MockChallengeResponseKey.h"
+
+MockChallengeResponseKey::MockChallengeResponseKey(const QByteArray& secret)
+    : m_secret(secret)
+{
+}
+
+MockChallengeResponseKey::~MockChallengeResponseKey()
+{
+}
+
+QByteArray MockChallengeResponseKey::rawKey() const
+{
+    return m_challenge + m_secret;
+}
+
+bool MockChallengeResponseKey::challenge(const QByteArray& challenge)
+{
+    m_challenge = challenge;
+    return true;
+}

tests/mock/MockChallengeResponseKey.h

@@ -0,0 +1,40 @@
+/*
+ *  Copyright (C) 2018 KeePassXC Team <team@keepassxc.org>
+ *
+ *  This program is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 2 or (at your option)
+ *  version 3 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#ifndef KEEPASSXC_MOCKCHALLENGERESPONSEKEY_H
+#define KEEPASSXC_MOCKCHALLENGERESPONSEKEY_H
+
+#include "keys/ChallengeResponseKey.h"
+
+/**
+ * Mock challenge-response key implementation that simply
+ * returns the challenge concatenated with a fixed secret.
+ */
+class MockChallengeResponseKey : public ChallengeResponseKey
+{
+public:
+    explicit MockChallengeResponseKey(const QByteArray& secret);
+    ~MockChallengeResponseKey() override;
+    QByteArray rawKey() const override;
+    bool challenge(const QByteArray& challenge) override;
+
+private:
+    QByteArray m_challenge;
+    QByteArray m_secret;
+};
+
+#endif //KEEPASSXC_MOCKCHALLENGERESPONSEKEY_H