From 207260d0c7e5f1e68a803642f282292e7c4bd8e2 Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Thu, 25 Oct 2012 11:52:34 +0200 Subject: [PATCH 1/6] ensure AskPassphraseDialog::eventFilter forwards events - instead of "return false;" use "return QDialog::eventFilter(object, event);" to harmonize this event filter with our default behaviour (partial of 83a3fb81f3da38461457e8dcdf5baf27b662a4b3) --- src/qt/askpassphrasedialog.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp index cae021c9823..7b9d4912ff4 100644 --- a/src/qt/askpassphrasedialog.cpp +++ b/src/qt/askpassphrasedialog.cpp @@ -222,7 +222,7 @@ bool AskPassphraseDialog::event(QEvent *event) return QWidget::event(event); } -bool AskPassphraseDialog::eventFilter(QObject *, QEvent *event) +bool AskPassphraseDialog::eventFilter(QObject *object, QEvent *event) { /* Detect Caps Lock. * There is no good OS-independent way to check a key state in Qt, but we @@ -245,5 +245,5 @@ bool AskPassphraseDialog::eventFilter(QObject *, QEvent *event) } } } - return false; + return QDialog::eventFilter(object, event); } From 2d2e5bdcf46d3b4b50d07edb2e010cf7e1cde1b1 Mon Sep 17 00:00:00 2001 From: Gregory Maxwell Date: Wed, 24 Oct 2012 01:41:52 -0400 Subject: [PATCH 2/6] 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 3/6] 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 deb9f100a04dbb9c25d6ba320eba4c653a2f5423 Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Tue, 18 Sep 2012 18:26:02 +0200 Subject: [PATCH 4/6] fix some double-spaces in strings (partial of 6b3783a9c9cc47afcf72aa0a86ea26122392efdb) --- src/bitcoinrpc.cpp | 2 +- src/init.cpp | 2 +- src/qt/bitcoingui.cpp | 8 +++----- src/qt/forms/sendcoinsentry.ui | 2 +- src/wallet.cpp | 8 ++++---- 5 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/bitcoinrpc.cpp b/src/bitcoinrpc.cpp index a062df02605..af75bc44a49 100644 --- a/src/bitcoinrpc.cpp +++ b/src/bitcoinrpc.cpp @@ -1589,7 +1589,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: StartShutdown(); - 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/init.cpp b/src/init.cpp index 0c20be976a6..9f50ef5ab89 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -341,7 +341,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/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index f444f3a1543..5768c5a4029 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -579,11 +579,9 @@ void BitcoinGUI::closeEvent(QCloseEvent *event) void BitcoinGUI::askFee(qint64 nFeeRequired, bool *payFee) { - QString strMessage = - tr("This transaction is over the size limit. You can still send it for a fee of %1, " - "which goes to the nodes that process your transaction and helps to support the network. " - "Do you want to pay the fee?").arg( - BitcoinUnits::formatWithUnit(BitcoinUnits::BTC, nFeeRequired)); + QString strMessage = tr("This transaction is over the size limit. You can still send it for a fee of %1, " + "which goes to the nodes that process your transaction and helps to support the network. " + "Do you want to pay the fee?").arg(BitcoinUnits::formatWithUnit(BitcoinUnits::BTC, nFeeRequired)); QMessageBox::StandardButton retval = QMessageBox::question( this, tr("Sending..."), strMessage, QMessageBox::Yes|QMessageBox::Cancel, QMessageBox::Yes); diff --git a/src/qt/forms/sendcoinsentry.ui b/src/qt/forms/sendcoinsentry.ui index 22a3f8fdc61..10bf603de82 100644 --- a/src/qt/forms/sendcoinsentry.ui +++ b/src/qt/forms/sendcoinsentry.ui @@ -90,7 +90,7 @@ - The address to send the payment to (e.g. 1NS17iag9jJgTHD1VXjvLCEnZuQ3rJDE9L) + The address to send the payment to (e.g. 1NS17iag9jJgTHD1VXjvLCEnZuQ3rJDE9L) 34 diff --git a/src/wallet.cpp b/src/wallet.cpp index b828473dfe4..9b164daf7d8 100644 --- a/src/wallet.cpp +++ b/src/wallet.cpp @@ -1101,7 +1101,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; } @@ -1109,9 +1109,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; } @@ -1120,7 +1120,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 220de9aafbdb76fa620531fc5c0b01ffa6616d7b Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Thu, 8 Nov 2012 21:45:32 +0100 Subject: [PATCH 5/6] Qt: small header changes / fixes - ensure header inclusion guard is named after the header file - add missing comments at the end of some inclusion guards - add a small Qt5 compatibility fix in macdockiconhandler.h --- src/qt/addressbookpage.h | 2 +- src/qt/bitcoinamountfield.h | 6 +++--- src/qt/bitcoingui.h | 2 +- src/qt/macdockiconhandler.h | 2 +- src/qt/transactiontablemodel.h | 3 +-- 5 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/qt/addressbookpage.h b/src/qt/addressbookpage.h index 1a97f3d6023..d885c768c91 100644 --- a/src/qt/addressbookpage.h +++ b/src/qt/addressbookpage.h @@ -56,4 +56,4 @@ private slots: void selectionChanged(); }; -#endif // ADDRESSBOOKDIALOG_H +#endif // ADDRESSBOOKPAGE_H diff --git a/src/qt/bitcoinamountfield.h b/src/qt/bitcoinamountfield.h index 66792e00a9e..4797c4c882a 100644 --- a/src/qt/bitcoinamountfield.h +++ b/src/qt/bitcoinamountfield.h @@ -1,5 +1,5 @@ -#ifndef BITCOINFIELD_H -#define BITCOINFIELD_H +#ifndef BITCOINAMOUNTFIELD_H +#define BITCOINAMOUNTFIELD_H #include @@ -57,4 +57,4 @@ private slots: }; -#endif // BITCOINFIELD_H +#endif // BITCOINAMOUNTFIELD_H diff --git a/src/qt/bitcoingui.h b/src/qt/bitcoingui.h index 65340775cd6..f56eecf01b1 100644 --- a/src/qt/bitcoingui.h +++ b/src/qt/bitcoingui.h @@ -157,4 +157,4 @@ private slots: void unlockWallet(); }; -#endif +#endif // BITCOINGUI_H diff --git a/src/qt/macdockiconhandler.h b/src/qt/macdockiconhandler.h index d02c148f910..d39c780233f 100644 --- a/src/qt/macdockiconhandler.h +++ b/src/qt/macdockiconhandler.h @@ -1,7 +1,7 @@ #ifndef MACDOCKICONHANDLER_H #define MACDOCKICONHANDLER_H -#include +#include class QMenu; class QIcon; diff --git a/src/qt/transactiontablemodel.h b/src/qt/transactiontablemodel.h index 3efeaa61bca..bcd35b80e6c 100644 --- a/src/qt/transactiontablemodel.h +++ b/src/qt/transactiontablemodel.h @@ -80,5 +80,4 @@ public slots: friend class TransactionTablePriv; }; -#endif - +#endif // TRANSACTIONTABLEMODEL_H From 65cee0bbbdea49c08bc84be7824ab004cc19f57e Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Thu, 8 Nov 2012 19:38:49 +0100 Subject: [PATCH 6/6] 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