Skip to content

Commit 5d89892

Browse files
committed
btc/policies: check that keypath points to our key
When deriving the descriptor at the keypath. This is a basic sanity check, as it does not make sense to derive it at the keypath used by a cosigner, even though it results in the same descriptor. This check also prevents signing inputs at a keypath belonging to a cosigner, which does not seem dangerous in this case, but there is no point in allowing it.
1 parent 550b33d commit 5d89892

File tree

1 file changed

+27
-1
lines changed

1 file changed

+27
-1
lines changed

src/rust/bitbox02-rust/src/hww/api/bitcoin/policies.rs

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ fn parse_wallet_policy_pk(pk: &str) -> Result<(usize, u32, u32), ()> {
108108
fn get_change_and_address_index<R: core::convert::AsRef<str>, T: core::iter::Iterator<Item = R>>(
109109
pubkeys: T,
110110
keys: &[pb::KeyOriginInfo],
111+
is_our_key: &[bool],
111112
keypath: &[u32],
112113
) -> Result<(bool, u32), Error> {
113114
for pk in pubkeys {
@@ -118,7 +119,8 @@ fn get_change_and_address_index<R: core::convert::AsRef<str>, T: core::iter::Ite
118119
Some(pb::KeyOriginInfo {
119120
keypath: keypath_account,
120121
..
121-
}) if keypath.starts_with(keypath_account)
122+
}) if is_our_key[key_index]
123+
&& keypath.starts_with(keypath_account)
122124
&& keypath.len() == keypath_account.len() + 2 =>
123125
{
124126
let keypath_change = keypath[keypath.len() - 2];
@@ -424,6 +426,7 @@ impl<'a> ParsedPolicy<'a> {
424426
let (is_change, address_index) = get_change_and_address_index(
425427
miniscript_expr.iter_pk(),
426428
&self.policy.keys,
429+
&self.is_our_key,
427430
keypath,
428431
)?;
429432
self.derive(is_change, address_index)
@@ -438,6 +441,7 @@ impl<'a> ParsedPolicy<'a> {
438441
let (is_change, _) = get_change_and_address_index(
439442
miniscript_expr.iter_pk(),
440443
&self.policy.keys,
444+
&self.is_our_key,
441445
keypath,
442446
)?;
443447
Ok(is_change)
@@ -836,6 +840,7 @@ mod tests {
836840
get_change_and_address_index(
837841
["@0/<10;11>/*", "@1/<20;21>/*"].iter(),
838842
&[our_key.clone(), some_key.clone()],
843+
&[true, false],
839844
&[
840845
48 + HARDENED,
841846
1 + HARDENED,
@@ -852,6 +857,7 @@ mod tests {
852857
get_change_and_address_index(
853858
["@0/<10;11>/*", "@1/<20;21>/*"].iter(),
854859
&[our_key.clone(), some_key.clone()],
860+
&[true, false],
855861
&[
856862
48 + HARDENED,
857863
1 + HARDENED,
@@ -868,6 +874,7 @@ mod tests {
868874
assert!(get_change_and_address_index(
869875
["@0/<10;11>/*", "@1/<20;21>/*"].iter(),
870876
&[our_key.clone(), some_key.clone()],
877+
&[true, false],
871878
&[
872879
48 + HARDENED,
873880
1 + HARDENED,
@@ -883,6 +890,7 @@ mod tests {
883890
assert!(get_change_and_address_index(
884891
["@0/<10;11>/*", "@1/<20;21>/*"].iter(),
885892
&[our_key.clone(), some_key.clone()],
893+
&[true, false],
886894
&[
887895
48 + HARDENED,
888896
1 + HARDENED,
@@ -898,6 +906,7 @@ mod tests {
898906
assert!(get_change_and_address_index(
899907
["@0/<10;11>/*", "@1/<20;21>/*"].iter(),
900908
&[our_key.clone(), some_key.clone()],
909+
&[true, false],
901910
&[
902911
48 + HARDENED,
903912
1 + HARDENED,
@@ -914,9 +923,26 @@ mod tests {
914923
assert!(get_change_and_address_index(
915924
["@0/<10;11>/*", "@1/<20;21>/*"].iter(),
916925
&[our_key.clone(), some_key.clone()],
926+
&[true, false],
917927
&[48 + HARDENED, 1 + HARDENED, 0 + HARDENED, 3 + HARDENED, 10,],
918928
)
919929
.is_err());
930+
931+
// Keypath is valid but uses a key in the policy that is not ours.
932+
assert!(get_change_and_address_index(
933+
["@0/<10;11>/*", "@1/<20;21>/*"].iter(),
934+
&[
935+
our_key.clone(),
936+
pb::KeyOriginInfo {
937+
root_fingerprint: b"aaaa".to_vec(),
938+
keypath: vec![99 + HARDENED],
939+
xpub: Some(parse_xpub(SOME_XPUB_1).unwrap()),
940+
}
941+
],
942+
&[true, false],
943+
&[99 + HARDENED, 20, 0],
944+
)
945+
.is_err());
920946
}
921947

922948
#[test]

0 commit comments

Comments
 (0)