From 2d2e5bdcf46d3b4b50d07edb2e010cf7e1cde1b1 Mon Sep 17 00:00:00 2001 From: Gregory Maxwell Date: Wed, 24 Oct 2012 01:41:52 -0400 Subject: [PATCH 1/3] Fixes a race condition in CreateNewBlock. CreateNewBlock was reading pindexBest at the start before taking the lock so it was possible to have the the block content not match the prevheader. (Partial of faff50d129b6d4b9e6397ac989218e83a26ae692) --- src/main.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index 26adbf71780..e740d8e3198 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3032,7 +3032,7 @@ public: CBlock* CreateNewBlock(CReserveKey& reservekey) { - CBlockIndex* pindexPrev = pindexBest; + CBlockIndex* pindexPrev; // Create new block auto_ptr pblock(new CBlock()); @@ -3054,6 +3054,7 @@ CBlock* CreateNewBlock(CReserveKey& reservekey) CRITICAL_BLOCK(cs_main) CRITICAL_BLOCK(cs_mapTransactions) { + pindexPrev = pindexBest; CTxDB txdb("r"); // Priority order to process transactions From caeafd1bd1b217276005c6bb422136f379d881cf Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Tue, 18 Sep 2012 18:26:02 +0200 Subject: [PATCH 2/3] fix some double-spaces in strings (partial of 6b3783a9c9cc47afcf72aa0a86ea26122392efdb) --- src/init.cpp | 2 +- src/rpc.cpp | 2 +- src/wallet.cpp | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 5faa6043946..516927457a4 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -358,7 +358,7 @@ bool AppInit2(int argc, char* argv[]) static boost::interprocess::file_lock lock(strLockFile.c_str()); if (!lock.try_lock()) { - wxMessageBox(strprintf(_("Cannot obtain a lock on data directory %s. Bitcoin is probably already running."), GetDataDir().c_str()), "Bitcoin"); + wxMessageBox(strprintf(_("Cannot obtain a lock on data directory %s. Bitcoin is probably already running."), GetDataDir().c_str()), "Bitcoin"); return false; } diff --git a/src/rpc.cpp b/src/rpc.cpp index 67bd88c9205..824297beafc 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -1479,7 +1479,7 @@ Value encryptwallet(const Array& params, bool fHelp) // slack space in .dat files; that is bad if the old data is // unencrypted private keys. So: CreateThread(Shutdown, NULL); - return "wallet encrypted; bitcoin server stopping, restart to run with encrypted wallet. The keypool has been flushed, you need to make a new backup."; + return "wallet encrypted; bitcoin server stopping, restart to run with encrypted wallet. The keypool has been flushed, you need to make a new backup."; } diff --git a/src/wallet.cpp b/src/wallet.cpp index b482aeae244..f6e85bc5563 100644 --- a/src/wallet.cpp +++ b/src/wallet.cpp @@ -1081,7 +1081,7 @@ string CWallet::SendMoney(CScript scriptPubKey, int64 nValue, CWalletTx& wtxNew, if (IsLocked()) { - string strError = _("Error: Wallet locked, unable to create transaction "); + string strError = _("Error: Wallet locked, unable to create transaction."); printf("SendMoney() : %s", strError.c_str()); return strError; } @@ -1089,9 +1089,9 @@ string CWallet::SendMoney(CScript scriptPubKey, int64 nValue, CWalletTx& wtxNew, { string strError; if (nValue + nFeeRequired > GetBalance()) - strError = strprintf(_("Error: This transaction requires a transaction fee of at least %s because of its amount, complexity, or use of recently received funds "), FormatMoney(nFeeRequired).c_str()); + strError = strprintf(_("Error: This transaction requires a transaction fee of at least %s because of its amount, complexity, or use of recently received funds."), FormatMoney(nFeeRequired).c_str()); else - strError = _("Error: Transaction creation failed "); + strError = _("Error: Transaction creation failed."); printf("SendMoney() : %s", strError.c_str()); return strError; } @@ -1100,7 +1100,7 @@ string CWallet::SendMoney(CScript scriptPubKey, int64 nValue, CWalletTx& wtxNew, return "ABORTED"; if (!CommitTransaction(wtxNew, reservekey)) - return _("Error: The transaction was rejected. This might happen if some of the coins in your wallet were already spent, such as if you used a copy of wallet.dat and coins were spent in the copy but not marked as spent here."); + return _("Error: The transaction was rejected. This might happen if some of the coins in your wallet were already spent, such as if you used a copy of wallet.dat and coins were spent in the copy but not marked as spent here."); MainFrameRepaint(); return ""; From 65cee0bbbdea49c08bc84be7824ab004cc19f57e Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Thu, 8 Nov 2012 19:38:49 +0100 Subject: [PATCH 3/3] 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/base58.h | 6 ++++-- src/crypter.cpp | 4 ++-- src/crypter.h | 4 ++-- src/serialize.h | 6 ++++-- src/util.cpp | 2 +- 5 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/base58.h b/src/base58.h index 9fe80781bd2..a5770c9af51 100644 --- a/src/base58.h +++ b/src/base58.h @@ -17,6 +17,8 @@ #include #include +#include // for OPENSSL_cleanse() + #include "bignum.h" static const char* pszBase58 = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz"; @@ -175,7 +177,7 @@ protected: ~CBase58Data() { 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) @@ -206,7 +208,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 8b0f8eb3370..c395b8e2349 100644 --- a/src/crypter.cpp +++ b/src/crypter.cpp @@ -33,8 +33,8 @@ bool CCrypter::SetKeyFromPassphrase(const std::string& strKeyData, const std::ve 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 5b95ea415e0..b23fb6edec2 100644 --- a/src/crypter.h +++ b/src/crypter.h @@ -72,8 +72,8 @@ public: void CleanKey() { - memset(&chKey, 0, sizeof chKey); - memset(&chIV, 0, sizeof chIV); + OPENSSL_cleanse(chKey, sizeof(chKey)); + OPENSSL_cleanse(chIV, sizeof(chIV)); munlock(&chKey, sizeof chKey); munlock(&chIV, sizeof chIV); fKeySet = false; diff --git a/src/serialize.h b/src/serialize.h index 18aa2a56a33..426c11a18f4 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -14,6 +14,8 @@ #include #include +#include // for OPENSSL_cleanse() + #include #include #include @@ -823,7 +825,7 @@ struct secure_allocator : public std::allocator { if (p != NULL) { - memset(p, 0, sizeof(T) * n); + OPENSSL_cleanse(p, sizeof(T) * n); munlock(p, sizeof(T) * n); } std::allocator::deallocate(p, n); @@ -857,7 +859,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/util.cpp b/src/util.cpp index 62fa4c05755..681eebaf082 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -131,7 +131,7 @@ void RandAddSeedPerfmon() if (ret == ERROR_SUCCESS) { RAND_add(pdata, nSize, nSize/100.0); - memset(pdata, 0, nSize); + OPENSSL_cleanse(pdata, nSize); printf("%s RandAddSeed() %d bytes\n", DateTimeStrFormat("%x %H:%M", GetTime()).c_str(), nSize); } #endif