From 027d149352bc25f0f37363da95a0b4c723100f63 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Wed, 14 Dec 2011 01:03:55 -0500 Subject: [PATCH 1/3] Bugfix: fForRelay should be false when deciding required fee to include in blocks During the rushed transition from 0.01 BTC to 0.0005 BTC fees, we took the approach of dropping the relay and block-inclusion fee to 0.0005 BTC immediately, and only delayed adjusting the sending fee for the next release. Afterward, the relay fee was lowered to 0.0001 BTC to avoid having the same problem in the future. However, the block inclusion code was left setting fForRelay to true! This fixes that, so the lower 0.0001 BTC allowance is (as intended) only permitted for real relaying. --- src/main.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index af00069d663..03e133b63cd 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2776,7 +2776,7 @@ CBlock* CreateNewBlock(CReserveKey& reservekey) // Transaction fee required depends on block size bool fAllowFree = (nBlockSize + nTxSize < 4000 || CTransaction::AllowFree(dPriority)); - int64 nMinFee = tx.GetMinFee(nBlockSize, fAllowFree, true); + int64 nMinFee = tx.GetMinFee(nBlockSize, fAllowFree); // Connecting shouldn't fail due to dependency on other memory pool transactions // because we're already processing them in order of dependency From 96f1723bb1f4155357b4e33988a2b99ee674c549 Mon Sep 17 00:00:00 2001 From: Dylan Noblesmith Date: Sat, 26 Nov 2011 06:02:04 +0000 Subject: [PATCH 2/3] Implement an mlock()'d string class for storing passphrases SecureString is identical to std::string except with secure_allocator substituting for std::allocator. This makes casting between them impossible, so converting between the two at API boundaries requires calling ::c_str() for now. --- src/bitcoinrpc.cpp | 48 ++++++++++------------------------ src/crypter.cpp | 2 +- src/crypter.h | 2 +- src/qt/askpassphrasedialog.cpp | 11 ++++---- src/qt/walletmodel.cpp | 6 ++--- src/qt/walletmodel.h | 9 ++++--- src/util.h | 4 +++ src/wallet.cpp | 6 ++--- src/wallet.h | 6 ++--- 9 files changed, 40 insertions(+), 54 deletions(-) diff --git a/src/bitcoinrpc.cpp b/src/bitcoinrpc.cpp index 31ef725d793..889dd7a1b18 100644 --- a/src/bitcoinrpc.cpp +++ b/src/bitcoinrpc.cpp @@ -1451,21 +1451,16 @@ Value walletpassphrase(const Array& params, bool fHelp) throw JSONRPCError(-17, "Error: Wallet is already unlocked."); // Note that the walletpassphrase is stored in params[0] which is not mlock()ed - string strWalletPass; + SecureString strWalletPass; strWalletPass.reserve(100); - mlock(&strWalletPass[0], strWalletPass.capacity()); - strWalletPass = params[0].get_str(); + // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string) + // Alternately, find a way to make params[0] mlock()'d to begin with. + strWalletPass = params[0].get_str().c_str(); if (strWalletPass.length() > 0) { if (!pwalletMain->Unlock(strWalletPass)) - { - fill(strWalletPass.begin(), strWalletPass.end(), '\0'); - munlock(&strWalletPass[0], strWalletPass.capacity()); throw JSONRPCError(-14, "Error: The wallet passphrase entered was incorrect."); - } - fill(strWalletPass.begin(), strWalletPass.end(), '\0'); - munlock(&strWalletPass[0], strWalletPass.capacity()); } else throw runtime_error( @@ -1491,15 +1486,15 @@ Value walletpassphrasechange(const Array& params, bool fHelp) if (!pwalletMain->IsCrypted()) throw JSONRPCError(-15, "Error: running with an unencrypted wallet, but walletpassphrasechange was called."); - string strOldWalletPass; + // TODO: get rid of these .c_str() calls by implementing SecureString::operator=(std::string) + // Alternately, find a way to make params[0] mlock()'d to begin with. + SecureString strOldWalletPass; strOldWalletPass.reserve(100); - mlock(&strOldWalletPass[0], strOldWalletPass.capacity()); - strOldWalletPass = params[0].get_str(); + strOldWalletPass = params[0].get_str().c_str(); - string strNewWalletPass; + SecureString strNewWalletPass; strNewWalletPass.reserve(100); - mlock(&strNewWalletPass[0], strNewWalletPass.capacity()); - strNewWalletPass = params[1].get_str(); + strNewWalletPass = params[1].get_str().c_str(); if (strOldWalletPass.length() < 1 || strNewWalletPass.length() < 1) throw runtime_error( @@ -1507,17 +1502,7 @@ Value walletpassphrasechange(const Array& params, bool fHelp) "Changes the wallet passphrase from to ."); if (!pwalletMain->ChangeWalletPassphrase(strOldWalletPass, strNewWalletPass)) - { - fill(strOldWalletPass.begin(), strOldWalletPass.end(), '\0'); - fill(strNewWalletPass.begin(), strNewWalletPass.end(), '\0'); - munlock(&strOldWalletPass[0], strOldWalletPass.capacity()); - munlock(&strNewWalletPass[0], strNewWalletPass.capacity()); throw JSONRPCError(-14, "Error: The wallet passphrase entered was incorrect."); - } - fill(strNewWalletPass.begin(), strNewWalletPass.end(), '\0'); - fill(strOldWalletPass.begin(), strOldWalletPass.end(), '\0'); - munlock(&strOldWalletPass[0], strOldWalletPass.capacity()); - munlock(&strNewWalletPass[0], strNewWalletPass.capacity()); return Value::null; } @@ -1562,10 +1547,11 @@ Value encryptwallet(const Array& params, bool fHelp) throw runtime_error("Not Yet Implemented: use GUI to encrypt wallet, not RPC command"); #endif - string strWalletPass; + // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string) + // Alternately, find a way to make params[0] mlock()'d to begin with. + SecureString strWalletPass; strWalletPass.reserve(100); - mlock(&strWalletPass[0], strWalletPass.capacity()); - strWalletPass = params[0].get_str(); + strWalletPass = params[0].get_str().c_str(); if (strWalletPass.length() < 1) throw runtime_error( @@ -1573,13 +1559,7 @@ Value encryptwallet(const Array& params, bool fHelp) "Encrypts the wallet with ."); if (!pwalletMain->EncryptWallet(strWalletPass)) - { - fill(strWalletPass.begin(), strWalletPass.end(), '\0'); - munlock(&strWalletPass[0], strWalletPass.capacity()); throw JSONRPCError(-16, "Error: Failed to encrypt the wallet."); - } - fill(strWalletPass.begin(), strWalletPass.end(), '\0'); - munlock(&strWalletPass[0], strWalletPass.capacity()); // BDB seems to have a bad habit of writing old data into // slack space in .dat files; that is bad if the old data is diff --git a/src/crypter.cpp b/src/crypter.cpp index bee7a3624b8..7f53e22f1e3 100644 --- a/src/crypter.cpp +++ b/src/crypter.cpp @@ -15,7 +15,7 @@ #include "main.h" #include "util.h" -bool CCrypter::SetKeyFromPassphrase(const std::string& strKeyData, const std::vector& chSalt, const unsigned int nRounds, const unsigned int nDerivationMethod) +bool CCrypter::SetKeyFromPassphrase(const SecureString& strKeyData, const std::vector& chSalt, const unsigned int nRounds, const unsigned int nDerivationMethod) { if (nRounds < 1 || chSalt.size() != WALLET_CRYPTO_SALT_SIZE) return false; diff --git a/src/crypter.h b/src/crypter.h index e8ca30a8cc1..d7f8a39d839 100644 --- a/src/crypter.h +++ b/src/crypter.h @@ -65,7 +65,7 @@ private: bool fKeySet; public: - bool SetKeyFromPassphrase(const std::string &strKeyData, const std::vector& chSalt, const unsigned int nRounds, const unsigned int nDerivationMethod); + bool SetKeyFromPassphrase(const SecureString &strKeyData, const std::vector& chSalt, const unsigned int nRounds, const unsigned int nDerivationMethod); bool Encrypt(const CKeyingMaterial& vchPlaintext, std::vector &vchCiphertext); bool Decrypt(const std::vector& vchCiphertext, CKeyingMaterial& vchPlaintext); bool SetKey(const CKeyingMaterial& chNewKey, const std::vector& chNewIV); diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp index b52acf4545e..4ee67e7c87b 100644 --- a/src/qt/askpassphrasedialog.cpp +++ b/src/qt/askpassphrasedialog.cpp @@ -70,16 +70,17 @@ void AskPassphraseDialog::setModel(WalletModel *model) void AskPassphraseDialog::accept() { - std::string oldpass, newpass1, newpass2; + SecureString oldpass, newpass1, newpass2; if(!model) return; - // TODO: mlock memory / munlock on return so they will not be swapped out, really need "mlockedstring" wrapper class to do this safely oldpass.reserve(MAX_PASSPHRASE_SIZE); newpass1.reserve(MAX_PASSPHRASE_SIZE); newpass2.reserve(MAX_PASSPHRASE_SIZE); - oldpass.assign(ui->passEdit1->text().toStdString()); - newpass1.assign(ui->passEdit2->text().toStdString()); - newpass2.assign(ui->passEdit3->text().toStdString()); + // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string) + // Alternately, find a way to make this input mlock()'d to begin with. + oldpass.assign(ui->passEdit1->text().toStdString().c_str()); + newpass1.assign(ui->passEdit2->text().toStdString().c_str()); + newpass2.assign(ui->passEdit3->text().toStdString().c_str()); switch(mode) { diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 2f989661f09..f028f10f6c6 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -200,7 +200,7 @@ WalletModel::EncryptionStatus WalletModel::getEncryptionStatus() const } } -bool WalletModel::setWalletEncrypted(bool encrypted, const std::string &passphrase) +bool WalletModel::setWalletEncrypted(bool encrypted, const SecureString &passphrase) { if(encrypted) { @@ -214,7 +214,7 @@ bool WalletModel::setWalletEncrypted(bool encrypted, const std::string &passphra } } -bool WalletModel::setWalletLocked(bool locked, const std::string &passPhrase) +bool WalletModel::setWalletLocked(bool locked, const SecureString &passPhrase) { if(locked) { @@ -228,7 +228,7 @@ bool WalletModel::setWalletLocked(bool locked, const std::string &passPhrase) } } -bool WalletModel::changePassphrase(const std::string &oldPass, const std::string &newPass) +bool WalletModel::changePassphrase(const SecureString &oldPass, const SecureString &newPass) { bool retval; CRITICAL_BLOCK(wallet->cs_wallet) diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index b7b6973b3bd..055ba184b0c 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -2,7 +2,8 @@ #define WALLETMODEL_H #include -#include + +#include "util.h" class OptionsModel; class AddressTableModel; @@ -72,10 +73,10 @@ public: SendCoinsReturn sendCoins(const QList &recipients); // Wallet encryption - bool setWalletEncrypted(bool encrypted, const std::string &passphrase); + bool setWalletEncrypted(bool encrypted, const SecureString &passphrase); // Passphrase only needed when unlocking - bool setWalletLocked(bool locked, const std::string &passPhrase=std::string()); - bool changePassphrase(const std::string &oldPass, const std::string &newPass); + bool setWalletLocked(bool locked, const SecureString &passPhrase=SecureString()); + bool changePassphrase(const SecureString &oldPass, const SecureString &newPass); // RAI object for unlocking wallet, returned by requestUnlock() class UnlockContext diff --git a/src/util.h b/src/util.h index 178923727a8..bcb9027148d 100644 --- a/src/util.h +++ b/src/util.h @@ -286,6 +286,10 @@ public: +// This is exactly like std::string, but with a custom allocator. +// (secure_allocator<> is defined in serialize.h) +typedef std::basic_string, secure_allocator > SecureString; + diff --git a/src/wallet.cpp b/src/wallet.cpp index af80cc16d55..28babdb3e2c 100644 --- a/src/wallet.cpp +++ b/src/wallet.cpp @@ -42,7 +42,7 @@ bool CWallet::AddCryptedKey(const vector &vchPubKey, const vector return false; } -bool CWallet::Unlock(const string& strWalletPassphrase) +bool CWallet::Unlock(const SecureString& strWalletPassphrase) { if (!IsLocked()) return false; @@ -63,7 +63,7 @@ bool CWallet::Unlock(const string& strWalletPassphrase) return false; } -bool CWallet::ChangeWalletPassphrase(const string& strOldWalletPassphrase, const string& strNewWalletPassphrase) +bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, const SecureString& strNewWalletPassphrase) { bool fWasLocked = IsLocked(); @@ -122,7 +122,7 @@ public: ) }; -bool CWallet::EncryptWallet(const string& strWalletPassphrase) +bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) { if (IsCrypted()) return false; diff --git a/src/wallet.h b/src/wallet.h index 19de803390a..ca7cf673179 100644 --- a/src/wallet.h +++ b/src/wallet.h @@ -70,9 +70,9 @@ public: // Adds an encrypted key to the store, without saving it to disk (used by LoadWallet) bool LoadCryptedKey(const std::vector &vchPubKey, const std::vector &vchCryptedSecret) { return CCryptoKeyStore::AddCryptedKey(vchPubKey, vchCryptedSecret); } - bool Unlock(const std::string& strWalletPassphrase); - bool ChangeWalletPassphrase(const std::string& strOldWalletPassphrase, const std::string& strNewWalletPassphrase); - bool EncryptWallet(const std::string& strWalletPassphrase); + bool Unlock(const SecureString& strWalletPassphrase); + bool ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, const SecureString& strNewWalletPassphrase); + bool EncryptWallet(const SecureString& strWalletPassphrase); bool AddToWallet(const CWalletTx& wtxIn); bool AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pblock, bool fUpdate = false); From 863238316144b0ae8d0208ed60d86d932475cfaa Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 23 Dec 2011 02:24:46 -0800 Subject: [PATCH 3/3] Fix #722. --- contrib/gitian-descriptors/gitian.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/gitian-descriptors/gitian.yml b/contrib/gitian-descriptors/gitian.yml index 47164b0afd4..11ee4dc5804 100644 --- a/contrib/gitian-descriptors/gitian.yml +++ b/contrib/gitian-descriptors/gitian.yml @@ -39,7 +39,7 @@ script: | cp $OUTDIR/src/COPYING $OUTDIR cd src sed 's/$(DEBUGFLAGS)//' -i makefile.unix - make -f makefile.unix STATIC=1 DEFS="-I$INSTDIR/include -L$INSTDIR/lib" $MAKEOPTS bitcoind USE_UPNP=0 USE_SSL=1 + make -f makefile.unix STATIC=1 OPENSSL_INCLUDE_PATH="$INSTDIR/include" OPENSSL_LIB_PATH="$INSTDIR/lib" $MAKEOPTS bitcoind USE_UPNP=0 USE_SSL=1 mkdir -p $OUTDIR/bin/$GBUILD_BITS install -s bitcoind $OUTDIR/bin/$GBUILD_BITS cd ..