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.

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(chain): sanitize derivation index before apply changeset #1792

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
49 changes: 46 additions & 3 deletions crates/chain/src/indexer/keychain_txout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,17 @@ use crate::{
DescriptorExt, DescriptorId, Indexed, Indexer, KeychainIndexed, SpkIterator,
};
use alloc::{borrow::ToOwned, vec::Vec};
use bitcoin::{Amount, OutPoint, ScriptBuf, SignedAmount, Transaction, TxOut, Txid};
use bitcoin::{
bip32::ChildNumber, Amount, OutPoint, ScriptBuf, SignedAmount, Transaction, TxOut, Txid,
};
use core::{
fmt::Debug,
ops::{Bound, RangeBounds},
};
use miniscript::{
descriptor::{DescriptorXKey, Wildcard},
ForEachKey,
};

use crate::Merge;

Expand Down Expand Up @@ -355,6 +361,23 @@ impl<K: Clone + Ord + Debug> KeychainTxOutIndex<K> {
keychain: K,
descriptor: Descriptor<DescriptorPublicKey>,
) -> Result<bool, InsertDescriptorError<K>> {
// Ensure the keys don't contain any hardened derivation steps or hardened wildcards
let descriptor_contains_hardened_steps = descriptor.for_any_key(|k| {
if let DescriptorPublicKey::XPub(DescriptorXKey {
derivation_path,
wildcard,
..
}) = k
{
return *wildcard == Wildcard::Hardened
|| derivation_path.into_iter().any(ChildNumber::is_hardened);
}
false
});
if descriptor_contains_hardened_steps {
return Err(InsertDescriptorError::HardenedDerivationXpub);
}
descriptor.sanity_check()?;
let did = descriptor.descriptor_id();
if !self.keychain_to_descriptor_id.contains_key(&keychain)
&& !self.descriptor_id_to_keychain.contains_key(&did)
Expand Down Expand Up @@ -770,13 +793,18 @@ impl<K: Clone + Ord + Debug> KeychainTxOutIndex<K> {
pub fn apply_changeset(&mut self, changeset: ChangeSet) {
for (&desc_id, &index) in &changeset.last_revealed {
let v = self.last_revealed.entry(desc_id).or_default();
*v = index.max(*v);
let sanitized_index = if index > BIP32_MAX_INDEX {
BIP32_MAX_INDEX
} else {
index
};
*v = sanitized_index.max(*v);
self.replenish_inner_index_did(desc_id, self.lookahead);
}
}
}

#[derive(Clone, Debug, PartialEq)]
#[derive(Debug, PartialEq)]
/// Error returned from [`KeychainTxOutIndex::insert_descriptor`]
pub enum InsertDescriptorError<K> {
/// The descriptor has already been assigned to a keychain so you can't assign it to another
Expand All @@ -793,6 +821,16 @@ pub enum InsertDescriptorError<K> {
/// The descriptor that the keychain is already assigned to
existing_assignment: Descriptor<DescriptorPublicKey>,
},
/// Miniscript error
Miniscript(miniscript::Error),
/// The descriptor contains hardened derivation steps on public extended keys
HardenedDerivationXpub,
}

impl<K> From<miniscript::Error> for InsertDescriptorError<K> {
fn from(err: miniscript::Error) -> Self {
InsertDescriptorError::Miniscript(err)
}
}

impl<K: core::fmt::Debug> core::fmt::Display for InsertDescriptorError<K> {
Expand All @@ -816,6 +854,11 @@ impl<K: core::fmt::Debug> core::fmt::Display for InsertDescriptorError<K> {
"attempt to re-assign keychain {keychain:?} already assigned to {existing:?}"
)
}
InsertDescriptorError::Miniscript(err) => write!(f, "Miniscript error: {}", err),
InsertDescriptorError::HardenedDerivationXpub => write!(
f,
"The descriptor contains hardened derivation steps on public extended keys"
),
}
}
}
Expand Down
13 changes: 13 additions & 0 deletions crates/chain/tests/test_keychain_txout_index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,19 @@ fn lookahead_to_target() {
}
}

#[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();

let mut indexer = KeychainTxOutIndex::<&str>::new(10);
assert!(indexer.insert_descriptor("keychain", desc).is_err())
}

#[test]
fn applying_changesets_one_by_one_vs_aggregate_must_have_same_result() {
let desc = parse_descriptor(DESCRIPTORS[0]);
Expand Down
6 changes: 6 additions & 0 deletions crates/wallet/src/wallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2547,6 +2547,12 @@ fn create_indexer(
InsertDescriptorError::KeychainAlreadyAssigned { .. } => {
unreachable!("this is the first time we're assigning internal")
}
InsertDescriptorError::Miniscript(err) => {
crate::descriptor::error::Error::Miniscript(err)
}
InsertDescriptorError::HardenedDerivationXpub => {
crate::descriptor::error::Error::HardenedDerivationXpub
}
}
})?);
}
Expand Down
Loading