Merge dashpay#6792: fix: follow-up bitcoin#27068 - apply changes for mnemonic too

28012eb test: test \0 for walletpassphrase too for rpc upgradetohd (Konstantin Akimov)
bfacf09 test: enforce stricter validation of mnemonic passphrase in wallet_upgradetohd (Konstantin Akimov)
95e9e66 test: fix incorrect key name (Konstantin Akimov)
6f841f5 test: add more comprehensive testing for mnemonic_passphrase including null character (Konstantin Akimov)
7f53b9f fix: helper ToSeed for mnemonics doesn't loose end of string after 0-characters (Konstantin Akimov)
3ab8b5e doc: update release notes to add PR num (Konstantin Akimov)
c85aa93 fix: follow-up bitcoin#27068 - apply changes for mnemonic too (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented

  bitcoin#27068 introduced support of null character in SecureString initialization, but this feature has not been supported for mnemonic passphrases.

  ## What was done?
   This PR apply changed introduced in bitcoin#27078 for dash specific code.

  ## How Has This Been Tested?
  Run unit & functional tests.

  ## Breaking Changes
  RPC `upgradetohd` supports null characters in mnemonic passphrase and in wallet's passphrase.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK 28012eb
  PastaPastaPasta:
    utACK 28012eb

Tree-SHA512: ced5766c8940fb22468e6d2b48ffa4bc845c26c858b8ea9a08450d1aff935f47af43eaf0f194ee3a3af919a05b47eae31d60daec9258933b0c66b60335f04756

Author: PastaPastaPasta

doc/release-notes-27068.md

@@ -2,5 +2,6 @@ Wallet
 ------
 
 - Wallet passphrases may now contain null characters.
+  Mnemonic passphrases may now contain null characters.
   Prior to this change, only characters up to the first
-  null character were recognized and accepted. (#27068)
+  null character were recognized and accepted. (#6780 #6792)

src/wallet/bip39.cpp

@@ -143,7 +143,8 @@ void CMnemonic::ToSeed(const SecureString& mnemonic, const SecureString& passphr
 {
 
     SecureString ssSalt = SecureString("mnemonic") + passphrase;
-    SecureVector vchSalt(ssSalt.begin(), ssSalt.begin() + strnlen(ssSalt.data(), 256));
+    SecureVector vchSalt(ssSalt.begin(), ssSalt.begin() + std::min<size_t>(256, ssSalt.size()));
     seedRet.resize(64);
+    // NOTE: c_str() here is fine because mnemonic has only [a-z ] characters
     PKCS5_PBKDF2_HMAC_SHA512(mnemonic.c_str(), mnemonic.size(), vchSalt.data(), vchSalt.size(), 2048, 64, seedRet.data());
 }

src/wallet/hdchain.cpp

@@ -67,7 +67,7 @@ bool CHDChain::SetMnemonic(const SecureString& ssMnemonic, const SecureString& s
 
         // printf("mnemonic: %s\n", ssMnemonicTmp.c_str());
         if (!CMnemonic::Check(ssMnemonicTmp)) {
-            throw std::runtime_error(std::string(__func__) + ": invalid mnemonic: `" + std::string(ssMnemonicTmp.c_str()) + "`");
+            throw std::runtime_error(std::string(__func__) + ": invalid mnemonic: `" + std::string(ssMnemonicTmp) + "`");
         }
 
         CMnemonic::ToSeed(ssMnemonicTmp, ssMnemonicPassphrase, vchSeed);

src/wallet/rpc/backup.cpp

@@ -908,8 +908,8 @@ RPCHelpMan dumphdinfo()
 
     UniValue obj(UniValue::VOBJ);
     obj.pushKV("hdseed", HexStr(hdChainCurrent.GetSeed()));
-    obj.pushKV("mnemonic", ssMnemonic.c_str());
-    obj.pushKV("mnemonicpassphrase", ssMnemonicPassphrase.c_str());
+    obj.pushKV("mnemonic", ssMnemonic);
+    obj.pushKV("mnemonicpassphrase", ssMnemonicPassphrase);
 
     return obj;
 },
@@ -2024,8 +2024,8 @@ RPCHelpMan listdescriptors()
             SecureString mnemonic;
             SecureString mnemonic_passphrase;
             if (desc_spk_man->GetMnemonicString(mnemonic, mnemonic_passphrase) && !mnemonic.empty()) {
-                spk.pushKV("mnemonic", mnemonic.c_str());
-                spk.pushKV("mnemonicpassphrase", mnemonic_passphrase.c_str());
+                spk.pushKV("mnemonic", mnemonic);
+                spk.pushKV("mnemonicpassphrase", mnemonic_passphrase);
             }
         }
         spk.pushKV("desc", descriptor);

src/wallet/rpc/wallet.cpp

@@ -375,29 +375,27 @@ static RPCHelpMan upgradetohd()
     {
         LOCK(pwallet->cs_wallet);
 
-        SecureString secureWalletPassphrase;
-        secureWalletPassphrase.reserve(100);
+        SecureString wallet_passphrase;
+        wallet_passphrase.reserve(100);
 
         if (request.params[2].isNull()) {
             if (pwallet->IsCrypted()) {
                 throw JSONRPCError(RPC_WALLET_UNLOCK_NEEDED, "Error: Wallet encrypted but passphrase not supplied to RPC.");
             }
         } else {
-            // 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.
-            secureWalletPassphrase = request.params[2].get_str().c_str();
+            wallet_passphrase = std::string_view{request.params[2].get_str()};
         }
 
-        SecureString secureMnemonic;
-        secureMnemonic.reserve(256);
+        SecureString mnemonic;
+        mnemonic.reserve(256);
         if (!generate_mnemonic) {
-            secureMnemonic = request.params[0].get_str().c_str();
+            mnemonic = std::string_view{request.params[0].get_str()};
         }
 
-        SecureString secureMnemonicPassphrase;
-        secureMnemonicPassphrase.reserve(256);
+        SecureString mnemonic_passphrase;
+        mnemonic_passphrase.reserve(256);
         if (!request.params[1].isNull()) {
-            secureMnemonicPassphrase = request.params[1].get_str().c_str();
+            mnemonic_passphrase = std::string_view{request.params[1].get_str()};
         }
 
         // TODO: breaking changes kept for v21!
@@ -421,7 +419,7 @@ static RPCHelpMan upgradetohd()
         pwallet->SetMinVersion(FEATURE_HD);
 
         if (pwallet->IsCrypted()) {
-            if (secureWalletPassphrase.empty()) {
+            if (wallet_passphrase.empty()) {
                 throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: Wallet encrypted but supplied empty wallet passphrase");
             }
 
@@ -430,13 +428,13 @@ static RPCHelpMan upgradetohd()
             pwallet->Lock();
 
             // Unlock the wallet
-            if (!pwallet->Unlock(secureWalletPassphrase)) {
+            if (!pwallet->Unlock(wallet_passphrase)) {
                 throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect");
             }
         }
 
         if (pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
-            pwallet->SetupDescriptorScriptPubKeyMans(secureMnemonic, secureMnemonicPassphrase);
+            pwallet->SetupDescriptorScriptPubKeyMans(mnemonic, mnemonic_passphrase);
         } else {
             auto spk_man = pwallet->GetLegacyScriptPubKeyMan();
             if (!spk_man) {
@@ -445,20 +443,20 @@ static RPCHelpMan upgradetohd()
 
             if (pwallet->IsCrypted()) {
                 pwallet->WithEncryptionKey([&](const CKeyingMaterial& encryption_key) {
-                        spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, encryption_key);
+                        spk_man->GenerateNewHDChain(mnemonic, mnemonic_passphrase, encryption_key);
                         return true;
                     });
             } else {
-                spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase);
+                spk_man->GenerateNewHDChain(mnemonic, mnemonic_passphrase);
             }
         }
 
         if (pwallet->IsCrypted()) {
             // Relock encrypted wallet
             pwallet->Lock();
-        } else if (!secureWalletPassphrase.empty()) {
+        } else if (!wallet_passphrase.empty()) {
             // Encrypt non-encrypted wallet
-            if (!pwallet->EncryptWallet(secureWalletPassphrase)) {
+            if (!pwallet->EncryptWallet(wallet_passphrase)) {
                 throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Failed to encrypt HD wallet");
             }
         }

src/wallet/wallet.cpp

@@ -2872,11 +2872,16 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
                         error = strprintf(_("%s -- Incorrect seed, it should be a hex string"), __func__);
                         return nullptr;
                     }
-                    SecureString secureMnemonic = args.GetArg("-mnemonic", "").c_str();
-                    SecureString secureMnemonicPassphrase = args.GetArg("-mnemonicpassphrase", "").c_str();
+
+                    SecureString mnemonic, mnemonic_passphrase;
+                    mnemonic.reserve(256);
+                    mnemonic_passphrase.reserve(256);
+
+                    mnemonic = args.GetArg("-mnemonic", "");
+                    mnemonic_passphrase = args.GetArg("-mnemonicpassphrase", "");
                     LOCK(walletInstance->cs_wallet);
                     if (auto spk_man = walletInstance->GetLegacyScriptPubKeyMan()) {
-                        spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase);
+                        spk_man->GenerateNewHDChain(mnemonic, mnemonic_passphrase);
                     }
                 }
 
@@ -2888,8 +2893,11 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
 
             LOCK(walletInstance->cs_wallet);
             if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
-                SecureString mnemonic = args.GetArg("-mnemonic", "").c_str();
-                SecureString mnemonic_passphrase = args.GetArg("-mnemonicpassphrase", "").c_str();
+                SecureString mnemonic, mnemonic_passphrase;
+                mnemonic.reserve(256);
+                mnemonic_passphrase.reserve(256);
+                mnemonic = args.GetArg("-mnemonic", "");
+                mnemonic_passphrase = args.GetArg("-mnemonicpassphrase", "");
                 args.ForceRemoveArg("mnemonic");
                 args.ForceRemoveArg("mnemonicpassphrase");
                 walletInstance->SetupDescriptorScriptPubKeyMans(mnemonic, mnemonic_passphrase);
@@ -3764,7 +3772,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans(const SecureString& mnemonic_arg,
     // TODO: remove duplicated code with CHDChain::SetMnemonic
     const SecureString mnemonic = mnemonic_arg.empty() ? CMnemonic::Generate(m_args.GetIntArg("-mnemonicbits", CHDChain::DEFAULT_MNEMONIC_BITS)) : mnemonic_arg;
     if (!CMnemonic::Check(mnemonic)) {
-        throw std::runtime_error(std::string(__func__) + ": invalid mnemonic: `" + std::string(mnemonic.c_str()) + "`");
+        throw std::runtime_error(std::string(__func__) + ": invalid mnemonic: `" + std::string(mnemonic) + "`");
     }
     SecureVector seed_key;
     CMnemonic::ToSeed(mnemonic, mnemonic_passphrase, seed_key);

test/functional/test_framework/util.py

@@ -543,21 +543,26 @@ def get_mnemonic(node):
     Raises exception if there is none.
     """
     if not node.getwalletinfo()['descriptors']:
-        return node.dumphdinfo()["mnemonic"]
+        hd = node.dumphdinfo()
+        return (hd["mnemonic"], hd["mnemonicpassphrase"])
 
     mnemonic = None
+    mnemonic_passphrase = None
     descriptors = node.listdescriptors(True)['descriptors']
     for desc in descriptors:
         if desc['desc'][:4] == 'pkh(':
             if mnemonic is None:
                 mnemonic = desc['mnemonic']
+                mnemonic_passphrase = desc['mnemonicpassphrase']
             else:
                 assert_equal(mnemonic, desc['mnemonic'])
+                assert_equal(mnemonic_passphrase, desc['mnemonicpassphrase'])
         elif desc['desc'][:6] == 'combo(':
             assert 'mnemonic' not in desc
+            assert 'mnemonicpassphrase' not in desc
         else:
             raise AssertionError(f"Unknown descriptor type: {desc['desc']}")
-    return mnemonic
+    return (mnemonic, mnemonic_passphrase)
 
 # Transaction/Block functions
 #############################

test/functional/wallet_mnemonicbits.py

@@ -26,7 +26,7 @@ def run_test(self):
         self.nodes[0].assert_start_raises_init_error(['-mnemonicbits=123'], "Error: Invalid '-mnemonicbits'. Allowed values: 128, 160, 192, 224, 256.")
         self.start_node(0)
 
-        mnemonic_pre = get_mnemonic(self.nodes[0])
+        mnemonic_pre = get_mnemonic(self.nodes[0])[0]
 
 
         self.nodes[0].encryptwallet('pass')
@@ -74,11 +74,11 @@ def run_test(self):
         self.nodes[0].createwallet("wallet_256", blank=True, descriptors=self.options.descriptors)  # blank wallet
         self.nodes[0].get_wallet_rpc("wallet_256").upgradetohd()
 
-        assert_equal(len(get_mnemonic(self.nodes[0].get_wallet_rpc(self.default_wallet_name)).split()), 12)  # 12 words by default
-        assert_equal(len(get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_160")).split()), 15)              # 15 words
-        assert_equal(len(get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_192")).split()), 18)              # 18 words
-        assert_equal(len(get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_224")).split()), 21)              # 21 words
-        assert_equal(len(get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_256")).split()), 24)              # 24 words
+        assert_equal(len(get_mnemonic(self.nodes[0].get_wallet_rpc(self.default_wallet_name))[0].split()), 12)  # 12 words by default
+        assert_equal(len(get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_160"))[0].split()), 15)              # 15 words
+        assert_equal(len(get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_192"))[0].split()), 18)              # 18 words
+        assert_equal(len(get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_224"))[0].split()), 21)              # 21 words
+        assert_equal(len(get_mnemonic(self.nodes[0].get_wallet_rpc("wallet_256"))[0].split()), 24)              # 24 words
 
 
 if __name__ == '__main__':

test/functional/wallet_upgradetohd.py

@@ -110,8 +110,9 @@ def run_test(self):
 
         self.log.info("Same mnemonic, another mnemonic passphrase, no wallet passphrase, should result in a different set of keys")
         new_mnemonic_passphrase = "somewords"
-        assert node.upgradetohd(mnemonic, new_mnemonic_passphrase)
-        assert_equal(mnemonic, get_mnemonic(node))
+        assert node.upgradetohd(mnemonic[0], new_mnemonic_passphrase)
+        assert_equal(mnemonic[0], get_mnemonic(node)[0])
+        assert_equal(new_mnemonic_passphrase, get_mnemonic(node)[1])
         if not self.options.descriptors:
             new_chainid = node.getwalletinfo()['hdchainid']
             assert chainid != new_chainid
@@ -125,8 +126,9 @@ def run_test(self):
         self.recover_non_hd()
 
         self.log.info("Same mnemonic, another mnemonic passphrase, no wallet passphrase, should result in a different set of keys (again)")
-        assert node.upgradetohd(mnemonic, new_mnemonic_passphrase)
-        assert_equal(mnemonic, get_mnemonic(node))
+        assert node.upgradetohd(mnemonic[0], new_mnemonic_passphrase)
+        assert_equal(mnemonic[0], get_mnemonic(node)[0])
+        assert_equal(new_mnemonic_passphrase, get_mnemonic(node)[1])
         if not self.options.descriptors:
             assert_equal(new_chainid, node.getwalletinfo()['hdchainid'])
         assert_equal(balance_non_HD, node.getbalance())
@@ -139,7 +141,7 @@ def run_test(self):
         self.recover_non_hd()
 
         self.log.info("Same mnemonic, no mnemonic passphrase, no wallet passphrase, should recover all coins after rescan")
-        assert node.upgradetohd(mnemonic)
+        assert node.upgradetohd(mnemonic[0], mnemonic[1])
         assert_equal(mnemonic, get_mnemonic(node))
         if not self.options.descriptors:
             assert_equal(chainid, node.getwalletinfo()['hdchainid'])
@@ -152,7 +154,7 @@ def run_test(self):
 
         self.log.info("Same mnemonic, no mnemonic passphrase, no wallet passphrase, large enough keepool, should recover all coins with no extra rescan")
         self.restart_node(0, extra_args=['-keypool=10'])
-        assert node.upgradetohd(mnemonic)
+        assert node.upgradetohd(mnemonic[0], mnemonic[1])
         assert_equal(mnemonic, get_mnemonic(node))
         if not self.options.descriptors:
             assert_equal(chainid, node.getwalletinfo()['hdchainid'])
@@ -163,7 +165,7 @@ def run_test(self):
 
         self.log.info("Same mnemonic, no mnemonic passphrase, no wallet passphrase, large enough keepool, rescan is skipped initially, should recover all coins after rescanblockchain")
         self.restart_node(0, extra_args=['-keypool=10'])
-        assert node.upgradetohd(mnemonic, "", "", False)
+        assert node.upgradetohd(mnemonic[0], mnemonic[1], "", False)
         assert_equal(mnemonic, get_mnemonic(node))
         if not self.options.descriptors:
             assert_equal(chainid, node.getwalletinfo()['hdchainid'])
@@ -176,7 +178,7 @@ def run_test(self):
 
         self.log.info("Same mnemonic, same mnemonic passphrase, encrypt wallet on upgrade, should recover all coins after rescan")
         walletpass = "111pass222"
-        assert node.upgradetohd(mnemonic, "", walletpass)
+        assert node.upgradetohd(mnemonic[0], "", walletpass)
         node.stop()
         node.wait_until_stopped()
         self.start_node(0, extra_args=['-rescan'])
@@ -197,14 +199,15 @@ def run_test(self):
         self.recover_non_hd()
 
         self.log.info("Same mnemonic, same mnemonic passphrase, encrypt wallet first, should recover all coins on upgrade after rescan")
-        walletpass = "111pass222"
+        # Null characters are allowed in wallet passphrases since v23
+        walletpass = "111\0pass222"
         node.encryptwallet(walletpass)
         node.stop()
         node.wait_until_stopped()
         self.start_node(0, extra_args=['-rescan'])
-        assert_raises_rpc_error(-13, "Error: Wallet encrypted but passphrase not supplied to RPC.", node.upgradetohd, mnemonic)
-        assert_raises_rpc_error(-14, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass")
-        assert node.upgradetohd(mnemonic, "", walletpass)
+        assert_raises_rpc_error(-13, "Error: Wallet encrypted but passphrase not supplied to RPC.", node.upgradetohd, mnemonic[0])
+        assert_raises_rpc_error(-14, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic[0], "", "111")
+        assert node.upgradetohd(mnemonic[0], "", walletpass)
         if not self.options.descriptors:
             assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", node.dumphdinfo)
         else:
@@ -241,9 +244,19 @@ def run_test(self):
         node.createwallet("wallet-12", blank=True)
         w12 = node.get_wallet_rpc("wallet-12")
         w12.upgradetohd(custom_mnemonic, "custom-passphrase")
+        assert_equal(get_mnemonic(w12)[0], custom_mnemonic)
+        assert_equal(get_mnemonic(w12)[1], "custom-passphrase")
         assert_equal(12, w12.getbalance())
         w12.unloadwallet()
 
+        self.log.info("Check if null character at the end of mnemonic-passphrase matters")
+        node.createwallet("wallet-null", blank=True)
+        w_null = node.get_wallet_rpc("wallet-null")
+        w_null.upgradetohd(custom_mnemonic, "custom-passphrase\0")
+        assert_equal(0, w_null.getbalance())
+        assert_equal(get_mnemonic(w_null)[1], "custom-passphrase\0")
+        w_null.unloadwallet()
+
 
 if __name__ == '__main__':
     WalletUpgradeToHDTest().main ()