wallet: fix wallet_keys_unlocker

Fix four issues with wallet_keys_unlocker:
1. It won't decrypt if there are unlockers open simulataneously on multiple `wallet2` instances
2. It won't decrypt if the first unlocker was disabled (i.e. `locked=false`), even with a second non-disabled unlocker
3. If a destructor of an earlier unlocker is triggered before the destructor of a later unlocker, it will re-encrypt too early, while the second unlocker is still in scope
4. Calling the 3-parameter constructor with `locked=true` after an unlocker already exists will "double-encrypt"  the spend key after its destructor since the local variable is `locked=false` and the field member is `locked=true`

Also, replace `wallet2::decrypt_account_for_multisig` with `wallet_keys_unlocker`.

src/simplewallet/simplewallet.cpp

@@ -117,7 +117,7 @@ typedef cryptonote::simple_wallet sw;
   LOCK_IDLE_SCOPE(); \
   boost::optional<tools::password_container> pwd_container = boost::none; \
   if (m_wallet->ask_password() && !(pwd_container = get_and_verify_password())) { code; } \
-  tools::wallet_keys_unlocker unlocker(*m_wallet, pwd_container);
+  tools::wallet_keys_unlocker unlocker(*m_wallet, pwd_container ? &pwd_container->password() : nullptr);
 
 #define SCOPED_WALLET_UNLOCK() SCOPED_WALLET_UNLOCK_ON_BAD_PASSWORD(return true;)
 

src/wallet/wallet2.cpp

@@ -30,6 +30,7 @@
 
 #include <algorithm>
 #include <numeric>
+#include <optional>
 #include <string>
 #include <tuple>
 #include <queue>
@@ -1097,50 +1098,46 @@ uint64_t gamma_picker::pick()
 };
 
 boost::mutex wallet_keys_unlocker::lockers_lock;
-unsigned int wallet_keys_unlocker::lockers = 0;
+std::map<wallet2*, std::size_t> wallet_keys_unlocker::lockers_per_wallet = {};
-wallet_keys_unlocker::wallet_keys_unlocker(wallet2 &w, const boost::optional<tools::password_container> &password):
+wallet_keys_unlocker::wallet_keys_unlocker(wallet2 &w, const epee::wipeable_string *password):
   w(w),
-  locked(password != boost::none)
+  should_relock(true)
 {
-  boost::lock_guard<boost::mutex> lock(lockers_lock);
+  static_assert(!std::is_move_assignable_v<wallet2> && !std::is_move_constructible_v<wallet2>,
-  if (lockers++ > 0)
+    "Keeping track of wallet instances by pointer doesn't work if wallet2 can be moved");
-    locked = false;
+
-  if (!locked || w.is_unattended() || w.ask_password() != tools::wallet2::AskPasswordToDecrypt || w.watch_only() || w.is_background_syncing())
+  if (password == nullptr || !w.is_key_encryption_enabled())
   {
-    locked = false;
+    should_relock = false;
     return;
   }
-  const epee::wipeable_string pass = password->password();
-  w.generate_chacha_key_from_password(pass, key);
-  w.decrypt_keys(key);
-}
 
-wallet_keys_unlocker::wallet_keys_unlocker(wallet2 &w, bool locked, const epee::wipeable_string &password):
+  w.generate_chacha_key_from_password(*password, key);
-  w(w),
+
-  locked(locked)
-{
   boost::lock_guard<boost::mutex> lock(lockers_lock);
-  if (lockers++ > 0)
+  if (lockers_per_wallet[std::addressof(w)]++ > 0)
-    locked = false;
-  if (!locked)
     return;
-  w.generate_chacha_key_from_password(password, key);
+
   w.decrypt_keys(key);
 }
 
 wallet_keys_unlocker::~wallet_keys_unlocker()
 {
+  if (!should_relock)
+    return;
+
   try
   {
     boost::lock_guard<boost::mutex> lock(lockers_lock);
-    if (lockers == 0)
+    wallet2* w_ptr = std::addressof(w);
+    if (lockers_per_wallet[w_ptr] == 0)
     {
       MERROR("There are no lockers in wallet_keys_unlocker dtor");
       return;
     }
-    --lockers;
+    if (--lockers_per_wallet[w_ptr] > 0)
-    if (!locked)
+      return; // there are other unlock-ers for this wallet, do nothing for now
-      return;
+    lockers_per_wallet.erase(w_ptr);
     w.encrypt_keys(key);
   }
   catch (...)
@@ -2178,7 +2175,7 @@ void wallet2::scan_output(const cryptonote::transaction &tx, bool miner_tx, cons
       boost::optional<epee::wipeable_string> pwd = m_callback->on_get_password(pool ? "output found in pool" : "output received");
       THROW_WALLET_EXCEPTION_IF(!pwd, error::password_needed, tr("Password is needed to compute key image for incoming monero"));
       THROW_WALLET_EXCEPTION_IF(!verify_password(*pwd), error::password_needed, tr("Invalid password: password is needed to compute key image for incoming monero"));
-      m_encrypt_keys_after_refresh.reset(new wallet_keys_unlocker(*this, m_ask_password == AskPasswordToDecrypt && !m_unattended && !m_watch_only, *pwd));
+      m_encrypt_keys_after_refresh.reset(new wallet_keys_unlocker(*this, &*pwd));
     }
   }
 
@@ -5408,6 +5405,14 @@ bool wallet2::verify_password(const std::string& keys_file_name, const epee::wip
   return r;
 }
 
+bool wallet2::is_key_encryption_enabled() const
+{
+  return !is_unattended()
+    && ask_password() == tools::wallet2::AskPasswordToDecrypt
+    && !watch_only()
+    && !is_background_syncing();
+}
+
 void wallet2::encrypt_keys(const crypto::chacha_key &key)
 {
   m_account.encrypt_keys(key);
@@ -5808,27 +5813,6 @@ void wallet2::restore(const std::string& wallet_, const epee::wipeable_string& p
   }
 }
 //----------------------------------------------------------------------------------------------------
-epee::misc_utils::auto_scope_leave_caller wallet2::decrypt_account_for_multisig(const epee::wipeable_string &password)
-{
-  // decrypt account keys
-  // note: this conditional's clauses are old and undocumented
-  epee::misc_utils::auto_scope_leave_caller keys_reencryptor;
-  if (m_ask_password == AskPasswordToDecrypt && !m_unattended && !m_watch_only)
-  {
-    crypto::chacha_key chacha_key;
-    crypto::generate_chacha_key(password.data(), password.size(), chacha_key, m_kdf_rounds);
-    this->decrypt_keys(chacha_key);
-    keys_reencryptor = epee::misc_utils::create_scope_leave_handler(
-        [this, chacha_key]()
-        {
-          this->encrypt_keys(chacha_key);
-        }
-      );
-  }
-
-  return keys_reencryptor;
-}
-//----------------------------------------------------------------------------------------------------
 void wallet2::get_uninitialized_multisig_account(multisig::multisig_account &account_out) const
 {
   // create uninitialized multisig account
@@ -5874,7 +5858,7 @@ std::string wallet2::make_multisig(const epee::wipeable_string &password,
   const std::uint32_t threshold)
 {
   // decrypt account keys
-  epee::misc_utils::auto_scope_leave_caller keys_reencryptor{this->decrypt_account_for_multisig(password)};
+  std::optional<wallet_keys_unlocker> unlocker(std::in_place, *this, &password);
 
   // create multisig account
   multisig::multisig_account multisig_account;
@@ -5952,7 +5936,7 @@ std::string wallet2::make_multisig(const epee::wipeable_string &password,
   m_account_public_address.m_spend_public_key = multisig_account.get_multisig_pubkey();
 
   // re-encrypt keys
-  keys_reencryptor = epee::misc_utils::auto_scope_leave_caller();
+  unlocker.reset();
 
   if (!m_wallet_file.empty())
     this->create_keys_file(m_wallet_file, false, password, boost::filesystem::exists(m_wallet_file + ".address.txt"));
@@ -5973,7 +5957,7 @@ std::string wallet2::exchange_multisig_keys(const epee::wipeable_string &passwor
   CHECK_AND_ASSERT_THROW_MES(ms_status.multisig_is_active, "The wallet is not multisig");
 
   // decrypt account keys
-  epee::misc_utils::auto_scope_leave_caller keys_reencryptor{this->decrypt_account_for_multisig(password)};
+  std::optional<wallet_keys_unlocker> unlocker(std::in_place, *this, &password);
 
   // reconstruct multisig account
   multisig::multisig_account multisig_account;
@@ -6025,7 +6009,7 @@ std::string wallet2::exchange_multisig_keys(const epee::wipeable_string &passwor
   if (multisig_account.multisig_is_ready())
   {
     // keys are encrypted again
-    keys_reencryptor = epee::misc_utils::auto_scope_leave_caller();
+    unlocker.reset();
 
     if (!m_wallet_file.empty())
     {
@@ -6071,7 +6055,7 @@ std::string wallet2::get_multisig_key_exchange_booster(const epee::wipeable_stri
   CHECK_AND_ASSERT_THROW_MES(kex_messages.size() > 0, "No key exchange messages passed in.");
 
   // decrypt account keys
-  epee::misc_utils::auto_scope_leave_caller keys_reencryptor{this->decrypt_account_for_multisig(password)};
+  wallet_keys_unlocker unlocker(*this, &password);
 
   // prepare multisig account
   multisig::multisig_account multisig_account;
@@ -6459,7 +6443,7 @@ void wallet2::load(const std::string& wallet_, const epee::wipeable_string& pass
     THROW_WALLET_EXCEPTION_IF(true, error::file_read_error, "failed to load keys from buffer");
   }
 
-  wallet_keys_unlocker unlocker(*this, m_ask_password == AskPasswordToDecrypt && !m_unattended && !m_watch_only && !m_is_background_wallet, password);
+  wallet_keys_unlocker unlocker(*this, &password);
 
   //keys loaded ok!
   //try to load wallet cache. but even if we failed, it is not big problem

src/wallet/wallet2.h

@@ -113,15 +113,14 @@ private:
   class wallet_keys_unlocker
   {
   public:
-    wallet_keys_unlocker(wallet2 &w, const boost::optional<tools::password_container> &password);
+    wallet_keys_unlocker(wallet2 &w, const epee::wipeable_string *password);
-    wallet_keys_unlocker(wallet2 &w, bool locked, const epee::wipeable_string &password);
     ~wallet_keys_unlocker();
   private:
     wallet2 &w;
-    bool locked;
+    bool should_relock;
     crypto::chacha_key key;
     static boost::mutex lockers_lock;
-    static unsigned int lockers;
+    static std::map<wallet2*, std::size_t> lockers_per_wallet;
   };
 
   class i_wallet2_callback
@@ -952,11 +951,6 @@ private:
     void restore(const std::string& wallet_, const epee::wipeable_string& password, const std::string &device_name, bool create_address_file = false);
 
   private:
-    /*!
-     * \brief Decrypts the account keys
-     * \return an RAII reencryptor for the account keys
-     */
-    epee::misc_utils::auto_scope_leave_caller decrypt_account_for_multisig(const epee::wipeable_string &password);
     /*!
      * \brief Creates an uninitialized multisig account
      * \outparam: the uninitialized multisig account
@@ -1061,6 +1055,7 @@ private:
     cryptonote::account_base& get_account(){return m_account;}
     const cryptonote::account_base& get_account()const{return m_account;}
 
+    bool is_key_encryption_enabled() const;
     void encrypt_keys(const crypto::chacha_key &key);
     void encrypt_keys(const epee::wipeable_string &password);
     void decrypt_keys(const crypto::chacha_key &key);

tests/unit_tests/wallet_storage.cpp

@@ -382,3 +382,216 @@ TEST(wallet_storage, change_export_format)
 
     EXPECT_EQ(primary_address_1, primary_address_2);
 }
+
+#define OLD_WALLET_KEYS_UNLOCKER 0
+#if OLD_WALLET_KEYS_UNLOCKER
+#define WALLET_KEYS_UNLOCKER_CTOR(wal, p_pwd) tools::wallet_keys_unlocker(wal, \
+    p_pwd ? tools::password_container(*static_cast<const epee::wipeable_string*>(p_pwd)) \
+    : boost::optional<tools::password_container>{})
+#else
+#define WALLET_KEYS_UNLOCKER_CTOR tools::wallet_keys_unlocker
+#endif
+
+static bool verify_wallet_privkeys(const tools::wallet2 &w)
+{
+    hw::device &hwdev = hw::get_device("default");
+    const cryptonote::account_keys &keys = w.get_account().get_keys();
+    return hwdev.verify_keys(keys.m_spend_secret_key, keys.m_account_address.m_spend_public_key)
+        && hwdev.verify_keys(keys.m_view_secret_key, keys.m_account_address.m_view_public_key);
+}
+
+TEST(wallet_keys_unlocker, is_key_encryption_enabled)
+{
+    const epee::wipeable_string password1("Beleza pura, malandro!");
+    const epee::wipeable_string password2("correct horse battery staple");
+    {
+        tools::wallet2 w;
+        w.generate("", password1);
+        ASSERT_TRUE(w.is_key_encryption_enabled());
+    }
+    {
+        tools::wallet2 w(cryptonote::MAINNET, /*kdf_rounds=*/1, /*unattended=*/true);
+        w.generate("", password1);
+        ASSERT_FALSE(w.is_key_encryption_enabled()); // because unattended
+    }
+    {
+        tools::wallet2 w_cold;
+        w_cold.generate("", password1);
+        ASSERT_TRUE(w_cold.is_key_encryption_enabled());
+        tools::wallet2 w_hot;
+        w_hot.generate("", password2,
+            w_cold.get_address(),
+            w_cold.get_account().get_keys().m_view_secret_key);
+        ASSERT_FALSE(w_hot.is_key_encryption_enabled()); // because watch only
+    }
+    {
+        const path bg_wallet_file = unit_test::data_dir / "is_key_encryption_enabled_bg1";
+        if (is_file_exist(bg_wallet_file.string()))
+            remove(bg_wallet_file);
+        if (is_file_exist(bg_wallet_file.string() + ".keys"))
+            remove(bg_wallet_file.string() + ".keys");
+
+        tools::wallet2 w;
+        w.generate(bg_wallet_file.string(), password1);
+        ASSERT_TRUE(w.is_key_encryption_enabled());
+        w.setup_background_sync(tools::wallet2::BackgroundSyncReusePassword, password1, boost::none);
+        ASSERT_FALSE(w.is_background_syncing());
+        w.start_background_sync();
+        ASSERT_TRUE(w.is_background_syncing());
+        ASSERT_FALSE(verify_wallet_privkeys(w));
+        ASSERT_FALSE(w.is_key_encryption_enabled()); // because background syncing
+    }
+    {
+        const path bg_wallet_file = unit_test::data_dir / "is_key_encryption_enabled_bg2";
+        if (is_file_exist(bg_wallet_file.string()))
+            remove(bg_wallet_file);
+        if (is_file_exist(bg_wallet_file.string() + ".keys"))
+            remove(bg_wallet_file.string() + ".keys");
+
+        tools::wallet2 w;
+        w.generate(bg_wallet_file.string(), password1);
+        ASSERT_TRUE(w.is_key_encryption_enabled());
+        w.setup_background_sync(tools::wallet2::BackgroundSyncCustomPassword, password1, password2);
+        ASSERT_FALSE(w.is_background_syncing());
+        w.start_background_sync();
+        ASSERT_TRUE(w.is_background_syncing());
+        ASSERT_FALSE(verify_wallet_privkeys(w));
+        ASSERT_FALSE(w.is_key_encryption_enabled()); // because background syncing
+    }
+}
+
+TEST(wallet_keys_unlocker, simple_nonce)
+{
+    // Test that encrypted keys are different each time, i.e. that a nonce may actually be used
+
+    const epee::wipeable_string password("1612");
+    tools::wallet2 w;
+    w.generate("", password);
+    ASSERT_TRUE(w.is_key_encryption_enabled());
+    ASSERT_FALSE(w.is_unattended());
+    ASSERT_FALSE(verify_wallet_privkeys(w));
+    std::unordered_set<crypto::secret_key> encrypted_spendkeys;
+    encrypted_spendkeys.insert(w.get_account().get_keys().m_spend_secret_key);
+
+    const int n_locks = 10;
+    for (int i = 0; i < n_locks; ++i)
+    {
+        {
+            tools::wallet_keys_unlocker ul(w, &password);
+        }
+        const crypto::secret_key enc_spendkey = w.get_account().get_keys().m_spend_secret_key;
+        ASSERT_FALSE(encrypted_spendkeys.count(enc_spendkey));
+        encrypted_spendkeys.insert(enc_spendkey);
+    }
+
+    ASSERT_EQ(n_locks + 1, encrypted_spendkeys.size());
+}
+
+TEST(wallet_keys_unlocker, mutiple_attended)
+{
+    // Test that multiple non-unattended wallets can be decrypted at the same time.
+
+    const epee::wipeable_string password1("https://www.justice.gov/archives/opa/pr/bitcoin-fog-operator-convicted-money-laundering-conspiracy");
+    const epee::wipeable_string password2("https://cointelegraph.com/news/bad-blockchain-forensics-convict-roman-sterlingov");
+
+    tools::wallet2 w1;
+    w1.generate("", password1);
+    ASSERT_TRUE(w1.is_key_encryption_enabled());
+    ASSERT_FALSE(w1.is_unattended());
+    ASSERT_FALSE(verify_wallet_privkeys(w1));
+    const crypto::secret_key w1_ks_encrypted = w1.get_account().get_keys().m_spend_secret_key;
+    const crypto::public_key w1_spend_pubkey = w1.get_account().get_keys().m_account_address.m_spend_public_key;
+
+    tools::wallet2 w2;
+    w2.generate("", password2);
+    ASSERT_TRUE(w2.is_key_encryption_enabled());
+    ASSERT_FALSE(w2.is_unattended());
+    ASSERT_FALSE(verify_wallet_privkeys(w2));
+    const crypto::secret_key w2_ks_encrypted = w2.get_account().get_keys().m_spend_secret_key;
+    const crypto::public_key w2_spend_pubkey = w2.get_account().get_keys().m_account_address.m_spend_public_key;
+
+    ASSERT_NE(w1_ks_encrypted, w2_ks_encrypted);
+    ASSERT_NE(w1_spend_pubkey, w2_spend_pubkey);
+
+    crypto::secret_key w1_ks_unencrypted;
+    crypto::secret_key w2_ks_unencrypted;
+    for (size_t i = 0; i < 2; ++i)
+    {
+        tools::wallet_keys_unlocker ul1 = WALLET_KEYS_UNLOCKER_CTOR(w1, &password1);
+        w1_ks_unencrypted = w1.get_account().get_keys().m_spend_secret_key;
+        ASSERT_NE(w1_ks_encrypted, w1_ks_unencrypted);
+        EXPECT_TRUE(verify_wallet_privkeys(w1));
+
+        tools::wallet_keys_unlocker ul2 = WALLET_KEYS_UNLOCKER_CTOR(w2, &password2);
+        w2_ks_unencrypted = w2.get_account().get_keys().m_spend_secret_key;
+        ASSERT_NE(w2_ks_encrypted, w2_ks_unencrypted);
+        EXPECT_TRUE(verify_wallet_privkeys(w2));
+    }
+
+    ASSERT_NE(w1_ks_unencrypted, w1.get_account().get_keys().m_spend_secret_key);
+    ASSERT_NE(w2_ks_unencrypted, w2.get_account().get_keys().m_spend_secret_key);
+}
+
+TEST(wallet_keys_unlocker, non_concentric_lifetime)
+{
+    // Test that wallet unlock-ers which aren't concentric still keep the wallet decrypted as
+    // long as one of them is alive.
+
+    const epee::wipeable_string password1("540fa389d7cf4476b061f6443215583d739b01b5d7d9b972a9c0600084bb3694");
+
+    tools::wallet2 w1;
+    w1.generate("", password1);
+    ASSERT_TRUE(w1.is_key_encryption_enabled());
+    ASSERT_FALSE(w1.is_unattended());
+    ASSERT_FALSE(verify_wallet_privkeys(w1));
+    const crypto::secret_key w1_ks_encrypted = w1.get_account().get_keys().m_spend_secret_key;
+
+    std::unique_ptr<tools::wallet_keys_unlocker> ul1(new WALLET_KEYS_UNLOCKER_CTOR(w1, &password1));
+    const crypto::secret_key w1_ks_unencrypted_1 = w1.get_account().get_keys().m_spend_secret_key;
+    ASSERT_NE(w1_ks_encrypted, w1_ks_unencrypted_1);
+    ASSERT_TRUE(verify_wallet_privkeys(w1));
+
+    std::unique_ptr<tools::wallet_keys_unlocker> ul2(new WALLET_KEYS_UNLOCKER_CTOR(w1, &password1));
+    const crypto::secret_key w1_ks_unencrypted_2 = w1.get_account().get_keys().m_spend_secret_key;
+    ASSERT_EQ(w1_ks_unencrypted_1, w1_ks_unencrypted_2);
+
+    ul1.reset(); // call ul1 destructor before ul2 destructor
+
+    const crypto::secret_key w1_ks_unencrypted_3 = w1.get_account().get_keys().m_spend_secret_key;
+    ASSERT_EQ(w1_ks_unencrypted_1, w1_ks_unencrypted_3);
+
+    ul2.reset(); // call ul2 destructor
+
+    ASSERT_NE(w1_ks_unencrypted_1, w1.get_account().get_keys().m_spend_secret_key);
+    ASSERT_NE(w1_ks_encrypted, w1.get_account().get_keys().m_spend_secret_key); // should use a unique nonce
+
+    // test that wallets were re-encrypted correctly & recoverable after non-concentric dtors
+    ASSERT_FALSE(verify_wallet_privkeys(w1));
+    tools::wallet_keys_unlocker ul3(w1, &password1);
+    ASSERT_TRUE(verify_wallet_privkeys(w1));
+}
+
+TEST(wallet_keys_unlocker, first_not_locked)
+{
+    // Test that if the first unlock-er is disabled, then subsequent unlock-ers decrypt successfully
+
+    const epee::wipeable_string password1("Ashigaru");
+
+    tools::wallet2 w1;
+    w1.generate("", password1);
+    ASSERT_TRUE(w1.is_key_encryption_enabled());
+    ASSERT_FALSE(w1.is_unattended());
+    ASSERT_FALSE(verify_wallet_privkeys(w1));
+    const crypto::secret_key w1_ks_encrypted = w1.get_account().get_keys().m_spend_secret_key;
+
+    {
+        tools::wallet_keys_unlocker ul1 = WALLET_KEYS_UNLOCKER_CTOR(w1, nullptr);
+        const crypto::secret_key w1_ks_encrypted_2 = w1.get_account().get_keys().m_spend_secret_key;
+        ASSERT_EQ(w1_ks_encrypted_2, w1_ks_encrypted); // is disabled ?
+
+        tools::wallet_keys_unlocker ul2 = WALLET_KEYS_UNLOCKER_CTOR(w1, &password1);
+        const crypto::secret_key w1_ks_unencrypted = w1.get_account().get_keys().m_spend_secret_key;
+        ASSERT_NE(w1_ks_encrypted_2, w1_ks_unencrypted);
+        ASSERT_TRUE(verify_wallet_privkeys(w1));
+    }
+}