mirror of
https://github.com/dashpay/dash.git
synced 2024-12-25 03:52:49 +01:00
Merge bitcoin/bitcoin#27080: Wallet: Zero out wallet master key upon locking so it doesn't persist in memory
3a11adc7004d21b3dfe028b190d83add31691c55 Zero out wallet master key upon lock (John Moffett)
Pull request description:
When an encrypted wallet is locked (for instance via the RPC `walletlock`), the documentation indicates that the key is removed from memory:
b92d609fb2/src/wallet/rpc/encrypt.cpp (L157-L158)
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 `mlock`ed 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 `mlock`ed memory may get written to disk. From the `mlock` [manpage](https://man7.org/linux/man-pages/man2/mlock.2.html):
> (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](https://web.archive.org/web/20190127110059/https://blogs.msdn.microsoft.com/oldnewthing/20140207-00/?p=1833#:~:text=%5BThere%20does%20not%20appear%20to%20be%20any%20guarantee%20that%20the%20memory%20won%27t%20be%20written%20to%20disk%20while%20locked.%20As%20you%20noted%2C%20the%20machine%20may%20be%20hibernated%2C%20or%20it%20may%20be%20running%20in%20a%20VM%20that%20gets%20snapshotted.%20%2DRaymond%5D) 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.
ACKs for top commit:
S3RK:
Code review ACK 3a11adc7004d21b3dfe028b190d83add31691c55
achow101:
ACK 3a11adc7004d21b3dfe028b190d83add31691c55
Tree-SHA512: c4e3dab452ad051da74855a13aa711892c9b34c43cc43a45a3b1688ab044e75d715b42843c229219761913b4861abccbcc8d5cb6ac54957d74f6e357f04e8730
This commit is contained in:
parent
90ced24f4a
commit
c98dd824b6
@ -25,6 +25,7 @@
|
|||||||
#include <script/script.h>
|
#include <script/script.h>
|
||||||
#include <script/sign.h>
|
#include <script/sign.h>
|
||||||
#include <script/signingprovider.h>
|
#include <script/signingprovider.h>
|
||||||
|
#include <support/cleanse.h>
|
||||||
#include <txmempool.h>
|
#include <txmempool.h>
|
||||||
#include <util/bip32.h>
|
#include <util/bip32.h>
|
||||||
#include <util/check.h>
|
#include <util/check.h>
|
||||||
@ -5583,7 +5584,10 @@ bool CWallet::Lock(bool fAllowMixing)
|
|||||||
|
|
||||||
if(!fAllowMixing) {
|
if(!fAllowMixing) {
|
||||||
LOCK(cs_wallet);
|
LOCK(cs_wallet);
|
||||||
vMasterKey.clear();
|
if (!vMasterKey.empty()) {
|
||||||
|
memory_cleanse(vMasterKey.data(), vMasterKey.size() * sizeof(decltype(vMasterKey)::value_type));
|
||||||
|
vMasterKey.clear();
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
fOnlyMixingAllowed = fAllowMixing;
|
fOnlyMixingAllowed = fAllowMixing;
|
||||||
|
Loading…
Reference in New Issue
Block a user