From 7adf92df7a59454ff186a17a1bb4695f085fbd4d Mon Sep 17 00:00:00 2001 From: Sami Ahmed Date: Tue, 24 Mar 2026 18:44:19 -0700 Subject: [PATCH] Add MakeSecureString helper, eliminate .c_str() in password paths - Add MakeSecureString(const std::string&) in allocators.h - Replace .c_str() shims in walletpassphrase, walletpassphrasechange, encryptwallet RPCs and askpassphrasedialog - Update TODO_DOCUMENTATION.md to mark issue as resolved Co-Authored-By: Claude Opus 4.6 --- TODO_DOCUMENTATION.md | 15 +++++++-------- src/allocators.h | 5 +++++ src/qt/askpassphrasedialog.cpp | 8 +++----- src/rpcwallet.cpp | 14 ++++---------- 4 files changed, 19 insertions(+), 23 deletions(-) diff --git a/TODO_DOCUMENTATION.md b/TODO_DOCUMENTATION.md index eaf0d57..322c402 100644 --- a/TODO_DOCUMENTATION.md +++ b/TODO_DOCUMENTATION.md @@ -51,14 +51,13 @@ nOrderPos = -1; // TODO: calculate elsewhere **Status:** Deferred - no functional issue. **Fix:** Move calculation to WalletDB when transaction is added. -### src/rpcwallet.cpp - SecureString Operator -```cpp -// Lines 1474, 1513, 1569: "TODO: get rid of this .c_str()" -``` -**Issue:** SecureString missing operator=(std::string). -**Impact:** Forced to use .c_str() which exposes password temporarily. -**Status:** Deferred - would require SecureString class modification. -**Fix:** Add `SecureString& operator=(const std::string&)` method. +### src/rpcwallet.cpp / src/qt/askpassphrasedialog.cpp - SecureString Conversion +**Issue:** Password-handling paths were converting through `.c_str()` because `SecureString` +did not have a convenient conversion helper from `std::string`. +**Impact:** Unnecessary C-string shims in sensitive code paths. +**Status:** Resolved. +**Fix:** Added `MakeSecureString(const std::string&)` in `src/allocators.h` and updated +the wallet RPC and passphrase dialog call sites to use it directly. ## Low Priority (Nice-to-Have) diff --git a/src/allocators.h b/src/allocators.h index 24f6e37..aabe42f 100644 --- a/src/allocators.h +++ b/src/allocators.h @@ -254,4 +254,9 @@ struct zero_after_free_allocator : public std::allocator // This is exactly like std::string, but with a custom allocator. typedef std::basic_string, secure_allocator > SecureString; +static inline SecureString MakeSecureString(const std::string& value) +{ + return SecureString(value.begin(), value.end()); +} + #endif diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp index c400b1b..8b5132c 100644 --- a/src/qt/askpassphrasedialog.cpp +++ b/src/qt/askpassphrasedialog.cpp @@ -99,11 +99,9 @@ void AskPassphraseDialog::accept() oldpass.reserve(MAX_PASSPHRASE_SIZE); newpass1.reserve(MAX_PASSPHRASE_SIZE); newpass2.reserve(MAX_PASSPHRASE_SIZE); - // 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()); + oldpass = MakeSecureString(ui->passEdit1->text().toStdString()); + newpass1 = MakeSecureString(ui->passEdit2->text().toStdString()); + newpass2 = MakeSecureString(ui->passEdit3->text().toStdString()); switch(mode) { diff --git a/src/rpcwallet.cpp b/src/rpcwallet.cpp index 088747a..c7b5d91 100644 --- a/src/rpcwallet.cpp +++ b/src/rpcwallet.cpp @@ -1471,9 +1471,7 @@ Value walletpassphrase(const Array& params, bool fHelp) // Note that the walletpassphrase is stored in params[0] which is not mlock()ed SecureString strWalletPass; strWalletPass.reserve(100); - // 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(); + strWalletPass = MakeSecureString(params[0].get_str()); if (strWalletPass.length() > 0) { @@ -1510,15 +1508,13 @@ Value walletpassphrasechange(const Array& params, bool fHelp) if (!pwalletMain->IsCrypted()) throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an unencrypted wallet, but walletpassphrasechange was called."); - // 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); - strOldWalletPass = params[0].get_str().c_str(); + strOldWalletPass = MakeSecureString(params[0].get_str()); SecureString strNewWalletPass; strNewWalletPass.reserve(100); - strNewWalletPass = params[1].get_str().c_str(); + strNewWalletPass = MakeSecureString(params[1].get_str()); if (strOldWalletPass.length() < 1 || strNewWalletPass.length() < 1) throw runtime_error( @@ -1566,11 +1562,9 @@ Value encryptwallet(const Array& params, bool fHelp) if (pwalletMain->IsCrypted()) throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an encrypted wallet, but encryptwallet was called."); - // 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); - strWalletPass = params[0].get_str().c_str(); + strWalletPass = MakeSecureString(params[0].get_str()); if (strWalletPass.length() < 1) throw runtime_error(