From c7d318236f367ad8ba61af61460973529c17155d Mon Sep 17 00:00:00 2001 From: christianwengert Date: Fri, 28 Jun 2024 11:24:44 +0200 Subject: [PATCH] Prevent duplicate entries in passphrase wordlists Replace a QVector for the wordlist with a QSet. This removes all duplicate entries in a given wordlist. Thus, it hinders a malicious wordlist that has the proper length (>4000 entries) but with repetitions (effectively << 4000 entries) to be used and potentially create weaker passphrases than estimated. Example: List with 4000 items but only 64 unique words would lead to only 48 bit of Entropy instead of ~95 bit! --- share/translations/keepassxc_en.ts | 8 ++--- src/cli/Diceware.cpp | 2 +- src/core/PassphraseGenerator.cpp | 31 +++++++++---------- src/core/PassphraseGenerator.h | 9 ++++-- tests/TestCli.cpp | 2 +- tests/TestPassphraseGenerator.cpp | 16 ++++++++++ tests/TestPassphraseGenerator.h | 1 + ...d_wordlist_with_duplicate_entries.wordlist | 4 +++ 8 files changed, 47 insertions(+), 26 deletions(-) create mode 100644 tests/data/wordlists/bad_wordlist_with_duplicate_entries.wordlist diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 43183f082..952e491ae 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -7653,10 +7653,6 @@ Do you want to overwrite it? Invalid word count %1 - - The word list is too small (< 1000 items) - - Title for the entry. @@ -8902,6 +8898,10 @@ This option is deprecated, use --set-key-file instead. Only PBKDF and Argon2 are supported, cannot decrypt json file + + Cannot generate valid passphrases because the wordlist is too short + + QtIOCompressor diff --git a/src/cli/Diceware.cpp b/src/cli/Diceware.cpp index 8a2e026db..c2e3f9fee 100644 --- a/src/cli/Diceware.cpp +++ b/src/cli/Diceware.cpp @@ -71,7 +71,7 @@ int Diceware::execute(const QStringList& arguments) if (!dicewareGenerator.isValid()) { // We already validated the word count input so if the generator is invalid, it // must be because the word list is too small. - err << QObject::tr("The word list is too small (< 1000 items)") << Qt::endl; + err << QObject::tr("Cannot generate valid passphrases because the wordlist is too short") << Qt::endl; return EXIT_FAILURE; } diff --git a/src/core/PassphraseGenerator.cpp b/src/core/PassphraseGenerator.cpp index 79227817e..e7d307e02 100644 --- a/src/core/PassphraseGenerator.cpp +++ b/src/core/PassphraseGenerator.cpp @@ -18,12 +18,14 @@ #include "PassphraseGenerator.h" #include +#include #include #include #include "core/Resources.h" #include "crypto/Random.h" +const int PassphraseGenerator::DefaultWordCount = 7; const char* PassphraseGenerator::DefaultSeparator = " "; const char* PassphraseGenerator::DefaultWordList = "eff_large.wordlist"; @@ -60,10 +62,12 @@ void PassphraseGenerator::setWordCase(PassphraseWordCase wordCase) void PassphraseGenerator::setWordList(const QString& path) { m_wordlist.clear(); + // Initially load wordlist into a set to avoid duplicates + QSet wordset; QFile file(path); if (!file.open(QIODevice::ReadOnly | QIODevice::Text)) { - qWarning("Couldn't load passphrase wordlist."); + qWarning("Couldn't load passphrase wordlist: %s", qPrintable(path)); return; } @@ -87,14 +91,15 @@ void PassphraseGenerator::setWordList(const QString& path) line = line.trimmed(); line.replace(rx, "\\2"); if (!line.isEmpty()) { - m_wordlist.append(line); + wordset.insert(line); } line = in.readLine(); } - if (m_wordlist.size() < 4000) { - qWarning("Wordlist too short!"); - return; + m_wordlist = wordset.toList(); + + if (m_wordlist.size() < m_minimum_wordlist_length) { + qWarning("Wordlist is less than minimum acceptable size: %s", qPrintable(path)); } } @@ -111,18 +116,15 @@ void PassphraseGenerator::setWordSeparator(const QString& separator) QString PassphraseGenerator::generatePassphrase() const { - QString tmpWord; - Q_ASSERT(isValid()); - // In case there was an error loading the wordlist - if (m_wordlist.length() == 0) { + if (!isValid() || m_wordlist.empty()) { return {}; } QStringList words; for (int i = 0; i < m_wordCount; ++i) { - int wordIndex = randomGen()->randomUInt(static_cast(m_wordlist.length())); - tmpWord = m_wordlist.at(wordIndex); + int wordIndex = randomGen()->randomUInt(static_cast(m_wordlist.size())); + auto tmpWord = m_wordlist.at(wordIndex); // convert case switch (m_wordCase) { @@ -133,7 +135,6 @@ QString PassphraseGenerator::generatePassphrase() const tmpWord = tmpWord.replace(0, 1, tmpWord.left(1).toUpper()); break; case LOWERCASE: - default: tmpWord = tmpWord.toLower(); break; } @@ -145,9 +146,5 @@ QString PassphraseGenerator::generatePassphrase() const bool PassphraseGenerator::isValid() const { - if (m_wordCount == 0) { - return false; - } - - return m_wordlist.size() >= 1000; + return m_wordCount > 0 && m_wordlist.size() >= m_minimum_wordlist_length; } diff --git a/src/core/PassphraseGenerator.h b/src/core/PassphraseGenerator.h index bb282f59b..e847f658c 100644 --- a/src/core/PassphraseGenerator.h +++ b/src/core/PassphraseGenerator.h @@ -18,7 +18,7 @@ #ifndef KEEPASSX_PASSPHRASEGENERATOR_H #define KEEPASSX_PASSPHRASEGENERATOR_H -#include +#include class PassphraseGenerator { @@ -43,15 +43,18 @@ public: QString generatePassphrase() const; - static constexpr int DefaultWordCount = 7; + static const int DefaultWordCount; static const char* DefaultSeparator; static const char* DefaultWordList; private: int m_wordCount; + int m_minimum_wordlist_length = 4000; PassphraseWordCase m_wordCase; QString m_separator; - QVector m_wordlist; + QList m_wordlist; + + friend class TestPassphraseGenerator; }; #endif // KEEPASSX_PASSPHRASEGENERATOR_H diff --git a/tests/TestCli.cpp b/tests/TestCli.cpp index 3022ae606..7200d21fa 100644 --- a/tests/TestCli.cpp +++ b/tests/TestCli.cpp @@ -1088,7 +1088,7 @@ void TestCli::testDiceware() smallWordFile.close(); execCmd(dicewareCmd, {"diceware", "-W", "11", "-w", smallWordFile.fileName()}); - QCOMPARE(m_stderr->readLine(), QByteArray("The word list is too small (< 1000 items)\n")); + QCOMPARE(m_stderr->readLine(), QByteArray("Cannot generate valid passphrases because the wordlist is too short\n")); } void TestCli::testEdit() diff --git a/tests/TestPassphraseGenerator.cpp b/tests/TestPassphraseGenerator.cpp index ffa21e9b7..b27922a60 100644 --- a/tests/TestPassphraseGenerator.cpp +++ b/tests/TestPassphraseGenerator.cpp @@ -16,6 +16,7 @@ */ #include "TestPassphraseGenerator.h" +#include "config-keepassx-tests.h" #include "core/PassphraseGenerator.h" #include "crypto/Crypto.h" @@ -52,3 +53,18 @@ void TestPassphraseGenerator::testWordCase() QRegularExpression regex("^(?:[A-Z][a-z-]* )*[A-Z][a-z-]*$"); QVERIFY2(regex.match(passphrase).hasMatch(), qPrintable(passphrase)); } + +void TestPassphraseGenerator::testUniqueEntriesInWordlist() +{ + PassphraseGenerator generator; + // set the limit down, so we don;t have to do a very large file + generator.m_minimum_wordlist_length = 4; + + // link to bad wordlist + QString path = QString(KEEPASSX_TEST_DATA_DIR).append("/wordlists/bad_wordlist_with_duplicate_entries.wordlist"); + + // setting will work, it creates the warning however, and isValid will fail + generator.setWordList(path); + // so this fails + QVERIFY(!generator.isValid()); +} diff --git a/tests/TestPassphraseGenerator.h b/tests/TestPassphraseGenerator.h index ca0fd0664..a4d86055a 100644 --- a/tests/TestPassphraseGenerator.h +++ b/tests/TestPassphraseGenerator.h @@ -27,6 +27,7 @@ class TestPassphraseGenerator : public QObject private slots: void initTestCase(); void testWordCase(); + void testUniqueEntriesInWordlist(); }; #endif // KEEPASSXC_TESTPASSPHRASEGENERATOR_H diff --git a/tests/data/wordlists/bad_wordlist_with_duplicate_entries.wordlist b/tests/data/wordlists/bad_wordlist_with_duplicate_entries.wordlist new file mode 100644 index 000000000..50246c96a --- /dev/null +++ b/tests/data/wordlists/bad_wordlist_with_duplicate_entries.wordlist @@ -0,0 +1,4 @@ +abacus +abdomen +abdominal +abdominal