Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CCrypter using secure allocator vector. #1488

Merged
merged 3 commits into from
May 17, 2020
Merged

CCrypter using secure allocator vector. #1488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
121e5c0
[Wallet] Change CCrypter to use vectors with secure allocator
furszy Apr 3, 2020
366bc8b
Get rid of LockObject and UnlockObject unused methods.
furszy Apr 3, 2020
f8fd095
Add missing lock in crypter GetKeys.
furszy Apr 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 0 additions & 17 deletions src/allocators.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,23 +160,6 @@ class LockedPageManager : public LockedPageManagerBase<MemoryPageLocker>
static boost::once_flag init_flag;
};

//
// Functions for directly locking/unlocking memory objects.
// Intended for non-dynamically allocated structures.
//
template <typename T>
void LockObject(const T& t)
{
LockedPageManager::Instance().LockRange((void*)(&t), sizeof(T));
}

template <typename T>
void UnlockObject(const T& t)
{
memory_cleanse((void*)(&t), sizeof(T));
LockedPageManager::Instance().UnlockRange((void*)(&t), sizeof(T));
}

//
// Allocator that locks its contents from being paged
// out of memory and clears its contents before deletion.
Expand Down
14 changes: 7 additions & 7 deletions src/crypter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ 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;
}

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

memcpy(&chKey[0], &chNewKey[0], sizeof chKey);
memcpy(&chIV[0], &chNewIV[0], sizeof chIV);
memcpy(vchKey.data(), chNewKey.data(), chNewKey.size());
memcpy(vchIV.data(), chNewIV.data(), chNewIV.size());

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

AES256CBCEncrypt enc(chKey, chIV, true);
AES256CBCEncrypt enc(vchKey.data(), vchIV.data(), true);
size_t nLen = enc.Encrypt(&vchPlaintext[0], vchPlaintext.size(), &vchCiphertext[0]);
if(nLen < vchPlaintext.size())
return false;
Expand All @@ -98,7 +98,7 @@ bool CCrypter::Decrypt(const std::vector<unsigned char>& vchCiphertext, CKeyingM
// plaintext will always be equal to or lesser than length of ciphertext
int nLen = vchCiphertext.size();
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;
Expand Down
20 changes: 7 additions & 13 deletions src/crypter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,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<unsigned char, secure_allocator<unsigned char>> vchKey;
std::vector<unsigned char, secure_allocator<unsigned char>> vchIV;
bool fKeySet;

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

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);
}
};

Expand Down Expand Up @@ -179,6 +172,7 @@ class CCryptoKeyStore : public CBasicKeyStore
bool GetPubKey(const CKeyID& address, CPubKey& vchPubKeyOut) const;
void GetKeys(std::set<CKeyID>& setAddress) const
{
LOCK(cs_KeyStore);
if (!IsCrypted()) {
CBasicKeyStore::GetKeys(setAddress);
return;
Expand Down
12 changes: 6 additions & 6 deletions src/wallet/test/crypto_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,10 @@ static void TestPassphraseSingle(const std::vector<unsigned char>& 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, \
Expand All @@ -127,7 +127,7 @@ static void TestDecrypt(const CCrypter& crypt, const std::vector<unsigned char>&
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
Expand All @@ -152,7 +152,7 @@ static void TestEncryptSingle(const CCrypter& crypt, const CKeyingMaterial& vchP
std::vector<unsigned char> 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);

Expand Down