diff --git a/src/simplewallet/simplewallet.cpp b/src/simplewallet/simplewallet.cpp index 28048590b..f02423f72 100644 --- a/src/simplewallet/simplewallet.cpp +++ b/src/simplewallet/simplewallet.cpp @@ -117,7 +117,7 @@ typedef cryptonote::simple_wallet sw; LOCK_IDLE_SCOPE(); \ boost::optional 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;) diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 04be12c13..6f787ed2a 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -30,6 +30,7 @@ #include #include +#include #include #include #include @@ -1097,50 +1098,46 @@ uint64_t gamma_picker::pick() }; boost::mutex wallet_keys_unlocker::lockers_lock; -unsigned int wallet_keys_unlocker::lockers = 0; -wallet_keys_unlocker::wallet_keys_unlocker(wallet2 &w, const boost::optional &password): +std::map wallet_keys_unlocker::lockers_per_wallet = {}; +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 lock(lockers_lock); - if (lockers++ > 0) - locked = false; - if (!locked || w.is_unattended() || w.ask_password() != tools::wallet2::AskPasswordToDecrypt || w.watch_only() || w.is_background_syncing()) + static_assert(!std::is_move_assignable_v && !std::is_move_constructible_v, + "Keeping track of wallet instances by pointer doesn't work if wallet2 can be moved"); + + 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(w), - locked(locked) -{ + w.generate_chacha_key_from_password(*password, key); + boost::lock_guard lock(lockers_lock); - if (lockers++ > 0) - locked = false; - if (!locked) + if (lockers_per_wallet[std::addressof(w)]++ > 0) 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 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 (!locked) - return; + if (--lockers_per_wallet[w_ptr] > 0) + return; // there are other unlock-ers for this wallet, do nothing for now + 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 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 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 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 diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h index ddd11d78a..188727736 100644 --- a/src/wallet/wallet2.h +++ b/src/wallet/wallet2.h @@ -113,15 +113,14 @@ private: class wallet_keys_unlocker { public: - wallet_keys_unlocker(wallet2 &w, const boost::optional &password); - wallet_keys_unlocker(wallet2 &w, bool locked, const epee::wipeable_string &password); + wallet_keys_unlocker(wallet2 &w, 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 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); diff --git a/tests/unit_tests/wallet_storage.cpp b/tests/unit_tests/wallet_storage.cpp index 43c5954cd..69c2f7664 100644 --- a/tests/unit_tests/wallet_storage.cpp +++ b/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(p_pwd)) \ + : boost::optional{}) +#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 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 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 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)); + } +}