Merge pull request #2857

7193b89f Scrub keys from memory just before scope end. (moneromooo-monero)

6 files changed, 60 insertions, 23 deletions

diff --git a/contrib/epee/include/span.h b/contrib/epee/include/span.h
index ea4ba63dd..5660c09d1 100644
--- a/contrib/epee/include/span.h
+++ b/contrib/epee/include/span.h
@@ -108,7 +108,7 @@ namespace epee
   template<typename T>
   constexpr bool has_padding() noexcept
   {
-    return !std::is_pod<T>() || alignof(T) != 1;
+    return !std::is_pod<T>::value || alignof(T) != 1;
   }
 
   //! \return Cast data from `src` as `span<const std::uint8_t>`.
diff --git a/src/common/memwipe.h b/src/common/memwipe.h
index e9a3fba7b..c3b4ce8ab 100644
--- a/src/common/memwipe.h
+++ b/src/common/memwipe.h
@@ -31,6 +31,8 @@
 #pragma once
 
 #ifdef __cplusplus
+#include <array>
+
 extern "C" {
 #endif
 
@@ -39,3 +41,44 @@ void *memwipe(void *src, size_t n);
 #ifdef __cplusplus
 }
 #endif
+
+#ifdef __cplusplus
+namespace tools {
+
+  /// Scrubs data in the contained type upon destruction.
+  ///
+  /// Primarily useful for making sure that private keys don't stick around in
+  /// memory after the objects that held them have gone out of scope.
+  template <class T>
+  struct scrubbed : public T {
+    using type = T;
+
+    ~scrubbed() {
+      scrub();
+    }
+
+    /// Destroy the contents of the contained type.
+    void scrub() {
+      static_assert(std::is_pod<T>::value,
+                    "T cannot be auto-scrubbed. T must be POD.");
+      static_assert(std::is_trivially_destructible<T>::value,
+                    "T cannot be auto-scrubbed. T must be trivially destructable.");
+      memwipe(this, sizeof(T));
+    }
+  };
+
+  template <class T, size_t N>
+  using scrubbed_arr = scrubbed<std::array<T, N>>;
+} // namespace tools
+
+// Partial specialization for std::is_pod<tools::scrubbed<T>> so that it can
+// pretend to be the containted type in those contexts.
+namespace std
+{
+  template<class t_scrubbee>
+  struct is_pod<tools::scrubbed<t_scrubbee>> {
+    static const bool value = is_pod<t_scrubbee>::value;
+  };
+}
+
+#endif // __cplusplus
diff --git a/src/crypto/chacha8.h b/src/crypto/chacha8.h
index 1bf695731..dcbe6a933 100644
--- a/src/crypto/chacha8.h
+++ b/src/crypto/chacha8.h
@@ -49,16 +49,9 @@ namespace crypto {
 #if defined(__cplusplus)
   }
 
-#pragma pack(push, 1)
-  struct chacha8_key {
-    uint8_t data[CHACHA8_KEY_SIZE];
-
-    ~chacha8_key()
-    {
-      memwipe(data, sizeof(data));
-    }
-  };
+  using chacha8_key = tools::scrubbed_arr<uint8_t, CHACHA8_KEY_SIZE>;
 
+#pragma pack(push, 1)
   // MS VC 2012 doesn't interpret `class chacha8_iv` as POD in spite of [9.0.10], so it is a struct
   struct chacha8_iv {
     uint8_t data[CHACHA8_IV_SIZE];
@@ -68,15 +61,14 @@ namespace crypto {
   static_assert(sizeof(chacha8_key) == CHACHA8_KEY_SIZE && sizeof(chacha8_iv) == CHACHA8_IV_SIZE, "Invalid structure size");
 
   inline void chacha8(const void* data, std::size_t length, const chacha8_key& key, const chacha8_iv& iv, char* cipher) {
-    chacha8(data, length, reinterpret_cast<const uint8_t*>(&key), reinterpret_cast<const uint8_t*>(&iv), cipher);
+    chacha8(data, length, key.data(), reinterpret_cast<const uint8_t*>(&iv), cipher);
   }
 
   inline void generate_chacha8_key(const void *data, size_t size, chacha8_key& key) {
     static_assert(sizeof(chacha8_key) <= sizeof(hash), "Size of hash must be at least that of chacha8_key");
-    char pwd_hash[HASH_SIZE];
-    crypto::cn_slow_hash(data, size, pwd_hash);
-    memcpy(&key, pwd_hash, sizeof(key));
-    memwipe(pwd_hash, sizeof(pwd_hash));
+    tools::scrubbed_arr<char, HASH_SIZE> pwd_hash;
+    crypto::cn_slow_hash(data, size, pwd_hash.data());
+    memcpy(&key, pwd_hash.data(), sizeof(key));
   }
 
   inline void generate_chacha8_key(std::string password, chacha8_key& key) {
diff --git a/src/crypto/crypto.h b/src/crypto/crypto.h
index abdea0165..0ce5e6d7a 100644
--- a/src/crypto/crypto.h
+++ b/src/crypto/crypto.h
@@ -36,9 +36,12 @@
 #include <boost/thread/lock_guard.hpp>
 #include <boost/utility/value_init.hpp>
 #include <boost/optional.hpp>
+#include <type_traits>
 #include <vector>
 
 #include "common/pod-class.h"
+#include "common/util.h"
+#include "common/memwipe.h"
 #include "generic-ops.h"
 #include "hex.h"
 #include "span.h"
@@ -65,9 +68,7 @@ namespace crypto {
     friend class crypto_ops;
   };
 
-  POD_CLASS secret_key: ec_scalar {
-    friend class crypto_ops;
-  };
+  using secret_key = tools::scrubbed<ec_scalar>;
 
   POD_CLASS public_keyV {
     std::vector<public_key> keys;
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 7b7e371a2..abd24295a 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -60,6 +60,7 @@ using namespace epee;
 #include "rapidjson/writer.h"
 #include "rapidjson/stringbuffer.h"
 #include "common/json_util.h"
+#include "common/memwipe.h"
 #include "common/base58.h"
 #include "ringct/rctSigs.h"
 
@@ -2764,12 +2765,11 @@ bool wallet2::generate_chacha8_key_from_secret_keys(crypto::chacha8_key &key) co
   const account_keys &keys = m_account.get_keys();
   const crypto::secret_key &view_key = keys.m_view_secret_key;
   const crypto::secret_key &spend_key = keys.m_spend_secret_key;
-  char data[sizeof(view_key) + sizeof(spend_key) + 1];
-  memcpy(data, &view_key, sizeof(view_key));
-  memcpy(data + sizeof(view_key), &spend_key, sizeof(spend_key));
+  tools::scrubbed_arr<char, sizeof(view_key) + sizeof(spend_key) + 1> data;
+  memcpy(data.data(), &view_key, sizeof(view_key));
+  memcpy(data.data() + sizeof(view_key), &spend_key, sizeof(spend_key));
   data[sizeof(data) - 1] = CHACHA8_KEY_TAIL;
-  crypto::generate_chacha8_key(data, sizeof(data), key);
-  memset(data, 0, sizeof(data));
+  crypto::generate_chacha8_key(data.data(), sizeof(data), key);
   return true;
 }
 //----------------------------------------------------------------------------------------------------
diff --git a/tests/crypto/CMakeLists.txt b/tests/crypto/CMakeLists.txt
index 573c62ad4..f4abb3a9a 100644
--- a/tests/crypto/CMakeLists.txt
+++ b/tests/crypto/CMakeLists.txt
@@ -42,6 +42,7 @@ add_executable(cncrypto-tests
   ${crypto_headers})
 target_link_libraries(cncrypto-tests
   PRIVATE
+    common
     ${Boost_SYSTEM_LIBRARY}
     ${EXTRA_LIBRARIES})
 set_property(TARGET cncrypto-tests