ethereum: allow mainnet on testnet keypath and vice versa

benma · benma · commit 18f2c907cd73 · 2021-09-06T12:22:08.000+02:00
Until now:

Mainnet coins were under the xpub: m/44'/60'/0'/0
Testnet coins were under the xpub: m/44'/1'/0'/0

With this commit, one can use either path with any Ethereum
network. If it is not matching the above, a warning is displayed.

The reason this is changed is that some wallets, like Metamask, use
the 60' path for all networks. Using testnet coins on Metamask hence
fails since the BitBox02 rejected the transactions based on the
keypath.

Mixing keypaths like this is safe even if the user ignores the
warning, as the chain id (unique per ETH network) is part of the
transaction sighash, so one cannot be tricked into sending mainnet
coins instead of testnet coins. The only risk is that mainnet coins
might not be found if sent to the testnet keypath, hence the warning.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@ customers cannot upgrade their bootloader, its changes are recorded separately.
 ## Firmware
 
 ### [Unreleased]
+- Allow mainnet keypaths (`m/44'/60'/0'/0/*`) Rinkeby and Ropsten, and testnet keypaths (`m/44'/1'/0'/0/*`) for Ethereum mainnet
 - Display granular error codes when unlock fails unexpectedly
 - Remove RandomNumber API endpoint
 
diff --git a/src/rust/apps/ethereum/src/keypath.rs b/src/rust/apps/ethereum/src/keypath.rs
@@ -17,21 +17,23 @@ use util::bip32::HARDENED;
 const ACCOUNT_MAX: u32 = 99; // 100 accounts
 
 /// Does limit checks the keypath, whitelisting bip44 purpose, account and change.
-/// Only allows the well-known xpub of m'/44'/60'/0'/0 for now.
+/// Only allows the well-known xpubs of m'/44'/60'/0'/0 and m'/44'/1'/0'/0 for now.
 /// Since ethereum doesn't use the "change" path part it is always 0 and have become part of the
 /// xpub keypath.
 /// @return true if the keypath is valid, false if it is invalid.
-pub fn is_valid_keypath_xpub(keypath: &[u32], expected_coin: u32) -> bool {
-    keypath.len() == 4 && keypath[..4] == [44 + HARDENED, expected_coin, 0 + HARDENED, 0]
+pub fn is_valid_keypath_xpub(keypath: &[u32]) -> bool {
+    keypath.len() == 4
+        && (keypath[..4] == [44 + HARDENED, 60 + HARDENED, 0 + HARDENED, 0]
+            || keypath[..4] == [44 + HARDENED, 1 + HARDENED, 0 + HARDENED, 0])
 }
 
 /// Does limit checks the keypath, whitelisting bip44 purpose, account and change.
 /// Returns true if the keypath is valid, false if it is invalid.
-pub fn is_valid_keypath_address(keypath: &[u32], expected_coin: u32) -> bool {
+pub fn is_valid_keypath_address(keypath: &[u32]) -> bool {
     if keypath.len() != 5 {
         return false;
     }
-    if !is_valid_keypath_xpub(&keypath[..4], expected_coin) {
+    if !is_valid_keypath_xpub(&keypath[..4]) {
         return false;
     }
     if keypath[4] > ACCOUNT_MAX {
@@ -46,66 +48,98 @@ mod tests {
 
     #[test]
     fn test_is_valid_keypath_xpub() {
-        let expected_coin = 60 + HARDENED;
-        assert!(is_valid_keypath_xpub(
-            &[44 + HARDENED, expected_coin, 0 + HARDENED, 0],
-            expected_coin
-        ));
+        assert!(is_valid_keypath_xpub(&[
+            44 + HARDENED,
+            60 + HARDENED,
+            0 + HARDENED,
+            0
+        ]));
+        assert!(is_valid_keypath_xpub(&[
+            44 + HARDENED,
+            1 + HARDENED,
+            0 + HARDENED,
+            0
+        ]));
         // wrong coin.
-        assert!(!is_valid_keypath_xpub(
-            &[44 + HARDENED, expected_coin, 0 + HARDENED, 0],
-            expected_coin + 1,
-        ));
+        assert!(!is_valid_keypath_xpub(&[
+            44 + HARDENED,
+            0 + HARDENED,
+            0 + HARDENED,
+            0
+        ]));
         // too short
-        assert!(!is_valid_keypath_xpub(
-            &[44 + HARDENED, expected_coin, 0 + HARDENED],
-            expected_coin + 1,
-        ));
+        assert!(!is_valid_keypath_xpub(&[
+            44 + HARDENED,
+            60 + HARDENED,
+            0 + HARDENED
+        ]));
         // too long
-        assert!(!is_valid_keypath_xpub(
-            &[44 + HARDENED, expected_coin, 0 + HARDENED, 0, 0],
-            expected_coin + 1,
-        ));
+        assert!(!is_valid_keypath_xpub(&[
+            44 + HARDENED,
+            60 + HARDENED,
+            0 + HARDENED,
+            0,
+            0
+        ]));
     }
 
     #[test]
     fn test_is_valid_keypath_address() {
-        let expected_coin = 60 + HARDENED;
-        let keypath_for_account =
-            |account| [44 + HARDENED, expected_coin, 0 + HARDENED, 0, account];
-
         // 100 good paths.
         for account in 0..100 {
-            assert!(is_valid_keypath_address(
-                &keypath_for_account(account),
-                expected_coin
-            ));
+            assert!(is_valid_keypath_address(&[
+                44 + HARDENED,
+                60 + HARDENED,
+                0 + HARDENED,
+                0,
+                account
+            ]));
+            assert!(is_valid_keypath_address(&[
+                44 + HARDENED,
+                1 + HARDENED,
+                0 + HARDENED,
+                0,
+                account
+            ]));
             // wrong coin
-            assert!(!is_valid_keypath_address(
-                &keypath_for_account(account),
-                expected_coin + 1
-            ));
+            assert!(!is_valid_keypath_address(&[
+                44 + HARDENED,
+                0 + HARDENED,
+                0 + HARDENED,
+                0,
+                account
+            ]));
         }
-        assert!(!is_valid_keypath_address(
-            &keypath_for_account(100),
-            expected_coin
-        ));
+        // account too high
+        assert!(!is_valid_keypath_address(&[
+            44 + HARDENED,
+            60 + HARDENED,
+            0 + HARDENED,
+            0,
+            100
+        ]));
 
         // too short
-        assert!(!is_valid_keypath_address(
-            &[44 + HARDENED, expected_coin, 0 + HARDENED, 0],
-            expected_coin
-        ));
+        assert!(!is_valid_keypath_address(&[
+            44 + HARDENED,
+            60 + HARDENED,
+            0 + HARDENED,
+            0
+        ]));
         // too long
-        assert!(!is_valid_keypath_address(
-            &[44 + HARDENED, expected_coin, 0 + HARDENED, 0, 0, 0],
-            expected_coin
-        ));
+        assert!(!is_valid_keypath_address(&[
+            44 + HARDENED,
+            60 + HARDENED,
+            0 + HARDENED,
+            0,
+            0,
+            0
+        ]));
         // tweak keypath elements
         for i in 0..4 {
-            let mut keypath = keypath_for_account(0);
+            let mut keypath = [44 + HARDENED, 60 + HARDENED, 0 + HARDENED, 0, 0];
             keypath[i] += 1;
-            assert!(!is_valid_keypath_address(&keypath, expected_coin));
+            assert!(!is_valid_keypath_address(&keypath));
         }
     }
 }
diff --git a/src/rust/bitbox02-rust/src/hww/api/ethereum.rs b/src/rust/bitbox02-rust/src/hww/api/ethereum.rs
@@ -18,6 +18,7 @@ compile_error!(
 );
 
 mod amount;
+mod keypath;
 mod pubrequest;
 mod sign;
 mod signmsg;
diff --git a/src/rust/bitbox02-rust/src/hww/api/ethereum/keypath.rs b/src/rust/bitbox02-rust/src/hww/api/ethereum/keypath.rs
@@ -0,0 +1,50 @@
+// Copyright 2021 Shift Crypto AG
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use super::Error;
+use crate::workflow::confirm;
+use bitbox02::app_eth::Params;
+
+/// If the second element of `keypath` does not match the expected bip44 coin value for the given
+/// coin, we warn the user about an unusual keypath.
+///
+/// The keypath is already assumed to be validated and that the second element is one of the
+/// Ethereum bip44 values, e.g. `60'` or `1'`.
+///
+/// A warning suffices so the user does not accidentally send e.g. mainnet coins to a testnet path
+/// (m/44'/1'/...). It is safe to make a transaction on the 'wrong' keypath as the chain id is
+/// unique and part of the transaction sighash.
+pub async fn warn_unusual_keypath(
+    params: &Params,
+    title: &str,
+    keypath: &[u32],
+) -> Result<(), Error> {
+    if keypath.len() < 2 {
+        return Err(Error::InvalidInput);
+    }
+    if keypath[1] != params.bip44_coin {
+        let body = format!(
+            "Unusual keypath warning: {}. Proceed only if you know what you are doing.",
+            util::bip32::to_string(keypath)
+        );
+        let params = confirm::Params {
+            title,
+            body: &body,
+            scrollable: true,
+            ..Default::default()
+        };
+        return Ok(confirm::confirm(&params).await?);
+    }
+    Ok(())
+}
diff --git a/src/rust/bitbox02-rust/src/hww/api/ethereum/pubrequest.rs b/src/rust/bitbox02-rust/src/hww/api/ethereum/pubrequest.rs
@@ -42,7 +42,7 @@ async fn process_address(request: &pb::EthPubRequest) -> Result<Response, Error>
             )
         };
 
-    if !ethereum::keypath::is_valid_keypath_address(&request.keypath, params.bip44_coin) {
+    if !ethereum::keypath::is_valid_keypath_address(&request.keypath) {
         return Err(Error::InvalidInput);
     }
     let pubkey = bitbox02::keystore::secp256k1_pubkey_uncompressed(&request.keypath)
@@ -54,6 +54,7 @@ async fn process_address(request: &pb::EthPubRequest) -> Result<Response, Error>
             Some(erc20_params) => erc20_params.name,
             None => params.name,
         };
+        super::keypath::warn_unusual_keypath(&params, title, &request.keypath).await?;
         let params = confirm::Params {
             title,
             title_autowrap: true,
@@ -73,8 +74,8 @@ fn process_xpub(request: &pb::EthPubRequest) -> Result<Response, Error> {
         return Err(Error::InvalidInput);
     }
 
-    let params = bitbox02::app_eth::params_get(request.coin as _).ok_or(Error::InvalidInput)?;
-    if !ethereum::keypath::is_valid_keypath_xpub(&request.keypath, params.bip44_coin) {
+    bitbox02::app_eth::params_get(request.coin as _).ok_or(Error::InvalidInput)?;
+    if !ethereum::keypath::is_valid_keypath_xpub(&request.keypath) {
         return Err(Error::InvalidInput);
     }
     let xpub = keystore::encode_xpub_at_keypath(&request.keypath, keystore::xpub_type_t::XPUB)
@@ -201,6 +202,44 @@ mod tests {
             }))
         );
 
+        static mut CONFIRM_COUNTER: u32 = 0;
+
+        // All good, with display, unusual keypath.
+        mock(Data {
+            ui_confirm_create: Some(Box::new(|params| {
+                match unsafe {
+                    CONFIRM_COUNTER += 1;
+                    CONFIRM_COUNTER
+                } {
+                    1 => {
+                        assert_eq!(params.title, "Ropsten");
+                        assert_eq!(params.body, "Unusual keypath warning: m/44'/60'/0'/0/0. Proceed only if you know what you are doing.");
+                    }
+                    2 => {
+                        assert_eq!(params.title, "Ropsten");
+                        assert_eq!(params.body, ADDRESS);
+                    }
+                    _ => panic!("too many user confirmations"),
+                }
+                true
+            })),
+            ..Default::default()
+        });
+        mock_unlocked();
+        assert_eq!(
+            block_on(process(&pb::EthPubRequest {
+                output_type: OutputType::Address as _,
+                keypath: [44 + HARDENED, 60 + HARDENED, 0 + HARDENED, 0, 0].to_vec(),
+                coin: pb::EthCoin::RopstenEth as _,
+                display: true,
+                contract_address: b"".to_vec(),
+            })),
+            Ok(Response::Pub(pb::PubResponse {
+                r#pub: ADDRESS.into()
+            }))
+        );
+        assert_eq!(unsafe { CONFIRM_COUNTER }, 2);
+
         // Keystore locked.
         mock(Data {
             ui_confirm_create: Some(Box::new(|_| true)),
diff --git a/src/rust/bitbox02-rust/src/hww/api/ethereum/sign.rs b/src/rust/bitbox02-rust/src/hww/api/ethereum/sign.rs
@@ -186,9 +186,10 @@ async fn verify_standard_transaction(
 pub async fn process(request: &pb::EthSignRequest) -> Result<Response, Error> {
     let params = params_get(request.coin as _).ok_or(Error::InvalidInput)?;
 
-    if !ethereum::keypath::is_valid_keypath_address(&request.keypath, params.bip44_coin) {
+    if !ethereum::keypath::is_valid_keypath_address(&request.keypath) {
         return Err(Error::InvalidInput);
     }
+    super::keypath::warn_unusual_keypath(&params, params.name, &request.keypath).await?;
 
     // Size limits.
     if request.nonce.len() > 16
@@ -376,6 +377,59 @@ mod tests {
         );
     }
 
+    /// Standard ETH transaction on an unusual keypath (Ropsten on mainnet keypath)
+    #[test]
+    pub fn test_process_warn_unusual_keypath() {
+        let _guard = MUTEX.lock().unwrap();
+
+        const KEYPATH: &[u32] = &[44 + HARDENED, 60 + HARDENED, 0 + HARDENED, 0, 0];
+
+        static mut CONFIRM_COUNTER: u32 = 0;
+        mock(Data {
+            ui_confirm_create: Some(Box::new(|params| {
+                match unsafe {
+                    CONFIRM_COUNTER += 1;
+                    CONFIRM_COUNTER
+                } {
+                    1 => {
+                        assert_eq!(params.title, "Ropsten");
+                        assert_eq!(params.body, "Unusual keypath warning: m/44'/60'/0'/0/0. Proceed only if you know what you are doing.");
+                        true
+                    }
+                    _ => panic!("too many user confirmations"),
+                }
+            })),
+            ui_transaction_address_create: Some(Box::new(|amount, address| {
+                assert_eq!(amount, "0.530564 TETH");
+                assert_eq!(address, "0x04F264Cf34440313B4A0192A352814FBe927b885");
+                true
+            })),
+            ui_transaction_fee_create: Some(Box::new(|total, fee| {
+                assert_eq!(total, "0.53069 TETH");
+                assert_eq!(fee, "0.000126 TETH");
+                true
+            })),
+            ..Default::default()
+        });
+        mock_unlocked();
+
+        block_on(process(&pb::EthSignRequest {
+            coin: pb::EthCoin::RopstenEth as _,
+            keypath: KEYPATH.to_vec(),
+            nonce: b"\x1f\xdc".to_vec(),
+            gas_price: b"\x01\x65\xa0\xbc\x00".to_vec(),
+            gas_limit: b"\x52\x08".to_vec(),
+            recipient:
+                b"\x04\xf2\x64\xcf\x34\x44\x03\x13\xb4\xa0\x19\x2a\x35\x28\x14\xfb\xe9\x27\xb8\x85"
+                    .to_vec(),
+            value: b"\x07\x5c\xf1\x25\x9e\x9c\x40\x00".to_vec(),
+            data: b"".to_vec(),
+            host_nonce_commitment: None,
+        }))
+        .unwrap();
+        assert_eq!(unsafe { CONFIRM_COUNTER }, 1);
+    }
+
     /// Standard ETH transaction with an unknown data field.
     #[test]
     pub fn test_process_standard_transaction_with_data() {
diff --git a/src/rust/bitbox02-rust/src/hww/api/ethereum/signmsg.rs b/src/rust/bitbox02-rust/src/hww/api/ethereum/signmsg.rs
