Skip to content

fix: improve wallet encryption robustness #6945

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
+44 −25
Open

fix: improve wallet encryption robustness #6945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 31 additions & 23 deletions src/wallet/scriptpubkeyman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,10 +244,14 @@ bool LegacyScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, WalletBat
return false;
}

// must get current HD chain before EncryptKeys
CHDChain hdChainCurrent;
GetHDChain(hdChainCurrent);

if (!hdChainCurrent.IsNull() && hdChainCurrent.IsCrypted()) {
encrypted_batch = nullptr;
return false;
}

KeyMap keys_to_encrypt;
keys_to_encrypt.swap(mapKeys); // Clear mapKeys so AddCryptedKeyInner will succeed.
for (const KeyMap::value_type& mKey : keys_to_encrypt)
Expand Down Expand Up @@ -340,10 +344,13 @@ void LegacyScriptPubKeyMan::UpgradeKeyMetadata()
CHDChain hdChainCurrent;
if (!GetHDChain(hdChainCurrent))
throw std::runtime_error(std::string(__func__) + ": GetHDChain failed");
if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
return DecryptHDChain(encryption_key, hdChainCurrent);
})) {
throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed");

if (!hdChainCurrent.IsNull() && hdChainCurrent.IsCrypted()) {
if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
return DecryptHDChain(encryption_key, hdChainCurrent);
})) {
throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed");
}
}

CExtKey masterKey;
Expand Down Expand Up @@ -475,10 +482,12 @@ bool LegacyScriptPubKeyMan::GetDecryptedHDChain(CHDChain& hdChainRet) const
return false;
}

if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
return DecryptHDChain(encryption_key, hdChainTmp);
})) {
return false;
if (!hdChainTmp.IsNull() && hdChainTmp.IsCrypted()) {
if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
return DecryptHDChain(encryption_key, hdChainTmp);
})) {
return false;
}
}

// make sure seed matches this chain
Expand All @@ -493,12 +502,9 @@ bool LegacyScriptPubKeyMan::GetDecryptedHDChain(CHDChain& hdChainRet) const
bool LegacyScriptPubKeyMan::EncryptHDChain(const CKeyingMaterial& vMasterKeyIn, CHDChain& chain)
{
LOCK(cs_KeyStore);
// should call EncryptKeys first
if (!m_storage.HasEncryptionKeys())
return false;

if (chain.IsCrypted())
return true;
return false;

// make sure seed matches this chain
if (chain.GetID() != chain.GetSeedHash())
Expand Down Expand Up @@ -541,8 +547,6 @@ bool LegacyScriptPubKeyMan::EncryptHDChain(const CKeyingMaterial& vMasterKeyIn,
bool LegacyScriptPubKeyMan::DecryptHDChain(const CKeyingMaterial& vMasterKeyIn, CHDChain& hdChainRet) const
{
LOCK(cs_KeyStore);
if (!m_storage.HasEncryptionKeys())
return true;

if (m_hd_chain.IsNull())
return false;
Expand Down Expand Up @@ -1170,10 +1174,12 @@ bool LegacyScriptPubKeyMan::GetKey(const CKeyID &address, CKey& keyOut) const
CHDChain hdChainCurrent;
if (!GetHDChain(hdChainCurrent))
throw std::runtime_error(std::string(__func__) + ": GetHDChain failed");
if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
return DecryptHDChain(encryption_key, hdChainCurrent);
})) {
throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed");
if (!hdChainCurrent.IsNull() && hdChainCurrent.IsCrypted()) {
if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
return DecryptHDChain(encryption_key, hdChainCurrent);
})) {
throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed");
}
}
// make sure seed matches this chain
if (hdChainCurrent.GetID() != hdChainCurrent.GetSeedHash())
Expand Down Expand Up @@ -1285,10 +1291,12 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata&
throw std::runtime_error(std::string(__func__) + ": GetHDChain failed");
}

if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
return DecryptHDChain(encryption_key, hdChainTmp);
})) {
throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed");
if (!hdChainTmp.IsNull() && hdChainTmp.IsCrypted()) {
if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
return DecryptHDChain(encryption_key, hdChainTmp);
})) {
throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed");
}
}
// make sure seed matches this chain
if (hdChainTmp.GetID() != hdChainTmp.GetSeedHash())
Expand Down
15 changes: 13 additions & 2 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,12 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)

{
LOCK(cs_wallet);

if (IsCrypted()) {
// verify again now that cs_wallet lock is held
return false;
}

mapMasterKeys[++nMasterKeyMaxID] = kMasterKey;
WalletBatch* encrypted_batch = new WalletBatch(GetDatabase());
if (!encrypted_batch->TxnBegin()) {
Expand Down Expand Up @@ -766,6 +772,10 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
} else if (auto spk_man = GetLegacyScriptPubKeyMan()) {
// if we are not using HD, generate new keypool
if (spk_man->IsHDEnabled()) {
// NOTE: using internal master key which should be populated on Unlock()
if (!spk_man->CheckDecryptionKey(vMasterKey)) {
return false;
}
if (!spk_man->TopUp()) {
return false;
}
Expand All @@ -779,8 +789,9 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)

// Need to completely rewrite the wallet file; if we don't, bdb might keep
// bits of the unencrypted private key in slack space in the database file.
GetDatabase().Rewrite();

if (!GetDatabase().Rewrite()) {
WalletLogPrintf("WARNING: Rewrite failed - wallet is in dangerous state!\n");
}
// BDB seems to have a bad habit of writing old data into
// slack space in .dat files; that is bad if the old data is
// unencrypted private keys. So:
Expand Down
Loading