Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: mitigate crashes associated with some upgradetohd edge cases #6116

Merged
merged 3 commits into from
Jul 19, 2024
Merged

fix: mitigate crashes associated with some upgradetohd edge cases #6116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
163d318
wallet: unify HD chain generation in LegacyScriptPubKeyMan
kwvg Jul 17, 2024
619b640
wallet: unify HD chain generation in CWallet
kwvg Jul 15, 2024
69c37f4
rpc: make sure `upgradetohd` always has the passphrase for `UpgradeToHD`
kwvg Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions src/wallet/rpcwallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2779,11 +2779,13 @@ static RPCHelpMan upgradetohd()
{
return RPCHelpMan{"upgradetohd",
"\nUpgrades non-HD wallets to HD.\n"
"\nIf your wallet is encrypted, the wallet passphrase must be supplied. Supplying an incorrect"
"\npassphrase may result in your wallet getting locked.\n"
"\nWarning: You will need to make a new backup of your wallet after setting the HD wallet mnemonic.\n",
{
{"mnemonic", RPCArg::Type::STR, /* default */ "", "Mnemonic as defined in BIP39 to use for the new HD wallet. Use an empty string \"\" to generate a new random mnemonic."},
{"mnemonicpassphrase", RPCArg::Type::STR, /* default */ "", "Optional mnemonic passphrase as defined in BIP39"},
{"walletpassphrase", RPCArg::Type::STR, /* default */ "", "If your wallet is encrypted you must have your wallet passphrase here. If your wallet is not encrypted specifying wallet passphrase will trigger wallet encryption."},
{"walletpassphrase", RPCArg::Type::STR, /* default */ "", "If your wallet is encrypted you must have your wallet passphrase here. If your wallet is not encrypted, specifying wallet passphrase will trigger wallet encryption."},
{"rescan", RPCArg::Type::BOOL, /* default */ "false if mnemonic is empty", "Whether to rescan the blockchain for missing transactions or not"},
},
RPCResult{
Expand All @@ -2793,6 +2795,7 @@ static RPCHelpMan upgradetohd()
HelpExampleCli("upgradetohd", "")
+ HelpExampleCli("upgradetohd", "\"mnemonicword1 ... mnemonicwordN\"")
+ HelpExampleCli("upgradetohd", "\"mnemonicword1 ... mnemonicwordN\" \"mnemonicpassphrase\"")
+ HelpExampleCli("upgradetohd", "\"mnemonicword1 ... mnemonicwordN\" \"\" \"walletpassphrase\"")
+ HelpExampleCli("upgradetohd", "\"mnemonicword1 ... mnemonicwordN\" \"mnemonicpassphrase\" \"walletpassphrase\"")
},
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
Expand All @@ -2803,17 +2806,17 @@ static RPCHelpMan upgradetohd()
bool generate_mnemonic = request.params[0].isNull() || request.params[0].get_str().empty();
SecureString secureWalletPassphrase;
secureWalletPassphrase.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.
if (!request.params[2].isNull()) {
secureWalletPassphrase = request.params[2].get_str().c_str();
if (!pwallet->Unlock(secureWalletPassphrase)) {
throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "The wallet passphrase entered was incorrect");

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();
}

EnsureWalletIsUnlocked(pwallet.get());

SecureString secureMnemonic;
secureMnemonic.reserve(256);
if (!generate_mnemonic) {
Expand All @@ -2825,6 +2828,7 @@ static RPCHelpMan upgradetohd()
if (!request.params[1].isNull()) {
secureMnemonicPassphrase = request.params[1].get_str().c_str();
}

// TODO: breaking changes kept for v21!
// instead upgradetohd let's use more straightforward 'sethdseed'
constexpr bool is_v21 = false;
Expand Down
58 changes: 22 additions & 36 deletions src/wallet/scriptpubkeyman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,55 +377,41 @@ void LegacyScriptPubKeyMan::UpgradeKeyMetadata()
}
}

void LegacyScriptPubKeyMan::GenerateNewCryptedHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, CKeyingMaterial vMasterKey)
void LegacyScriptPubKeyMan::GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, std::optional<CKeyingMaterial> vMasterKeyOpt)
{
assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));


CHDChain hdChainTmp;
CHDChain newHdChain;

// NOTE: an empty mnemonic means "generate a new one for me"
// NOTE: default mnemonic passphrase is an empty string
if (!hdChainTmp.SetMnemonic(secureMnemonic, secureMnemonicPassphrase, true)) {
if (!newHdChain.SetMnemonic(secureMnemonic, secureMnemonicPassphrase, /* fUpdateID = */ true)) {
throw std::runtime_error(std::string(__func__) + ": SetMnemonic failed");
}

// add default account
hdChainTmp.AddAccount();
// Add default account
newHdChain.AddAccount();

// We need to safe chain for validation further
CHDChain hdChainPrev = hdChainTmp;
bool res = EncryptHDChain(vMasterKey, hdChainTmp);
assert(res);
res = LoadHDChain(hdChainTmp);
assert(res);
// Encryption routine if vMasterKey has been supplied
if (vMasterKeyOpt.has_value()) {
auto vMasterKey = vMasterKeyOpt.value();
if (vMasterKey.size() != WALLET_CRYPTO_KEY_SIZE) {
throw std::runtime_error(strprintf("%s : invalid vMasterKey size, got %zd (expected %lld)", __func__, vMasterKey.size(), WALLET_CRYPTO_KEY_SIZE));
}

CHDChain hdChainCrypted;
res = GetHDChain(hdChainCrypted);
assert(res);
// Maintain an unencrypted copy of the chain for sanity checking
CHDChain prevHdChain{newHdChain};

// ids should match, seed hashes should not
assert(hdChainPrev.GetID() == hdChainCrypted.GetID());
assert(hdChainPrev.GetSeedHash() != hdChainCrypted.GetSeedHash());
bool res = EncryptHDChain(vMasterKey, newHdChain);
assert(res);
res = LoadHDChain(newHdChain);
assert(res);
res = GetHDChain(newHdChain);
assert(res);

if (!AddHDChainSingle(hdChainCrypted)) {
throw std::runtime_error(std::string(__func__) + ": AddHDChainSingle failed");
// IDs should match, seed hashes should not
assert(prevHdChain.GetID() == newHdChain.GetID());
assert(prevHdChain.GetSeedHash() != newHdChain.GetSeedHash());
}
}

void LegacyScriptPubKeyMan::GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase)
{
assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
CHDChain newHdChain;

// NOTE: an empty mnemonic means "generate a new one for me"
// NOTE: default mnemonic passphrase is an empty string
if (!newHdChain.SetMnemonic(secureMnemonic, secureMnemonicPassphrase, true)) {
throw std::runtime_error(std::string(__func__) + ": SetMnemonic failed");
}

// add default account
newHdChain.AddAccount();

if (!AddHDChainSingle(newHdChain)) {
throw std::runtime_error(std::string(__func__) + ": AddHDChainSingle failed");
Expand Down
3 changes: 1 addition & 2 deletions src/wallet/scriptpubkeyman.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,8 +463,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
bool GetDecryptedHDChain(CHDChain& hdChainRet);

/* Generates a new HD chain */
void GenerateNewCryptedHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, CKeyingMaterial vMasterKey);
void GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase);
void GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, std::optional<CKeyingMaterial> vMasterKey = std::nullopt);

/**
* Explicitly make the wallet learn the related scripts for outputs to the
Expand Down
82 changes: 46 additions & 36 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, interfaces::Coin
// TODO: drop this condition after removing option to create non-HD wallets
// related backport bitcoin#11250
if (wallet->GetVersion() >= FEATURE_HD) {
if (!wallet->GenerateNewHDChainEncrypted(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/"", passphrase)) {
if (!wallet->GenerateNewHDChain(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/"", passphrase)) {
error = Untranslated("Error: Failed to generate encrypted HD wallet");
status = DatabaseStatus::FAILED_CREATE;
return nullptr;
Expand Down Expand Up @@ -5022,18 +5022,9 @@ bool CWallet::UpgradeToHD(const SecureString& secureMnemonic, const SecureString
WalletLogPrintf("Upgrading wallet to HD\n");
SetMinVersion(FEATURE_HD);

// TODO: replace to GetLegacyScriptPubKeyMan() when `sethdseed` is backported
auto spk_man = GetOrCreateLegacyScriptPubKeyMan();
bool prev_encrypted = IsCrypted();
// TODO: unify encrypted and plain chains usages here
if (prev_encrypted) {
if (!GenerateNewHDChainEncrypted(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase)) {
error = Untranslated("Failed to generate encrypted HD wallet");
return false;
}
Lock();
} else {
spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase);
if (!GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase)) {
error = Untranslated("Failed to generate HD wallet");
return false;
}
return true;
}
Expand Down Expand Up @@ -5651,42 +5642,61 @@ void CWallet::ConnectScriptPubKeyManNotifiers()
}
}

bool CWallet::GenerateNewHDChainEncrypted(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase)
bool CWallet::GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase)
{
auto spk_man = GetLegacyScriptPubKeyMan();
if (!spk_man) {
throw std::runtime_error(strprintf("%s: spk_man is not available", __func__));
}

if (!HasEncryptionKeys()) {
return false;
}
if (IsCrypted()) {
if (secureWalletPassphrase.empty()) {
throw std::runtime_error(strprintf("%s: encrypted but supplied empty wallet passphrase", __func__));
}

CCrypter crypter;
CKeyingMaterial vMasterKey;
bool is_locked = IsLocked();

LOCK(cs_wallet);
for (const CWallet::MasterKeyMap::value_type& pMasterKey : mapMasterKeys) {
if (!crypter.SetKeyFromPassphrase(secureWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod)) {
return false;
}
// get vMasterKey to encrypt new hdChain
if (crypter.Decrypt(pMasterKey.second.vchCryptedKey, vMasterKey)) {
break;
CCrypter crypter;
CKeyingMaterial vMasterKey;

// We are intentionally re-locking the wallet so we can validate vMasterKey
// by verifying if it can unlock the wallet
Lock();

LOCK(cs_wallet);
for (const auto& [_, master_key] : mapMasterKeys) {
CKeyingMaterial _vMasterKey;
if (!crypter.SetKeyFromPassphrase(secureWalletPassphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod)) {
return false;
}
// Try another key if it cannot be decrypted or the key is incapable of encrypting
if (!crypter.Decrypt(master_key.vchCryptedKey, _vMasterKey) || _vMasterKey.size() != WALLET_CRYPTO_KEY_SIZE) {
continue;
}
// The likelihood of the plaintext being gibberish but also of the expected size is low but not zero.
// If it can unlock the wallet, it's a good key.
if (Unlock(_vMasterKey)) {
vMasterKey = _vMasterKey;
break;
}
}

}
// We got a gibberish key...
if (vMasterKey.empty()) {
// Mimicking the error message of RPC_WALLET_PASSPHRASE_INCORRECT as it's possible
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@kwvg kwvg Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

@kwvg kwvg Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// that the user may see this error when interacting with the upgradetohd RPC
throw std::runtime_error("Error: The wallet passphrase entered was incorrect");
}

spk_man->GenerateNewCryptedHDChain(secureMnemonic, secureMnemonicPassphrase, vMasterKey);
spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, vMasterKey);

Lock();
if (!Unlock(secureWalletPassphrase)) {
// this should never happen
throw std::runtime_error(std::string(__func__) + ": Unlock failed");
}
if (!spk_man->NewKeyPool()) {
throw std::runtime_error(std::string(__func__) + ": NewKeyPool failed");
if (is_locked) {
Lock();
}
} else {
spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase);
}

return true;
}

Expand Down
2 changes: 1 addition & 1 deletion src/wallet/wallet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1343,7 +1343,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati

// TODO: move it to scriptpubkeyman
/* Generates a new HD chain */
bool GenerateNewHDChainEncrypted(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase);
bool GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase = "");

/* Returns true if the wallet can give out new addresses. This means it has keys in the keypool or can generate new keys */
bool CanGetAddresses(bool internal = false) const;
Expand Down
4 changes: 2 additions & 2 deletions test/functional/wallet_upgradetohd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,8 @@ def run_test(self):
node.stop()
node.wait_until_stopped()
self.start_node(0, extra_args=['-rescan'])
assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", node.upgradetohd, mnemonic)
assert_raises_rpc_error(-14, "The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass")
assert_raises_rpc_error(-13, "Error: Wallet encrypted but passphrase not supplied to RPC.", node.upgradetohd, mnemonic)
assert_raises_rpc_error(-1, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass")
assert node.upgradetohd(mnemonic, "", walletpass)
assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", node.dumphdinfo)
node.walletpassphrase(walletpass, 100)
Expand Down
Loading