fix: mitigate crashes associated with some upgradetohd edge cases

[kwvg]

Motivation

When filming demo footage for #6093, I realized that if I tried to create an encrypted blank legacy wallet and run upgradetohd [mnemonic], the client would crash.

dash@b9c6631a824d:/src/dash$ ./src/qt/dash-qt
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-dash'
dash-qt: wallet/scriptpubkeyman.cpp:399: void LegacyScriptPubKeyMan::GenerateNewCryptedHDChain(const SecureString &, const SecureString &, CKeyingMaterial): Assertion `res' failed.
Posix Signal: Aborted
No debug information available for stacktrace. You should add debug information and then run:
dash-qt -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaadwiyltnawxc5avkbxxg2lyebjwsz3omfwduicbmjxxe5dfmqaaa===

The expected set of operations when performing privileged operations is to first use walletpassphrase [passphrase] [time] to unlock the wallet and then perform the privileged operation. This routine that applies for almost all privileged RPCs doesn't apply here, the unlock state of the wallet has no bearing on constructing an encrypted HD chain as it needs to be encrypted with the master key stored in the wallet, which in turn is encrypted with a key derived from the passphrase (i.e., upgradetohd imports always need the passphrase, if encrypted).

You might have noticed that I used upgradetohd [mnemonic] instead of the correct syntax, upgradetohd [mnemonic] "" [passphrase] that is supposed to be used when supplying a mnemonic to an encrypted wallet, because when you run the former, you don't get told to enter the passphrase into the RPC command, you're told.

Error: Please enter the wallet passphrase with walletpassphrase first.

Which tells you to treat it like any other routine privileged operation and follow the routine as mentioned above. This is where insufficient validation starts rearing its head, we only validate the passphrase if we're supplied one even though we should be demanding one if the wallet is encrypted and it isn't supplied. We didn't supply a passphrase because we're following the normal routine, we unlocked the wallet so EnsureWalletIsUnlocked() is happy, so now the following happens.

upgradetohd()
  | Insufficient validation has allowed us to supply a blank passphrase
  | for an encrypted wallet
  |- CWallet::UpgradeToHD()
    |- CWallet::GenerateNewHDChainEncrypted()
     | We get our hands on vMasterKey by generating the key from our passphrase
     | and using it to unlock vCryptedMasterKey. 
     | 
     | There's one small problem, we don't know if the output of CCrypter::Decrypt
     | isn't just gibberish. Since we don't have a passphrase, whatever came from
     | CCrypter::SetKeyFromPassphrase isn't the decryption key, meaning, the 
     | vMasterKey we just got is gibberish
     |- LegacyScriptPubKeyMan::GenerateNewCryptedHDChain()
       |- res = LegacyScriptPubKeyMan::EncryptHDChain()
       | |- EncryptSecret()
       |   |- CCrypter::SetKey()
       |      This is where everything unravels, the gibberish key's size doesn't
       |      match WALLET_CRYPTO_KEY_SIZE, it's no good for encryption. We bail out.
       |- assert(res)
          We assume are inputs are safe so there's no real reason we should crash.
          Except our inputs aren't safe, so we crash. Welp! :c

This problem has existed for a while but didn't cause the client to crash, in v20.1.1 (1951298), trying to do the same thing would return you a vague error

Failed to generate encrypted HD wallet (code -4)

In the process of working on mitigating this crash, another edge case was discovered, where if the wallet was unlocked and an incorrect passphrase was provided to upgradetohd, the user would not receive any feedback that they entered the wrong passphrase and the client would similarly crash.

upgradetohd()
 | We've been supplied a passphrase, so we can try and validate it by
 | trying to unlock the wallet with it. If it fails, we know we got the
 | wrong passphrase.
 |- CWallet::Unlock()
 | | Before we bother unlocking the wallet, we should check if we're
 | | already unlocked, if we are, we can just say "unlock successful".
 | |- CWallet::IsLocked()
 | |  Wallet is indeed unlocked.
 | |- return true; 
 | The validation method we just tried to use has a bail-out mechanism
 | that we don't account for, the "unlock" succeded so I guess we have the
 | right passphrase.
 [...] (continue call chain as mentioned earlier)
       |- assert(res)
          Oh...

This pull request aims to resolve crashes caused by the above two edge cases.

Additional Information

As this PR was required me to add additional guardrails on GenerateNewCryptedHDChain() and GenerateNewHDChainEncrypted(), it was taken as an opportunity to resolve a TODO (source). The following mitigations have been implemented.

• Validating vMasterKey size (any key not of WALLET_CRYPTO_KEY_SIZE size cannot be used for encryption and so, cannot be a valid key)
• Validating secureWalletPassphrase's presence to catch attempts at passing a blank value (an encrypted wallet cannot have a blank passphrase)
• Using Unlock() to validate the correctness of vMasterKey. (the two other instances of iterating through mapMasterKeys use Unlock(), see here and here)
  • Lock()'ing the wallet before Unlock()'ing the wallet to avoid the IsLocked() bail-out condition and then restoring to the previous lock state afterwards.
• Add an IsCrypted() check to see if upgradetohd's walletpassphrase is allowed to be empty.

Checklist:

• 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
• I have made corresponding changes to the documentation (note: N/A)
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[knst]

I think you should not remove the check EnsureWalletIsUnlocked

The error message GenerateNewHDChain: supplied incorrect passphrase is less friendly than The wallet passphrase entered was incorrect

From the user's perspective, he has no idea what is GenerateNewHDChain

[knst]

also all over all RPC the check EnsureWalletIsUnlocked is a general-use-practice, that is used for all other rpc which requires wallet to be unlocked, and don't really care how exactly wallet is unlocked - because it is a wrong passphrase or because it is not provided

[kwvg]

EnsureWalletIsUnlocked() doesn't tell if you have the wrong passphrase (source), it tells you to use walletpassphrase (the RPC) if your wallet isn't unlocked.

As I mentioned in my PR description, that reenforces the idea that this is just like any other privileged command, unlocking your wallet let you perform the privileged operation, when this isn't the case. Having an unlocked wallet won't give us the passphrase needed so the unlock state doesn't matter. Under the hood, we're locking-unlocking the wallet anyways.

The incorrect passphrase error comes from within upgradetohd() (source) and I had thought about doing the passphrase validation in upgradetohd to keep around the friendlier error message and retain the error code but that would require doing the Lock()-Unlock() routine, only for GenerateNewHDChain() to do it again (and I'd prefer to keep it in GenerateNewHDChain() than only in upgradetohd()).

We could make the message more friendly by changing the text in GenerateNewHDChain() itself but that would mean bucking with error message convention for exceptions thrown from non-RPC function bodies of "[function name]: [error message]", thoughts?

EDIT: Exception message has been changed to mimic the error message that comes with RPC_WALLET_PASSPHRASE_INCORRECT

[PastaPastaPasta]

Generally LGTM

[thephez]

utACK 👍

[knst]

one more comment about exception error - Otherwise PR LGTM

[knst]

you can actually write try/catch inside rpc code and if the exception - is 'passphrase is incorrect' - re-throw it as an RPCError.

I think that would be the best from the rpc's user point of view.

[kwvg]

Seems a bit convoluted, it'll help us retain the right error code but has catching-processing-rethrowing been done anywhere else?

[knst]

what's about:

+try {
      if (!GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase)) {
        error = Untranslated("Failed to generate HD wallet");
        return false;
+ } catch (const std::runtime_error& e) {
+    error = Unstransalted(sprintf("Generatin new chain is failed due to : %s", e.what());
+    return false;
+}

(that's not working code, just a concept)

[kwvg]

This makes the user-facing error look like "Generating new chain failed due to: GenerateNewHDChain: supplied incorrect passphrase" (assuming we restore the pre-RPC_WALLET_PASSPHRASE_INCORRECT error message).

If trying to ensure the incorrect passphrase error has the correct error code, it'd look a bit like this

    bool wallet_upgraded{false};
    try {
        wallet_upgraded = pwallet->UpgradeToHD(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase, error);
    } catch (const std::runtime_error& e) {
        if (e.what() == std::string("supplied incorrect passphrase")) {
            throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect");
        }
        // No more exceptions to translate, rethrow them
        throw std::runtime_error(e.what());
    } // Let other exceptions remain unhandled by us

But this feels like excessive effort when the alternative (just mirroring the error message we expect from RPC_WALLET_PASSPHRASE_INCORRECT) seems fine.

[knst]

utACK 69c37f4

[UdjinM6]

utACK 69c37f4

[PastaPastaPasta]

utACK 69c37f4