wallet: roll back unlocker count on decrypt failure
What the developer wrote
Authored by fuyua9
wallet: roll back unlocker count on decrypt failure
What changed, and why it matters
This commit fixes a bookkeeping bug in the Monero wallet's key-unlock mechanism. Previously, if a user entered the wrong password while trying to decrypt wallet keys, the internal counter that tracks active unlockers was increased but never decreased. That left the wallet thinking it was still unlocked even though the keys had not actually been decrypted. The fix rolls back the counter and removes the wallet's entry from the tracking map when decryption fails. The included test confirms that after a failed unlock attempt, a later correct password still works and keys remain properly locked in between.
Treat as a low-to-moderate reliability/security fix. Merge the patch and ensure the new unit test passes. Review other RAII state-changing constructors in the wallet for similar imbalance patterns. No immediate incident response is indicated absent a demonstrated exploit path.
Security signals we found
Incorrect state tracking after failed cryptographic operation
Resource/accounting imbalance in RAII unlocker constructor/destructor pair
Potential for wallet to believe keys are unlocked when they are not, or vice versa
Test added to reproduce and prevent regression of the failure path
Evidence from the diff
wallet_keys_unlocker’s constructor increments lockers_per_wallet[wallet_ptr] before calling w.decrypt_keys(key). If decrypt_keys throws (e.g., because the password is wrong), the pre-incremented count was never reverted, so the map retained a positive count for that wallet. The destructor only decrements and erases, so the stale count persisted. The patch wraps decrypt_keys in a try/catch, decrements the counter on exception, and erases the map entry if the count reaches zero. A unit test verifies the rollback: wrong-password unlocker throws, keys stay encrypted, and a subsequent correct unlocker succeeds and then re-locks on scope exit.
Changed components
src/wallet/wallet2.cppwallet_keys_unlocker RAII classwallet key encryption/decryption state trackingInspect captured patch +35 / −2
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 15756ab..e701a0a 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -1121,10 +1121,20 @@ wallet_keys_unlocker::wallet_keys_unlocker(wallet2 &w, const epee::wipeable_stri
w.generate_chacha_key_from_password(*password, key);
boost::lock_guard<boost::mutex> lock(lockers_lock);
- if (lockers_per_wallet[std::addressof(w)]++ > 0)
+ wallet2* w_ptr = std::addressof(w);
+ if (lockers_per_wallet[w_ptr]++ > 0)
return;
- w.decrypt_keys(key);
+ try
+ {
+ w.decrypt_keys(key);
+ }
+ catch (...)
+ {
+ if (--lockers_per_wallet[w_ptr] == 0)
+ lockers_per_wallet.erase(w_ptr);
+ throw;
+ }
}
wallet_keys_unlocker::~wallet_keys_unlocker()
diff --git a/tests/unit_tests/wallet_storage.cpp b/tests/unit_tests/wallet_storage.cpp
index 69c2f76..6d25686 100644
--- a/tests/unit_tests/wallet_storage.cpp
+++ b/tests/unit_tests/wallet_storage.cpp
@@ -595,3 +595,26 @@ TEST(wallet_keys_unlocker, first_not_locked)
ASSERT_TRUE(verify_wallet_privkeys(w1));
}
}
+
+TEST(wallet_keys_unlocker, construction_failure_rolls_back_lock_count)
+{
+ const epee::wipeable_string password("correct horse battery staple");
+ const epee::wipeable_string wrong_password("correct horse battery stable");
+
+ tools::wallet2 w;
+ w.generate("", password);
+ ASSERT_TRUE(w.is_key_encryption_enabled());
+ ASSERT_FALSE(w.is_unattended());
+ ASSERT_FALSE(verify_wallet_privkeys(w));
+
+ ASSERT_ANY_THROW({
+ tools::wallet_keys_unlocker ul(w, &wrong_password);
+ });
+ ASSERT_FALSE(verify_wallet_privkeys(w));
+
+ {
+ tools::wallet_keys_unlocker ul(w, &password);
+ ASSERT_TRUE(verify_wallet_privkeys(w));
+ }
+ ASSERT_FALSE(verify_wallet_privkeys(w));
+}
Why this scored 44/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.