From 1a2721529d6356125895d5b54a8ffa04800843d5 Mon Sep 17 00:00:00 2001
From: Jonathan White <support@dmapps.us>
Date: Tue, 30 Oct 2018 17:17:52 -0400
Subject: [PATCH] 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
---
 src/keys/YkChallengeResponseKey.cpp | 24 ++++++------------------
 src/keys/YkChallengeResponseKey.h   |  2 +-
 src/keys/drivers/YubiKey.cpp        | 18 ++++++++----------
 src/keys/drivers/YubiKey.h          | 12 +-----------
 4 files changed, 16 insertions(+), 40 deletions(-)

diff --git a/src/keys/YkChallengeResponseKey.cpp b/src/keys/YkChallengeResponseKey.cpp
index ade1b6324..830e0c0e0 100644
--- a/src/keys/YkChallengeResponseKey.cpp
+++ b/src/keys/YkChallengeResponseKey.cpp
@@ -18,6 +18,7 @@
 #include "keys/YkChallengeResponseKey.h"
 #include "keys/drivers/YubiKey.h"
 
+#include "core/AsyncTask.h"
 #include "core/Tools.h"
 #include "crypto/CryptoHash.h"
 #include "crypto/Random.h"
@@ -56,10 +57,8 @@ bool YkChallengeResponseKey::challenge(const QByteArray& challenge)
     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 {
         --retries;
 
@@ -67,28 +66,17 @@ bool YkChallengeResponseKey::challenge(const QByteArray& challenge, unsigned ret
             emit userInteractionRequired();
         }
 
-        QFuture<YubiKey::ChallengeResult> future = QtConcurrent::run(
-            [this, challenge]() { 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();
+        auto result = AsyncTask::runAndWaitForFuture([this, challenge]() {
+            return YubiKey::instance()->challenge(m_slot, true, challenge, m_key);
+        });
 
         if (m_blocking) {
             emit userConfirmed();
         }
 
-        if (future.result() != YubiKey::ERROR) {
+        if (result == YubiKey::SUCCESS) {
             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);
 
     return false;
diff --git a/src/keys/YkChallengeResponseKey.h b/src/keys/YkChallengeResponseKey.h
index 0ce915271..fccd90811 100644
--- a/src/keys/YkChallengeResponseKey.h
+++ b/src/keys/YkChallengeResponseKey.h
@@ -35,7 +35,7 @@ public:
 
     QByteArray rawKey() const 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;
     bool isBlocking() const;
 
diff --git a/src/keys/drivers/YubiKey.cpp b/src/keys/drivers/YubiKey.cpp
index 38ea818e5..bd09fb9f6 100644
--- a/src/keys/drivers/YubiKey.cpp
+++ b/src/keys/drivers/YubiKey.cpp
@@ -161,19 +161,14 @@ bool YubiKey::getSerial(unsigned int& serial)
 
 YubiKey::ChallengeResult YubiKey::challenge(int slot, bool mayBlock, const QByteArray& challenge, QByteArray& response)
 {
-    if (!m_mutex.tryLock()) {
-        return ALREADY_RUNNING;
+    // ensure that YubiKey::init() succeeded
+    if (!init()) {
+        return ERROR;
     }
 
     int yk_cmd = (slot == 1) ? SLOT_CHAL_HMAC1 : SLOT_CHAL_HMAC2;
     QByteArray paddedChallenge = challenge;
 
-    // ensure that YubiKey::init() succeeded
-    if (!init()) {
-        m_mutex.unlock();
-        return ERROR;
-    }
-
     // yk_challenge_response() insists on 64 byte response buffer */
     response.clear();
     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());
     r = reinterpret_cast<unsigned char*>(response.data());
 
-    int ret = yk_challenge_response(m_yk, yk_cmd, mayBlock, paddedChallenge.size(), c, response.size(), r);
-    emit challenged();
+    // 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(m_yk, yk_cmd, mayBlock, paddedChallenge.size(), c, response.size(), r);
     m_mutex.unlock();
 
     if (!ret) {
diff --git a/src/keys/drivers/YubiKey.h b/src/keys/drivers/YubiKey.h
index 39085546b..14cb43394 100644
--- a/src/keys/drivers/YubiKey.h
+++ b/src/keys/drivers/YubiKey.h
@@ -68,7 +68,7 @@ public:
      * @param mayBlock operation is allowed to block
      * @param challenge challenge input to 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);
 
@@ -98,21 +98,11 @@ signals:
      */
     void detectComplete();
 
-    /**
-     * Emitted when the YubiKey was challenged and has returned a response.
-     */
-    void challenged();
-
     /**
      * Emitted when no Yubikey could be found.
      */
     void notFound();
 
-    /**
-     * Emitted when detection is already running.
-     */
-    void alreadyRunning();
-
 private:
     explicit YubiKey();
     static YubiKey* m_instance;