Cleanup + TODOs

- Password data removal - TODOs and descriptions for some functions
keepassxreboot · Aug 6, 2022 · e552c11 · e552c11
1 parent 0af6bfc
commit e552c11
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 14 deletions.
diff --git a/src/touchid/TouchID.h b/src/touchid/TouchID.h
@@ -23,11 +23,12 @@ class TouchID
     bool containsKey(const QString& databasePath) const;
     void reset(const QString& databasePath = "");
 
-    static bool isAvailable();
+    bool isAvailable();
+
+private:
     static bool isWatchAvailable();
     static bool isTouchIdAvailable();
-
-private:
+
     static void deleteKeyEntry(const QString& accountName);
     static QString databaseKeyName(const QString& databasePath);
 

diff --git a/src/touchid/TouchID.mm b/src/touchid/TouchID.mm
@@ -5,6 +5,8 @@
 #include "crypto/CryptoHash.h"
 #include "config-keepassx.h"
 
+#include <botan/mem_ops.h>
+
 #include <Foundation/Foundation.h>
 #include <CoreFoundation/CoreFoundation.h>
 #include <LocalAuthentication/LocalAuthentication.h>
@@ -115,9 +117,6 @@ inline CFMutableDictionaryRef makeDictionary() {
         return false;
     }
 
-    // memorize which database the stored key is for
-    m_encryptedMasterKeys.insert(databasePath, encryptedMasterKey);
-
     const QString keyName = databaseKeyName(databasePath);
 
     deleteKeyEntry(keyName); // Try to delete the existing key entry
@@ -177,12 +176,16 @@ inline CFMutableDictionaryRef makeDictionary() {
     CFRelease(attributes);
 
     if (status != errSecSuccess) {
-        m_encryptedMasterKeys.remove(databasePath);
         return false;
     }
 
-    debug("TouchID::storeKey - Success!");
+    // Cleanse the key information from the memory
+    Botan::secure_scrub_memory(randomKey.data(), randomKey.size());
+    Botan::secure_scrub_memory(randomIV.data(), randomIV.size());
 
+    // memorize which database the stored key is for
+    m_encryptedMasterKeys.insert(databasePath, encryptedMasterKey);
+    debug("TouchID::storeKey - Success!");
     return true;
 }
 
@@ -254,6 +257,10 @@ inline CFMutableDictionaryRef makeDictionary() {
         return false;
     }
 
+    // Cleanse the key information from the memory
+    Botan::secure_scrub_memory(key.data(), key.size());
+    Botan::secure_scrub_memory(iv.data(), iv.size());
+
     return true;
 }
 
@@ -262,6 +269,12 @@ inline CFMutableDictionaryRef makeDictionary() {
     return m_encryptedMasterKeys.contains(dbPath);
 }
 
+// TODO: Both functions below should probably handle the returned errors to
+// provide more information on availability. E.g.: the closed laptop lid results
+// in an error (because touch id is not unavailable). That error could be
+// displayed to the user when we first check for availability instead of just
+// hiding the checkbox.
+
 //! @return true if Apple Watch is available for authentication.
 bool TouchID::isWatchAvailable()
 {
@@ -270,10 +283,17 @@ inline CFMutableDictionaryRef makeDictionary() {
       LAContext *context = [[LAContext alloc] init];
 
       LAPolicy policyCode = LAPolicyDeviceOwnerAuthenticationWithWatch;
+      NSError *error;
 
-      bool canAuthenticate = [context canEvaluatePolicy:policyCode error:nil];
+      bool canAuthenticate = [context canEvaluatePolicy:policyCode error:&error];
       [context release];
-      debug("Apple Wach available: %d", canAuthenticate);
+      if (error) {
+         debug("Apple Wach available: %d (%ld / %s / %s)", canAuthenticate,
+               (long)error.code, error.description.UTF8String,
+               error.localizedDescription.UTF8String);
+      } else {
+          debug("Apple Wach available: %d", canAuthenticate);
+      }
       return canAuthenticate;
    } @catch (NSException *) {
       return false;
@@ -291,10 +311,17 @@ inline CFMutableDictionaryRef makeDictionary() {
       LAContext *context = [[LAContext alloc] init];
 
       LAPolicy policyCode = LAPolicyDeviceOwnerAuthenticationWithBiometrics;
+      NSError *error;
 
-      bool canAuthenticate = [context canEvaluatePolicy:policyCode error:nil];
+      bool canAuthenticate = [context canEvaluatePolicy:policyCode error:&error];
       [context release];
-      debug("Touch ID available: %d", canAuthenticate);
+      if (error) {
+         debug("Touch ID available: %d (%ld / %s / %s)", canAuthenticate,
+               (long)error.code, error.description.UTF8String,
+               error.localizedDescription.UTF8String);
+      } else {
+          debug("Touch ID available: %d", canAuthenticate);
+      }
       return canAuthenticate;
    } @catch (NSException *) {
       return false;
@@ -309,8 +336,10 @@ inline CFMutableDictionaryRef makeDictionary() {
 {
    // note: we cannot cache the check results because the configuration
    // is dynamic in its nature. User can close the laptop lid or take off
-   // the watch, thus making one (or both )of the authentication types unavailable.
-   return isWatchAvailable() || isTouchIdAvailable();
+   // the watch, thus making one (or both) of the authentication types unavailable.
+   const bool watchAvailable = isWatchAvailable();
+   const bool touchIdAvailable = isTouchIdAvailable();
+   return  watchAvailable || touchIdAvailable;
 }
 
 /**