When an encrypted wallet is locked (for instance via the RPC walletlock), the documentation indicates that the key is removed from memory:
However, the vector (a std::vector<unsigned char, secure_allocator<unsigned char>>) is merely cleared. As it is a member variable, it also stays in scope as long as the wallet is loaded, preventing the secure allocator from deallocating. This allows the key to persist indefinitely in memory. I confirmed this behavior on my macOS machine by using an open-source third party memory inspector ("Bit Slicer"). I was able to find my wallet's master key in Bit Slicer after unlocking and re-locking my encrypted wallet. I then confirmed the key data was at the address in LLDB.
This PR manually fills the bytes with zeroes before calling clear() by using our memory_cleanse function, which is designed to prevent the compiler from optimizing it away. I confirmed that it does remove the data from memory on my machine upon locking.
Note: An alternative approach could be to call vMasterKey.shrink_to_fit() after the clear(), which would trigger the secure allocator's deallocation. However, shrink_to_fit() is not guaranteed to actually change the vector's capacity, so I think it's unwise to rely on it.
Edit: A little more clarity on why this is an improvement.
Since mlocked memory is guaranteed not to be swapped to disk and our threat model doesn't consider a super-user monitoring the memory in realtime, why is this an improvement? Most importantly, consider hibernation. Even mlocked memory may get written to disk. From the mlock manpage:
(But be aware that the suspend mode on laptops and some desktop computers will save a copy of the system's RAM to disk, regardless of memory locks.)
As far as I can tell, this is true of Windows and macOS as well.
Therefore, a user with a strong OS password and a strong wallet passphrase could still have their keys stolen if a thief takes their (hibernated) machine and reads the permanent storage.
