From 093fe5c7ef9a513f18b7f258ffcf6711e7346ba8 Mon Sep 17 00:00:00 2001 From: Janek Bevendorff Date: Thu, 23 Feb 2017 23:52:36 +0100 Subject: [PATCH] Use QSharedPointer instead of cloning YkChallengeResponseKey and make it a QObject to allow emitting signals --- src/core/Database.cpp | 6 ++-- src/format/KeePass2Reader.cpp | 2 +- src/gui/ChangeMasterKeyWidget.cpp | 13 +++---- src/gui/DatabaseOpenWidget.cpp | 16 +++++---- src/keys/ChallengeResponseKey.h | 1 - src/keys/CompositeKey.cpp | 21 ++++++----- src/keys/CompositeKey.h | 5 +-- src/keys/YkChallengeResponseKey.cpp | 56 ++++++++++++++--------------- src/keys/YkChallengeResponseKey.h | 24 ++++++++++--- 9 files changed, 78 insertions(+), 66 deletions(-) diff --git a/src/core/Database.cpp b/src/core/Database.cpp index 22fc0723..a441910d 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -257,18 +257,16 @@ bool Database::verifyKey(const CompositeKey& key) const { Q_ASSERT(hasKey()); - /* If the database has challenge response keys, then the the verification - * key better as well */ if (!m_data.challengeResponseKey.isEmpty()) { QByteArray result; if (!key.challenge(m_data.masterSeed, result)) { - /* Challenge failed, (YubiKey?) removed? */ + // challenge failed, (YubiKey?) removed? return false; } if (m_data.challengeResponseKey != result) { - /* Wrong response from challenged device(s) */ + // wrong response from challenged device(s) return false; } } diff --git a/src/format/KeePass2Reader.cpp b/src/format/KeePass2Reader.cpp index 33bea620..ffe4e94f 100644 --- a/src/format/KeePass2Reader.cpp +++ b/src/format/KeePass2Reader.cpp @@ -115,7 +115,7 @@ Database* KeePass2Reader::readDatabase(QIODevice* device, const CompositeKey& ke if (m_db->challengeMasterSeed(m_masterSeed) == false) { raiseError(tr("Unable to issue challenge-response.")); - return Q_NULLPTR; + return nullptr; } CryptoHash hash(CryptoHash::Sha256); diff --git a/src/gui/ChangeMasterKeyWidget.cpp b/src/gui/ChangeMasterKeyWidget.cpp index 8c17ff57..453498a1 100644 --- a/src/gui/ChangeMasterKeyWidget.cpp +++ b/src/gui/ChangeMasterKeyWidget.cpp @@ -15,8 +15,6 @@ * along with this program. If not, see . */ -#include - #include "ChangeMasterKeyWidget.h" #include "ui_ChangeMasterKeyWidget.h" @@ -30,6 +28,9 @@ #include "config-keepassx.h" +#include +#include + ChangeMasterKeyWidget::ChangeMasterKeyWidget(QWidget* parent) : DialogyWidget(parent) , m_ui(new Ui::ChangeMasterKeyWidget()) @@ -172,9 +173,9 @@ void ChangeMasterKeyWidget::generateKey() MessageWidget::Error); return; } - - YkChallengeResponseKey key(i); - + bool blocking = i & true; + int slot = i >> 1; + auto key = QSharedPointer(new YkChallengeResponseKey(slot, blocking)); m_key.addChallengeResponseKey(key); } #endif @@ -210,7 +211,7 @@ void ChangeMasterKeyWidget::pollYubikey() void ChangeMasterKeyWidget::yubikeyDetected(int slot, bool blocking) { YkChallengeResponseKey yk(slot, blocking); - m_ui->comboChallengeResponse->addItem(yk.getName(), QVariant(slot)); + m_ui->comboChallengeResponse->addItem(yk.getName(), QVariant((slot << 1) | blocking)); m_ui->comboChallengeResponse->setEnabled(m_ui->challengeResponseGroup->isChecked()); m_ui->buttonRedetectYubikey->setEnabled(m_ui->challengeResponseGroup->isChecked()); m_ui->yubikeyProgress->setVisible(false); diff --git a/src/gui/DatabaseOpenWidget.cpp b/src/gui/DatabaseOpenWidget.cpp index 1543dc43..2eebcae3 100644 --- a/src/gui/DatabaseOpenWidget.cpp +++ b/src/gui/DatabaseOpenWidget.cpp @@ -15,8 +15,6 @@ * along with this program. If not, see . */ -#include - #include "DatabaseOpenWidget.h" #include "ui_DatabaseOpenWidget.h" @@ -34,6 +32,9 @@ #include "config-keepassx.h" +#include +#include + DatabaseOpenWidget::DatabaseOpenWidget(QWidget* parent) : DialogyWidget(parent), @@ -156,7 +157,7 @@ void DatabaseOpenWidget::openDatabase() if (m_ui->messageWidget->isVisible()) { m_ui->messageWidget->animatedHide(); } - Q_EMIT editFinished(true); + emit editFinished(true); } else { m_ui->messageWidget->showMessage(tr("Unable to open the database.") @@ -209,8 +210,10 @@ CompositeKey DatabaseOpenWidget::databaseKey() if (m_ui->checkChallengeResponse->isChecked()) { int i = m_ui->comboChallengeResponse->currentIndex(); i = m_ui->comboChallengeResponse->itemData(i).toInt(); - YkChallengeResponseKey key(i); + bool blocking = i & true; + int slot = i >> 1; + auto key = QSharedPointer(new YkChallengeResponseKey(slot, blocking)); masterKey.addChallengeResponseKey(key); } #endif @@ -250,20 +253,21 @@ void DatabaseOpenWidget::browseKeyFile() void DatabaseOpenWidget::pollYubikey() { - // YubiKey init is slow, detect asynchronously to not block the UI m_ui->buttonRedetectYubikey->setEnabled(false); m_ui->checkChallengeResponse->setEnabled(false); m_ui->checkChallengeResponse->setChecked(false); m_ui->comboChallengeResponse->setEnabled(false); m_ui->comboChallengeResponse->clear(); m_ui->yubikeyProgress->setVisible(true); + + // YubiKey init is slow, detect asynchronously to not block the UI QtConcurrent::run(YubiKey::instance(), &YubiKey::detect); } void DatabaseOpenWidget::yubikeyDetected(int slot, bool blocking) { YkChallengeResponseKey yk(slot, blocking); - m_ui->comboChallengeResponse->addItem(yk.getName(), QVariant(slot)); + m_ui->comboChallengeResponse->addItem(yk.getName(), QVariant((slot << 1) | blocking)); m_ui->comboChallengeResponse->setEnabled(true); m_ui->checkChallengeResponse->setEnabled(true); m_ui->buttonRedetectYubikey->setEnabled(true); diff --git a/src/keys/ChallengeResponseKey.h b/src/keys/ChallengeResponseKey.h index e03a2f9f..ac8c8165 100644 --- a/src/keys/ChallengeResponseKey.h +++ b/src/keys/ChallengeResponseKey.h @@ -25,7 +25,6 @@ class ChallengeResponseKey public: virtual ~ChallengeResponseKey() {} virtual QByteArray rawKey() const = 0; - virtual ChallengeResponseKey* clone() const = 0; virtual bool challenge(const QByteArray& challenge) = 0; }; diff --git a/src/keys/CompositeKey.cpp b/src/keys/CompositeKey.cpp index 4e79cd05..6114fd36 100644 --- a/src/keys/CompositeKey.cpp +++ b/src/keys/CompositeKey.cpp @@ -46,7 +46,6 @@ CompositeKey::~CompositeKey() void CompositeKey::clear() { qDeleteAll(m_keys); - qDeleteAll(m_challengeResponseKeys); m_keys.clear(); m_challengeResponseKeys.clear(); } @@ -73,8 +72,8 @@ CompositeKey& CompositeKey::operator=(const CompositeKey& key) for (const Key* subKey : asConst(key.m_keys)) { addKey(*subKey); } - for (const ChallengeResponseKey* subKey : asConst(key.m_challengeResponseKeys)) { - addChallengeResponseKey(*subKey); + for (const auto subKey : asConst(key.m_challengeResponseKeys)) { + addChallengeResponseKey(subKey); } return *this; @@ -176,9 +175,8 @@ QByteArray CompositeKey::transformKeyRaw(const QByteArray& key, const QByteArray bool CompositeKey::challenge(const QByteArray& seed, QByteArray& result) const { - /* If no challenge response was requested, return nothing to - * maintain backwards compatability with regular databases. - */ + // if no challenge response was requested, return nothing to + // maintain backwards compatibility with regular databases. if (m_challengeResponseKeys.length() == 0) { result.clear(); return true; @@ -186,9 +184,9 @@ bool CompositeKey::challenge(const QByteArray& seed, QByteArray& result) const CryptoHash cryptoHash(CryptoHash::Sha256); - for (ChallengeResponseKey* key : m_challengeResponseKeys) { - /* If the device isn't present or fails, return an error */ - if (key->challenge(seed) == false) { + for (const auto key : m_challengeResponseKeys) { + // if the device isn't present or fails, return an error + if (!key->challenge(seed)) { return false; } cryptoHash.addData(key->rawKey()); @@ -203,11 +201,12 @@ void CompositeKey::addKey(const Key& key) m_keys.append(key.clone()); } -void CompositeKey::addChallengeResponseKey(const ChallengeResponseKey& key) +void CompositeKey::addChallengeResponseKey(QSharedPointer key) { - m_challengeResponseKeys.append(key.clone()); + m_challengeResponseKeys.append(key); } + int CompositeKey::transformKeyBenchmark(int msec) { TransformKeyBenchmarkThread thread1(msec); diff --git a/src/keys/CompositeKey.h b/src/keys/CompositeKey.h index 531c2d9b..50b2f699 100644 --- a/src/keys/CompositeKey.h +++ b/src/keys/CompositeKey.h @@ -20,6 +20,7 @@ #include #include +#include #include "keys/Key.h" #include "keys/ChallengeResponseKey.h" @@ -41,7 +42,7 @@ public: bool challenge(const QByteArray& seed, QByteArray &result) const; void addKey(const Key& key); - void addChallengeResponseKey(const ChallengeResponseKey& key); + void addChallengeResponseKey(QSharedPointer key); static int transformKeyBenchmark(int msec); static CompositeKey readFromLine(QString line); @@ -51,7 +52,7 @@ private: quint64 rounds, bool* ok, QString* errorString); QList m_keys; - QList m_challengeResponseKeys; + QList> m_challengeResponseKeys; }; #endif // KEEPASSX_COMPOSITEKEY_H diff --git a/src/keys/YkChallengeResponseKey.cpp b/src/keys/YkChallengeResponseKey.cpp index 51520e3d..a6e5cdbc 100644 --- a/src/keys/YkChallengeResponseKey.cpp +++ b/src/keys/YkChallengeResponseKey.cpp @@ -15,7 +15,6 @@ * along with this program. If not, see . */ - #include #include @@ -26,11 +25,7 @@ #include "keys/YkChallengeResponseKey.h" #include "keys/drivers/YubiKey.h" -#include -#include - -YkChallengeResponseKey::YkChallengeResponseKey(int slot, - bool blocking) +YkChallengeResponseKey::YkChallengeResponseKey(int slot, bool blocking) : m_slot(slot), m_blocking(blocking) { @@ -41,40 +36,41 @@ QByteArray YkChallengeResponseKey::rawKey() const return m_key; } -YkChallengeResponseKey* YkChallengeResponseKey::clone() const -{ - return new YkChallengeResponseKey(*this); -} - - -/** Assumes yubikey()->init() was called */ +/** + * Assumes yubikey()->init() was called + */ bool YkChallengeResponseKey::challenge(const QByteArray& chal) { return challenge(chal, 1); } - -bool YkChallengeResponseKey::challenge(const QByteArray& chal, int retries) +#include +bool YkChallengeResponseKey::challenge(const QByteArray& chal, unsigned retries) { - if (YubiKey::instance()->challenge(m_slot, true, chal, m_key) != YubiKey::ERROR) { - return true; - } + Q_ASSERT(retries > 0); - /* If challenge failed, retry to detect YubiKeys in the event the YubiKey - * was un-plugged and re-plugged */ - while (retries > 0) { -#ifdef QT_DEBUG - qDebug() << "Attempt" << retries << "to re-detect YubiKey(s)"; -#endif - retries--; + do { + --retries; - if (YubiKey::instance()->init() != true) { + if (m_blocking) { + emit userInteractionRequired(); + } + + auto result = YubiKey::instance()->challenge(m_slot, true, chal, m_key); + + if (m_blocking) { + emit userConfirmed(); + } + + if (result != YubiKey::ERROR) { + return true; + } + + // if challenge failed, retry to detect YubiKeys in the event the YubiKey was un-plugged and re-plugged + if (retries > 0 && YubiKey::instance()->init() != true) { continue; } - if (YubiKey::instance()->challenge(m_slot, true, chal, m_key) != YubiKey::ERROR) { - return true; - } - } + } while (retries > 0); return false; } diff --git a/src/keys/YkChallengeResponseKey.h b/src/keys/YkChallengeResponseKey.h index 8acb0f9e..96e44220 100644 --- a/src/keys/YkChallengeResponseKey.h +++ b/src/keys/YkChallengeResponseKey.h @@ -22,20 +22,34 @@ #include "keys/ChallengeResponseKey.h" #include "keys/drivers/YubiKey.h" -class YkChallengeResponseKey : public ChallengeResponseKey +#include + +class YkChallengeResponseKey : public QObject, public ChallengeResponseKey { + Q_OBJECT + public: - YkChallengeResponseKey(int slot = -1, - bool blocking = false); + YkChallengeResponseKey(int slot = -1, bool blocking = false); QByteArray rawKey() const; - YkChallengeResponseKey* clone() const; bool challenge(const QByteArray& chal); - bool challenge(const QByteArray& chal, int retries); + bool challenge(const QByteArray& chal, unsigned 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: QByteArray m_key; int m_slot;