From 16d140f4a2cd56197df3a911e27677509e839bd1 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 10 Aug 2016 17:47:13 +1200 Subject: [PATCH 1/7] Add support for encrypting spending keys --- src/gtest/test_keystore.cpp | 57 ++++++++++++++++++++ src/keystore.h | 1 + src/wallet/crypter.cpp | 104 +++++++++++++++++++++++++++++++++++- src/wallet/crypter.h | 32 ++++++++++- src/zcash/Address.cpp | 8 +++ src/zcash/Address.hpp | 3 ++ 6 files changed, 203 insertions(+), 2 deletions(-) diff --git a/src/gtest/test_keystore.cpp b/src/gtest/test_keystore.cpp index 11d967e89..b41cb4f4a 100644 --- a/src/gtest/test_keystore.cpp +++ b/src/gtest/test_keystore.cpp @@ -1,6 +1,8 @@ #include #include "keystore.h" +#include "random.h" +#include "wallet/crypter.h" #include "zcash/Address.hpp" TEST(keystore_tests, store_and_retrieve_spending_key) { @@ -41,3 +43,58 @@ TEST(keystore_tests, store_and_retrieve_note_decryptor) { EXPECT_TRUE(keyStore.GetNoteDecryptor(addr, decOut)); EXPECT_EQ(ZCNoteDecryption(sk.viewing_key()), decOut); } + +class TestCCryptoKeyStore : public CCryptoKeyStore +{ +public: + bool EncryptKeys(CKeyingMaterial& vMasterKeyIn) { return CCryptoKeyStore::EncryptKeys(vMasterKeyIn); } + bool Unlock(const CKeyingMaterial& vMasterKeyIn) { return CCryptoKeyStore::Unlock(vMasterKeyIn); } +}; + +TEST(keystore_tests, store_and_retrieve_spending_key_in_encrypted_store) { + TestCCryptoKeyStore keyStore; + uint256 r {GetRandHash()}; + CKeyingMaterial vMasterKey (r.begin(), r.end()); + libzcash::SpendingKey keyOut; + std::set addrs; + + // 1) Test adding a key to an unencrypted key store, then encrypting it + auto sk = libzcash::SpendingKey::random(); + auto addr = sk.address(); + keyStore.AddSpendingKey(sk); + ASSERT_TRUE(keyStore.HaveSpendingKey(addr)); + ASSERT_TRUE(keyStore.GetSpendingKey(addr, keyOut)); + ASSERT_EQ(sk, keyOut); + + ASSERT_TRUE(keyStore.EncryptKeys(vMasterKey)); + ASSERT_TRUE(keyStore.HaveSpendingKey(addr)); + ASSERT_FALSE(keyStore.GetSpendingKey(addr, keyOut)); + + ASSERT_TRUE(keyStore.Unlock(vMasterKey)); + ASSERT_TRUE(keyStore.GetSpendingKey(addr, keyOut)); + ASSERT_EQ(sk, keyOut); + + keyStore.GetPaymentAddresses(addrs); + ASSERT_EQ(1, addrs.size()); + ASSERT_EQ(1, addrs.count(addr)); + + // 2) Test adding a spending key to an already-encrypted key store + auto sk2 = libzcash::SpendingKey::random(); + auto addr2 = sk2.address(); + keyStore.AddSpendingKey(sk2); + ASSERT_TRUE(keyStore.HaveSpendingKey(addr2)); + ASSERT_TRUE(keyStore.GetSpendingKey(addr2, keyOut)); + ASSERT_EQ(sk2, keyOut); + + ASSERT_TRUE(keyStore.Lock()); + ASSERT_TRUE(keyStore.HaveSpendingKey(addr2)); + ASSERT_FALSE(keyStore.GetSpendingKey(addr2, keyOut)); + + ASSERT_TRUE(keyStore.Unlock(vMasterKey)); + ASSERT_TRUE(keyStore.GetSpendingKey(addr2, keyOut)); + ASSERT_EQ(sk2, keyOut); + + keyStore.GetPaymentAddresses(addrs); + ASSERT_EQ(2, addrs.size()); + ASSERT_EQ(1, addrs.count(addr2)); +} diff --git a/src/keystore.h b/src/keystore.h index aa3aefdf2..84595cfb0 100644 --- a/src/keystore.h +++ b/src/keystore.h @@ -172,5 +172,6 @@ public: typedef std::vector > CKeyingMaterial; typedef std::map > > CryptedKeyMap; +typedef std::map > CryptedSpendingKeyMap; #endif // BITCOIN_KEYSTORE_H diff --git a/src/wallet/crypter.cpp b/src/wallet/crypter.cpp index 0b0fb562e..06753bf15 100644 --- a/src/wallet/crypter.cpp +++ b/src/wallet/crypter.cpp @@ -6,6 +6,7 @@ #include "script/script.h" #include "script/standard.h" +#include "streams.h" #include "util.h" #include @@ -135,12 +136,31 @@ static bool DecryptKey(const CKeyingMaterial& vMasterKey, const std::vector& vchCryptedSecret, + const libzcash::PaymentAddress& address, + libzcash::SpendingKey& sk) +{ + CKeyingMaterial vchSecret; + if(!DecryptSecret(vMasterKey, vchCryptedSecret, address.GetHash(), vchSecret)) + return false; + + if (vchSecret.size() != 32) + return false; + + // TODO does this undo the benefits of using CKeyingMaterial? + std::vector serialized(vchSecret.begin(), vchSecret.end()); + CDataStream ss(serialized, SER_NETWORK, PROTOCOL_VERSION); + ss >> sk; + return sk.address() == address; +} + bool CCryptoKeyStore::SetCrypted() { LOCK(cs_KeyStore); if (fUseCrypto) return true; - if (!mapKeys.empty()) + if (!(mapKeys.empty() && mapSpendingKeys.empty())) return false; fUseCrypto = true; return true; @@ -184,6 +204,21 @@ bool CCryptoKeyStore::Unlock(const CKeyingMaterial& vMasterKeyIn) if (fDecryptionThoroughlyChecked) break; } + CryptedSpendingKeyMap::const_iterator skmi = mapCryptedSpendingKeys.begin(); + for (; skmi != mapCryptedSpendingKeys.end(); ++skmi) + { + const libzcash::PaymentAddress &address = (*skmi).first; + const std::vector &vchCryptedSecret = (*skmi).second; + libzcash::SpendingKey sk; + if (!DecryptSpendingKey(vMasterKeyIn, vchCryptedSecret, address, sk)) + { + keyFail = true; + break; + } + keyPass = true; + if (fDecryptionThoroughlyChecked) + break; + } if (keyPass && keyFail) { LogPrintf("The wallet is probably corrupted: Some keys decrypt but not all.\n"); @@ -267,6 +302,59 @@ bool CCryptoKeyStore::GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) co return false; } +bool CCryptoKeyStore::AddSpendingKey(const libzcash::SpendingKey &sk) +{ + { + LOCK(cs_KeyStore); + if (!IsCrypted()) + return CBasicKeyStore::AddSpendingKey(sk); + + if (IsLocked()) + return false; + + std::vector vchCryptedSecret; + CDataStream 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 (!AddCryptedSpendingKey(address, vchCryptedSecret)) + return false; + } + return true; +} + +bool CCryptoKeyStore::AddCryptedSpendingKey(const libzcash::PaymentAddress &address, const std::vector &vchCryptedSecret) +{ + { + LOCK(cs_KeyStore); + if (!SetCrypted()) + return false; + + mapCryptedSpendingKeys[address] = vchCryptedSecret; + } + return true; +} + +bool CCryptoKeyStore::GetSpendingKey(const libzcash::PaymentAddress &address, libzcash::SpendingKey &skOut) const +{ + { + LOCK(cs_KeyStore); + if (!IsCrypted()) + return CBasicKeyStore::GetSpendingKey(address, skOut); + + CryptedSpendingKeyMap::const_iterator mi = mapCryptedSpendingKeys.find(address); + if (mi != mapCryptedSpendingKeys.end()) + { + const std::vector &vchCryptedSecret = (*mi).second; + return DecryptSpendingKey(vMasterKey, vchCryptedSecret, address, skOut); + } + } + return false; +} + bool CCryptoKeyStore::EncryptKeys(CKeyingMaterial& vMasterKeyIn) { { @@ -287,6 +375,20 @@ bool CCryptoKeyStore::EncryptKeys(CKeyingMaterial& vMasterKeyIn) return false; } mapKeys.clear(); + BOOST_FOREACH(SpendingKeyMap::value_type& mSpendingKey, mapSpendingKeys) + { + const libzcash::SpendingKey &sk = mSpendingKey.second; + CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss << sk; + CKeyingMaterial vchSecret(ss.begin(), ss.end()); + libzcash::PaymentAddress address = sk.address(); + std::vector vchCryptedSecret; + if (!EncryptSecret(vMasterKeyIn, vchSecret, address.GetHash(), vchCryptedSecret)) + return false; + if (!AddCryptedSpendingKey(address, vchCryptedSecret)) + return false; + } + mapSpendingKeys.clear(); } return true; } diff --git a/src/wallet/crypter.h b/src/wallet/crypter.h index 70aeb7672..4dfffa50e 100644 --- a/src/wallet/crypter.h +++ b/src/wallet/crypter.h @@ -8,6 +8,7 @@ #include "keystore.h" #include "serialize.h" #include "support/allocators/secure.h" +#include "zcash/Address.hpp" class uint256; @@ -114,10 +115,11 @@ class CCryptoKeyStore : public CBasicKeyStore { private: CryptedKeyMap mapCryptedKeys; + CryptedSpendingKeyMap mapCryptedSpendingKeys; CKeyingMaterial vMasterKey; - //! if fUseCrypto is true, mapKeys must be empty + //! if fUseCrypto is true, mapKeys and mapSpendingKeys must be empty //! if fUseCrypto is false, vMasterKey must be empty bool fUseCrypto; @@ -185,6 +187,34 @@ public: mi++; } } + virtual bool AddCryptedSpendingKey(const libzcash::PaymentAddress &address, const std::vector &vchCryptedSecret); + bool AddSpendingKey(const libzcash::SpendingKey &sk); + bool HaveSpendingKey(const libzcash::PaymentAddress &address) const + { + { + LOCK(cs_KeyStore); + if (!IsCrypted()) + return CBasicKeyStore::HaveSpendingKey(address); + return mapCryptedSpendingKeys.count(address) > 0; + } + return false; + } + bool GetSpendingKey(const libzcash::PaymentAddress &address, libzcash::SpendingKey &skOut) const; + void GetPaymentAddresses(std::set &setAddress) const + { + if (!IsCrypted()) + { + CBasicKeyStore::GetPaymentAddresses(setAddress); + return; + } + setAddress.clear(); + CryptedSpendingKeyMap::const_iterator mi = mapCryptedSpendingKeys.begin(); + while (mi != mapCryptedSpendingKeys.end()) + { + setAddress.insert((*mi).first); + mi++; + } + } /** * Wallet status (encrypted, locked) changed. diff --git a/src/zcash/Address.cpp b/src/zcash/Address.cpp index 9bb32fb6c..3849b2ffc 100644 --- a/src/zcash/Address.cpp +++ b/src/zcash/Address.cpp @@ -1,9 +1,17 @@ #include "Address.hpp" #include "NoteEncryption.hpp" +#include "hash.h" #include "prf.h" +#include "streams.h" namespace libzcash { +uint256 PaymentAddress::GetHash() const { + CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss << *this; + return Hash(ss.begin(), ss.end()); +} + uint256 ViewingKey::pk_enc() { return ZCNoteEncryption::generate_pubkey(*this); } diff --git a/src/zcash/Address.hpp b/src/zcash/Address.hpp index 58caae772..f259818af 100644 --- a/src/zcash/Address.hpp +++ b/src/zcash/Address.hpp @@ -23,6 +23,9 @@ public: READWRITE(pk_enc); } + //! Get the 256-bit SHA256d hash of this payment address. + uint256 GetHash() const; + friend inline bool operator==(const PaymentAddress& a, const PaymentAddress& b) { return a.a_pk == b.a_pk && a.pk_enc == b.pk_enc; } From 6ae516f10b08c7805413086fa350e049c23c0f8f Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 11 Aug 2016 14:32:00 +1200 Subject: [PATCH 2/7] Check we haven't trashed the first key entry with the second --- src/gtest/test_keystore.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/gtest/test_keystore.cpp b/src/gtest/test_keystore.cpp index b41cb4f4a..219f614b7 100644 --- a/src/gtest/test_keystore.cpp +++ b/src/gtest/test_keystore.cpp @@ -96,5 +96,6 @@ TEST(keystore_tests, store_and_retrieve_spending_key_in_encrypted_store) { keyStore.GetPaymentAddresses(addrs); ASSERT_EQ(2, addrs.size()); + ASSERT_EQ(1, addrs.count(addr)); ASSERT_EQ(1, addrs.count(addr2)); } From 3a15b1637e686a924656a43e75e14852ada6c42f Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 16 Aug 2016 22:55:06 +1200 Subject: [PATCH 3/7] Move serialized Zcash address length constants into zcash/Address.hpp --- src/base58.cpp | 16 ++++++---------- src/wallet/crypter.cpp | 2 +- src/zcash/Address.hpp | 3 +++ 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/base58.cpp b/src/base58.cpp index efce9186f..a6152e692 100644 --- a/src/base58.cpp +++ b/src/base58.cpp @@ -313,21 +313,19 @@ bool CBitcoinSecret::SetString(const std::string& strSecret) return SetString(strSecret.c_str()); } -const size_t serializedPaymentAddressSize = 64; - bool CZCPaymentAddress::Set(const libzcash::PaymentAddress& addr) { CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); ss << addr; std::vector addrSerialized(ss.begin(), ss.end()); - assert(addrSerialized.size() == serializedPaymentAddressSize); - SetData(Params().Base58Prefix(CChainParams::ZCPAYMENT_ADDRRESS), &addrSerialized[0], serializedPaymentAddressSize); + assert(addrSerialized.size() == libzcash::SerializedPaymentAddressSize); + SetData(Params().Base58Prefix(CChainParams::ZCPAYMENT_ADDRRESS), &addrSerialized[0], libzcash::SerializedPaymentAddressSize); return true; } libzcash::PaymentAddress CZCPaymentAddress::Get() const { - if (vchData.size() != serializedPaymentAddressSize) { + if (vchData.size() != libzcash::SerializedPaymentAddressSize) { throw std::runtime_error( "payment address is invalid" ); @@ -347,21 +345,19 @@ libzcash::PaymentAddress CZCPaymentAddress::Get() const return ret; } -const size_t serializedSpendingKeySize = 32; - bool CZCSpendingKey::Set(const libzcash::SpendingKey& addr) { CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); ss << addr; std::vector addrSerialized(ss.begin(), ss.end()); - assert(addrSerialized.size() == serializedSpendingKeySize); - SetData(Params().Base58Prefix(CChainParams::ZCSPENDING_KEY), &addrSerialized[0], serializedSpendingKeySize); + assert(addrSerialized.size() == libzcash::SerializedSpendingKeySize); + SetData(Params().Base58Prefix(CChainParams::ZCSPENDING_KEY), &addrSerialized[0], libzcash::SerializedSpendingKeySize); return true; } libzcash::SpendingKey CZCSpendingKey::Get() const { - if (vchData.size() != serializedSpendingKeySize) { + if (vchData.size() != libzcash::SerializedSpendingKeySize) { throw std::runtime_error( "spending key is invalid" ); diff --git a/src/wallet/crypter.cpp b/src/wallet/crypter.cpp index 06753bf15..06ace79c6 100644 --- a/src/wallet/crypter.cpp +++ b/src/wallet/crypter.cpp @@ -145,7 +145,7 @@ static bool DecryptSpendingKey(const CKeyingMaterial& vMasterKey, if(!DecryptSecret(vMasterKey, vchCryptedSecret, address.GetHash(), vchSecret)) return false; - if (vchSecret.size() != 32) + if (vchSecret.size() != libzcash::SerializedSpendingKeySize) return false; // TODO does this undo the benefits of using CKeyingMaterial? diff --git a/src/zcash/Address.hpp b/src/zcash/Address.hpp index f259818af..efae2af22 100644 --- a/src/zcash/Address.hpp +++ b/src/zcash/Address.hpp @@ -7,6 +7,9 @@ namespace libzcash { +const size_t SerializedPaymentAddressSize = 64; +const size_t SerializedSpendingKeySize = 32; + class PaymentAddress { public: uint256 a_pk; From 3bbf2c14227579d029562aaf6a550ea2e6a671a8 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 28 Sep 2016 12:52:38 +1300 Subject: [PATCH 4/7] Test that invalid keys fail to unlock the keystore --- src/gtest/test_keystore.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/gtest/test_keystore.cpp b/src/gtest/test_keystore.cpp index 219f614b7..a82cd711a 100644 --- a/src/gtest/test_keystore.cpp +++ b/src/gtest/test_keystore.cpp @@ -70,6 +70,17 @@ TEST(keystore_tests, store_and_retrieve_spending_key_in_encrypted_store) { ASSERT_TRUE(keyStore.HaveSpendingKey(addr)); ASSERT_FALSE(keyStore.GetSpendingKey(addr, keyOut)); + // Unlocking with a random key should fail + uint256 r2 {GetRandHash()}; + CKeyingMaterial vRandomKey (r2.begin(), r2.end()); + EXPECT_FALSE(keyStore.Unlock(vRandomKey)); + + // Unlocking with a slightly-modified vMasterKey should fail + CKeyingMaterial vModifiedKey (r.begin(), r.end()); + vModifiedKey[0] += 1; + EXPECT_FALSE(keyStore.Unlock(vModifiedKey)); + + // Unlocking with vMasterKey should succeed ASSERT_TRUE(keyStore.Unlock(vMasterKey)); ASSERT_TRUE(keyStore.GetSpendingKey(addr, keyOut)); ASSERT_EQ(sk, keyOut); From 6bffc46a874320a3845aebe81040c2c6b119faab Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 28 Sep 2016 13:44:31 +1300 Subject: [PATCH 5/7] Implement CSecureDataStream for streaming CKeyingMaterial --- src/streams.h | 81 +++++++++++++++++++++++++++--------------- src/wallet/crypter.cpp | 8 ++--- src/wallet/crypter.h | 13 +++++++ 3 files changed, 69 insertions(+), 33 deletions(-) diff --git a/src/streams.h b/src/streams.h index fa1e18def..787d8e297 100644 --- a/src/streams.h +++ b/src/streams.h @@ -27,54 +27,55 @@ * >> and << read and write unformatted data using the above serialization templates. * Fills with data in linear time; some stringstream implementations take N^2 time. */ -class CDataStream +template +class CBaseDataStream { protected: - typedef CSerializeData vector_type; + typedef SerializeType vector_type; vector_type vch; unsigned int nReadPos; public: int nType; int nVersion; - typedef vector_type::allocator_type allocator_type; - typedef vector_type::size_type size_type; - typedef vector_type::difference_type difference_type; - typedef vector_type::reference reference; - typedef vector_type::const_reference const_reference; - typedef vector_type::value_type value_type; - typedef vector_type::iterator iterator; - typedef vector_type::const_iterator const_iterator; - typedef vector_type::reverse_iterator reverse_iterator; + typedef typename vector_type::allocator_type allocator_type; + typedef typename vector_type::size_type size_type; + typedef typename vector_type::difference_type difference_type; + typedef typename vector_type::reference reference; + typedef typename vector_type::const_reference const_reference; + typedef typename vector_type::value_type value_type; + typedef typename vector_type::iterator iterator; + typedef typename vector_type::const_iterator const_iterator; + typedef typename vector_type::reverse_iterator reverse_iterator; - explicit CDataStream(int nTypeIn, int nVersionIn) + explicit CBaseDataStream(int nTypeIn, int nVersionIn) { Init(nTypeIn, nVersionIn); } - CDataStream(const_iterator pbegin, const_iterator pend, int nTypeIn, int nVersionIn) : vch(pbegin, pend) + CBaseDataStream(const_iterator pbegin, const_iterator pend, int nTypeIn, int nVersionIn) : vch(pbegin, pend) { Init(nTypeIn, nVersionIn); } #if !defined(_MSC_VER) || _MSC_VER >= 1300 - CDataStream(const char* pbegin, const char* pend, int nTypeIn, int nVersionIn) : vch(pbegin, pend) + CBaseDataStream(const char* pbegin, const char* pend, int nTypeIn, int nVersionIn) : vch(pbegin, pend) { Init(nTypeIn, nVersionIn); } #endif - CDataStream(const vector_type& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end()) + CBaseDataStream(const vector_type& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end()) { Init(nTypeIn, nVersionIn); } - CDataStream(const std::vector& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end()) + CBaseDataStream(const std::vector& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end()) { Init(nTypeIn, nVersionIn); } - CDataStream(const std::vector& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end()) + CBaseDataStream(const std::vector& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end()) { Init(nTypeIn, nVersionIn); } @@ -86,15 +87,15 @@ public: nVersion = nVersionIn; } - CDataStream& operator+=(const CDataStream& b) + CBaseDataStream& operator+=(const CBaseDataStream& b) { vch.insert(vch.end(), b.begin(), b.end()); return *this; } - friend CDataStream operator+(const CDataStream& a, const CDataStream& b) + friend CBaseDataStream operator+(const CBaseDataStream& a, const CBaseDataStream& b) { - CDataStream ret = a; + CBaseDataStream ret = a; ret += b; return (ret); } @@ -207,7 +208,7 @@ public: // Stream subset // bool eof() const { return size() == 0; } - CDataStream* rdbuf() { return this; } + CBaseDataStream* rdbuf() { return this; } int in_avail() { return size(); } void SetType(int n) { nType = n; } @@ -217,7 +218,7 @@ public: void ReadVersion() { *this >> nVersion; } void WriteVersion() { *this << nVersion; } - CDataStream& read(char* pch, size_t nSize) + CBaseDataStream& read(char* pch, size_t nSize) { // Read from the beginning of the buffer unsigned int nReadPosNext = nReadPos + nSize; @@ -225,7 +226,7 @@ public: { if (nReadPosNext > vch.size()) { - throw std::ios_base::failure("CDataStream::read(): end of data"); + throw std::ios_base::failure("CBaseDataStream::read(): end of data"); } memcpy(pch, &vch[nReadPos], nSize); nReadPos = 0; @@ -237,7 +238,7 @@ public: return (*this); } - CDataStream& ignore(int nSize) + CBaseDataStream& ignore(int nSize) { // Ignore from the beginning of the buffer assert(nSize >= 0); @@ -245,7 +246,7 @@ public: if (nReadPosNext >= vch.size()) { if (nReadPosNext > vch.size()) - throw std::ios_base::failure("CDataStream::ignore(): end of data"); + throw std::ios_base::failure("CBaseDataStream::ignore(): end of data"); nReadPos = 0; vch.clear(); return (*this); @@ -254,7 +255,7 @@ public: return (*this); } - CDataStream& write(const char* pch, size_t nSize) + CBaseDataStream& write(const char* pch, size_t nSize) { // Write to the end of the buffer vch.insert(vch.end(), pch, pch + nSize); @@ -277,7 +278,7 @@ public: } template - CDataStream& operator<<(const T& obj) + CBaseDataStream& operator<<(const T& obj) { // Serialize to this stream ::Serialize(*this, obj, nType, nVersion); @@ -285,7 +286,7 @@ public: } template - CDataStream& operator>>(T& obj) + CBaseDataStream& operator>>(T& obj) { // Unserialize from this stream ::Unserialize(*this, obj, nType, nVersion); @@ -298,6 +299,30 @@ public: } }; +class CDataStream : public CBaseDataStream +{ +public: + explicit CDataStream(int nTypeIn, int nVersionIn) : CBaseDataStream(nTypeIn, nVersionIn) { } + + CDataStream(const_iterator pbegin, const_iterator pend, int nTypeIn, int nVersionIn) : + CBaseDataStream(pbegin, pend, nTypeIn, nVersionIn) { } + +#if !defined(_MSC_VER) || _MSC_VER >= 1300 + CDataStream(const char* pbegin, const char* pend, int nTypeIn, int nVersionIn) : + CBaseDataStream(pbegin, pend, nTypeIn, nVersionIn) { } +#endif + + CDataStream(const vector_type& vchIn, int nTypeIn, int nVersionIn) : + CBaseDataStream(vchIn, nTypeIn, nVersionIn) { } + + CDataStream(const std::vector& vchIn, int nTypeIn, int nVersionIn) : + CBaseDataStream(vchIn, nTypeIn, nVersionIn) { } + + CDataStream(const std::vector& vchIn, int nTypeIn, int nVersionIn) : + CBaseDataStream(vchIn, nTypeIn, nVersionIn) { } + +}; + diff --git a/src/wallet/crypter.cpp b/src/wallet/crypter.cpp index 06ace79c6..5572ebe2f 100644 --- a/src/wallet/crypter.cpp +++ b/src/wallet/crypter.cpp @@ -148,9 +148,7 @@ static bool DecryptSpendingKey(const CKeyingMaterial& vMasterKey, if (vchSecret.size() != libzcash::SerializedSpendingKeySize) return false; - // TODO does this undo the benefits of using CKeyingMaterial? - std::vector serialized(vchSecret.begin(), vchSecret.end()); - CDataStream ss(serialized, SER_NETWORK, PROTOCOL_VERSION); + CSecureDataStream ss(vchSecret, SER_NETWORK, PROTOCOL_VERSION); ss >> sk; return sk.address() == address; } @@ -313,7 +311,7 @@ bool CCryptoKeyStore::AddSpendingKey(const libzcash::SpendingKey &sk) return false; std::vector vchCryptedSecret; - CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + CSecureDataStream ss(SER_NETWORK, PROTOCOL_VERSION); ss << sk; CKeyingMaterial vchSecret(ss.begin(), ss.end()); auto address = sk.address(); @@ -378,7 +376,7 @@ bool CCryptoKeyStore::EncryptKeys(CKeyingMaterial& vMasterKeyIn) BOOST_FOREACH(SpendingKeyMap::value_type& mSpendingKey, mapSpendingKeys) { const libzcash::SpendingKey &sk = mSpendingKey.second; - CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + CSecureDataStream ss(SER_NETWORK, PROTOCOL_VERSION); ss << sk; CKeyingMaterial vchSecret(ss.begin(), ss.end()); libzcash::PaymentAddress address = sk.address(); diff --git a/src/wallet/crypter.h b/src/wallet/crypter.h index 4dfffa50e..e95a841a4 100644 --- a/src/wallet/crypter.h +++ b/src/wallet/crypter.h @@ -7,6 +7,7 @@ #include "keystore.h" #include "serialize.h" +#include "streams.h" #include "support/allocators/secure.h" #include "zcash/Address.hpp" @@ -67,6 +68,18 @@ public: typedef std::vector > CKeyingMaterial; +class CSecureDataStream : public CBaseDataStream +{ +public: + explicit CSecureDataStream(int nTypeIn, int nVersionIn) : CBaseDataStream(nTypeIn, nVersionIn) { } + + CSecureDataStream(const_iterator pbegin, const_iterator pend, int nTypeIn, int nVersionIn) : + CBaseDataStream(pbegin, pend, nTypeIn, nVersionIn) { } + + CSecureDataStream(const vector_type& vchIn, int nTypeIn, int nVersionIn) : + CBaseDataStream(vchIn, nTypeIn, nVersionIn) { } +}; + /** Encryption/decryption context with key information */ class CCrypter { From ad041fceec7a86dcfd405be2bd9b18d268839643 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 29 Sep 2016 11:34:59 +1300 Subject: [PATCH 6/7] Cache note decryptors in encrypted keystore --- src/gtest/test_keystore.cpp | 15 +++++++++++++++ src/wallet/crypter.cpp | 9 ++++++--- src/wallet/crypter.h | 4 +++- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/gtest/test_keystore.cpp b/src/gtest/test_keystore.cpp index a82cd711a..26fde42d9 100644 --- a/src/gtest/test_keystore.cpp +++ b/src/gtest/test_keystore.cpp @@ -56,19 +56,26 @@ TEST(keystore_tests, store_and_retrieve_spending_key_in_encrypted_store) { uint256 r {GetRandHash()}; CKeyingMaterial vMasterKey (r.begin(), r.end()); libzcash::SpendingKey keyOut; + ZCNoteDecryption decOut; std::set addrs; // 1) Test adding a key to an unencrypted key store, then encrypting it auto sk = libzcash::SpendingKey::random(); auto addr = sk.address(); + EXPECT_FALSE(keyStore.GetNoteDecryptor(addr, decOut)); + keyStore.AddSpendingKey(sk); ASSERT_TRUE(keyStore.HaveSpendingKey(addr)); ASSERT_TRUE(keyStore.GetSpendingKey(addr, keyOut)); ASSERT_EQ(sk, keyOut); + EXPECT_TRUE(keyStore.GetNoteDecryptor(addr, decOut)); + EXPECT_EQ(ZCNoteDecryption(sk.viewing_key()), decOut); ASSERT_TRUE(keyStore.EncryptKeys(vMasterKey)); ASSERT_TRUE(keyStore.HaveSpendingKey(addr)); ASSERT_FALSE(keyStore.GetSpendingKey(addr, keyOut)); + EXPECT_TRUE(keyStore.GetNoteDecryptor(addr, decOut)); + EXPECT_EQ(ZCNoteDecryption(sk.viewing_key()), decOut); // Unlocking with a random key should fail uint256 r2 {GetRandHash()}; @@ -92,18 +99,26 @@ TEST(keystore_tests, store_and_retrieve_spending_key_in_encrypted_store) { // 2) Test adding a spending key to an already-encrypted key store auto sk2 = libzcash::SpendingKey::random(); auto addr2 = sk2.address(); + EXPECT_FALSE(keyStore.GetNoteDecryptor(addr2, decOut)); + keyStore.AddSpendingKey(sk2); ASSERT_TRUE(keyStore.HaveSpendingKey(addr2)); ASSERT_TRUE(keyStore.GetSpendingKey(addr2, keyOut)); ASSERT_EQ(sk2, keyOut); + EXPECT_TRUE(keyStore.GetNoteDecryptor(addr2, decOut)); + EXPECT_EQ(ZCNoteDecryption(sk2.viewing_key()), decOut); ASSERT_TRUE(keyStore.Lock()); ASSERT_TRUE(keyStore.HaveSpendingKey(addr2)); ASSERT_FALSE(keyStore.GetSpendingKey(addr2, keyOut)); + EXPECT_TRUE(keyStore.GetNoteDecryptor(addr2, decOut)); + EXPECT_EQ(ZCNoteDecryption(sk2.viewing_key()), decOut); ASSERT_TRUE(keyStore.Unlock(vMasterKey)); ASSERT_TRUE(keyStore.GetSpendingKey(addr2, keyOut)); ASSERT_EQ(sk2, keyOut); + EXPECT_TRUE(keyStore.GetNoteDecryptor(addr2, decOut)); + EXPECT_EQ(ZCNoteDecryption(sk2.viewing_key()), decOut); keyStore.GetPaymentAddresses(addrs); ASSERT_EQ(2, addrs.size()); diff --git a/src/wallet/crypter.cpp b/src/wallet/crypter.cpp index 5572ebe2f..5a066d75d 100644 --- a/src/wallet/crypter.cpp +++ b/src/wallet/crypter.cpp @@ -318,13 +318,15 @@ bool CCryptoKeyStore::AddSpendingKey(const libzcash::SpendingKey &sk) if (!EncryptSecret(vMasterKey, vchSecret, address.GetHash(), vchCryptedSecret)) return false; - if (!AddCryptedSpendingKey(address, vchCryptedSecret)) + if (!AddCryptedSpendingKey(address, sk.viewing_key(), vchCryptedSecret)) return false; } return true; } -bool CCryptoKeyStore::AddCryptedSpendingKey(const libzcash::PaymentAddress &address, const std::vector &vchCryptedSecret) +bool CCryptoKeyStore::AddCryptedSpendingKey(const libzcash::PaymentAddress &address, + const libzcash::ViewingKey &vk, + const std::vector &vchCryptedSecret) { { LOCK(cs_KeyStore); @@ -332,6 +334,7 @@ bool CCryptoKeyStore::AddCryptedSpendingKey(const libzcash::PaymentAddress &addr return false; mapCryptedSpendingKeys[address] = vchCryptedSecret; + mapNoteDecryptors.insert(std::make_pair(address, ZCNoteDecryption(vk))); } return true; } @@ -383,7 +386,7 @@ bool CCryptoKeyStore::EncryptKeys(CKeyingMaterial& vMasterKeyIn) std::vector vchCryptedSecret; if (!EncryptSecret(vMasterKeyIn, vchSecret, address.GetHash(), vchCryptedSecret)) return false; - if (!AddCryptedSpendingKey(address, vchCryptedSecret)) + if (!AddCryptedSpendingKey(address, sk.viewing_key(), vchCryptedSecret)) return false; } mapSpendingKeys.clear(); diff --git a/src/wallet/crypter.h b/src/wallet/crypter.h index e95a841a4..b310b77b0 100644 --- a/src/wallet/crypter.h +++ b/src/wallet/crypter.h @@ -200,7 +200,9 @@ public: mi++; } } - virtual bool AddCryptedSpendingKey(const libzcash::PaymentAddress &address, const std::vector &vchCryptedSecret); + virtual bool AddCryptedSpendingKey(const libzcash::PaymentAddress &address, + const libzcash::ViewingKey &vk, + const std::vector &vchCryptedSecret); bool AddSpendingKey(const libzcash::SpendingKey &sk); bool HaveSpendingKey(const libzcash::PaymentAddress &address) const { From ef3a6a97ae7f4c05773689a2fe2f8c4a629c2c82 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 29 Sep 2016 11:35:18 +1300 Subject: [PATCH 7/7] Use correct lock for spending keys --- src/wallet/crypter.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wallet/crypter.cpp b/src/wallet/crypter.cpp index 5a066d75d..886492aaa 100644 --- a/src/wallet/crypter.cpp +++ b/src/wallet/crypter.cpp @@ -303,7 +303,7 @@ bool CCryptoKeyStore::GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) co bool CCryptoKeyStore::AddSpendingKey(const libzcash::SpendingKey &sk) { { - LOCK(cs_KeyStore); + LOCK(cs_SpendingKeyStore); if (!IsCrypted()) return CBasicKeyStore::AddSpendingKey(sk); @@ -329,7 +329,7 @@ bool CCryptoKeyStore::AddCryptedSpendingKey(const libzcash::PaymentAddress &addr const std::vector &vchCryptedSecret) { { - LOCK(cs_KeyStore); + LOCK(cs_SpendingKeyStore); if (!SetCrypted()) return false; @@ -342,7 +342,7 @@ bool CCryptoKeyStore::AddCryptedSpendingKey(const libzcash::PaymentAddress &addr bool CCryptoKeyStore::GetSpendingKey(const libzcash::PaymentAddress &address, libzcash::SpendingKey &skOut) const { { - LOCK(cs_KeyStore); + LOCK(cs_SpendingKeyStore); if (!IsCrypted()) return CBasicKeyStore::GetSpendingKey(address, skOut); @@ -359,7 +359,7 @@ bool CCryptoKeyStore::GetSpendingKey(const libzcash::PaymentAddress &address, li bool CCryptoKeyStore::EncryptKeys(CKeyingMaterial& vMasterKeyIn) { { - LOCK(cs_KeyStore); + LOCK2(cs_KeyStore, cs_SpendingKeyStore); if (!mapCryptedKeys.empty() || IsCrypted()) return false;