Merge pull request #666 from nobled/secstrings

Implement an mlock()'d string class for storing passphrases

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 <oldpassphrase> to <newpassphrase>.");
 
     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 <passphrase>.");
 
     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

src/crypter.cpp

@@ -15,7 +15,7 @@
 #include "main.h"
 #include "util.h"
 
-bool CCrypter::SetKeyFromPassphrase(const std::string& strKeyData, const std::vector<unsigned char>& chSalt, const unsigned int nRounds, const unsigned int nDerivationMethod)
+bool CCrypter::SetKeyFromPassphrase(const SecureString& strKeyData, const std::vector<unsigned char>& chSalt, const unsigned int nRounds, const unsigned int nDerivationMethod)
 {
     if (nRounds < 1 || chSalt.size() != WALLET_CRYPTO_SALT_SIZE)
         return false;

src/crypter.h

@@ -65,7 +65,7 @@ private:
     bool fKeySet;
 
 public:
-    bool SetKeyFromPassphrase(const std::string &strKeyData, const std::vector<unsigned char>& chSalt, const unsigned int nRounds, const unsigned int nDerivationMethod);
+    bool SetKeyFromPassphrase(const SecureString &strKeyData, const std::vector<unsigned char>& chSalt, const unsigned int nRounds, const unsigned int nDerivationMethod);
     bool Encrypt(const CKeyingMaterial& vchPlaintext, std::vector<unsigned char> &vchCiphertext);
     bool Decrypt(const std::vector<unsigned char>& vchCiphertext, CKeyingMaterial& vchPlaintext);
     bool SetKey(const CKeyingMaterial& chNewKey, const std::vector<unsigned char>& chNewIV);

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)
     {

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)

src/qt/walletmodel.h

@@ -2,7 +2,8 @@
 #define WALLETMODEL_H
 
 #include <QObject>
-#include <string>
+
+#include "util.h"
 
 class OptionsModel;
 class AddressTableModel;
@@ -72,10 +73,10 @@ public:
     SendCoinsReturn sendCoins(const QList<SendCoinsRecipient> &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

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<char, std::char_traits<char>, secure_allocator<char> > SecureString;
+
 
 
 

src/wallet.cpp

@@ -42,7 +42,7 @@ bool CWallet::AddCryptedKey(const vector<unsigned char> &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;

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<unsigned char> &vchPubKey, const std::vector<unsigned char> &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);