Merge pull request #4306

56b50faa wallet: use wipeable_string in more places where a secret is used (moneromooo-monero)
07ec748c wipeable_string: add hex_to_pod function (moneromooo-monero)

6 files changed, 69 insertions, 39 deletions

diff --git a/contrib/epee/include/hex.h b/contrib/epee/include/hex.h
index 02600c320..901c666a9 100644
--- a/contrib/epee/include/hex.h
+++ b/contrib/epee/include/hex.h
@@ -44,6 +44,7 @@ namespace epee
     static std::string string(const span<const std::uint8_t> src);
     //! \return A epee::wipeable_string containing hex of `src`.
     static epee::wipeable_string wipeable_string(const span<const std::uint8_t> src);
+    template<typename T> static epee::wipeable_string wipeable_string(const T &pod) { return wipeable_string(span<const uint8_t>((const uint8_t*)&pod, sizeof(pod))); }
 
     //! \return An array containing hex of `src`.
     template<std::size_t N>
diff --git a/contrib/epee/include/wipeable_string.h b/contrib/epee/include/wipeable_string.h
index 4cebe5fdf..31854fe2e 100644
--- a/contrib/epee/include/wipeable_string.h
+++ b/contrib/epee/include/wipeable_string.h
@@ -28,10 +28,11 @@
 
 #pragma once
 
-#include <boost/optional/optional_fwd.hpp>
+#include <boost/optional/optional.hpp>
 #include <stddef.h>
 #include <vector>
 #include <string>
+#include "memwipe.h"
 #include "fnv1.h"
 
 namespace epee
@@ -65,6 +66,8 @@ namespace epee
     void trim();
     void split(std::vector<wipeable_string> &fields) const;
     boost::optional<wipeable_string> parse_hexstr() const;
+    template<typename T> inline bool hex_to_pod(T &pod) const;
+    template<typename T> inline bool hex_to_pod(tools::scrubbed<T> &pod) const { return hex_to_pod(unwrap(pod)); }
     void resize(size_t sz);
     void reserve(size_t sz);
     void clear();
@@ -79,6 +82,20 @@ namespace epee
   private:
     std::vector<char> buffer;
   };
+
+  template<typename T> inline bool wipeable_string::hex_to_pod(T &pod) const
+  {
+    static_assert(std::is_pod<T>::value, "expected pod type");
+    if (size() != sizeof(T) * 2)
+      return false;
+    boost::optional<epee::wipeable_string> blob = parse_hexstr();
+    if (!blob)
+      return false;
+    if (blob->size() != sizeof(T))
+      return false;
+    pod = *(const T*)blob->data();
+    return true;
+  }
 }
 
 namespace std
diff --git a/contrib/epee/src/wipeable_string.cpp b/contrib/epee/src/wipeable_string.cpp
index 7c9722765..69f92e106 100644
--- a/contrib/epee/src/wipeable_string.cpp
+++ b/contrib/epee/src/wipeable_string.cpp
@@ -32,6 +32,8 @@
 #include "misc_log_ex.h"
 #include "wipeable_string.h"
 
+static constexpr const char hex[] = u8"0123456789abcdef";
+
 namespace
 {
   int atolower(int c)
@@ -197,7 +199,6 @@ boost::optional<epee::wipeable_string> wipeable_string::parse_hexstr() const
   const size_t len = size();
   const char *d = data();
   res->grow(0, len / 2);
-  static constexpr const char hex[] = u8"0123456789abcdef";
   for (size_t i = 0; i < len; i += 2)
   {
     char c = atolower(d[i]);
diff --git a/src/simplewallet/simplewallet.cpp b/src/simplewallet/simplewallet.cpp
index 4046f5fce..391d1f03e 100644
--- a/src/simplewallet/simplewallet.cpp
+++ b/src/simplewallet/simplewallet.cpp
@@ -3014,20 +3014,19 @@ bool simple_wallet::init(const boost::program_options::variables_map& vm)
       }
 
       // parse view secret key
-      std::string viewkey_string = input_line("View key: ");
+      epee::wipeable_string viewkey_string = input_secure_line("Secret view key: ");
       if (std::cin.eof())
         return false;
       if (viewkey_string.empty()) {
         fail_msg_writer() << tr("No data supplied, cancelled");
         return false;
       }
-      cryptonote::blobdata viewkey_data;
-      if(!epee::string_tools::parse_hexstr_to_binbuff(viewkey_string, viewkey_data) || viewkey_data.size() != sizeof(crypto::secret_key))
+      crypto::secret_key viewkey;
+      if (viewkey_string.hex_to_pod(unwrap(unwrap(viewkey))))
       {
         fail_msg_writer() << tr("failed to parse view key secret key");
         return false;
       }
-      crypto::secret_key viewkey = *reinterpret_cast<const crypto::secret_key*>(viewkey_data.data());
 
       m_wallet_file=m_generate_from_view_key;
 
@@ -3050,14 +3049,14 @@ bool simple_wallet::init(const boost::program_options::variables_map& vm)
     {
       m_wallet_file = m_generate_from_spend_key;
       // parse spend secret key
-      std::string spendkey_string = input_line("Secret spend key: ");
+      epee::wipeable_string spendkey_string = input_secure_line("Secret spend key: ");
       if (std::cin.eof())
         return false;
       if (spendkey_string.empty()) {
         fail_msg_writer() << tr("No data supplied, cancelled");
         return false;
       }
-      if (!epee::string_tools::hex_to_pod(spendkey_string, m_recovery_key))
+      if (!spendkey_string.hex_to_pod(unwrap(unwrap(m_recovery_key))))
       {
         fail_msg_writer() << tr("failed to parse spend key secret key");
         return false;
@@ -3090,36 +3089,34 @@ bool simple_wallet::init(const boost::program_options::variables_map& vm)
       }
 
       // parse spend secret key
-      std::string spendkey_string = input_line("Secret spend key: ");
+      epee::wipeable_string spendkey_string = input_secure_line("Secret spend key: ");
       if (std::cin.eof())
         return false;
       if (spendkey_string.empty()) {
         fail_msg_writer() << tr("No data supplied, cancelled");
         return false;
       }
-      cryptonote::blobdata spendkey_data;
-      if(!epee::string_tools::parse_hexstr_to_binbuff(spendkey_string, spendkey_data) || spendkey_data.size() != sizeof(crypto::secret_key))
+      crypto::secret_key spendkey;
+      if (!spendkey_string.hex_to_pod(unwrap(unwrap(spendkey))))
       {
         fail_msg_writer() << tr("failed to parse spend key secret key");
         return false;
       }
-      crypto::secret_key spendkey = *reinterpret_cast<const crypto::secret_key*>(spendkey_data.data());
 
       // parse view secret key
-      std::string viewkey_string = input_line("Secret view key: ");
+      epee::wipeable_string viewkey_string = input_secure_line("Secret view key: ");
       if (std::cin.eof())
         return false;
       if (viewkey_string.empty()) {
         fail_msg_writer() << tr("No data supplied, cancelled");
         return false;
       }
-      cryptonote::blobdata viewkey_data;
-      if(!epee::string_tools::parse_hexstr_to_binbuff(viewkey_string, viewkey_data) || viewkey_data.size() != sizeof(crypto::secret_key))
+      crypto::secret_key viewkey;
+      if(!viewkey_string.hex_to_pod(unwrap(unwrap(viewkey))))
       {
         fail_msg_writer() << tr("failed to parse view key secret key");
         return false;
       }
-      crypto::secret_key viewkey = *reinterpret_cast<const crypto::secret_key*>(viewkey_data.data());
 
       m_wallet_file=m_generate_from_keys;
 
@@ -3195,7 +3192,7 @@ bool simple_wallet::init(const boost::program_options::variables_map& vm)
       }
       
       // parse secret view key
-      std::string viewkey_string = input_line("Secret view key: ");
+      epee::wipeable_string viewkey_string = input_secure_line("Secret view key: ");
       if (std::cin.eof())
         return false;
       if (viewkey_string.empty())
@@ -3203,13 +3200,12 @@ bool simple_wallet::init(const boost::program_options::variables_map& vm)
         fail_msg_writer() << tr("No data supplied, cancelled");
         return false;
       }
-      cryptonote::blobdata viewkey_data;
-      if(!epee::string_tools::parse_hexstr_to_binbuff(viewkey_string, viewkey_data) || viewkey_data.size() != sizeof(crypto::secret_key))
+      crypto::secret_key viewkey;
+      if(!viewkey_string.hex_to_pod(unwrap(unwrap(viewkey))))
       {
         fail_msg_writer() << tr("failed to parse secret view key");
         return false;
       }
-      crypto::secret_key viewkey = *reinterpret_cast<const crypto::secret_key*>(viewkey_data.data());
       
       // check that the view key matches the given address
       crypto::public_key pkey;
@@ -3230,12 +3226,12 @@ bool simple_wallet::init(const boost::program_options::variables_map& vm)
       if(multisig_m == multisig_n)
       {
         std::vector<crypto::secret_key> multisig_secret_spendkeys(multisig_n);
-        std::string spendkey_string;
+        epee::wipeable_string spendkey_string;
         cryptonote::blobdata spendkey_data;
         // get N secret spend keys from user
         for(unsigned int i=0; i<multisig_n; ++i)
         {
-          spendkey_string = input_line(tr((boost::format(tr("Secret spend key (%u of %u):")) % (i+1) % multisig_m).str().c_str()));
+          spendkey_string = input_secure_line(tr((boost::format(tr("Secret spend key (%u of %u):")) % (i+1) % multisig_m).str().c_str()));
           if (std::cin.eof())
             return false;
           if (spendkey_string.empty())
@@ -3243,12 +3239,11 @@ bool simple_wallet::init(const boost::program_options::variables_map& vm)
             fail_msg_writer() << tr("No data supplied, cancelled");
             return false;
           }
-          if(!epee::string_tools::parse_hexstr_to_binbuff(spendkey_string, spendkey_data) || spendkey_data.size() != sizeof(crypto::secret_key))
+          if(!spendkey_string.hex_to_pod(unwrap(unwrap(multisig_secret_spendkeys[i]))))
           {
             fail_msg_writer() << tr("failed to parse spend key secret key");
             return false;
           }
-          multisig_secret_spendkeys[i] = *reinterpret_cast<const crypto::secret_key*>(spendkey_data.data());
         }
         
         // sum the spend keys together to get the master spend key
diff --git a/src/wallet/wallet_rpc_server.cpp b/src/wallet/wallet_rpc_server.cpp
index 121c67549..86b46b173 100644
--- a/src/wallet/wallet_rpc_server.cpp
+++ b/src/wallet/wallet_rpc_server.cpp
@@ -760,10 +760,10 @@ namespace tools
     {
       if (get_tx_key)
       {
-        std::string s = epee::string_tools::pod_to_hex(ptx.tx_key);
+        epee::wipeable_string s = epee::to_hex::wipeable_string(ptx.tx_key);
         for (const crypto::secret_key& additional_tx_key : ptx.additional_tx_keys)
-          s += epee::string_tools::pod_to_hex(additional_tx_key);
-        fill(tx_key, s);
+          s += epee::to_hex::wipeable_string(additional_tx_key);
+        fill(tx_key, std::string(s.data(), s.size()));
       }
       // Compute amount leaving wallet in tx. By convention dests does not include change outputs
       fill(amount, total_amount(ptx));
@@ -1570,11 +1570,13 @@ namespace tools
       }
       else if(req.key_type.compare("view_key") == 0)
       {
-          res.key = string_tools::pod_to_hex(m_wallet->get_account().get_keys().m_view_secret_key);
+          epee::wipeable_string key = epee::to_hex::wipeable_string(m_wallet->get_account().get_keys().m_view_secret_key);
+          res.key = std::string(key.data(), key.size());
       }
       else if(req.key_type.compare("spend_key") == 0)
       {
-          res.key = string_tools::pod_to_hex(m_wallet->get_account().get_keys().m_spend_secret_key);
+          epee::wipeable_string key = epee::to_hex::wipeable_string(m_wallet->get_account().get_keys().m_spend_secret_key);
+          res.key = std::string(key.data(), key.size());
       }
       else
       {
@@ -1800,11 +1802,11 @@ namespace tools
       return false;
     }
 
-    std::ostringstream oss;
-    oss << epee::string_tools::pod_to_hex(tx_key);
+    epee::wipeable_string s;
+    s += epee::to_hex::wipeable_string(tx_key);
     for (size_t i = 0; i < additional_tx_keys.size(); ++i)
-      oss << epee::string_tools::pod_to_hex(additional_tx_keys[i]);
-    res.tx_key = oss.str();
+      s += epee::to_hex::wipeable_string(additional_tx_keys[i]);
+    res.tx_key = std::string(s.data(), s.size());
     return true;
   }
   //------------------------------------------------------------------------------------------------------------------------------
@@ -1820,26 +1822,33 @@ namespace tools
       return false;
     }
 
-    std::string tx_key_str = req.tx_key;
+    epee::wipeable_string tx_key_str = req.tx_key;
+    if (tx_key_str.size() < 64 || tx_key_str.size() % 64)
+    {
+      er.code = WALLET_RPC_ERROR_CODE_WRONG_KEY;
+      er.message = "Tx key has invalid format";
+      return false;
+    }
+    const char *data = tx_key_str.data();
     crypto::secret_key tx_key;
-    if (!epee::string_tools::hex_to_pod(tx_key_str.substr(0, 64), tx_key))
+    if (!epee::wipeable_string(data, 64).hex_to_pod(unwrap(unwrap(tx_key))))
     {
       er.code = WALLET_RPC_ERROR_CODE_WRONG_KEY;
       er.message = "Tx key has invalid format";
       return false;
     }
-    tx_key_str = tx_key_str.substr(64);
+    size_t offset = 64;
     std::vector<crypto::secret_key> additional_tx_keys;
-    while (!tx_key_str.empty())
+    while (offset < tx_key_str.size())
     {
       additional_tx_keys.resize(additional_tx_keys.size() + 1);
-      if (!epee::string_tools::hex_to_pod(tx_key_str.substr(0, 64), additional_tx_keys.back()))
+      if (!epee::wipeable_string(data + offset, 64).hex_to_pod(unwrap(unwrap(additional_tx_keys.back()))))
       {
         er.code = WALLET_RPC_ERROR_CODE_WRONG_KEY;
         er.message = "Tx key has invalid format";
         return false;
       }
-      tx_key_str = tx_key_str.substr(64);
+      offset += 64;
     }
 
     cryptonote::address_parse_info info;
diff --git a/tests/unit_tests/wipeable_string.cpp b/tests/unit_tests/wipeable_string.cpp
index 5ea1c1729..65718fd45 100644
--- a/tests/unit_tests/wipeable_string.cpp
+++ b/tests/unit_tests/wipeable_string.cpp
@@ -32,6 +32,7 @@
 
 #include "misc_log_ex.h"
 #include "wipeable_string.h"
+#include "hex.h"
 
 TEST(wipeable_string, ctor)
 {
@@ -202,3 +203,9 @@ TEST(wipeable_string, parse_hexstr)
   ASSERT_TRUE((s = epee::wipeable_string("414243").parse_hexstr()));
   ASSERT_EQ(*s, epee::wipeable_string("ABC"));
 }
+
+TEST(wipeable_string, to_hex)
+{
+  ASSERT_TRUE(epee::to_hex::wipeable_string(epee::span<const uint8_t>((const uint8_t*)"", 0)) == epee::wipeable_string(""));
+  ASSERT_TRUE(epee::to_hex::wipeable_string(epee::span<const uint8_t>((const uint8_t*)"abc", 3)) == epee::wipeable_string("616263"));
+}