mirror of
https://github.com/bitcoin/bitcoin.git
synced 2025-03-06 14:19:59 -05:00
wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it
`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; | ^ ```
This commit is contained in:
parent
03c5b0064d
commit
32a9f13cb8
4 changed files with 20 additions and 8 deletions
|
@ -811,7 +811,9 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyInner(const CKey& key, const CPubKey &pu
|
||||||
|
|
||||||
std::vector<unsigned char> vchCryptedSecret;
|
std::vector<unsigned char> vchCryptedSecret;
|
||||||
CKeyingMaterial vchSecret{UCharCast(key.begin()), UCharCast(key.end())};
|
CKeyingMaterial vchSecret{UCharCast(key.begin()), UCharCast(key.end())};
|
||||||
if (!EncryptSecret(m_storage.GetEncryptionKey(), vchSecret, pubkey.GetHash(), vchCryptedSecret)) {
|
if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
|
||||||
|
return EncryptSecret(encryption_key, vchSecret, pubkey.GetHash(), vchCryptedSecret);
|
||||||
|
})) {
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -997,7 +999,9 @@ bool LegacyScriptPubKeyMan::GetKey(const CKeyID &address, CKey& keyOut) const
|
||||||
{
|
{
|
||||||
const CPubKey &vchPubKey = (*mi).second.first;
|
const CPubKey &vchPubKey = (*mi).second.first;
|
||||||
const std::vector<unsigned char> &vchCryptedSecret = (*mi).second.second;
|
const std::vector<unsigned char> &vchCryptedSecret = (*mi).second.second;
|
||||||
return DecryptKey(m_storage.GetEncryptionKey(), vchCryptedSecret, vchPubKey, keyOut);
|
return m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
|
||||||
|
return DecryptKey(encryption_key, vchCryptedSecret, vchPubKey, keyOut);
|
||||||
|
});
|
||||||
}
|
}
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
@ -2128,7 +2132,9 @@ std::map<CKeyID, CKey> DescriptorScriptPubKeyMan::GetKeys() const
|
||||||
const CPubKey& pubkey = key_pair.second.first;
|
const CPubKey& pubkey = key_pair.second.first;
|
||||||
const std::vector<unsigned char>& crypted_secret = key_pair.second.second;
|
const std::vector<unsigned char>& crypted_secret = key_pair.second.second;
|
||||||
CKey key;
|
CKey key;
|
||||||
DecryptKey(m_storage.GetEncryptionKey(), crypted_secret, pubkey, key);
|
m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
|
||||||
|
return DecryptKey(encryption_key, crypted_secret, pubkey, key);
|
||||||
|
});
|
||||||
keys[pubkey.GetID()] = key;
|
keys[pubkey.GetID()] = key;
|
||||||
}
|
}
|
||||||
return keys;
|
return keys;
|
||||||
|
@ -2262,7 +2268,9 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const
|
||||||
|
|
||||||
std::vector<unsigned char> crypted_secret;
|
std::vector<unsigned char> crypted_secret;
|
||||||
CKeyingMaterial secret{UCharCast(key.begin()), UCharCast(key.end())};
|
CKeyingMaterial secret{UCharCast(key.begin()), UCharCast(key.end())};
|
||||||
if (!EncryptSecret(m_storage.GetEncryptionKey(), secret, pubkey.GetHash(), crypted_secret)) {
|
if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
|
||||||
|
return EncryptSecret(encryption_key, secret, pubkey.GetHash(), crypted_secret);
|
||||||
|
})) {
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -22,6 +22,7 @@
|
||||||
|
|
||||||
#include <boost/signals2/signal.hpp>
|
#include <boost/signals2/signal.hpp>
|
||||||
|
|
||||||
|
#include <functional>
|
||||||
#include <optional>
|
#include <optional>
|
||||||
#include <unordered_map>
|
#include <unordered_map>
|
||||||
|
|
||||||
|
@ -46,7 +47,8 @@ public:
|
||||||
virtual void UnsetBlankWalletFlag(WalletBatch&) = 0;
|
virtual void UnsetBlankWalletFlag(WalletBatch&) = 0;
|
||||||
virtual bool CanSupportFeature(enum WalletFeature) const = 0;
|
virtual bool CanSupportFeature(enum WalletFeature) const = 0;
|
||||||
virtual void SetMinVersion(enum WalletFeature, WalletBatch* = nullptr) = 0;
|
virtual void SetMinVersion(enum WalletFeature, WalletBatch* = nullptr) = 0;
|
||||||
virtual const CKeyingMaterial& GetEncryptionKey() const = 0;
|
//! Pass the encryption key to cb().
|
||||||
|
virtual bool WithEncryptionKey(std::function<bool (const CKeyingMaterial&)> cb) const = 0;
|
||||||
virtual bool HasEncryptionKeys() const = 0;
|
virtual bool HasEncryptionKeys() const = 0;
|
||||||
virtual bool IsLocked() const = 0;
|
virtual bool IsLocked() const = 0;
|
||||||
};
|
};
|
||||||
|
|
|
@ -3513,9 +3513,10 @@ void CWallet::SetupLegacyScriptPubKeyMan()
|
||||||
AddScriptPubKeyMan(id, std::move(spk_manager));
|
AddScriptPubKeyMan(id, std::move(spk_manager));
|
||||||
}
|
}
|
||||||
|
|
||||||
const CKeyingMaterial& CWallet::GetEncryptionKey() const
|
bool CWallet::WithEncryptionKey(std::function<bool (const CKeyingMaterial&)> cb) const
|
||||||
{
|
{
|
||||||
return vMasterKey;
|
LOCK(cs_wallet);
|
||||||
|
return cb(vMasterKey);
|
||||||
}
|
}
|
||||||
|
|
||||||
bool CWallet::HasEncryptionKeys() const
|
bool CWallet::HasEncryptionKeys() const
|
||||||
|
|
|
@ -962,7 +962,8 @@ public:
|
||||||
//! Make a LegacyScriptPubKeyMan and set it for all types, internal, and external.
|
//! Make a LegacyScriptPubKeyMan and set it for all types, internal, and external.
|
||||||
void SetupLegacyScriptPubKeyMan();
|
void SetupLegacyScriptPubKeyMan();
|
||||||
|
|
||||||
const CKeyingMaterial& GetEncryptionKey() const override;
|
bool WithEncryptionKey(std::function<bool (const CKeyingMaterial&)> cb) const override;
|
||||||
|
|
||||||
bool HasEncryptionKeys() const override;
|
bool HasEncryptionKeys() const override;
|
||||||
|
|
||||||
/** Get last block processed height */
|
/** Get last block processed height */
|
||||||
|
|
Loading…
Add table
Reference in a new issue