fix(chain): sanitize derivation index before apply changeset

[f3r10]

fix: #1688

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:

• I've signed all my commits
• I followed the contribution guidelines
• 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 😅 It panics because of desc.descriptor_id() 🤔

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 🤔

#[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?

[evanlinjin]

Approach NACK

The thing with changesets is that they are designed to be loaded from persistence and ideally be fully monotone in nature. Erroring when applying the changeset is against this design principle.

I think the correct solution would be to modify the ChangeSet::last_revealed values in KeychainTxOutIndex::apply_changeset to be maxed at 2^31-1.

Whether the descriptor has any hardened steps in the derivation path should be checked in insert_descriptor (which already returns an error).

[ValuedMammal]

🔧 This comment #1792 (review) is saying, and I agree, that in principle apply_changeset should not have to return a Result.