Skip to content

Commit 999e4c9

Browse files
laanwjlaanwj
committed
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.
1 parent 97c7f73 commit 999e4c9

File tree

3 files changed

+19
-26
lines changed

3 files changed

+19
-26
lines changed

‎src/wallet/crypter.cpp‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -48,12 +48,12 @@ bool CCrypter::SetKeyFromPassphrase(const SecureString& strKeyData, const std::v
4848

4949
int i = 0;
5050
if (nDerivationMethod == 0)
51-
i = BytesToKeySHA512AES(chSalt, strKeyData, nRounds, chKey, chIV);
51+
i = BytesToKeySHA512AES(chSalt, strKeyData, nRounds, vchKey.data(), vchIV.data());
5252

5353
if (i != (int)WALLET_CRYPTO_KEY_SIZE)
5454
{
55-
memory_cleanse(chKey, sizeof(chKey));
56-
memory_cleanse(chIV, sizeof(chIV));
55+
memory_cleanse(vchKey.data(), vchKey.size());
56+
memory_cleanse(vchIV.data(), vchIV.size());
5757
return false;
5858
}
5959

@@ -66,8 +66,8 @@ bool CCrypter::SetKey(const CKeyingMaterial& chNewKey, const std::vector<unsigne
6666
if (chNewKey.size() != WALLET_CRYPTO_KEY_SIZE || chNewIV.size() != WALLET_CRYPTO_IV_SIZE)
6767
return false;
6868

69-
memcpy(&chKey[0], &chNewKey[0], sizeof chKey);
70-
memcpy(&chIV[0], &chNewIV[0], sizeof chIV);
69+
memcpy(vchKey.data(), chNewKey.data(), chNewKey.size());
70+
memcpy(vchIV.data(), chNewIV.data(), chNewIV.size());
7171

7272
fKeySet = true;
7373
return true;
@@ -82,7 +82,7 @@ bool CCrypter::Encrypt(const CKeyingMaterial& vchPlaintext, std::vector<unsigned
8282
// n + AES_BLOCKSIZE bytes
8383
vchCiphertext.resize(vchPlaintext.size() + AES_BLOCKSIZE);
8484

85-
AES256CBCEncrypt enc(chKey, chIV, true);
85+
AES256CBCEncrypt enc(vchKey.data(), vchIV.data(), true);
8686
size_t nLen = enc.Encrypt(&vchPlaintext[0], vchPlaintext.size(), &vchCiphertext[0]);
8787
if(nLen < vchPlaintext.size())
8888
return false;
@@ -101,7 +101,7 @@ bool CCrypter::Decrypt(const std::vector<unsigned char>& vchCiphertext, CKeyingM
101101

102102
vchPlaintext.resize(nLen);
103103

104-
AES256CBCDecrypt dec(chKey, chIV, true);
104+
AES256CBCDecrypt dec(vchKey.data(), vchIV.data(), true);
105105
nLen = dec.Decrypt(&vchCiphertext[0], vchCiphertext.size(), &vchPlaintext[0]);
106106
if(nLen == 0)
107107
return false;

‎src/wallet/crypter.h‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,8 @@ class CCrypter
7777
{
7878
friend class wallet_crypto::TestCrypter; // for test access to chKey/chIV
7979
private:
80-
unsigned char chKey[WALLET_CRYPTO_KEY_SIZE];
81-
unsigned char chIV[WALLET_CRYPTO_IV_SIZE];
80+
std::vector<unsigned char, secure_allocator<unsigned char>> vchKey;
81+
std::vector<unsigned char, secure_allocator<unsigned char>> vchIV;
8282
bool fKeySet;
8383

8484
int BytesToKeySHA512AES(const std::vector<unsigned char>& chSalt, const SecureString& strKeyData, int count, unsigned char *key,unsigned char *iv) const;
@@ -91,28 +91,21 @@ friend class wallet_crypto::TestCrypter; // for test access to chKey/chIV
9191

9292
void CleanKey()
9393
{
94-
memory_cleanse(chKey, sizeof(chKey));
95-
memory_cleanse(chIV, sizeof(chIV));
94+
memory_cleanse(vchKey.data(), vchKey.size());
95+
memory_cleanse(vchIV.data(), vchIV.size());
9696
fKeySet = false;
9797
}
9898

9999
CCrypter()
100100
{
101101
fKeySet = false;
102-
103-
// 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)
104-
// Note that this does nothing about suspend-to-disk (which will put all our key data on disk)
105-
// 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.
106-
LockedPageManager::Instance().LockRange(&chKey[0], sizeof chKey);
107-
LockedPageManager::Instance().LockRange(&chIV[0], sizeof chIV);
102+
vchKey.resize(WALLET_CRYPTO_KEY_SIZE);
103+
vchIV.resize(WALLET_CRYPTO_IV_SIZE);
108104
}
109105

110106
~CCrypter()
111107
{
112108
CleanKey();
113-
114-
LockedPageManager::Instance().UnlockRange(&chKey[0], sizeof chKey);
115-
LockedPageManager::Instance().UnlockRange(&chIV[0], sizeof chIV);
116109
}
117110
};
118111

‎src/wallet/test/crypto_tests.cpp‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -97,10 +97,10 @@ static void TestPassphraseSingle(const std::vector<unsigned char>& vchSalt, cons
9797

9898
OldSetKeyFromPassphrase(passphrase, vchSalt, rounds, 0, chKey, chIV);
9999

100-
BOOST_CHECK_MESSAGE(memcmp(chKey, crypt.chKey, sizeof(chKey)) == 0, \
101-
HexStr(chKey, chKey+sizeof(chKey)) + std::string(" != ") + HexStr(crypt.chKey, crypt.chKey + (sizeof crypt.chKey)));
102-
BOOST_CHECK_MESSAGE(memcmp(chIV, crypt.chIV, sizeof(chIV)) == 0, \
103-
HexStr(chIV, chIV+sizeof(chIV)) + std::string(" != ") + HexStr(crypt.chIV, crypt.chIV + (sizeof crypt.chIV)));
100+
BOOST_CHECK_MESSAGE(memcmp(chKey, crypt.vchKey.data(), crypt.vchKey.size()) == 0, \
101+
HexStr(chKey, chKey+sizeof(chKey)) + std::string(" != ") + HexStr(crypt.vchKey));
102+
BOOST_CHECK_MESSAGE(memcmp(chIV, crypt.vchIV.data(), crypt.vchIV.size()) == 0, \
103+
HexStr(chIV, chIV+sizeof(chIV)) + std::string(" != ") + HexStr(crypt.vchIV));
104104

105105
if(!correctKey.empty())
106106
BOOST_CHECK_MESSAGE(memcmp(chKey, &correctKey[0], sizeof(chKey)) == 0, \
@@ -127,7 +127,7 @@ static void TestDecrypt(const CCrypter& crypt, const std::vector<unsigned char>&
127127
CKeyingMaterial vchDecrypted2;
128128
int result1, result2;
129129
result1 = crypt.Decrypt(vchCiphertext, vchDecrypted1);
130-
result2 = OldDecrypt(vchCiphertext, vchDecrypted2, crypt.chKey, crypt.chIV);
130+
result2 = OldDecrypt(vchCiphertext, vchDecrypted2, crypt.vchKey.data(), crypt.vchIV.data());
131131
BOOST_CHECK(result1 == result2);
132132

133133
// These two should be equal. However, OpenSSL 1.0.1j introduced a change
@@ -152,7 +152,7 @@ static void TestEncryptSingle(const CCrypter& crypt, const CKeyingMaterial& vchP
152152
std::vector<unsigned char> vchCiphertext2;
153153
int result1 = crypt.Encrypt(vchPlaintext, vchCiphertext1);
154154

155-
int result2 = OldEncrypt(vchPlaintext, vchCiphertext2, crypt.chKey, crypt.chIV);
155+
int result2 = OldEncrypt(vchPlaintext, vchCiphertext2, crypt.vchKey.data(), crypt.vchIV.data());
156156
BOOST_CHECK(result1 == result2);
157157
BOOST_CHECK(vchCiphertext1 == vchCiphertext2);
158158

0 commit comments

Comments
 (0)