What the developer wrote
Authored by Lee *!* Clagett
Fixes for issues reported by llm (#297)
* Fix infinite loop when importing lookahead address with {0,0} output
* Fix zero threads on client
* Restrict remote scanner client message sizes until authentication
* Fix untrusted array (users, blocks) reads from client
* Fix overflow check
* Harden gamma picker
* Add check for relayed flag before adding to mempool list
* Fix double response on tx parse error
* Fix erase call in remote scanning round-robin algo
* Fix mempool metadata in pessimistic state
What changed, and why it matters
This commit fixes nine separate bugs in the Monero Light Wallet Server (LWS). The most serious ones are: an infinite loop when importing certain address data, a missing size limit that let unauthenticated remote clients request huge amounts of memory, reading untrusted array data from clients before proper validation, an incorrect overflow check when calculating subaddress counts, and a double HTTP response that could confuse clients. Several other fixes correct thread handling, mempool behavior, and a round-robin scheduling erase bug. The commit title says these issues were 'reported by llm'—likely an automated or large-language-model-assisted review—so the security relevance is acknowledged by the project author but not via a formal vendor security advisory.
Deploy this patch promptly, especially on any LWS instance that exposes the remote scanner or REST submit-tx endpoints. Review authentication boundaries on the scanner protocol and consider adding automated fuzzing for client-provided arrays and sizes. Monitor for any follow-up fixes from the 'llm' review.
Security signals we found
Infinite loop in database import path (DoS)
Missing authentication-time message size limit on remote scanner protocol (memory exhaustion / DoS)
Untrusted array reads from client in light wallet RPC
Incorrect integer overflow check for subaddress count
Double HTTP response on transaction parse error
Hardened gamma picker validity check
Zero-thread remote scanner client handling
Incorrect vector erase in round-robin account distribution
Evidence from the diff
The patch addresses multiple correctness/security issues: (1) moves mdb_cursor_get outside the inner loop in storage.cpp to prevent an infinite loop when importing lookahead addresses with a {0,0} output; (2) fixes an inverted overflow test in subaddress multiplication (major <= max/minor instead of major/minor <= max); (3) prevents a double response in /submit_raw_tx by popping the resumer before returning an error; (4) adds a relayed-flag check before mempool insertion; (5) enforces a 1 MiB message-size cap on unauthenticated remote-scanner clients and requires a next_size() hook so connections can reject oversized/untrusted messages; (6) fixes untrusted array reads in light_wallet.cpp by continuing instead of breaking and using a const reference; (7) hardens gamma_picker validity by requiring positive outputs_per_second; (8) skips zero-thread remote scanner clients; (9) fixes a single-iterator erase in the round-robin distribution that left stale elements. The changes are defensive and reduce DoS/overflow/remote-exploitation surface, but the commit is a collection of fixes rather than a single CVE-class vulnerability.
Changed components
src/db/storage.cppsrc/rest_server.cppsrc/rpc/light_wallet.cppsrc/rpc/scanner/client.cppsrc/rpc/scanner/client.hsrc/rpc/scanner/read_commands.hsrc/rpc/scanner/server.cppsrc/util/gamma_picker.cpptests/unit/db/storage.test.cppInspect captured patch +62 / −12
### src/db/storage.cpp
@@ -2390,9 +2390,9 @@ namespace db
}
else
elem = {major_index(major), index_ranges{{index_range{minor_index(0), minor_index(this_minor)}}}};
-
- err = mdb_cursor_get(&outputs_cur, &key, &value, MDB_NEXT_DUP);
}
+
+ err = mdb_cursor_get(&outputs_cur, &key, &value, MDB_NEXT_DUP);
}
}
}
@@ -3403,7 +3403,7 @@ namespace db
return 0;
expect<std::vector<subaddress_dict>> upserted{error::max_subaddresses};
- if (major / minor <= std::numeric_limits<decltype(major)>::max())
+ if (major <= std::numeric_limits<decltype(major)>::max() / minor)
{
if (major * minor <= max_subaddresses)
{
### src/rest_server.cpp
@@ -1494,7 +1494,10 @@ namespace lws
std::get<0>(elem) = std::move(msg);
std::get<1>(elem) = std::move(resume);
if (!cryptonote::parse_and_validate_tx_from_blob(tx_blob, std::get<2>(elem)))
+ {
+ active->resumers.pop_back();
return {lws::error::bad_client_tx};
+ }
return success();
}
@@ -1524,7 +1527,7 @@ namespace lws
}
else
{
- if (self_->parent && self_->parent->mempool)
+ if (value && self_->parent && self_->parent->mempool)
self_->parent->mempool->add_txs({std::addressof(std::get<2>(self_->resumers.front())), 1});
MDEBUG("Completed ZMQ request in /submit_raw_tx");
### src/rpc/light_wallet.cpp
@@ -741,11 +741,12 @@ namespace lws
}
else
{
- if (row.spends.empty() || from_future) break;
- auto spend = row.spends.front();
tx.info.link.tx_hash = row.hash;
tx.info.link.height = db::block_id::txpool;
tx.info.spend_meta.amount = tx_total;
+
+ if(row.spends.empty() || from_future) continue;
+ const auto& spend = row.spends.front();
tx.info.spend_meta.mixin_count = spend.mixin_count;
tx.info.timestamp = spend.timestamp;
tx.info.unlock_time = spend.unlock_time;
### src/rpc/scanner/client.cpp
@@ -173,6 +173,11 @@ namespace lws { namespace rpc { namespace scanner
client::~client()
{}
+ header::length_type::value_type client::next_size() const noexcept
+ {
+ return next_.length.value();
+ }
+
//! \return Handlers for commands from server
const std::array<client::command, 2>& client::commands() noexcept
{
### src/rpc/scanner/client.h
@@ -66,6 +66,9 @@ namespace lws { namespace rpc { namespace scanner
client& operator=(const client&) = delete;
client& operator=(client&&) = delete;
+ //! \return accept all message sizes
+ header::length_type::value_type next_size() const noexcept;
+
//! \return Handlers for client commands
static const std::array<command, 2>& commands() noexcept;
### src/rpc/scanner/read_commands.h
@@ -82,6 +82,8 @@ namespace lws { namespace rpc { namespace scanner
\tparam T concept requirements:
* Must be derived from `lws::rpc::scanner::connection`.
+ * Must have a `header::length_type::value_type next_size()` function that
+ returns the payload size of the next message, or `0` on error.
* Must have `cleanup()` function that invokes `base_cleanup()`, and
does any other necessary work given that the socket connection is being
terminated.
@@ -117,6 +119,7 @@ namespace lws { namespace rpc { namespace scanner
if (self_->cleanup_)
return; // callback queued before cancellation
+ header::length_type::value_type next_length = 0;
BOOST_ASIO_CORO_REENTER(*this)
{
for (;;) // multiple commands
@@ -127,9 +130,16 @@ namespace lws { namespace rpc { namespace scanner
);
std::memcpy(std::addressof(self_->next_), self_->read_buf_.data(), sizeof(self_->next_));
+ next_length = self_->next_size();
+ if (!next_length)
+ {
+ self_->cleanup();
+ return;
+ }
+
static_assert(std::numeric_limits<header::length_type::value_type>::max() <= std::numeric_limits<std::size_t>::max());
BOOST_ASIO_CORO_YIELD boost::asio::async_read(
- self_->sock_, self_->read_buffer(self_->next_.length.value()), boost::asio::bind_executor(self_->strand_, *this)
+ self_->sock_, self_->read_buffer(next_length), boost::asio::bind_executor(self_->strand_, *this)
);
const auto& commands = T::commands();
### src/rpc/scanner/server.cpp
@@ -57,6 +57,9 @@ namespace lws { namespace rpc { namespace scanner
//! Threshold for resetting/replacing state instead of pushing
constexpr const std::size_t replace_threshold = 10000;
+ //! Max incoming message size before valid authentication
+ constexpr const std::size_t max_unauthenticated = 1 * 1024 * 1024;
+
//! \brief Handler for server to initialize new scanner
struct initialize_handler
{
@@ -92,6 +95,14 @@ namespace lws { namespace rpc { namespace scanner
MONERO_THROW(common_error::kInvalidArgument, "nullptr parent");
}
+ header::length_type::value_type next_size() const noexcept
+ {
+ const auto length = next_.length.value();
+ if (authenticated_ || (next_.id == initialize_handler::input::id() && length <= max_unauthenticated))
+ return length;
+ return 0;
+ }
+
//! \return Handlers for commands from client
static const std::array<command, 2>& commands() noexcept
{
@@ -233,7 +244,8 @@ namespace lws { namespace rpc { namespace scanner
if (std::numeric_limits<std::size_t>::max() - total_threads < conn->threads_)
MONERO_THROW(error::configuration, "Exceeded max threads (size_t) across all systems");
total_threads += conn->threads_;
- remotes.push_back(std::move(conn));
+ if (conn->threads_)
+ remotes.push_back(std::move(conn));
}
if (!total_threads)
@@ -381,7 +393,7 @@ namespace lws { namespace rpc { namespace scanner
std::make_move_iterator(new_accounts.end() - user_count),
std::make_move_iterator(new_accounts.end())
};
- new_accounts.erase(new_accounts.end() - user_count);
+ new_accounts.erase(new_accounts.end() - user_count, new_accounts.end());
write_command(remotes[j], push_accounts{std::move(next)});
self_->next_thread_ += remotes[j]->threads_;
}
### src/util/gamma_picker.cpp
@@ -69,7 +69,7 @@ namespace lws
bool gamma_picker::is_valid() const noexcept
{
static_assert(CRYPTONOTE_DEFAULT_TX_SPENDABLE_AGE > 0);
- return CRYPTONOTE_DEFAULT_TX_SPENDABLE_AGE - 1 < rct_offsets.size();
+ return CRYPTONOTE_DEFAULT_TX_SPENDABLE_AGE - 1 < rct_offsets.size() && 0.f < outputs_per_second;
}
std::uint64_t gamma_picker::spendable_upper_bound() const noexcept
### tests/unit/db/storage.test.cpp
@@ -130,7 +130,7 @@ LWS_CASE("lws::db::storage")
EXPECT(get_account().lookahead_fail == lws::db::block_id(0));
}
- const auto add_output = [&] ()
+ const auto add_output = [&] (const std::uint32_t major = 2, const std::uint32_t minor = 10)
{
auto account = get_account();
const lws::db::transaction_link link{
@@ -170,7 +170,7 @@ LWS_CASE("lws::db::storage")
lws::db::pack(extra, sizeof(crypto::hash)),
payment_id_,
std::uint64_t(100),
- lws::db::address_index{lws::db::major_index(2), lws::db::minor_index(10)}
+ lws::db::address_index{lws::db::major_index(major), lws::db::minor_index(minor)}
}
);
@@ -267,5 +267,21 @@ LWS_CASE("lws::db::storage")
EXPECT(!db.shrink_lookahead(account_address, shrink));
}
}
+
+ SECTION("Lookahead with outputs to {0, 0}")
+ {
+ add_output(0, 0);
+ const auto scan_height = get_account().scan_height;
+
+ EXPECT(MONERO_UNWRAP(MONERO_UNWRAP(db.start_read()).get_subaddresses(lws::db::account_id(1))).empty());
+ EXPECT(db.import_request(account_address, scan_height, lookahead));
+ EXPECT(db.accept_requests(lws::db::request::import_scan, {std::addressof(account_address), 1}, 18));
+
+ const std::vector<lws::db::subaddress_dict> expected_range{
+ {lws::db::major_index(0), {{lws::db::index_range{lws::db::minor_index(0), lws::db::minor_index(1)}}}},
+ {lws::db::major_index(1), {{lws::db::index_range{lws::db::minor_index(0), lws::db::minor_index(1)}}}}
+ };
+ EXPECT(MONERO_UNWRAP(MONERO_UNWRAP(db.start_read()).get_subaddresses(lws::db::account_id(1))) == expected_range);
+ }
}
}Why this scored 71/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.