wallet2: read multisig restore fields with memcpy in generate
What the developer wrote
Authored by alhudz
wallet2: read multisig restore fields with memcpy in generate
What changed, and why it matters
This commit changes how a Monero wallet reads sensitive key data from a multisig restore string. Previously, the code directly cast a pointer to the data buffer and read keys as if they were already in the correct structure. Now it uses memcpy, which is the safer, standard way to copy raw bytes into typed variables. The main practical concern is avoiding undefined behavior from unaligned or improperly typed memory access, especially for secret key types that may have stricter alignment requirements. It is a hardening fix rather than a clear-cut remote exploit.
Treat as a defensive hardening patch. Review whether the existing minimum-size check is sufficient for the full multisig_data layout and consider adding explicit bounds checks before each memcpy. Backport to maintained release branches if the undefined-behavior pattern is present there.
Security signals we found
Replaced type-punned pointer dereferences with memcpy for secret and public key material
Use of unwrap(unwrap(...)) to access the underlying buffer of wrapped secret key types
Existing length check only verifies multisig_data.size() >= 32; no per-field bounds validation added
Hardens multisig wallet restore/initialization path
Evidence from the diff
In wallet2::generate(), the patch replaces pointer-cast loads of uint32_t fields and crypto::secret_key/crypto::public_key fields from a std::string byte buffer with explicit memcpy calls. The old pattern *(T*)(buffer.data() + offset) is undefined behavior if the buffer is not suitably aligned for T and can violate strict aliasing. For crypto::secret_key, which is wrapped in epee::mlocked and possibly other wrappers, the patch uses unwrap(unwrap(key)) to obtain a writable byte buffer before memcpy. This reduces risk of alignment faults, aliasing violations, and potential information leaks via compiler optimizations, but the commit itself does not add bounds checks beyond the existing 32-byte minimum-size check.
Changed components
src/wallet/wallet2.cppwallet2::generate()Multisig wallet restore/seed importInspect captured patch +17 / −8
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 490c6a5..06fd967 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -5641,9 +5641,10 @@ void wallet2::generate(const std::string& wallet_, const epee::wipeable_string&
THROW_WALLET_EXCEPTION_IF(multisig_data.size() < 32, error::invalid_multisig_seed);
size_t offset = 0;
- uint32_t threshold = *(uint32_t*)(multisig_data.data() + offset);
+ uint32_t threshold, total;
+ memcpy(&threshold, multisig_data.data() + offset, sizeof(uint32_t));
offset += sizeof(uint32_t);
- uint32_t total = *(uint32_t*)(multisig_data.data() + offset);
+ memcpy(&total, multisig_data.data() + offset, sizeof(uint32_t));
offset += sizeof(uint32_t);
THROW_WALLET_EXCEPTION_IF(threshold < 1, error::invalid_multisig_seed);
@@ -5654,22 +5655,30 @@ void wallet2::generate(const std::string& wallet_, const epee::wipeable_string&
std::vector<crypto::secret_key> multisig_keys;
std::vector<crypto::public_key> multisig_signers;
- crypto::secret_key spend_secret_key = *(crypto::secret_key*)(multisig_data.data() + offset);
+ crypto::secret_key spend_secret_key;
+ memcpy(&unwrap(unwrap(spend_secret_key)), multisig_data.data() + offset, sizeof(crypto::secret_key));
offset += sizeof(crypto::secret_key);
- crypto::public_key spend_public_key = *(crypto::public_key*)(multisig_data.data() + offset);
+ crypto::public_key spend_public_key;
+ memcpy(&spend_public_key, multisig_data.data() + offset, sizeof(crypto::public_key));
offset += sizeof(crypto::public_key);
- crypto::secret_key view_secret_key = *(crypto::secret_key*)(multisig_data.data() + offset);
+ crypto::secret_key view_secret_key;
+ memcpy(&unwrap(unwrap(view_secret_key)), multisig_data.data() + offset, sizeof(crypto::secret_key));
offset += sizeof(crypto::secret_key);
- crypto::public_key view_public_key = *(crypto::public_key*)(multisig_data.data() + offset);
+ crypto::public_key view_public_key;
+ memcpy(&view_public_key, multisig_data.data() + offset, sizeof(crypto::public_key));
offset += sizeof(crypto::public_key);
for (size_t n = 0; n < n_multisig_keys; ++n)
{
- multisig_keys.push_back(*(crypto::secret_key*)(multisig_data.data() + offset));
+ crypto::secret_key multisig_key;
+ memcpy(&unwrap(unwrap(multisig_key)), multisig_data.data() + offset, sizeof(crypto::secret_key));
+ multisig_keys.push_back(multisig_key);
offset += sizeof(crypto::secret_key);
}
for (size_t n = 0; n < total; ++n)
{
- multisig_signers.push_back(*(crypto::public_key*)(multisig_data.data() + offset));
+ crypto::public_key signer;
+ memcpy(&signer, multisig_data.data() + offset, sizeof(crypto::public_key));
+ multisig_signers.push_back(signer);
offset += sizeof(crypto::public_key);
}
Why this scored 48/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.