From 7193b89fe567327bb78f4c61c887b2e2fad2ed51 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Thu, 26 Oct 2017 10:21:06 +0100 Subject: [PATCH] Scrub keys from memory just before scope end. Partially implements #74. Securely erases keys from memory after they are no longer needed. Might have a performance impact, which I haven't measured (perf measurements aren't generally reliable on laptops). Thanks to @stoffu for the suggestion to specialize the pod_to_hex/hex_to_pod functions. Using overloads + SFINAE instead generalizes it so other types can be marked as scrubbed without adding more boilerplate. --- contrib/epee/include/span.h | 2 +- src/common/memwipe.h | 43 +++++++++++++++++++++++++++++++++++++ src/crypto/chacha8.h | 20 ++++++----------- src/crypto/crypto.h | 7 +++--- src/wallet/wallet2.cpp | 10 ++++----- tests/crypto/CMakeLists.txt | 1 + 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 constexpr bool has_padding() noexcept { - return !std::is_pod() || alignof(T) != 1; + return !std::is_pod::value || alignof(T) != 1; } //! \return Cast data from `src` as `span`. 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 + 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 + struct scrubbed : public T { + using type = T; + + ~scrubbed() { + scrub(); + } + + /// Destroy the contents of the contained type. + void scrub() { + static_assert(std::is_pod::value, + "T cannot be auto-scrubbed. T must be POD."); + static_assert(std::is_trivially_destructible::value, + "T cannot be auto-scrubbed. T must be trivially destructable."); + memwipe(this, sizeof(T)); + } + }; + + template + using scrubbed_arr = scrubbed>; +} // namespace tools + +// Partial specialization for std::is_pod> so that it can +// pretend to be the containted type in those contexts. +namespace std +{ + template + struct is_pod> { + static const bool value = is_pod::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) } + using chacha8_key = tools::scrubbed_arr; + #pragma pack(push, 1) - struct chacha8_key { - uint8_t data[CHACHA8_KEY_SIZE]; - - ~chacha8_key() - { - memwipe(data, sizeof(data)); - } - }; - // 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(&key), reinterpret_cast(&iv), cipher); + chacha8(data, length, key.data(), reinterpret_cast(&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 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 #include #include +#include #include #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; POD_CLASS public_keyV { std::vector keys; diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 59e759bfc..ef8a75375 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -57,6 +57,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" @@ -2761,12 +2762,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 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