HomePhabricator

wallet: avoid returning a reference to vMasterKey after releasing the mutex…

Description

wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it

Summary:

`CWallet::GetEncryptionKey()` would return a reference to the internal
`CWallet::vMasterKey`, guarded by `CWallet::cs_wallet`, which is unsafe.

Returning a copy would be a shorter solution, but could have security
implications of the master key remaining somewhere in the memory even
after `CWallet::Lock()` (the current calls to
`CWallet::GetEncryptionKey()` are safe, but that is not future proof).

So, instead of `EncryptSecret(m_storage.GetEncryptionKey(), ...)`
change the `GetEncryptionKey()` method to provide the encryption
key to a given callback:
`m_storage.WithEncryptionKey([](const CKeyingMaterial& k) { EncryptSecret(k, ...); })`

This silences the following (clang 18):

wallet/wallet.cpp:3520:12: error: returning variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-reference-return]
 3520 |     return vMasterKey;
      |            ^

Backport of core#28774.

Test Plan:
With clang 18:

ninja all check-all

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D17013

Details

Provenance
Vasil Dimov <vd@FreeBSD.org>Authored on Nov 2 2023, 12:26
FabienCommitted on Oct 26 2024, 18:20
FabienPushed on Oct 26 2024, 18:20
Reviewer
Restricted Project
Differential Revision
D17013: wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it
Parents
rABC3ab65b20c797: ArgsManager: return path by value from GetBlocksDirPath()
Branches
Unknown
Tags
Unknown