From c98dd824b692bae38e6c74a5a459a8a41e9b469c Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 13 Feb 2023 15:08:37 -0500 Subject: [PATCH] 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: https://github.com/bitcoin/bitcoin/blob/b92d609fb25637ccda000e182da854d4b762eee9/src/wallet/rpc/encrypt.cpp#L157-L158 However, the vector (a `std::vector>`) 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 --- src/wallet/wallet.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a5db987bbb..d7694f44d9 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -25,6 +25,7 @@ #include