diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 626a03533..13baa5d65 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -7214,10 +7214,6 @@ Do you want to overwrite it? Invalid word count %1 - - The word list is too small (< 1000 items) - - Title for the entry. @@ -8547,6 +8543,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 cea5bf1d6..3d26486ad 100644 --- a/src/cli/Diceware.cpp +++ b/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; } diff --git a/src/core/PassphraseGenerator.cpp b/src/core/PassphraseGenerator.cpp index ef1d867e2..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) { - return QString(); + 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 2235890dc..9d6137d29 100644 --- a/tests/TestCli.cpp +++ b/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() 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