From fa06c39d9731b90dfd58ecd6cdfd3c936ee139a7 Mon Sep 17 00:00:00 2001 From: Sarang Noether <32460187+SarangNoether@users.noreply.github.com> Date: Fri, 28 Aug 2020 19:38:00 -0400 Subject: [PATCH] Bind signature to full address and signing mode --- src/cryptonote_config.h | 2 +- src/device/device.hpp | 1 - src/device/device_default.cpp | 6 -- src/device/device_default.hpp | 1 - src/device/device_ledger.cpp | 6 -- src/device/device_ledger.hpp | 1 - src/simplewallet/simplewallet.cpp | 8 +-- src/wallet/wallet2.cpp | 64 ++++++++++++-------- src/wallet/wallet2.h | 2 +- src/wallet/wallet_rpc_server.cpp | 3 - src/wallet/wallet_rpc_server_commands_defs.h | 2 +- tests/functional_tests/sign_message.py | 2 + 12 files changed, 47 insertions(+), 51 deletions(-) diff --git a/src/cryptonote_config.h b/src/cryptonote_config.h index 99d998824..f50ab6a40 100644 --- a/src/cryptonote_config.h +++ b/src/cryptonote_config.h @@ -230,7 +230,7 @@ namespace config const unsigned char HASH_KEY_CLSAG_ROUND[] = "CLSAG_round"; const unsigned char HASH_KEY_CLSAG_AGG_0[] = "CLSAG_agg_0"; const unsigned char HASH_KEY_CLSAG_AGG_1[] = "CLSAG_agg_1"; - const char HASH_KEY_MESSAGE_SIGNING[] = "MessageSignature"; + const char HASH_KEY_MESSAGE_SIGNING[] = "MoneroMessageSignature"; namespace testnet { diff --git a/src/device/device.hpp b/src/device/device.hpp index a6694ad09..582eb2242 100644 --- a/src/device/device.hpp +++ b/src/device/device.hpp @@ -162,7 +162,6 @@ namespace hw { virtual std::vector get_subaddress_spend_public_keys(const cryptonote::account_keys &keys, uint32_t account, uint32_t begin, uint32_t end) = 0; virtual cryptonote::account_public_address get_subaddress(const cryptonote::account_keys& keys, const cryptonote::subaddress_index &index) = 0; virtual crypto::secret_key get_subaddress_secret_key(const crypto::secret_key &sec, const cryptonote::subaddress_index &index) = 0; - virtual crypto::secret_key get_subaddress_view_secret_key(const crypto::secret_key &sec, const cryptonote::subaddress_index &index) = 0; /* ======================================================================= */ /* DERIVATION & KEY */ diff --git a/src/device/device_default.cpp b/src/device/device_default.cpp index cb63deb15..145197212 100644 --- a/src/device/device_default.cpp +++ b/src/device/device_default.cpp @@ -207,12 +207,6 @@ namespace hw { return m; } - crypto::secret_key device_default::get_subaddress_view_secret_key(const crypto::secret_key &a, const cryptonote::subaddress_index &index) { - crypto::secret_key skey = get_subaddress_secret_key(a, index); - sc_mul((unsigned char*)skey.data, (const unsigned char*)skey.data, (const unsigned char*)a.data); - return skey; - } - /* ======================================================================= */ /* DERIVATION & KEY */ /* ======================================================================= */ diff --git a/src/device/device_default.hpp b/src/device/device_default.hpp index e49a75b1c..2493bd67d 100644 --- a/src/device/device_default.hpp +++ b/src/device/device_default.hpp @@ -85,7 +85,6 @@ namespace hw { std::vector get_subaddress_spend_public_keys(const cryptonote::account_keys &keys, uint32_t account, uint32_t begin, uint32_t end) override; cryptonote::account_public_address get_subaddress(const cryptonote::account_keys& keys, const cryptonote::subaddress_index &index) override; crypto::secret_key get_subaddress_secret_key(const crypto::secret_key &sec, const cryptonote::subaddress_index &index) override; - crypto::secret_key get_subaddress_view_secret_key(const crypto::secret_key &sec, const cryptonote::subaddress_index &index) override; /* ======================================================================= */ /* DERIVATION & KEY */ diff --git a/src/device/device_ledger.cpp b/src/device/device_ledger.cpp index ec5c52e58..4e89f835d 100644 --- a/src/device/device_ledger.cpp +++ b/src/device/device_ledger.cpp @@ -880,12 +880,6 @@ namespace hw { return sub_sec; } - crypto::secret_key device_ledger::get_subaddress_view_secret_key(const crypto::secret_key &sec, const cryptonote::subaddress_index &index) { -#warning TODO - MERROR("Not implemented yet"); - return crypto::null_skey; - } - /* ======================================================================= */ /* DERIVATION & KEY */ /* ======================================================================= */ diff --git a/src/device/device_ledger.hpp b/src/device/device_ledger.hpp index f82339f7a..d3ec08288 100644 --- a/src/device/device_ledger.hpp +++ b/src/device/device_ledger.hpp @@ -249,7 +249,6 @@ namespace hw { std::vector get_subaddress_spend_public_keys(const cryptonote::account_keys &keys, uint32_t account, uint32_t begin, uint32_t end) override; cryptonote::account_public_address get_subaddress(const cryptonote::account_keys& keys, const cryptonote::subaddress_index &index) override; crypto::secret_key get_subaddress_secret_key(const crypto::secret_key &sec, const cryptonote::subaddress_index &index) override; - crypto::secret_key get_subaddress_view_secret_key(const crypto::secret_key &sec, const cryptonote::subaddress_index &index) override; /* ======================================================================= */ /* DERIVATION & KEY */ diff --git a/src/simplewallet/simplewallet.cpp b/src/simplewallet/simplewallet.cpp index c974313a0..f37d77933 100644 --- a/src/simplewallet/simplewallet.cpp +++ b/src/simplewallet/simplewallet.cpp @@ -223,7 +223,7 @@ namespace const char* USAGE_GET_TX_NOTE("get_tx_note "); const char* USAGE_GET_DESCRIPTION("get_description"); const char* USAGE_SET_DESCRIPTION("set_description [free text note]"); - const char* USAGE_SIGN("sign [,] [--spend|--view|--both] "); + const char* USAGE_SIGN("sign [,] [--spend|--view] "); const char* USAGE_VERIFY("verify
"); const char* USAGE_EXPORT_KEY_IMAGES("export_key_images [all] "); const char* USAGE_IMPORT_KEY_IMAGES("import_key_images "); @@ -9911,10 +9911,6 @@ bool simple_wallet::sign(const std::vector &args) { message_signature_type = tools::wallet2::sign_with_view_key; } - else if (args[idx] == "--both") - { - message_signature_type = tools::wallet2::sign_with_both_keys; - } else { fail_msg_writer() << tr("Invalid subaddress index format, and not a signature type: ") << args[idx]; @@ -9971,7 +9967,7 @@ bool simple_wallet::verify(const std::vector &args) } else { - success_msg_writer() << tr("Good signature from ") << address_string << (result.old ? " (using old signature algorithm)" : "") << " with " << (result.type == tools::wallet2::sign_with_spend_key ? "spend key" : result.type == tools::wallet2::sign_with_view_key ? "view key" : result.type == tools::wallet2::sign_with_both_keys ? "both spend and view keys" : "unknown key combination (suspicious)"); + success_msg_writer() << tr("Good signature from ") << address_string << (result.old ? " (using old signature algorithm)" : "") << " with " << (result.type == tools::wallet2::sign_with_spend_key ? "spend key" : result.type == tools::wallet2::sign_with_view_key ? "view key" : "unknown key combination (suspicious)"); } return true; } diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 653b4da12..918b3fd41 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -12207,11 +12207,16 @@ void wallet2::set_account_tag_description(const std::string& tag, const std::str m_account_tags.first[tag] = description; } -static crypto::hash get_message_hash(const std::string &data) +// Set up an address signature message hash +// Hash data: domain separator, spend public key, view public key, mode identifier, payload data +static crypto::hash get_message_hash(const std::string &data, const crypto::public_key &spend_key, const crypto::public_key &view_key, const uint8_t mode) { KECCAK_CTX ctx; keccak_init(&ctx); keccak_update(&ctx, (const uint8_t*)config::HASH_KEY_MESSAGE_SIGNING, sizeof(config::HASH_KEY_MESSAGE_SIGNING)); // includes NUL + keccak_update(&ctx, (const uint8_t*)&spend_key, sizeof(crypto::public_key)); + keccak_update(&ctx, (const uint8_t*)&view_key, sizeof(crypto::public_key)); + keccak_update(&ctx, (const uint8_t*)&mode, sizeof(uint8_t)); char len_buf[(sizeof(size_t) * 8 + 6) / 7]; char *ptr = len_buf; tools::write_varint(ptr, data.size()); @@ -12223,13 +12228,20 @@ static crypto::hash get_message_hash(const std::string &data) return hash; } +// Sign a message with a private key from either the base address or a subaddress +// The signature is also bound to both keys and the signature mode (spend, view) to prevent unintended reuse std::string wallet2::sign(const std::string &data, message_signature_type_t signature_type, cryptonote::subaddress_index index) const { - const crypto::hash hash = get_message_hash(data); const cryptonote::account_keys &keys = m_account.get_keys(); crypto::signature signature; crypto::secret_key skey, m; + crypto::secret_key skey_spend, skey_view; crypto::public_key pkey; + crypto::public_key pkey_spend, pkey_view; // to include both in hash + crypto::hash hash; + uint8_t mode; + + // Use the base address if (index.is_zero()) { switch (signature_type) @@ -12237,38 +12249,42 @@ std::string wallet2::sign(const std::string &data, message_signature_type_t sign case sign_with_spend_key: skey = keys.m_spend_secret_key; pkey = keys.m_account_address.m_spend_public_key; + mode = 0; break; case sign_with_view_key: skey = keys.m_view_secret_key; pkey = keys.m_account_address.m_view_public_key; + mode = 1; break; -#if 0 - case sign_with_both_keys: -#endif default: CHECK_AND_ASSERT_THROW_MES(false, "Invalid signature type requested"); } + hash = get_message_hash(data,keys.m_account_address.m_spend_public_key,keys.m_account_address.m_view_public_key,mode); } + // Use a subaddress else { + skey_spend = keys.m_spend_secret_key; + m = m_account.get_device().get_subaddress_secret_key(keys.m_view_secret_key, index); + sc_add((unsigned char*)&skey_spend, (unsigned char*)&m, (unsigned char*)&skey_spend); + secret_key_to_public_key(skey_spend,pkey_spend); + sc_mul((unsigned char*)&skey_view, (unsigned char*)&keys.m_view_secret_key, (unsigned char*)&skey_spend); + secret_key_to_public_key(skey_view,pkey_view); switch (signature_type) { case sign_with_spend_key: - skey = keys.m_spend_secret_key; - m = m_account.get_device().get_subaddress_secret_key(keys.m_view_secret_key, index); - sc_add((unsigned char*)&skey, (unsigned char*)&m, (unsigned char*)&skey); + skey = skey_spend; + pkey = pkey_spend; + mode = 0; break; case sign_with_view_key: - skey = keys.m_spend_secret_key; - m = m_account.get_device().get_subaddress_secret_key(keys.m_view_secret_key, index); - sc_add((unsigned char*)&skey, (unsigned char*)&m, (unsigned char*)&skey); - sc_mul((unsigned char*)&skey, (unsigned char*)&keys.m_view_secret_key, (unsigned char*)&skey); + skey = skey_view; + pkey = pkey_view; + mode = 1; break; -#if 0 - case sign_with_both_keys: skey = ...; break; -#endif default: CHECK_AND_ASSERT_THROW_MES(false, "Invalid signature type requested"); } secret_key_to_public_key(skey, pkey); + hash = get_message_hash(data,pkey_spend,pkey_view,mode); } crypto::generate_signature(hash, pkey, skey, signature); return std::string("SigV2") + tools::base58::encode(std::string((const char *)&signature, sizeof(signature))); @@ -12290,10 +12306,6 @@ tools::wallet2::message_signature_result_t wallet2::verify(const std::string &da { crypto::cn_fast_hash(data.data(), data.size(), hash); } - else - { - hash = get_message_hash(data); - } std::string decoded; if (!tools::base58::decode(signature.substr(v1 ? v1_header_len : v2_header_len), decoded)) { LOG_PRINT_L0("Signature decoding error"); @@ -12305,15 +12317,19 @@ tools::wallet2::message_signature_result_t wallet2::verify(const std::string &da return {}; } memcpy(&s, decoded.data(), sizeof(s)); + + // Test each mode and return which mode, if either, succeeded + if (v2) + hash = get_message_hash(data,address.m_spend_public_key,address.m_view_public_key,(uint8_t) 0); if (crypto::check_signature(hash, address.m_spend_public_key, s)) return {true, v1 ? 1u : 2u, !v2, sign_with_spend_key }; + + if (v2) + hash = get_message_hash(data,address.m_spend_public_key,address.m_view_public_key,(uint8_t) 1); if (crypto::check_signature(hash, address.m_view_public_key, s)) return {true, v1 ? 1u : 2u, !v2, sign_with_view_key }; -#if 0 - rct::key both = ...; - if (crypto::check_signature(hash, rct::rct2pk(both), s)) - return {true, v1 ? 1u : 2u, !v2, sign_with_both_keys }; -#endif + + // Both modes failed return {}; } diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h index 7a142d016..62ed111f1 100644 --- a/src/wallet/wallet2.h +++ b/src/wallet/wallet2.h @@ -1341,7 +1341,7 @@ private: */ void set_account_tag_description(const std::string& tag, const std::string& description); - enum message_signature_type_t { sign_with_spend_key, sign_with_view_key, sign_with_both_keys }; + enum message_signature_type_t { sign_with_spend_key, sign_with_view_key }; std::string sign(const std::string &data, message_signature_type_t signature_type, cryptonote::subaddress_index index = {0, 0}) const; struct message_signature_result_t { bool valid; unsigned version; bool old; message_signature_type_t type; }; message_signature_result_t verify(const std::string &data, const cryptonote::account_public_address &address, const std::string &signature) const; diff --git a/src/wallet/wallet_rpc_server.cpp b/src/wallet/wallet_rpc_server.cpp index ae5b692df..03db8b70f 100644 --- a/src/wallet/wallet_rpc_server.cpp +++ b/src/wallet/wallet_rpc_server.cpp @@ -2012,8 +2012,6 @@ namespace tools signature_type = tools::wallet2::sign_with_spend_key; else if (req.signature_type == "view") signature_type = tools::wallet2::sign_with_view_key; - else if (req.signature_type == "both") - signature_type = tools::wallet2::sign_with_both_keys; else { er.code = WALLET_RPC_ERROR_CODE_INVALID_SIGNATURE_TYPE; @@ -2063,7 +2061,6 @@ namespace tools { case tools::wallet2::sign_with_spend_key: res.signature_type = "spend"; break; case tools::wallet2::sign_with_view_key: res.signature_type = "view"; break; - case tools::wallet2::sign_with_both_keys: res.signature_type = "both"; break; default: res.signature_type = "invalid"; break; } return true; diff --git a/src/wallet/wallet_rpc_server_commands_defs.h b/src/wallet/wallet_rpc_server_commands_defs.h index 73904bae8..81f83fb18 100644 --- a/src/wallet/wallet_rpc_server_commands_defs.h +++ b/src/wallet/wallet_rpc_server_commands_defs.h @@ -47,7 +47,7 @@ // advance which version they will stop working with // Don't go over 32767 for any of these #define WALLET_RPC_VERSION_MAJOR 1 -#define WALLET_RPC_VERSION_MINOR 19 +#define WALLET_RPC_VERSION_MINOR 20 #define MAKE_WALLET_RPC_VERSION(major,minor) (((major)<<16)|(minor)) #define WALLET_RPC_VERSION MAKE_WALLET_RPC_VERSION(WALLET_RPC_VERSION_MAJOR, WALLET_RPC_VERSION_MINOR) namespace tools diff --git a/tests/functional_tests/sign_message.py b/tests/functional_tests/sign_message.py index 73c714450..dbb7cfd6d 100755 --- a/tests/functional_tests/sign_message.py +++ b/tests/functional_tests/sign_message.py @@ -100,6 +100,8 @@ class MessageSigningTest(): assert not res.good res = self.wallet[i].verify(message, address[0], signature + 'x') assert not res.good + res = self.wallet[i].verify(message, address[0], signature.replace('SigV2','SigV1')) + assert not res.good if __name__ == '__main__': MessageSigningTest().run_test()