bitcoin: allow multisig at arbitrary keypaths

Before, we would restrict the account-level keypaths for multisig to be: - m/48'/coin'/account'/1' for P2WSH_P2SH - m/48'/coin'/account'/2' for P2WSH Since the keypath is verified by the user and the coin network is confirmed for every receive/send, random and isolation bypass attacks are mitigated sufficiently, and we can lift this restriction. Note that for wallet policies (of which multisig is a subset of), arbitrary keypaths are already allowed. When exporting an xpub, we furthermore warned about "unusual" keypaths. In addition to the above keypaths, we also allow exporting the xpub at m/45' without warning, as this path is used by Unchained for their vaults.
benma · Jun 4, 2024 · 07d0bc4 · 07d0bc4
1 parent b2d0f21
commit 07d0bc4
Show file tree

Hide file tree

Showing 6 changed files with 68 additions and 240 deletions.
diff --git 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
@@ -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<Response, Error> {
     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
@@ -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;