From b997b30a91964d003de5c474ef8f618c2df13914 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Sun, 18 Sep 2016 08:23:29 +0200 Subject: [PATCH] wallet: Change CCrypter to use vectors with secure allocator Change CCrypter to use vectors with secure allocator instead of buffers on in the object itself which will end up on the stack. This avoids having to call LockedPageManager to lock stack memory pages to prevent the memory from being swapped to disk. This is wasteful. --- src/wallet/crypter.cpp | 14 +++++++------- src/wallet/crypter.h | 19 ++++++------------- src/wallet/test/crypto_tests.cpp | 12 ++++++------ 3 files changed, 19 insertions(+), 26 deletions(-) diff --git a/src/wallet/crypter.cpp b/src/wallet/crypter.cpp index 5fcf52a35..cefc13732 100644 --- a/src/wallet/crypter.cpp +++ b/src/wallet/crypter.cpp @@ -49,12 +49,12 @@ bool CCrypter::SetKeyFromPassphrase(const SecureString& strKeyData, const std::v int i = 0; if (nDerivationMethod == 0) - i = BytesToKeySHA512AES(chSalt, strKeyData, nRounds, chKey, chIV); + i = BytesToKeySHA512AES(chSalt, strKeyData, nRounds, vchKey.data(), vchIV.data()); if (i != (int)WALLET_CRYPTO_KEY_SIZE) { - memory_cleanse(chKey, sizeof(chKey)); - memory_cleanse(chIV, sizeof(chIV)); + memory_cleanse(vchKey.data(), vchKey.size()); + memory_cleanse(vchIV.data(), vchIV.size()); return false; } @@ -67,8 +67,8 @@ bool CCrypter::SetKey(const CKeyingMaterial& chNewKey, const std::vector& vchCiphertext, CKeyingM vchPlaintext.resize(nLen); - AES256CBCDecrypt dec(chKey, chIV, true); + AES256CBCDecrypt dec(vchKey.data(), vchIV.data(), true); nLen = dec.Decrypt(&vchCiphertext[0], vchCiphertext.size(), &vchPlaintext[0]); if(nLen == 0) return false; diff --git a/src/wallet/crypter.h b/src/wallet/crypter.h index 4608271f3..7b291cf04 100644 --- a/src/wallet/crypter.h +++ b/src/wallet/crypter.h @@ -91,8 +91,8 @@ class CCrypter { friend class wallet_crypto::TestCrypter; // for test access to chKey/chIV private: - unsigned char chKey[WALLET_CRYPTO_KEY_SIZE]; - unsigned char chIV[WALLET_CRYPTO_IV_SIZE]; + std::vector> vchKey; + std::vector> vchIV; bool fKeySet; int BytesToKeySHA512AES(const std::vector& chSalt, const SecureString& strKeyData, int count, unsigned char *key,unsigned char *iv) const; @@ -105,28 +105,21 @@ public: void CleanKey() { - memory_cleanse(chKey, sizeof(chKey)); - memory_cleanse(chIV, sizeof(chIV)); + memory_cleanse(vchKey.data(), vchKey.size()); + memory_cleanse(vchIV.data(), vchIV.size()); fKeySet = false; } CCrypter() { fKeySet = false; - - // Try to keep the key data out of swap (and be a bit over-careful to keep the IV that we don't even use out of swap) - // Note that this does nothing about suspend-to-disk (which will put all our key data on disk) - // Note as well that at no point in this program is any attempt made to prevent stealing of keys by reading the memory of the running process. - LockedPageManager::Instance().LockRange(&chKey[0], sizeof chKey); - LockedPageManager::Instance().LockRange(&chIV[0], sizeof chIV); + vchKey.resize(WALLET_CRYPTO_KEY_SIZE); + vchIV.resize(WALLET_CRYPTO_IV_SIZE); } ~CCrypter() { CleanKey(); - - LockedPageManager::Instance().UnlockRange(&chKey[0], sizeof chKey); - LockedPageManager::Instance().UnlockRange(&chIV[0], sizeof chIV); } }; diff --git a/src/wallet/test/crypto_tests.cpp b/src/wallet/test/crypto_tests.cpp index 281661334..cd7aa989b 100644 --- a/src/wallet/test/crypto_tests.cpp +++ b/src/wallet/test/crypto_tests.cpp @@ -95,10 +95,10 @@ static void TestPassphraseSingle(const std::vector& vchSalt, cons OldSetKeyFromPassphrase(passphrase, vchSalt, rounds, 0, chKey, chIV); - BOOST_CHECK_MESSAGE(memcmp(chKey, crypt.chKey, sizeof(chKey)) == 0, \ - HexStr(chKey, chKey+sizeof(chKey)) + std::string(" != ") + HexStr(crypt.chKey, crypt.chKey + (sizeof crypt.chKey))); - BOOST_CHECK_MESSAGE(memcmp(chIV, crypt.chIV, sizeof(chIV)) == 0, \ - HexStr(chIV, chIV+sizeof(chIV)) + std::string(" != ") + HexStr(crypt.chIV, crypt.chIV + (sizeof crypt.chIV))); + BOOST_CHECK_MESSAGE(memcmp(chKey, crypt.vchKey.data(), crypt.vchKey.size()) == 0, \ + HexStr(chKey, chKey+sizeof(chKey)) + std::string(" != ") + HexStr(crypt.vchKey)); + BOOST_CHECK_MESSAGE(memcmp(chIV, crypt.vchIV.data(), crypt.vchIV.size()) == 0, \ + HexStr(chIV, chIV+sizeof(chIV)) + std::string(" != ") + HexStr(crypt.vchIV)); if(!correctKey.empty()) BOOST_CHECK_MESSAGE(memcmp(chKey, &correctKey[0], sizeof(chKey)) == 0, \ @@ -125,7 +125,7 @@ static void TestDecrypt(const CCrypter& crypt, const std::vector& CKeyingMaterial vchDecrypted2; int result1, result2; result1 = crypt.Decrypt(vchCiphertext, vchDecrypted1); - result2 = OldDecrypt(vchCiphertext, vchDecrypted2, crypt.chKey, crypt.chIV); + result2 = OldDecrypt(vchCiphertext, vchDecrypted2, crypt.vchKey.data(), crypt.vchIV.data()); BOOST_CHECK(result1 == result2); // These two should be equal. However, OpenSSL 1.0.1j introduced a change @@ -150,7 +150,7 @@ static void TestEncryptSingle(const CCrypter& crypt, const CKeyingMaterial& vchP std::vector vchCiphertext2; int result1 = crypt.Encrypt(vchPlaintext, vchCiphertext1); - int result2 = OldEncrypt(vchPlaintext, vchCiphertext2, crypt.chKey, crypt.chIV); + int result2 = OldEncrypt(vchPlaintext, vchCiphertext2, crypt.vchKey.data(), crypt.vchIV.data()); BOOST_CHECK(result1 == result2); BOOST_CHECK(vchCiphertext1 == vchCiphertext2);