From 00a0861181cc7f4771ac2690ca6be5731c30b005 Mon Sep 17 00:00:00 2001 From: John Moffett Date: Thu, 9 Feb 2023 10:53:54 -0500 Subject: [PATCH 1/4] 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. --- src/qt/askpassphrasedialog.cpp | 9 ++++----- src/support/allocators/secure.h | 1 + src/wallet/rpc/encrypt.cpp | 14 ++++---------- src/wallet/rpc/wallet.cpp | 2 +- 4 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp index d15aba5cdd3..d437e92a937 100644 --- a/src/qt/askpassphrasedialog.cpp +++ b/src/qt/askpassphrasedialog.cpp @@ -89,11 +89,10 @@ 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.assign(std::string_view{ui->passEdit1->text().toStdString()}); + newpass1.assign(std::string_view{ui->passEdit2->text().toStdString()}); + newpass2.assign(std::string_view{ui->passEdit3->text().toStdString()}); secureClearPassFields(); diff --git a/src/support/allocators/secure.h b/src/support/allocators/secure.h index c6bd6851898..a0918bf4639 100644 --- a/src/support/allocators/secure.h +++ b/src/support/allocators/secure.h @@ -56,6 +56,7 @@ struct secure_allocator : public std::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, secure_allocator > SecureString; #endif // BITCOIN_SUPPORT_ALLOCATORS_SECURE_H diff --git a/src/wallet/rpc/encrypt.cpp b/src/wallet/rpc/encrypt.cpp index 38960bda7b1..b9271e8dd19 100644 --- a/src/wallet/rpc/encrypt.cpp +++ b/src/wallet/rpc/encrypt.cpp @@ -49,9 +49,7 @@ RPCHelpMan walletpassphrase() // Note that the walletpassphrase is stored in request.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 request.params[0] mlock()'d to begin with. - strWalletPass = request.params[0].get_str().c_str(); + strWalletPass = std::string_view{request.params[0].get_str()}; // Get the timeout nSleepTime = request.params[1].getInt(); @@ -132,15 +130,13 @@ RPCHelpMan walletpassphrasechange() 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; strOldWalletPass.reserve(100); - strOldWalletPass = request.params[0].get_str().c_str(); + strOldWalletPass = std::string_view{request.params[0].get_str()}; SecureString strNewWalletPass; 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()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "passphrase cannot be empty"); @@ -241,11 +237,9 @@ RPCHelpMan encryptwallet() 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; strWalletPass.reserve(100); - strWalletPass = request.params[0].get_str().c_str(); + strWalletPass = std::string_view{request.params[0].get_str()}; if (strWalletPass.empty()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "passphrase cannot be empty"); diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 23a88cd51b1..a6cdad66428 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -359,7 +359,7 @@ static RPCHelpMan createwallet() passphrase.reserve(100); std::vector warnings; 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()) { // Empty string means unencrypted warnings.emplace_back(Untranslated("Empty string given as passphrase, wallet will not be encrypted.")); From 4b1205ba37d6737722d2087696b1a054a852286a Mon Sep 17 00:00:00 2001 From: John Moffett Date: Thu, 9 Feb 2023 12:11:02 -0500 Subject: [PATCH 2/4] Test case for passphrases with null characters Add a functional test to make sure the system properly accepts passphrases with null characters. --- test/functional/wallet_encryption.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/functional/wallet_encryption.py b/test/functional/wallet_encryption.py index 885c52cf2ec..88b9ebbddd4 100755 --- a/test/functional/wallet_encryption.py +++ b/test/functional/wallet_encryption.py @@ -90,6 +90,17 @@ class WalletEncryptionTest(BitcoinTestFramework): self.nodes[0].walletpassphrase(passphrase2, MAX_VALUE + 1000) actual_time = self.nodes[0].getwalletinfo()['unlocked_until'] assert_equal(actual_time, expected_time) + self.nodes[0].walletlock() + + # Test passphrase with null characters + passphrase_with_nulls = "Phrase\0With\0Nulls" + self.nodes[0].walletpassphrasechange(passphrase2, passphrase_with_nulls) + # walletpassphrasechange should not stop at null characters + assert_raises_rpc_error(-14, "wallet passphrase entered was incorrect", self.nodes[0].walletpassphrase, passphrase_with_nulls.partition("\0")[0], 10) + self.nodes[0].walletpassphrase(passphrase_with_nulls, 10) + sig = self.nodes[0].signmessage(address, msg) + assert self.nodes[0].verifymessage(address, sig, msg) + self.nodes[0].walletlock() if __name__ == '__main__': From b4bdabc2238750a1f6e72cb1403f8b770fc4f365 Mon Sep 17 00:00:00 2001 From: John Moffett Date: Thu, 9 Feb 2023 15:25:19 -0500 Subject: [PATCH 3/4] doc: Release notes for 27068 To reflect the change in behavior. --- doc/release-notes-27068.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 doc/release-notes-27068.md diff --git a/doc/release-notes-27068.md b/doc/release-notes-27068.md new file mode 100644 index 00000000000..3f5c5dba37b --- /dev/null +++ b/doc/release-notes-27068.md @@ -0,0 +1,6 @@ +Wallet +------ + +- Wallet passphrases may now contain null characters. + Prior to this change, only characters up to the first + null character were recognized and accepted. (#27068) \ No newline at end of file From 4bbf5ddd44bde15b328be131922123eaa3212a7e Mon Sep 17 00:00:00 2001 From: John Moffett Date: Tue, 14 Feb 2023 11:13:22 -0500 Subject: [PATCH 4/4] Detailed error message for passphrases with null chars Since users may have thought the null characters in their passphrases were actually evaluated prior to this change, they may be surprised to learn that their passphrases no longer work. Give them feedback to explain how to remedy the issue. --- src/qt/askpassphrasedialog.cpp | 29 +++++++++++++++++++++++++---- src/wallet/rpc/encrypt.cpp | 23 +++++++++++++++++++++-- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp index d437e92a937..0a96be038b2 100644 --- a/src/qt/askpassphrasedialog.cpp +++ b/src/qt/askpassphrasedialog.cpp @@ -153,8 +153,19 @@ void AskPassphraseDialog::accept() case Unlock: try { if (!model->setWalletLocked(false, oldpass)) { - QMessageBox::critical(this, tr("Wallet unlock failed"), - tr("The passphrase entered for the wallet decryption was incorrect.")); + // Check if the passphrase has a null character (see #27067 for details) + if (oldpass.find('\0') == std::string::npos) { + QMessageBox::critical(this, tr("Wallet unlock failed"), + tr("The passphrase entered for the wallet decryption was incorrect.")); + } else { + QMessageBox::critical(this, tr("Wallet unlock failed"), + tr("The passphrase entered for the wallet decryption is incorrect. " + "It contains a null character (ie - a zero byte). " + "If the passphrase was set with a version of this software prior to 25.0, " + "please try again with only the characters up to — but not including — " + "the first null character. If this is successful, please set a new " + "passphrase to avoid this issue in the future.")); + } } else { QDialog::accept(); // Success } @@ -173,8 +184,18 @@ void AskPassphraseDialog::accept() } else { - QMessageBox::critical(this, tr("Wallet encryption failed"), - tr("The passphrase entered for the wallet decryption was incorrect.")); + // Check if the old passphrase had a null character (see #27067 for details) + if (oldpass.find('\0') == std::string::npos) { + QMessageBox::critical(this, tr("Passphrase change failed"), + tr("The passphrase entered for the wallet decryption was incorrect.")); + } else { + QMessageBox::critical(this, tr("Passphrase change failed"), + tr("The old passphrase entered for the wallet decryption is incorrect. " + "It contains a null character (ie - a zero byte). " + "If the passphrase was set with a version of this software prior to 25.0, " + "please try again with only the characters up to — but not including — " + "the first null character.")); + } } } else diff --git a/src/wallet/rpc/encrypt.cpp b/src/wallet/rpc/encrypt.cpp index b9271e8dd19..0226d15698b 100644 --- a/src/wallet/rpc/encrypt.cpp +++ b/src/wallet/rpc/encrypt.cpp @@ -68,7 +68,17 @@ RPCHelpMan walletpassphrase() } if (!pwallet->Unlock(strWalletPass)) { - throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect."); + // Check if the passphrase has a null character (see #27067 for details) + if (strWalletPass.find('\0') == std::string::npos) { + throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect."); + } else { + throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered is incorrect. " + "It contains a null character (ie - a zero byte). " + "If the passphrase was set with a version of this software prior to 25.0, " + "please try again with only the characters up to — but not including — " + "the first null character. If this is successful, please set a new " + "passphrase to avoid this issue in the future."); + } } pwallet->TopUpKeyPool(); @@ -143,7 +153,16 @@ RPCHelpMan walletpassphrasechange() } if (!pwallet->ChangeWalletPassphrase(strOldWalletPass, strNewWalletPass)) { - throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect."); + // Check if the old passphrase had a null character (see #27067 for details) + if (strOldWalletPass.find('\0') == std::string::npos) { + throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect."); + } else { + throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The old wallet passphrase entered is incorrect. " + "It contains a null character (ie - a zero byte). " + "If the old passphrase was set with a version of this software prior to 25.0, " + "please try again with only the characters up to — but not including — " + "the first null character."); + } } return UniValue::VNULL;