From c7b2944f8960c208ceddeb3075a673630ae000cd Mon Sep 17 00:00:00 2001 From: anon Date: Mon, 6 Dec 2021 10:25:01 +0000 Subject: multisig: fix critical vulnerabilities in signing --- tests/core_tests/chaingen.cpp | 2 +- tests/core_tests/multisig.cpp | 181 ++++++++++++++++++-------------------- tests/core_tests/multisig.h | 2 +- tests/core_tests/rct.cpp | 2 +- tests/core_tests/wallet_tools.cpp | 2 +- 5 files changed, 89 insertions(+), 100 deletions(-) (limited to 'tests/core_tests') diff --git a/tests/core_tests/chaingen.cpp b/tests/core_tests/chaingen.cpp index 61195c7b0..144e87bc2 100644 --- a/tests/core_tests/chaingen.cpp +++ b/tests/core_tests/chaingen.cpp @@ -1076,7 +1076,7 @@ bool construct_tx_rct(const cryptonote::account_keys& sender_account_keys, std:: std::vector additional_tx_keys; std::vector destinations_copy = destinations; rct::RCTConfig rct_config = {range_proof_type, bp_version}; - return construct_tx_and_get_tx_key(sender_account_keys, subaddresses, sources, destinations_copy, change_addr, extra, tx, unlock_time, tx_key, additional_tx_keys, rct, rct_config, nullptr); + return construct_tx_and_get_tx_key(sender_account_keys, subaddresses, sources, destinations_copy, change_addr, extra, tx, unlock_time, tx_key, additional_tx_keys, rct, rct_config); } transaction construct_tx_with_fee(std::vector& events, const block& blk_head, diff --git a/tests/core_tests/multisig.cpp b/tests/core_tests/multisig.cpp index 3db3d4059..28d176e56 100644 --- a/tests/core_tests/multisig.cpp +++ b/tests/core_tests/multisig.cpp @@ -28,6 +28,11 @@ // // Parts of this file are originally copyright (c) 2012-2013 The Cryptonote developers +#include "ringct/rctSigs.h" +#include "cryptonote_basic/cryptonote_basic.h" +#include "multisig/multisig.h" +#include "multisig/multisig_tx_builder_ringct.h" +#include "common/apply_permutation.h" #include "chaingen.h" #include "multisig.h" @@ -113,7 +118,7 @@ static bool make_multisig_accounts(std::vector &accoun bool gen_multisig_tx_validation_base::generate_with(std::vector& events, size_t inputs, size_t mixin, uint64_t amount_paid, bool valid, - size_t threshold, size_t total, size_t creator, std::vector signers, + size_t threshold, size_t total, size_t creator, std::vector other_signers, const std::function &sources, std::vector &destinations)> &pre_tx, const std::function &post_tx) const { @@ -122,30 +127,18 @@ bool gen_multisig_tx_validation_base::generate_with(std::vector= 2, false, "Bad scheme"); CHECK_AND_ASSERT_MES(threshold <= total, false, "Bad scheme"); -#ifdef NO_MULTISIG - CHECK_AND_ASSERT_MES(total <= 5, false, "Unsupported scheme"); -#endif CHECK_AND_ASSERT_MES(inputs >= 1 && inputs <= 8, false, "Inputs should between 1 and 8"); // given as 1 based for clarity --creator; - for (size_t &signer: signers) + for (size_t &signer: other_signers) --signer; CHECK_AND_ASSERT_MES(creator < total, false, "invalid creator"); - for (size_t signer: signers) + for (size_t signer: other_signers) CHECK_AND_ASSERT_MES(signer < total, false, "invalid signer"); -#ifdef NO_MULTISIG - GENERATE_ACCOUNT(acc0); - GENERATE_ACCOUNT(acc1); - GENERATE_ACCOUNT(acc2); - GENERATE_ACCOUNT(acc3); - GENERATE_ACCOUNT(acc4); - account_base miner_account[5] = {acc0, acc1, acc2, acc3, acc4}; -#else GENERATE_MULTISIG_ACCOUNT(miner_account, threshold, total); -#endif MAKE_GENESIS_BLOCK(events, blk_0, miner_account[creator], ts_start); @@ -193,14 +186,13 @@ bool gen_multisig_tx_validation_base::generate_with(std::vector(blocks[n].miner_tx.vout[0].target).key; + cryptonote::get_output_public_key(blocks[n].miner_tx.vout[0], output_pub_key[n]); MDEBUG("output_pub_key: " << output_pub_key); } std::unordered_map subaddresses; subaddresses[miner_account[0].get_keys().m_account_address.m_spend_public_key] = {0,0}; -#ifndef NO_MULTISIG // create k/L/R/ki for that output we're going to spend std::vector>> account_k(total); std::vector>> account_L(total); @@ -213,6 +205,7 @@ bool gen_multisig_tx_validation_base::generate_with(std::vector kLRkis; @@ -257,34 +249,6 @@ bool gen_multisig_tx_validation_base::generate_with(std::vector pkis; for (size_t msidx = 0; msidx < total; ++msidx) for (size_t n = 0; n < account_ki[msidx][tdidx].size(); ++n) @@ -292,8 +256,6 @@ bool gen_multisig_tx_validation_base::generate_with(std::vector sources; for (size_t n = 0; n < inputs; ++n) { @@ -322,7 +283,9 @@ bool gen_multisig_tx_validation_base::generate_with(std::vector(blocks[m].miner_tx.vout[0].target).key); + crypto::public_key output_public_key; + cryptonote::get_output_public_key(blocks[m].miner_tx.vout[0], output_public_key); + ctkey.dest = rct::pk2rct(output_public_key); MDEBUG("using " << (m == n ? "real" : "fake") << " input " << ctkey.dest); ctkey.mask = rct::commit(blocks[m].miner_tx.vout[0].amount, rct::identity()); // since those are coinbases, the masks are known src.outputs.push_back(std::make_pair(m, ctkey)); @@ -333,11 +296,8 @@ bool gen_multisig_tx_validation_base::generate_with(std::vector destinations; + std::vector destinations; //tx need two outputs since HF_VERSION_MIN_2_OUTPUTS destinations.push_back(td); - cryptonote::account_base dummy; - dummy.generate(); - td.addr = dummy.get_keys().m_account_address; td.amount = 0; destinations.push_back(td); @@ -346,18 +306,11 @@ bool gen_multisig_tx_validation_base::generate_with(std::vector additional_tx_secret_keys; auto sources_copy = sources; - r = construct_tx_and_get_tx_key(miner_account[creator].get_keys(), subaddresses, sources, destinations, boost::none, std::vector(), tx, 0, tx_key, additional_tx_secret_keys, true, { rct::RangeProofPaddedBulletproof, 0 }, msoutp); - CHECK_AND_ASSERT_MES(r, false, "failed to construct transaction"); + multisig::signing::tx_builder_ringct_t tx_builder; + CHECK_AND_ASSERT_MES(tx_builder.init(miner_account[creator].get_keys(), {}, 0, 0, {0}, sources, destinations, {}, {rct::RangeProofPaddedBulletproof, 4}, true, false, tx_key, additional_tx_secret_keys, tx), false, "error: multisig::signing::tx_builder_t::init"); -#ifndef NO_MULTISIG // work out the permutation done on sources std::vector ins_order; for (size_t n = 0; n < sources.size(); ++n) @@ -371,15 +324,50 @@ bool gen_multisig_tx_validation_base::generate_with(std::vector used_keys; const std::vector &msk0 = miner_account[creator].get_multisig_keys(); for (const auto &sk: msk0) - used_keys.insert(sk); - for (size_t signer: signers) + used_keys.insert(sk); //these were used in 'tx_builder.init() -> tx_builder.first_partial_sign()' + for (size_t signer: other_signers) { rct::key skey = rct::zero(); const std::vector &msk1 = miner_account[signer].get_multisig_keys(); @@ -393,38 +381,37 @@ bool gen_multisig_tx_validation_base::generate_with(std::vector indices; - for (const auto &src: sources_copy) - indices.push_back(src.real_output); - rct::keyV k; - for (size_t tdidx = 0; tdidx < inputs; ++tdidx) - { - k.push_back(rct::zero()); - for (size_t n = 0; n < account_k[signer][tdidx].size(); ++n) - { - crypto::public_key L; - rct::scalarmultBase((rct::key&)L, rct::sk2rct(account_k[signer][tdidx][n])); - if (used_L.find(L) != used_L.end()) - { - sc_add(k.back().bytes, k.back().bytes, rct::sk2rct(account_k[signer][tdidx][n]).bytes); + rct::keyM k(sources.size(), rct::keyV(multisig::signing::kAlphaComponents)); + for (std::size_t i = 0; i < sources.size(); ++i) { + for (std::size_t j = 0; j < multisig::signing::kAlphaComponents; ++j) { + for (std::size_t n = 0; n < account_k[signer][i].size(); ++n) { + crypto::public_key L; + rct::scalarmultBase((rct::key&)L, rct::sk2rct(account_k[signer][i][n])); + if (used_L.find(L) != used_L.end()) { + k[i][j] = rct::sk2rct(account_k[signer][i][n]); + account_k[signer][i][n] = rct::rct2sk(rct::zero()); //demo: always clear nonces from long-term storage after use + break; + } } + CHECK_AND_ASSERT_MES(!(k[i][j] == rct::zero()), false, "failed to find k to sign transaction"); } - CHECK_AND_ASSERT_MES(!(k.back() == rct::zero()), false, "failed to find k to sign transaction"); } - tools::apply_permutation(ins_order, indices); tools::apply_permutation(ins_order, k); + multisig::signing::tx_builder_ringct_t signer_tx_builder; + CHECK_AND_ASSERT_MES(signer_tx_builder.init(miner_account[signer].get_keys(), {}, 0, 0, {0}, sources, destinations, {}, {rct::RangeProofPaddedBulletproof, 4}, true, true, tx_key, additional_tx_secret_keys, tx), false, "error: multisig::signing::tx_builder_t::init"); MDEBUG("signing with k size " << k.size()); - MDEBUG("signing with k " << k.back()); + for (size_t n = 0; n < multisig::signing::kAlphaComponents; ++n) + MDEBUG("signing with k " << k.back()[n]); MDEBUG("signing with sk " << skey); for (const auto &sk: used_keys) MDEBUG(" created with sk " << sk); - MDEBUG("signing with c size " << msout.c.size()); - MDEBUG("signing with c " << msout.c.back()); - r = rct::signMultisig(tx.rct_signatures, indices, k, msout, skey); - CHECK_AND_ASSERT_MES(r, false, "failed to sign transaction"); + CHECK_AND_ASSERT_MES(signer_tx_builder.next_partial_sign(sig.total_alpha_G, sig.total_alpha_H, k, skey, sig.c_0, sig.s), false, "error: multisig::signing::tx_builder_ringct_t::next_partial_sign"); + + // in round-robin signing, the last signer finalizes the tx + if (signer == other_signers.back()) + CHECK_AND_ASSERT_MES(signer_tx_builder.finalize_tx(sources, sig.c_0, sig.s, tx), false, "error: multisig::signing::tx_builder_ringct_t::finalize_tx"); } -#endif // verify this tx is really to the expected address const crypto::public_key tx_pub_key2 = get_tx_pub_key_from_extra(tx, 0); @@ -433,10 +420,12 @@ bool gen_multisig_tx_validation_base::generate_with(std::vector additional_derivations; + crypto::public_key output_public_key; for (size_t n = 0; n < tx.vout.size(); ++n) { - CHECK_AND_ASSERT_MES(typeid(txout_to_key) == tx.vout[n].target.type(), false, "Unexpected tx out type"); - if (is_out_to_acc_precomp(subaddresses, boost::get(tx.vout[n].target).key, derivation, additional_derivations, n, hw::get_device(("default")))) + CHECK_AND_ASSERT_MES(typeid(txout_to_tagged_key) == tx.vout[n].target.type(), false, "Unexpected tx out type"); + cryptonote::get_output_public_key(tx.vout[n], output_public_key); + if (is_out_to_acc_precomp(subaddresses, output_public_key, derivation, additional_derivations, n, hw::get_device(("default")))) { ++n_outs; CHECK_AND_ASSERT_MES(tx.vout[n].amount == 0, false, "Destination amount is not zero"); @@ -451,7 +440,7 @@ bool gen_multisig_tx_validation_base::generate_with(std::vector& events, size_t inputs, size_t mixin, uint64_t amount_paid, bool valid, - size_t threshold, size_t total, size_t creator, std::vector signers, + size_t threshold, size_t total, size_t creator, std::vector other_signers, const std::function &sources, std::vector &destinations)> &pre_tx, const std::function &post_tx) const; diff --git a/tests/core_tests/rct.cpp b/tests/core_tests/rct.cpp index 0926483fe..4e51ed713 100644 --- a/tests/core_tests/rct.cpp +++ b/tests/core_tests/rct.cpp @@ -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, 0, tx_key, additional_tx_keys, true, rct_config, NULL, use_view_tags); + bool r = construct_tx_and_get_tx_key(miner_accounts[0].get_keys(), subaddresses, sources, destinations, cryptonote::account_public_address{}, std::vector(), tx, 0, 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/wallet_tools.cpp b/tests/core_tests/wallet_tools.cpp index fdc4753f9..a3b66e835 100644 --- a/tests/core_tests/wallet_tools.cpp +++ b/tests/core_tests/wallet_tools.cpp @@ -280,5 +280,5 @@ bool construct_tx_rct(tools::wallet2 * sender_wallet, std::vector additional_tx_keys; std::vector destinations_copy = destinations; rct::RCTConfig rct_config = {range_proof_type, bp_version}; - return construct_tx_and_get_tx_key(sender_wallet->get_account().get_keys(), subaddresses, sources, destinations_copy, change_addr, extra, tx, unlock_time, tx_key, additional_tx_keys, rct, rct_config, nullptr); + return construct_tx_and_get_tx_key(sender_wallet->get_account().get_keys(), subaddresses, sources, destinations_copy, change_addr, extra, tx, unlock_time, tx_key, additional_tx_keys, rct, rct_config); } -- cgit v1.2.3