Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multisig: fixes #8114

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

multisig: fixes #8114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
94cf745
multisig: forbid signing nonce reusage
Dec 6, 2021
5c62cdc
multisig: reproduce challenge before signing
Dec 6, 2021
b555ad7
multisig: use combination of multicomponent nonce
Dec 6, 2021
c68abe4
multisig: remove unused code
Dec 6, 2021
84907ee
multisig: misc review
UkoeHB Dec 6, 2021
e98c6be
review comments 2
UkoeHB Dec 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/common/varint.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,22 @@ namespace tools {
dest++; /* Seems kinda pointless... */
}

/*! \brief writes a varint to a fixed-size uchar array of size N.
*/
template<typename T, std::size_t N>
void encode_varint(T t, unsigned char (&out)[N]) {
static_assert(std::is_integral<T>::value, "");
static_assert(std::is_unsigned<T>::value, "");
static_assert((sizeof(T) * 8 + 6) / 7 <= N, ""); //output array must be large enough to store any varint encoding of 't'
for (std::size_t i = 0; i < N && t; ++i) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will write nothing if the value passed is 0. I don't think this is the intent. A unit test checking it writes the same as the existing varint writer (or that the reader gets back the same values) would find this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And why the new function at all? Static bounds checking? Couldn't it just do the static asserts, and then forward to the other existing function (so there isn't unnecessary code duplication)? This does have an extra runtime check for the array bounds, but if that triggers a truncation occurs that isn't handled.

Also, trivial bikeshedding, but the parameters are flipped from the other function, which bothers my OCD.

if (t >= 0x80)
out[i] = (static_cast<unsigned char>(t) & 0x7F) | 0x80;
else
out[i] = static_cast<unsigned char>(t);
t >>= 7;
}
}

/*! \brief Returns the string that represents the varint
*/
template<typename T>
Expand Down
1 change: 1 addition & 0 deletions src/cryptonote_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ namespace config
const unsigned char HASH_KEY_RPC_PAYMENT_NONCE = 0x58;
const unsigned char HASH_KEY_MEMORY = 'k';
const unsigned char HASH_KEY_MULTISIG[] = {'M', 'u', 'l', 't' , 'i', 's', 'i', 'g', 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
const unsigned char HASH_KEY_CLSAG_ROUND_MULTISIG[] = "CLSAG_round_ms_merge_factor";
const unsigned char HASH_KEY_TXPROOF_V2[] = "TXPROOF_V2";
const unsigned char HASH_KEY_CLSAG_ROUND[] = "CLSAG_round";
const unsigned char HASH_KEY_CLSAG_AGG_0[] = "CLSAG_agg_0";
Expand Down
1 change: 0 additions & 1 deletion src/cryptonote_core/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ target_link_libraries(cryptonote_core
common
cncrypto
blockchain_db
multisig
ringct
device
hardforks
Expand Down
28 changes: 9 additions & 19 deletions src/cryptonote_core/cryptonote_tx_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ using namespace epee;
#include "crypto/crypto.h"
#include "crypto/hash.h"
#include "ringct/rctSigs.h"
#include "multisig/multisig.h"

using namespace crypto;

Expand Down Expand Up @@ -199,7 +198,7 @@ namespace cryptonote
return addr.m_view_public_key;
}
//---------------------------------------------------------------
bool construct_tx_with_tx_key(const account_keys& sender_account_keys, const std::unordered_map<crypto::public_key, subaddress_index>& subaddresses, std::vector<tx_source_entry>& sources, std::vector<tx_destination_entry>& destinations, const boost::optional<cryptonote::account_public_address>& change_addr, const std::vector<uint8_t> &extra, transaction& tx, uint64_t unlock_time, const crypto::secret_key &tx_key, const std::vector<crypto::secret_key> &additional_tx_keys, bool rct, const rct::RCTConfig &rct_config, rct::multisig_out *msout, bool shuffle_outs)
bool construct_tx_with_tx_key(const account_keys& sender_account_keys, const std::unordered_map<crypto::public_key, subaddress_index>& subaddresses, std::vector<tx_source_entry>& sources, std::vector<tx_destination_entry>& destinations, const boost::optional<cryptonote::account_public_address>& change_addr, const std::vector<uint8_t> &extra, transaction& tx, uint64_t unlock_time, const crypto::secret_key &tx_key, const std::vector<crypto::secret_key> &additional_tx_keys, bool rct, const rct::RCTConfig &rct_config, bool shuffle_outs)
{
hw::device &hwdev = sender_account_keys.get_device();

Expand All @@ -212,10 +211,6 @@ namespace cryptonote
std::vector<rct::key> amount_keys;
tx.set_null();
amount_keys.clear();
if (msout)
{
msout->c.clear();
}

tx.version = rct ? 2 : 1;
tx.unlock_time = unlock_time;
Expand Down Expand Up @@ -329,8 +324,8 @@ namespace cryptonote
return false;
}

//check that derivated key is equal with real output key (if non multisig)
if(!msout && !(in_ephemeral.pub == src_entr.outputs[src_entr.real_output].second.dest) )
//check that derivated key is equal with real output key
if(!(in_ephemeral.pub == src_entr.outputs[src_entr.real_output].second.dest) )
{
LOG_ERROR("derived public key mismatch with output public key at index " << idx << ", real out " << src_entr.real_output << "! "<< ENDL << "derived_key:"
<< string_tools::pod_to_hex(in_ephemeral.pub) << ENDL << "real output_public_key:"
Expand All @@ -343,7 +338,7 @@ namespace cryptonote
//put key image into tx input
txin_to_key input_to_key;
input_to_key.amount = src_entr.amount;
input_to_key.k_image = msout ? rct::rct2ki(src_entr.multisig_kLRki.ki) : img;
input_to_key.k_image = img;

//fill outputs array and use relative offsets
for(const tx_source_entry::output_entry& out_entry: src_entr.outputs)
Expand Down Expand Up @@ -526,7 +521,6 @@ namespace cryptonote
rct::keyV destinations;
std::vector<uint64_t> inamounts, outamounts;
std::vector<unsigned int> index;
std::vector<rct::multisig_kLRki> kLRki;
for (size_t i = 0; i < sources.size(); ++i)
{
rct::ctkey ctkey;
Expand All @@ -540,10 +534,6 @@ namespace cryptonote
memwipe(&ctkey, sizeof(rct::ctkey));
// inPk: (public key, commitment)
// will be done when filling in mixRing
if (msout)
{
kLRki.push_back(sources[i].multisig_kLRki);
}
}
for (size_t i = 0; i < tx.vout.size(); ++i)
{
Expand Down Expand Up @@ -593,9 +583,9 @@ namespace cryptonote
get_transaction_prefix_hash(tx, tx_prefix_hash, hwdev);
rct::ctkeyV outSk;
if (use_simple_rct)
tx.rct_signatures = rct::genRctSimple(rct::hash2rct(tx_prefix_hash), inSk, destinations, inamounts, outamounts, amount_in - amount_out, mixRing, amount_keys, msout ? &kLRki : NULL, msout, index, outSk, rct_config, hwdev);
tx.rct_signatures = rct::genRctSimple(rct::hash2rct(tx_prefix_hash), inSk, destinations, inamounts, outamounts, amount_in - amount_out, mixRing, amount_keys, index, outSk, rct_config, hwdev);
else
tx.rct_signatures = rct::genRct(rct::hash2rct(tx_prefix_hash), inSk, destinations, outamounts, mixRing, amount_keys, msout ? &kLRki[0] : NULL, msout, sources[0].real_output, outSk, rct_config, hwdev); // same index assumption
tx.rct_signatures = rct::genRct(rct::hash2rct(tx_prefix_hash), inSk, destinations, outamounts, mixRing, amount_keys, sources[0].real_output, outSk, rct_config, hwdev); // same index assumption
memwipe(inSk.data(), inSk.size() * sizeof(rct::ctkey));

CHECK_AND_ASSERT_MES(tx.vout.size() == outSk.size(), false, "outSk size does not match vout");
Expand All @@ -608,7 +598,7 @@ namespace cryptonote
return true;
}
//---------------------------------------------------------------
bool construct_tx_and_get_tx_key(const account_keys& sender_account_keys, const std::unordered_map<crypto::public_key, subaddress_index>& subaddresses, std::vector<tx_source_entry>& sources, std::vector<tx_destination_entry>& destinations, const boost::optional<cryptonote::account_public_address>& change_addr, const std::vector<uint8_t> &extra, transaction& tx, uint64_t unlock_time, crypto::secret_key &tx_key, std::vector<crypto::secret_key> &additional_tx_keys, bool rct, const rct::RCTConfig &rct_config, rct::multisig_out *msout)
bool construct_tx_and_get_tx_key(const account_keys& sender_account_keys, const std::unordered_map<crypto::public_key, subaddress_index>& subaddresses, std::vector<tx_source_entry>& sources, std::vector<tx_destination_entry>& destinations, const boost::optional<cryptonote::account_public_address>& change_addr, const std::vector<uint8_t> &extra, transaction& tx, uint64_t unlock_time, crypto::secret_key &tx_key, std::vector<crypto::secret_key> &additional_tx_keys, bool rct, const rct::RCTConfig &rct_config)
{
hw::device &hwdev = sender_account_keys.get_device();
hwdev.open_tx(tx_key);
Expand All @@ -628,7 +618,7 @@ namespace cryptonote
}
}

bool r = construct_tx_with_tx_key(sender_account_keys, subaddresses, sources, destinations, change_addr, extra, tx, unlock_time, tx_key, additional_tx_keys, rct, rct_config, msout);
bool r = construct_tx_with_tx_key(sender_account_keys, subaddresses, sources, destinations, change_addr, extra, tx, unlock_time, tx_key, additional_tx_keys, rct, rct_config);
hwdev.close_tx();
return r;
} catch(...) {
Expand All @@ -644,7 +634,7 @@ namespace cryptonote
crypto::secret_key tx_key;
std::vector<crypto::secret_key> additional_tx_keys;
std::vector<tx_destination_entry> destinations_copy = destinations;
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, false, { rct::RangeProofBorromean, 0}, NULL);
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, false, { rct::RangeProofBorromean, 0});
}
//---------------------------------------------------------------
bool generate_genesis_block(
Expand Down
4 changes: 2 additions & 2 deletions src/cryptonote_core/cryptonote_tx_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ namespace cryptonote
//---------------------------------------------------------------
crypto::public_key get_destination_view_key_pub(const std::vector<tx_destination_entry> &destinations, const boost::optional<cryptonote::account_public_address>& change_addr);
bool construct_tx(const account_keys& sender_account_keys, std::vector<tx_source_entry> &sources, const std::vector<tx_destination_entry>& destinations, const boost::optional<cryptonote::account_public_address>& change_addr, const std::vector<uint8_t> &extra, transaction& tx, uint64_t unlock_time);
bool construct_tx_with_tx_key(const account_keys& sender_account_keys, const std::unordered_map<crypto::public_key, subaddress_index>& subaddresses, std::vector<tx_source_entry>& sources, std::vector<tx_destination_entry>& destinations, const boost::optional<cryptonote::account_public_address>& change_addr, const std::vector<uint8_t> &extra, transaction& tx, uint64_t unlock_time, const crypto::secret_key &tx_key, const std::vector<crypto::secret_key> &additional_tx_keys, bool rct = false, const rct::RCTConfig &rct_config = { rct::RangeProofBorromean, 0 }, rct::multisig_out *msout = NULL, bool shuffle_outs = true);
bool construct_tx_and_get_tx_key(const account_keys& sender_account_keys, const std::unordered_map<crypto::public_key, subaddress_index>& subaddresses, std::vector<tx_source_entry>& sources, std::vector<tx_destination_entry>& destinations, const boost::optional<cryptonote::account_public_address>& change_addr, const std::vector<uint8_t> &extra, transaction& tx, uint64_t unlock_time, crypto::secret_key &tx_key, std::vector<crypto::secret_key> &additional_tx_keys, bool rct = false, const rct::RCTConfig &rct_config = { rct::RangeProofBorromean, 0 }, rct::multisig_out *msout = NULL);
bool construct_tx_with_tx_key(const account_keys& sender_account_keys, const std::unordered_map<crypto::public_key, subaddress_index>& subaddresses, std::vector<tx_source_entry>& sources, std::vector<tx_destination_entry>& destinations, const boost::optional<cryptonote::account_public_address>& change_addr, const std::vector<uint8_t> &extra, transaction& tx, uint64_t unlock_time, const crypto::secret_key &tx_key, const std::vector<crypto::secret_key> &additional_tx_keys, bool rct = false, const rct::RCTConfig &rct_config = { rct::RangeProofBorromean, 0 }, bool shuffle_outs = true);
bool construct_tx_and_get_tx_key(const account_keys& sender_account_keys, const std::unordered_map<crypto::public_key, subaddress_index>& subaddresses, std::vector<tx_source_entry>& sources, std::vector<tx_destination_entry>& destinations, const boost::optional<cryptonote::account_public_address>& change_addr, const std::vector<uint8_t> &extra, transaction& tx, uint64_t unlock_time, crypto::secret_key &tx_key, std::vector<crypto::secret_key> &additional_tx_keys, bool rct = false, const rct::RCTConfig &rct_config = { rct::RangeProofBorromean, 0 });
bool generate_output_ephemeral_keys(const size_t tx_version, const cryptonote::account_keys &sender_account_keys, const crypto::public_key &txkey_pub, const crypto::secret_key &tx_key,
const cryptonote::tx_destination_entry &dst_entr, const boost::optional<cryptonote::account_public_address> &change_addr, const size_t output_index,
const bool &need_additional_txkeys, const std::vector<crypto::secret_key> &additional_tx_keys,
Expand Down
3 changes: 3 additions & 0 deletions src/multisig/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@
# THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

set(multisig_sources
signing_protocol.cpp
multisig.cpp)

set(multisig_headers)

set(multisig_private_headers
signing_protocol.h
multisig.h)

monero_private_headers(multisig
Expand All @@ -46,6 +48,7 @@ target_link_libraries(multisig
PUBLIC
ringct
cryptonote_basic
cryptonote_core
common
cncrypto
PRIVATE
Expand Down
Loading