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

@@ -7214,10 +7214,6 @@ Do you want to overwrite it?</source>
         <source>Invalid word count %1</source>
         <translation type="unfinished"></translation>
     </message>
-    <message>
-        <source>The word list is too small (&lt; 1000 items)</source>
-        <translation type="unfinished"></translation>
-    </message>
     <message>
         <source>Title for the entry.</source>
         <translation type="unfinished"></translation>
@@ -8547,6 +8543,10 @@ This option is deprecated, use --set-key-file instead.</source>
         <source>Only PBKDF and Argon2 are supported, cannot decrypt json file</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>Cannot generate valid passphrases because the wordlist is too short</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>QtIOCompressor</name>

src/cli/Diceware.cpp

@@ -70,7 +70,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)") << endl;
+        err << QObject::tr("Cannot generate valid passphrases because the wordlist is too short") << endl;
         return EXIT_FAILURE;
     }
 

src/core/PassphraseGenerator.cpp

@@ -18,12 +18,14 @@
 #include "PassphraseGenerator.h"
 
 #include <QFile>
+#include <QSet>
 #include <QTextStream>
 #include <cmath>
 
 #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<QString> 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) {
-        return QString();
+    if (!isValid() || m_wordlist.empty()) {
+        return {};
     }
 
     QStringList words;
     for (int i = 0; i < m_wordCount; ++i) {
-        int wordIndex = randomGen()->randomUInt(static_cast<quint32>(m_wordlist.length()));
-        tmpWord = m_wordlist.at(wordIndex);
+        int wordIndex = randomGen()->randomUInt(static_cast<quint32>(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;
 }

src/core/PassphraseGenerator.h

@@ -18,7 +18,7 @@
 #ifndef KEEPASSX_PASSPHRASEGENERATOR_H
 #define KEEPASSX_PASSPHRASEGENERATOR_H
 
-#include <QVector>
+#include <QList>
 
 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<QString> m_wordlist;
+    QList<QString> m_wordlist;
+
+    friend class TestPassphraseGenerator;
 };
 
 #endif // KEEPASSX_PASSPHRASEGENERATOR_H

tests/TestCli.cpp

@@ -1087,7 +1087,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()

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());
+}

tests/TestPassphraseGenerator.h

@@ -27,6 +27,7 @@ class TestPassphraseGenerator : public QObject
 private slots:
     void initTestCase();
     void testWordCase();
+    void testUniqueEntriesInWordlist();
 };
 
 #endif // KEEPASSXC_TESTPASSPHRASEGENERATOR_H

tests/data/wordlists/bad_wordlist_with_duplicate_entries.wordlist

@@ -0,0 +1,4 @@
+abacus
+abdomen
+abdominal
+abdominal