Merge bitcoin/bitcoin#27068: wallet: SecureString to allow null characters

4bbf5ddd44 Detailed error message for passphrases with null chars (John Moffett)
b4bdabc223 doc: Release notes for 27068 (John Moffett)
4b1205ba37 Test case for passphrases with null characters (John Moffett)
00a0861181 Pass all characters to SecureString including nulls (John Moffett)

Pull request description:

  `SecureString` is a `std::string` specialization with a secure allocator. However, in practice it's treated like a C- string (no explicit length and null-terminated). This can cause unexpected and potentially insecure behavior. For instance, if a user enters a passphrase with embedded null characters (which is possible through Qt and the JSON-RPC), it will ignore any characters after the first null, potentially giving the user a false sense of security.

  Instead of assigning to `SecureString` via `std::string::c_str()`, assign it via a `std::string_view` of the original. This explicitly captures the size and still doesn't make any extraneous copies in memory.

  Note to reviewers, the following all compile identically in recent `GCC` (x86-64 and ARM64) with `-O2` (and `-std=c++17`):

  ```C++
  std::string orig_string;
  std::cin >> orig_string;
  SecureString s;
  s.reserve(100);
  // The following all compile identically
  s = orig_string;
  s = std::string_view{orig_string};
  s.assign(std::string_view{orig_string});
  s.assign(orig_string.data(), orig_string.size());
  ```

  So it's largely a matter of preference. However, one thing to keep in mind is that we want to avoid making unnecessary copies of any sensitive data in memory.

  Something like `SecureString s{orig_string};` is still invalid and probably unwanted in our case, since it'd get treated as a short string and optimized away from the secure allocator. I presume that's the reason for the `reserve()` calls.

  Fixes #27067.

ACKs for top commit:
  achow101:
    re-ACK 4bbf5ddd44
  stickies-v:
    re-ACK [4bbf5dd](4bbf5ddd44)
  furszy:
    utACK 4bbf5ddd

Tree-SHA512: 47a96905a82ca674b18076a20a388123beedf70e9de73e42574ea68afbb434734e56021835dd9b148cdbf61709926b487cc95e9021d9bc534a7c93b3e143d2f7

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)

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

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

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");

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/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__':