From f4d1fc259b5a62580d952c180b1189ccaa6af1bc Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Sun, 18 Sep 2016 08:40:14 +0200 Subject: [PATCH] wallet: Get rid of LockObject and UnlockObject calls in key.h Replace these with vectors allocated from the secure allocator. This avoids mlock syscall churn on stack pages, as well as makes it possible to get rid of these functions. Please review this commit and the previous one carefully that no `sizeof(vectortype)` remains in the memcpys and memcmps usage (ick!), and `.data()` or `&vec[x]` is used as appropriate instead of &vec. --- src/key.cpp | 34 ++++++++++++++-------------------- src/key.h | 27 +++++++++------------------ src/support/pagelocker.h | 17 ----------------- 3 files changed, 23 insertions(+), 55 deletions(-) diff --git a/src/key.cpp b/src/key.cpp index aae9b042a..b3ea98fb9 100644 --- a/src/key.cpp +++ b/src/key.cpp @@ -125,8 +125,8 @@ bool CKey::Check(const unsigned char *vch) { void CKey::MakeNewKey(bool fCompressedIn) { do { - GetStrongRandBytes(vch, sizeof(vch)); - } while (!Check(vch)); + GetStrongRandBytes(keydata.data(), keydata.size()); + } while (!Check(keydata.data())); fValid = true; fCompressed = fCompressedIn; } @@ -224,20 +224,18 @@ bool CKey::Load(CPrivKey &privkey, CPubKey &vchPubKey, bool fSkipCheck=false) { bool CKey::Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const ChainCode& cc) const { assert(IsValid()); assert(IsCompressed()); - unsigned char out[64]; - LockObject(out); + std::vector> vout(64); if ((nChild >> 31) == 0) { CPubKey pubkey = GetPubKey(); assert(pubkey.begin() + 33 == pubkey.end()); - BIP32Hash(cc, nChild, *pubkey.begin(), pubkey.begin()+1, out); + BIP32Hash(cc, nChild, *pubkey.begin(), pubkey.begin()+1, vout.data()); } else { assert(begin() + 32 == end()); - BIP32Hash(cc, nChild, 0, begin(), out); + BIP32Hash(cc, nChild, 0, begin(), vout.data()); } - memcpy(ccChild.begin(), out+32, 32); + memcpy(ccChild.begin(), vout.data()+32, 32); memcpy((unsigned char*)keyChild.begin(), begin(), 32); - bool ret = secp256k1_ec_privkey_tweak_add(secp256k1_context_sign, (unsigned char*)keyChild.begin(), out); - UnlockObject(out); + bool ret = secp256k1_ec_privkey_tweak_add(secp256k1_context_sign, (unsigned char*)keyChild.begin(), vout.data()); keyChild.fCompressed = true; keyChild.fValid = ret; return ret; @@ -253,12 +251,10 @@ bool CExtKey::Derive(CExtKey &out, unsigned int _nChild) const { void CExtKey::SetMaster(const unsigned char *seed, unsigned int nSeedLen) { static const unsigned char hashkey[] = {'B','i','t','c','o','i','n',' ','s','e','e','d'}; - unsigned char out[64]; - LockObject(out); - CHMAC_SHA512(hashkey, sizeof(hashkey)).Write(seed, nSeedLen).Finalize(out); - key.Set(&out[0], &out[32], true); - memcpy(chaincode.begin(), &out[32], 32); - UnlockObject(out); + std::vector> vout(64); + CHMAC_SHA512(hashkey, sizeof(hashkey)).Write(seed, nSeedLen).Finalize(vout.data()); + key.Set(&vout[0], &vout[32], true); + memcpy(chaincode.begin(), &vout[32], 32); nDepth = 0; nChild = 0; memset(vchFingerprint, 0, sizeof(vchFingerprint)); @@ -308,12 +304,10 @@ void ECC_Start() { { // Pass in a random blinding seed to the secp256k1 context. - unsigned char seed[32]; - LockObject(seed); - GetRandBytes(seed, 32); - bool ret = secp256k1_context_randomize(ctx, seed); + std::vector> vseed(32); + GetRandBytes(vseed.data(), 32); + bool ret = secp256k1_context_randomize(ctx, vseed.data()); assert(ret); - UnlockObject(seed); } secp256k1_context_sign = ctx; diff --git a/src/key.h b/src/key.h index b589710ba..48a07d62c 100644 --- a/src/key.h +++ b/src/key.h @@ -43,9 +43,7 @@ private: bool fCompressed; //! The actual byte data - unsigned char vch[32]; - - static_assert(sizeof(vch) == 32, "vch must be 32 bytes in length to not break serialization"); + std::vector > keydata; //! Check whether the 32-byte array pointed to be vch is valid keydata. bool static Check(const unsigned char* vch); @@ -54,37 +52,30 @@ public: //! Construct an invalid private key. CKey() : fValid(false), fCompressed(false) { - LockObject(vch); - } - - //! Copy constructor. This is necessary because of memlocking. - CKey(const CKey& secret) : fValid(secret.fValid), fCompressed(secret.fCompressed) - { - LockObject(vch); - memcpy(vch, secret.vch, sizeof(vch)); + // Important: vch must be 32 bytes in length to not break serialization + keydata.resize(32); } //! Destructor (again necessary because of memlocking). ~CKey() { - UnlockObject(vch); } friend bool operator==(const CKey& a, const CKey& b) { return a.fCompressed == b.fCompressed && a.size() == b.size() && - memcmp(&a.vch[0], &b.vch[0], a.size()) == 0; + memcmp(a.keydata.data(), b.keydata.data(), a.size()) == 0; } //! Initialize using begin and end iterators to byte data. template void Set(const T pbegin, const T pend, bool fCompressedIn) { - if (pend - pbegin != sizeof(vch)) { + if (size_t(pend - pbegin) != keydata.size()) { fValid = false; } else if (Check(&pbegin[0])) { - memcpy(vch, (unsigned char*)&pbegin[0], sizeof(vch)); + memcpy(keydata.data(), (unsigned char*)&pbegin[0], keydata.size()); fValid = true; fCompressed = fCompressedIn; } else { @@ -93,9 +84,9 @@ public: } //! Simple read-only vector-like interface. - unsigned int size() const { return (fValid ? sizeof(vch) : 0); } - const unsigned char* begin() const { return vch; } - const unsigned char* end() const { return vch + size(); } + unsigned int size() const { return (fValid ? keydata.size() : 0); } + const unsigned char* begin() const { return keydata.data(); } + const unsigned char* end() const { return keydata.data() + size(); } //! Check whether this private key is valid. bool IsValid() const { return fValid; } diff --git a/src/support/pagelocker.h b/src/support/pagelocker.h index 538bf3945..042144fad 100644 --- a/src/support/pagelocker.h +++ b/src/support/pagelocker.h @@ -157,21 +157,4 @@ private: static boost::once_flag init_flag; }; -// -// Functions for directly locking/unlocking memory objects. -// Intended for non-dynamically allocated structures. -// -template -void LockObject(const T& t) -{ - LockedPageManager::Instance().LockRange((void*)(&t), sizeof(T)); -} - -template -void UnlockObject(const T& t) -{ - memory_cleanse((void*)(&t), sizeof(T)); - LockedPageManager::Instance().UnlockRange((void*)(&t), sizeof(T)); -} - #endif // BITCOIN_SUPPORT_PAGELOCKER_H