serialization: avoid blob memcpy of secret_key vector
What the developer wrote
Authored by selsta
serialization: avoid blob memcpy of secret_key vector
What changed, and why it matters
This commit changes how Monero stores and loads a list of sensitive multisignature wallet keys. Previously, the code used a generic 'copy the raw bytes' path for a container of secret keys. That generic path could leave copies of secret key bytes in temporary memory or use a copy method not safe for non-trivial types. The patch forces the keys to be converted through an explicitly cleared intermediate type and adds a compile-time guard so the generic blob-copy helper can only be used on simple, safe types. In short: it reduces the chance that secret key material leaks or gets mishandled during wallet serialization.
Treat this as a security-hardening fix and include it in any release that handles multisig wallets. Wallet users and services using multisig should upgrade to a version containing this commit. Developers should audit other uses of KV_SERIALIZE_CONTAINER_POD_AS_BLOB for containers of scrubbed or non-trivial types.
Security signals we found
Avoids raw byte-copy serialization of secret_key vector
Adds compile-time trivially-copyable guard to blob serialization helpers
Uses explicit unwrap/wrap through ec_scalar and scrubbed to preserve secure cleanup semantics
Prevents undefined behavior from memcpy on non-trivially-copyable scrubbed type
Reduces risk of secret key material lingering in temporary std::string/blob buffers
Evidence from the diff
The patch removes KV_SERIALIZE_CONTAINER_POD_AS_BLOB(m_multisig_keys) from account.h and replaces it with a hand-rolled branch. On store, it unwraps each crypto::secret_key to crypto::ec_scalar and serializes that vector via serialize_stl_container_pod_val_as_blob. On load, it deserializes into a vector
Changed components
contrib/epee/include/serialization/keyvalue_serialization_overloads.hsrc/cryptonote_basic/account.hMonero wallet/account serializationMultisig key storage/loadingInspect captured patch +26 / −1
diff --git a/contrib/epee/include/serialization/keyvalue_serialization_overloads.h b/contrib/epee/include/serialization/keyvalue_serialization_overloads.h
index 8c5e28d..fc85e38 100644
--- a/contrib/epee/include/serialization/keyvalue_serialization_overloads.h
+++ b/contrib/epee/include/serialization/keyvalue_serialization_overloads.h
@@ -32,6 +32,7 @@
#include <list>
#include <vector>
#include <deque>
+#include <type_traits>
#include <boost/mpl/vector.hpp>
#include <boost/mpl/contains_fwd.hpp>
@@ -114,6 +115,9 @@ namespace epee
template<class stl_container, class t_storage>
static bool serialize_stl_container_pod_val_as_blob(const stl_container& container, t_storage& stg, typename t_storage::hsection hparent_section, const char* pname)
{
+ static_assert(std::is_trivially_copyable_v<typename stl_container::value_type>,
+ "serialize_stl_container_pod_val_as_blob requires trivially copyable value_type");
+
if(!container.size()) return true;
std::string mb;
mb.resize(sizeof(typename stl_container::value_type)*container.size());
@@ -129,6 +133,9 @@ namespace epee
template<class stl_container, class t_storage>
static bool unserialize_stl_container_pod_val_as_blob(stl_container& container, t_storage& stg, typename t_storage::hsection hparent_section, const char* pname)
{
+ static_assert(std::is_trivially_copyable_v<typename stl_container::value_type>,
+ "unserialize_stl_container_pod_val_as_blob requires trivially copyable value_type");
+
container.clear();
std::string buff;
bool res = stg.get_value(pname, buff, hparent_section);
diff --git a/src/cryptonote_basic/account.h b/src/cryptonote_basic/account.h
index de59120..71b5b64 100644
--- a/src/cryptonote_basic/account.h
+++ b/src/cryptonote_basic/account.h
@@ -50,7 +50,25 @@ namespace cryptonote
KV_SERIALIZE(m_account_address)
KV_SERIALIZE_VAL_POD_AS_BLOB_FORCE(m_spend_secret_key)
KV_SERIALIZE_VAL_POD_AS_BLOB_FORCE(m_view_secret_key)
- KV_SERIALIZE_CONTAINER_POD_AS_BLOB(m_multisig_keys)
+ if constexpr (is_store)
+ {
+ std::vector<crypto::ec_scalar> multisig_keys;
+ multisig_keys.reserve(this_ref.m_multisig_keys.size());
+ for (const crypto::secret_key& key : this_ref.m_multisig_keys)
+ multisig_keys.push_back(unwrap(unwrap(key)));
+ epee::serialization::selector<is_store>::serialize_stl_container_pod_val_as_blob(
+ multisig_keys, stg, hparent_section, "m_multisig_keys");
+ }
+ else
+ {
+ std::vector<crypto::ec_scalar> multisig_keys;
+ epee::serialization::selector<is_store>::serialize_stl_container_pod_val_as_blob(
+ multisig_keys, stg, hparent_section, "m_multisig_keys");
+ this_ref.m_multisig_keys.clear();
+ this_ref.m_multisig_keys.reserve(multisig_keys.size());
+ for (const crypto::ec_scalar& key : multisig_keys)
+ this_ref.m_multisig_keys.emplace_back(tools::scrubbed<crypto::ec_scalar>{key});
+ }
const crypto::chacha_iv default_iv{{0, 0, 0, 0, 0, 0, 0, 0}};
KV_SERIALIZE_VAL_POD_AS_BLOB_OPT(m_encryption_iv, default_iv)
END_KV_SERIALIZE_MAP()
Why this scored 56/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.