From d9fcc2b558730919f5bbf784a326a6ef3ca94334 Mon Sep 17 00:00:00 2001 From: Larry Ruane Date: Mon, 8 Apr 2019 11:02:08 -0600 Subject: [PATCH] eliminate races: hold cs_KeyStore lock while reading fUseCrypto --- src/wallet/crypter.cpp | 253 ++++++++++++++++++----------------------- src/wallet/crypter.h | 53 ++++----- 2 files changed, 132 insertions(+), 174 deletions(-) diff --git a/src/wallet/crypter.cpp b/src/wallet/crypter.cpp index 3ebc7bbc8..2122ebf20 100644 --- a/src/wallet/crypter.cpp +++ b/src/wallet/crypter.cpp @@ -185,24 +185,22 @@ static bool DecryptSaplingSpendingKey(const CKeyingMaterial& vMasterKey, return sk.expsk.full_viewing_key() == extfvk.fvk; } +// cs_KeyStore lock must be held by caller bool CCryptoKeyStore::SetCrypted() { - LOCK(cs_KeyStore); if (fUseCrypto) return true; if (!(mapKeys.empty() && mapSproutSpendingKeys.empty() && mapSaplingSpendingKeys.empty())) return false; - fUseCrypto = true; - return true; + return fUseCrypto = true; } bool CCryptoKeyStore::Lock() { - if (!SetCrypted()) - return false; - { LOCK(cs_KeyStore); + if (!SetCrypted()) + return false; vMasterKey.clear(); } @@ -291,7 +289,7 @@ bool CCryptoKeyStore::SetHDSeed(const HDSeed& seed) { { LOCK(cs_KeyStore); - if (!IsCrypted()) { + if (!fUseCrypto) { return CBasicKeyStore::SetHDSeed(seed); } @@ -316,27 +314,25 @@ bool CCryptoKeyStore::SetCryptedHDSeed( const uint256& seedFp, const std::vector& vchCryptedSecret) { - { - LOCK(cs_KeyStore); - if (!IsCrypted()) { - return false; - } - - if (!cryptedHDSeed.first.IsNull()) { - // Don't allow an existing seed to be changed. We can maybe relax this - // restriction later once we have worked out the UX implications. - return false; - } - - cryptedHDSeed = std::make_pair(seedFp, vchCryptedSecret); + LOCK(cs_KeyStore); + if (!fUseCrypto) { + return false; } + + if (!cryptedHDSeed.first.IsNull()) { + // Don't allow an existing seed to be changed. We can maybe relax this + // restriction later once we have worked out the UX implications. + return false; + } + + cryptedHDSeed = std::make_pair(seedFp, vchCryptedSecret); return true; } bool CCryptoKeyStore::HaveHDSeed() const { LOCK(cs_KeyStore); - if (!IsCrypted()) + if (!fUseCrypto) return CBasicKeyStore::HaveHDSeed(); return !cryptedHDSeed.second.empty(); @@ -345,7 +341,7 @@ bool CCryptoKeyStore::HaveHDSeed() const bool CCryptoKeyStore::GetHDSeed(HDSeed& seedOut) const { LOCK(cs_KeyStore); - if (!IsCrypted()) + if (!fUseCrypto) return CBasicKeyStore::GetHDSeed(seedOut); if (cryptedHDSeed.second.empty()) @@ -356,125 +352,106 @@ bool CCryptoKeyStore::GetHDSeed(HDSeed& seedOut) const bool CCryptoKeyStore::AddKeyPubKey(const CKey& key, const CPubKey &pubkey) { - { - LOCK(cs_KeyStore); - if (!IsCrypted()) - return CBasicKeyStore::AddKeyPubKey(key, pubkey); + LOCK(cs_KeyStore); + if (!fUseCrypto) + return CBasicKeyStore::AddKeyPubKey(key, pubkey); - if (IsLocked()) - return false; + if (IsLocked()) + return false; - std::vector vchCryptedSecret; - CKeyingMaterial vchSecret(key.begin(), key.end()); - if (!EncryptSecret(vMasterKey, vchSecret, pubkey.GetHash(), vchCryptedSecret)) - return false; + std::vector vchCryptedSecret; + CKeyingMaterial vchSecret(key.begin(), key.end()); + if (!EncryptSecret(vMasterKey, vchSecret, pubkey.GetHash(), vchCryptedSecret)) + return false; - if (!AddCryptedKey(pubkey, vchCryptedSecret)) - return false; - } - return true; + return AddCryptedKey(pubkey, vchCryptedSecret); } bool CCryptoKeyStore::AddCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret) { - { - LOCK(cs_KeyStore); - if (!SetCrypted()) - return false; + LOCK(cs_KeyStore); + if (!SetCrypted()) + return false; - mapCryptedKeys[vchPubKey.GetID()] = make_pair(vchPubKey, vchCryptedSecret); - } + mapCryptedKeys[vchPubKey.GetID()] = make_pair(vchPubKey, vchCryptedSecret); return true; } bool CCryptoKeyStore::GetKey(const CKeyID &address, CKey& keyOut) const { - { - LOCK(cs_KeyStore); - if (!IsCrypted()) - return CBasicKeyStore::GetKey(address, keyOut); + LOCK(cs_KeyStore); + if (!fUseCrypto) + return CBasicKeyStore::GetKey(address, keyOut); - CryptedKeyMap::const_iterator mi = mapCryptedKeys.find(address); - if (mi != mapCryptedKeys.end()) - { - const CPubKey &vchPubKey = (*mi).second.first; - const std::vector &vchCryptedSecret = (*mi).second.second; - return DecryptKey(vMasterKey, vchCryptedSecret, vchPubKey, keyOut); - } + CryptedKeyMap::const_iterator mi = mapCryptedKeys.find(address); + if (mi != mapCryptedKeys.end()) + { + const CPubKey &vchPubKey = (*mi).second.first; + const std::vector &vchCryptedSecret = (*mi).second.second; + return DecryptKey(vMasterKey, vchCryptedSecret, vchPubKey, keyOut); } return false; } bool CCryptoKeyStore::GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const { - { - LOCK(cs_KeyStore); - if (!IsCrypted()) - return CKeyStore::GetPubKey(address, vchPubKeyOut); + LOCK(cs_KeyStore); + if (!fUseCrypto) + return CKeyStore::GetPubKey(address, vchPubKeyOut); - CryptedKeyMap::const_iterator mi = mapCryptedKeys.find(address); - if (mi != mapCryptedKeys.end()) - { - vchPubKeyOut = (*mi).second.first; - return true; - } + CryptedKeyMap::const_iterator mi = mapCryptedKeys.find(address); + if (mi != mapCryptedKeys.end()) + { + vchPubKeyOut = (*mi).second.first; + return true; } return false; } bool CCryptoKeyStore::AddSproutSpendingKey(const libzcash::SproutSpendingKey &sk) { - { - LOCK(cs_KeyStore); - if (!IsCrypted()) - return CBasicKeyStore::AddSproutSpendingKey(sk); + LOCK(cs_KeyStore); + if (!fUseCrypto) + return CBasicKeyStore::AddSproutSpendingKey(sk); - if (IsLocked()) - return false; + if (IsLocked()) + return false; - std::vector vchCryptedSecret; - CSecureDataStream ss(SER_NETWORK, PROTOCOL_VERSION); - ss << sk; - CKeyingMaterial vchSecret(ss.begin(), ss.end()); - auto address = sk.address(); - if (!EncryptSecret(vMasterKey, vchSecret, address.GetHash(), vchCryptedSecret)) - return false; + std::vector vchCryptedSecret; + CSecureDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss << sk; + CKeyingMaterial vchSecret(ss.begin(), ss.end()); + auto address = sk.address(); + if (!EncryptSecret(vMasterKey, vchSecret, address.GetHash(), vchCryptedSecret)) + return false; - if (!AddCryptedSproutSpendingKey(address, sk.receiving_key(), vchCryptedSecret)) - return false; - } - return true; + return AddCryptedSproutSpendingKey(address, sk.receiving_key(), vchCryptedSecret); } bool CCryptoKeyStore::AddSaplingSpendingKey( const libzcash::SaplingExtendedSpendingKey &sk, const libzcash::SaplingPaymentAddress &defaultAddr) { - { - LOCK(cs_KeyStore); - if (!IsCrypted()) { - return CBasicKeyStore::AddSaplingSpendingKey(sk, defaultAddr); - } - - if (IsLocked()) { - return false; - } - - std::vector vchCryptedSecret; - CSecureDataStream ss(SER_NETWORK, PROTOCOL_VERSION); - ss << sk; - CKeyingMaterial vchSecret(ss.begin(), ss.end()); - auto extfvk = sk.ToXFVK(); - if (!EncryptSecret(vMasterKey, vchSecret, extfvk.fvk.GetFingerprint(), vchCryptedSecret)) { - return false; - } - - if (!AddCryptedSaplingSpendingKey(extfvk, vchCryptedSecret, defaultAddr)) { - return false; - } + LOCK(cs_KeyStore); + if (!fUseCrypto) { + return CBasicKeyStore::AddSaplingSpendingKey(sk, defaultAddr); } - return true; + + if (IsLocked()) { + return false; + } + + std::vector vchCryptedSecret; + CSecureDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss << sk; + CKeyingMaterial vchSecret(ss.begin(), ss.end()); + auto extfvk = sk.ToXFVK(); + if (!EncryptSecret(vMasterKey, vchSecret, extfvk.fvk.GetFingerprint(), vchCryptedSecret)) { + return false; + } + + return AddCryptedSaplingSpendingKey(extfvk, vchCryptedSecret, defaultAddr); } bool CCryptoKeyStore::AddCryptedSproutSpendingKey( @@ -482,14 +459,12 @@ bool CCryptoKeyStore::AddCryptedSproutSpendingKey( const libzcash::ReceivingKey &rk, const std::vector &vchCryptedSecret) { - { - LOCK(cs_KeyStore); - if (!SetCrypted()) - return false; + LOCK(cs_KeyStore); + if (!SetCrypted()) + return false; - mapCryptedSproutSpendingKeys[address] = vchCryptedSecret; - mapNoteDecryptors.insert(std::make_pair(address, ZCNoteDecryption(rk))); - } + mapCryptedSproutSpendingKeys[address] = vchCryptedSecret; + mapNoteDecryptors.insert(std::make_pair(address, ZCNoteDecryption(rk))); return true; } @@ -498,51 +473,45 @@ bool CCryptoKeyStore::AddCryptedSaplingSpendingKey( const std::vector &vchCryptedSecret, const libzcash::SaplingPaymentAddress &defaultAddr) { - { - LOCK(cs_KeyStore); - if (!SetCrypted()) { - return false; - } - - // if SaplingFullViewingKey is not in SaplingFullViewingKeyMap, add it - if (!AddSaplingFullViewingKey(extfvk.fvk, defaultAddr)) { - return false; - } - - mapCryptedSaplingSpendingKeys[extfvk] = vchCryptedSecret; + LOCK(cs_KeyStore); + if (!SetCrypted()) { + return false; } + + // if SaplingFullViewingKey is not in SaplingFullViewingKeyMap, add it + if (!AddSaplingFullViewingKey(extfvk.fvk, defaultAddr)) { + return false; + } + + mapCryptedSaplingSpendingKeys[extfvk] = vchCryptedSecret; return true; } bool CCryptoKeyStore::GetSproutSpendingKey(const libzcash::SproutPaymentAddress &address, libzcash::SproutSpendingKey &skOut) const { - { - LOCK(cs_KeyStore); - if (!IsCrypted()) - return CBasicKeyStore::GetSproutSpendingKey(address, skOut); + LOCK(cs_KeyStore); + if (!fUseCrypto) + return CBasicKeyStore::GetSproutSpendingKey(address, skOut); - CryptedSproutSpendingKeyMap::const_iterator mi = mapCryptedSproutSpendingKeys.find(address); - if (mi != mapCryptedSproutSpendingKeys.end()) - { - const std::vector &vchCryptedSecret = (*mi).second; - return DecryptSproutSpendingKey(vMasterKey, vchCryptedSecret, address, skOut); - } + CryptedSproutSpendingKeyMap::const_iterator mi = mapCryptedSproutSpendingKeys.find(address); + if (mi != mapCryptedSproutSpendingKeys.end()) + { + const std::vector &vchCryptedSecret = (*mi).second; + return DecryptSproutSpendingKey(vMasterKey, vchCryptedSecret, address, skOut); } return false; } bool CCryptoKeyStore::GetSaplingSpendingKey(const libzcash::SaplingFullViewingKey &fvk, libzcash::SaplingExtendedSpendingKey &skOut) const { - { - LOCK(cs_KeyStore); - if (!IsCrypted()) - return CBasicKeyStore::GetSaplingSpendingKey(fvk, skOut); + LOCK(cs_KeyStore); + if (!fUseCrypto) + return CBasicKeyStore::GetSaplingSpendingKey(fvk, skOut); - for (auto entry : mapCryptedSaplingSpendingKeys) { - if (entry.first.fvk == fvk) { - const std::vector &vchCryptedSecret = entry.second; - return DecryptSaplingSpendingKey(vMasterKey, vchCryptedSecret, entry.first, skOut); - } + for (auto entry : mapCryptedSaplingSpendingKeys) { + if (entry.first.fvk == fvk) { + const std::vector &vchCryptedSecret = entry.second; + return DecryptSaplingSpendingKey(vMasterKey, vchCryptedSecret, entry.first, skOut); } } return false; @@ -552,7 +521,7 @@ bool CCryptoKeyStore::EncryptKeys(CKeyingMaterial& vMasterKeyIn) { { LOCK(cs_KeyStore); - if (!mapCryptedKeys.empty() || IsCrypted()) + if (!mapCryptedKeys.empty() || fUseCrypto) return false; fUseCrypto = true; diff --git a/src/wallet/crypter.h b/src/wallet/crypter.h index 2e4fd0795..443a0606f 100644 --- a/src/wallet/crypter.h +++ b/src/wallet/crypter.h @@ -157,19 +157,14 @@ public: bool IsCrypted() const { + LOCK(cs_KeyStore); return fUseCrypto; } bool IsLocked() const { - if (!IsCrypted()) - return false; - bool result; - { - LOCK(cs_KeyStore); - result = vMasterKey.empty(); - } - return result; + LOCK(cs_KeyStore); + return fUseCrypto && vMasterKey.empty(); } bool Lock(); @@ -183,19 +178,17 @@ public: bool AddKeyPubKey(const CKey& key, const CPubKey &pubkey); bool HaveKey(const CKeyID &address) const { - { - LOCK(cs_KeyStore); - if (!IsCrypted()) - return CBasicKeyStore::HaveKey(address); - return mapCryptedKeys.count(address) > 0; - } - return false; + LOCK(cs_KeyStore); + if (!fUseCrypto) + return CBasicKeyStore::HaveKey(address); + return mapCryptedKeys.count(address) > 0; } bool GetKey(const CKeyID &address, CKey& keyOut) const; bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const; void GetKeys(std::set &setAddress) const { - if (!IsCrypted()) + LOCK(cs_KeyStore); + if (!fUseCrypto) { CBasicKeyStore::GetKeys(setAddress); return; @@ -215,18 +208,16 @@ public: bool AddSproutSpendingKey(const libzcash::SproutSpendingKey &sk); bool HaveSproutSpendingKey(const libzcash::SproutPaymentAddress &address) const { - { - LOCK(cs_KeyStore); - if (!IsCrypted()) - return CBasicKeyStore::HaveSproutSpendingKey(address); - return mapCryptedSproutSpendingKeys.count(address) > 0; - } - return false; + LOCK(cs_KeyStore); + if (!fUseCrypto) + return CBasicKeyStore::HaveSproutSpendingKey(address); + return mapCryptedSproutSpendingKeys.count(address) > 0; } bool GetSproutSpendingKey(const libzcash::SproutPaymentAddress &address, libzcash::SproutSpendingKey &skOut) const; void GetSproutPaymentAddresses(std::set &setAddress) const { - if (!IsCrypted()) + LOCK(cs_KeyStore); + if (!fUseCrypto) { CBasicKeyStore::GetSproutPaymentAddresses(setAddress); return; @@ -249,14 +240,12 @@ public: const libzcash::SaplingPaymentAddress &defaultAddr); bool HaveSaplingSpendingKey(const libzcash::SaplingFullViewingKey &fvk) const { - { - LOCK(cs_KeyStore); - if (!IsCrypted()) - return CBasicKeyStore::HaveSaplingSpendingKey(fvk); - for (auto entry : mapCryptedSaplingSpendingKeys) { - if (entry.first.fvk == fvk) { - return true; - } + LOCK(cs_KeyStore); + if (!fUseCrypto) + return CBasicKeyStore::HaveSaplingSpendingKey(fvk); + for (auto entry : mapCryptedSaplingSpendingKeys) { + if (entry.first.fvk == fvk) { + return true; } } return false;