cryptonote_basic: keep additional derivations aligned in key image helper
What the developer wrote
Authored by Cole Munz
cryptonote_basic: keep additional derivations aligned in key image helper
generate_key_image_helper only appended to additional_recv_derivations when
generate_key_derivation succeeded. A tx pubkey that is not a valid point makes
that call fail, so every later derivation shifted down one slot.
is_out_to_acc_precomp indexes that vector by the output index, so once the
list is short the lookup either falls off the bounds check or reads the
derivation belonging to a different output. Either way the helper reports that
the output does not belong to the address, and the wallet cannot build a key
image for an output it owns. Anyone who can put a transaction in front of the
wallet chooses those pubkeys.
The main tx pubkey a few lines up already handles a failed derivation by
keeping its slot and padding with identity, and wallet2 does the same in the
three places it builds this list (wallet2.cpp:2393, :7887, :13397). Do it here
too.
What changed, and why it matters
This patch fixes a bug in how Monero wallets build key images for transaction outputs they own. When a transaction contained an invalid extra public key, the wallet would misalign its internal list of cryptographic derivations. This caused the wallet to either look at the wrong entry or fail to recognize its own output, preventing it from creating a key image. Because anyone can craft a transaction with such an invalid key, this could be used to stop a wallet from spending its own funds. The fix pads the failed derivation with a placeholder so the list stays aligned with output indexes.
Apply the patch and run the included unit test. Review other call sites that build additional_recv_derivations to ensure consistent alignment behavior. Consider adding fuzzing or validation for malformed additional_tx_public_keys in transaction parsing.
Security signals we found
Off-by-one / index misalignment in cryptographic derivation list
Missing placeholder for failed key derivation causing vector desynchronization
Denial-of-spend: wallet cannot construct key image for owned output
Attacker-controlled transaction public keys trigger the failure
Inconsistent handling between main tx pubkey and additional tx pubkeys
Evidence from the diff
In generate_key_image_helper, additional_recv_derivations was only appended when generate_key_derivation succeeded for each additional_tx_public_keys entry. If an additional tx pubkey was not a valid curve point, generate_key_derivation failed and no entry was pushed, causing subsequent derivations to shift down by one index. is_out_to_acc_precomp indexes additional_recv_derivations by real_output_index, so the lookup would either go out of bounds or return the derivation for a different output. The result: the helper returns false (output not owned) and the wallet cannot build a key image for an owned output. The patch mirrors existing handling for the main tx pubkey by padding failed additional derivations with rct::identity() and always pushing an entry. A unit test reproduces the misalignment with an invalid pubkey at index 0 and an owned output at index 1.
Changed components
src/cryptonote_basic/cryptonote_format_utils.cppgenerate_key_image_helperis_out_to_acc_precompwallet key image generationInspect captured patch +39 / −4
diff --git a/src/cryptonote_basic/cryptonote_format_utils.cpp b/src/cryptonote_basic/cryptonote_format_utils.cpp
index 09ec8ff..7be0f6a 100644
--- a/src/cryptonote_basic/cryptonote_format_utils.cpp
+++ b/src/cryptonote_basic/cryptonote_format_utils.cpp
@@ -282,11 +282,9 @@ namespace cryptonote
if (!r)
{
MWARNING("key image helper: failed to generate_key_derivation(" << additional_tx_public_keys[i] << ", <viewkey>)");
+ memcpy(&additional_recv_derivation, rct::identity().bytes, sizeof(additional_recv_derivation));
}
- else
- {
- additional_recv_derivations.push_back(additional_recv_derivation);
- }
+ additional_recv_derivations.push_back(additional_recv_derivation);
}
boost::optional<subaddress_receive_info> subaddr_recv_info = is_out_to_acc_precomp(subaddresses, out_key, recv_derivation, additional_recv_derivations, real_output_index,hwdev);
diff --git a/tests/unit_tests/cryptonote_format_utils.cpp b/tests/unit_tests/cryptonote_format_utils.cpp
index 55bc147..8e41c0e 100644
--- a/tests/unit_tests/cryptonote_format_utils.cpp
+++ b/tests/unit_tests/cryptonote_format_utils.cpp
@@ -410,3 +410,40 @@ TEST(cn_format_utils, tx_extra_merge_mining_tag_store_load)
}
}
}
+
+TEST(cn_format_utils, generate_key_image_helper_additional_derivations_stay_aligned)
+{
+ // An additional tx pubkey that is not a valid point makes generate_key_derivation()
+ // fail for that entry. The derivations must stay index-aligned with the pubkeys
+ // anyway, because is_out_to_acc_precomp() indexes them by the output index.
+ cryptonote::account_base acc;
+ acc.generate();
+ const cryptonote::account_keys &keys = acc.get_keys();
+ hw::device &hwdev = hw::get_device("default");
+
+ crypto::public_key tx_pub_key, additional_tx_pub_key;
+ crypto::secret_key tx_sec_key, additional_tx_sec_key;
+ crypto::generate_keys(tx_pub_key, tx_sec_key);
+ crypto::generate_keys(additional_tx_pub_key, additional_tx_sec_key);
+
+ // The output we own sits at index 1, derived from the additional pubkey there.
+ crypto::key_derivation derivation;
+ ASSERT_TRUE(hwdev.generate_key_derivation(additional_tx_pub_key, keys.m_view_secret_key, derivation));
+ crypto::public_key out_key;
+ ASSERT_TRUE(crypto::derive_public_key(derivation, 1, keys.m_account_address.m_spend_public_key, out_key));
+
+ std::unordered_map<crypto::public_key, cryptonote::subaddress_index> subaddresses;
+ subaddresses[keys.m_account_address.m_spend_public_key] = {0, 0};
+
+ crypto::public_key invalid_pub_key = crypto::null_pkey;
+ reinterpret_cast<unsigned char*>(&invalid_pub_key)[31] = 0x7f;
+ crypto::key_derivation unused;
+ ASSERT_FALSE(hwdev.generate_key_derivation(invalid_pub_key, keys.m_view_secret_key, unused));
+
+ const std::vector<crypto::public_key> additional_tx_pub_keys = {invalid_pub_key, additional_tx_pub_key};
+
+ cryptonote::keypair in_ephemeral;
+ crypto::key_image ki;
+ ASSERT_TRUE(cryptonote::generate_key_image_helper(keys, subaddresses, out_key, tx_pub_key,
+ additional_tx_pub_keys, 1, in_ephemeral, ki, hwdev));
+}
Why this scored 76/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.