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 diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp index d15aba5cdd3..0a96be038b2 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(); @@ -154,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 } @@ -174,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/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..0226d15698b 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(); @@ -70,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(); @@ -132,22 +140,29 @@ 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"); } 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; @@ -241,11 +256,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 3162cce9873..16595267b40 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.")); 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__':