From 0f8a6477825fbaad0d37233bdd3011d748f607ab Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Thu, 8 Nov 2012 19:38:49 +0100 Subject: [PATCH 1/2] don't use memset() in privacy/security relevant code parts As memset() can be optimized out by a compiler it should not be used in privacy/security relevant code parts. OpenSSL provides the safe OPENSSL_cleanse() function in crypto.h, which perfectly does the job of clean and overwrite data. For details see: http://www.viva64.com/en/b/0178/ - change memset() to OPENSSL_cleanse() where appropriate - change a hard-coded number from netbase.cpp into a sizeof() --- src/allocators.h | 5 +++-- src/base58.h | 6 ++++-- src/crypter.cpp | 4 ++-- src/crypter.h | 4 ++-- src/netbase.cpp | 2 +- src/util.cpp | 2 +- 6 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/allocators.h b/src/allocators.h index 99afa10c2..eb2aed672 100644 --- a/src/allocators.h +++ b/src/allocators.h @@ -9,6 +9,7 @@ #include #include #include +#include // for OPENSSL_cleanse() #ifdef WIN32 #ifdef _WIN32_WINNT @@ -212,7 +213,7 @@ struct secure_allocator : public std::allocator { if (p != NULL) { - memset(p, 0, sizeof(T) * n); + OPENSSL_cleanse(p, sizeof(T) * n); LockedPageManager::instance.UnlockRange(p, sizeof(T) * n); } std::allocator::deallocate(p, n); @@ -246,7 +247,7 @@ struct zero_after_free_allocator : public std::allocator void deallocate(T* p, std::size_t n) { if (p != NULL) - memset(p, 0, sizeof(T) * n); + OPENSSL_cleanse(p, sizeof(T) * n); std::allocator::deallocate(p, n); } }; diff --git a/src/base58.h b/src/base58.h index 9dfea86ff..309d35f5d 100644 --- a/src/base58.h +++ b/src/base58.h @@ -17,6 +17,8 @@ #include #include +#include // for OPENSSL_cleanse() + #include "bignum.h" #include "key.h" #include "script.h" @@ -190,7 +192,7 @@ protected: { // zero the memory, as it may contain sensitive data if (!vchData.empty()) - memset(&vchData[0], 0, vchData.size()); + OPENSSL_cleanse(&vchData[0], vchData.size()); } void SetData(int nVersionIn, const void* pdata, size_t nSize) @@ -221,7 +223,7 @@ public: vchData.resize(vchTemp.size() - 1); if (!vchData.empty()) memcpy(&vchData[0], &vchTemp[1], vchData.size()); - memset(&vchTemp[0], 0, vchTemp.size()); + OPENSSL_cleanse(&vchTemp[0], vchData.size()); return true; } diff --git a/src/crypter.cpp b/src/crypter.cpp index 181b8fa00..a2b62a87c 100644 --- a/src/crypter.cpp +++ b/src/crypter.cpp @@ -24,8 +24,8 @@ bool CCrypter::SetKeyFromPassphrase(const SecureString& strKeyData, const std::v if (i != (int)WALLET_CRYPTO_KEY_SIZE) { - memset(&chKey, 0, sizeof chKey); - memset(&chIV, 0, sizeof chIV); + OPENSSL_cleanse(chKey, sizeof(chKey)); + OPENSSL_cleanse(chIV, sizeof(chIV)); return false; } diff --git a/src/crypter.h b/src/crypter.h index 04538a3fa..6f75170ba 100644 --- a/src/crypter.h +++ b/src/crypter.h @@ -76,8 +76,8 @@ public: void CleanKey() { - memset(&chKey, 0, sizeof chKey); - memset(&chIV, 0, sizeof chIV); + OPENSSL_cleanse(chKey, sizeof(chKey)); + OPENSSL_cleanse(chIV, sizeof(chIV)); fKeySet = false; } diff --git a/src/netbase.cpp b/src/netbase.cpp index 7b28e7f1b..9e7307204 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -545,7 +545,7 @@ bool ConnectSocketByName(CService &addr, SOCKET& hSocketRet, const char *pszDest void CNetAddr::Init() { - memset(ip, 0, 16); + memset(ip, 0, sizeof(ip)); } void CNetAddr::SetIP(const CNetAddr& ipIn) diff --git a/src/util.cpp b/src/util.cpp index 916288645..03014a5da 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -156,7 +156,7 @@ void RandAddSeedPerfmon() if (ret == ERROR_SUCCESS) { RAND_add(pdata, nSize, nSize/100.0); - memset(pdata, 0, nSize); + OPENSSL_cleanse(pdata, nSize); printf("RandAddSeed() %lu bytes\n", nSize); } #endif From d0b0925be9f94068deb98940103a7b8a8c56f027 Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Fri, 9 Nov 2012 12:50:59 +0100 Subject: [PATCH 2/2] make CBase58Data class use zero_after_free_allocator - this way there is no need for an explicit destructor, who does the same thing anyway --- src/base58.h | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/base58.h b/src/base58.h index 309d35f5d..be8a541f6 100644 --- a/src/base58.h +++ b/src/base58.h @@ -17,11 +17,11 @@ #include #include -#include // for OPENSSL_cleanse() #include "bignum.h" #include "key.h" #include "script.h" +#include "allocators.h" static const char* pszBase58 = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz"; @@ -180,7 +180,8 @@ protected: unsigned char nVersion; // the actually encoded data - std::vector vchData; + typedef std::vector > vector_uchar; + vector_uchar vchData; CBase58Data() { @@ -188,13 +189,6 @@ protected: vchData.clear(); } - ~CBase58Data() - { - // zero the memory, as it may contain sensitive data - if (!vchData.empty()) - OPENSSL_cleanse(&vchData[0], vchData.size()); - } - void SetData(int nVersionIn, const void* pdata, size_t nSize) { nVersion = nVersionIn; @@ -459,4 +453,4 @@ public: } }; -#endif +#endif // BITCOIN_BASE58_H