Skip to content

Commit

Permalink
eliminate races: hold cs_KeyStore lock while reading fUseCrypto
Browse files Browse the repository at this point in the history
  • Loading branch information
@LarryRuane
LarryRuane committed Nov 4, 2019
1 parent 5a5094b commit d9fcc2b
Show file tree
Hide file tree
Showing 2 changed files with 128 additions and 170 deletions.
245 changes: 107 additions & 138 deletions src/wallet/crypter.cpp
View file
Expand Up @@ -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();
}

Expand Down Expand Up @@ -291,7 +289,7 @@ bool CCryptoKeyStore::SetHDSeed(const HDSeed& seed)
{
{
LOCK(cs_KeyStore);
if (!IsCrypted()) {
if (!fUseCrypto) {
return CBasicKeyStore::SetHDSeed(seed);
}

Expand All @@ -316,27 +314,25 @@ bool CCryptoKeyStore::SetCryptedHDSeed(
const uint256& seedFp,
const std::vector<unsigned char>& 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;
}
LOCK(cs_KeyStore);
if (!fUseCrypto) {
return false;
}

cryptedHDSeed = std::make_pair(seedFp, vchCryptedSecret);
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();
Expand All @@ -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())
Expand All @@ -356,140 +352,119 @@ 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<unsigned char> vchCryptedSecret;
CKeyingMaterial vchSecret(key.begin(), key.end());
if (!EncryptSecret(vMasterKey, vchSecret, pubkey.GetHash(), vchCryptedSecret))
return false;
std::vector<unsigned char> 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<unsigned char> &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<unsigned char> &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<unsigned char> &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<unsigned char> 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<unsigned char> 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;
}
LOCK(cs_KeyStore);
if (!fUseCrypto) {
return CBasicKeyStore::AddSaplingSpendingKey(sk, defaultAddr);
}

std::vector<unsigned char> 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 (IsLocked()) {
return false;
}

if (!AddCryptedSaplingSpendingKey(extfvk, vchCryptedSecret, defaultAddr)) {
return false;
}
std::vector<unsigned char> 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 true;

return AddCryptedSaplingSpendingKey(extfvk, vchCryptedSecret, defaultAddr);
}

bool CCryptoKeyStore::AddCryptedSproutSpendingKey(
const libzcash::SproutPaymentAddress &address,
const libzcash::ReceivingKey &rk,
const std::vector<unsigned char> &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;
}

Expand All @@ -498,51 +473,45 @@ bool CCryptoKeyStore::AddCryptedSaplingSpendingKey(
const std::vector<unsigned char> &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;
}
LOCK(cs_KeyStore);
if (!SetCrypted()) {
return false;
}

mapCryptedSaplingSpendingKeys[extfvk] = vchCryptedSecret;
// 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<unsigned char> &vchCryptedSecret = (*mi).second;
return DecryptSproutSpendingKey(vMasterKey, vchCryptedSecret, address, skOut);
}
CryptedSproutSpendingKeyMap::const_iterator mi = mapCryptedSproutSpendingKeys.find(address);
if (mi != mapCryptedSproutSpendingKeys.end())
{
const std::vector<unsigned char> &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<unsigned char> &vchCryptedSecret = entry.second;
return DecryptSaplingSpendingKey(vMasterKey, vchCryptedSecret, entry.first, skOut);
}
for (auto entry : mapCryptedSaplingSpendingKeys) {
if (entry.first.fvk == fvk) {
const std::vector<unsigned char> &vchCryptedSecret = entry.second;
return DecryptSaplingSpendingKey(vMasterKey, vchCryptedSecret, entry.first, skOut);
}
}
return false;
Expand All @@ -552,7 +521,7 @@ bool CCryptoKeyStore::EncryptKeys(CKeyingMaterial& vMasterKeyIn)
{
{
LOCK(cs_KeyStore);
if (!mapCryptedKeys.empty() || IsCrypted())
if (!mapCryptedKeys.empty() || fUseCrypto)
return false;

fUseCrypto = true;
Expand Down

0 comments on commit d9fcc2b

Please sign in to comment.