Skip to content

Commit 00a0861

Browse files
committed
Pass all characters to SecureString including nulls
`SecureString` is a `std::string` specialization with a secure allocator. However, it's treated like a C- string (no explicit length and null-terminated). This can cause unexpected behavior. For instance, if a user enters a passphrase with an embedded null character (which is possible through Qt and the JSON-RPC), it will ignore any characters after the null, giving the user a false sense of security. Instead of assigning `SecureString` via `std::string::c_str()`, assign it via a `std::string_view` of the original. This explicitly captures the size and doesn't make any extraneous copies in memory.
1 parent 80f4979 commit 00a0861

File tree

4 files changed

+10
-16
lines changed

4 files changed

+10
-16
lines changed

src/qt/askpassphrasedialog.cpp

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -89,11 +89,10 @@ void AskPassphraseDialog::accept()
8989
oldpass.reserve(MAX_PASSPHRASE_SIZE);
9090
newpass1.reserve(MAX_PASSPHRASE_SIZE);
9191
newpass2.reserve(MAX_PASSPHRASE_SIZE);
92-
// TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
93-
// Alternately, find a way to make this input mlock()'d to begin with.
94-
oldpass.assign(ui->passEdit1->text().toStdString().c_str());
95-
newpass1.assign(ui->passEdit2->text().toStdString().c_str());
96-
newpass2.assign(ui->passEdit3->text().toStdString().c_str());
92+
93+
oldpass.assign(std::string_view{ui->passEdit1->text().toStdString()});
94+
newpass1.assign(std::string_view{ui->passEdit2->text().toStdString()});
95+
newpass2.assign(std::string_view{ui->passEdit3->text().toStdString()});
9796

9897
secureClearPassFields();
9998

src/support/allocators/secure.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ struct secure_allocator : public std::allocator<T> {
5656
};
5757

5858
// This is exactly like std::string, but with a custom allocator.
59+
// TODO: Consider finding a way to make incoming RPC request.params[i] mlock()ed as well
5960
typedef std::basic_string<char, std::char_traits<char>, secure_allocator<char> > SecureString;
6061

6162
#endif // BITCOIN_SUPPORT_ALLOCATORS_SECURE_H

src/wallet/rpc/encrypt.cpp

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,7 @@ RPCHelpMan walletpassphrase()
4949
// Note that the walletpassphrase is stored in request.params[0] which is not mlock()ed
5050
SecureString strWalletPass;
5151
strWalletPass.reserve(100);
52-
// TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
53-
// Alternately, find a way to make request.params[0] mlock()'d to begin with.
54-
strWalletPass = request.params[0].get_str().c_str();
52+
strWalletPass = std::string_view{request.params[0].get_str()};
5553

5654
// Get the timeout
5755
nSleepTime = request.params[1].getInt<int64_t>();
@@ -132,15 +130,13 @@ RPCHelpMan walletpassphrasechange()
132130

133131
LOCK2(pwallet->m_relock_mutex, pwallet->cs_wallet);
134132

135-
// TODO: get rid of these .c_str() calls by implementing SecureString::operator=(std::string)
136-
// Alternately, find a way to make request.params[0] mlock()'d to begin with.
137133
SecureString strOldWalletPass;
138134
strOldWalletPass.reserve(100);
139-
strOldWalletPass = request.params[0].get_str().c_str();
135+
strOldWalletPass = std::string_view{request.params[0].get_str()};
140136

141137
SecureString strNewWalletPass;
142138
strNewWalletPass.reserve(100);
143-
strNewWalletPass = request.params[1].get_str().c_str();
139+
strNewWalletPass = std::string_view{request.params[1].get_str()};
144140

145141
if (strOldWalletPass.empty() || strNewWalletPass.empty()) {
146142
throw JSONRPCError(RPC_INVALID_PARAMETER, "passphrase cannot be empty");
@@ -241,11 +237,9 @@ RPCHelpMan encryptwallet()
241237

242238
LOCK2(pwallet->m_relock_mutex, pwallet->cs_wallet);
243239

244-
// TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
245-
// Alternately, find a way to make request.params[0] mlock()'d to begin with.
246240
SecureString strWalletPass;
247241
strWalletPass.reserve(100);
248-
strWalletPass = request.params[0].get_str().c_str();
242+
strWalletPass = std::string_view{request.params[0].get_str()};
249243

250244
if (strWalletPass.empty()) {
251245
throw JSONRPCError(RPC_INVALID_PARAMETER, "passphrase cannot be empty");

src/wallet/rpc/wallet.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ static RPCHelpMan createwallet()
359359
passphrase.reserve(100);
360360
std::vector<bilingual_str> warnings;
361361
if (!request.params[3].isNull()) {
362-
passphrase = request.params[3].get_str().c_str();
362+
passphrase = std::string_view{request.params[3].get_str()};
363363
if (passphrase.empty()) {
364364
// Empty string means unencrypted
365365
warnings.emplace_back(Untranslated("Empty string given as passphrase, wallet will not be encrypted."));

0 commit comments

Comments
 (0)