Bind signature to full address and signing mode

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<crypto::public_key>  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<crypto::public_key>  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<crypto::public_key>  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 <txid>");
   const char* USAGE_GET_DESCRIPTION("get_description");
   const char* USAGE_SET_DESCRIPTION("set_description [free text note]");
-  const char* USAGE_SIGN("sign [<account_index>,<address_index>] [--spend|--view|--both] <filename>");
+  const char* USAGE_SIGN("sign [<account_index>,<address_index>] [--spend|--view] <filename>");
   const char* USAGE_VERIFY("verify <filename> <address> <signature>");
   const char* USAGE_EXPORT_KEY_IMAGES("export_key_images [all] <filename>");
   const char* USAGE_IMPORT_KEY_IMAGES("import_key_images <filename>");
@@ -9911,10 +9911,6 @@ bool simple_wallet::sign(const std::vector<std::string> &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<std::string> &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()