From 94f778bdebe1438fbe0206621ce44b4e25196e59 Mon Sep 17 00:00:00 2001 From: Dylan Noblesmith Date: Sat, 26 Nov 2011 06:02:04 +0000 Subject: [PATCH] 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 2857b7575ea..317f6e66dcf 100644 --- a/src/bitcoinrpc.cpp +++ b/src/bitcoinrpc.cpp @@ -1453,21 +1453,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( @@ -1493,15 +1488,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( @@ -1509,17 +1504,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; } @@ -1564,10 +1549,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( @@ -1575,13 +1561,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 a574ef925b0..24f622d6378 100644 --- a/src/qt/askpassphrasedialog.cpp +++ b/src/qt/askpassphrasedialog.cpp @@ -71,16 +71,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 43b96f6d0e2..89e8cdd2a0f 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 4c966486f7a..1ef0e6f15cd 100644 --- a/src/util.h +++ b/src/util.h @@ -292,6 +292,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);