Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Wallet: Zero out wallet master key upon locking so it doesn't persist in memory #27080

Merged
merged 1 commit into from Feb 13, 2023
Merged

Wallet: Zero out wallet master key upon locking so it doesn't persist in memory #27080

merged 1 commit into from Feb 13, 2023

Conversation

john-moffett
Copy link
Contributor

@john-moffett john-moffett commented Feb 10, 2023 •
edited

When an encrypted wallet is locked (for instance via the RPC walletlock), the documentation indicates that the key is removed from memory:

bitcoin/src/wallet/rpc/encrypt.cpp

Lines 157 to 158 in b92d609

return RPCHelpMan{"walletlock",
"\nRemoves the wallet encryption key from memory, locking the wallet.\n"

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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 10, 2023 •
edited

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK S3RK, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26347 (wallet: ensure the wallet is unlocked when needed for rescanning by ishaanam)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@john-moffett
Zero out wallet master key upon lock
3a11adc 
When an encrypted wallet is locked (for instance via the
RPC `walletlock`), the docs indicate that the key is
removed from memory. However, the vector (with a secure
allocator) is merely cleared. This allows the key to persist
indefinitely in memory. Instead, manually fill the bytes with
zeroes before clearing.
@DrahtBot DrahtBot mentioned this pull request Feb 11, 2023
Merged
@S3RK
Copy link
Contributor

S3RK commented Feb 13, 2023

Code review ACK 3a11adc

@achow101
Copy link
Member

ACK 3a11adc

@achow101 achow101 merged commit 2c1fe27 into bitcoin:master Feb 13, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 14, 2023
@achow101
Merge bitcoin#27080: Wallet: Zero out wallet master key upon locking …
149cd11 
…so it doesn't persist in memory

3a11adc 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<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 3a11adc
  achow101:
    ACK 3a11adc

Tree-SHA512: c4e3dab452ad051da74855a13aa711892c9b34c43cc43a45a3b1688ab044e75d715b42843c229219761913b4861abccbcc8d5cb6ac54957d74f6e357f04e8730
@fanquake fanquake mentioned this pull request Feb 14, 2023
Merged
@fanquake
Copy link
Member

Added this to #26878 for backporting to 24.x.

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 14, 2023
@john-moffett @fanquake
Zero out wallet master key upon lock
40ca6e3 
When an encrypted wallet is locked (for instance via the
RPC `walletlock`), the docs indicate that the key is
removed from memory. However, the vector (with a secure
allocator) is merely cleared. This allows the key to persist
indefinitely in memory. Instead, manually fill the bytes with
zeroes before clearing.

Github-Pull: bitcoin#27080
Rebased-From: 3a11adc
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 22, 2023
@john-moffett @fanquake
Zero out wallet master key upon lock
64e7db6 
When an encrypted wallet is locked (for instance via the
RPC `walletlock`), the docs indicate that the key is
removed from memory. However, the vector (with a secure
allocator) is merely cleared. This allows the key to persist
indefinitely in memory. Instead, manually fill the bytes with
zeroes before clearing.

Github-Pull: bitcoin#27080
Rebased-From: 3a11adc
glozow added a commit that referenced this pull request Feb 27, 2023
@glozow
Merge #26878: [24.x] Backports
c8c85ca 
784a754 wallet, rpc: Update migratewallet help text for encrypted wallets (Andrew Chow)
debcfe3 tests: Tests for migrating wallets by name, and providing passphrase (Andrew Chow)
ccc72fe wallet: Be able to unlock the wallet for migration (Andrew Chow)
50dd8b1 rpc: Allow users to specify wallet name for migratewallet (Andrew Chow)
648b062 wallet: Allow MigrateLegacyToDescriptor to take a wallet name (Andrew Chow)
ab3bd45 i2p: use consistent number of tunnels with i2pd and Java I2P (Vasil Dimov)
29cdf42 i2p: lower the number of tunnels for transient sessions (Vasil Dimov)
5027e93 i2p: reuse created I2P sessions if not used (Vasil Dimov)
a62c541 wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin)
64e7db6 Zero out wallet master key upon lock (John Moffett)
b7e242e Correctly limit overview transaction list (John Moffett)
cff6718 depends: fix systemtap download URL (fanquake)
7cf73df Add missing includes to fix gcc-13 compile error (MarcoFalke)
07397cd addrdb: Only call Serialize() once (Martin Zumsande)
91f83db hash: add HashedSourceWriter (Martin Zumsande)
5c824ac For feebump, ignore abandoned descendant spends (John Moffett)
428dcd5 wallet: Skip rescanning if wallet is more recent than tip (Andrew Chow)
cbcdafa test: wallet: check that labels are migrated to watchonly wallet (Sebastian Falbesoner)
342abfb wallet: fully migrate address book entries for watchonly/solvable wallets (Sebastian Falbesoner)

Pull request description:

  Backports:
  * #26595
  * #26675
  * #26679
  * #26761
  * #26837
  * #26909
  * #26924
  * #26944
  * bitcoin-core/gui#704
  * #27053
  * #27080

ACKs for top commit:
  instagibbs:
    ACK 784a754
  achow101:
    ACK 784a754
  hebasto:
    ACK 784a754, I've made backporting locally and got a diff between my branch and this PR as follows:

Tree-SHA512: 8ea84aa02d7907ff1e202e1302b441ce9ed2198bf383620ad40056a5d7e8ea88e1047abef0b92d85648016bf9b3195c974be3806ccebd85bef4f85c326869e43
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 14, 2023
@john-moffett @PiRK
Zero out wallet master key upon lock
05238e3 
Summary:
When an encrypted wallet is locked (for instance via the
RPC `walletlock`), the docs indicate that the key is
removed from memory. However, the vector (with a secure
allocator) is merely cleared. This allows the key to persist
indefinitely in memory. Instead, manually fill the bytes with
zeroes before clearing.

This is a backport of [[bitcoin/bitcoin#27080 | core#27080]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D13327
@bitcoin bitcoin locked and limited conversation to collaborators Feb 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@achow101 achow101 Awaiting requested review from achow101

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@john-moffett @DrahtBot @S3RK @achow101 @fanquake