Merge pull request #6493

921dd8dde Use domain-separated ChaCha20 for in-memory key encryption (Sarang Noether)
2024-11-17 08:17:37 +00:00 · 2020-08-16 12:39:58 -07:00 · 2020-08-16 12:39:58 -07:00 · 6d29ee6c52
commit 6d29ee6c52
parent e2e09d00c2 921dd8dde5
6 changed files with 173 additions and 51 deletions
--- a/src/cryptonote_basic/account.cpp
+++ b/src/cryptonote_basic/account.cpp
@ -61,7 +61,8 @@ DISABLE_VS_WARNINGS(4244 4345)
    m_device = &hwdev;
    MCDEBUG("device", "account_keys::set_device device type: "<<typeid(hwdev).name());
  }
-  //-----------------------------------------------------------------
+
+  // Generate a derived chacha key
  static void derive_key(const crypto::chacha_key &base_key, crypto::chacha_key &key)
  {
    static_assert(sizeof(base_key) == sizeof(crypto::hash), "chacha key and hash should be the same size");
@ -70,25 +71,38 @@ DISABLE_VS_WARNINGS(4244 4345)
    data[sizeof(base_key)] = config::HASH_KEY_MEMORY;
    crypto::generate_chacha_key(data.data(), sizeof(data), key, 1);
  }
-  //-----------------------------------------------------------------
-  static epee::wipeable_string get_key_stream(const crypto::chacha_key &base_key, const crypto::chacha_iv &iv, size_t bytes)
+  
+  // Prepare IVs and start chacha for encryption
+  void account_keys::encrypt_wrapper(const crypto::chacha_key &key, const bool all_keys)
  {
-    // derive a new key
-    crypto::chacha_key key;
-    derive_key(base_key, key);
+    // Set a fresh IV only for all-key encryption
+    if (all_keys)
+      m_encryption_iv = crypto::rand<crypto::chacha_iv>();

-    // chacha
-    epee::wipeable_string buffer0(std::string(bytes, '\0'));
-    epee::wipeable_string buffer1 = buffer0;
-    crypto::chacha20(buffer0.data(), buffer0.size(), key, iv, buffer1.data());
-    return buffer1;
+    // Now do the chacha
+    chacha_wrapper(key, all_keys);
  }
-  //-----------------------------------------------------------------
-  void account_keys::xor_with_key_stream(const crypto::chacha_key &key)
+
+  // Start chacha for decryption
+  void account_keys::decrypt_wrapper(const crypto::chacha_key &key, const bool all_keys)
  {
-    // encrypt a large enough byte stream with chacha20
-    epee::wipeable_string key_stream = get_key_stream(key, m_encryption_iv, sizeof(crypto::secret_key) * (2 + m_multisig_keys.size()));
-    const char *ptr = key_stream.data();
+    chacha_wrapper(key, all_keys);
+  }
+
+  // Decrypt keys using the legacy method
+  void account_keys::decrypt_legacy(const crypto::chacha_key &key)
+  {
+    // Derive domain-separated chacha key
+    crypto::chacha_key derived_key;
+    derive_key(key, derived_key);
+
+    // Build key stream
+    epee::wipeable_string temp(std::string(sizeof(crypto::secret_key)*(2 + m_multisig_keys.size()), '\0'));
+    epee::wipeable_string stream = temp;
+    crypto::chacha20(temp.data(), temp.size(), derived_key, m_encryption_iv, stream.data());
+
+    // Decrypt all keys
+    const char *ptr = stream.data();
    for (size_t i = 0; i < sizeof(crypto::secret_key); ++i)
      m_spend_secret_key.data[i] ^= *ptr++;
    for (size_t i = 0; i < sizeof(crypto::secret_key); ++i)
@ -99,33 +113,39 @@ DISABLE_VS_WARNINGS(4244 4345)
        k.data[i] ^= *ptr++;
    }
  }
-  //-----------------------------------------------------------------
-  void account_keys::encrypt(const crypto::chacha_key &key)
+  
+  // Perform chacha on either the view key or all keys
+  void account_keys::chacha_wrapper(const crypto::chacha_key &key, const bool all_keys)
  {
-    m_encryption_iv = crypto::rand<crypto::chacha_iv>();
-    xor_with_key_stream(key);
+    // Derive domain-seprated chacha key
+    crypto::chacha_key derived_key;
+    derive_key(key, derived_key);
+
+    // Chacha the specified keys using the appropriate IVs
+    if (all_keys)
+    {
+      // Spend key
+      crypto::secret_key temp_key;
+      chacha20((char *) &m_spend_secret_key, sizeof(crypto::secret_key), derived_key, m_encryption_iv, (char *) &temp_key);
+      memcpy(&m_spend_secret_key, &temp_key, sizeof(crypto::secret_key));
+      memwipe(&temp_key, sizeof(crypto::secret_key));
+
+      // Multisig keys
+      std::vector<crypto::secret_key> temp_keys;
+      temp_keys.reserve(m_multisig_keys.size());
+      temp_keys.resize(m_multisig_keys.size());
+      chacha20((char *) &m_multisig_keys[0], sizeof(crypto::secret_key)*m_multisig_keys.size(), derived_key, m_encryption_iv, (char *) &temp_keys[0]);
+      memcpy(&m_multisig_keys[0], &temp_keys[0], sizeof(crypto::secret_key)*temp_keys.size());
+      memwipe(&temp_keys[0], sizeof(crypto::secret_key)*temp_keys.size());
+    }
+
+    // View key
+    crypto::secret_key temp_key;
+    chacha20((char *) &m_view_secret_key, sizeof(crypto::secret_key), derived_key, m_encryption_iv, (char *) &temp_key);
+    memcpy(&m_view_secret_key, &temp_key, sizeof(crypto::secret_key));
+    memwipe(&temp_key, sizeof(crypto::secret_key));
  }
-  //-----------------------------------------------------------------
-  void account_keys::decrypt(const crypto::chacha_key &key)
-  {
-    xor_with_key_stream(key);
-  }
-  //-----------------------------------------------------------------
-  void account_keys::encrypt_viewkey(const crypto::chacha_key &key)
-  {
-    // encrypt a large enough byte stream with chacha20
-    epee::wipeable_string key_stream = get_key_stream(key, m_encryption_iv, sizeof(crypto::secret_key) * 2);
-    const char *ptr = key_stream.data();
-    ptr += sizeof(crypto::secret_key);
-    for (size_t i = 0; i < sizeof(crypto::secret_key); ++i)
-      m_view_secret_key.data[i] ^= *ptr++;
-  }
-  //-----------------------------------------------------------------
-  void account_keys::decrypt_viewkey(const crypto::chacha_key &key)
-  {
-    encrypt_viewkey(key);
-  }
-  //-----------------------------------------------------------------
+
  account_base::account_base()
  {
    set_null();
--- a/src/cryptonote_basic/account.h
+++ b/src/cryptonote_basic/account.h
@ -57,16 +57,15 @@ namespace cryptonote

    account_keys& operator=(account_keys const&) = default;

-    void encrypt(const crypto::chacha_key &key);
-    void decrypt(const crypto::chacha_key &key);
-    void encrypt_viewkey(const crypto::chacha_key &key);
-    void decrypt_viewkey(const crypto::chacha_key &key);
+    void encrypt_wrapper(const crypto::chacha_key &key, const bool all_keys);
+    void decrypt_wrapper(const crypto::chacha_key &key, const bool all_keys);
+    void decrypt_legacy(const crypto::chacha_key &key);

    hw::device& get_device()  const ;
    void set_device( hw::device &hwdev) ;

  private:
-    void xor_with_key_stream(const crypto::chacha_key &key);
+    void chacha_wrapper(const crypto::chacha_key &key, const bool all_keys);
  };

  /************************************************************************/
@ -100,10 +99,12 @@ namespace cryptonote
    void forget_spend_key();
    const std::vector<crypto::secret_key> &get_multisig_keys() const { return m_keys.m_multisig_keys; }

-    void encrypt_keys(const crypto::chacha_key &key) { m_keys.encrypt(key); }
-    void decrypt_keys(const crypto::chacha_key &key) { m_keys.decrypt(key); }
-    void encrypt_viewkey(const crypto::chacha_key &key) { m_keys.encrypt_viewkey(key); }
-    void decrypt_viewkey(const crypto::chacha_key &key) { m_keys.decrypt_viewkey(key); }
+    void encrypt_keys(const crypto::chacha_key &key) { m_keys.encrypt_wrapper(key, true); }
+    void encrypt_keys_same_iv(const crypto::chacha_key &key) { m_keys.decrypt_wrapper(key, true); } // encryption with the same IV is the same as decryption due to symmetry
+    void decrypt_keys(const crypto::chacha_key &key) { m_keys.decrypt_wrapper(key, true); }
+    void encrypt_viewkey(const crypto::chacha_key &key) { m_keys.encrypt_wrapper(key, false); }
+    void decrypt_viewkey(const crypto::chacha_key &key) { m_keys.decrypt_wrapper(key, false); }
+    void decrypt_legacy(const crypto::chacha_key &key) { m_keys.decrypt_legacy(key); }

    template <class t_archive>
    inline void serialize(t_archive &a, const unsigned int /*ver*/)
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@ -4349,9 +4349,24 @@ bool wallet2::load_keys_buf(const std::string& keys_buf, const epee::wipeable_st

  if (r)
  {
+    // Decrypt keys, using one of two possible methods
    if (encrypted_secret_keys)
    {
+      // First try the updated method
      m_account.decrypt_keys(key);
+      load_info.is_legacy_key_encryption = false;
+
+      // Test address construction to see if decryption succeeded
+      const cryptonote::account_keys &keys = m_account.get_keys();
+      hw::device &hwdev = m_account.get_device();
+      if (!hwdev.verify_keys(keys.m_view_secret_key, keys.m_account_address.m_view_public_key) || !hwdev.verify_keys(keys.m_spend_secret_key, keys.m_account_address.m_spend_public_key))
+      {
+        // Updated method failed; try the legacy method
+        // Note that we must first encrypt the keys again with the same IV
+        m_account.encrypt_keys_same_iv(key);
+        m_account.decrypt_legacy(key);
+        load_info.is_legacy_key_encryption = true;
+      }
    }
    else
    {
@ -5555,6 +5570,7 @@ void wallet2::load(const std::string& wallet_, const epee::wipeable_string& pass
 {
  clear();
  prepare_file_names(wallet_);
+  MINFO("Keys file: " << m_keys_file);

  // determine if loading from file system or string buffer
  bool use_fs = !wallet_.empty();
--- a/src/wallet/wallet2.h
+++ b/src/wallet/wallet2.h
@ -219,6 +219,15 @@ private:
    friend class wallet_keys_unlocker;
    friend class wallet_device_callback;
  public:
+    // Contains data on how keys were loaded, primarily for unit test purposes
+    struct load_info_t {
+        bool is_legacy_key_encryption;
+    };
+
+    const load_info_t &get_load_info() const {
+        return load_info;
+    }
+
    static constexpr const std::chrono::seconds rpc_timeout = std::chrono::minutes(3) + std::chrono::seconds(30);

    enum RefreshType {
@ -1407,6 +1416,8 @@ private:
    static std::string get_default_daemon_address() { CRITICAL_REGION_LOCAL(default_daemon_address_lock); return default_daemon_address; }

  private:
+    load_info_t load_info;
+
    /*!
     * \brief  Stores wallet information to wallet file.
     * \param  keys_file_name Name of wallet file
--- a/tests/unit_tests/account.cpp
+++ b/tests/unit_tests/account.cpp
@ -29,14 +29,30 @@
 #include "gtest/gtest.h"

 #include "cryptonote_basic/account.h"
+#include "ringct/rctOps.h"

+// Tests in-memory encryption of account secret keys
 TEST(account, encrypt_keys)
 {
+  // Generate account keys and random multisig keys
  cryptonote::keypair recovery_key = cryptonote::keypair::generate(hw::get_device("default"));
  cryptonote::account_base account;
  crypto::secret_key key = account.generate(recovery_key.sec);
-  const cryptonote::account_keys keys = account.get_keys();

+  const size_t n_multisig = 4;
+  std::vector<crypto::secret_key> multisig_keys;
+  multisig_keys.reserve(n_multisig);
+  multisig_keys.resize(0);
+  for (size_t i = 0; i < n_multisig; ++i)
+  {
+    multisig_keys.push_back(rct::rct2sk(rct::skGen()));
+  }
+  ASSERT_TRUE(account.make_multisig(account.get_keys().m_view_secret_key, account.get_keys().m_spend_secret_key, account.get_keys().m_account_address.m_spend_public_key, multisig_keys));
+
+  const cryptonote::account_keys keys = account.get_keys();
+  ASSERT_EQ(keys.m_multisig_keys.size(),n_multisig);
+
+  // Encrypt and decrypt keys
  ASSERT_EQ(account.get_keys().m_account_address, keys.m_account_address);
  ASSERT_EQ(account.get_keys().m_spend_secret_key, keys.m_spend_secret_key);
  ASSERT_EQ(account.get_keys().m_view_secret_key, keys.m_view_secret_key);
@ -50,22 +66,40 @@ TEST(account, encrypt_keys)
  ASSERT_EQ(account.get_keys().m_account_address, keys.m_account_address);
  ASSERT_NE(account.get_keys().m_spend_secret_key, keys.m_spend_secret_key);
  ASSERT_NE(account.get_keys().m_view_secret_key, keys.m_view_secret_key);
+  ASSERT_NE(account.get_keys().m_multisig_keys, keys.m_multisig_keys);

  account.decrypt_viewkey(chacha_key);

  ASSERT_EQ(account.get_keys().m_account_address, keys.m_account_address);
  ASSERT_NE(account.get_keys().m_spend_secret_key, keys.m_spend_secret_key);
  ASSERT_EQ(account.get_keys().m_view_secret_key, keys.m_view_secret_key);
+  ASSERT_NE(account.get_keys().m_multisig_keys, keys.m_multisig_keys);

  account.encrypt_viewkey(chacha_key);

  ASSERT_EQ(account.get_keys().m_account_address, keys.m_account_address);
  ASSERT_NE(account.get_keys().m_spend_secret_key, keys.m_spend_secret_key);
  ASSERT_NE(account.get_keys().m_view_secret_key, keys.m_view_secret_key);
+  ASSERT_NE(account.get_keys().m_multisig_keys, keys.m_multisig_keys);
+
+  account.decrypt_viewkey(chacha_key);
+
+  ASSERT_EQ(account.get_keys().m_account_address, keys.m_account_address);
+  ASSERT_NE(account.get_keys().m_spend_secret_key, keys.m_spend_secret_key);
+  ASSERT_EQ(account.get_keys().m_view_secret_key, keys.m_view_secret_key);
+  ASSERT_NE(account.get_keys().m_multisig_keys, keys.m_multisig_keys);
+
+  account.encrypt_viewkey(chacha_key);
+
+  ASSERT_EQ(account.get_keys().m_account_address, keys.m_account_address);
+  ASSERT_NE(account.get_keys().m_spend_secret_key, keys.m_spend_secret_key);
+  ASSERT_NE(account.get_keys().m_view_secret_key, keys.m_view_secret_key);
+  ASSERT_NE(account.get_keys().m_multisig_keys, keys.m_multisig_keys);

  account.decrypt_keys(chacha_key);

  ASSERT_EQ(account.get_keys().m_account_address, keys.m_account_address);
  ASSERT_EQ(account.get_keys().m_spend_secret_key, keys.m_spend_secret_key);
  ASSERT_EQ(account.get_keys().m_view_secret_key, keys.m_view_secret_key);
+  ASSERT_EQ(account.get_keys().m_multisig_keys, keys.m_multisig_keys);
 }
--- a/tests/unit_tests/serialization.cpp
+++ b/tests/unit_tests/serialization.cpp
@ -616,6 +616,46 @@ TEST(Serialization, serializes_ringct_types)
  ASSERT_EQ(bp0, bp1);
 }

+TEST(Serialization, key_encryption_transition)
+{
+  const cryptonote::network_type nettype = cryptonote::TESTNET;
+  tools::wallet2 w(nettype);
+  const boost::filesystem::path wallet_file = unit_test::data_dir / "wallet_9svHk1";
+  const boost::filesystem::path key_file = unit_test::data_dir / "wallet_9svHk1.keys";
+  const boost::filesystem::path temp_wallet_file = unit_test::data_dir / "wallet_9svHk1_temp";
+  const boost::filesystem::path temp_key_file = unit_test::data_dir / "wallet_9svHk1_temp.keys";
+  string password = "test";
+  bool r = false;
+
+  // Copy the original files for this test
+  boost::filesystem::copy(wallet_file,temp_wallet_file);
+  boost::filesystem::copy(key_file,temp_key_file);
+
+  try
+  {
+    // Key transition
+    w.load(temp_wallet_file.string(), password); // legacy decryption method
+    ASSERT_TRUE(w.get_load_info().is_legacy_key_encryption);
+    const crypto::secret_key view_secret_key = w.get_account().get_keys().m_view_secret_key;
+
+    w.rewrite(temp_wallet_file.string(), password); // transition to new key format
+
+    w.load(temp_wallet_file.string(), password); // new decryption method
+    ASSERT_FALSE(w.get_load_info().is_legacy_key_encryption);
+    ASSERT_EQ(w.get_account().get_keys().m_view_secret_key,view_secret_key);
+
+    r = true;
+  }
+  catch (const exception& e)
+  {}
+
+  // Remove the temporary files
+  boost::filesystem::remove(temp_wallet_file);
+  boost::filesystem::remove(temp_key_file);
+
+  ASSERT_TRUE(r);
+}
+
 TEST(Serialization, portability_wallet)
 {
  const cryptonote::network_type nettype = cryptonote::TESTNET;