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.
This commit is contained in:
John Moffett
2023-02-09 10:53:54 -05:00
parent 80f4979322
commit 00a0861181
4 changed files with 10 additions and 16 deletions

View File

@@ -89,11 +89,10 @@ void AskPassphraseDialog::accept()
oldpass.reserve(MAX_PASSPHRASE_SIZE); oldpass.reserve(MAX_PASSPHRASE_SIZE);
newpass1.reserve(MAX_PASSPHRASE_SIZE); newpass1.reserve(MAX_PASSPHRASE_SIZE);
newpass2.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(std::string_view{ui->passEdit1->text().toStdString()});
oldpass.assign(ui->passEdit1->text().toStdString().c_str()); newpass1.assign(std::string_view{ui->passEdit2->text().toStdString()});
newpass1.assign(ui->passEdit2->text().toStdString().c_str()); newpass2.assign(std::string_view{ui->passEdit3->text().toStdString()});
newpass2.assign(ui->passEdit3->text().toStdString().c_str());
secureClearPassFields(); secureClearPassFields();

View File

@@ -56,6 +56,7 @@ struct secure_allocator : public std::allocator<T> {
}; };
// This is exactly like std::string, but with a custom allocator. // This is exactly like std::string, but with a custom allocator.
// TODO: Consider finding a way to make incoming RPC request.params[i] mlock()ed as well
typedef std::basic_string<char, std::char_traits<char>, secure_allocator<char> > SecureString; typedef std::basic_string<char, std::char_traits<char>, secure_allocator<char> > SecureString;
#endif // BITCOIN_SUPPORT_ALLOCATORS_SECURE_H #endif // BITCOIN_SUPPORT_ALLOCATORS_SECURE_H

View File

@@ -49,9 +49,7 @@ RPCHelpMan walletpassphrase()
// Note that the walletpassphrase is stored in request.params[0] which is not mlock()ed // Note that the walletpassphrase is stored in request.params[0] which is not mlock()ed
SecureString strWalletPass; SecureString strWalletPass;
strWalletPass.reserve(100); strWalletPass.reserve(100);
// TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string) strWalletPass = std::string_view{request.params[0].get_str()};
// Alternately, find a way to make request.params[0] mlock()'d to begin with.
strWalletPass = request.params[0].get_str().c_str();
// Get the timeout // Get the timeout
nSleepTime = request.params[1].getInt<int64_t>(); nSleepTime = request.params[1].getInt<int64_t>();
@@ -132,15 +130,13 @@ RPCHelpMan walletpassphrasechange()
LOCK2(pwallet->m_relock_mutex, pwallet->cs_wallet); LOCK2(pwallet->m_relock_mutex, pwallet->cs_wallet);
// TODO: get rid of these .c_str() calls by implementing SecureString::operator=(std::string)
// Alternately, find a way to make request.params[0] mlock()'d to begin with.
SecureString strOldWalletPass; SecureString strOldWalletPass;
strOldWalletPass.reserve(100); strOldWalletPass.reserve(100);
strOldWalletPass = request.params[0].get_str().c_str(); strOldWalletPass = std::string_view{request.params[0].get_str()};
SecureString strNewWalletPass; SecureString strNewWalletPass;
strNewWalletPass.reserve(100); strNewWalletPass.reserve(100);
strNewWalletPass = request.params[1].get_str().c_str(); strNewWalletPass = std::string_view{request.params[1].get_str()};
if (strOldWalletPass.empty() || strNewWalletPass.empty()) { if (strOldWalletPass.empty() || strNewWalletPass.empty()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "passphrase cannot be empty"); throw JSONRPCError(RPC_INVALID_PARAMETER, "passphrase cannot be empty");
@@ -241,11 +237,9 @@ RPCHelpMan encryptwallet()
LOCK2(pwallet->m_relock_mutex, pwallet->cs_wallet); LOCK2(pwallet->m_relock_mutex, pwallet->cs_wallet);
// TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
// Alternately, find a way to make request.params[0] mlock()'d to begin with.
SecureString strWalletPass; SecureString strWalletPass;
strWalletPass.reserve(100); strWalletPass.reserve(100);
strWalletPass = request.params[0].get_str().c_str(); strWalletPass = std::string_view{request.params[0].get_str()};
if (strWalletPass.empty()) { if (strWalletPass.empty()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "passphrase cannot be empty"); throw JSONRPCError(RPC_INVALID_PARAMETER, "passphrase cannot be empty");

View File

@@ -359,7 +359,7 @@ static RPCHelpMan createwallet()
passphrase.reserve(100); passphrase.reserve(100);
std::vector<bilingual_str> warnings; std::vector<bilingual_str> warnings;
if (!request.params[3].isNull()) { if (!request.params[3].isNull()) {
passphrase = request.params[3].get_str().c_str(); passphrase = std::string_view{request.params[3].get_str()};
if (passphrase.empty()) { if (passphrase.empty()) {
// Empty string means unencrypted // Empty string means unencrypted
warnings.emplace_back(Untranslated("Empty string given as passphrase, wallet will not be encrypted.")); warnings.emplace_back(Untranslated("Empty string given as passphrase, wallet will not be encrypted."));