diff --git a/src/cryptonote_core/cryptonote_tx_utils.cpp b/src/cryptonote_core/cryptonote_tx_utils.cpp index e9a66ac50..65a0ffc6a 100644 --- a/src/cryptonote_core/cryptonote_tx_utils.cpp +++ b/src/cryptonote_core/cryptonote_tx_utils.cpp @@ -28,9 +28,11 @@ // // Parts of this file are originally copyright (c) 2012-2013 The Cryptonote developers +#include #include #include #include "include_base_utils.h" +#include "misc_log_ex.h" #include "string_tools.h" using namespace epee; @@ -46,6 +48,40 @@ 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 + */ +std::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 std::nullopt; + + return {subaddr_index}; +} +//--------------------------------------------------------------- +} //anonymous namespace + namespace cryptonote { //--------------------------------------------------------------- @@ -213,6 +249,10 @@ namespace cryptonote return false; } + std::optional recognized_change_index; + if (change_addr) + recognized_change_index = sanity_check_change_address(*change_addr, subaddresses, sender_account_keys); + std::vector amount_keys; tx.set_null(); amount_keys.clear(); @@ -406,6 +446,11 @@ namespace cryptonote for(const tx_destination_entry& dst_entr: destinations) { CHECK_AND_ASSERT_MES(dst_entr.amount > 0 || tx.version > 1, false, "Destination with wrong amount: " << dst_entr.amount); + const bool matches_change_addr = change_addr && dst_entr.addr == *change_addr; + const bool is_bad_change_dst = matches_change_addr && dst_entr.amount > 0 && !recognized_change_index; + CHECK_AND_ASSERT_MES(!is_bad_change_dst, false, + "Non-zero amount change address is not recognized as belonging to the sender account"); + crypto::public_key out_eph_public_key; crypto::view_tag view_tag; diff --git a/tests/core_tests/bulletproof_plus.cpp b/tests/core_tests/bulletproof_plus.cpp index 1c7a70dfb..b829bcf06 100644 --- a/tests/core_tests/bulletproof_plus.cpp +++ b/tests/core_tests/bulletproof_plus.cpp @@ -136,7 +136,7 @@ bool gen_bpp_tx_validation_base::generate_with(std::vector& ev std::unordered_map subaddresses; subaddresses[miner_accounts[n].get_keys().m_account_address.m_spend_public_key] = {0,0}; rct_txes.resize(rct_txes.size() + 1); - bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, cryptonote::account_public_address{}, std::vector(), rct_txes.back(), tx_key, additional_tx_keys, true, rct_config[n]); + bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, boost::none, std::vector(), rct_txes.back(), tx_key, additional_tx_keys, true, rct_config[n]); CHECK_AND_ASSERT_MES(r, false, "failed to construct transaction"); if (post_tx && !post_tx(rct_txes.back(), n)) diff --git a/tests/core_tests/bulletproofs.cpp b/tests/core_tests/bulletproofs.cpp index 320601b31..784580541 100644 --- a/tests/core_tests/bulletproofs.cpp +++ b/tests/core_tests/bulletproofs.cpp @@ -136,7 +136,7 @@ bool gen_bp_tx_validation_base::generate_with(std::vector& eve std::unordered_map subaddresses; subaddresses[miner_accounts[n].get_keys().m_account_address.m_spend_public_key] = {0,0}; rct_txes.resize(rct_txes.size() + 1); - bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, cryptonote::account_public_address{}, std::vector(), rct_txes.back(), tx_key, additional_tx_keys, true, rct_config[n]); + bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, boost::none, std::vector(), rct_txes.back(), tx_key, additional_tx_keys, true, rct_config[n]); CHECK_AND_ASSERT_MES(r, false, "failed to construct transaction"); if (post_tx && !post_tx(rct_txes.back(), n)) diff --git a/tests/core_tests/rct.cpp b/tests/core_tests/rct.cpp index 0252c4885..883f26363 100644 --- a/tests/core_tests/rct.cpp +++ b/tests/core_tests/rct.cpp @@ -122,7 +122,7 @@ bool gen_rct_tx_validation_base::generate_with_full(std::vector additional_tx_keys; std::unordered_map subaddresses; subaddresses[miner_accounts[n].get_keys().m_account_address.m_spend_public_key] = {0,0}; - bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, cryptonote::account_public_address{}, std::vector(), rct_txes[n], tx_key, additional_tx_keys, true); + bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, boost::none, std::vector(), rct_txes[n], tx_key, additional_tx_keys, true); CHECK_AND_ASSERT_MES(r, false, "failed to construct transaction"); events.push_back(rct_txes[n]); starting_rct_tx_hashes.push_back(get_transaction_hash(rct_txes[n])); @@ -229,7 +229,7 @@ bool gen_rct_tx_validation_base::generate_with_full(std::vector additional_tx_keys; std::unordered_map subaddresses; subaddresses[miner_accounts[0].get_keys().m_account_address.m_spend_public_key] = {0,0}; - bool r = construct_tx_and_get_tx_key(miner_accounts[0].get_keys(), subaddresses, sources, destinations, cryptonote::account_public_address{}, std::vector(), tx, tx_key, additional_tx_keys, true, rct_config, use_view_tags); + bool r = construct_tx_and_get_tx_key(miner_accounts[0].get_keys(), subaddresses, sources, destinations, boost::none, std::vector(), tx, tx_key, additional_tx_keys, true, rct_config, use_view_tags); CHECK_AND_ASSERT_MES(r, false, "failed to construct transaction"); if (post_tx) diff --git a/tests/core_tests/rct2.cpp b/tests/core_tests/rct2.cpp index c4197fff4..182d80404 100644 --- a/tests/core_tests/rct2.cpp +++ b/tests/core_tests/rct2.cpp @@ -136,7 +136,7 @@ bool gen_rct2_tx_validation_base::generate_with(std::vector& e std::unordered_map subaddresses; subaddresses[miner_accounts[n].get_keys().m_account_address.m_spend_public_key] = {0,0}; rct_txes.resize(rct_txes.size() + 1); - bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, cryptonote::account_public_address{}, std::vector(), rct_txes.back(), tx_key, additional_tx_keys, true, rct_config[n]); + bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, boost::none, std::vector(), rct_txes.back(), tx_key, additional_tx_keys, true, rct_config[n]); CHECK_AND_ASSERT_MES(r, false, "failed to construct transaction"); if (post_tx && !post_tx(rct_txes.back(), n)) diff --git a/tests/performance_tests/check_tx_signature.h b/tests/performance_tests/check_tx_signature.h index 0ca514bfa..59dafb1e1 100644 --- a/tests/performance_tests/check_tx_signature.h +++ b/tests/performance_tests/check_tx_signature.h @@ -72,7 +72,7 @@ public: std::unordered_map subaddresses; subaddresses[this->m_miners[this->real_source_idx].get_keys().m_account_address.m_spend_public_key] = {0,0}; rct::RCTConfig rct_config{range_proof_type, bp_version}; - if (!construct_tx_and_get_tx_key(this->m_miners[this->real_source_idx].get_keys(), subaddresses, this->m_sources, destinations, cryptonote::account_public_address{}, std::vector(), m_tx, tx_key, additional_tx_keys, rct, rct_config)) + if (!construct_tx_and_get_tx_key(this->m_miners[this->real_source_idx].get_keys(), subaddresses, this->m_sources, destinations, boost::none, std::vector(), m_tx, tx_key, additional_tx_keys, rct, rct_config)) return false; get_transaction_prefix_hash(m_tx, m_tx_prefix_hash); @@ -136,7 +136,7 @@ public: m_txes.resize(a_num_txes); for (size_t n = 0; n < a_num_txes; ++n) { - if (!construct_tx_and_get_tx_key(this->m_miners[this->real_source_idx].get_keys(), subaddresses, this->m_sources, destinations, cryptonote::account_public_address{}, std::vector(), m_txes[n], tx_key, additional_tx_keys, true, {rct::RangeProofPaddedBulletproof, 2})) + if (!construct_tx_and_get_tx_key(this->m_miners[this->real_source_idx].get_keys(), subaddresses, this->m_sources, destinations, boost::none, std::vector(), m_txes[n], tx_key, additional_tx_keys, true, {rct::RangeProofPaddedBulletproof, 2})) return false; } diff --git a/tests/performance_tests/construct_tx.h b/tests/performance_tests/construct_tx.h index 4bbd870e9..d3566d102 100644 --- a/tests/performance_tests/construct_tx.h +++ b/tests/performance_tests/construct_tx.h @@ -74,7 +74,7 @@ public: std::unordered_map subaddresses; subaddresses[this->m_miners[this->real_source_idx].get_keys().m_account_address.m_spend_public_key] = {0,0}; rct::RCTConfig rct_config{range_proof_type, bp_version}; - return cryptonote::construct_tx_and_get_tx_key(this->m_miners[this->real_source_idx].get_keys(), subaddresses, this->m_sources, m_destinations, cryptonote::account_public_address{}, std::vector(), m_tx, tx_key, additional_tx_keys, rct, rct_config); + return cryptonote::construct_tx_and_get_tx_key(this->m_miners[this->real_source_idx].get_keys(), subaddresses, this->m_sources, m_destinations, boost::none, std::vector(), m_tx, tx_key, additional_tx_keys, rct, rct_config); } private: