Merge pull request #635 from gavinandresen/encryptionbug

Prevent unencrypted private keys from being written to wallet.dat

src/bitcoinrpc.cpp

@@ -1557,6 +1557,11 @@ Value encryptwallet(const Array& params, bool fHelp)
     if (pwalletMain->IsCrypted())
         throw JSONRPCError(-15, "Error: running with an encrypted wallet, but encryptwallet was called.");
 
+#ifdef QT_GUI
+    // shutting down via RPC while the GUI is running does not work (yet):
+    throw runtime_error("Not Yet Implemented: use GUI to encrypt wallet, not RPC command");
+#endif
+
     string strWalletPass;
     strWalletPass.reserve(100);
     mlock(&strWalletPass[0], strWalletPass.capacity());
@@ -1576,7 +1581,11 @@ Value encryptwallet(const Array& params, bool fHelp)
     fill(strWalletPass.begin(), strWalletPass.end(), '\0');
     munlock(&strWalletPass[0], strWalletPass.capacity());
 
-    return Value::null;
+    // 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
+    // unencrypted private keys.  So:
+    CreateThread(Shutdown, NULL);
+    return "wallet encrypted; bitcoin server stopping, restart to run with encrypted wallet";
 }
 
 

src/db.cpp

@@ -28,6 +28,34 @@ DbEnv dbenv(0);
 static map<string, int> mapFileUseCount;
 static map<string, Db*> mapDb;
 
+static void EnvShutdown(bool fRemoveLogFiles)
+{
+    if (!fDbEnvInit)
+        return;
+
+    fDbEnvInit = false;
+    dbenv.close(0);
+    DbEnv(0).remove(GetDataDir().c_str(), 0);
+
+    if (fRemoveLogFiles)
+    {
+        filesystem::path datadir(GetDataDir());
+        filesystem::directory_iterator it(datadir / "database");
+        while (it != filesystem::directory_iterator())
+        {
+            const filesystem::path& p = it->path();
+#if BOOST_FILESYSTEM_VERSION == 2
+            std::string f = p.filename();
+#else
+            std::string f = p.filename().generic_string();
+#endif
+            if (f.find("log.") == 0)
+                filesystem::remove(p);
+            ++it;
+        }
+    }
+}
+
 class CDBInit
 {
 public:
@@ -36,11 +64,7 @@ public:
     }
     ~CDBInit()
     {
-        if (fDbEnvInit)
+        EnvShutdown(false);
-        {
-            dbenv.close(0);
-            fDbEnvInit = false;
-        }
     }
 }
 instance_of_cdbinit;
@@ -165,7 +189,100 @@ void static CloseDb(const string& strFile)
     }
 }
 
-void DBFlush(bool fShutdown)
+bool CDB::Rewrite(const string& strFile, const char* pszSkip)
+{
+    while (!fShutdown)
+    {
+        CRITICAL_BLOCK(cs_db)
+        {
+            if (!mapFileUseCount.count(strFile) || mapFileUseCount[strFile] == 0)
+            {
+                // Flush log data to the dat file
+                CloseDb(strFile);
+                dbenv.txn_checkpoint(0, 0, 0);
+                dbenv.lsn_reset(strFile.c_str(), 0);
+                mapFileUseCount.erase(strFile);
+
+                bool fSuccess = true;
+                printf("Rewriting %s...\n", strFile.c_str());
+                string strFileRes = strFile + ".rewrite";
+                CDB db(strFile.c_str(), "r");
+                Db* pdbCopy = new Db(&dbenv, 0);
+
+                int ret = pdbCopy->open(NULL,                 // Txn pointer
+                                        strFileRes.c_str(),   // Filename
+                                        "main",    // Logical db name
+                                        DB_BTREE,  // Database type
+                                        DB_CREATE,    // Flags
+                                        0);
+                if (ret > 0)
+                {
+                    printf("Cannot create database file %s\n", strFileRes.c_str());
+                    fSuccess = false;
+                }
+
+                Dbc* pcursor = db.GetCursor();
+                if (pcursor)
+                    while (fSuccess)
+                    {
+                        CDataStream ssKey;
+                        CDataStream ssValue;
+                        int ret = db.ReadAtCursor(pcursor, ssKey, ssValue, DB_NEXT);
+                        if (ret == DB_NOTFOUND)
+                            break;
+                        else if (ret != 0)
+                        {
+                            pcursor->close();
+                            fSuccess = false;
+                            break;
+                        }
+                        if (pszSkip &&
+                            strncmp(&ssKey[0], pszSkip, std::min(ssKey.size(), strlen(pszSkip))) == 0)
+                            continue;
+                        if (strncmp(&ssKey[0], "\x07version", 8) == 0)
+                        {
+                            // Update version:
+                            ssValue.clear();
+                            ssValue << VERSION;
+                        }
+                        Dbt datKey(&ssKey[0], ssKey.size());
+                        Dbt datValue(&ssValue[0], ssValue.size());
+                        int ret2 = pdbCopy->put(NULL, &datKey, &datValue, DB_NOOVERWRITE);
+                        if (ret2 > 0)
+                            fSuccess = false;
+                    }
+                if (fSuccess)
+                {
+                    Db* pdb = mapDb[strFile];
+                    if (pdb->close(0))
+                        fSuccess = false;
+                    if (pdbCopy->close(0))
+                        fSuccess = false;
+                    delete pdb;
+                    delete pdbCopy;
+                    mapDb[strFile] = NULL;
+                }
+                if (fSuccess)
+                {
+                    Db dbA(&dbenv, 0);
+                    if (dbA.remove(strFile.c_str(), NULL, 0))
+                        fSuccess = false;
+                    Db dbB(&dbenv, 0);
+                    if (dbB.rename(strFileRes.c_str(), NULL, strFile.c_str(), 0))
+                        fSuccess = false;
+                }
+                if (!fSuccess)
+                    printf("Rewriting of %s FAILED!\n", strFileRes.c_str());
+                return fSuccess;
+            }
+        }
+        Sleep(100);
+    }
+    return false;
+}
+
+
+void DBFlush(bool fShutdown, bool fRemoveLogFiles)
 {
     // Flush log data to the actual data file
     //  on all files that are not in use
@@ -196,9 +313,10 @@ void DBFlush(bool fShutdown)
         {
             char** listp;
             if (mapFileUseCount.empty())
+            {
                 dbenv.log_archive(&listp, DB_ARCH_REMOVE);
-            dbenv.close(0);
+                EnvShutdown(fRemoveLogFiles);
-            fDbEnvInit = false;
+            }
         }
     }
 }
@@ -656,6 +774,7 @@ int CWalletDB::LoadWallet(CWallet* pwallet)
     pwallet->vchDefaultKey.clear();
     int nFileVersion = 0;
     vector<uint256> vWalletUpgrade;
+    bool fIsEncrypted = false;
 
     // Modify defaults
 #ifndef WIN32
@@ -781,6 +900,7 @@ int CWalletDB::LoadWallet(CWallet* pwallet)
                 ssValue >> vchPrivKey;
                 if (!pwallet->LoadCryptedKey(vchPubKey, vchPrivKey))
                     return DB_CORRUPT;
+                fIsEncrypted = true;
             }
             else if (strType == "defaultkey")
             {
@@ -841,8 +961,11 @@ int CWalletDB::LoadWallet(CWallet* pwallet)
         printf("fUseUPnP = %d\n", fUseUPnP);
 
 
-    // Upgrade
+    // Rewrite encrypted wallets of versions 0.4.0 and 0.5.0rc:
-    if (nFileVersion < VERSION)
+    if (fIsEncrypted && (nFileVersion == 40000 || nFileVersion == 50000))
+        return DB_NEED_REWRITE;
+
+    if (nFileVersion < VERSION) // Update
     {
         // Get rid of old debug.log file in current directory
         if (nFileVersion <= 105 && !pszSetDataDir[0])
@@ -851,7 +974,6 @@ int CWalletDB::LoadWallet(CWallet* pwallet)
         WriteVersion(VERSION);
     }
 
-
     return DB_LOAD_OK;
 }
 

src/db.h

@@ -29,13 +29,12 @@ extern unsigned int nWalletDBUpdated;
 extern DbEnv dbenv;
 
 
-extern void DBFlush(bool fShutdown);
+extern void DBFlush(bool fShutdown, bool fRemoveLogFiles);
 void ThreadFlushWalletDB(void* parg);
 bool BackupWallet(const CWallet& wallet, const std::string& strDest);
 
 
 
-
 class CDB
 {
 protected:
@@ -257,6 +256,8 @@ public:
     {
         return Write(std::string("version"), nVersion);
     }
+
+    bool static Rewrite(const std::string& strFile, const char* pszSkip = NULL);
 };
 
 
@@ -349,6 +350,7 @@ enum DBErrors
     DB_CORRUPT,
     DB_TOO_NEW,
     DB_LOAD_FAIL,
+    DB_NEED_REWRITE
 };
 
 class CWalletDB : public CDB

src/init.cpp

@@ -44,8 +44,8 @@ void Shutdown(void* parg)
 {
     static CCriticalSection cs_Shutdown;
     static bool fTaken;
-    bool fFirstThread;
+    bool fFirstThread = false;
-    CRITICAL_BLOCK(cs_Shutdown)
+    TRY_CRITICAL_BLOCK(cs_Shutdown)
     {
         fFirstThread = !fTaken;
         fTaken = true;
@@ -55,9 +55,9 @@ void Shutdown(void* parg)
     {
         fShutdown = true;
         nTransactionsUpdated++;
-        DBFlush(false);
+        DBFlush(false, false);
         StopNode();
-        DBFlush(true);
+        DBFlush(true, true);
         boost::filesystem::remove(GetPidFile());
         UnregisterWallet(pwalletMain);
         delete pwalletMain;
@@ -362,6 +362,12 @@ bool AppInit2(int argc, char* argv[])
             strErrors += _("Error loading wallet.dat: Wallet corrupted      \n");
         else if (nLoadWalletRet == DB_TOO_NEW)
             strErrors += _("Error loading wallet.dat: Wallet requires newer version of Bitcoin      \n");
+        else if (nLoadWalletRet == DB_NEED_REWRITE)
+        {
+            strErrors += _("Wallet needed to be rewritten: restart Bitcoin to complete    \n");
+            wxMessageBox(strErrors, "Bitcoin", wxOK | wxICON_ERROR);
+            return false;
+        }
         else
             strErrors += _("Error loading wallet.dat      \n");
     }

src/qt/askpassphrasedialog.cpp

@@ -101,7 +101,8 @@ void AskPassphraseDialog::accept()
                 if(model->setWalletEncrypted(true, newpass1))
                 {
                     QMessageBox::warning(this, tr("Wallet encrypted"),
-                                         tr("Remember that encrypting your wallet cannot fully protect your bitcoins from being stolen by malware infecting your computer."));
+                                         tr("Bitcoin will close now to finish the encryption process. Remember that encrypting your wallet cannot fully protect your bitcoins from being stolen by malware infecting your computer."));
+                    QApplication::quit();
                 }
                 else
                 {

src/serialize.h

@@ -60,7 +60,7 @@ class CDataStream;
 class CAutoFile;
 static const unsigned int MAX_SIZE = 0x02000000;
 
-static const int VERSION = 50000;
+static const int VERSION = 50001;
 static const char* pszSubVer = "";
 static const bool VERSION_IS_BETA = true;
 

src/wallet.cpp

@@ -187,6 +187,11 @@ bool CWallet::EncryptWallet(const string& strWalletPassphrase)
         }
 
         Lock();
+
+        // Need to completely rewrite the wallet file; if we don't, bdb might keep
+        // bits of the unencrypted private key in slack space in the database file.
+        setKeyPool.clear();
+        CDB::Rewrite(strWalletFile, "\x04pool");
     }
 
     return true;
@@ -1142,6 +1147,18 @@ int CWallet::LoadWallet(bool& fFirstRunRet)
         return false;
     fFirstRunRet = false;
     int nLoadWalletRet = CWalletDB(strWalletFile,"cr+").LoadWallet(this);
+    if (nLoadWalletRet == DB_NEED_REWRITE)
+    {
+        if (CDB::Rewrite(strWalletFile, "\x04pool"))
+        {
+            setKeyPool.clear();
+            // Note: can't top-up keypool here, because wallet is locked.
+            // User will be prompted to unlock wallet the next operation
+            // the requires a new key.
+        }
+        nLoadWalletRet = DB_NEED_REWRITE;
+    }
+
     if (nLoadWalletRet != DB_LOAD_OK)
         return nLoadWalletRet;
     fFirstRunRet = vchDefaultKey.empty();