diff --git a/CHANGELOG.md b/CHANGELOG.md index 4039fc5843..27762e559e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ customers cannot upgrade their bootloader, its changes are recorded separately. ### [Unreleased] - Bitcoin: add support for payment requests +- Bitcoin: allow multisig accounts at arbitrary keypaths - Ethereum: allow signing EIP-712 messages containing multi-line strings ### 9.18.0 diff --git a/src/rust/bitbox02-rust/src/hww/api/bitcoin.rs b/src/rust/bitbox02-rust/src/hww/api/bitcoin.rs index 5b13bb7288..4be94c4267 100644 --- a/src/rust/bitbox02-rust/src/hww/api/bitcoin.rs +++ b/src/rust/bitbox02-rust/src/hww/api/bitcoin.rs @@ -39,7 +39,6 @@ use crate::keystore; use pb::btc_pub_request::{Output, XPubType}; use pb::btc_request::Request; -use pb::btc_script_config::multisig::ScriptType as MultisigScriptType; use pb::btc_script_config::{Config, SimpleType}; use pb::btc_script_config::{Multisig, Policy}; use pb::response::Response; @@ -128,6 +127,8 @@ async fn xpub( if display { let title = if is_unusual { "".into() + } else if keypath == &[45 + HARDENED] { + format!("{}\nat\n{}", params.name, util::bip32::to_string(keypath)) } else { format!("{}\naccount #{}", params.name, keypath[2] - HARDENED + 1) }; @@ -193,11 +194,9 @@ pub async fn address_multisig( display: bool, ) -> Result { let coin_params = params::get(coin); - let script_type = MultisigScriptType::try_from(multisig.script_type)?; - keypath::validate_address_multisig(keypath, coin_params.bip44_coin, script_type) - .or(Err(Error::InvalidInput))?; + keypath::validate_address_policy(keypath).or(Err(Error::InvalidInput))?; let account_keypath = &keypath[..keypath.len() - 2]; - multisig::validate(multisig, account_keypath, coin_params.bip44_coin)?; + multisig::validate(multisig, account_keypath)?; let name = match multisig::get_name(coin, multisig, account_keypath)? { Some(name) => name, None => return Err(Error::InvalidInput), @@ -320,6 +319,7 @@ mod tests { use bitbox02::testing::{ mock, mock_memory, mock_unlocked, mock_unlocked_using_mnemonic, Data, TEST_MNEMONIC, }; + use pb::btc_script_config::multisig::ScriptType as MultisigScriptType; use util::bip32::HARDENED; #[test] @@ -376,6 +376,15 @@ mod tests { expected_xpub: "zpub6qMaznTYmLk3vtEVNKbozbjRJedYqnHPwSwDwEAVaDkuQd7YEnqBvcbmCDpgvEqw2sqHUMtrJTwD6yNYLoqULriz6PXDYsS14LuoLr3KxUC", expected_display_title: "Bitcoin\naccount #2", }, + // BTC m/45', no warning + Test { + mnemonic: TEST_MNEMONIC, + coin: BtcCoin::Btc, + keypath: &[45 + HARDENED], + xpub_type: XPubType::Xpub, + expected_xpub: "xpub67uTYzYstMMVao9Z7sseYh5m9N51ft82f6Wo3Lp773Qxe1JxFFDyP71C3xvo3jZ3p1Cg3xZQ8eqsFBHYEVFZt9iqoTBEcCigcmxF1xgqBPm", + expected_display_title: "Bitcoin\nat\nm/45'", + }, // BTC P2TR Test { // Test vector from https://github.com/bitcoin/bips/blob/edffe529056f6dfd33d8f716fb871467c3c09263/bip-0086.mediawiki#test-vectors @@ -909,6 +918,24 @@ mod tests { script_type: MultisigScriptType::P2wsh, expected_address: "tb1qndz49j0arp8g6jc8vcrgf9ugrsw96a0j5d7vqcun6jev6rlv47jsv99y5m", }, + // An arbitrary "non-standard" keypath + Test { + coin: BtcCoin::Btc, + threshold: 1, + xpubs: &[ + "xpub6FEZ9Bv73h1vnE4TJG4QFj2RPXJhhsPbnXgFyH3ErLvpcZrDcynY65bhWga8PazWHLSLi23PoBhGcLcYW6JRiJ12zXZ9Aop4LbAqsS3gtcy", + "xpub68yJakxtRe3azab9rb8DJqxDeCG7oBY3zhsNnvZybjTE9qc9Hgw4bCqdLjVGykZrwD6CC6r6xHrnuep5Dmb9uq2R4emCm8YzBuddFyhgvAD", + ], + expected_info: "1-of-2\nBitcoin multisig", + our_xpub_index: 1, + keypath: &[ + 45 + HARDENED, + 1, + 2 + ], + script_type: MultisigScriptType::P2wsh, + expected_address: "bc1qtsvlhzltl05etjjeqh00urwttu6ep4xn3c0ccndz77unttut9h0qvrcs04", + }, /* P2WSH-P2SH */ Test { coin: BtcCoin::Btc, diff --git a/src/rust/bitbox02-rust/src/hww/api/bitcoin/keypath.rs b/src/rust/bitbox02-rust/src/hww/api/bitcoin/keypath.rs index 71a62dcb62..48360323a5 100644 --- a/src/rust/bitbox02-rust/src/hww/api/bitcoin/keypath.rs +++ b/src/rust/bitbox02-rust/src/hww/api/bitcoin/keypath.rs @@ -58,7 +58,7 @@ pub fn validate_account( /// - Electrum-style: m/48'/coin'/account'/script_type', where script_type is 1 for p2wsh-p2sh and 2 /// for p2wsh. /// - Nunchuk-style: m/48'/coin'/account', independent of the script type. -pub fn validate_account_multisig( +fn validate_account_multisig( keypath: &[u32], expected_coin: u32, script_type: MultisigScriptType, @@ -89,22 +89,6 @@ fn validate_change_address(change: u32, address: u32) -> Result<(), ()> { } } -/// Validates that the prefix (all but last two elements) of the keypath is a valid multisig account -/// keypath and the last to elements are a valid change and receive element. -pub fn validate_address_multisig( - keypath: &[u32], - expected_coin: u32, - script_type: MultisigScriptType, -) -> Result<(), ()> { - if keypath.len() >= 2 { - let (keypath_account, keypath_rest) = keypath.split_at(keypath.len() - 2); - validate_account_multisig(keypath_account, expected_coin, script_type)?; - validate_change_address(keypath_rest[0], keypath_rest[1]) - } else { - Err(()) - } -} - /// Validates that the address index is valid and that the account keypath prefix is not empty. /// /// Note that the change index is not verified as in policies, it can be different to 0 and 1 @@ -166,7 +150,8 @@ pub fn validate_address_simple( } } -/// Checks if the keypath is a valid account-level keypath for any supported script type. +/// Checks if the the xpub at this keypath can be exported without warning the user of that it is an +/// unusual keypath. pub fn validate_xpub(keypath: &[u32], expected_coin: u32, taproot_support: bool) -> Result<(), ()> { for &script_type in ALL_MULTISCRIPT_SCRIPT_TYPES.iter() { if validate_account_multisig(keypath, expected_coin, script_type).is_ok() { @@ -178,6 +163,10 @@ pub fn validate_xpub(keypath: &[u32], expected_coin: u32, taproot_support: bool) return Ok(()); } } + // m/45', used/exported by Unchained. + if keypath == &[45 + HARDENED] { + return Ok(()); + } Err(()) } @@ -265,161 +254,6 @@ mod tests { .is_err()); } - #[test] - fn test_validate_address_multisig() { - let coin = 1 + HARDENED; - // valid p2wsh - assert!(validate_address_multisig( - &[48 + HARDENED, coin, 0 + HARDENED, 2 + HARDENED, 0, 0], - coin, - MultisigScriptType::P2wsh - ) - .is_ok()); - - // valid p2wsh-p2sh - assert!(validate_address_multisig( - &[48 + HARDENED, coin, 0 + HARDENED, 1 + HARDENED, 0, 0], - coin, - MultisigScriptType::P2wshP2sh - ) - .is_ok()); - - // valid Nunchuk-style - assert!(validate_address_multisig( - &[48 + HARDENED, coin, 0 + HARDENED, 0, 0], - coin, - MultisigScriptType::P2wsh - ) - .is_ok()); - assert!(validate_address_multisig( - &[48 + HARDENED, coin, 0 + HARDENED, 0, 0], - coin, - MultisigScriptType::P2wshP2sh - ) - .is_ok()); - - // wrong script type for p2wsh - assert!(validate_address_multisig( - &[ - 48 + HARDENED, - coin, - 0 + HARDENED, - 1 + HARDENED, // should be 2' - 0, - 0, - ], - coin, - MultisigScriptType::P2wsh - ) - .is_err()); - - // wrong script type for p2wsh-p2sh - assert!(validate_address_multisig( - &[ - 48 + HARDENED, - coin, - 0 + HARDENED, - 2 + HARDENED, // should be 1' - 0, - 0, - ], - coin, - MultisigScriptType::P2wshP2sh - ) - .is_err()); - - // too short - assert!(validate_address_multisig( - &[48 + HARDENED, coin, 0 + HARDENED, 2 + HARDENED, 0], - coin, - MultisigScriptType::P2wsh - ) - .is_err()); - - // too long - assert!(validate_address_multisig( - &[48 + HARDENED, coin, 0 + HARDENED, 2 + HARDENED, 0, 0, 0], - coin, - MultisigScriptType::P2wsh - ) - .is_err()); - - // wrong purpose - assert!(validate_address_multisig( - &[ - 49 + HARDENED, // <- wrong - coin, - 0 + HARDENED, - 2 + HARDENED, - 0, - 0, - ], - coin, - MultisigScriptType::P2wsh - ) - .is_err()); - - // unhardened account - assert!(validate_address_multisig( - &[ - 48 + HARDENED, - coin, - 0, // <- wrong - 2 + HARDENED, - 0, - 0, - ], - coin, - MultisigScriptType::P2wsh - ) - .is_err()); - - // account too high - assert!(validate_address_multisig( - &[ - 48 + HARDENED, - coin, - 100 + HARDENED, // <- wrong - 2 + HARDENED, - 0, - 0, - ], - coin, - MultisigScriptType::P2wsh - ) - .is_err()); - - // wrong change - assert!(validate_address_multisig( - &[ - 48 + HARDENED, - coin, - 0 + HARDENED, - 2 + HARDENED, - 2, // <- wrong - 0, - ], - coin, - MultisigScriptType::P2wsh - ) - .is_err()); - - // address too high - assert!(validate_address_multisig( - &[ - 48 + HARDENED, - coin, - 0 + HARDENED, - 2 + HARDENED, - 0, - 10000, // <- wrong - ], - coin, - MultisigScriptType::P2wsh - ) - .is_err()); - } - #[test] fn test_validate_address_simple() { let bip44_account = 99 + HARDENED; diff --git a/src/rust/bitbox02-rust/src/hww/api/bitcoin/multisig.rs b/src/rust/bitbox02-rust/src/hww/api/bitcoin/multisig.rs index ff1b99f76c..1ed7a36b7d 100644 --- a/src/rust/bitbox02-rust/src/hww/api/bitcoin/multisig.rs +++ b/src/rust/bitbox02-rust/src/hww/api/bitcoin/multisig.rs @@ -251,7 +251,7 @@ pub async fn confirm_extended( /// - no two xpubs are the same. /// keypath: account-level keypath, e.g. m/48'/0'/10'/2' /// expected:_coin expected bip44 coin in the keypath. -pub fn validate(multisig: &Multisig, keypath: &[u32], expected_coin: u32) -> Result<(), Error> { +pub fn validate(multisig: &Multisig, keypath: &[u32]) -> Result<(), Error> { if multisig.xpubs.len() < 2 || multisig.xpubs.len() > MAX_SIGNERS { return Err(Error::InvalidInput); } @@ -261,12 +261,6 @@ pub fn validate(multisig: &Multisig, keypath: &[u32], expected_coin: u32) -> Res if multisig.our_xpub_index >= multisig.xpubs.len() as _ { return Err(Error::InvalidInput); } - super::keypath::validate_account_multisig( - keypath, - expected_coin, - ScriptType::try_from(multisig.script_type)?, - ) - .or(Err(Error::InvalidInput))?; let our_xpub = crate::keystore::get_xpub(keypath)?.serialize(None)?; let maybe_our_xpub = @@ -573,8 +567,7 @@ mod tests { #[test] fn test_validate() { - let expected_coin = 1 + HARDENED; - let keypath = &[48 + HARDENED, expected_coin, 0 + HARDENED, 2 + HARDENED]; + let keypath = &[48 + HARDENED, 1 + HARDENED, 0 + HARDENED, 2 + HARDENED]; let our_xpub_str = "xpub6EMfjyGVUvwhpc3WKN1zXhMFGKJGMaSBPqbja4tbGoYvRBSXeTBCaqrRDjcuGTcaY95JrrAnQvDG3pdQPdtnYUCugjeksHSbyZT7rq38VQF"; let multisig = Multisig { threshold: 1, @@ -589,14 +582,25 @@ mod tests { // Keystore locked. bitbox02::keystore::lock(); - assert!(validate(&multisig, keypath, expected_coin).is_err()); + assert!(validate(&multisig, keypath).is_err()); - // ok. + // Ok. mock_unlocked_using_mnemonic( "sudden tenant fault inject concert weather maid people chunk youth stumble grit", "", ); - assert!(validate(&multisig, keypath, expected_coin).is_ok()); + assert!(validate(&multisig, keypath).is_ok()); + // Ok at arbitrary keypath. + assert!(validate(&Multisig { + threshold: 1, + xpubs: vec![ + parse_xpub("xpub6FMWuwbCA9KhoRzAMm63ZhLspk5S2DM5sePo8J8mQhcS1xyMbAqnc7Q7UescVEVFCS6qBMQLkEJWQ9Z3aDPgBov5nFUYxsJhwumsxM4npSo").unwrap(), + // this xpub corresponds to the mocked seed above at m/45'. + parse_xpub("xpub68yJakxtRe3azab9rb8DJqxDeCG7oBY3zhsNnvZybjTE9qc9Hgw4bCqdLjVGykZrwD6CC6r6xHrnuep5Dmb9uq2R4emCm8YzBuddFyhgvAD").unwrap(), + ], + our_xpub_index: 1, + script_type: ScriptType::P2wsh as _, + }, &[45 + HARDENED]).is_ok()); { // number of cosigners too large @@ -620,7 +624,7 @@ mod tests { "xpub6ECHc4kmTC2tQg2ZoAoazwyag9C4V6yFsZEhjwMJixdVNsUibot6uEvsZY38ZLVqWCtyc9gbzFEwHQLHCT8EiDDKSNNsFAB8NQYRgkiAQwu", "xpub6F7CaxXzBCtvXwpRi61KYyhBRkgT1856ujHV5AbJK6ySCUYoDruBH6Pnsi6eHkDiuKuAJ2tSc9x3emP7aax9Dc3u7nP7RCQXEjLKihQu6w1", ].iter().map(|s| parse_xpub(s).unwrap()).collect(); - assert!(validate(&invalid, keypath, expected_coin).is_err()); + assert!(validate(&invalid, keypath).is_err()); } { @@ -628,21 +632,21 @@ mod tests { let mut invalid = multisig.clone(); invalid.xpubs = vec![]; - assert!(validate(&invalid, keypath, expected_coin).is_err()); + assert!(validate(&invalid, keypath).is_err()); invalid.our_xpub_index = 0; invalid.xpubs = vec![parse_xpub(our_xpub_str).unwrap()]; - assert!(validate(&invalid, keypath, expected_coin).is_err()); + assert!(validate(&invalid, keypath).is_err()); } { // threshold larger than number of cosigners let mut invalid = multisig.clone(); invalid.threshold = 3; - assert!(validate(&invalid, keypath, expected_coin).is_err()); + assert!(validate(&invalid, keypath).is_err()); // threshold zero invalid.threshold = 0; - assert!(validate(&invalid, keypath, expected_coin).is_err()); + assert!(validate(&invalid, keypath).is_err()); } { @@ -650,42 +654,7 @@ mod tests { // bounds). let mut invalid = multisig.clone(); invalid.our_xpub_index = 2; - assert!(validate(&invalid, keypath, expected_coin).is_err()); - } - - { - // invalid keypath, wrong purpose - let mut invalid = multisig.clone(); - let keypath = &[49 + HARDENED, expected_coin, 0 + HARDENED, 2 + HARDENED]; - invalid.xpubs[1] = crate::keystore::get_xpub(keypath).unwrap().into(); - assert!(validate(&invalid, keypath, expected_coin).is_err()); - } - - { - // invalid keypath, wrong coin - - let mut invalid = multisig.clone(); - let keypath = &[48 + HARDENED, expected_coin + 1, 0 + HARDENED, 2 + HARDENED]; - invalid.xpubs[1] = crate::keystore::get_xpub(keypath).unwrap().into(); - assert!(validate(&invalid, keypath, expected_coin).is_err()); - } - - { - // invalid keypath, account too large - - let mut invalid = multisig.clone(); - let keypath = &[48 + HARDENED, expected_coin, 100 + HARDENED, 2 + HARDENED]; - invalid.xpubs[1] = crate::keystore::get_xpub(keypath).unwrap().into(); - assert!(validate(&invalid, keypath, expected_coin).is_err()); - } - - { - // invalid keypath, wrong script type - - let mut invalid = multisig.clone(); - let keypath = &[48 + HARDENED, expected_coin, 0 + HARDENED, 1 + HARDENED]; - invalid.xpubs[1] = crate::keystore::get_xpub(keypath).unwrap().into(); - assert!(validate(&invalid, keypath, expected_coin).is_err()); + assert!(validate(&invalid, keypath).is_err()); } { @@ -693,7 +662,7 @@ mod tests { let mut invalid = multisig.clone(); invalid.xpubs[1] = parse_xpub("xpub6FNT7x2ZEBMhs4jvZJSEBV2qBCBnRidNsyqe7inT9V2wmEn4sqidTEudB4dVSvEjXz2NytcymwWJb8PPYExRycNf9SH8fAHzPWUsQJAmbR3").unwrap(); - assert!(validate(&invalid, keypath, expected_coin).is_err()); + assert!(validate(&invalid, keypath).is_err()); } { @@ -701,7 +670,7 @@ mod tests { let mut invalid = multisig.clone(); invalid.xpubs[0] = invalid.xpubs[1].clone(); - assert!(validate(&invalid, keypath, expected_coin).is_err()); + assert!(validate(&invalid, keypath).is_err()); } } diff --git a/src/rust/bitbox02-rust/src/hww/api/bitcoin/registration.rs b/src/rust/bitbox02-rust/src/hww/api/bitcoin/registration.rs index 949b7d6fe8..bccf299322 100644 --- a/src/rust/bitbox02-rust/src/hww/api/bitcoin/registration.rs +++ b/src/rust/bitbox02-rust/src/hww/api/bitcoin/registration.rs @@ -115,7 +115,7 @@ pub async fn process_register_script_config( let coin = BtcCoin::try_from(*coin)?; let coin_params = params::get(coin); let name = get_name(request).await?; - super::multisig::validate(multisig, keypath, coin_params.bip44_coin)?; + super::multisig::validate(multisig, keypath)?; let xpub_type = XPubType::try_from(request.xpub_type)?; super::multisig::confirm_extended( title, diff --git a/src/rust/bitbox02-rust/src/hww/api/bitcoin/signtx.rs b/src/rust/bitbox02-rust/src/hww/api/bitcoin/signtx.rs index b071d12307..8900fe3e9e 100644 --- a/src/rust/bitbox02-rust/src/hww/api/bitcoin/signtx.rs +++ b/src/rust/bitbox02-rust/src/hww/api/bitcoin/signtx.rs @@ -218,12 +218,9 @@ fn validate_keypath( .or(Err(Error::InvalidInput))?; } Some(pb::BtcScriptConfig { - config: Some(pb::btc_script_config::Config::Multisig(multisig)), + config: Some(pb::btc_script_config::Config::Multisig(_)), }) => { - let script_type = - pb::btc_script_config::multisig::ScriptType::try_from(multisig.script_type)?; - keypath::validate_address_multisig(keypath, params.bip44_coin, script_type) - .or(Err(Error::InvalidInput))?; + keypath::validate_address_policy(keypath).or(Err(Error::InvalidInput))?; } Some(pb::BtcScriptConfig { config: Some(pb::btc_script_config::Config::Policy(_)), @@ -411,7 +408,7 @@ async fn validate_script_configs( keypath, }] = script_configs { - super::multisig::validate(multisig, keypath, coin_params.bip44_coin)?; + super::multisig::validate(multisig, keypath)?; let name = super::multisig::get_name(coin_params.coin, multisig, keypath)? .ok_or(Error::InvalidInput)?; super::multisig::confirm("Spend from", coin_params, &name, multisig).await?;