Repository: bitcoin/bitcoin (language: cpp)

Issue:
Wallet passphrases silently ignore everything after a null character
## Problem

Encrypting a wallet through JSON RPC or Qt *appears to* allow a user to include null characters in the passphrase, but silently ignores everything including and after the first null character.

For instance (on regtest), trying to set a passphrase of "`a{null character}b`":

`curl --user __cookie__ --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "encryptwallet", "params": ["a\u0000b"]}' -H 'content-type: text/plain;' http://127.0.0.1:18443/`

This will succeed, but allow the user to unlock with the passphrase "`a`", instead of the expected full passphrase (which also works):

`curl --user __cookie__ --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "walletpassphrase", "params": ["a",10]}' -H 'content-type: text/plain;' http://127.0.0.1:18443/`

I am also able to replicate it in Qt on my macOS machine by running `printf 'a\0b' | pbcopy` and pasting the result into the passphrase dialog. 

My main concern is a user thinking that they're generating, say, 32 random bytes as a passphrase, and if they're unlucky and get a zero in the first few bytes, it unexpectedly cuts their security down to almost nothing. 

## Root Cause

The reason is due to our `SecureString` type. `SecureString` is a `std::string` specialization with a secure allocator. However, when assigned, it's treated like a C- string (no explicit length and null-terminated). See the [original PR](https://github.com/bitcoin/bitcoin/pull/666) for more details.

## Potential Solutions

I think there are two plausible approaches to take. The first (and my preference) is to allow and support null characters, and I will submit a PR that enables that (by making `SecureString` use the entire string). The second is to explicitly reject any passphrases that contain null characters.

One significant complication may be that, if anyone is *already* using a passphrase with a null, then my first solution would stop their wallet from unlocking. However, it would still be unlockable just by trimming the null and any subsequent characters.


Discussion / hints:

Fix patch:
diff --git a/doc/release-notes-27068.md b/doc/release-notes-27068.md
new file mode 100644
index 0000000000000..3f5c5dba37bd4
--- /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 d15aba5cdd310..0a96be038b22a 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 c6bd6851898d6..a0918bf463990 100644
--- a/src/support/allocators/secure.h
+++ b/src/support/allocators/secure.h
@@ -56,6 +56,7 @@ struct secure_allocator : public std::allocator<T> {
 };
 
 // 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;
 
 #endif // BITCOIN_SUPPORT_ALLOCATORS_SECURE_H
diff --git a/src/wallet/rpc/encrypt.cpp b/src/wallet/rpc/encrypt.cpp
index 38960bda7b10d..0226d15698bab 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<int64_t>();
@@ -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 23a88cd51b1c8..a6cdad66428c2 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<bilingual_str> 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."));


Test patch:
diff --git a/test/functional/wallet_encryption.py b/test/functional/wallet_encryption.py
index 885c52cf2ec5c..88b9ebbddd4ed 100755
--- a/test/functional/wallet_encryption.py
+++ b/test/functional/wallet_encryption.py
@@ -90,6 +90,17 @@ def run_test(self):
         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__':