simplewallet: fix potential out-of-bounds read
What the developer wrote
Authored by jpk68
simplewallet: fix potential out-of-bounds read
What changed, and why it matters
This commit fixes the 'donate' command in Monero's command-line wallet. Previously, the command accepted an optional extra argument (an obsolete payment ID) and its argument-count check allowed up to 5 items. The code then tried to read the last item as an amount after optionally popping the payment ID, which could lead to reading from an empty argument list or misinterpreting arguments. The patch removes the obsolete payment ID handling and tightens the argument limit to 4, preventing the out-of-bounds or misdirected read. There is no evidence in the commit that this was exploitable for code execution or theft; it appears to be a defensive correctness fix.
Apply the patch. It is a low-risk cleanup that removes undefined behavior. Users of the CLI wallet should update to a version containing this commit. No immediate incident response is indicated by the diff alone.
Security signals we found
Out-of-bounds read / undefined behavior via std::vector::back() on potentially empty container
Removal of obsolete payment ID argument handling
Argument-count validation tightened from >5 to >4
Defensive hardening in CLI wallet command parsing
Evidence from the diff
In simple_wallet::donate(), the original code accepted 1-5 arguments and conditionally treated the last argument as a payment ID, popping it. After popping, it accessed local_args.back() assuming an amount remained. If a user supplied only a payment-ID-like string, or if parsing failed in unexpected ways, local_args could be empty, causing an out-of-bounds read (std::vector::back() on an empty vector is undefined behavior). The patch removes payment_id/payment_id8 parsing entirely, caps arguments at 4, and always treats the last argument as the amount. This eliminates the empty-vector back() scenario and simplifies the command.
Changed components
src/simplewallet/simplewallet.cppsimple_wallet::donate() CLI commandInspect captured patch +3 / −15
diff --git a/src/simplewallet/simplewallet.cpp b/src/simplewallet/simplewallet.cpp
index e2d9b37..fc937bf 100644
--- a/src/simplewallet/simplewallet.cpp
+++ b/src/simplewallet/simplewallet.cpp
@@ -203,7 +203,7 @@ namespace
const char* USAGE_SWEEP_ACCOUNT("sweep_account <account> [index=<N1>[,<N2>,...] | index=all] [<priority>] [<ring_size>] [outputs=<N>] <address> [<payment_id (obsolete)>]");
const char* USAGE_SWEEP_BELOW("sweep_below <amount_threshold> [index=<N1>[,<N2>,...]] [<priority>] [<ring_size>] <address> [<payment_id (obsolete)>]");
const char* USAGE_SWEEP_SINGLE("sweep_single [<priority>] [<ring_size>] [outputs=<N>] <key_image> <address> [<payment_id (obsolete)>]");
- const char* USAGE_DONATE("donate [index=<N1>[,<N2>,...]] [<priority>] [<ring_size>] <amount> [<payment_id (obsolete)>]");
+ const char* USAGE_DONATE("donate [index=<N1>[,<N2>,...]] [<priority>] [<ring_size>] <amount>");
const char* USAGE_SIGN_TRANSFER("sign_transfer [export_raw] [<filename>]");
const char* USAGE_SET_LOG("set_log <level>|{+,-,}<categories>");
const char* USAGE_ACCOUNT("account\n"
@@ -7389,22 +7389,12 @@ bool simple_wallet::donate(const std::vector<std::string> &args_)
{
CHECK_IF_BACKGROUND_SYNCING("cannot donate");
std::vector<std::string> local_args = args_;
- if(local_args.empty() || local_args.size() > 5)
+ if(local_args.empty() || local_args.size() > 4)
{
PRINT_USAGE(USAGE_DONATE);
return true;
}
std::string amount_str;
- std::string payment_id_str;
- // get payment id and pop
- crypto::hash payment_id;
- crypto::hash8 payment_id8;
- if (tools::wallet2::parse_long_payment_id (local_args.back(), payment_id ) ||
- tools::wallet2::parse_short_payment_id(local_args.back(), payment_id8))
- {
- payment_id_str = local_args.back();
- local_args.pop_back();
- }
// get amount and pop
uint64_t amount;
bool ok = cryptonote::parse_amount(amount, local_args.back());
@@ -7418,7 +7408,7 @@ bool simple_wallet::donate(const std::vector<std::string> &args_)
fail_msg_writer() << tr("amount is wrong: ") << local_args.back() << ", " << tr("expected number from 0 to ") << print_money(std::numeric_limits<uint64_t>::max());
return true;
}
- // push back address, amount, payment id
+ // push back address, amount
std::string address_str;
if (m_wallet->nettype() != cryptonote::MAINNET)
{
@@ -7437,8 +7427,6 @@ bool simple_wallet::donate(const std::vector<std::string> &args_)
}
local_args.push_back(address_str);
local_args.push_back(amount_str);
- if (!payment_id_str.empty())
- local_args.push_back(payment_id_str);
if (m_wallet->nettype() == cryptonote::MAINNET)
message_writer() << (boost::format(tr("Donating %s %s to The Monero Project (donate.getmonero.org or %s).")) % amount_str % cryptonote::get_unit(cryptonote::get_default_decimal_point()) % MONERO_DONATION_ADDR).str();
else
Why this scored 36/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.