wallet2: validate key data in password and device checks
What the developer wrote
Authored by selsta
wallet2: validate key data in password and device checks
Reported by xmrack and the MAGIC Monero Fund.
What changed, and why it matters
This update adds safety checks to Monero's wallet code when reading wallet key files. Previously, the code assumed the file contained a properly formatted JSON object with a 'key_data' text field. If a file was malformed or missing that field, the program could read from invalid memory and crash or behave unpredictably. The patch now checks the structure before using the data and returns an error instead.
Apply the patch. Ensure all RapidJSON value accesses elsewhere in wallet2.cpp and related wallet code follow the same validate-before-use pattern, especially for fields loaded from user-supplied wallet files.
Security signals we found
Missing input validation on parsed JSON before pointer/string access
Potential null-pointer or out-of-bounds read in wallet key file handling
Crash or undefined behavior possible with malformed wallet keys file
Reported by external party (xmrack and MAGIC Monero Fund)
Evidence from the diff
In wallet2.cpp, both verify_password() and query_device() parse a wallet keys JSON file using RapidJSON. Before accessing json[“key_data”].GetString() and GetStringLength(), the code now verifies that json is an object, contains the member “key_data”, and that the member is a string. Without these checks, calling GetString() on a non-string Value or accessing a missing member could lead to undefined behavior, including null-pointer dereference or out-of-bounds reads. The fix is defensive input validation for wallet key file parsing.
Changed components
src/wallet/wallet2.cppwallet2::verify_password()wallet2::query_device()Monero wallet key file parsingInspect captured patch +10 / −0
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index a0191e6..d10c0d6 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -5530,6 +5530,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);
@@ -5650,6 +5655,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());
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.