From e86d98e7ca8f82b47e830a8db87ac5d0acd68fdf Mon Sep 17 00:00:00 2001 From: koe Date: Sun, 6 Sep 2026 15:21:36 -0500 Subject: [PATCH] tx/partial tx validation improvements (cherry picked from commit b09e9bfe56824cd1d70a14db86d0507c928c1780) --- src/wallet/pending_tx_validation.cpp | 150 ++++++++++++++++++++++++++- src/wallet/pending_tx_validation.h | 23 ++++ src/wallet/wallet2.cpp | 138 ++++++++++++++++-------- src/wallet/wallet2.h | 1 + 4 files changed, 266 insertions(+), 46 deletions(-) diff --git a/src/wallet/pending_tx_validation.cpp b/src/wallet/pending_tx_validation.cpp index 6a3e912d7..af2cbf584 100644 --- a/src/wallet/pending_tx_validation.cpp +++ b/src/wallet/pending_tx_validation.cpp @@ -53,6 +53,7 @@ #include #include #include +#include #include #undef MONERO_DEFAULT_LOG_CATEGORY @@ -63,6 +64,87 @@ namespace tools namespace wallet { //------------------------------------------------------------------------------------------------------------------- +static const std::vector& get_tx_destinations(const wallet2::tx_construction_data &tx) +{ + return tx.splitted_dsts; +} +//------------------------------------------------------------------------------------------------------------------- +static const std::vector& get_tx_destinations(const wallet2::pending_tx &tx) +{ + return tx.construction_data.splitted_dsts; +} +//------------------------------------------------------------------------------------------------------------------- +static const std::vector& get_tx_sources(const wallet2::tx_construction_data &tx) +{ + return tx.sources; +} +//------------------------------------------------------------------------------------------------------------------- +static const std::vector& get_tx_sources(const wallet2::pending_tx &tx) +{ + return tx.construction_data.sources; +} +//------------------------------------------------------------------------------------------------------------------- +template +static bool check_consistent_ins_outs_impl(const std::vector &txes) +{ + std::unordered_set seen_ins; + std::unordered_map destination_types{}; + boost::multiprecision::uint128_t total_input_amount = 0; + for (const auto &tx: txes) + { + // Inputs + for (const auto &src: get_tx_sources(tx)) + { + CHECK_AND_ASSERT_THROW_MES(src.real_output < src.outputs.size(), + "check_consistent_ins_outs: ring sig index " << src.real_output << " out of input set size " + << src.outputs.size()); + const auto &dest = src.outputs[src.real_output].second.dest; + const auto result = seen_ins.emplace(dest); + CHECK_AND_ASSERT_THROW_MES(result.second, + "check_consistent_ins_outs: duplicate input pubkey"); + total_input_amount += src.amount; + } + + // Outputs + // Keep the address type consistent across transactions, including zero-amount + // outputs and change. + // We only care about normal vs subaddress consistency, not integrated address consistency. + for (const auto &dest: get_tx_destinations(tx)) + { + const auto result = destination_types.emplace(dest.addr, dest.is_subaddress); + CHECK_AND_ASSERT_THROW_MES(result.second || result.first->second == dest.is_subaddress, + "check_consistent_ins_outs: duplicate destinations do not have matching subaddress designations"); + } + } + CHECK_AND_ASSERT_THROW_MES(total_input_amount <= UINT64_MAX, + "check_consistent_ins_outs: tx set input amount > 2^64 - 1"); + return true; +} +//------------------------------------------------------------------------------------------------------------------- +static void sanity_check_unsigned_tx(const wallet2::tx_construction_data &tx, + const cryptonote::account_keys &account_keys, + const std::unordered_map &subaddresses) +{ + sanity_check_tx_construction_data(tx); + + const auto &change = tx.change_dts; + const bool pays_change = std::any_of(tx.splitted_dsts.begin(), tx.splitted_dsts.end(), + [&change](const cryptonote::tx_destination_entry &dest) { + return dest.amount > 0 && dest.addr == change.addr; + }); + + // Zero-valued dummy change outputs deliberately use an unrelated address. + if (change.amount == 0 && !pays_change) + return; + + if (cryptonote::sanity_check_change_address(change.addr, subaddresses, account_keys)) + return; + + const auto expected_change = account_keys.get_device().get_subaddress(account_keys, {tx.subaddr_account, 0}); + CHECK_AND_ASSERT_THROW_MES(change.addr == expected_change, + "sanity_check_unsigned_tx: change address does not belong to the sender account"); +} +//------------------------------------------------------------------------------------------------------------------- static void validate_tx_outs( const wallet2::pending_tx &ptx, const std::vector &ext_outputs, @@ -158,7 +240,7 @@ static void validate_tx_outs( // - encoded amount rct::ecdhTuple amnt_data{}; - memcpy(amnt_data.amount.bytes, &dest.amount, sizeof(dest.amount)); + amnt_data.amount = rct::d2h(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"); @@ -369,11 +451,13 @@ void sanity_check_pending_tx(const wallet2::pending_tx &ptx, // - 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; + std::unordered_set seen_ins; 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); + const auto transfer_pkey = transfer.get_public_key(); for (size_t i = 0; i < sources_ordered.size(); ++i) { const auto &src = sources_ordered.at(i); @@ -384,11 +468,16 @@ void sanity_check_pending_tx(const wallet2::pending_tx &ptx, // '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) + || memcmp(src.outputs[src.real_output].second.dest.bytes, rct::pk2rct(transfer_pkey).bytes, sizeof(rct::key)) != 0) continue; ins_order.push_back(i); break; } + // We check for duplicate onetime addr instead of selected_transfer in case of transfers with + // the same destination. Note that we assume `transfers` is sanitized of non-canonical pubkey representations. + CHECK_AND_ASSERT_THROW_MES(seen_ins.count(transfer_pkey) == 0, + "sanity_check_pending_tx: duplicate input pubkey"); + seen_ins.insert(transfer_pkey); } CHECK_AND_ASSERT_THROW_MES(ins_order.size() == sources_ordered.size(), "sanity_check_pending_tx: global index mismatch between sources and tx"); @@ -776,5 +865,62 @@ void sanity_check_pending_tx(const wallet2::pending_tx &ptx, "sanity_check_pending_tx: output amount != input amount"); } //------------------------------------------------------------------------------------------------------------------- +void check_consistent_ins_outs(const std::vector &txes) +{ + check_consistent_ins_outs_impl(txes); +} +//------------------------------------------------------------------------------------------------------------------- +void check_consistent_ins_outs(const std::vector &txes) +{ + check_consistent_ins_outs_impl(txes); +} +//------------------------------------------------------------------------------------------------------------------- +void sanity_check_tx_construction_data(const wallet2::tx_construction_data &tx) +{ + boost::multiprecision::uint128_t input_amount = 0; + boost::multiprecision::uint128_t output_amount = 0; + boost::multiprecision::uint128_t amount_to_change_address = 0; + for (const auto &src: tx.sources) + input_amount += src.amount; + for (const auto &dest: tx.splitted_dsts) + { + output_amount += dest.amount; + if (dest.addr == tx.change_dts.addr) + amount_to_change_address += dest.amount; + } + CHECK_AND_ASSERT_THROW_MES(output_amount <= input_amount, + "sanity_check_tx_construction_data: output amount exceeds input amount"); + CHECK_AND_ASSERT_THROW_MES(tx.change_dts.amount <= amount_to_change_address, + "sanity_check_tx_construction_data: change exceeds payment to change address"); +} +//------------------------------------------------------------------------------------------------------------------- +void sanity_check_unsigned_tx_set(const std::vector &txes, + const cryptonote::account_keys &account_keys, + const std::unordered_map &subaddresses) +{ + check_consistent_ins_outs(txes); + + for (const auto &tx: txes) + sanity_check_unsigned_tx(tx, account_keys, subaddresses); +} +//------------------------------------------------------------------------------------------------------------------- +void sanity_check_pending_tx_set(const std::vector &ptxs, + 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) +{ + check_consistent_ins_outs(ptxs); + + for (const auto &ptx: ptxs) + { + sanity_check_pending_tx(ptx, nettype, account_keys, subaddresses, transfers, + redacted, transfer_ki_resolver, allow_read_only); + } +} +//------------------------------------------------------------------------------------------------------------------- } //namespace wallet } //namespace tools diff --git a/src/wallet/pending_tx_validation.h b/src/wallet/pending_tx_validation.h index 77ba4018c..b6a31486a 100644 --- a/src/wallet/pending_tx_validation.h +++ b/src/wallet/pending_tx_validation.h @@ -47,6 +47,20 @@ namespace tools { namespace wallet { +// Checks that inputs are not duplicated across transactions in the set. +// Checks that duplicate destination addresses across the tx set have the same normal/subaddress +// designations. +// Checks that input amounts summed across the tx set fit in uint64_t. +// *Only* for checking consistency between txs (which may be proposals, partially signed, or fully signed). +// Use `sanity_check_pending_tx` to validate internal consistency for individual `pending_tx`, or +// `sanity_check_pending_tx_set` to check consistent ins/outs + internals. +void check_consistent_ins_outs(const std::vector &txes); +void check_consistent_ins_outs(const std::vector &txes); +void sanity_check_tx_construction_data(const wallet2::tx_construction_data &tx); +// Checks ins/outs consistency across the set, then validates each unsigned transaction. +void sanity_check_unsigned_tx_set(const std::vector &txes, + const cryptonote::account_keys &account_keys, + const std::unordered_map &subaddresses); /** * brief: sanity_check_pending_tx - validate `pending_tx` consistency with itself and with with `transfer_details` * Assumes `ptx` version is >= v16. @@ -76,5 +90,14 @@ void sanity_check_pending_tx(const wallet2::pending_tx &ptx, const bool redacted, const boost::optional> &transfer_ki_resolver, const bool allow_read_only); +// Checks ins/outs consistency across the set, then validates each pending transaction. +void sanity_check_pending_tx_set(const std::vector &ptxs, + 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 b52891437..3fb73b08f 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -7277,13 +7277,12 @@ void wallet2::get_unconfirmed_payments(std::list> make_transfer_ki_resolver( + const wallet2::transfer_container &transfers, const bool expect_imported_key_images, - boost::optional> transfer_ki_resolver, - const bool allow_read_only) const + boost::optional> transfer_ki_resolver) { if (expect_imported_key_images) { @@ -7292,17 +7291,28 @@ void wallet2::sanity_check_pending_tx(const wallet2::pending_tx &ptx, "sanity_check_pending_tx (wallet2): expected imported key images but a ki resolver was provided"); const std::function temp = - [this](const size_t i) + [&transfers](const size_t i) { - CHECK_AND_ASSERT_THROW_MES(i < m_transfers.size(), + CHECK_AND_ASSERT_THROW_MES(i < transfers.size(), "sanity_check_pending_tx (wallet2): transfer - selected transfer idx out of known transfers"); - const auto &transfer = m_transfers.at(i); + const auto &transfer = 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; } + return transfer_ki_resolver; +} +//---------------------------------------------------------------------------------------------------- +void wallet2::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 +{ + transfer_ki_resolver = make_transfer_ki_resolver(m_transfers, + expect_imported_key_images, std::move(transfer_ki_resolver)); wallet::sanity_check_pending_tx(ptx, this->nettype(), @@ -7314,6 +7324,25 @@ void wallet2::sanity_check_pending_tx(const wallet2::pending_tx &ptx, allow_read_only); } //---------------------------------------------------------------------------------------------------- +void wallet2::sanity_check_pending_tx_set(const std::vector &ptxs, + const bool redacted, + const bool expect_imported_key_images, + boost::optional> transfer_ki_resolver, + const bool allow_read_only) const +{ + transfer_ki_resolver = make_transfer_ki_resolver(m_transfers, + expect_imported_key_images, std::move(transfer_ki_resolver)); + + wallet::sanity_check_pending_tx_set(ptxs, + 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, @@ -7881,12 +7910,19 @@ bool wallet2::parse_unsigned_tx_from_str(const std::string &unsigned_tx_st, unsi LOG_PRINT_L0("Failed to parse data from unsigned tx"); return false; } + try { wallet::sanity_check_unsigned_tx_set(exported_txs.txes, m_account.get_keys(), m_subaddresses); } + catch (const std::exception &e) + { + LOG_PRINT_L0("Failed to validate unsigned txs: " << e.what()); + return false; + } } else { LOG_PRINT_L0("Unsupported version in unsigned tx"); return false; } + LOG_PRINT_L1("Loaded tx unsigned data from binary: " << exported_txs.txes.size() << " transactions"); return true; @@ -7908,6 +7944,8 @@ bool wallet2::sign_tx(const std::string &unsigned_filename, const std::string &s //---------------------------------------------------------------------------------------------------- bool wallet2::sign_tx(unsigned_tx_set &exported_txs, std::vector &txs, signed_tx_set &signed_txes) { + wallet::sanity_check_unsigned_tx_set(exported_txs.txes, m_account.get_keys(), m_subaddresses); + if (!std::get<2>(exported_txs.new_transfers).empty()) import_outputs(exported_txs.new_transfers); else if (!std::get<2>(exported_txs.transfers).empty()) @@ -8044,8 +8082,8 @@ bool wallet2::sign_tx(unsigned_tx_set &exported_txs, std::vectorsanity_check_pending_tx(ptx, false, true, boost::none, false); } + this->sanity_check_pending_tx_set(txs, false, true, boost::none, false); return true; } @@ -8221,23 +8259,20 @@ 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); - } + // Manually check `signed_txs.key_images` so local state is not mutated before we validate. + // We inject the key image checker because `ptx` internal sorting ambiguity makes it cumbersome to + // directly validate key images here. + // NOTE: These txs may be READ-ONLY, which means spent/frozen inputs are allowed. + const auto &kis = signed_txs.key_images; + this->sanity_check_pending_tx_set(signed_txs.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) { @@ -8329,8 +8364,7 @@ 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); + this->sanity_check_pending_tx_set(ptx_vector, false, true, boost::none, false); multisig_tx_set txs; txs.m_ptx = ptx_vector; @@ -8405,13 +8439,10 @@ bool wallet2::parse_multisig_tx_from_str(std::string multisig_tx_st, multisig_tx try { // 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); - } + // 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_set(exported_txs.m_ptx, false, false, boost::none, true); } catch (const std::exception &e) { @@ -8447,6 +8478,24 @@ bool wallet2::load_multisig_tx(cryptonote::blobdata s, multisig_tx_set &exported const crypto::hash txid = get_transaction_hash(ptx.tx); if (store_tx_info()) { + // SPECIAL CONDITION: before saving any info about the tx we should fully validate it. This + // requires knowing the key images. parse_multisig_tx_from_str() is allowed to treat the + // tx as informational, which permits unknown key images, but here we enforce it. + try + { + for (const auto idx : ptx.construction_data.selected_transfers) + { + if (idx >= m_transfers.size() || !m_transfers[idx].m_key_image_known) + THROW_WALLET_EXCEPTION(error::multisig_import_needed); + } + this->sanity_check_pending_tx(ptx, false, true, boost::none, true); + } + catch (const std::exception &e) + { + LOG_PRINT_L0("load_multisig_tx failed: " << e.what()); + return false; + } + m_tx_keys[txid] = ptx.tx_key; m_additional_tx_keys[txid] = ptx.additional_tx_keys; } @@ -8482,6 +8531,10 @@ bool wallet2::load_multisig_tx_from_file(const std::string &filename, multisig_t //---------------------------------------------------------------------------------------------------- bool wallet2::sign_multisig_tx(multisig_tx_set &exported_txs_inout, std::vector &txids) { + wallet::check_consistent_ins_outs(exported_txs_inout.m_ptx); + for (const auto &ptx: exported_txs_inout.m_ptx) + wallet::sanity_check_tx_construction_data(ptx.construction_data); + multisig_tx_set exported_txs = exported_txs_inout; std::vector signed_txids; std::vector> signed_tx_key_indices; @@ -8666,14 +8719,8 @@ bool wallet2::sign_multisig_tx_to_file(multisig_tx_set &exported_txs, const std: bool wallet2::sign_multisig_tx_from_file(const std::string &filename, std::vector &txids, std::function accept_func) { multisig_tx_set exported_txs; - if(!load_multisig_tx_from_file(filename, exported_txs)) + if(!load_multisig_tx_from_file(filename, exported_txs, accept_func)) return false; - - if (accept_func && !accept_func(exported_txs)) - { - LOG_PRINT_L1("Transactions rejected by callback"); - return false; - } return sign_multisig_tx_to_file(exported_txs, filename, txids); } //---------------------------------------------------------------------------------------------------- @@ -12302,6 +12349,10 @@ void wallet2::cold_tx_aux_import(const std::vector & ptx, const std: //---------------------------------------------------------------------------------------------------- void wallet2::cold_sign_tx(const std::vector& ptx_vector, signed_tx_set &exported_txs, std::vector &dsts_info, std::vector & tx_device_aux) { + wallet::check_consistent_ins_outs(ptx_vector); + for (const auto &ptx: ptx_vector) + wallet::sanity_check_tx_construction_data(ptx.construction_data); + auto & hwdev = get_account().get_device(); if (!hwdev.has_tx_cold_sign()){ throw std::invalid_argument("Device does not support cold sign protocol"); @@ -12335,8 +12386,7 @@ void wallet2::cold_sign_tx(const std::vector& ptx_vector, signed_tx_ // 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); + this->sanity_check_pending_tx_set(exported_txs.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)); diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h index d951addee..ec8893873 100644 --- a/src/wallet/wallet2.h +++ b/src/wallet/wallet2.h @@ -1231,6 +1231,7 @@ private: 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; + void sanity_check_pending_tx_set(const std::vector &ptxs, 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();