From f4212ace98354b36e05b0c8ceb9d5197804b0c81 Mon Sep 17 00:00:00 2001 From: koe Date: Tue, 25 Aug 2026 20:01:40 -0500 Subject: [PATCH] add sanity_check_pending_tx (cherry picked from commit 4eae9740460fb69e30bedeea48c2ab3268974bc0) --- src/cryptonote_core/cryptonote_tx_utils.cpp | 53 +- src/cryptonote_core/cryptonote_tx_utils.h | 22 + src/wallet/CMakeLists.txt | 1 + src/wallet/api/wallet.cpp | 3 + src/wallet/pending_tx_validation.cpp | 780 ++++++++++++++++++++ src/wallet/pending_tx_validation.h | 80 ++ src/wallet/wallet2.cpp | 186 ++++- src/wallet/wallet2.h | 3 + tests/functional_tests/cold_signing.py | 49 +- 9 files changed, 1127 insertions(+), 50 deletions(-) create mode 100644 src/wallet/pending_tx_validation.cpp create mode 100644 src/wallet/pending_tx_validation.h diff --git a/src/cryptonote_core/cryptonote_tx_utils.cpp b/src/cryptonote_core/cryptonote_tx_utils.cpp index 425918ef2..6f2ac8813 100644 --- a/src/cryptonote_core/cryptonote_tx_utils.cpp +++ b/src/cryptonote_core/cryptonote_tx_utils.cpp @@ -48,41 +48,30 @@ using namespace epee; using namespace crypto; -namespace -{ -//--------------------------------------------------------------- -/** - * @brief check if can re-derive change address from device / keys - * @param change_addr address to attempt to re-derive - * @param subaddresses subaddress map - * @param keys account keys of sender - * @return subaddress index of `change_addr` if in the subaddress map and re-derives from device, otherwise nullopt - */ -boost::optional sanity_check_change_address( - const cryptonote::account_public_address& change_addr, - const std::unordered_map& subaddresses, - const cryptonote::account_keys &keys -) -{ - // guess/find subaddress index of `change_addr`, works for main addresses if `subaddresses` is empty - cryptonote::subaddress_index subaddr_index{}; // (0, 0) by default - const auto subaddr_it = subaddresses.find(change_addr.m_spend_public_key); - if (subaddr_it != subaddresses.cend()) - subaddr_index = subaddr_it->second; - - // if device does not return same address given index, then fail - hw::device &hwdev = keys.get_device(); - const auto recomputed_addr = hwdev.get_subaddress(keys, subaddr_index); - if (change_addr != recomputed_addr) - return boost::none; - - return boost::optional(subaddr_index); -} -//--------------------------------------------------------------- -} //anonymous namespace namespace cryptonote { + //--------------------------------------------------------------- + boost::optional sanity_check_change_address( + const cryptonote::account_public_address& change_addr, + const std::unordered_map& subaddresses, + const cryptonote::account_keys &keys + ) + { + // guess/find subaddress index of `change_addr`, works for main addresses if `subaddresses` is empty + cryptonote::subaddress_index subaddr_index{}; // (0, 0) by default + const auto subaddr_it = subaddresses.find(change_addr.m_spend_public_key); + if (subaddr_it != subaddresses.cend()) + subaddr_index = subaddr_it->second; + + // if device does not return same address given index, then fail + hw::device &hwdev = keys.get_device(); + const auto recomputed_addr = hwdev.get_subaddress(keys, subaddr_index); + if (change_addr != recomputed_addr) + return boost::none; + + return boost::optional(subaddr_index); + } //--------------------------------------------------------------- void classify_addresses(const std::vector &destinations, const boost::optional& change_addr, size_t &num_stdaddresses, size_t &num_subaddresses, account_public_address &single_dest_subaddress) { diff --git a/src/cryptonote_core/cryptonote_tx_utils.h b/src/cryptonote_core/cryptonote_tx_utils.h index 1e87968d1..f297b3ce9 100644 --- a/src/cryptonote_core/cryptonote_tx_utils.h +++ b/src/cryptonote_core/cryptonote_tx_utils.h @@ -108,6 +108,15 @@ namespace cryptonote END_SERIALIZE() }; + inline bool operator==(const tx_destination_entry &a, const tx_destination_entry &b) + { + return a.original == b.original + && a.amount == b.amount + && a.addr == b.addr + && a.is_subaddress == b.is_subaddress + && a.is_integrated == b.is_integrated; + } + //--------------------------------------------------------------- struct tx_block_template_backlog_entry @@ -118,6 +127,19 @@ namespace cryptonote }; //--------------------------------------------------------------- + /** + * @brief check if can re-derive change address from device / keys + * @param change_addr address to attempt to re-derive + * @param subaddresses subaddress map + * @param keys account keys of sender + * @return subaddress index of `change_addr` if in the subaddress map and re-derives from device, otherwise nullopt + */ + boost::optional sanity_check_change_address( + const cryptonote::account_public_address& change_addr, + const std::unordered_map& subaddresses, + const cryptonote::account_keys &keys + ); + void classify_addresses(const std::vector &destinations, const boost::optional& change_addr, size_t &num_stdaddresses, size_t &num_subaddresses, account_public_address &single_dest_subaddress); crypto::public_key get_destination_view_key_pub(const std::vector &destinations, const boost::optional& change_addr); bool construct_tx(const account_keys& sender_account_keys, std::vector &sources, const std::vector& destinations, const boost::optional& change_addr, const std::vector &extra, transaction& tx); bool construct_tx_with_tx_key(const account_keys& sender_account_keys, const std::unordered_map& subaddresses, std::vector& sources, std::vector& destinations, const boost::optional& change_addr, const std::vector &extra, transaction& tx, const crypto::secret_key &tx_key, const std::vector &additional_tx_keys, bool rct = false, const rct::RCTConfig &rct_config = { rct::RangeProofBorromean, 0 }, bool shuffle_outs = true, bool use_view_tags = false); diff --git a/src/wallet/CMakeLists.txt b/src/wallet/CMakeLists.txt index fdf3f2f5d..cfa1cb20c 100644 --- a/src/wallet/CMakeLists.txt +++ b/src/wallet/CMakeLists.txt @@ -33,6 +33,7 @@ set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib) set(wallet_sources wallet2.cpp wallet_args.cpp + pending_tx_validation.cpp ringdb.cpp node_rpc_proxy.cpp message_store.cpp diff --git a/src/wallet/api/wallet.cpp b/src/wallet/api/wallet.cpp index 7fe1b86be..a94f78e21 100644 --- a/src/wallet/api/wallet.cpp +++ b/src/wallet/api/wallet.cpp @@ -2561,6 +2561,9 @@ void WalletImpl::pendingTxPostProcess(PendingTransactionImpl * pending) tools::wallet2::signed_tx_set exported_txs; std::vector dsts_info; + // NOTE: We expect `cold_sign_tx` to validate `pending->m_pending_tx` with `sanity_check_pending_tx`. + // It is not possible to pre-validate here because the pending tx may be 'half-formed' at this point (e.g. + // trezor makes a tx proposal with no key images and the cold wallet has to supply those). m_wallet->cold_sign_tx(pending->m_pending_tx, exported_txs, dsts_info, pending->m_tx_device_aux); pending->m_key_images = exported_txs.key_images; pending->m_pending_tx = exported_txs.ptx; diff --git a/src/wallet/pending_tx_validation.cpp b/src/wallet/pending_tx_validation.cpp new file mode 100644 index 000000000..6a3e912d7 --- /dev/null +++ b/src/wallet/pending_tx_validation.cpp @@ -0,0 +1,780 @@ +// Copyright (c) 2025-2026, The Monero Project +// +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without modification, are +// permitted provided that the following conditions are met: +// +// 1. Redistributions of source code must retain the above copyright notice, this list of +// conditions and the following disclaimer. +// +// 2. Redistributions in binary form must reproduce the above copyright notice, this list +// of conditions and the following disclaimer in the documentation and/or other +// materials provided with the distribution. +// +// 3. Neither the name of the copyright holder nor the names of its contributors may be +// used to endorse or promote products derived from this software without specific +// prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY +// EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF +// MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL +// THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, +// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS +// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, +// STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF +// THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +//paired header +#include "pending_tx_validation.h" + +//local headers +#include "misc_log_ex.h" +#include "common/apply_permutation.h" +#include "crypto/crypto.h" +#include "cryptonote_basic/account.h" +#include "cryptonote_basic/cryptonote_basic_impl.h" +#include "cryptonote_basic/cryptonote_format_utils.h" +#include "cryptonote_basic/subaddress_index.h" +#include "cryptonote_core/cryptonote_tx_utils.h" +#include "cryptonote_config.h" +#include "device/device_default.hpp" +#include "ringct/rctOps.h" +#include "ringct/rctTypes.h" +#include "string_tools.h" +#include "wallet2.h" + +//third party headers +#include +#include + +//standard headers +#include +#include +#include +#include + +#undef MONERO_DEFAULT_LOG_CATEGORY +#define MONERO_DEFAULT_LOG_CATEGORY "wallet.tx_builder" + +namespace tools +{ +namespace wallet +{ +//------------------------------------------------------------------------------------------------------------------- +static void validate_tx_outs( + const wallet2::pending_tx &ptx, + const std::vector &ext_outputs, + const crypto::secret_key &k_view, + const cryptonote::tx_destination_entry &change, + const std::vector &dests, + const crypto::secret_key &tx_key, + const std::vector &additional_tx_keys, + crypto::public_key &tx_pubkey_out, + std::vector &tx_additional_pubkeys_out) +{ + tx_pubkey_out = crypto::public_key{}; + tx_additional_pubkeys_out.clear(); + CHECK_AND_ASSERT_THROW_MES(ptx.tx.vout.size() == dests.size(), + "validate_tx_outs: dests size mismatch"); + CHECK_AND_ASSERT_THROW_MES(ext_outputs.size() == dests.size(), + "validate_tx_outs: ext_outputs size mismatch"); + CHECK_AND_ASSERT_THROW_MES(ptx.tx.version == 1 || ptx.tx.rct_signatures.ecdhInfo.size() == dests.size(), + "validate_tx_outs: ecdhInfo size mismatch"); + CHECK_AND_ASSERT_THROW_MES(ptx.tx.version == 1 || ptx.tx.rct_signatures.outPk.size() == dests.size(), + "validate_tx_outs: outPk size mismatch"); + + // Check if multiple keys are expected + // Note: this is the same method used in `construct_tx_and_get_tx_key()`, so we should expect the same results. + size_t num_stdaddresses = 0; + size_t num_subaddresses = 0; + cryptonote::account_public_address single_dest_subaddress; + cryptonote::classify_addresses(dests, change.addr, num_stdaddresses, num_subaddresses, single_dest_subaddress); + const bool need_additional_txkeys = num_subaddresses > 0 && (num_stdaddresses > 0 || num_subaddresses > 1); + + const size_t expect_num_additional = need_additional_txkeys ? dests.size() : 0; + CHECK_AND_ASSERT_THROW_MES(additional_tx_keys.size() == expect_num_additional, + "validate_tx_outs: additional tx keys invalid size"); + + // if this is a single-destination transfer to a subaddress, we set the tx pubkey to R=s*D + if (num_stdaddresses == 0 && num_subaddresses == 1) + { + tx_pubkey_out = rct::rct2pk(rct::scalarmultKey( + rct::pk2rct(single_dest_subaddress.m_spend_public_key), + rct::sk2rct(tx_key) + )); + } + else + { + tx_pubkey_out = rct::rct2pk(rct::scalarmultBase(rct::sk2rct(tx_key))); + } + + // It's assumed here generate_output_ephemeral_keys only needs the private view key + cryptonote::account_keys sender_keys; + sender_keys.m_view_secret_key = k_view; + + std::vector amount_keys; + for (size_t i = 0; i < dests.size(); ++i) + { + const auto &dest = dests.at(i); + + crypto::public_key repro_onetime_addr{}; + crypto::view_tag repro_view_tag{}; + const bool r = hw::core::device_default().generate_output_ephemeral_keys( + ptx.tx.version, + sender_keys, + tx_pubkey_out, + tx_key, + dest, + change.addr, + i, + need_additional_txkeys, + additional_tx_keys, + tx_additional_pubkeys_out, + amount_keys, + repro_onetime_addr, + ptx.construction_data.use_view_tags, + repro_view_tag + ); + CHECK_AND_ASSERT_THROW_MES(r, "validate_tx_outs: failed to generate output ephemeral key"); + + // - Ko + CHECK_AND_ASSERT_THROW_MES(repro_onetime_addr == ext_outputs.at(i).key, + "validate_tx_outs: failed reproducing onetime address"); + + // - view tag + CHECK_AND_ASSERT_THROW_MES(repro_view_tag == ext_outputs.at(i).view_tag, + "validate_tx_outs: failed reproducing view tag"); + + if (ptx.tx.version == 1) + { + // - cleartext amount + CHECK_AND_ASSERT_THROW_MES(ptx.tx.vout.at(i).amount == dest.amount, + "validate_tx_outs: v1 tx output amount doesn't match destination amount"); + + continue; + } + + // - encoded amount + rct::ecdhTuple amnt_data{}; + memcpy(amnt_data.amount.bytes, &dest.amount, sizeof(dest.amount)); + rct::ecdhDecode(amnt_data, amount_keys.at(i), true); //v2, decode gives mask + CHECK_AND_ASSERT_THROW_MES(ptx.tx.rct_signatures.ecdhInfo.at(i).amount == amnt_data.amount, + "validate_tx_outs: failed reproducing encoded amount"); + + // - C + const rct::key repro_C = rct::commit(dest.amount, amnt_data.mask); + CHECK_AND_ASSERT_THROW_MES(ptx.tx.rct_signatures.outPk.at(i).mask == repro_C, + "validate_tx_outs: failed reproducing amount commitment"); + } +} +//------------------------------------------------------------------------------------------------------------------- +static void reconstruct_payment_id(const std::string &extracted_payment_id, + const boost::optional &parsed_to_check, + // const boost::optional &pre_decrypted_to_check, + const cryptonote::account_public_address &change_addr, + const std::vector &dests, + // Will be boost::none if tx_key is redacted. + const boost::optional tx_key, + const std::vector &tx_extra_fields) +{ + // Get the destination that will be able to read the payment id in the final tx. + // Returns non-null if there is exactly one destination (may or may not be the change addr). + const crypto::public_key view_key_pub = cryptonote::get_destination_view_key_pub(dests, change_addr); + CHECK_AND_ASSERT_THROW_MES(view_key_pub != crypto::null_pkey, + "reconstruct_payment_id: encrypted payment IDs must only be in txs with one destination"); + + // Extract the expected encrypted payment id. + cryptonote::tx_extra_nonce extra_nonce; + crypto::hash8 encrypted_payment_id8 = crypto::null_hash8; + CHECK_AND_ASSERT_THROW_MES(cryptonote::find_tx_extra_field_by_type(tx_extra_fields, extra_nonce), + "reconstruct_payment_id: expected payment id is missing"); + CHECK_AND_ASSERT_THROW_MES(cryptonote::get_encrypted_payment_id_from_tx_extra_nonce( + extra_nonce.nonce, + encrypted_payment_id8 + ), + "reconstruct_payment_id: expected payment id is missing"); + + // Parse the payment ID string extracted from the address. + crypto::hash8 to_encrypt_payment_id; + CHECK_AND_ASSERT_THROW_MES(tools::wallet2::parse_short_payment_id(extracted_payment_id, to_encrypt_payment_id), + "reconstruct_payment_id: failed parsing short payment id from integrated address"); + if (parsed_to_check) + { + CHECK_AND_ASSERT_THROW_MES(*parsed_to_check == to_encrypt_payment_id, + "reconstruct_payment_id: did not extract the same parsed payment id"); + } + // if (pre_decrypted_to_check) + // { + // CHECK_AND_ASSERT_THROW_MES(*pre_decrypted_to_check == to_encrypt_payment_id, + // "reconstruct_payment_id: did not extract the same pre-decrypted payment id"); + // } + + if (tx_key) + { + // Encrypt the address's payment ID. + CHECK_AND_ASSERT_THROW_MES(hw::core::device_default().encrypt_payment_id(to_encrypt_payment_id, + view_key_pub, *tx_key), + "reconstruct_payment_id: failed encrypting payment id"); + + // Check equivalence. + CHECK_AND_ASSERT_THROW_MES(to_encrypt_payment_id == encrypted_payment_id8, + "reconstruct_payment_id: failed encrypting payment id"); + } +} +//------------------------------------------------------------------------------------------------------------------- +void sanity_check_pending_tx(const wallet2::pending_tx &ptx, + const cryptonote::network_type nettype, + const cryptonote::account_keys &account_keys, + const std::unordered_map &subaddresses, + const std::vector &transfers, + const bool redacted, + const boost::optional> &transfer_ki_resolver, + const bool allow_read_only) +{ + const auto &construct = ptx.construction_data; + const bool transfer_ki_accessible = transfer_ki_resolver.has_value(); + + // Extract outputs + std::vector ext_outputs; + const bool all_are_txout_to_tagged_key = std::all_of(ptx.tx.vout.begin(), ptx.tx.vout.end(), + [&](const cryptonote::tx_out& s_e) -> bool + { + CHECKED_GET_SPECIFIC_VARIANT(s_e.target, const cryptonote::txout_to_tagged_key, out, false); + ext_outputs.push_back(out); + return true; + } + ); + CHECK_AND_ASSERT_THROW_MES(all_are_txout_to_tagged_key, + "sanity_check_pending_tx: all outputs should have view tags"); + CHECK_AND_ASSERT_THROW_MES(construct.use_view_tags, + "sanity_check_pending_tx: must be view-tag-enabled (hardfork >= 15)"); + CHECK_AND_ASSERT_THROW_MES(construct.splitted_dsts.size() == ptx.tx.vout.size(), + "sanity_check_pending_tx: destination vecs are inconsistent"); + CHECK_AND_ASSERT_THROW_MES(ptx.tx.unlock_time == 0 && construct.unlock_time == 0, + "sanity_check_pending_tx: unlock_time must be zero"); + + // Extra + std::vector tx_extra_fields; + CHECK_AND_ASSERT_THROW_MES(cryptonote::parse_tx_extra(ptx.tx.extra, tx_extra_fields), + "sanity_check_pending_tx: ptx.tx.extra extraction failure"); + // NOTE: Due to upstream chaos, we are unable to reliably validate `construct.extra`. + // if (!cleartext_payment_id) + // { + // CHECK_AND_ASSERT_THROW_MES(construct.extra == ptx.tx.extra, + // "sanity_check_pending_tx: tx_extra mismatch"); + // } + // else + // { + // std::vector extra_clone_unencrypted = construct.extra; + // std::vector extra_clone_encrypted = ptx.tx.extra; + // cryptonote::remove_field_from_tx_extra(extra_clone_unencrypted, typeid(cryptonote::tx_extra_nonce)); + // cryptonote::remove_field_from_tx_extra(extra_clone_encrypted, typeid(cryptonote::tx_extra_nonce)); + // CHECK_AND_ASSERT_THROW_MES(extra_clone_unencrypted == extra_clone_encrypted, + // "sanity_check_pending_tx: tx_extra mismatch w/ payment id removal"); + + // // The decrypted payment ID will be checked manually later. + // } + + if (!redacted) + { + crypto::public_key reconstruct_pubkey; + std::vector reconstruct_additional_pubkeys; + + validate_tx_outs( + ptx, + ext_outputs, + account_keys.m_view_secret_key, + construct.change_dts, + construct.splitted_dsts, + ptx.tx_key, + ptx.additional_tx_keys, + reconstruct_pubkey, + reconstruct_additional_pubkeys + ); + + cryptonote::tx_extra_pub_key tx_extra_pub_key; + CHECK_AND_ASSERT_THROW_MES(cryptonote::find_tx_extra_field_by_type(tx_extra_fields, tx_extra_pub_key), + "sanity_check_pending_tx: tx_extra missing tx pub key"); + CHECK_AND_ASSERT_THROW_MES(tx_extra_pub_key.pub_key == reconstruct_pubkey, + "sanity_check_pending_tx: tx_extra unable to reproduce tx pubkey"); + if (reconstruct_additional_pubkeys.size()) + { + cryptonote::tx_extra_additional_pub_keys tx_extra_additional_pub_keys; + CHECK_AND_ASSERT_THROW_MES(cryptonote::find_tx_extra_field_by_type(tx_extra_fields, tx_extra_additional_pub_keys), + "sanity_check_pending_tx: tx_extra missing extra tx pub keys"); + CHECK_AND_ASSERT_THROW_MES(tx_extra_additional_pub_keys.data.size() == reconstruct_additional_pubkeys.size(), + "sanity_check_pending_tx: tx_extra extra tx pub keys size mismatch"); + for (size_t p = 0; p < reconstruct_additional_pubkeys.size(); ++p) + { + const auto &extra_pk = tx_extra_additional_pub_keys.data.at(p); + const auto &re_pk = reconstruct_additional_pubkeys.at(p); + CHECK_AND_ASSERT_THROW_MES(extra_pk == re_pk, + "sanity_check_pending_tx: tx_extra unable to reproduce extra tx pubkey"); + } + } + } + + // Extract tx inputs + std::vector ext_key_images; + std::vector ext_input_amounts; + std::vector> ext_key_offsets; + const bool all_are_txin_to_key = std::all_of(ptx.tx.vin.begin(), ptx.tx.vin.end(), + [&](const cryptonote::txin_v& s_e) -> bool + { + CHECKED_GET_SPECIFIC_VARIANT(s_e, const cryptonote::txin_to_key, in, false); + ext_key_images.push_back(in.k_image); + ext_input_amounts.push_back(in.amount); + ext_key_offsets.push_back(in.key_offsets); + return true; + } + ); + std::string ext_key_images_str; + for (const auto &ki : ext_key_images) + { + ext_key_images_str += boost::to_string(ki) + " "; + } + CHECK_AND_ASSERT_THROW_MES(all_are_txin_to_key, + "sanity_check_pending_tx: all inputs are not txin_to_key"); + CHECK_AND_ASSERT_THROW_MES(ptx.key_images == ext_key_images_str, + "sanity_check_pending_tx: failed reconstructing key_images field"); + + const std::unordered_set ext_key_images_set(ext_key_images.begin(), ext_key_images.end()); + CHECK_AND_ASSERT_THROW_MES(ext_key_images.size() == ext_key_images_set.size(), + "sanity_check_pending_tx: duplicate key images"); + + // Inputs + // - We don't check the validity of tx_source_entry::real_out_additional_tx_keys. + CHECK_AND_ASSERT_THROW_MES(construct.sources.size() > 0, + "sanity_check_pending_tx: tx has no inputs"); + CHECK_AND_ASSERT_THROW_MES(construct.sources.size() == ptx.tx.vin.size(), + "sanity_check_pending_tx: source/vin size mismatch"); + // NOTE: due to upstream inconsistencies, we are unable to validate mixRing reliably + // if (recovered_from_serialized) + // { + // CHECK_AND_ASSERT_THROW_MES(ptx.tx.rct_signatures.mixRing.size() == 0, + // "sanity_check_pending_tx: ptx is recovered from serialized but mixRing.size() != 0"); + // } + // else + // { + // CHECK_AND_ASSERT_THROW_MES(construct.sources.size() == ptx.tx.rct_signatures.mixRing.size(), + // "sanity_check_pending_tx: source(" << construct.sources.size() << ")/mixring(" + // << ptx.tx.rct_signatures.mixRing.size() << ") size mismatch"); + // } + CHECK_AND_ASSERT_THROW_MES(construct.sources.size() == ptx.selected_transfers.size(), + "sanity_check_pending_tx: selected_transfers invalid size"); + CHECK_AND_ASSERT_THROW_MES(construct.selected_transfers == ptx.selected_transfers, + "sanity_check_pending_tx: selected_transfers inconsistent"); + // - For SOME REASON, construction data sources are not always in the same order as inputs, so we need to fix that + auto sources_ordered = construct.sources; + std::vector ins_order; + for (const size_t selected_transfer : ptx.selected_transfers) + { + CHECK_AND_ASSERT_THROW_MES(selected_transfer < transfers.size(), + "sanity_check_pending_tx: invalid transfers index"); + const auto &transfer = transfers.at(selected_transfer); + for (size_t i = 0; i < sources_ordered.size(); ++i) + { + const auto &src = sources_ordered.at(i); + CHECK_AND_ASSERT_THROW_MES(src.real_output < src.outputs.size(), + "sanity_check_pending_tx: ring sig index " << src.real_output << " out of input set size " + << src.outputs.size()); + // Check both global output index and amount since 'global output index' is actually + // 'index in global array of same-amount outputs'. + if (src.outputs[src.real_output].first != transfer.m_global_output_index + || src.amount != transfer.m_amount + || memcmp(src.outputs[src.real_output].second.dest.bytes, rct::pk2rct(transfer.get_public_key()).bytes, sizeof(rct::key)) != 0) + continue; + ins_order.push_back(i); + break; + } + } + CHECK_AND_ASSERT_THROW_MES(ins_order.size() == sources_ordered.size(), + "sanity_check_pending_tx: global index mismatch between sources and tx"); + tools::apply_permutation(ins_order, sources_ordered); + + // Similarly, tx vins may not be in the same order as the selected transfers. + // Check selected transfers key images and map to tx vins order. + // Note: if the wallet hasn't imported key images, it won't be able to line up the order. + std::vector inp_order(ptx.selected_transfers.size()); + size_t found_kis_count = 0; + CHECK_AND_ASSERT_THROW_MES(ext_key_images.size() == ptx.selected_transfers.size(), + "sanity_check_pending_tx: extracted key images size mismatch to selected transfers"); + for (size_t i = 0; i < ext_key_images.size() && transfer_ki_accessible; ++i) + { + const crypto::key_image &ki = ext_key_images[i]; + for (size_t j = 0; j < ptx.selected_transfers.size(); ++j) + { + const auto &selected_transfer = ptx.selected_transfers.at(j); + CHECK_AND_ASSERT_THROW_MES(selected_transfer < transfers.size(), + "sanity_check_pending_tx: invalid transfers index"); + if ((*transfer_ki_resolver)(selected_transfer) != ki) + continue; + inp_order.at(j) = i; + ++found_kis_count; + break; + } + } + CHECK_AND_ASSERT_THROW_MES(!transfer_ki_accessible || found_kis_count == ptx.selected_transfers.size(), + "sanity_check_pending_tx: ki mismatch between selected transfers and tx"); + + boost::multiprecision::uint128_t input_amnt = 0; + for (size_t i = 0; i < ptx.selected_transfers.size(); ++i) + { + const auto &selected_transfer = ptx.selected_transfers.at(i); + CHECK_AND_ASSERT_THROW_MES(selected_transfer < transfers.size(), + "sanity_check_pending_tx: invalid transfers index"); + + const auto &src = sources_ordered.at(i); + const auto &transfer = transfers.at(selected_transfer); + + if (ptx.tx.version == 1) + { + CHECK_AND_ASSERT_THROW_MES(!src.rct, + "sanity_check_pending_tx: rct sources not allowed for v1 txs"); + } + CHECK_AND_ASSERT_THROW_MES(src.amount == transfer.m_amount, + "sanity_check_pending_tx: transfer amount mismatch"); + CHECK_AND_ASSERT_THROW_MES(src.real_output < src.outputs.size(), + "sanity_check_pending_tx: ring sig index out of input set size"); + // `src.mask` may be uninitialized for v1 txs + const rct::key commitment = rct::commit(src.amount, ptx.tx.version > 1 ? src.mask : rct::identity()); + // non-rct inputs to > v1 txs should always have identity masks set + if (ptx.tx.version > 1 && !src.rct) + { + CHECK_AND_ASSERT_THROW_MES(src.mask == rct::identity(), + "sanity_check_pending_tx: pre-rct input in > v1 tx has non-identity mask recorded"); + } + CHECK_AND_ASSERT_THROW_MES(src.outputs[src.real_output].second.mask == commitment, + "sanity_check_pending_tx: failed reproducing real input's amount commitment"); + + // NOTE: due to upstream inconsistencies, we are unable to validate mixRing reliably. + // if (!recovered_from_serialized) + // { + // const auto &input_ring = ptx.tx.rct_signatures.mixRing.at(i); + // CHECK_AND_ASSERT_THROW_MES(input_ring.size() == src.outputs.size(), + // "sanity_check_pending_tx: input ring size mismatch"); + // for (size_t r = 0; r < input_ring.size(); ++r) + // { + // CHECK_AND_ASSERT_THROW_MES(input_ring.at(r) == src.outputs.at(r).second, + // "sanity_check_pending_tx: input ring member mismatch"); + // } + // } + + if (!allow_read_only) + { + CHECK_AND_ASSERT_THROW_MES(!transfer.m_spent, + "sanity_check_pending_tx: non-read-only transfer - is marked as spent"); + CHECK_AND_ASSERT_THROW_MES(!transfer.m_frozen, + "sanity_check_pending_tx: non-read-only transfer - is marked as frozen"); + } + CHECK_AND_ASSERT_THROW_MES(src.rct == transfer.m_rct, + "sanity_check_pending_tx: transfer - 'is rct' mismatch"); + CHECK_AND_ASSERT_THROW_MES(src.real_output_in_tx_index == transfer.m_internal_output_index, + "sanity_check_pending_tx: transfer - input's output-set index mismatch"); + CHECK_AND_ASSERT_THROW_MES(src.outputs[src.real_output].second.dest == rct::pk2rct(transfer.get_public_key()), + "sanity_check_pending_tx: transfer - onetime addr mismatch"); + + if (memcmp(transfer.m_mask.bytes, rct::identity().bytes, sizeof(rct::key)) != 0) + { + // NOTE: as of this writing wallet2::sign_tx when called by a cold wallet is the only caller + // where transfer.m_mask may be set to the placeholder identity when the mask should be + // something else, since it's the only caller that didn't scan the chain which would have set + // it to the expected value. All other callers should have transfer.m_mask equal to src.mask. + CHECK_AND_ASSERT_THROW_MES(src.mask == transfer.m_mask, + "sanity_check_pending_tx: transfer - mask mismatch"); + } + + input_amnt += src.amount; + // We need `inp_order` from here, which is only valid if key images are available. + if (!transfer_ki_accessible) + continue; + + const uint64_t ext_input_amount = ext_input_amounts.at(inp_order.at(i)); + const std::vector &ext_relative_key_offsets = ext_key_offsets.at(inp_order.at(i)); + + // Note: input amounts are cleartext for v1 outputs (even in an rct tx) because ring members are + // looked up in same-amount buckets. + CHECK_AND_ASSERT_THROW_MES(src.rct + ? ext_input_amount == 0 + : ext_input_amount == src.amount, + "sanity_check_pending_tx: amount extracted from inputs is wrong"); + + // Key offsets line up + std::vector src_absolute_key_offsets; + for (const auto &src_out : src.outputs) + src_absolute_key_offsets.push_back(src_out.first); + const std::vector ext_absolute_key_offsets = cryptonote::relative_output_offsets_to_absolute( + ext_relative_key_offsets + ); + CHECK_AND_ASSERT_THROW_MES(src_absolute_key_offsets == ext_absolute_key_offsets, + "sanity_check_pending_tx: key offsets mismatch"); + } + + CHECK_AND_ASSERT_THROW_MES(input_amnt <= UINT64_MAX, + "sanity_check_pending_tx: input amount > 2^64 - 1"); + + // Check change address + boost::optional recognized_change_index; + recognized_change_index = cryptonote::sanity_check_change_address(ptx.change_dts.addr, subaddresses, account_keys); + for (const auto &dst_entr : construct.splitted_dsts) + { + CHECK_AND_ASSERT_THROW_MES(dst_entr.amount > 0 || ptx.tx.version > 1, + "sanity_check_pending_tx: Destination with wrong amount: " << dst_entr.amount); + const bool matches_change_addr = dst_entr.addr == ptx.change_dts.addr; + const bool is_bad_change_dst = matches_change_addr && dst_entr.amount > 0 && !recognized_change_index; + CHECK_AND_ASSERT_THROW_MES(!is_bad_change_dst, + "sanity_check_pending_tx: Non-zero amount change address is not recognized as belonging to the sender account"); + } + + // Destination consistency + CHECK_AND_ASSERT_THROW_MES(construct.change_dts == ptx.change_dts, + "sanity_check_pending_tx: change_dts inconsistent"); + CHECK_AND_ASSERT_THROW_MES(construct.dests == ptx.dests, + "sanity_check_pending_tx: destination vecs are inconsistent"); + std::vector splitted_dsts_repro; + + if (ptx.tx.version == 1) + { + splitted_dsts_repro = construct.splitted_dsts; + + // In v1, 'splitted' means destinations may be split into amount digits (e.g. sweep_unmixable). + // We iter splitted dests and accumulate them, then match amounts/keys with dests + change_dest + // (which also need to be accumulated in case of duplicates). + // The map keys use `account_public_address` instead of the spendkey in case of malicous construction. + + // Accumulate splitted dests + std::unordered_map acc_split; + for (const auto &split : construct.splitted_dsts) + { + auto maybe_found = acc_split.find(split.addr); + if (maybe_found != acc_split.cend()) + { + maybe_found->second += split.amount; + } + else + { + acc_split.insert({split.addr, split.amount}); + } + } + + // Accumulate dests + std::unordered_map acc_dest; + if (ptx.change_dts.amount > 0) + acc_dest.emplace(ptx.change_dts.addr, ptx.change_dts.amount); + + for (const auto &dest : ptx.dests) + { + auto maybe_found = acc_dest.find(dest.addr); + if (maybe_found != acc_dest.cend()) + { + maybe_found->second += dest.amount; + } + else + { + acc_dest.insert({dest.addr, dest.amount}); + } + } + + // Consistency check + CHECK_AND_ASSERT_THROW_MES(acc_split.size() == acc_dest.size(), + "sanity_check_pending_tx: v1 dest inconsistency (size)"); + for (const auto &split : acc_split) + { + const auto &dest_res = acc_dest.find(split.first); + CHECK_AND_ASSERT_THROW_MES(dest_res != acc_dest.cend(), + "sanity_check_pending_tx: v1 dest inconsistency (addr matching)"); + CHECK_AND_ASSERT_THROW_MES(split.second == dest_res->second, + "sanity_check_pending_tx: v1 dest inconsistency (amount matching)"); + CHECK_AND_ASSERT_THROW_MES(split.second <= UINT64_MAX, + "sanity_check_pending_tx: v1 dest error (amount out of bounds)"); + } + } + else //(ptx.tx.version > 1) + { + splitted_dsts_repro = ptx.dests; + + if (ptx.change_dts.amount > 0 || ptx.dests.size() == 1) + { + splitted_dsts_repro.push_back(ptx.change_dts); + } + CHECK_AND_ASSERT_THROW_MES(construct.splitted_dsts.size() == splitted_dsts_repro.size(), + "sanity_check_pending_tx: failed checking splitted_dsts size"); + CHECK_AND_ASSERT_THROW_MES(ptx.tx.rct_signatures.ecdhInfo.size() == splitted_dsts_repro.size(), + "sanity_check_pending_tx: ecdhInfo size mismatch"); + CHECK_AND_ASSERT_THROW_MES(ptx.tx.rct_signatures.outPk.size() == splitted_dsts_repro.size(), + "sanity_check_pending_tx: outPk size mismatch"); + } + + boost::multiprecision::uint128_t output_amnt = 0; + size_t integrated_count = 0; + std::unordered_map dest_duplicates{}; + + for (size_t i = 0; i < construct.splitted_dsts.size(); ++i) + { + const auto &dest = construct.splitted_dsts.at(i); + + // All integrated addresses must include the original string. + CHECK_AND_ASSERT_THROW_MES(!dest.is_integrated || dest.original.size() > 0, + "sanity_check_pending_tx: integrated destination missing original address string"); + + // Check original dst string + if (dest.original.size() > 0) + { + std::string address; + std::string extracted_payment_id; + uint64_t _amount; + std::string _tx_description; + std::string _recipient_name; + std::vector _unknown_parameters; + std::string _error; + const bool parsed_uri = wallet2::parse_uri_impl(dest.original, + nettype, + address, + extracted_payment_id, + _amount, + _tx_description, + _recipient_name, + _unknown_parameters, + _error); + + if (!parsed_uri) + { + // Treat original as the address string + address = dest.original; + extracted_payment_id = ""; + } + + // The `address` string may be an explicit address OR a URI that we can't access or assess here. + cryptonote::address_parse_info parse_info; + const bool parsed_info = cryptonote::get_account_address_from_str(parse_info, nettype, address); + if (parsed_info) + { + CHECK_AND_ASSERT_THROW_MES(parse_info.is_subaddress == dest.is_subaddress, + "sanity_check_pending_tx: destination addr string mismatch - is_subaddress"); + CHECK_AND_ASSERT_THROW_MES(parse_info.has_payment_id == dest.is_integrated, + "sanity_check_pending_tx: destination addr string mismatch - is_integrated"); + CHECK_AND_ASSERT_THROW_MES(parse_info.address == dest.addr, + "sanity_check_pending_tx: destination addr string mismatch - address"); + + if (parse_info.has_payment_id && !extracted_payment_id.empty()) + { + CHECK_AND_ASSERT_THROW_MES(extracted_payment_id == epee::string_tools::pod_to_hex(parse_info.payment_id), + "sanity_check_pending_tx: destination addr string mismatch - internal payment id inconsistency"); + } + + // Even if the URI was parsed successfully, the payment ID may only be stored in the internal + // 'address' substring. + if (parse_info.has_payment_id && extracted_payment_id.empty()) + extracted_payment_id = epee::string_tools::pod_to_hex(parse_info.payment_id); + } + + if (dest.is_integrated) + { + ++integrated_count; + CHECK_AND_ASSERT_THROW_MES(integrated_count == 1, + "sanity_check_pending_tx: more than one integrated address detected"); + + // NOTE: Due to upstream chaos, we are unable to reliably validate the construction data's payment ID. + // `tx_construction_data::extra` is *sometimes* pre-decrypted, so we handle it here by extracting and + // comparing directly with our decrypted version. + // boost::optional decrypted_payment_id8 = boost::none; + // if (cleartext_payment_id) + // { + // std::vector construct_tx_extra_fields; + // CHECK_AND_ASSERT_THROW_MES(cryptonote::parse_tx_extra(construct.extra, construct_tx_extra_fields), + // "sanity_check_pending_tx: construct.extra extraction failure"); + + // cryptonote::tx_extra_nonce extra_nonce; + // CHECK_AND_ASSERT_THROW_MES(cryptonote::find_tx_extra_field_by_type(construct_tx_extra_fields, + // extra_nonce), + // "reconstruct_payment_id: expected decrypted payment id is missing"); + // CHECK_AND_ASSERT_THROW_MES(cryptonote::get_encrypted_payment_id_from_tx_extra_nonce( + // extra_nonce.nonce, + // *decrypted_payment_id8 + // ), + // "reconstruct_payment_id: expected decrypted payment id is missing"); + // } + + if (!extracted_payment_id.empty()) + { + reconstruct_payment_id(extracted_payment_id, + parsed_info ? boost::optional{parse_info.payment_id} : boost::optional(), + // decrypted_payment_id8, + ptx.change_dts.addr, + construct.splitted_dsts, + redacted ? boost::optional() : boost::optional{ptx.tx_key}, + tx_extra_fields); + } + } + } + + // Check reproduced dests + const auto &it = std::find_if(splitted_dsts_repro.cbegin(), splitted_dsts_repro.cend(), + [&dest](const auto &a) { + return a == dest; + } + ); + CHECK_AND_ASSERT_THROW_MES(it != splitted_dsts_repro.cend(), + "sanity_check_pending_tx: failed checking splitted_dsts consistency"); + + // Check that addresses keys are canonical. + // Without this check, the following duplicate address check may be weakened. + CHECK_AND_ASSERT_THROW_MES(rct::isInMainSubgroup(rct::pk2rct(dest.addr.m_spend_public_key)), + "sanity_check_pending_tx: destination spendkey is not in the main subgroup (suspicious!)"); + CHECK_AND_ASSERT_THROW_MES(rct::isInMainSubgroup(rct::pk2rct(dest.addr.m_view_public_key)), + "sanity_check_pending_tx: destination viewkey is not in the main subgroup (suspicious!)"); + + // Check that duplicate addresses are all either subaddresses or normal addresses (ignoring is_integrated). + const auto &dup = dest_duplicates.find(dest.addr.m_spend_public_key); + if (dup != dest_duplicates.cend()) + { + CHECK_AND_ASSERT_THROW_MES(dup->second.addr == dest.addr, + "sanity_check_pending_tx: destinations have duplicate spend keys but not duplicate view keys"); + CHECK_AND_ASSERT_THROW_MES(dup->second.is_subaddress == dest.is_subaddress, + "sanity_check_pending_tx: duplicate destinations do not have matching subaddress designations"); + // NOTE: only one destination may have an integrated address, so we do not require duplicate destinations to + // have the same is_integrated designation. + } + else + { + dest_duplicates[dest.addr.m_spend_public_key] = dest; + } + + // Carefully erase found copies one by one in case of duplicates. + splitted_dsts_repro.erase(it); + + // Double-check v1 amounts (normally checked in the tx-out checker if not redacted) + if (ptx.tx.version == 1) + { + CHECK_AND_ASSERT_THROW_MES(ptx.tx.vout.at(i).amount == dest.amount, + "sanity_check_pending_tx: v1 tx output amount doesn't match destination amount"); + } + + output_amnt += dest.amount; + } + CHECK_AND_ASSERT_THROW_MES(splitted_dsts_repro.size() == 0, + "sanity_check_pending_tx: failed checking splitted_dsts consistency"); + + // Balance check + output_amnt += ptx.fee; + + if (ptx.tx.version > 1) + { + CHECK_AND_ASSERT_THROW_MES(ptx.tx.rct_signatures.txnFee == ptx.fee, + "sanity_check_pending_tx: inconsistent fee for tx.v > 1 transaction"); + } + if (ptx.dust_added_to_fee) + { + CHECK_AND_ASSERT_THROW_MES(ptx.dust <= ptx.fee, + "sanity_check_pending_tx: invalid dust amount when dust added to fee"); + } + CHECK_AND_ASSERT_THROW_MES(output_amnt <= UINT64_MAX, + "sanity_check_pending_tx: output amount > 2^64 - 1"); + CHECK_AND_ASSERT_THROW_MES(output_amnt == input_amnt, + "sanity_check_pending_tx: output amount != input amount"); +} +//------------------------------------------------------------------------------------------------------------------- +} //namespace wallet +} //namespace tools diff --git a/src/wallet/pending_tx_validation.h b/src/wallet/pending_tx_validation.h new file mode 100644 index 000000000..77ba4018c --- /dev/null +++ b/src/wallet/pending_tx_validation.h @@ -0,0 +1,80 @@ +// Copyright (c) 2025-2026, The Monero Project +// +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without modification, are +// permitted provided that the following conditions are met: +// +// 1. Redistributions of source code must retain the above copyright notice, this list of +// conditions and the following disclaimer. +// +// 2. Redistributions in binary form must reproduce the above copyright notice, this list +// of conditions and the following disclaimer in the documentation and/or other +// materials provided with the distribution. +// +// 3. Neither the name of the copyright holder nor the names of its contributors may be +// used to endorse or promote products derived from this software without specific +// prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY +// EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF +// MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL +// THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, +// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS +// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, +// STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF +// THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +#pragma once + +//local headers +#include "crypto/crypto.h" +#include "cryptonote_basic/account.h" +#include "cryptonote_basic/subaddress_index.h" +#include "cryptonote_config.h" +#include "wallet2.h" + +//third party headers + +//standard headers +#include +#include +#include +#include + +namespace tools +{ +namespace wallet +{ +/** + * brief: sanity_check_pending_tx - validate `pending_tx` consistency with itself and with with `transfer_details` + * Assumes `ptx` version is >= v16. + * WARNING: Unable to verify destination addresses that are URLs and not explicit addresses. + * WARNING: Due to upstream chaos, we are unable to reliably validate that `tx_construction_data::extra` matches + * with `ptx.tx.tx_extra`. + * NOTE: Due to upstream inconsistencies, we are unable to validate mixRing reliably (it is not serialized in + * final txs). + * param: ptx - the pending_tx to validate + * param: nettype - the network that will receive the tx (e.g. mainnet/stressnet/testnet) + * param: account_keys - the keys of the tx author (only the private view key and base address are needed) + * param: subaddresses - the tx author's subaddress map + * param: transfers - transfer_details from inside `wallet2` (required because `ptx` includes transfer references) + * param: redacted - if `true` then: + * - We assume `ptx.tx_key` and `ptx.additional_tx_keys` are nullified. + * param: transfer_ki_resolver - if not-std::nullopt then this will be used to access `transfers` key images to + verify key images line up to wallets' imported key images + callback: const crypto::key_image _your_lambda_(const size_t transfer_idx) + - The callback should panic if `transfer_idx` is invalid. + * param: allow_read_only - if `true` then `ptx` transfers may be already-spent or frozen + */ +void sanity_check_pending_tx(const wallet2::pending_tx &ptx, + const cryptonote::network_type nettype, + const cryptonote::account_keys &account_keys, + const std::unordered_map &subaddresses, + const std::vector &transfers, + const bool redacted, + const boost::optional> &transfer_ki_resolver, + const bool allow_read_only); +} //namespace wallet +} //namespace tools diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 93c220983..2038e259d 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -92,6 +92,7 @@ using namespace epee; #include "device/device_cold.hpp" #include "device_trezor/device_trezor.hpp" #include "net/socks_connect.h" +#include "pending_tx_validation.h" extern "C" { @@ -7265,6 +7266,43 @@ void wallet2::get_unconfirmed_payments(std::list> transfer_ki_resolver, + const bool allow_read_only) const +{ + if (expect_imported_key_images) + { + // NOTE: Update this code if there is a usecase for checking both m_transfers and using a custom resolver. + CHECK_AND_ASSERT_THROW_MES(!transfer_ki_resolver, + "sanity_check_pending_tx (wallet2): expected imported key images but a ki resolver was provided"); + + const std::function temp = + [this](const size_t i) + { + CHECK_AND_ASSERT_THROW_MES(i < m_transfers.size(), + "sanity_check_pending_tx (wallet2): transfer - selected transfer idx out of known transfers"); + const auto &transfer = m_transfers.at(i); + CHECK_AND_ASSERT_THROW_MES(transfer.m_key_image_known, + "sanity_check_pending_tx (wallet2): transfer - KI is expected but unknown"); + return transfer.m_key_image; + }; + transfer_ki_resolver = temp; + } + + wallet::sanity_check_pending_tx(ptx, + this->nettype(), + m_account.get_keys(), + m_subaddresses, + m_transfers, + redacted, + transfer_ki_resolver, + allow_read_only); +} +//---------------------------------------------------------------------------------------------------- void wallet2::rescan_spent() { // This is RPC call that can take a long time if there are many outputs, @@ -7986,6 +8024,18 @@ bool wallet2::sign_tx(unsigned_tx_set &exported_txs, std::vector= m_transfers.size(), error::wallet_internal_error, + "Cold wallet signing: No record of output " + std::to_string(idx) + " in this wallet, run " + "`export_outputs all` on the online wallet and `import_outputs` here."); + } + this->sanity_check_pending_tx(ptx, false, true, boost::none, false); + } + return true; } //---------------------------------------------------------------------------------------------------- @@ -8067,7 +8117,7 @@ bool wallet2::load_tx(const std::string &signed_filename, std::vectorparse_tx_from_str(s, ptx, accept_func); } //---------------------------------------------------------------------------------------------------- bool wallet2::parse_tx_from_str(const std::string &signed_tx_st, std::vector &ptx, std::function accept_func) @@ -8155,6 +8205,36 @@ bool wallet2::parse_tx_from_str(const std::string &signed_tx_st, std::vectorsanity_check_pending_tx(ptx, + true, + false, + boost::optional>{ [&kis](const size_t i) { + CHECK_AND_ASSERT_THROW_MES(i < kis.size(), "failed loading signed tx: ptx selected transfer " + "is outside the bounds of imported key images"); + return kis.at(i); + } }, + true); + } + } + catch (const std::exception &e) + { + LOG_PRINT_L0("Failed to validate signed transaction: " << e.what()); + return false; + } + + // print result for user LOG_PRINT_L0("Loaded signed tx data from binary: " << signed_txs.ptx.size() << " transactions"); for (auto &c_ptx: signed_txs.ptx) LOG_PRINT_L0(cryptonote::obj_to_json_str(c_ptx.tx)); @@ -8165,13 +8245,25 @@ bool wallet2::parse_tx_from_str(const std::string &signed_tx_st, std::vectorimport_key_images(signed_txs.key_images); if (!r) return false; // remember key images for this tx, for when we get those txes from the blockchain for (const auto &e: signed_txs.tx_key_images) m_cold_key_images.insert(e); + try + { + // extra/redundant validation making sure key images line up + for (const auto &ptx : signed_txs.ptx) + this->sanity_check_pending_tx(ptx, true, true, boost::none, true); + } + catch (const std::exception &e) + { + LOG_PRINT_L0("Failed to validate signed transaction (post import): " << e.what()); + return false; + } + ptx = signed_txs.ptx; return true; @@ -8226,6 +8318,9 @@ bool wallet2::save_multisig_tx(const multisig_tx_set &txs, const std::string &fi //---------------------------------------------------------------------------------------------------- wallet2::multisig_tx_set wallet2::make_multisig_tx_set(const std::vector& ptx_vector) const { + for (const auto &ptx : ptx_vector) + this->sanity_check_pending_tx(ptx, false, true, boost::none, false); + multisig_tx_set txs; txs.m_ptx = ptx_vector; @@ -8296,18 +8391,21 @@ bool wallet2::parse_multisig_tx_from_str(std::string multisig_tx_st, multisig_tx return false; } - // sanity checks - for (const auto &ptx: exported_txs.m_ptx) + try { - CHECK_AND_ASSERT_MES(ptx.selected_transfers.size() == ptx.tx.vin.size(), false, "Mismatched selected_transfers/vin sizes"); - for (size_t idx: ptx.selected_transfers) - CHECK_AND_ASSERT_MES(idx < m_transfers.size(), false, "Transfer index out of range"); - CHECK_AND_ASSERT_MES(ptx.construction_data.selected_transfers.size() == ptx.tx.vin.size(), false, "Mismatched cd selected_transfers/vin sizes"); - for (size_t idx: ptx.construction_data.selected_transfers) - CHECK_AND_ASSERT_MES(idx < m_transfers.size(), false, "Transfer index out of range"); - CHECK_AND_ASSERT_MES(ptx.construction_data.sources.size() == ptx.tx.vin.size(), false, "Mismatched sources/vin sizes"); - CHECK_AND_ASSERT_MES(!ptx.tx.vin.empty(), false, "Multisig tx has no inputs"); - CHECK_AND_ASSERT_MES(!ptx.construction_data.sources.empty(), false, "Multisig tx has no sources"); + // sanity checks + for (const auto &ptx: exported_txs.m_ptx) + { + // If key images have not been imported then this could fail if we check key images. We'd rather fail elsewhere + // with a better error message. + // Note: These may be READ-ONLY, so spent/frozen inputs are allowed. + this->sanity_check_pending_tx(ptx, false, false, boost::none, true); + } + } + catch (const std::exception &e) + { + LOG_PRINT_L0("Failed to validate multisig tx data: " << e.what()); + return false; } return true; @@ -8390,14 +8488,33 @@ bool wallet2::sign_multisig_tx(multisig_tx_set &exported_txs_inout, std::vector< THROW_WALLET_EXCEPTION_IF(frozen(exported_txs), error::wallet_internal_error, "Will not sign multisig tx containing frozen outputs") + // We only sign if key images have been imported to ensure: + // A) signatures can only be extracted from us if our key images are valid (the sanity checker will make sure + // the tx has the same key images as in our m_transfers, and a valid tx signature will mean key images are valid) + // B) signatures involving honest signers will always have imported key images, so at least one honest signer + // will be able to detect spends from such txs + // NOTE: This goes outside the main loop to harden against a theoretical synchronization issue where this triggers + // after we get half-way through partial signing, causing our next import to run into 'export needed' because + // the previous attempt had cleared some private nonces. At this time exports/imports are atomic so that bug can't + // occur. + for (size_t n = 0; n < exported_txs.m_ptx.size(); ++n) + { + for (const size_t idx : exported_txs.m_ptx[n].construction_data.selected_transfers) + { + if (idx >= m_transfers.size() || !m_transfers[idx].m_key_image_known) + THROW_WALLET_EXCEPTION(error::multisig_import_needed); + } + } + // The 'exported_txs' contains a set of different transactions for the multisig group to try to sign. Each of those // transactions has a set of 'signing attempts' corresponding to all the possible signing groups within the multisig. // - Here, we will partially sign as many of those signing attempts as possible, for each proposed transaction. for (size_t n = 0; n < exported_txs.m_ptx.size(); ++n) { tools::wallet2::pending_tx &ptx = exported_txs.m_ptx[n]; - THROW_WALLET_EXCEPTION_IF(ptx.multisig_sigs.empty(), error::wallet_internal_error, "No signatures found in multisig tx"); const tools::wallet2::tx_construction_data &sd = ptx.construction_data; + THROW_WALLET_EXCEPTION_IF(ptx.multisig_sigs.empty(), error::wallet_internal_error, "No signatures found in multisig tx"); + LOG_PRINT_L1(" " << (n+1) << ": " << sd.sources.size() << " inputs, ring size " << (sd.sources[0].outputs.size()) << ", signed by " << exported_txs.m_signers.size() << "/" << m_multisig_threshold); @@ -8425,6 +8542,11 @@ bool wallet2::sign_multisig_tx(multisig_tx_set &exported_txs_inout, std::vector< "error: multisig::signing::tx_builder_ringct_t::init" ); + // Fully validate + // NOTE: This must occur after `tx_builder_ringct_t::init` because that function edits `ptx.tx` in-place. + // Spaghetti is as spaghetti does. + this->sanity_check_pending_tx(ptx, false, true, boost::none, false); + // go through each signing attempt for this transaction (each signing attempt corresponds to some subgroup of signers // of size 'threshold') for (auto &sig: ptx.multisig_sigs) @@ -12194,6 +12316,18 @@ void wallet2::cold_sign_tx(const std::vector& ptx_vector, signed_tx_ tx_device_aux = aux_data.tx_device_aux; MDEBUG("Signed tx data from hw: " << exported_txs.ptx.size() << " transactions"); + + // Double-check final values. + // Cold wallets are not expected to have a complete store of key images. + // Cold devices (e.g. trezor) may or may not redact outputs, so we set `redact = true`. + // TODO: Redacting means we can't fully validate ptx and that we must trust the cold wallet. + // For robustness it would be better for both devices to distrust each other. Note that all redacted + // info is left in plaintext in the `pending_tx` construction data, so it's unclear *why* anything + // is redacted in the first place. + for (const auto &ptx : exported_txs.ptx) + this->sanity_check_pending_tx(ptx, true, false, boost::none, false); + + // Print for (auto &c_ptx: exported_txs.ptx) LOG_PRINT_L0(cryptonote::obj_to_json_str(c_ptx.tx)); } //---------------------------------------------------------------------------------------------------- @@ -15688,7 +15822,27 @@ std::string wallet2::make_uri(const std::string &address, const std::string &pay return uri; } //---------------------------------------------------------------------------------------------------- -bool wallet2::parse_uri(const std::string &uri, std::string &address, std::string &payment_id, uint64_t &amount, std::string &tx_description, std::string &recipient_name, std::vector &unknown_parameters, std::string &error) +bool wallet2::parse_uri(const std::string &uri, + std::string &address, + std::string &payment_id, + uint64_t &amount, + std::string &tx_description, + std::string &recipient_name, + std::vector &unknown_parameters, + std::string &error) +{ + return wallet2::parse_uri_impl(uri, + this->nettype(), + address, + payment_id, + amount, + tx_description, + recipient_name, + unknown_parameters, + error); +} +//---------------------------------------------------------------------------------------------------- +bool wallet2::parse_uri_impl(const std::string &uri, const cryptonote::network_type nettype, std::string &address, std::string &payment_id, uint64_t &amount, std::string &tx_description, std::string &recipient_name, std::vector &unknown_parameters, std::string &error) { if (uri.substr(0, 8) != "wownero:") { @@ -15701,7 +15855,7 @@ bool wallet2::parse_uri(const std::string &uri, std::string &address, std::strin address = ptr ? remainder.substr(0, ptr-remainder.c_str()) : remainder; cryptonote::address_parse_info info; - if(!get_account_address_from_str(info, nettype(), address)) + if(!get_account_address_from_str(info, nettype, address)) { error = std::string("URI has wrong address: ") + address; return false; diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h index 71f319a49..e06b43ec5 100644 --- a/src/wallet/wallet2.h +++ b/src/wallet/wallet2.h @@ -729,6 +729,7 @@ private: struct signed_tx_set { std::vector ptx; + // All key images in `m_transfers` from the signing wallet. Does not include key images in `ptx`. std::vector key_images; serializable_unordered_map tx_key_images; @@ -1228,6 +1229,7 @@ private: uint64_t min_height, uint64_t max_height = (uint64_t)-1, const boost::optional& subaddr_account = boost::none, const std::set& subaddr_indices = {}) const; void get_unconfirmed_payments_out(std::list>& unconfirmed_payments, const boost::optional& subaddr_account = boost::none, const std::set& subaddr_indices = {}) const; void get_unconfirmed_payments(std::list>& unconfirmed_payments, const boost::optional& subaddr_account = boost::none, const std::set& subaddr_indices = {}) const; + void sanity_check_pending_tx(const wallet2::pending_tx &ptx, const bool redacted, const bool expect_imported_key_images, boost::optional> transfer_ki_resolver, const bool allow_read_only) const; uint64_t get_blockchain_current_height() const { return m_light_wallet_blockchain_height ? m_light_wallet_blockchain_height : m_blockchain.size(); } void rescan_spent(); @@ -1664,6 +1666,7 @@ private: std::string make_uri(const std::string &address, const std::string &payment_id, uint64_t amount, const std::string &tx_description, const std::string &recipient_name, std::string &error) const; bool parse_uri(const std::string &uri, std::string &address, std::string &payment_id, uint64_t &amount, std::string &tx_description, std::string &recipient_name, std::vector &unknown_parameters, std::string &error); + static bool parse_uri_impl(const std::string &uri, const cryptonote::network_type, std::string &address, std::string &payment_id, uint64_t &amount, std::string &tx_description, std::string &recipient_name, std::vector &unknown_parameters, std::string &error); uint64_t get_blockchain_height_by_date(uint16_t year, uint8_t month, uint8_t day); // 1<=month<=12, 1<=day<=31 diff --git a/tests/functional_tests/cold_signing.py b/tests/functional_tests/cold_signing.py index 40bcac603..84315e6bf 100755 --- a/tests/functional_tests/cold_signing.py +++ b/tests/functional_tests/cold_signing.py @@ -39,6 +39,7 @@ import random SEED = 'velvet lymph giddy number token physics poetry unquoted nibs useful sabotage limits benches lifestyle eden nitrogen anvil fewest avoid batch vials washing fences goat unquoted' STANDARD_ADDRESS = '42ey1afDFnn4886T7196doS9GPMzexD9gXpsZJDwVjeRVdFCSoHnv7KPbBeGpzJBzHRCAs9UxqeoyFQMYbqSWYTfJJQAWDm' SUBADDRESS = '84QRUYawRNrU3NN1VpFRndSukeyEb3Xpv8qZjjsoJZnTYpDYceuUTpog13D7qPxpviS7J29bSgSkR11hFFoXWk2yNdsR9WF' +SUBADDRESS2 = '85M4M1RVRcoEeC8sdSxN1ef6GhQYChSfKPWkB4FLKYJiSWuMXXT4Ewv8BHCRzSJB4ZYvXUcFxN4DPcVu6uwoPNRvQ1QwaXB' class ColdSigningTest(): def run_test(self): @@ -50,6 +51,8 @@ class ColdSigningTest(): for piecemeal_output_export in [False, True]: self.self_transfer_to_subaddress(piecemeal_output_export) self.transfer_after_empty_export_import() + self.transfer_to_multi_subaddresses() + self.sweep() def reset(self): print('Resetting blockchain') @@ -88,12 +91,12 @@ class ColdSigningTest(): assert self.cold_wallet.get_address().address == self.hot_wallet.get_address().address assert self.cold_wallet.get_address().address == STANDARD_ADDRESS - def mine(self): + def mine(self, n_blocks = 80): print("Mining some blocks") daemon = Daemon() wallet = Wallet() - daemon.generateblocks(STANDARD_ADDRESS, 80) + daemon.generateblocks(STANDARD_ADDRESS, n_blocks) wallet.refresh() def export_import(self, piecemeal_output_export): @@ -244,6 +247,48 @@ class ColdSigningTest(): self.create_tx(STANDARD_ADDRESS, False) assert start_len == len(self.hot_wallet.get_transfers()['in']) - 1 + def sign_and_submit(self, unsigned_txset): + print("Signing transaction with cold wallet") + res = self.cold_wallet.sign_transfer(unsigned_txset) + + print("Submitting transaction with hot wallet") + res = self.hot_wallet.submit_transfer(res.signed_txset) + assert len(res.tx_hash_list) > 0 + tx_hash_list = res.tx_hash_list + + # Make sure it ends up in the chain + daemon = Daemon() + daemon.generateblocks(STANDARD_ADDRESS, 1) + self.hot_wallet.refresh() + + res = self.hot_wallet.get_transfers() + def in_hash_list(txid, tx_hash_list): + return len([x for x in tx_hash_list if x == txid]) > 0 + assert len([x for x in (res['pending'] if 'pending' in res else []) if in_hash_list(x.txid, tx_hash_list)]) == 0 + assert len([x for x in (res['out'] if 'out' in res else []) if in_hash_list(x.txid, tx_hash_list)]) > 0 + + def transfer_to_multi_subaddresses(self): + # This test triggers the non-standard additional keys case + # const bool need_additional_txkeys = num_subaddresses > 0 && (num_stdaddresses > 0 || num_subaddresses > 1); + print("Transfer to 2 subaddresses in 1 tx") + dst1 = {'address': SUBADDRESS, 'amount': 1000000000000} + dst2 = {'address': SUBADDRESS2, 'amount': 1000000000000} + + self.export_import(False) + res = self.hot_wallet.transfer([dst1, dst2]) + + self.sign_and_submit(res.unsigned_txset) + + def sweep(self): + print("Mine 10 blocks so all non-coinbase outs from prior txs unlock") + self.mine(10) + + print("Sweeping the wallet") + self.export_import(False) + res = self.hot_wallet.sweep_all(STANDARD_ADDRESS) + + self.sign_and_submit(res.unsigned_txset) + class Guard: def __enter__(self): for i in range(2):