wallet2: fix derivation handling in check_tx_proof and is_out_to_acc
What the developer wrote
Authored by selsta
wallet2: fix derivation handling in check_tx_proof and is_out_to_acc
What changed, and why it matters
This patch fixes two related bugs in how Monero's wallet checks whether a transaction output belongs to your account and how it verifies transaction proofs. The bugs involve using an uninitialized or 'null' key derivation value, which could lead to incorrect matching of outputs or failed proof verification. In the worst case, a wallet might wrongly decide an output belongs to someone else, or an attacker might craft a proof that passes when it should not. The patch adds explicit null-derivation checks and ensures additional derivations are only accessed when they exist.
Apply the patch. Review callers of `is_out_to_acc` and `check_tx_proof` to confirm no other paths pass null or uninitialized derivations. Consider adding unit tests for null-derivation and out-of-bounds additional-derivation cases. Monitor Monero Project advisories for any follow-up security disclosure.
Security signals we found
Use of uninitialized key derivation in `check_tx_proof`
Use of null/zero key derivation in `is_out_to_acc` without validation
Potential out-of-bounds read in `additional_derivations[output_index]`
Incorrect output ownership classification could affect balance detection or proof verification
Patch is defensive and partial; no explicit CVE or advisory supplied
Evidence from the diff
In wallet2::is_out_to_acc, the code now checks whether the primary derivation is a null (all-zero) derivation before calling out_can_be_to_acc, and similarly checks each additional_derivation for null before use. It also replaces the !additional_derivations.empty() guard with an output_index < additional_derivations.size() bounds check, preventing out-of-bounds access when the vector is non-empty but shorter than the output index. In wallet2::check_tx_proof, the local derivation variable is now value-initialized to zero (crypto::key_derivation derivation{};) instead of being left uninitialized. These changes prevent use of invalid/zero/undefined derivations in key derivation and comparison operations.
Changed components
src/wallet/wallet2.cppwallet2::is_out_to_accwallet2::check_tx_proofcryptonote key derivation handlingInspect captured patch +18 / −9
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 2746a5b..a81c4ef 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -12182,8 +12182,14 @@ bool wallet2::is_out_to_acc(const cryptonote::account_public_address &address, c
crypto::public_key derived_out_key;
bool found = false;
bool r;
+
+ const auto is_null_derivation = [](const crypto::key_derivation &d) {
+ static const crypto::key_derivation null_derivation{};
+ return memcmp(&d, &null_derivation, sizeof(d)) == 0;
+ };
+
// first run quick check if output has matching view tag, otherwise output should not belong to account
- if (out_can_be_to_acc(view_tag_opt, derivation, output_index))
+ if (!is_null_derivation(derivation) && out_can_be_to_acc(view_tag_opt, derivation, output_index))
{
// if view tag match, run slower check deriving output pub key and comparing to expected
r = crypto::derive_public_key(derivation, output_index, address.m_spend_public_key, derived_out_key);
@@ -12195,17 +12201,20 @@ bool wallet2::is_out_to_acc(const cryptonote::account_public_address &address, c
}
}
- if (!found && !additional_derivations.empty())
+ if (!found && output_index < additional_derivations.size())
{
const crypto::key_derivation &additional_derivation = additional_derivations[output_index];
- if (out_can_be_to_acc(view_tag_opt, additional_derivation, output_index))
+ if (!is_null_derivation(additional_derivation))
{
- r = crypto::derive_public_key(additional_derivation, output_index, address.m_spend_public_key, derived_out_key);
- THROW_WALLET_EXCEPTION_IF(!r, error::wallet_internal_error, "Failed to derive public key");
- if (out_key == derived_out_key)
+ if (out_can_be_to_acc(view_tag_opt, additional_derivation, output_index))
{
- found = true;
- found_derivation = additional_derivation;
+ r = crypto::derive_public_key(additional_derivation, output_index, address.m_spend_public_key, derived_out_key);
+ THROW_WALLET_EXCEPTION_IF(!r, error::wallet_internal_error, "Failed to derive public key");
+ if (out_key == derived_out_key)
+ {
+ found = true;
+ found_derivation = additional_derivation;
+ }
}
}
}
@@ -12482,7 +12491,7 @@ bool wallet2::check_tx_proof(const cryptonote::transaction &tx, const cryptonote
if (std::any_of(good_signature.begin(), good_signature.end(), [](int i) { return i > 0; }))
{
// obtain key derivation by multiplying scalar 1 to the shared secret
- crypto::key_derivation derivation;
+ crypto::key_derivation derivation{};
if (good_signature[0])
THROW_WALLET_EXCEPTION_IF(!crypto::generate_key_derivation(shared_secret[0], rct::rct2sk(rct::I), derivation), error::wallet_internal_error, "Failed to generate key derivation");
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.