Correct simultaneous saving with Yubikey

* Move mutex lock to right before challenge call and wait for up to 1 second for unlock
* Fix bug where ALREADY_RUNNING was interpreted as success and causing database corruption
This commit is contained in:
Jonathan White 2018-10-30 17:17:52 -04:00
parent d84ba23c81
commit 1a2721529d
4 changed files with 16 additions and 40 deletions

View file

@ -18,6 +18,7 @@
#include "keys/YkChallengeResponseKey.h" #include "keys/YkChallengeResponseKey.h"
#include "keys/drivers/YubiKey.h" #include "keys/drivers/YubiKey.h"
#include "core/AsyncTask.h"
#include "core/Tools.h" #include "core/Tools.h"
#include "crypto/CryptoHash.h" #include "crypto/CryptoHash.h"
#include "crypto/Random.h" #include "crypto/Random.h"
@ -56,10 +57,8 @@ bool YkChallengeResponseKey::challenge(const QByteArray& challenge)
return this->challenge(challenge, 2); return this->challenge(challenge, 2);
} }
bool YkChallengeResponseKey::challenge(const QByteArray& challenge, unsigned retries) bool YkChallengeResponseKey::challenge(const QByteArray& challenge, unsigned int retries)
{ {
Q_ASSERT(retries > 0);
do { do {
--retries; --retries;
@ -67,28 +66,17 @@ bool YkChallengeResponseKey::challenge(const QByteArray& challenge, unsigned ret
emit userInteractionRequired(); emit userInteractionRequired();
} }
QFuture<YubiKey::ChallengeResult> future = QtConcurrent::run( auto result = AsyncTask::runAndWaitForFuture([this, challenge]() {
[this, challenge]() { return YubiKey::instance()->challenge(m_slot, true, challenge, m_key); }); return YubiKey::instance()->challenge(m_slot, true, challenge, m_key);
});
QEventLoop loop;
QFutureWatcher<YubiKey::ChallengeResult> watcher;
connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit()));
watcher.setFuture(future);
loop.exec();
if (m_blocking) { if (m_blocking) {
emit userConfirmed(); emit userConfirmed();
} }
if (future.result() != YubiKey::ERROR) { if (result == YubiKey::SUCCESS) {
return true; 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()) {
continue;
}
} while (retries > 0); } while (retries > 0);
return false; return false;

View file

@ -35,7 +35,7 @@ public:
QByteArray rawKey() const override; QByteArray rawKey() const override;
bool challenge(const QByteArray& challenge) override; bool challenge(const QByteArray& challenge) override;
bool challenge(const QByteArray& challenge, unsigned retries); bool challenge(const QByteArray& challenge, unsigned int retries);
QString getName() const; QString getName() const;
bool isBlocking() const; bool isBlocking() const;

View file

@ -161,19 +161,14 @@ bool YubiKey::getSerial(unsigned int& serial)
YubiKey::ChallengeResult YubiKey::challenge(int slot, bool mayBlock, const QByteArray& challenge, QByteArray& response) YubiKey::ChallengeResult YubiKey::challenge(int slot, bool mayBlock, const QByteArray& challenge, QByteArray& response)
{ {
if (!m_mutex.tryLock()) { // ensure that YubiKey::init() succeeded
return ALREADY_RUNNING; if (!init()) {
return ERROR;
} }
int yk_cmd = (slot == 1) ? SLOT_CHAL_HMAC1 : SLOT_CHAL_HMAC2; int yk_cmd = (slot == 1) ? SLOT_CHAL_HMAC1 : SLOT_CHAL_HMAC2;
QByteArray paddedChallenge = challenge; QByteArray paddedChallenge = challenge;
// ensure that YubiKey::init() succeeded
if (!init()) {
m_mutex.unlock();
return ERROR;
}
// yk_challenge_response() insists on 64 byte response buffer */ // yk_challenge_response() insists on 64 byte response buffer */
response.clear(); response.clear();
response.resize(64); response.resize(64);
@ -194,9 +189,12 @@ YubiKey::ChallengeResult YubiKey::challenge(int slot, bool mayBlock, const QByte
c = reinterpret_cast<const unsigned char*>(paddedChallenge.constData()); c = reinterpret_cast<const unsigned char*>(paddedChallenge.constData());
r = reinterpret_cast<unsigned char*>(response.data()); r = reinterpret_cast<unsigned char*>(response.data());
int ret = yk_challenge_response(m_yk, yk_cmd, mayBlock, paddedChallenge.size(), c, response.size(), r); // Try to grab a lock for 1 second, fail out if not possible
emit challenged(); if (!m_mutex.tryLock(1000)) {
return ALREADY_RUNNING;
}
int ret = yk_challenge_response(m_yk, yk_cmd, mayBlock, paddedChallenge.size(), c, response.size(), r);
m_mutex.unlock(); m_mutex.unlock();
if (!ret) { if (!ret) {

View file

@ -68,7 +68,7 @@ public:
* @param mayBlock operation is allowed to block * @param mayBlock operation is allowed to block
* @param challenge challenge input to YubiKey * @param challenge challenge input to YubiKey
* @param response response output from YubiKey * @param response response output from YubiKey
* @return true on success * @return challenge result
*/ */
ChallengeResult challenge(int slot, bool mayBlock, const QByteArray& challenge, QByteArray& response); ChallengeResult challenge(int slot, bool mayBlock, const QByteArray& challenge, QByteArray& response);
@ -98,21 +98,11 @@ signals:
*/ */
void detectComplete(); void detectComplete();
/**
* Emitted when the YubiKey was challenged and has returned a response.
*/
void challenged();
/** /**
* Emitted when no Yubikey could be found. * Emitted when no Yubikey could be found.
*/ */
void notFound(); void notFound();
/**
* Emitted when detection is already running.
*/
void alreadyRunning();
private: private:
explicit YubiKey(); explicit YubiKey();
static YubiKey* m_instance; static YubiKey* m_instance;