What the developer wrote
Authored by tobtoht
Merge pull request #11274
a319868 wallet2: validate account tags before mutation (Samy)
ACKs: jpk68, selsta
What changed, and why it matters
This commit fixes a bug in Monero's wallet software where the program would partially change account labels before checking whether all requested accounts actually exist. The fix moves the boundary check to the very beginning so the operation either fully succeeds or fully fails, and no partial changes are saved. The practical security impact is limited because the bug only affects account tagging metadata, not balances or transactions.
Treat as a routine correctness/defensive fix. No urgent security response is indicated, but include it in normal release notes as a wallet robustness improvement.
Security signals we found
Atomicity bug: partial state mutation before full input validation
Out-of-bounds access prevented by early validation loop
Functional test added for negative-path rejection
No cryptographic, consensus, or balance-handling code changed
Evidence from the diff
In wallet2::set_account_tag(), the original loop both validated each account index and mutated m_account_tags.second[account_index] in the same iteration. That meant an out-of-bounds account index appearing after a valid one could leave earlier tags already changed. The patch splits validation into a separate first loop that throws wallet_internal_error before any mutation occurs. A functional test is added to verify that tagging/untagging with an invalid index [0, 3] is rejected atomically and leaves existing tags unchanged.
Changed components
src/wallet/wallet2.cpp::wallet2::set_account_tag()tests/functional_tests/wallet.py::WalletTest.tags()Inspect captured patch +24 / −0
### src/wallet/wallet2.cpp
@@ -13142,6 +13142,10 @@ void wallet2::set_account_tag(const std::set<uint32_t> &account_indices, const s
for (uint32_t account_index : account_indices)
{
THROW_WALLET_EXCEPTION_IF(account_index >= get_num_subaddress_accounts(), error::wallet_internal_error, "Account index out of bound");
+ }
+
+ for (uint32_t account_index : account_indices)
+ {
if (m_account_tags.second[account_index] == tag)
MDEBUG("This tag is already assigned to this account");
else
### tests/functional_tests/wallet.py
@@ -247,6 +247,26 @@ def tags(self):
assert res.account_tags[0].tag == 'tagB'
assert res.account_tags[0].label == ''
assert res.account_tags[0].accounts == [0, 1]
+ ok = False
+ try: wallet.tag_accounts('tagC', [0, 3])
+ except Exception as e:
+ assert 'Account index out of bound' in str(e)
+ ok = True
+ assert ok
+ res = wallet.get_account_tags()
+ assert len(res.account_tags) == 1
+ assert res.account_tags[0].tag == 'tagB'
+ assert res.account_tags[0].accounts == [0, 1]
+ ok = False
+ try: wallet.untag_accounts([0, 3])
+ except Exception as e:
+ assert 'Account index out of bound' in str(e)
+ ok = True
+ assert ok
+ res = wallet.get_account_tags()
+ assert len(res.account_tags) == 1
+ assert res.account_tags[0].tag == 'tagB'
+ assert res.account_tags[0].accounts == [0, 1]
wallet.set_account_tag_description('tagB', 'tag B')
res = wallet.get_account_tags()
assert len(res.account_tags) == 1Why this scored 37/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.