What the developer wrote
Authored by tobtoht
Merge pull request #11263
cbf3331 wallet2: validate key data in password and device checks (selsta)
923ee54 wallet2: check reserve proof additional key index (selsta)
4c82440 wallet2: validate legacy transfer output indices (selsta)
b76c4b7 wallet2: normalize account tags before assignment (selsta)
ACKs: thomasbuilds*, jpk68
What changed, and why it matters
This Monero wallet update adds several safety checks to prevent the wallet software from crashing or behaving incorrectly when handling unusual wallet files, transaction proofs, or old account data. It does not appear to be a remote money-stealing bug, but rather hardening against malformed inputs that could cause errors or unexpected behavior.
Treat as a routine hardening patch. Users running wallet software should upgrade to a release containing this commit. Wallet file authors and integrators should ensure wallet keys files conform to the expected JSON schema and avoid loading untrusted cache files.
Security signals we found
Input validation added to wallet keys JSON parsing
Bounds check added before indexing additional_tx_pub_keys vector
Legacy deserialization now rejects out-of-range output indices
Account tag assignment now normalizes internal state first
No explicit security framing in commit message or title
Evidence from the diff
The merge commit bundles four wallet2 hardening changes: (1) validation that wallet keys JSON contains a string ‘key_data’ field before use in verify_password and query_device; (2) a bounds check on additional_tx_pub_keys index when generating reserve proofs; (3) validation that legacy (pre-v4) transfer_details output indices are within the transaction’s vout vector before deserialization proceeds; and (4) a call to get_account_tags() before assigning account tags to ensure internal state is initialized. These are defensive fixes against malformed wallet files, crafted proofs, and corrupted/old cache data.
Changed components
src/wallet/wallet2.cppsrc/wallet/wallet2_basic/wallet2_boost_serialization.hInspect captured patch +16 / −2
### src/wallet/wallet2.cpp
@@ -5553,6 +5553,11 @@ bool wallet2::verify_password(const std::string& keys_file_name, const epee::wip
}
else
{
+ if (!json.IsObject() || !json.HasMember("key_data") || !json["key_data"].IsString())
+ {
+ LOG_ERROR("Invalid wallet keys JSON: expected an object with a string key_data field");
+ return false;
+ }
account_data = std::string(json["key_data"].GetString(), json["key_data"].GetString() +
json["key_data"].GetStringLength());
GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, encrypted_secret_keys, uint32_t, Uint, false, false);
@@ -5673,6 +5678,11 @@ bool wallet2::query_device(hw::device::device_type& device_type, const std::stri
}
else
{
+ if (!json.IsObject() || !json.HasMember("key_data") || !json["key_data"].IsString())
+ {
+ LOG_ERROR("Invalid wallet keys JSON: expected an object with a string key_data field");
+ return false;
+ }
account_data = std::string(json["key_data"].GetString(), json["key_data"].GetString() +
json["key_data"].GetStringLength());
@@ -12764,8 +12774,8 @@ std::string wallet2::get_reserve_proof(const boost::optional<std::pair<uint32_t,
error::wallet_internal_error, "Failed to derive subaddress public key");
if (m_subaddresses.count(subaddress_spendkey) == 1)
break;
- THROW_WALLET_EXCEPTION_IF(additional_tx_pub_keys.empty(), error::wallet_internal_error,
- "Normal tx pub key doesn't derive the expected output, while the additional tx pub keys are empty");
+ THROW_WALLET_EXCEPTION_IF(proof.index_in_tx >= additional_tx_pub_keys.size(), error::wallet_internal_error,
+ "Normal tx pub key doesn't derive the expected output, and no additional tx pub key exists for this output index");
THROW_WALLET_EXCEPTION_IF(i == 1, error::wallet_internal_error,
"Neither normal tx pub key nor additional tx pub key derive the expected output key");
tx_pub_key_used = &additional_tx_pub_keys[proof.index_in_tx];
@@ -13139,6 +13149,7 @@ const std::pair<std::map<std::string, std::string>, std::vector<std::string>>& w
void wallet2::set_account_tag(const std::set<uint32_t> &account_indices, const std::string& tag)
{
+ get_account_tags();
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");
### src/wallet/wallet2_basic/wallet2_boost_serialization.h
@@ -35,6 +35,7 @@
#include "wallet2_types.h"
//third party headers
+#include <boost/archive/archive_exception.hpp>
#include <boost/serialization/deque.hpp>
#include <boost/serialization/set.hpp>
#include <boost/serialization/vector.hpp>
@@ -71,6 +72,8 @@ template <class Archive>
std::enable_if_t<Archive::is_loading::value>
initialize_transfer_details(Archive &a, wallet2_basic::transfer_details &x, const unsigned int ver)
{
+ if (ver < 4 && x.m_internal_output_index >= x.m_tx.vout.size())
+ throw boost::archive::archive_exception(boost::archive::archive_exception::other_exception, "Invalid transfer output index");
if (ver < 1)
{
x.m_mask = rct::identity();Why this scored 59/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.