From 9f3b4dc5ea50c00350570c83c95672bf6c72fc69 Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Mon, 23 Oct 2023 23:22:12 -0400 Subject: [PATCH] Fix multiple TOTP issues * Fix #9847 - don't provide TOTP values if settings are blank or completely wrong * Fix #6838 - don't reset the ui when creating a new entry and applying TOTP to it * Move totp source into the core folder --- src/CMakeLists.txt | 4 ++-- src/core/Entry.cpp | 4 ++-- src/{totp/totp.cpp => core/Totp.cpp} | 12 +++++++++++- src/{totp/totp.h => core/Totp.h} | 0 src/format/OpVaultReaderSections.cpp | 2 +- src/gui/DatabaseWidget.cpp | 4 ++++ src/gui/EntryPreviewWidget.cpp | 2 +- src/gui/TotpDialog.cpp | 2 +- src/gui/TotpExportSettingsDialog.cpp | 2 +- src/gui/TotpSetupDialog.cpp | 2 +- src/gui/csvImport/CsvImportWidget.cpp | 4 ++-- src/gui/entry/EditEntryWidget.cpp | 12 ++++++++++-- src/gui/entry/EditEntryWidget.h | 1 + tests/TestCsvExporter.cpp | 2 +- tests/TestOpVaultReader.cpp | 2 +- tests/TestTotp.cpp | 14 +++++++++++++- 16 files changed, 52 insertions(+), 17 deletions(-) rename src/{totp/totp.cpp => core/Totp.cpp} (97%) rename src/{totp/totp.h => core/Totp.h} (100%) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index f52b02d06..2f7bebd87 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -59,6 +59,7 @@ set(keepassx_SOURCES core/TimeDelta.cpp core/TimeInfo.cpp core/Tools.cpp + core/Totp.cpp core/Translator.cpp core/UrlTools.cpp cli/Utils.cpp @@ -194,8 +195,7 @@ set(keepassx_SOURCES streams/qtiocompressor.cpp streams/StoreDataStream.cpp streams/SymmetricCipherStream.cpp - quickunlock/QuickUnlockInterface.cpp - totp/totp.cpp) + quickunlock/QuickUnlockInterface.cpp) if(APPLE) set(keepassx_SOURCES ${keepassx_SOURCES} diff --git a/src/core/Entry.cpp b/src/core/Entry.cpp index ecd4228e6..d384654a6 100644 --- a/src/core/Entry.cpp +++ b/src/core/Entry.cpp @@ -24,7 +24,7 @@ #include "core/Metadata.h" #include "core/PasswordHealth.h" #include "core/Tools.h" -#include "totp/totp.h" +#include "core/Totp.h" #include #include @@ -566,7 +566,7 @@ void Entry::setTotp(QSharedPointer settings) m_attributes->remove(Totp::ATTRIBUTE_SEED); m_attributes->remove(Totp::ATTRIBUTE_SETTINGS); - if (settings->key.isEmpty()) { + if (!settings || settings->key.isEmpty()) { m_data.totpSettings.reset(); } else { m_data.totpSettings = std::move(settings); diff --git a/src/totp/totp.cpp b/src/core/Totp.cpp similarity index 97% rename from src/totp/totp.cpp rename to src/core/Totp.cpp index cd98d38a1..4c491a6e7 100644 --- a/src/totp/totp.cpp +++ b/src/core/Totp.cpp @@ -16,7 +16,7 @@ * along with this program. If not, see . */ -#include "totp.h" +#include "Totp.h" #include "core/Base32.h" #include "core/Clock.h" @@ -59,6 +59,11 @@ static QString getNameForHashType(const Totp::Algorithm hashType) QSharedPointer Totp::parseSettings(const QString& rawSettings, const QString& key) { + // Early out if both strings are empty + if (rawSettings.isEmpty() && key.isEmpty()) { + return {}; + } + // Create default settings auto settings = createSettings(key, DEFAULT_DIGITS, DEFAULT_STEP); @@ -96,6 +101,11 @@ QSharedPointer Totp::parseSettings(const QString& rawSettings, c settings->algorithm = getHashTypeByName(query.queryItemValue("otpHashMode")); } } else { + if (settings->key.isEmpty()) { + // Legacy format cannot work with an empty key + return {}; + } + // Parse semi-colon separated values ([step];[digits|S]) settings->format = StorageFormat::LEGACY; auto vars = rawSettings.split(";"); diff --git a/src/totp/totp.h b/src/core/Totp.h similarity index 100% rename from src/totp/totp.h rename to src/core/Totp.h diff --git a/src/format/OpVaultReaderSections.cpp b/src/format/OpVaultReaderSections.cpp index 661b9d6c3..d05f8fca1 100644 --- a/src/format/OpVaultReaderSections.cpp +++ b/src/format/OpVaultReaderSections.cpp @@ -18,7 +18,7 @@ #include "OpVaultReader.h" #include "core/Entry.h" -#include "totp/totp.h" +#include "core/Totp.h" #include #include diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index b20f5a240..c9dce441d 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -497,6 +497,10 @@ void DatabaseWidget::setupTotp() auto setupTotpDialog = new TotpSetupDialog(this, currentEntry); connect(setupTotpDialog, SIGNAL(totpUpdated()), SIGNAL(entrySelectionChanged())); + if (currentWidget() == m_editEntryWidget) { + // Entry is being edited, tell it when we are finished updating TOTP + connect(setupTotpDialog, SIGNAL(totpUpdated()), m_editEntryWidget, SLOT(updateTotp())); + } connect(this, &DatabaseWidget::databaseLockRequested, setupTotpDialog, &TotpSetupDialog::close); setupTotpDialog->open(); } diff --git a/src/gui/EntryPreviewWidget.cpp b/src/gui/EntryPreviewWidget.cpp index 50c94325b..64b85a9a8 100644 --- a/src/gui/EntryPreviewWidget.cpp +++ b/src/gui/EntryPreviewWidget.cpp @@ -21,10 +21,10 @@ #include "Application.h" #include "core/Config.h" +#include "core/Totp.h" #include "gui/Clipboard.h" #include "gui/Font.h" #include "gui/Icons.h" -#include "totp/totp.h" #if defined(WITH_XC_KEESHARE) #include "keeshare/KeeShare.h" #include "keeshare/KeeShareSettings.h" diff --git a/src/gui/TotpDialog.cpp b/src/gui/TotpDialog.cpp index 83d8a6973..40888bd99 100644 --- a/src/gui/TotpDialog.cpp +++ b/src/gui/TotpDialog.cpp @@ -20,9 +20,9 @@ #include "ui_TotpDialog.h" #include "core/Clock.h" +#include "core/Totp.h" #include "gui/Clipboard.h" #include "gui/MainWindow.h" -#include "totp/totp.h" #include #include diff --git a/src/gui/TotpExportSettingsDialog.cpp b/src/gui/TotpExportSettingsDialog.cpp index fa40aa62a..b60657e40 100644 --- a/src/gui/TotpExportSettingsDialog.cpp +++ b/src/gui/TotpExportSettingsDialog.cpp @@ -17,11 +17,11 @@ #include "TotpExportSettingsDialog.h" +#include "core/Totp.h" #include "gui/Clipboard.h" #include "gui/MainWindow.h" #include "gui/SquareSvgWidget.h" #include "qrcode/QrCode.h" -#include "totp/totp.h" #include #include diff --git a/src/gui/TotpSetupDialog.cpp b/src/gui/TotpSetupDialog.cpp index a204401b1..54fa2402a 100644 --- a/src/gui/TotpSetupDialog.cpp +++ b/src/gui/TotpSetupDialog.cpp @@ -19,8 +19,8 @@ #include "ui_TotpSetupDialog.h" #include "core/Base32.h" +#include "core/Totp.h" #include "gui/MessageBox.h" -#include "totp/totp.h" TotpSetupDialog::TotpSetupDialog(QWidget* parent, Entry* entry) : QDialog(parent) diff --git a/src/gui/csvImport/CsvImportWidget.cpp b/src/gui/csvImport/CsvImportWidget.cpp index 6781974f5..56743bd96 100644 --- a/src/gui/csvImport/CsvImportWidget.cpp +++ b/src/gui/csvImport/CsvImportWidget.cpp @@ -22,9 +22,9 @@ #include #include "core/Clock.h" +#include "core/Totp.h" #include "format/KeePass2Writer.h" #include "gui/MessageBox.h" -#include "totp/totp.h" // I wanted to make the CSV import GUI future-proof, so if one day you need a new field, // all you have to do is add a field to m_columnHeader, and the GUI will follow: @@ -206,7 +206,7 @@ void CsvImportWidget::writeDatabase() auto otpString = m_parserModel->data(m_parserModel->index(r, 6)); if (otpString.isValid() && !otpString.toString().isEmpty()) { auto totp = Totp::parseSettings(otpString.toString()); - if (totp->key.isEmpty()) { + if (!totp || totp->key.isEmpty()) { // Bare secret, use default TOTP settings totp = Totp::parseSettings({}, otpString.toString()); } diff --git a/src/gui/entry/EditEntryWidget.cpp b/src/gui/entry/EditEntryWidget.cpp index 9466d0918..042f67a53 100644 --- a/src/gui/entry/EditEntryWidget.cpp +++ b/src/gui/entry/EditEntryWidget.cpp @@ -115,6 +115,7 @@ EditEntryWidget::EditEntryWidget(QWidget* parent) m_entryModifiedTimer.setSingleShot(true); m_entryModifiedTimer.setInterval(0); connect(&m_entryModifiedTimer, &QTimer::timeout, this, [this] { + // TODO: Upon refactor of this widget, this needs to merge unsaved changes in the UI if (isVisible() && m_entry) { setForms(m_entry); } @@ -711,6 +712,13 @@ void EditEntryWidget::toKeeAgentSettings(KeeAgentSettings& settings) const settings.setSaveAttachmentToTempFile(m_sshAgentSettings.saveAttachmentToTempFile()); } +void EditEntryWidget::updateTotp() +{ + if (m_entry) { + m_attributesModel->setEntryAttributes(m_entry->attributes()); + } +} + void EditEntryWidget::browsePrivateKey() { auto fileName = fileDialog()->getOpenFileName(this, tr("Select private key"), FileDialog::getLastDir("sshagent")); @@ -867,8 +875,6 @@ void EditEntryWidget::loadEntry(Entry* entry, m_create = create; m_history = history; - connect(m_entry, &Entry::modified, this, [this] { m_entryModifiedTimer.start(); }); - if (history) { setHeadline(QString("%1 \u2022 %2").arg(parentName, tr("Entry history"))); } else { @@ -876,6 +882,8 @@ void EditEntryWidget::loadEntry(Entry* entry, setHeadline(QString("%1 \u2022 %2").arg(parentName, tr("Add entry"))); } else { setHeadline(QString("%1 \u2022 %2 \u2022 %3").arg(parentName, entry->title(), tr("Edit entry"))); + // Reload entry details if changed outside of the edit dialog + connect(m_entry, &Entry::modified, this, [this] { m_entryModifiedTimer.start(); }); } } diff --git a/src/gui/entry/EditEntryWidget.h b/src/gui/entry/EditEntryWidget.h index 27be1d339..734294837 100644 --- a/src/gui/entry/EditEntryWidget.h +++ b/src/gui/entry/EditEntryWidget.h @@ -118,6 +118,7 @@ private slots: void updateSSHAgentAttachment(); void updateSSHAgentAttachments(); void updateSSHAgentKeyInfo(); + void updateTotp(); void browsePrivateKey(); void addKeyToAgent(); void removeKeyFromAgent(); diff --git a/tests/TestCsvExporter.cpp b/tests/TestCsvExporter.cpp index 4854da111..c4c937e5d 100644 --- a/tests/TestCsvExporter.cpp +++ b/tests/TestCsvExporter.cpp @@ -22,9 +22,9 @@ #include #include "core/Group.h" +#include "core/Totp.h" #include "crypto/Crypto.h" #include "format/CsvExporter.h" -#include "totp/totp.h" QTEST_GUILESS_MAIN(TestCsvExporter) diff --git a/tests/TestOpVaultReader.cpp b/tests/TestOpVaultReader.cpp index a5d05e9b0..4899d335f 100644 --- a/tests/TestOpVaultReader.cpp +++ b/tests/TestOpVaultReader.cpp @@ -20,9 +20,9 @@ #include "config-keepassx-tests.h" #include "core/Group.h" #include "core/Metadata.h" +#include "core/Totp.h" #include "crypto/Crypto.h" #include "format/OpVaultReader.h" -#include "totp/totp.h" #include #include diff --git a/tests/TestTotp.cpp b/tests/TestTotp.cpp index bb3b55dbd..f2bb3d47a 100644 --- a/tests/TestTotp.cpp +++ b/tests/TestTotp.cpp @@ -19,8 +19,8 @@ #include "TestTotp.h" #include "core/Entry.h" +#include "core/Totp.h" #include "crypto/Crypto.h" -#include "totp/totp.h" #include @@ -97,6 +97,14 @@ void TestTotp::testParseSecret() QCOMPARE(settings->digits, 6u); QCOMPARE(settings->step, 30u); QCOMPARE(settings->algorithm, Totp::Algorithm::Sha1); + + // Blank settings (expected failure) + settings = Totp::parseSettings("", ""); + QVERIFY(settings.isNull()); + + // TOTP Settings with blank secret (expected failure) + settings = Totp::parseSettings("30;8", ""); + QVERIFY(settings.isNull()); } void TestTotp::testTotpCode() @@ -164,4 +172,8 @@ void TestTotp::testEntryHistory() entry.setTotp(settings); QCOMPARE(entry.historyItems().size(), 2); QCOMPARE(entry.totpSettings()->key, QString("foo")); + // Nullptr Settings (expected reset of TOTP) + entry.setTotp(nullptr); + QVERIFY(!entry.hasTotp()); + QCOMPARE(entry.historyItems().size(), 3); }