From a8334f7ac39532528c5f8bd3b0eea05aa63e8794 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 23 Sep 2019 18:10:13 -0400 Subject: [PATCH 1/3] Read and write a checksum for encrypted keys --- src/wallet/walletdb.cpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index a1928f45c45..098047bb398 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -109,7 +109,11 @@ bool WalletBatch::WriteCryptedKey(const CPubKey& vchPubKey, return false; } - if (!WriteIC(std::make_pair(DBKeys::CRYPTED_KEY, vchPubKey), vchCryptedSecret, false)) { + // Compute a checksum of the encrypted key + uint256 checksum = Hash(vchCryptedSecret.begin(), vchCryptedSecret.end()); + + const auto key = std::make_pair(DBKeys::CRYPTED_KEY, vchPubKey); + if (!WriteIC(key, std::make_pair(vchCryptedSecret, checksum), false)) { return false; } EraseIC(std::make_pair(DBKeys::KEY, vchPubKey)); @@ -332,6 +336,17 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, } std::vector vchPrivKey; ssValue >> vchPrivKey; + + // Get the checksum and check it + if (!ssValue.eof()) { + uint256 checksum; + ssValue >> checksum; + if (Hash(vchPrivKey.begin(), vchPrivKey.end()) != checksum) { + strErr = "Error reading wallet database: Crypted key corrupt"; + return false; + } + } + wss.nCKeys++; if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadCryptedKey(vchPubKey, vchPrivKey)) From c9a9ddb4142af0af5f7b1a5ccd13f8e585007089 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 3 Dec 2019 18:57:51 -0500 Subject: [PATCH 2/3] Set fDecryptionThoroughlyChecked based on whether crypted key checksums are valid Change fDecryptionThoroughlyChecked to default to true so that it can latch to false when an invalid checksum is seen. Checksums may be invalid if the wallet does not have checksums or if the wallet became corrupted. It is safe to default fDecryptionThoroughlyChecked to true because any existing wallet without a checksum will set it to false. Any new or blank wallet where encrypted keys are added will then set this to true when the first encrypted key is generated by virtue of CheckDecryptionKey doing that during the initial Unlock prior to keys being added. --- src/wallet/scriptpubkeyman.cpp | 7 ++++++- src/wallet/scriptpubkeyman.h | 4 ++-- src/wallet/walletdb.cpp | 5 +++-- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 4c9d88973e9..6d65d6f69d1 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -643,8 +643,13 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyInner(const CKey& key, const CPubKey &pu return true; } -bool LegacyScriptPubKeyMan::LoadCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret) +bool LegacyScriptPubKeyMan::LoadCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret, bool checksum_valid) { + // Set fDecryptionThoroughlyChecked to false when the checksum is invalid + if (!checksum_valid) { + fDecryptionThoroughlyChecked = false; + } + return AddCryptedKeyInner(vchPubKey, vchCryptedSecret); } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 7b1c023bc97..176743322d3 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -229,7 +229,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv { private: //! keeps track of whether Unlock has run a thorough check before - bool fDecryptionThoroughlyChecked = false; + bool fDecryptionThoroughlyChecked = true; using WatchOnlySet = std::set; using WatchKeyMap = std::map; @@ -365,7 +365,7 @@ public: //! Adds an encrypted key to the store, and saves it to disk. bool AddCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret); //! Adds an encrypted key to the store, without saving it to disk (used by LoadWallet) - bool LoadCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret); + bool LoadCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret, bool checksum_valid); void UpdateTimeFirstKey(int64_t nCreateTime) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore); //! Adds a CScript to the store bool LoadCScript(const CScript& redeemScript); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 098047bb398..ab1ad1a64a7 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -338,10 +338,11 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, ssValue >> vchPrivKey; // Get the checksum and check it + bool checksum_valid = false; if (!ssValue.eof()) { uint256 checksum; ssValue >> checksum; - if (Hash(vchPrivKey.begin(), vchPrivKey.end()) != checksum) { + if ((checksum_valid = Hash(vchPrivKey.begin(), vchPrivKey.end()) != checksum)) { strErr = "Error reading wallet database: Crypted key corrupt"; return false; } @@ -349,7 +350,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, wss.nCKeys++; - if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadCryptedKey(vchPubKey, vchPrivKey)) + if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadCryptedKey(vchPubKey, vchPrivKey, checksum_valid)) { strErr = "Error reading wallet database: LegacyScriptPubKeyMan::LoadCryptedKey failed"; return false; From d67055e00dd90f504384e5c3f229fc95306d5aac Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 3 Dec 2019 19:06:15 -0500 Subject: [PATCH 3/3] Upgrade or rewrite encrypted key checksums If fDecryptionThoroughlyChecked is false, after a key has been checked, write (or rewrite) its checksum. This serves to upgrade wallets and correct those which have the checksum corrupted but not the key. --- src/wallet/scriptpubkeyman.cpp | 5 +++++ src/wallet/walletdb.cpp | 9 ++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 6d65d6f69d1..fba2bf73108 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -210,6 +210,7 @@ bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key bool keyPass = mapCryptedKeys.empty(); // Always pass when there are no encrypted keys bool keyFail = false; CryptedKeyMap::const_iterator mi = mapCryptedKeys.begin(); + WalletBatch batch(m_storage.GetDatabase()); for (; mi != mapCryptedKeys.end(); ++mi) { const CPubKey &vchPubKey = (*mi).second.first; @@ -223,6 +224,10 @@ bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key keyPass = true; if (fDecryptionThoroughlyChecked) break; + else { + // Rewrite these encrypted keys with checksums + batch.WriteCryptedKey(vchPubKey, vchCryptedSecret, mapKeyMetadata[vchPubKey.GetID()]); + } } if (keyPass && keyFail) { diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index ab1ad1a64a7..6cb8c720f03 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -114,7 +114,14 @@ bool WalletBatch::WriteCryptedKey(const CPubKey& vchPubKey, const auto key = std::make_pair(DBKeys::CRYPTED_KEY, vchPubKey); if (!WriteIC(key, std::make_pair(vchCryptedSecret, checksum), false)) { - return false; + // It may already exist, so try writing just the checksum + std::vector val; + if (!m_batch.Read(key, val)) { + return false; + } + if (!WriteIC(key, std::make_pair(val, checksum), true)) { + return false; + } } EraseIC(std::make_pair(DBKeys::KEY, vchPubKey)); return true;