diff --git a/COPYING b/COPYING index 4e85fc078..a00aaf28c 100644 --- a/COPYING +++ b/COPYING @@ -223,6 +223,7 @@ Files: share/icons/application/scalable/actions/application-exit.svg share/icons/application/scalable/actions/totp-copy.svg share/icons/application/scalable/actions/totp-copy-password.svg share/icons/application/scalable/actions/totp-edit.svg + share/icons/application/scalable/actions/totp-invalid.svg share/icons/application/scalable/actions/trash.svg share/icons/application/scalable/actions/url-copy.svg share/icons/application/scalable/actions/user-guide.svg diff --git a/share/icons/application/scalable/actions/totp-invalid.svg b/share/icons/application/scalable/actions/totp-invalid.svg new file mode 100644 index 000000000..c4146ab84 --- /dev/null +++ b/share/icons/application/scalable/actions/totp-invalid.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/share/icons/icons.qrc b/share/icons/icons.qrc index e92c98a35..3e82e172d 100644 --- a/share/icons/icons.qrc +++ b/share/icons/icons.qrc @@ -92,6 +92,7 @@ application/scalable/actions/totp-copy.svg application/scalable/actions/totp-copy-password.svg application/scalable/actions/totp-edit.svg + application/scalable/actions/totp-invalid.svg application/scalable/actions/trash.svg application/scalable/actions/url-copy.svg application/scalable/actions/user-guide.svg diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 6b4c4c211..413c9fb1c 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -724,6 +724,10 @@ Invalid placeholder: %1 + + Entry has invalid TOTP settings + + AutoTypeAssociationsModel @@ -9247,6 +9251,16 @@ This option is deprecated, use --set-key-file instead. Warning: the chosen wordlist is smaller than the minimum recommended size! + + Invalid Step + TOTP + + + + Invalid Digits + TOTP + + Fit @@ -10317,6 +10331,10 @@ Example: JBSWY3DPEHPK3PXP Are you sure you want to delete TOTP settings for this entry? + + Error: secret key is invalid + + URLEdit diff --git a/src/autotype/AutoType.cpp b/src/autotype/AutoType.cpp index 738dc5ac6..c42333fba 100644 --- a/src/autotype/AutoType.cpp +++ b/src/autotype/AutoType.cpp @@ -637,10 +637,16 @@ AutoType::parseSequence(const QString& entrySequence, const Entry* entry, QStrin // Platform-specific field clearing actions << QSharedPointer::create(); } else if (placeholder == "totp") { - // Entry totp (requires special handling) - QString totp = entry->totp(); - for (const auto& ch : totp) { - actions << QSharedPointer::create(ch); + if (entry->hasValidTotp()) { + // Entry totp (requires special handling) + QString totp = entry->totp(); + for (const auto& ch : totp) { + actions << QSharedPointer::create(ch); + } + } else if (entry->hasTotp()) { + // Entry has TOTP configured but invalid settings + error = tr("Entry has invalid TOTP settings"); + return {}; } } else if (placeholder.startsWith("pickchars")) { // Reset to the original capture to preserve case diff --git a/src/autotype/AutoTypeSelectDialog.cpp b/src/autotype/AutoTypeSelectDialog.cpp index e7d184c8b..d06eaa1c3 100644 --- a/src/autotype/AutoTypeSelectDialog.cpp +++ b/src/autotype/AutoTypeSelectDialog.cpp @@ -264,7 +264,7 @@ void AutoTypeSelectDialog::updateActionMenu(const AutoTypeMatch& match) bool hasUsername = !match.first->username().isEmpty(); bool hasPassword = !match.first->password().isEmpty(); - bool hasTotp = match.first->hasTotp(); + bool hasTotp = match.first->hasValidTotp(); for (auto action : m_actionMenu->actions()) { auto prop = action->property(MENU_FIELD_PROP_NAME); diff --git a/src/browser/BrowserService.cpp b/src/browser/BrowserService.cpp index edca82270..202f420a5 100644 --- a/src/browser/BrowserService.cpp +++ b/src/browser/BrowserService.cpp @@ -1192,7 +1192,7 @@ QJsonObject BrowserService::prepareEntry(const Entry* entry) res["uuid"] = entry->resolveMultiplePlaceholders(entry->uuidToHex()); res["group"] = entry->resolveMultiplePlaceholders(entry->group()->name()); - if (entry->hasTotp()) { + if (entry->hasValidTotp()) { res["totp"] = entry->totp(); } diff --git a/src/core/Entry.cpp b/src/core/Entry.cpp index 0147ec0f9..1ef96aabe 100644 --- a/src/core/Entry.cpp +++ b/src/core/Entry.cpp @@ -570,6 +570,12 @@ bool Entry::hasTotp() const return !m_data.totpSettings.isNull(); } +bool Entry::hasValidTotp() const +{ + auto error = Totp::checkValidSettings(m_data.totpSettings); + return error.isEmpty(); +} + bool Entry::hasPasskey() const { return m_attributes->hasPasskey(); @@ -581,10 +587,13 @@ void Entry::removePasskey() removeTag(tr("Passkey")); } -QString Entry::totp() const +QString Entry::totp(bool* isValid) const { if (hasTotp()) { - return Totp::generateTotp(m_data.totpSettings); + return Totp::generateTotp(m_data.totpSettings, isValid); + } + if (isValid) { + *isValid = false; } return {}; } diff --git a/src/core/Entry.h b/src/core/Entry.h index 3c9a0f3de..ddf6b21a6 100644 --- a/src/core/Entry.h +++ b/src/core/Entry.h @@ -109,7 +109,7 @@ public: QString password() const; QString notes() const; QString attribute(const QString& key) const; - QString totp() const; + QString totp(bool* isValid = nullptr) const; QString totpSettingsString() const; QSharedPointer totpSettings() const; Group* previousParentGroup(); @@ -126,6 +126,7 @@ public: void removePasskey(); bool hasTotp() const; + bool hasValidTotp() const; bool isExpired() const; bool willExpireInDays(int days) const; void expireNow(); diff --git a/src/core/Totp.cpp b/src/core/Totp.cpp index 9d1e00440..ed15a9fb8 100644 --- a/src/core/Totp.cpp +++ b/src/core/Totp.cpp @@ -210,12 +210,33 @@ QString Totp::writeSettings(const QSharedPointer& settings, } } -QString Totp::generateTotp(const QSharedPointer& settings, const quint64 time) +QString Totp::checkValidSettings(const QSharedPointer& settings) { - Q_ASSERT(!settings.isNull()); if (settings.isNull()) { return QObject::tr("Invalid Settings", "TOTP"); } + QVariant secret = Base32::decode(Base32::sanitizeInput(settings->key.toLatin1())); + if (secret.isNull()) { + return QObject::tr("Invalid Key", "TOTP"); + } + if (settings->step == 0) { + return QObject::tr("Invalid Step", "TOTP"); + } + if (settings->digits == 0) { + return QObject::tr("Invalid Digits", "TOTP"); + } + return {}; +} + +QString Totp::generateTotp(const QSharedPointer& settings, bool* isValid, const quint64 time) +{ + auto error = checkValidSettings(settings); + if (!error.isEmpty()) { + if (isValid) { + *isValid = false; + } + return error; + } const Encoder& encoder = settings->encoder; uint step = settings->step; @@ -229,9 +250,6 @@ QString Totp::generateTotp(const QSharedPointer& settings, const } QVariant secret = Base32::decode(Base32::sanitizeInput(settings->key.toLatin1())); - if (secret.isNull()) { - return QObject::tr("Invalid Key", "TOTP"); - } QCryptographicHash::Algorithm cryptoHash; switch (settings->algorithm) { @@ -274,6 +292,9 @@ QString Totp::generateTotp(const QSharedPointer& settings, const retval[pos] = encoder.alphabet[int(password % encoder.alphabet.size())]; password /= encoder.alphabet.size(); } + if (isValid) { + *isValid = true; + } return retval; } diff --git a/src/core/Totp.h b/src/core/Totp.h index 8dbdbd50f..da857aef2 100644 --- a/src/core/Totp.h +++ b/src/core/Totp.h @@ -91,8 +91,10 @@ namespace Totp const QString& title = {}, const QString& username = {}, bool forceOtp = false); - - QString generateTotp(const QSharedPointer& settings, const quint64 time = 0ull); + // Returns an empty string if settings are valid, otherwise an error message is supplied + QString checkValidSettings(const QSharedPointer& settings); + QString + generateTotp(const QSharedPointer& settings, bool* isValid = nullptr, const quint64 time = 0ull); bool hasCustomSettings(const QSharedPointer& settings); diff --git a/src/fdosecrets/objects/Item.cpp b/src/fdosecrets/objects/Item.cpp index 2c6d13435..373735205 100644 --- a/src/fdosecrets/objects/Item.cpp +++ b/src/fdosecrets/objects/Item.cpp @@ -125,7 +125,7 @@ namespace FdoSecrets // add some informative and readonly attributes attrs[ItemAttributes::UuidKey] = m_backend->uuidToHex(); attrs[ItemAttributes::PathKey] = path(); - if (m_backend->hasTotp()) { + if (m_backend->hasValidTotp()) { attrs[ItemAttributes::TotpKey] = m_backend->totp(); } return {}; diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 155f02465..a9f7f5a81 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -1527,7 +1527,7 @@ void DatabaseWidget::entryActivationSignalReceived(Entry* entry, EntryModel::Mod } break; case EntryModel::Totp: - if (entry->hasTotp()) { + if (entry->hasValidTotp()) { setClipboardTextAndMinimize(entry->totp()); } else { setupTotp(); @@ -2386,7 +2386,7 @@ bool DatabaseWidget::currentEntryHasTotp() if (!currentEntry) { return false; } - return currentEntry->hasTotp(); + return currentEntry->hasValidTotp(); } #ifdef WITH_XC_SSHAGENT diff --git a/src/gui/EntryPreviewWidget.cpp b/src/gui/EntryPreviewWidget.cpp index 69f6b56dc..d61d4ca06 100644 --- a/src/gui/EntryPreviewWidget.cpp +++ b/src/gui/EntryPreviewWidget.cpp @@ -70,8 +70,7 @@ EntryPreviewWidget::EntryPreviewWidget(QWidget* parent) m_ui->entryTotpLabel->installEventFilter(this); - connect(m_ui->entryTotpButton, SIGNAL(toggled(bool)), m_ui->entryTotpLabel, SLOT(setVisible(bool))); - connect(m_ui->entryTotpButton, SIGNAL(toggled(bool)), m_ui->entryTotpProgress, SLOT(setVisible(bool))); + connect(m_ui->entryTotpButton, SIGNAL(toggled(bool)), m_ui->entryTotp, SLOT(setVisible(bool))); connect(m_ui->entryCloseButton, SIGNAL(clicked()), SLOT(hide())); connect(m_ui->toggleUsernameButton, SIGNAL(clicked(bool)), SLOT(setUsernameVisible(bool))); connect(m_ui->togglePasswordButton, SIGNAL(clicked(bool)), SLOT(setPasswordVisible(bool))); @@ -260,8 +259,7 @@ void EntryPreviewWidget::updateEntryTotp() m_ui->entryTotpProgress->setMaximum(m_currentEntry->totpSettings()->step); updateTotpLabel(); } else { - m_ui->entryTotpLabel->hide(); - m_ui->entryTotpProgress->hide(); + m_ui->entryTotp->hide(); m_ui->entryTotpButton->setChecked(false); m_ui->entryTotpLabel->clear(); m_totpTimer.stop(); @@ -546,16 +544,23 @@ void EntryPreviewWidget::updateGroupSharingTab() void EntryPreviewWidget::updateTotpLabel() { if (!m_locked && m_currentEntry && m_currentEntry->hasTotp()) { - auto totpCode = m_currentEntry->totp(); - totpCode.insert(totpCode.size() / 2, " "); - m_ui->entryTotpLabel->setText(totpCode); + bool isValid = false; + auto totpCode = m_currentEntry->totp(&isValid); + if (isValid) { + totpCode.insert(totpCode.size() / 2, " "); - auto step = m_currentEntry->totpSettings()->step; - auto timeleft = step - (Clock::currentSecondsSinceEpoch() % step); - m_ui->entryTotpProgress->setValue(timeleft); - m_ui->entryTotpProgress->update(); + auto step = m_currentEntry->totpSettings()->step; + auto timeleft = step - (Clock::currentSecondsSinceEpoch() % step); + m_ui->entryTotpProgress->setValue(timeleft); + m_ui->entryTotpProgress->update(); + } else { + m_totpTimer.stop(); + } + + m_ui->entryTotpProgress->setVisible(isValid); + m_ui->entryTotpLabel->setText(totpCode); } else { - m_ui->entryTotpLabel->clear(); + m_ui->entryTotp->setVisible(false); m_totpTimer.stop(); } } diff --git a/src/gui/EntryPreviewWidget.ui b/src/gui/EntryPreviewWidget.ui index 1fc8a33ff..e44218b3e 100644 --- a/src/gui/EntryPreviewWidget.ui +++ b/src/gui/EntryPreviewWidget.ui @@ -110,47 +110,66 @@ - - - 0 + + + + 0 + 0 + - - - - - 10 - 75 - true - - - - Double click to copy to clipboard - - - 1234567 - - - Qt::LinksAccessibleByMouse|Qt::TextSelectableByKeyboard|Qt::TextSelectableByMouse - - - - - - - - 57 - 4 - - - - 50 - - - false - - - - + + + 0 + + + 0 + + + 0 + + + 0 + + + 0 + + + + + + 10 + true + + + + Double click to copy to clipboard + + + 1234567 + + + Qt::TextInteractionFlag::LinksAccessibleByMouse|Qt::TextInteractionFlag::TextSelectableByKeyboard|Qt::TextInteractionFlag::TextSelectableByMouse + + + + + + + + 57 + 4 + + + + 50 + + + false + + + + + diff --git a/src/gui/TotpDialog.cpp b/src/gui/TotpDialog.cpp index 40888bd99..7b794c08d 100644 --- a/src/gui/TotpDialog.cpp +++ b/src/gui/TotpDialog.cpp @@ -39,6 +39,7 @@ TotpDialog::TotpDialog(QWidget* parent, Entry* entry) m_step = m_entry->totpSettings()->step; resetCounter(); updateProgressBar(); + updateSeconds(); connect(&m_totpUpdateTimer, SIGNAL(timeout()), this, SLOT(updateProgressBar())); connect(&m_totpUpdateTimer, SIGNAL(timeout()), this, SLOT(updateSeconds())); @@ -88,10 +89,15 @@ void TotpDialog::updateSeconds() void TotpDialog::updateTotp() { - QString totpCode = m_entry->totp(); - QString firstHalf = totpCode.left(totpCode.size() / 2); - QString secondHalf = totpCode.mid(totpCode.size() / 2); - m_ui->totpLabel->setText(firstHalf + " " + secondHalf); + bool isValid = false; + QString totpCode = m_entry->totp(&isValid); + if (isValid) { + totpCode.insert(totpCode.size() / 2, " "); + } + m_ui->buttonBox->button(QDialogButtonBox::Ok)->setEnabled(isValid); + m_ui->progressBar->setVisible(isValid); + m_ui->timerLabel->setVisible(isValid); + m_ui->totpLabel->setText(totpCode); } void TotpDialog::resetCounter() diff --git a/src/gui/TotpSetupDialog.cpp b/src/gui/TotpSetupDialog.cpp index 4dab00370..27bad9782 100644 --- a/src/gui/TotpSetupDialog.cpp +++ b/src/gui/TotpSetupDialog.cpp @@ -127,5 +127,8 @@ void TotpSetupDialog::init() m_ui->algorithmComboBox->setCurrentIndex(index); } } + + auto error = Totp::checkValidSettings(settings); + m_ui->invalidKeyLabel->setVisible(!error.isEmpty()); } } diff --git a/src/gui/TotpSetupDialog.ui b/src/gui/TotpSetupDialog.ui index ab15b8ea8..f8c95f4c4 100644 --- a/src/gui/TotpSetupDialog.ui +++ b/src/gui/TotpSetupDialog.ui @@ -14,6 +14,22 @@ Setup TOTP + + + + + 75 + true + + + + Error: secret key is invalid + + + Qt::AlignCenter + + + @@ -210,6 +226,7 @@ customSettingsGroup buttonBox groupBox + invalidKeyLabel seedEdit diff --git a/src/gui/entry/EntryModel.cpp b/src/gui/entry/EntryModel.cpp index 178d710fc..218da6ca8 100644 --- a/src/gui/entry/EntryModel.cpp +++ b/src/gui/entry/EntryModel.cpp @@ -297,7 +297,7 @@ QVariant EntryModel::data(const QModelIndex& index, int role) const break; case Totp: if (entry->hasTotp()) { - return icons()->icon("totp"); + return entry->hasValidTotp() ? icons()->icon("totp") : icons()->icon("totp-invalid"); } break; case PasswordStrength: diff --git a/tests/TestTotp.cpp b/tests/TestTotp.cpp index 0a323f457..13aaf9a2d 100644 --- a/tests/TestTotp.cpp +++ b/tests/TestTotp.cpp @@ -115,18 +115,18 @@ void TestTotp::testTotpCode() // Test 6 digit TOTP (default) quint64 time = 1234567890; - QCOMPARE(Totp::generateTotp(settings, time), QString("005924")); + QCOMPARE(Totp::generateTotp(settings, nullptr, time), QString("005924")); time = 1111111109; - QCOMPARE(Totp::generateTotp(settings, time), QString("081804")); + QCOMPARE(Totp::generateTotp(settings, nullptr, time), QString("081804")); // Test 8 digit TOTP (custom) settings->digits = 8; time = 1111111111; - QCOMPARE(Totp::generateTotp(settings, time), QString("14050471")); + QCOMPARE(Totp::generateTotp(settings, nullptr, time), QString("14050471")); time = 2000000000; - QCOMPARE(Totp::generateTotp(settings, time), QString("69279037")); + QCOMPARE(Totp::generateTotp(settings, nullptr, time), QString("69279037")); } void TestTotp::testSteamTotp() @@ -155,9 +155,9 @@ void TestTotp::testSteamTotp() // Steam mobile app with a throw-away steam account. The above secret was extracted // from the Steam app's data for use in testing here. quint64 time = 1511200518; - QCOMPARE(Totp::generateTotp(settings, time), QString("FR8RV")); + QCOMPARE(Totp::generateTotp(settings, nullptr, time), QString("FR8RV")); time = 1511200714; - QCOMPARE(Totp::generateTotp(settings, time), QString("9P3VP")); + QCOMPARE(Totp::generateTotp(settings, nullptr, time), QString("9P3VP")); } void TestTotp::testEntryHistory()