fix(chain): sanitize derivation index before apply changeset

[f3r10]

fix: bitcoindevkit/bdk_wallet#60

Description

Right now, ff a descriptor has hardened child steps when inserting a descriptor throws a panic. This PR adds a previous descriptor sanitization controlling such cases.

An additional problem pointed out by @ValuedMammal is that when the apply_changest is applied a malicious crafter index could be greater that BIP32_MAX_INDEX. This PR adds a verification to handle such cases.

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

• [x] I've signed all my commits
• [x] I followed the contribution guidelines
• [x] I ran cargo fmt and cargo clippy before committing

[f3r10]

@notmandatory I am also working on this PR but is not been assigned to me yet.

[ValuedMammal]

If this helps, it appears that insert_descriptor can cause a panic in miniscript if passed a descriptor with hardened steps.

#[test]
fn insert_descriptor_should_reject_hardened_steps() {
    use bdk_chain::keychain_txout::KeychainTxOutIndex;

    // This descriptor has hardened child steps
    let s = "wpkh([e273fe42/84h/1h/0h/0h]tpubDEBY7DLZ5kK6pMC2qdtVvKpe6CiAmVDx1SiLtLS3V4GAGyRvsuVbCYReJ9oQz1rfMapghJyUAYLqkqJ3Xadp3GSTCtdAFcKPgzAXC1hzz8a/*h)";
    let (desc, _) =
        <Descriptor<DescriptorPublicKey>>::parse_descriptor(&Secp256k1::new(), s).unwrap();

    // FIXME: This will panic by attempting to derive a script pubkey from a hardened step
    let mut indexer = KeychainTxOutIndex::<&str>::new(10);
    let _ = indexer.insert_descriptor("keychain", desc);
}

[f3r10]

it appears that insert_descriptor can cause a panic in miniscript if passed a descriptor with hardened steps.

yes thanks @ValuedMammal I did a similar test :sweat_smile: It panics because of desc.descriptor_id() :thinking:

So I am going to add the sanitize part in the insert_descriptor function trying to handle that error. WDYT?

[ValuedMammal]

I think it makes sense to handle the error in insert_descriptor, but also I thought about it more and I guess we could have a test showing that KeychainTxOutIndex::apply_changeset can handle a maliciously crafted changeset e.g. where the index is >BIP32_MAX_INDEX, although I'm fairly certain that SpkIterator always caps the range at this value.

[f3r10]

I think it makes sense to handle the error in insert_descriptor, but also I thought about it more and I guess we could have a test showing that KeychainTxOutIndex::apply_changeset can handle a maliciously crafted changeset e.g. where the index is >BIP32_MAX_INDEX, although I'm fairly certain that SpkIterator always caps the range at this value.

I tried to create such a test but it takes a lot of time to complete .. has been running for over 60 seconds :thinking:

#[test]
fn insert_descriptor_maliciously() {
    use bdk_chain::keychain_txout::KeychainTxOutIndex;

    let s = "wpkh([e273fe42/84h/1h/0h/0h]tpubDEBY7DLZ5kK6pMC2qdtVvKpe6CiAmVDx1SiLtLS3V4GAGyRvsuVbCYReJ9oQz1rfMapghJyUAYLqkqJ3Xadp3GSTCtdAFcKPgzAXC1hzz8a/*)";
    let (desc, _) =
        <Descriptor<DescriptorPublicKey>>::parse_descriptor(&Secp256k1::new(), s).unwrap();

     let changeset: ChangeSet = ChangeSet {
        last_revealed: [(desc.descriptor_id(), bdk_chain::BIP32_MAX_INDEX + 1)].into(),
    };

    let mut indexer_a = KeychainTxOutIndex::<TestKeychain>::new(10);
    indexer_a
        .insert_descriptor(TestKeychain::External, desc.clone())
        .expect("must insert keychain");
    indexer_a
        .apply_changeset(changeset.clone())
        .expect("must apply changeset");

}

[f3r10]

@ValuedMammal just to be sure, the sanitize check has to be only made in insert_descriptor and not in KeychainTxOutIndex::apply_changeset, right?

[notmandatory]

I moved the state of this PR back to "In Progress" and took it out of the 1.1 release since https://github.com/bitcoindevkit/bdk/pull/1792#pullrequestreview-2541613102 still needs to be addressed.

[notmandatory]

Since we've moved bdk_wallet into it's own repo (#1928) you'll need to remove the bdk_wallet change from this PR and put it in it's own PR in the new bdk_wallet repo. I recommend doing that after this the bdk_chain parts of this PR are approved. Thanks!