From 901cc1820f471f537553f6cf62798461b8f18865 Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Mon, 6 Apr 2020 08:42:20 -0400 Subject: [PATCH] Significantly enhance hardware key robustness * Significantly improve user experience when using hardware keys on databases in both GUI and CLI modes. Prevent locking up the YubiKey USB interface for prolonged periods of time. Allows for other apps to use the key concurrently with KeePassXC. * Improve messages displayed to user when finding keys and when user interaction is required. Output specific error messages when handling hardware keys during database read/write. * Only poll for keys when previously used or upon user request. Prevent continuously polling keys when accessing the UI such as switching tabs and minimize/maximize. * Add support for using multiple hardware keys simultaneously. Keys are identified by their serial number which prevents using the wrong key during open and save operations. * Fixes #4400 * Fixes #4065 * Fixes #1050 * Fixes #1215 * Fixes #3087 * Fixes #1088 * Fixes #1869 --- src/cli/Command.cpp | 15 +- src/cli/Utils.cpp | 33 ++- src/cli/Utils.h | 6 - src/core/Database.cpp | 26 +- src/core/Database.h | 3 +- src/format/Kdbx3Reader.cpp | 2 +- src/format/Kdbx3Writer.cpp | 2 +- src/format/Kdbx4Reader.cpp | 2 +- src/format/Kdbx4Writer.cpp | 2 +- src/gui/ApplicationSettingsWidget.cpp | 1 + src/gui/DatabaseOpenWidget.cpp | 151 +++++----- src/gui/DatabaseOpenWidget.h | 9 +- src/gui/DatabaseWidget.cpp | 4 +- src/gui/MainWindow.cpp | 9 + src/gui/masterkey/YubiKeyEditWidget.cpp | 85 ++---- src/gui/masterkey/YubiKeyEditWidget.h | 6 +- src/keys/ChallengeResponseKey.h | 14 +- src/keys/CompositeKey.cpp | 17 +- src/keys/CompositeKey.h | 8 +- src/keys/YkChallengeResponseKey.cpp | 81 ++---- src/keys/YkChallengeResponseKey.h | 20 +- src/keys/YkChallengeResponseKeyCLI.cpp | 42 +-- src/keys/YkChallengeResponseKeyCLI.h | 11 +- src/keys/drivers/YubiKey.cpp | 362 ++++++++++++++---------- src/keys/drivers/YubiKey.h | 106 +++---- src/keys/drivers/YubiKeyStub.cpp | 42 +-- tests/TestCli.cpp | 60 +++- tests/TestYkChallengeResponseKey.cpp | 101 +++---- tests/TestYkChallengeResponseKey.h | 26 +- 29 files changed, 582 insertions(+), 664 deletions(-) diff --git a/src/cli/Command.cpp b/src/cli/Command.cpp index e76f633ef2..03e653ad43 100644 --- a/src/cli/Command.cpp +++ b/src/cli/Command.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include "Command.h" @@ -72,8 +73,8 @@ const QCommandLineOption Command::NoPasswordOption = const QCommandLineOption Command::YubiKeyOption = QCommandLineOption(QStringList() << "y" << "yubikey", - QObject::tr("Yubikey slot used to encrypt the database."), - QObject::tr("slot")); + QObject::tr("Yubikey slot and optional serial used to access the database (e.g., 1:7370001)."), + QObject::tr("slot[:serial]")); namespace { @@ -121,7 +122,15 @@ QString Command::getDescriptionLine() QString Command::getHelpText() { - return buildParser(this)->helpText().replace("[options]", name + " [options]"); + auto help = buildParser(this)->helpText(); + // Fix spacing of options parameter + help.replace("[options]", name + " [options]"); + // Remove application directory from command line example + auto appname = QFileInfo(QCoreApplication::applicationFilePath()).fileName(); + auto regex = QRegularExpression(QString(" .*%1").arg(QRegularExpression::escape(appname))); + help.replace(regex, appname.prepend(" ")); + + return help; } QSharedPointer Command::getCommandLineParser(const QStringList& arguments) diff --git a/src/cli/Utils.cpp b/src/cli/Utils.cpp index ab7c23186b..93f577f457 100644 --- a/src/cli/Utils.cpp +++ b/src/cli/Utils.cpp @@ -17,6 +17,10 @@ #include "Utils.h" +#ifdef WITH_XC_YUBIKEY +#include "keys/YkChallengeResponseKeyCLI.h" +#endif + #ifdef Q_OS_WIN #include #else @@ -159,25 +163,30 @@ namespace Utils #ifdef WITH_XC_YUBIKEY if (!yubiKeySlot.isEmpty()) { + unsigned int serial = 0; + int slot; + bool ok = false; - int slot = yubiKeySlot.toInt(&ok, 10); + auto parts = yubiKeySlot.split(":"); + slot = parts[0].toInt(&ok); + if (!ok || (slot != 1 && slot != 2)) { - err << QObject::tr("Invalid YubiKey slot %1").arg(yubiKeySlot) << endl; + err << QObject::tr("Invalid YubiKey slot %1").arg(parts[0]) << endl; return {}; } - QString errorMessage; - bool blocking = YubiKey::instance()->checkSlotIsBlocking(slot, errorMessage); - if (!errorMessage.isEmpty()) { - err << errorMessage << endl; - return {}; + if (parts.size() > 1) { + serial = parts[1].toUInt(&ok, 10); + if (!ok) { + err << QObject::tr("Invalid YubiKey serial %1").arg(parts[1]) << endl; + return {}; + } } - auto key = QSharedPointer(new YkChallengeResponseKeyCLI( - slot, - blocking, - QObject::tr("Please touch the button on your YubiKey to unlock %1").arg(databaseFilename), - outputDescriptor)); + auto key = QSharedPointer( + new YkChallengeResponseKeyCLI({serial, slot}, + QObject::tr("Please touch the button on your YubiKey to continue..."), + errorDescriptor)); compositeKey->addChallengeResponseKey(key); } #else diff --git a/src/cli/Utils.h b/src/cli/Utils.h index 512b209d6c..9802a1574f 100644 --- a/src/cli/Utils.h +++ b/src/cli/Utils.h @@ -26,12 +26,6 @@ #include "keys/PasswordKey.h" #include -#ifdef WITH_XC_YUBIKEY -#include "keys/YkChallengeResponseKey.h" -#include "keys/YkChallengeResponseKeyCLI.h" -#include "keys/drivers/YubiKey.h" -#endif - namespace Utils { extern FILE* STDOUT; diff --git a/src/core/Database.cpp b/src/core/Database.cpp index 66af624ecb..cae937d572 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -659,10 +659,11 @@ QByteArray Database::challengeResponseKey() const bool Database::challengeMasterSeed(const QByteArray& masterSeed) { + m_keyError.clear(); if (m_data.key) { m_data.masterSeed->setHash(masterSeed); QByteArray response; - bool ok = m_data.key->challenge(masterSeed, response); + bool ok = m_data.key->challenge(masterSeed, response, &m_keyError); if (ok && !response.isEmpty()) { m_data.challengeResponseKey->setHash(response); } else if (ok && response.isEmpty()) { @@ -703,6 +704,7 @@ bool Database::setKey(const QSharedPointer& key, bool transformKey) { Q_ASSERT(!m_data.isReadOnly); + m_keyError.clear(); if (!key) { m_data.key.reset(); @@ -724,7 +726,7 @@ bool Database::setKey(const QSharedPointer& key, if (!transformKey) { transformedMasterKey = QByteArray(oldTransformedMasterKey.rawKey()); - } else if (!key->transform(*m_data.kdf, transformedMasterKey)) { + } else if (!key->transform(*m_data.kdf, transformedMasterKey, &m_keyError)) { return false; } @@ -743,25 +745,9 @@ bool Database::setKey(const QSharedPointer& key, return true; } -bool Database::verifyKey(const QSharedPointer& key) const +QString Database::keyError() { - Q_ASSERT(!m_data.key->isEmpty()); - - if (!m_data.challengeResponseKey->rawKey().isEmpty()) { - QByteArray result; - - if (!key->challenge(m_data.masterSeed->rawKey(), result)) { - // challenge failed, (YubiKey?) removed? - return false; - } - - if (m_data.challengeResponseKey->rawKey() != result) { - // wrong response from challenged device(s) - return false; - } - } - - return (m_data.key->rawKey() == key->rawKey()); + return m_keyError; } QVariantMap& Database::publicCustomData() diff --git a/src/core/Database.h b/src/core/Database.h index 63a1f8cb6c..8feb7e56bf 100644 --- a/src/core/Database.h +++ b/src/core/Database.h @@ -119,9 +119,9 @@ class Database : public QObject bool updateChangedTime = true, bool updateTransformSalt = false, bool transformKey = true); + QString keyError(); QByteArray challengeResponseKey() const; bool challengeMasterSeed(const QByteArray& masterSeed); - bool verifyKey(const QSharedPointer& key) const; const QUuid& cipher() const; void setCipher(const QUuid& cipher); Database::CompressionAlgorithm compressionAlgorithm() const; @@ -210,6 +210,7 @@ public slots: QPointer m_fileWatcher; bool m_modified = false; bool m_emitModified; + QString m_keyError; QList m_commonUsernames; diff --git a/src/format/Kdbx3Reader.cpp b/src/format/Kdbx3Reader.cpp index cce46deb40..002aa8af0e 100644 --- a/src/format/Kdbx3Reader.cpp +++ b/src/format/Kdbx3Reader.cpp @@ -55,7 +55,7 @@ bool Kdbx3Reader::readDatabaseImpl(QIODevice* device, } if (!db->challengeMasterSeed(m_masterSeed)) { - raiseError(tr("Unable to issue challenge-response.")); + raiseError(tr("Unable to issue challenge-response: %1").arg(db->keyError())); return false; } diff --git a/src/format/Kdbx3Writer.cpp b/src/format/Kdbx3Writer.cpp index 0728dc294c..7d492e4e71 100644 --- a/src/format/Kdbx3Writer.cpp +++ b/src/format/Kdbx3Writer.cpp @@ -42,7 +42,7 @@ bool Kdbx3Writer::writeDatabase(QIODevice* device, Database* db) QByteArray endOfHeader = "\r\n\r\n"; if (!db->challengeMasterSeed(masterSeed)) { - raiseError(tr("Unable to issue challenge-response.")); + raiseError(tr("Unable to issue challenge-response: %1").arg(db->keyError())); return false; } diff --git a/src/format/Kdbx4Reader.cpp b/src/format/Kdbx4Reader.cpp index ebdf634a1d..f5e7382fe0 100644 --- a/src/format/Kdbx4Reader.cpp +++ b/src/format/Kdbx4Reader.cpp @@ -50,7 +50,7 @@ bool Kdbx4Reader::readDatabaseImpl(QIODevice* device, bool ok = AsyncTask::runAndWaitForFuture([&] { return db->setKey(key, false, false); }); if (!ok) { - raiseError(tr("Unable to calculate master key")); + raiseError(tr("Unable to calculate master key: %1").arg(db->keyError())); return false; } diff --git a/src/format/Kdbx4Writer.cpp b/src/format/Kdbx4Writer.cpp index 03d549cf0e..57211248e9 100644 --- a/src/format/Kdbx4Writer.cpp +++ b/src/format/Kdbx4Writer.cpp @@ -53,7 +53,7 @@ bool Kdbx4Writer::writeDatabase(QIODevice* device, Database* db) QByteArray endOfHeader = "\r\n\r\n"; if (!db->setKey(db->key(), false, true)) { - raiseError(tr("Unable to calculate master key")); + raiseError(tr("Unable to calculate master key: %1").arg(db->keyError())); return false; } diff --git a/src/gui/ApplicationSettingsWidget.cpp b/src/gui/ApplicationSettingsWidget.cpp index 3424a46fb1..5d4cb77b2f 100644 --- a/src/gui/ApplicationSettingsWidget.cpp +++ b/src/gui/ApplicationSettingsWidget.cpp @@ -374,6 +374,7 @@ void ApplicationSettingsWidget::saveSettings() if (!config()->get("RememberLastKeyFiles").toBool()) { config()->set("LastKeyFiles", {}); + config()->set("LastChallengeResponse", {}); config()->set("LastDir", ""); } diff --git a/src/gui/DatabaseOpenWidget.cpp b/src/gui/DatabaseOpenWidget.cpp index 31391b12b4..4599ae4dc6 100644 --- a/src/gui/DatabaseOpenWidget.cpp +++ b/src/gui/DatabaseOpenWidget.cpp @@ -37,7 +37,6 @@ #include #include #include -#include DatabaseOpenWidget::DatabaseOpenWidget(QWidget* parent) : DialogyWidget(parent) @@ -78,7 +77,18 @@ DatabaseOpenWidget::DatabaseOpenWidget(QWidget* parent) sp.setRetainSizeWhenHidden(true); m_ui->yubikeyProgress->setSizePolicy(sp); - connect(m_ui->buttonRedetectYubikey, SIGNAL(clicked()), SLOT(pollYubikey())); + connect(m_ui->buttonRedetectYubikey, SIGNAL(clicked()), SLOT(pollHardwareKey())); + connect(YubiKey::instance(), SIGNAL(detectComplete(bool)), SLOT(hardwareKeyResponse(bool)), Qt::QueuedConnection); + + connect(YubiKey::instance(), &YubiKey::userInteractionRequest, this, [this] { + // Show the press notification if we are in an independent window (e.g., DatabaseOpenDialog) + if (window() != getMainWindow()) { + m_ui->messageWidget->showMessage(tr("Please touch the button on your YubiKey!"), + MessageWidget::Information, + MessageWidget::DisableAutoHide); + } + }); + connect(YubiKey::instance(), &YubiKey::challengeCompleted, this, [this] { m_ui->messageWidget->hide(); }); #else m_ui->hardwareKeyLabel->setVisible(false); m_ui->hardwareKeyLabelHelp->setVisible(false); @@ -104,37 +114,16 @@ void DatabaseOpenWidget::showEvent(QShowEvent* event) { DialogyWidget::showEvent(event); m_ui->editPassword->setFocus(); - -#ifdef WITH_XC_YUBIKEY - // showEvent() may be called twice, so make sure we are only polling once - if (!m_yubiKeyBeingPolled) { - // clang-format off - connect(YubiKey::instance(), SIGNAL(detected(int,bool)), SLOT(yubikeyDetected(int,bool)), Qt::QueuedConnection); - connect(YubiKey::instance(), SIGNAL(detectComplete()), SLOT(yubikeyDetectComplete()), Qt::QueuedConnection); - connect(YubiKey::instance(), SIGNAL(notFound()), SLOT(noYubikeyFound()), Qt::QueuedConnection); - // clang-format on - - pollYubikey(); - m_yubiKeyBeingPolled = true; - } -#endif } void DatabaseOpenWidget::hideEvent(QHideEvent* event) { DialogyWidget::hideEvent(event); -#ifdef WITH_XC_YUBIKEY - // Don't listen to any Yubikey events if we are hidden - disconnect(YubiKey::instance(), nullptr, this, nullptr); - m_yubiKeyBeingPolled = false; -#endif - - if (isVisible()) { - return; + // Clear the forms if we are minimized + if (!isVisible()) { + clearForms(); } - - clearForms(); } void DatabaseOpenWidget::load(const QString& filename) @@ -148,7 +137,7 @@ void DatabaseOpenWidget::load(const QString& filename) m_keyFileComboEdited = false; if (config()->get("RememberLastKeyFiles").toBool()) { - QHash lastKeyFiles = config()->get("LastKeyFiles").toHash(); + auto lastKeyFiles = config()->get("LastKeyFiles").toHash(); if (lastKeyFiles.contains(m_filename)) { m_ui->comboKeyFile->addItem(lastKeyFiles[m_filename].toString()); m_ui->comboKeyFile->setCurrentIndex(1); @@ -157,6 +146,17 @@ void DatabaseOpenWidget::load(const QString& filename) QHash useTouchID = config()->get("UseTouchID").toHash(); m_ui->checkTouchID->setChecked(useTouchID.value(m_filename, false).toBool()); + +#ifdef WITH_XC_YUBIKEY + // Only auto-poll for hardware keys if we previously used one with this database file + if (config()->get("RememberLastKeyFiles").toBool()) { + auto variant = config()->get("LastChallengeResponse"); + auto lastChallengeResponse = config()->get("LastChallengeResponse").toHash(); + if (lastChallengeResponse.contains(m_filename)) { + pollHardwareKey(); + } + } +#endif } void DatabaseOpenWidget::clearForms() @@ -176,6 +176,11 @@ QSharedPointer DatabaseOpenWidget::database() return m_db; } +QString DatabaseOpenWidget::filename() +{ + return m_filename; +} + void DatabaseOpenWidget::enterKey(const QString& pw, const QString& keyFile) { m_ui->editPassword->setText(pw); @@ -186,6 +191,8 @@ void DatabaseOpenWidget::enterKey(const QString& pw, const QString& keyFile) void DatabaseOpenWidget::openDatabase() { + m_ui->messageWidget->hide(); + QSharedPointer masterKey = databaseKey(); if (!masterKey) { return; @@ -223,11 +230,6 @@ void DatabaseOpenWidget::openDatabase() config()->set("UseTouchID", useTouchID); #endif - - if (m_ui->messageWidget->isVisible()) { - m_ui->messageWidget->animatedHide(); - } - emit dialogFinished(true); m_isOpeningDatabase = false; clearForms(); @@ -293,7 +295,7 @@ QSharedPointer DatabaseOpenWidget::databaseKey() } #endif - QHash lastKeyFiles = config()->get("LastKeyFiles").toHash(); + auto lastKeyFiles = config()->get("LastKeyFiles").toHash(); lastKeyFiles.remove(m_filename); auto key = QSharedPointer::create(); @@ -315,14 +317,14 @@ QSharedPointer DatabaseOpenWidget::databaseKey() legacyWarning.setDefaultButton(QMessageBox::Ok); legacyWarning.setCheckBox(new QCheckBox(tr("Don't show this warning again"))); - connect(legacyWarning.checkBox(), &QCheckBox::stateChanged, [](int state) { + connect(legacyWarning.checkBox(), &QCheckBox::stateChanged, this, [](int state) { config()->set("Messages/NoLegacyKeyFileWarning", state == Qt::CheckState::Checked); }); legacyWarning.exec(); } masterKey->addKey(key); - lastKeyFiles[m_filename] = keyFilename; + lastKeyFiles.insert(m_filename, keyFilename); } if (config()->get("RememberLastKeyFiles").toBool()) { @@ -330,19 +332,17 @@ QSharedPointer DatabaseOpenWidget::databaseKey() } #ifdef WITH_XC_YUBIKEY - QHash lastChallengeResponse = config()->get("LastChallengeResponse").toHash(); + auto lastChallengeResponse = config()->get("LastChallengeResponse").toHash(); lastChallengeResponse.remove(m_filename); int selectionIndex = m_ui->comboChallengeResponse->currentIndex(); if (selectionIndex > 0) { - int comboPayload = m_ui->comboChallengeResponse->itemData(selectionIndex).toInt(); - - // read blocking mode from LSB and slot index number from second LSB - bool blocking = comboPayload & 1; - int slot = comboPayload >> 1; - auto crKey = QSharedPointer(new YkChallengeResponseKey(slot, blocking)); + auto slot = m_ui->comboChallengeResponse->itemData(selectionIndex).value(); + auto crKey = QSharedPointer(new YkChallengeResponseKey(slot)); masterKey->addChallengeResponseKey(crKey); - lastChallengeResponse[m_filename] = true; + + // Qt doesn't read custom types in settings so stuff into a QString + lastChallengeResponse.insert(m_filename, QString("%1:%2").arg(slot.first).arg(slot.second)); } if (config()->get("RememberLastKeyFiles").toBool()) { @@ -400,45 +400,62 @@ void DatabaseOpenWidget::handleKeyFileComboChanged() m_ui->keyFileClearIcon->setVisible(m_keyFileComboEdited); } -void DatabaseOpenWidget::pollYubikey() +void DatabaseOpenWidget::pollHardwareKey() { + if (m_yubiKeyBeingPolled) { + return; + } + + m_ui->comboChallengeResponse->clear(); + m_ui->comboChallengeResponse->addItem(tr("Detecting Hardware Keys...")); + m_ui->buttonRedetectYubikey->setEnabled(false); m_ui->comboChallengeResponse->setEnabled(false); - m_ui->comboChallengeResponse->clear(); - m_ui->comboChallengeResponse->addItem(tr("Select slot..."), -1); m_ui->yubikeyProgress->setVisible(true); + m_yubiKeyBeingPolled = true; - // YubiKey init is slow, detect asynchronously to not block the UI - QtConcurrent::run(YubiKey::instance(), &YubiKey::detect); + YubiKey::instance()->findValidKeys(); } -void DatabaseOpenWidget::yubikeyDetected(int slot, bool blocking) +void DatabaseOpenWidget::hardwareKeyResponse(bool found) { - YkChallengeResponseKey yk(slot, blocking); - // add detected YubiKey to combo box and encode blocking mode in LSB, slot number in second LSB - m_ui->comboChallengeResponse->addItem(yk.getName(), QVariant((slot << 1) | blocking)); + m_ui->comboChallengeResponse->clear(); + m_ui->buttonRedetectYubikey->setEnabled(true); + m_ui->yubikeyProgress->setVisible(false); + m_yubiKeyBeingPolled = false; + if (!found) { + m_ui->comboChallengeResponse->addItem(tr("No Hardware Keys Detected")); + m_ui->comboChallengeResponse->setEnabled(false); + return; + } else { + m_ui->comboChallengeResponse->addItem(tr("Select Hardware Key...")); + } + + YubiKeySlot lastUsedSlot; if (config()->get("RememberLastKeyFiles").toBool()) { - QHash lastChallengeResponse = config()->get("LastChallengeResponse").toHash(); + auto lastChallengeResponse = config()->get("LastChallengeResponse").toHash(); if (lastChallengeResponse.contains(m_filename)) { - m_ui->comboChallengeResponse->setCurrentIndex(1); + // Qt doesn't read custom types in settings so extract from QString + auto split = lastChallengeResponse.value(m_filename).toString().split(":"); + if (split.size() > 1) { + lastUsedSlot = YubiKeySlot(split[0].toUInt(), split[1].toInt()); + } } } -} -void DatabaseOpenWidget::yubikeyDetectComplete() -{ - m_ui->comboChallengeResponse->setEnabled(true); - m_ui->buttonRedetectYubikey->setEnabled(true); - m_ui->yubikeyProgress->setVisible(false); - m_yubiKeyBeingPolled = false; -} + int selectedIndex = 0; + for (auto& slot : YubiKey::instance()->foundKeys()) { + // add detected YubiKey to combo box + m_ui->comboChallengeResponse->addItem(YubiKey::instance()->getDisplayName(slot), QVariant::fromValue(slot)); + // Select this YubiKey + Slot if we used it in the past + if (lastUsedSlot == slot) { + selectedIndex = m_ui->comboChallengeResponse->count() - 1; + } + } -void DatabaseOpenWidget::noYubikeyFound() -{ - m_ui->buttonRedetectYubikey->setEnabled(true); - m_ui->yubikeyProgress->setVisible(false); - m_yubiKeyBeingPolled = false; + m_ui->comboChallengeResponse->setCurrentIndex(selectedIndex); + m_ui->comboChallengeResponse->setEnabled(true); } void DatabaseOpenWidget::openHardwareKeyHelp() diff --git a/src/gui/DatabaseOpenWidget.h b/src/gui/DatabaseOpenWidget.h index 61a220f438..969a476d49 100644 --- a/src/gui/DatabaseOpenWidget.h +++ b/src/gui/DatabaseOpenWidget.h @@ -40,13 +40,11 @@ class DatabaseOpenWidget : public DialogyWidget explicit DatabaseOpenWidget(QWidget* parent = nullptr); ~DatabaseOpenWidget(); void load(const QString& filename); + QString filename(); void clearForms(); void enterKey(const QString& pw, const QString& keyFile); QSharedPointer database(); -public slots: - void pollYubikey(); - signals: void dialogFinished(bool accepted); @@ -64,9 +62,8 @@ private slots: void clearKeyFileEdit(); void handleKeyFileComboEdited(); void handleKeyFileComboChanged(); - void yubikeyDetected(int slot, bool blocking); - void yubikeyDetectComplete(); - void noYubikeyFound(); + void pollHardwareKey(); + void hardwareKeyResponse(bool found); void openHardwareKeyHelp(); void openKeyFileHelp(); diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 2078adca22..b261a5bd7a 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -1183,7 +1183,9 @@ void DatabaseWidget::switchToDatabaseSettings() void DatabaseWidget::switchToOpenDatabase() { - switchToOpenDatabase(m_db->filePath()); + if (currentWidget() != m_databaseOpenWidget || m_databaseOpenWidget->filename() != m_db->filePath()) { + switchToOpenDatabase(m_db->filePath()); + } } void DatabaseWidget::switchToOpenDatabase(const QString& filePath) diff --git a/src/gui/MainWindow.cpp b/src/gui/MainWindow.cpp index d4257a7725..028fb91a0b 100644 --- a/src/gui/MainWindow.cpp +++ b/src/gui/MainWindow.cpp @@ -65,6 +65,10 @@ #include "fdosecrets/FdoSecretsPlugin.h" #endif +#ifdef WITH_XC_YUBIKEY +#include "keys/drivers/YubiKey.h" +#endif + #ifdef WITH_XC_BROWSER #include "browser/BrowserOptionDialog.h" #include "browser/BrowserSettings.h" @@ -221,6 +225,11 @@ MainWindow::MainWindow() m_ui->settingsWidget->addSettingsPage(fdoSS); #endif +#ifdef WITH_XC_YUBIKEY + connect(YubiKey::instance(), SIGNAL(userInteractionRequest()), SLOT(showYubiKeyPopup()), Qt::QueuedConnection); + connect(YubiKey::instance(), SIGNAL(challengeCompleted()), SLOT(hideYubiKeyPopup()), Qt::QueuedConnection); +#endif + setWindowIcon(resources()->applicationIcon()); m_ui->globalMessageWidget->setHidden(true); // clang-format off diff --git a/src/gui/masterkey/YubiKeyEditWidget.cpp b/src/gui/masterkey/YubiKeyEditWidget.cpp index 9ecd918ef1..d07bed762c 100644 --- a/src/gui/masterkey/YubiKeyEditWidget.cpp +++ b/src/gui/masterkey/YubiKeyEditWidget.cpp @@ -36,6 +36,8 @@ YubiKeyEditWidget::YubiKeyEditWidget(QWidget* parent) "for additional security.

The YubiKey requires one of its slots to be programmed as " "" "HMAC-SHA1 Challenge-Response.

")); + + connect(YubiKey::instance(), SIGNAL(detectComplete(bool)), SLOT(hardwareKeyResponse(bool)), Qt::QueuedConnection); } YubiKeyEditWidget::~YubiKeyEditWidget() @@ -44,24 +46,22 @@ YubiKeyEditWidget::~YubiKeyEditWidget() bool YubiKeyEditWidget::addToCompositeKey(QSharedPointer key) { - QSharedPointer keyPtr; - if (!createCrKey(keyPtr, false)) { + if (!m_isDetected || !m_compEditWidget) { return false; } - key->addChallengeResponseKey(keyPtr); + int selectionIndex = m_compUi->comboChallengeResponse->currentIndex(); + auto slot = m_compUi->comboChallengeResponse->itemData(selectionIndex).value(); + key->addChallengeResponseKey(QSharedPointer::create(slot)); return true; } bool YubiKeyEditWidget::validate(QString& errorMessage) const { - QSharedPointer keyPtr; - if (!createCrKey(keyPtr)) { - errorMessage = tr("No YubiKey detected, please ensure it's plugged in."); - return false; + if (!m_isDetected) { + errorMessage = tr("Could not find any hardware keys!"); } - - return true; + return m_isDetected; } QWidget* YubiKeyEditWidget::componentEditWidget() @@ -76,13 +76,6 @@ QWidget* YubiKeyEditWidget::componentEditWidget() #ifdef WITH_XC_YUBIKEY connect(m_compUi->buttonRedetectYubikey, SIGNAL(clicked()), SLOT(pollYubikey())); - - // clang-format off - connect(YubiKey::instance(), SIGNAL(detected(int,bool)), SLOT(yubikeyDetected(int,bool)), Qt::QueuedConnection); - connect(YubiKey::instance(), SIGNAL(detectComplete()), SLOT(yubikeyDetectComplete()), Qt::QueuedConnection); - connect(YubiKey::instance(), SIGNAL(notFound()), SLOT(noYubikeyFound()), Qt::QueuedConnection); - // clang-format on - pollYubikey(); #endif @@ -105,72 +98,36 @@ void YubiKeyEditWidget::pollYubikey() m_isDetected = false; m_compUi->comboChallengeResponse->clear(); + m_compUi->comboChallengeResponse->addItem(tr("Detecting Hardware Keys...")); m_compUi->buttonRedetectYubikey->setEnabled(false); m_compUi->comboChallengeResponse->setEnabled(false); m_compUi->yubikeyProgress->setVisible(true); - // YubiKey init is slow, detect asynchronously to not block the UI - QtConcurrent::run(YubiKey::instance(), &YubiKey::detect); + YubiKey::instance()->findValidKeys(); #endif } -void YubiKeyEditWidget::yubikeyDetected(int slot, bool blocking) +void YubiKeyEditWidget::hardwareKeyResponse(bool found) { -#ifdef WITH_XC_YUBIKEY if (!m_compEditWidget) { return; } - YkChallengeResponseKey yk(slot, blocking); - // add detected YubiKey to combo box and encode blocking mode in LSB, slot number in second LSB - m_compUi->comboChallengeResponse->addItem(yk.getName(), QVariant((slot << 1u) | blocking)); - m_isDetected = true; -#else - Q_UNUSED(slot); - Q_UNUSED(blocking); -#endif -} - -void YubiKeyEditWidget::yubikeyDetectComplete() -{ - m_compUi->comboChallengeResponse->setEnabled(true); - m_compUi->buttonRedetectYubikey->setEnabled(true); - m_compUi->yubikeyProgress->setVisible(false); -} -void YubiKeyEditWidget::noYubikeyFound() -{ -#ifdef WITH_XC_YUBIKEY - if (!m_compEditWidget) { - return; - } m_compUi->comboChallengeResponse->clear(); - m_compUi->comboChallengeResponse->setEnabled(false); - m_compUi->comboChallengeResponse->addItem(tr("No YubiKey inserted.")); m_compUi->buttonRedetectYubikey->setEnabled(true); m_compUi->yubikeyProgress->setVisible(false); - m_isDetected = false; -#endif -} -bool YubiKeyEditWidget::createCrKey(QSharedPointer& key, bool testChallenge) const -{ - Q_ASSERT(m_compEditWidget); - if (!m_isDetected || !m_compEditWidget) { - return false; + if (!found) { + m_compUi->comboChallengeResponse->addItem(tr("No Hardware Keys Detected")); + m_isDetected = false; + return; } - int selectionIndex = m_compUi->comboChallengeResponse->currentIndex(); - int comboPayload = m_compUi->comboChallengeResponse->itemData(selectionIndex).toInt(); - - if (0 == comboPayload) { - return false; + for (auto& slot : YubiKey::instance()->foundKeys()) { + // add detected YubiKey to combo box and encode blocking mode in LSB, slot number in second LSB + m_compUi->comboChallengeResponse->addItem(YubiKey::instance()->getDisplayName(slot), QVariant::fromValue(slot)); } - auto blocking = static_cast(comboPayload & 1u); - int slot = comboPayload >> 1u; - key.reset(new YkChallengeResponseKey(slot, blocking)); - if (testChallenge) { - return key->challenge(QByteArray("0000")); - } - return true; + m_isDetected = true; + m_compUi->comboChallengeResponse->setEnabled(true); } diff --git a/src/gui/masterkey/YubiKeyEditWidget.h b/src/gui/masterkey/YubiKeyEditWidget.h index 549f7f44fd..d5be0a683b 100644 --- a/src/gui/masterkey/YubiKeyEditWidget.h +++ b/src/gui/masterkey/YubiKeyEditWidget.h @@ -45,14 +45,10 @@ class YubiKeyEditWidget : public KeyComponentWidget void initComponentEditWidget(QWidget* widget) override; private slots: - void yubikeyDetected(int slot, bool blocking); - void yubikeyDetectComplete(); - void noYubikeyFound(); + void hardwareKeyResponse(bool found); void pollYubikey(); private: - bool createCrKey(QSharedPointer& key, bool testChallenge = true) const; - const QScopedPointer m_compUi; QPointer m_compEditWidget; bool m_isDetected = false; diff --git a/src/keys/ChallengeResponseKey.h b/src/keys/ChallengeResponseKey.h index 8d1fa57746..263b507e0e 100644 --- a/src/keys/ChallengeResponseKey.h +++ b/src/keys/ChallengeResponseKey.h @@ -29,18 +29,24 @@ class ChallengeResponseKey : m_uuid(uuid) { } - Q_DISABLE_COPY(ChallengeResponseKey); - virtual ~ChallengeResponseKey() - { - } + virtual ~ChallengeResponseKey() = default; + virtual QByteArray rawKey() const = 0; virtual bool challenge(const QByteArray& challenge) = 0; virtual QUuid uuid() const { return m_uuid; } + QString error() const + { + return m_error; + } + +protected: + QString m_error; private: + Q_DISABLE_COPY(ChallengeResponseKey); QUuid m_uuid; }; diff --git a/src/keys/CompositeKey.cpp b/src/keys/CompositeKey.cpp index 3eb4691cd9..23c830f9fa 100644 --- a/src/keys/CompositeKey.cpp +++ b/src/keys/CompositeKey.cpp @@ -60,7 +60,7 @@ bool CompositeKey::isEmpty() const */ QByteArray CompositeKey::rawKey() const { - return rawKey(nullptr, nullptr); + return rawKey(nullptr); } /** @@ -73,7 +73,7 @@ QByteArray CompositeKey::rawKey() const * @param ok true if challenges were successful and all key components could be added to the composite key * @return key hash */ -QByteArray CompositeKey::rawKey(const QByteArray* transformSeed, bool* ok) const +QByteArray CompositeKey::rawKey(const QByteArray* transformSeed, bool* ok, QString* error) const { CryptoHash cryptoHash(CryptoHash::Sha256); @@ -87,7 +87,7 @@ QByteArray CompositeKey::rawKey(const QByteArray* transformSeed, bool* ok) const if (transformSeed) { QByteArray challengeResult; - bool challengeOk = challenge(*transformSeed, challengeResult); + bool challengeOk = challenge(*transformSeed, challengeResult, error); if (ok) { *ok = challengeOk; } @@ -110,7 +110,7 @@ QByteArray CompositeKey::rawKey(const QByteArray* transformSeed, bool* ok) const * @param result transformed key hash * @return true on success */ -bool CompositeKey::transform(const Kdf& kdf, QByteArray& result) const +bool CompositeKey::transform(const Kdf& kdf, QByteArray& result, QString* error) const { if (kdf.uuid() == KeePass2::KDF_AES_KDBX3) { // legacy KDBX3 AES-KDF, challenge response is added later to the hash @@ -120,10 +120,10 @@ bool CompositeKey::transform(const Kdf& kdf, QByteArray& result) const QByteArray seed = kdf.seed(); Q_ASSERT(!seed.isEmpty()); bool ok = false; - return kdf.transform(rawKey(&seed, &ok), result) && ok; + return kdf.transform(rawKey(&seed, &ok, error), result) && ok; } -bool CompositeKey::challenge(const QByteArray& seed, QByteArray& result) const +bool CompositeKey::challenge(const QByteArray& seed, QByteArray& result, QString* error) const { // if no challenge response was requested, return nothing to // maintain backwards compatibility with regular databases. @@ -137,7 +137,10 @@ bool CompositeKey::challenge(const QByteArray& seed, QByteArray& result) const for (const auto& key : m_challengeResponseKeys) { // if the device isn't present or fails, return an error if (!key->challenge(seed)) { - qWarning("Failed to issue challenge"); + if (error) { + *error = key->error(); + } + qWarning() << "Failed to issue challenge: " << key->error(); return false; } cryptoHash.addData(key->rawKey()); diff --git a/src/keys/CompositeKey.h b/src/keys/CompositeKey.h index c0c77f2f42..865864d168 100644 --- a/src/keys/CompositeKey.h +++ b/src/keys/CompositeKey.h @@ -38,9 +38,9 @@ class CompositeKey : public Key bool isEmpty() const; QByteArray rawKey() const override; - QByteArray rawKey(const QByteArray* transformSeed, bool* ok = nullptr) const; - Q_REQUIRED_RESULT bool transform(const Kdf& kdf, QByteArray& result) const; - bool challenge(const QByteArray& seed, QByteArray& result) const; + + Q_REQUIRED_RESULT bool transform(const Kdf& kdf, QByteArray& result, QString* error = nullptr) const; + bool challenge(const QByteArray& seed, QByteArray& result, QString* error = nullptr) const; void addKey(const QSharedPointer& key); const QList>& keys() const; @@ -49,6 +49,8 @@ class CompositeKey : public Key const QList>& challengeResponseKeys() const; private: + QByteArray rawKey(const QByteArray* transformSeed, bool* ok = nullptr, QString* error = nullptr) const; + QList> m_keys; QList> m_challengeResponseKeys; }; diff --git a/src/keys/YkChallengeResponseKey.cpp b/src/keys/YkChallengeResponseKey.cpp index ecf11fe1cc..4bf2ab196b 100644 --- a/src/keys/YkChallengeResponseKey.cpp +++ b/src/keys/YkChallengeResponseKey.cpp @@ -23,7 +23,6 @@ #include "core/Tools.h" #include "crypto/CryptoHash.h" #include "crypto/Random.h" -#include "gui/MainWindow.h" #include #include @@ -38,15 +37,10 @@ QUuid YkChallengeResponseKey::UUID("e092495c-e77d-498b-84a1-05ae0d955508"); -YkChallengeResponseKey::YkChallengeResponseKey(int slot, bool blocking) +YkChallengeResponseKey::YkChallengeResponseKey(YubiKeySlot keySlot) : ChallengeResponseKey(UUID) - , m_slot(slot) - , m_blocking(blocking) + , m_keySlot(keySlot) { - if (getMainWindow()) { - connect(this, SIGNAL(userInteractionRequired()), getMainWindow(), SLOT(showYubiKeyPopup())); - connect(this, SIGNAL(userConfirmed()), getMainWindow(), SLOT(hideYubiKeyPopup())); - } } YkChallengeResponseKey::~YkChallengeResponseKey() @@ -63,60 +57,25 @@ QByteArray YkChallengeResponseKey::rawKey() const return QByteArray::fromRawData(m_key, static_cast(m_keySize)); } -/** - * Assumes yubikey()->init() was called - */ -bool YkChallengeResponseKey::challenge(const QByteArray& c) -{ - return challenge(c, 2); -} - -bool YkChallengeResponseKey::challenge(const QByteArray& challenge, unsigned int retries) +bool YkChallengeResponseKey::challenge(const QByteArray& challenge) { - do { - --retries; - - if (m_blocking) { - emit userInteractionRequired(); - } - - QByteArray key; - auto result = AsyncTask::runAndWaitForFuture( - [this, challenge, &key]() { return YubiKey::instance()->challenge(m_slot, true, challenge, key); }); - - if (m_blocking) { - emit userConfirmed(); - } - - if (result == YubiKey::SUCCESS) { - if (m_key) { - gcry_free(m_key); - } - m_keySize = static_cast(key.size()); - m_key = static_cast(gcry_malloc_secure(m_keySize)); - std::memcpy(m_key, key.data(), m_keySize); - sodium_memzero(key.data(), static_cast(key.capacity())); - return true; + m_error.clear(); + QByteArray key; + auto result = + AsyncTask::runAndWaitForFuture([&] { return YubiKey::instance()->challenge(m_keySlot, challenge, key); }); + + if (result == YubiKey::SUCCESS) { + if (m_key) { + gcry_free(m_key); } - } while (retries > 0); - - return false; -} - -QString YkChallengeResponseKey::getName() const -{ - unsigned int serial; - QString fmt(QObject::tr("%1[%2] Challenge Response - Slot %3 - %4")); - - YubiKey::instance()->getSerial(serial); - - return fmt.arg(YubiKey::instance()->getVendorName(), - QString::number(serial), - QString::number(m_slot), - (m_blocking) ? QObject::tr("Press") : QObject::tr("Passive")); -} + m_keySize = static_cast(key.size()); + m_key = static_cast(gcry_malloc_secure(m_keySize)); + std::memcpy(m_key, key.data(), m_keySize); + sodium_memzero(key.data(), static_cast(key.capacity())); + } else { + // Record the error message + m_error = YubiKey::instance()->errorMessage(); + } -bool YkChallengeResponseKey::isBlocking() const -{ - return m_blocking; + return result == YubiKey::SUCCESS; } diff --git a/src/keys/YkChallengeResponseKey.h b/src/keys/YkChallengeResponseKey.h index 5f7c40e721..ba213f4891 100644 --- a/src/keys/YkChallengeResponseKey.h +++ b/src/keys/YkChallengeResponseKey.h @@ -31,32 +31,16 @@ class YkChallengeResponseKey : public QObject, public ChallengeResponseKey public: static QUuid UUID; - explicit YkChallengeResponseKey(int slot = -1, bool blocking = false); + explicit YkChallengeResponseKey(YubiKeySlot keySlot = {}); ~YkChallengeResponseKey() override; QByteArray rawKey() const override; bool challenge(const QByteArray& challenge) override; - bool challenge(const QByteArray& challenge, unsigned int retries); - QString getName() const; - bool isBlocking() const; - -signals: - /** - * Emitted whenever user interaction is required to proceed with the challenge-response protocol. - * You can use this to show a helpful dialog informing the user that his assistance is required. - */ - void userInteractionRequired(); - - /** - * Emitted when the user has provided their required input. - */ - void userConfirmed(); private: char* m_key = nullptr; std::size_t m_keySize = 0; - int m_slot; - bool m_blocking; + YubiKeySlot m_keySlot; }; #endif // KEEPASSX_YK_CHALLENGERESPONSEKEY_H diff --git a/src/keys/YkChallengeResponseKeyCLI.cpp b/src/keys/YkChallengeResponseKeyCLI.cpp index c218f1f55d..050bec2da0 100644 --- a/src/keys/YkChallengeResponseKeyCLI.cpp +++ b/src/keys/YkChallengeResponseKeyCLI.cpp @@ -23,49 +23,33 @@ #include "crypto/Random.h" #include -#include QUuid YkChallengeResponseKeyCLI::UUID("e2be77c0-c810-417a-8437-32f41d00bd1d"); -YkChallengeResponseKeyCLI::YkChallengeResponseKeyCLI(int slot, - bool blocking, - QString messageInteraction, +YkChallengeResponseKeyCLI::YkChallengeResponseKeyCLI(YubiKeySlot keySlot, + QString interactionMessage, FILE* outputDescriptor) : ChallengeResponseKey(UUID) - , m_slot(slot) - , m_blocking(blocking) - , m_messageInteraction(messageInteraction) + , m_keySlot(keySlot) + , m_interactionMessage(interactionMessage) , m_out(outputDescriptor) { + connect(YubiKey::instance(), SIGNAL(userInteractionRequest()), SLOT(showInteractionMessage())); } -QByteArray YkChallengeResponseKeyCLI::rawKey() const +void YkChallengeResponseKeyCLI::showInteractionMessage() { - return m_key; + QTextStream out(m_out, QIODevice::WriteOnly); + out << m_interactionMessage << endl << endl; } -/** - * Assumes yubikey()->init() was called - */ -bool YkChallengeResponseKeyCLI::challenge(const QByteArray& c) +QByteArray YkChallengeResponseKeyCLI::rawKey() const { - return challenge(c, 2); + return m_key; } -bool YkChallengeResponseKeyCLI::challenge(const QByteArray& challenge, unsigned int retries) +bool YkChallengeResponseKeyCLI::challenge(const QByteArray& challenge) { - QTextStream out(m_out, QIODevice::WriteOnly); - do { - --retries; - - if (m_blocking) { - out << m_messageInteraction << endl; - } - YubiKey::ChallengeResult result = YubiKey::instance()->challenge(m_slot, m_blocking, challenge, m_key); - if (result == YubiKey::SUCCESS) { - return true; - } - } while (retries > 0); - - return false; + auto result = YubiKey::instance()->challenge(m_keySlot, challenge, m_key); + return result == YubiKey::SUCCESS; } diff --git a/src/keys/YkChallengeResponseKeyCLI.h b/src/keys/YkChallengeResponseKeyCLI.h index ed2d62b2ab..1d046a2a90 100644 --- a/src/keys/YkChallengeResponseKeyCLI.h +++ b/src/keys/YkChallengeResponseKeyCLI.h @@ -32,17 +32,18 @@ class YkChallengeResponseKeyCLI : public QObject, public ChallengeResponseKey public: static QUuid UUID; - explicit YkChallengeResponseKeyCLI(int slot, bool blocking, QString messageInteraction, FILE* outputDescriptor); + explicit YkChallengeResponseKeyCLI(YubiKeySlot keySlot, QString interactionMessage, FILE* outputDescriptor); QByteArray rawKey() const override; bool challenge(const QByteArray& challenge) override; - bool challenge(const QByteArray& challenge, unsigned int retries); + +private slots: + void showInteractionMessage(); private: QByteArray m_key; - int m_slot; - bool m_blocking; - QString m_messageInteraction; + YubiKeySlot m_keySlot; + QString m_interactionMessage; FILE* m_out; }; diff --git a/src/keys/drivers/YubiKey.cpp b/src/keys/drivers/YubiKey.cpp index 18cf8323a2..d410aab3b9 100644 --- a/src/keys/drivers/YubiKey.cpp +++ b/src/keys/drivers/YubiKey.cpp @@ -30,17 +30,83 @@ #include "YubiKey.h" -// Cast the void pointer from the generalized class definition -// to the proper pointer type from the now included system headers -#define m_yk (static_cast(m_yk_void)) -#define m_ykds (static_cast(m_ykds_void)) +#include + +namespace +{ + YK_KEY* openKey(int ykIndex, int okIndex, bool* onlyKey = nullptr) + { + auto* key = yk_open_key(ykIndex); +#if YKPERS_VERSION_NUMBER >= 0x011400 + // New fuction available in yubikey-personalization version >= 1.20.0 that allows + // selecting device VID/PID (yk_open_key_vid_pid) + if (!key) { + static const int device_pids[] = {0x60fc}; // OnlyKey PID + key = yk_open_key_vid_pid(0x1d50, device_pids, 1, okIndex); + if (onlyKey) { + *onlyKey = true; + } + } else if (onlyKey) { + *onlyKey = false; + } +#endif + return key; + } + + void closeKey(YK_KEY* key) + { + yk_close_key(key); + } + + unsigned int getSerial(YK_KEY* key) + { + unsigned int serial; + yk_get_serial(key, 1, 0, &serial); + return serial; + } + + YK_KEY* openKeySerial(unsigned int serial) + { + bool onlykey; + for (int i = 0, j = 0; i + j < 4;) { + auto* yk_key = openKey(i, j, &onlykey); + if (yk_key) { + onlykey ? ++j : ++i; + // If the provided serial number is 0, or the key matches the serial, return it + if (serial == 0 || getSerial(yk_key) == serial) { + return yk_key; + } + closeKey(yk_key); + } else { + // No more connected keys + break; + } + } + return nullptr; + } +} // namespace YubiKey::YubiKey() - : m_yk_void(nullptr) - , m_ykds_void(nullptr) - , m_onlyKey(false) - , m_mutex(QMutex::Recursive) + : m_mutex(QMutex::Recursive) +{ + m_interactionTimer.setSingleShot(true); + m_interactionTimer.setInterval(300); + + if (!yk_init()) { + qDebug("YubiKey: Failed to initialize USB interface."); + } else { + m_initialized = true; + // clang-format off + connect(&m_interactionTimer, SIGNAL(timeout()), this, SIGNAL(userInteractionRequest())); + connect(this, &YubiKey::challengeStarted, this, [this] { m_interactionTimer.start(); }, Qt::QueuedConnection); + connect(this, &YubiKey::challengeCompleted, this, [this] { m_interactionTimer.stop(); }, Qt::QueuedConnection); + // clang-format on + } +} + +YubiKey::~YubiKey() { + yk_release(); } YubiKey* YubiKey::m_instance(Q_NULLPTR); @@ -54,161 +120,162 @@ YubiKey* YubiKey::instance() return m_instance; } -bool YubiKey::init() +bool YubiKey::isInitialized() { - m_mutex.lock(); - - // previously initialized - if (m_yk != nullptr && m_ykds != nullptr) { - - if (yk_get_status(m_yk, m_ykds)) { - // Still connected - m_mutex.unlock(); - return true; - } else { - // Initialized but not connected anymore, re-init - deinit(); - } - } + return m_initialized; +} - if (!yk_init()) { - m_mutex.unlock(); - return false; +void YubiKey::findValidKeys() +{ + m_error.clear(); + if (!isInitialized()) { + return; } - // TODO: handle multiple attached hardware devices - m_onlyKey = false; - m_yk_void = static_cast(yk_open_first_key()); -#if YKPERS_VERSION_NUMBER >= 0x011400 - // New fuction available in yubikey-personalization version >= 1.20.0 that allows - // selecting device VID/PID (yk_open_key_vid_pid) - if (m_yk == nullptr) { - static const int device_pids[] = {0x60fc}; // OnlyKey PID - m_yk_void = static_cast(yk_open_key_vid_pid(0x1d50, device_pids, 1, 0)); - m_onlyKey = true; - } -#endif - if (m_yk == nullptr) { - yk_release(); - m_mutex.unlock(); - return false; - } + QtConcurrent::run([this] { + if (!m_mutex.tryLock(1000)) { + emit detectComplete(false); + return; + } - m_ykds_void = static_cast(ykds_alloc()); - if (m_ykds == nullptr) { - yk_close_key(m_yk); - m_yk_void = nullptr; - yk_release(); - m_mutex.unlock(); - return false; - } + // Remove all known keys + m_foundKeys.clear(); + + // Try to detect up to 4 connected hardware keys + for (int i = 0, j = 0; i + j < 4;) { + bool onlyKey = false; + auto* yk_key = openKey(i, j, &onlyKey); + Tools::wait(50); + if (yk_key) { + onlyKey ? ++j : ++i; + auto vender = onlyKey ? QStringLiteral("OnlyKey") : QStringLiteral("YubiKey"); + auto serial = getSerial(yk_key); + if (serial == 0) { + closeKey(yk_key); + continue; + } + + auto st = ykds_alloc(); + yk_get_status(yk_key, st); + int vid, pid; + yk_get_key_vid_pid(yk_key, &vid, &pid); + + auto chall = randomGen()->randomArray(1); + QByteArray resp; + QList> ykSlots; + for (int slot = 1; slot <= 2; ++slot) { + auto config = (i == 1 ? CONFIG1_VALID : CONFIG2_VALID); + if (ykds_touch_level(st) & config) { + // Don't actually challenge a YubiKey Neo or below, they always require button press + // if it is enabled for the slot resulting in failed detection + if (pid <= 0x0116) { + auto display = tr("%1 [%2] Configured Slot - %3") + .arg(vender, QString::number(serial), QString::number(slot)); + ykSlots.append({slot, display}); + } else { + auto ret = performChallenge(yk_key, slot, false, chall, resp); + if (ret == SUCCESS || ret == WOULDBLOCK) { + auto blocking = ret == WOULDBLOCK ? tr("Press") : tr("Passive"); + auto display = + tr("%1 [%2] Challenge Response - Slot %3 - %4") + .arg(vender, QString::number(serial), QString::number(slot), blocking); + ykSlots.append({slot, display}); + } + } + } + } + + if (!ykSlots.isEmpty()) { + m_foundKeys.insert(serial, ykSlots); + } + + ykds_free(st); + closeKey(yk_key); + } else { + // No more keys are connected + break; + } + Tools::wait(50); + } - m_mutex.unlock(); - return true; + m_mutex.unlock(); + emit detectComplete(!m_foundKeys.isEmpty()); + }); } -bool YubiKey::deinit() +QList YubiKey::foundKeys() { - m_mutex.lock(); - - if (m_yk) { - yk_close_key(m_yk); - m_yk_void = nullptr; - } - - if (m_ykds) { - ykds_free(m_ykds); - m_ykds_void = nullptr; + QList keys; + for (auto serial : m_foundKeys.uniqueKeys()) { + for (auto key : m_foundKeys.value(serial)) { + keys.append({serial, key.first}); + } } - - yk_release(); - - m_mutex.unlock(); - - return true; + return keys; } -void YubiKey::detect() +QString YubiKey::getDisplayName(YubiKeySlot slot) { - bool found = false; - - // Check slot 1 and 2 for Challenge-Response HMAC capability - for (int i = 1; i <= 2; ++i) { - QString errorMsg; - bool isBlocking = checkSlotIsBlocking(i, errorMsg); - if (errorMsg.isEmpty()) { - found = true; - emit detected(i, isBlocking); + for (auto key : m_foundKeys.value(slot.first)) { + if (slot.second == key.first) { + return key.second; } - - // Wait between slots to let the yubikey settle. - Tools::sleep(150); } + return tr("%1 Invalid slot specified - %2").arg(QString::number(slot.first), QString::number(slot.second)); +} - if (!found) { - emit notFound(); - } else { - emit detectComplete(); - } +QString YubiKey::errorMessage() +{ + return m_error; } -bool YubiKey::checkSlotIsBlocking(int slot, QString& errorMessage) +/** + * Issue a challenge to the specified slot + * This operation could block if the YubiKey requires a touch to trigger. + * + * @param serial Specific YubiKey to use, set to 0 for first available key + * @param slot YubiKey configuration slot + * @param challenge challenge input to YubiKey + * @param response response output from YubiKey + * @return challenge result + */ +YubiKey::ChallengeResult YubiKey::challenge(YubiKeySlot slot, const QByteArray& challenge, QByteArray& response) { - if (!init()) { - errorMessage = QString("Could not initialize YubiKey."); - return false; + m_error.clear(); + if (!m_initialized) { + m_error = tr("The YubiKey interface has not been initialized."); + return ERROR; } - YubiKey::ChallengeResult result; - QByteArray rand = randomGen()->randomArray(1); - QByteArray resp; - - result = challenge(slot, false, rand, resp); - if (result == ALREADY_RUNNING) { - // Try this slot again after waiting - Tools::sleep(300); - result = challenge(slot, false, rand, resp); + // Try to grab a lock for 1 second, fail out if not possible + if (!m_mutex.tryLock(1000)) { + m_error = tr("Hardware key is currently in use."); + return ERROR; } - if (result == SUCCESS || result == WOULDBLOCK) { - return result == WOULDBLOCK; - } else if (result == ALREADY_RUNNING) { - errorMessage = QString("YubiKey busy"); - return false; - } else if (result == ERROR) { - errorMessage = QString("YubiKey error"); - return false; + auto* yk_key = openKeySerial(slot.first); + if (!yk_key) { + // Key with specified serial number is not connected + m_error = + tr("Could not find hardware key with serial number %1. Please plug it in to continue.").arg(slot.first); + m_mutex.unlock(); + return ERROR; } - errorMessage = QString("Error while polling YubiKey"); - return false; -} + emit challengeStarted(); + auto ret = performChallenge(yk_key, slot.second, true, challenge, response); -bool YubiKey::getSerial(unsigned int& serial) -{ - m_mutex.lock(); - int result = yk_get_serial(m_yk, 1, 0, &serial); + closeKey(yk_key); + emit challengeCompleted(); m_mutex.unlock(); - if (!result) { - return false; - } - - return true; + return ret; } -QString YubiKey::getVendorName() +YubiKey::ChallengeResult +YubiKey::performChallenge(void* key, int slot, bool mayBlock, const QByteArray& challenge, QByteArray& response) { - return m_onlyKey ? "OnlyKey" : "YubiKey"; -} - -YubiKey::ChallengeResult YubiKey::challenge(int slot, bool mayBlock, const QByteArray& challenge, QByteArray& response) -{ - // ensure that YubiKey::init() succeeded - if (!init()) { - return ERROR; - } - + m_error.clear(); int yk_cmd = (slot == 1) ? SLOT_CHAL_HMAC1 : SLOT_CHAL_HMAC2; QByteArray paddedChallenge = challenge; @@ -232,39 +299,28 @@ YubiKey::ChallengeResult YubiKey::challenge(int slot, bool mayBlock, const QByte c = reinterpret_cast(paddedChallenge.constData()); r = reinterpret_cast(response.data()); - // Try to grab a lock for 1 second, fail out if not possible - if (!m_mutex.tryLock(1000)) { - return ALREADY_RUNNING; - } + int ret = yk_challenge_response( + static_cast(key), yk_cmd, mayBlock, paddedChallenge.size(), c, response.size(), r); - int ret = yk_challenge_response(m_yk, yk_cmd, mayBlock, paddedChallenge.size(), c, response.size(), r); - m_mutex.unlock(); + // actual HMAC-SHA1 response is only 20 bytes + response.resize(20); if (!ret) { if (yk_errno == YK_EWOULDBLOCK) { return WOULDBLOCK; - } else if (yk_errno == YK_ETIMEOUT) { - return ERROR; } else if (yk_errno) { - - /* Something went wrong, close the key, so that the next call to - * can try to re-open. - * - * Likely caused by the YubiKey being unplugged. - */ - - if (yk_errno == YK_EUSBERR) { - qWarning("USB error: %s", yk_usb_strerror()); + if (yk_errno == YK_ETIMEOUT) { + m_error = tr("Hardware key timed out waiting for user interaction."); + } else if (yk_errno == YK_EUSBERR) { + m_error = tr("A USB error ocurred when accessing the hardware key: %1").arg(yk_usb_strerror()); } else { - qWarning("YubiKey core error: %s", yk_strerror(yk_errno)); + m_error = tr("Failed to complete a challenge-response, the specific error was: %1") + .arg(yk_strerror(yk_errno)); } return ERROR; } } - // actual HMAC-SHA1 response is only 20 bytes - response.resize(20); - return SUCCESS; } diff --git a/src/keys/drivers/YubiKey.h b/src/keys/drivers/YubiKey.h index 7bb527a285..f5253bdbb0 100644 --- a/src/keys/drivers/YubiKey.h +++ b/src/keys/drivers/YubiKey.h @@ -19,11 +19,16 @@ #ifndef KEEPASSX_YUBIKEY_H #define KEEPASSX_YUBIKEY_H +#include #include #include +#include + +typedef QPair YubiKeySlot; +Q_DECLARE_METATYPE(YubiKeySlot); /** - * Singleton class to manage the interface to the hardware + * Singleton class to manage the interface to hardware key(s) */ class YubiKey : public QObject { @@ -32,100 +37,63 @@ class YubiKey : public QObject public: enum ChallengeResult { - ERROR = -1, - SUCCESS = 0, - WOULDBLOCK, - ALREADY_RUNNING + ERROR, + SUCCESS, + WOULDBLOCK }; - /** - * @brief YubiKey::instance - get instance of singleton - * @return instance - */ static YubiKey* instance(); + bool isInitialized(); - /** - * @brief YubiKey::init - initialize yubikey library and hardware - * @return true on success - */ - bool init(); + void findValidKeys(); - /** - * @brief YubiKey::deinit - cleanup after init - * @return true on success - */ - bool deinit(); + QList foundKeys(); + QString getDisplayName(YubiKeySlot slot); - /** - * @brief YubiKey::challenge - issue a challenge - * - * This operation could block if the YubiKey requires a touch to trigger. - * - * TODO: Signal to the UI that the system is waiting for challenge response - * touch. - * - * @param slot YubiKey configuration slot - * @param mayBlock operation is allowed to block - * @param challenge challenge input to YubiKey - * @param response response output from YubiKey - * @return challenge result - */ - ChallengeResult challenge(int slot, bool mayBlock, const QByteArray& challenge, QByteArray& response); + ChallengeResult challenge(YubiKeySlot slot, const QByteArray& challenge, QByteArray& response); - /** - * @brief YubiKey::getSerial - serial number of YubiKey - * @param serial serial number - * @return true on success - */ - bool getSerial(unsigned int& serial); - - /** - * @brief YubiKey::getVendorName - vendor name of token - * @return vendor name - */ - QString getVendorName(); + QString errorMessage(); +signals: /** - * @brief YubiKey::detect - probe for attached YubiKeys + * Emitted when a detection process completes. Use the `detectedSlots` + * accessor function to get information on the available slots. + * + * @param found - true if a key was found */ - void detect(); + void detectComplete(bool found); /** - * @param slot the yubikey slot. - * @param errorMessage populated if an error occured. - * - * @return whether the key is blocking or not. - */ - bool checkSlotIsBlocking(int slot, QString& errorMessage); -signals: - /** Emitted in response to detect() when a device is found - * - * @slot is the slot number detected - * @blocking signifies if the YK is setup in passive mode or if requires - * the user to touch it for a response + * Emitted when user needs to interact with the hardware key to continue */ - void detected(int slot, bool blocking); + void userInteractionRequest(); /** - * Emitted when detection is complete + * Emitted before/after a challenge-response is performed */ - void detectComplete(); + void challengeStarted(); + void challengeCompleted(); /** - * Emitted when no Yubikey could be found. + * Emitted when an error occurred during challenge/response */ - void notFound(); + void challengeError(QString error); private: explicit YubiKey(); + ~YubiKey(); + static YubiKey* m_instance; - // Create void ptr here to avoid ifdef header include mess - void* m_yk_void; - void* m_ykds_void; - bool m_onlyKey; + ChallengeResult + performChallenge(void* key, int slot, bool mayBlock, const QByteArray& challenge, QByteArray& response); + + QHash>> m_foundKeys; QMutex m_mutex; + QTimer m_interactionTimer; + bool m_initialized = false; + QString m_error; Q_DISABLE_COPY(YubiKey) }; diff --git a/src/keys/drivers/YubiKeyStub.cpp b/src/keys/drivers/YubiKeyStub.cpp index 1c2fcb8b6e..4f19488f17 100644 --- a/src/keys/drivers/YubiKeyStub.cpp +++ b/src/keys/drivers/YubiKeyStub.cpp @@ -16,20 +16,13 @@ * along with this program. If not, see . */ -#include - -#include "core/Global.h" -#include "crypto/Random.h" - #include "YubiKey.h" YubiKey::YubiKey() - : m_yk_void(NULL) - , m_ykds_void(NULL) { } -YubiKey* YubiKey::m_instance(Q_NULLPTR); +YubiKey* YubiKey::m_instance(nullptr); YubiKey* YubiKey::instance() { @@ -40,45 +33,26 @@ YubiKey* YubiKey::instance() return m_instance; } -bool YubiKey::init() -{ - return false; -} - -bool YubiKey::deinit() -{ - return false; -} - -void YubiKey::detect() +void YubiKey::findValidKeys() { } -bool YubiKey::getSerial(unsigned int& serial) +QList YubiKey::foundKeys() { - Q_UNUSED(serial); - - return false; + return {}; } -QString YubiKey::getVendorName() +QString YubiKey::getDisplayName(YubiKeySlot slot) { - return "YubiKeyStub"; + Q_UNUSED(slot); + return {}; } -YubiKey::ChallengeResult YubiKey::challenge(int slot, bool mayBlock, const QByteArray& chal, QByteArray& resp) +YubiKey::ChallengeResult YubiKey::challenge(YubiKeySlot slot, const QByteArray& chal, QByteArray& resp) { Q_UNUSED(slot); - Q_UNUSED(mayBlock); Q_UNUSED(chal); Q_UNUSED(resp); return ERROR; } - -bool YubiKey::checkSlotIsBlocking(int slot, QString& errorMessage) -{ - Q_UNUSED(slot); - Q_UNUSED(errorMessage); - return false; -} diff --git a/tests/TestCli.cpp b/tests/TestCli.cpp index 4ebc2f4e6a..e35914199d 100644 --- a/tests/TestCli.cpp +++ b/tests/TestCli.cpp @@ -59,6 +59,7 @@ #include #include #include +#include #include #include @@ -2096,32 +2097,56 @@ void TestCli::testInvalidDbFiles() /** * Secret key for the YubiKey slot used by the unit test is * 1c e3 0f d7 8d 20 dc fa 40 b5 0c 18 77 9a fb 0f 02 28 8d b7 - * This secret should be configured at slot 2, and the slot - * should be configured as passive. + * This secret can be on either slot but must be passive. */ void TestCli::testYubiKeyOption() { - if (!YubiKey::instance()->init()) { - QSKIP("Unable to connect to YubiKey"); + if (!YubiKey::instance()->isInitialized()) { + QSKIP("Unable to initialize YubiKey interface."); } - QString errorMessage; - bool isBlocking = YubiKey::instance()->checkSlotIsBlocking(2, errorMessage); - if (isBlocking && errorMessage.isEmpty()) { - QSKIP("Skipping YubiKey in press mode."); + YubiKey::instance()->findValidKeys(); + + // Wait for the hardware to respond + QSignalSpy detected(YubiKey::instance(), SIGNAL(detectComplete(bool))); + QTRY_VERIFY_WITH_TIMEOUT(detected.count() > 0, 2000); + + auto keys = YubiKey::instance()->foundKeys(); + if (keys.isEmpty()) { + QSKIP("No YubiKey devices were detected."); } + bool passive = false; QByteArray challenge("CLITest"); QByteArray response; - YubiKey::instance()->challenge(2, false, challenge, response); QByteArray expected("\xA2\x3B\x94\x00\xBE\x47\x9A\x30\xA9\xEB\x50\x9B\x85\x56\x5B\x6B\x30\x25\xB4\x8E", 20); - QVERIFY2(response == expected, "YubiKey Slot 2 is not configured with correct secret key."); + + QPair pKey(0, 0); + for (auto key : keys) { + if (YubiKey::instance()->getDisplayName(key).contains("Passive")) { + passive = true; + YubiKey::instance()->challenge(key, challenge, response); + if (response == expected) { + pKey = key; + break; + } + Tools::wait(100); + } + } + + if (!passive) { + /* Testing active mode in unit tests is unreasonable */ + QSKIP("No YubiKey contains a slot in passive mode."); + } + + QVERIFY2(pKey.first != 0, "YubiKey is not configured with the correct secret key."); List listCmd; Add addCmd; + auto keyParam = QString("%1:%2").arg(pKey.second).arg(pKey.first); Utils::Test::setNextPassword("a"); - listCmd.execute({"ls", "-y", "2", m_yubiKeyProtectedDbFile->fileName()}); + listCmd.execute({"ls", "-y", keyParam, m_yubiKeyProtectedDbFile->fileName()}); m_stdoutFile->reset(); m_stderrFile->reset(); m_stdoutFile->readLine(); // skip password prompt @@ -2143,7 +2168,7 @@ void TestCli::testYubiKeyOption() QCOMPARE(m_stderrFile->readLine(), QByteArray("If this reoccurs, then your database file may be corrupt. (HMAC mismatch)\n")); - // Should raise an error if yubikey slot is not a string + // Should raise an error if yubikey slot is not a number pos = m_stdoutFile->pos(); posErr = m_stderrFile->pos(); Utils::Test::setNextPassword("a"); @@ -2164,6 +2189,17 @@ void TestCli::testYubiKeyOption() m_stderrFile->seek(posErr); QCOMPARE(m_stdoutFile->readAll(), QByteArray("")); QCOMPARE(m_stderrFile->readAll().split(':').at(0), QByteArray("Invalid YubiKey slot 3\n")); + + // Should raise an error if yubikey serial is not a number + pos = m_stdoutFile->pos(); + posErr = m_stderrFile->pos(); + Utils::Test::setNextPassword("a"); + listCmd.execute({"ls", "-y", "1:badserial", m_yubiKeyProtectedDbFile->fileName()}); + m_stdoutFile->seek(pos); + m_stdoutFile->readLine(); // skip password prompt + m_stderrFile->seek(posErr); + QCOMPARE(m_stdoutFile->readAll(), QByteArray("")); + QCOMPARE(m_stderrFile->readAll().split(':').at(0), QByteArray("Invalid YubiKey serial badserial\n")); } namespace diff --git a/tests/TestYkChallengeResponseKey.cpp b/tests/TestYkChallengeResponseKey.cpp index a4dd762701..30f304960e 100644 --- a/tests/TestYkChallengeResponseKey.cpp +++ b/tests/TestYkChallengeResponseKey.cpp @@ -19,82 +19,69 @@ #include "TestYkChallengeResponseKey.h" #include "TestGlobal.h" + #include "crypto/Crypto.h" +#include "keys/YkChallengeResponseKey.h" -#include +#include +#include -QTEST_GUILESS_MAIN(TestYubiKeyChalResp) +QTEST_GUILESS_MAIN(TestYubiKeyChallengeResponse) -void TestYubiKeyChalResp::initTestCase() +void TestYubiKeyChallengeResponse::initTestCase() { // crypto subsystem needs to be initialized for YubiKey testing QVERIFY(Crypto::init()); -} -void TestYubiKeyChalResp::init() -{ - if (!YubiKey::instance()->init()) { - QSKIP("Unable to connect to YubiKey"); + if (!YubiKey::instance()->isInitialized()) { + QSKIP("Unable to initialize YubiKey interface."); } } -void TestYubiKeyChalResp::detectDevices() -{ - connect(YubiKey::instance(), SIGNAL(detected(int, bool)), SLOT(ykDetected(int, bool)), Qt::QueuedConnection); - QtConcurrent::run(YubiKey::instance(), &YubiKey::detect); - - // need to wait for the hardware (that's hopefully plugged in)... - QTest::qWait(2000); - QVERIFY2(m_detected > 0, "Is a YubiKey attached?"); -} - -void TestYubiKeyChalResp::getSerial() -{ - unsigned int serial; - QVERIFY(YubiKey::instance()->getSerial(serial)); -} - -void TestYubiKeyChalResp::keyGetName() -{ - QVERIFY(m_key); - QVERIFY(m_key->getName().length() > 0); -} - -void TestYubiKeyChalResp::keyIssueChallenge() +void TestYubiKeyChallengeResponse::testDetectDevices() { - QVERIFY(m_key); - if (m_key->isBlocking()) { - /* Testing active mode in unit tests is unreasonable */ - QSKIP("YubiKey not in passive mode", SkipSingle); + YubiKey::instance()->findValidKeys(); + + // Wait for the hardware to respond + QSignalSpy detected(YubiKey::instance(), SIGNAL(detectComplete(bool))); + QTRY_VERIFY_WITH_TIMEOUT(detected.count() > 0, 2000); + + // Look at the information retrieved from the key(s) + for (auto key : YubiKey::instance()->foundKeys()) { + auto displayName = YubiKey::instance()->getDisplayName(key); + QVERIFY(displayName.contains("Challenge Response - Slot")); + QVERIFY(displayName.contains(QString::number(key.first))); + QVERIFY(displayName.contains(QString::number(key.second))); } - - QByteArray ba("UnitTest"); - QVERIFY(m_key->challenge(ba)); - - /* TODO Determine if it's reasonable to provide a fixed secret key for - * verification testing. Obviously simple technically, but annoying - * if devs need to re-program their yubikeys or have a spare test key - * for unit tests to pass. - * - * Might be worth it for integrity verification though. - */ } -void TestYubiKeyChalResp::ykDetected(int slot, bool blocking) +/** + * Secret key for the YubiKey slot used by the unit test is + * 1c e3 0f d7 8d 20 dc fa 40 b5 0c 18 77 9a fb 0f 02 28 8d b7 + * This secret can be on either slot but must be passive. + */ +void TestYubiKeyChallengeResponse::testKeyChallenge() { - Q_UNUSED(blocking); + auto keys = YubiKey::instance()->foundKeys(); + if (keys.isEmpty()) { + QSKIP("No YubiKey devices were detected."); + } - if (slot > 0) { - m_detected++; + QPair passiveKey; + for (auto key : keys) { + if (YubiKey::instance()->getDisplayName(key).contains("Passive")) { + passiveKey = key; + } } - /* Key used for later testing */ - if (!m_key) { - m_key.reset(new YkChallengeResponseKey(slot, blocking)); + if (passiveKey.first == 0) { + /* Testing active mode in unit tests is unreasonable */ + QSKIP("No YubiKey contains a slot in passive mode."); } -} -void TestYubiKeyChalResp::deinit() -{ - QVERIFY(YubiKey::instance()->deinit()); + QScopedPointer key(new YkChallengeResponseKey(passiveKey)); + + QByteArray ba("UnitTest"); + QVERIFY(key->challenge(ba)); + QCOMPARE(key->rawKey().size(), 20); } diff --git a/tests/TestYkChallengeResponseKey.h b/tests/TestYkChallengeResponseKey.h index 81253cc908..63fcaf6ee8 100644 --- a/tests/TestYkChallengeResponseKey.h +++ b/tests/TestYkChallengeResponseKey.h @@ -20,36 +20,16 @@ #define KEEPASSX_TESTYUBIKEYCHALRESP_H #include -#include -#include "keys/YkChallengeResponseKey.h" - -class TestYubiKeyChalResp : public QObject +class TestYubiKeyChallengeResponse : public QObject { Q_OBJECT private slots: void initTestCase(); - void init(); - - /* Order is important! - * Need to init and detectDevices() before proceeding - */ - void detectDevices(); - - void getSerial(); - void keyGetName(); - void keyIssueChallenge(); - - void deinit(); - - /* Callback for detectDevices() */ - void ykDetected(int slot, bool blocking); - -private: - int m_detected = 0; - QScopedPointer m_key; + void testDetectDevices(); + void testKeyChallenge(); }; #endif // KEEPASSX_TESTYUBIKEYCHALRESP_H