Merge #637: Improve support for the coming Ledger app 2.1.0

fa3c059 Add comment on disabled multisig tests; multiple keys in multisig are now supported (Salvatore Ingala)
fbcbaee Convert hardened character "h" with "'" in signmessage (Salvatore Ingala)
4c293a4 Removed redundant check: - already done in the previous loop over all inputs. - moreover, the reference to the input_num variable in the error message was wrong (it should have been idx). (Salvatore Ingala)
0030b63 Update docs in can_sign_taproot (Salvatore Ingala)
fb6fb63 Remove unused imports (Salvatore Ingala)
6f148a0 Make sure to run speculos with the correct app version number to use the new protocol (Salvatore Ingala)
1a0c8fa Add implementation of display_multisig_address to ledger.py (Salvatore Ingala)
961f9b5 Cold-who? (Salvatore Ingala)
8627ca7 Add support for message signing with the new protocol; kept old code for legacy protocol (Salvatore Ingala)
486cef5 Never retry legacy protocol for versions >= 2.1.* (Salvatore Ingala)
0cf2df1 Clarify comment about derivation path limitation (Salvatore Ingala)
276183a Remove mentions of limitations that were addressed before version 2.1.0 of the app (Salvatore Ingala)
fc817d9 Switch to new protocol from version 2.1.* (Salvatore Ingala)
099252e Add "Bitcoin Legacy" and "Bitcoin Test Legacy" to recognized app names; use legacy protocol if app version is 1.*, or 2.0.*, or if the app name is "Bitcoin Legacy" or "Bitcoin Test Legacy" (Salvatore Ingala)

Pull request description:

  ### Intro and context
  The upcoming version of the Ledger bitcoin app (version 2.1.0) will remove support to the legacy API (1.*).

  A number of limitations of the 2.* protocol have been addressed since the first release:
  - Each returned signature is now accompanied by the corresponding pubkey, instead of just the input index.
  - Key origin information is no longer required for external xpubs.
  - Multiple internal xpubs are supported (the device will sign for each internal xpub).
  - Introduces a modified wallet policy language matching [this proposal on bitcoin-dev](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-May/020423.html), which addresses some drawbacks of the earlier version (most notably, removing the __change_/address-index_ from key information, and adding it to the policy's descriptor template instead).
  - Support for message signing was added.
  - OP_RETURN outputs are now supported.

  Since there are incompatibilities, the modified protocol from version 2.1.0 is opt-in (signaled by using `01` instead of `00` in the _p1_ field of the APDU), and the protocol of version 2.0.* (using `00` in _p1_) will still be supported in future versions for some time. Nevertheless, the `p1 == 0` protocol should is to be considered deprecated from the moment the 2.1.0 app goes live (expected in early November).

  All 2.* versions before 2.1.0 support the legacy 1.x protocol; therefore, in order to keep things simple, this PR **uses the legacy protocol for any version prior to 2.1.0**). I believe disruption will be minimal: users of the 2.0.* app will downgrade their multisig experience to the legacy protocol (which doesn't recognize change addresses), and are therefore recommended to upgrade to 2.1.0. Nothing changes for users that are still on the 1.* app series (upgrade is, of course, still recommended!).

  Moreover, the 2.1.0 adds **full support to miniscript** within `wsh` descriptors.

  ### Changes in this PR

  This is the list of changes in this PR:
  - Recognize "Bitcoin Legacy" and "Bitcoin Testnet Legacy" as a valid app name for the legacy protocol (regardless of the version). It will be deployed and available as an app in the Ledger app store.
  - Completely switch to the updated version (_p1_ = 1) of the new protocol starting from version 2.1.0; use legacy (1.*) protocol below version 2.1.0. Never retry with the legacy protocol on failure.
  - Remove checks for the limitations that were addressed.
  - Add support for message signing using the new protocol.
  - Add support for displaying multisig addresses.
  - Support multisig wallets with 16 keys (a previous version of the python library was limiting to 15 keys by mistake).

  Miniscript support is left for a separate PR (more comments below).

  ### Drawbacks

  Signing a psbt and displaying an address requires registering the wallet policy on the device. The current version of HWI does that right before signing (and this PR adds the same behavior for displaying addresses).

  This is not secure in a compromised machine if the device participates to multiple multisignature/script wallet accounts, as the adversary could trick the user into spending from a different wallet account.

  Moreover, miniscript support in HWI is not in this PR, since it requires larger changes to HWI.

  I'm working on a BIP for [miniscript wallet policies](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-May/020423.html), and will propose a backwards-compatible and vendor-agnostic way to add native support for wallet policies in a separate PR.

ACKs for top commit:
  achow101:
    ACK fa3c059

Tree-SHA512: 5391c0ad949737336e74050860ed8cf4c8bcef56e58ddedf52dc1e82e737b89a5bb1aa2af080132d2c059f71869ede9d79e1861d590c96e0e1b1d3602e314c35

hwilib/devices/ledger_bitcoin/__init__.py

@@ -5,6 +5,6 @@
 from .client import createClient
 from ...common import Chain
 
-from .wallet import AddressType, Wallet, MultisigWallet, PolicyMapWallet
+from .wallet import AddressType, WalletPolicy, MultisigWallet
 
-__all__ = ["Client", "TransportClient", "createClient", "Chain", "AddressType", "Wallet", "MultisigWallet", "PolicyMapWallet"]
+__all__ = ["Client", "TransportClient", "createClient", "Chain", "AddressType", "WalletPolicy", "MultisigWallet"]

hwilib/devices/ledger_bitcoin/client.py

@@ -9,7 +9,7 @@
 from .client_legacy import LegacyClient
 from .exception import DeviceException, NotSupportedError
 from .merkle import get_merkleized_map_commitment
-from .wallet import Wallet, WalletType, PolicyMapWallet
+from .wallet import WalletPolicy, WalletType
 from ...psbt import PSBT
 from ..._serialize import deser_string
 
@@ -32,6 +32,37 @@ def parse_stream_to_map(f: BufferedReader) -> Mapping[bytes, bytes]:
     return result
 
 
+def read_uint(buf: BytesIO,
+              bit_len: int,
+              byteorder: str = 'little') -> int:
+    assert byteorder in ['little', 'big']
+
+    size: int = bit_len // 8
+    b: bytes = buf.read(size)
+
+    if len(b) < size:
+        raise ValueError(f"Can't read u{bit_len} in buffer!")
+
+    return int.from_bytes(b, byteorder)
+
+
+def read_varint(buf: BytesIO,
+                prefix: Optional[bytes] = None) -> int:
+    b: bytes = prefix if prefix else buf.read(1)
+
+    if not b:
+        raise ValueError(f"Can't read prefix: '{b}'!")
+
+    n: int = {b"\xfd": 2, b"\xfe": 4, b"\xff": 8}.get(b, 1)  # default to 1
+
+    b = buf.read(n) if n > 1 else b
+
+    if len(b) != n:
+        raise ValueError("Can't read varint!")
+
+    return int.from_bytes(b, byteorder="little")
+
+
 class NewClient(Client):
     # internal use for testing: if set to True, sign_psbt will not clone the psbt before converting to psbt version 2
     _no_clone_psbt: bool = False
@@ -65,14 +96,17 @@ def get_extended_pubkey(self, path: str, display: bool = False) -> str:
 
         return response.decode()
 
-    def register_wallet(self, wallet: Wallet) -> Tuple[bytes, bytes]:
-        if wallet.type != WalletType.POLICYMAP:
-            raise ValueError("wallet type must be POLICYMAP")
+    def register_wallet(self, wallet: WalletPolicy) -> Tuple[bytes, bytes]:
+        if wallet.version not in [WalletType.WALLET_POLICY_V1, WalletType.WALLET_POLICY_V2]:
+            raise ValueError("invalid wallet policy version")
 
         client_intepreter = ClientCommandInterpreter()
         client_intepreter.add_known_preimage(wallet.serialize())
         client_intepreter.add_known_list([k.encode() for k in wallet.keys_info])
 
+        # necessary for version 1 of the protocol (available since version 2.1.0 of the app)
+        client_intepreter.add_known_preimage(wallet.descriptor_template.encode())
+
         sw, response = self._make_request(
             self.builder.register_wallet(wallet), client_intepreter
         )
@@ -90,17 +124,15 @@ def register_wallet(self, wallet: Wallet) -> Tuple[bytes, bytes]:
 
     def get_wallet_address(
         self,
-        wallet: Wallet,
+        wallet: WalletPolicy,
         wallet_hmac: Optional[bytes],
         change: int,
         address_index: int,
         display: bool,
     ) -> str:
 
-        if wallet.type != WalletType.POLICYMAP or not isinstance(
-            wallet, PolicyMapWallet
-        ):
-            raise ValueError("wallet type must be POLICYMAP")
+        if not isinstance(wallet, WalletPolicy) or wallet.version not in [WalletType.WALLET_POLICY_V1, WalletType.WALLET_POLICY_V2]:
+            raise ValueError("wallet type must be WalletPolicy, with version either WALLET_POLICY_V1 or WALLET_POLICY_V2")
 
         if change != 0 and change != 1:
             raise ValueError("Invalid change")
@@ -109,6 +141,9 @@ def get_wallet_address(
         client_intepreter.add_known_list([k.encode() for k in wallet.keys_info])
         client_intepreter.add_known_preimage(wallet.serialize())
 
+        # necessary for version 1 of the protocol (available since version 2.1.0 of the app)
+        client_intepreter.add_known_preimage(wallet.descriptor_template.encode())
+
         sw, response = self._make_request(
             self.builder.get_wallet_address(
                 wallet, wallet_hmac, address_index, change, display
@@ -121,7 +156,7 @@ def get_wallet_address(
 
         return response.decode()
 
-    def sign_psbt(self, psbt: PSBT, wallet: Wallet, wallet_hmac: Optional[bytes]) -> Mapping[int, bytes]:
+    def sign_psbt(self, psbt: PSBT, wallet: WalletPolicy, wallet_hmac: Optional[bytes]) -> List[Tuple[int, bytes, bytes]]:
         """Signs a PSBT using a registered wallet (or a standard wallet that does not need registration).
 
         Signature requires explicit approval from the user.
@@ -132,18 +167,21 @@ def sign_psbt(self, psbt: PSBT, wallet: Wallet, wallet_hmac: Optional[bytes]) ->
             A PSBT of version 0 or 2, with all the necessary information to sign the inputs already filled in; what the
             required fields changes depending on the type of input.
             The non-witness UTXO must be present for both legacy and SegWit inputs, or the hardware wallet will reject
-            signing (this will change for Taproot inputs).
+            signing. This is not required for Taproot inputs.
 
-        wallet : Wallet
+        wallet : WalletPolicy
             The registered wallet policy, or a standard wallet policy.
 
         wallet_hmac: Optional[bytes]
             For a registered wallet, the hmac obtained at wallet registration. `None` for a standard wallet policy.
 
         Returns
         -------
-        Mapping[int, bytes]
-            A mapping that has as keys the indexes of inputs that the Hardware Wallet signed, and the corresponding signatures as values.
+        List[Tuple[int, bytes, bytes]]
+            A list of tuples returned by the hardware wallets, where each element is a tuple of:
+            - an integer, the index of the input being signed;
+            - a `bytes` array of length 33 (compressed ecdsa pubkey) or 32 (x-only BIP-0340 pubkey), the corresponding pubkey for this signature;
+            - a `bytes` array with the signature.
         """
         assert psbt.version == 2
         psbt_v2 = psbt
@@ -161,6 +199,9 @@ def sign_psbt(self, psbt: PSBT, wallet: Wallet, wallet_hmac: Optional[bytes]) ->
         client_intepreter.add_known_list([k.encode() for k in wallet.keys_info])
         client_intepreter.add_known_preimage(wallet.serialize())
 
+        # necessary for version 1 of the protocol (available since version 2.1.0 of the app)
+        client_intepreter.add_known_preimage(wallet.descriptor_template.encode())
+
         global_map: Mapping[bytes, bytes] = parse_stream_to_map(f)
         client_intepreter.add_known_mapping(global_map)
 
@@ -199,7 +240,19 @@ def sign_psbt(self, psbt: PSBT, wallet: Wallet, wallet_hmac: Optional[bytes]) ->
         if any(len(x) <= 1 for x in results):
             raise RuntimeError("Invalid response")
 
-        return {int(res[0]): res[1:] for res in results}
+        results_list: List[Tuple[int, bytes, bytes]] = []
+        for res in results:
+            res_buffer = BytesIO(res)
+            input_index = read_varint(res_buffer)
+
+            pubkey_len = read_uint(res_buffer, 8)
+            pubkey = res_buffer.read(pubkey_len)
+
+            signature = res_buffer.read()
+
+            results_list.append((input_index, pubkey, signature))
+
+        return results_list
 
     def get_master_fingerprint(self) -> bytes:
 
@@ -210,6 +263,24 @@ def get_master_fingerprint(self) -> bytes:
 
         return response
 
+    def sign_message(self, message: Union[str, bytes], bip32_path: str) -> str:
+        if isinstance(message, str):
+            message_bytes = message.encode("utf-8")
+        else:
+            message_bytes = message
+
+        chunks = [message_bytes[64 * i: 64 * i + 64] for i in range((len(message_bytes) + 63) // 64)]
+
+        client_intepreter = ClientCommandInterpreter()
+        client_intepreter.add_known_list(chunks)
+
+        sw, response = self._make_request(self.builder.sign_message(message_bytes, bip32_path), client_intepreter)
+
+        if sw != 0x9000:
+            raise DeviceException(error_code=sw, ins=BitcoinInsType.GET_EXTENDED_PUBKEY)
+
+        return base64.b64encode(response).decode('utf-8')
+
 
 def createClient(comm_client: Optional[TransportClient] = None, chain: Chain = Chain.MAIN, debug: bool = False) -> Union[LegacyClient, NewClient]:
     if comm_client is None:
@@ -218,10 +289,18 @@ def createClient(comm_client: Optional[TransportClient] = None, chain: Chain = C
     base_client = Client(comm_client, chain)
     app_name, app_version, _ = base_client.get_version()
 
-    if app_name not in ["Bitcoin", "Bitcoin Test", "app"]:
+    if app_name not in ["Bitcoin", "Bitcoin Test", "Bitcoin Legacy", "Bitcoin Test Legacy", "app"]:
         raise NotSupportedError(0x6A82, None, "Ledger is not in either the Bitcoin or Bitcoin Testnet app")
 
-    if app_version >= "2":
-        return NewClient(comm_client, chain)
-    else:
+    app_version_major, app_version_minor, _ = app_version.split(".", 2)
+
+    # App versions using the legacy protocol:
+    # - any 1.*.* version
+    # - any 2.0.* version
+    # - any version if the app name is "Bitcoin Legacy" or "Bitcoin Test Legacy"
+    is_legacy = int(app_version_major) <= 1 or (int(app_version_major) == 2 and int(app_version_minor) == 0) or "Legacy" in app_name
+
+    if is_legacy:
         return LegacyClient(comm_client, chain)
+    else:
+        return NewClient(comm_client, chain)

hwilib/devices/ledger_bitcoin/client_base.py

@@ -1,4 +1,4 @@
-from typing import Tuple, Mapping, Optional, Union
+from typing import Tuple, Optional, Union, List
 from io import BytesIO
 
 from .ledgercomm import Transport
@@ -8,7 +8,7 @@
 from .command_builder import DefaultInsType
 from .exception import DeviceException
 
-from .wallet import Wallet
+from .wallet import WalletPolicy
 from ...psbt import PSBT
 from ..._serialize import deser_string
 
@@ -130,13 +130,13 @@ def get_extended_pubkey(self, path: str, display: bool = False) -> str:
 
         raise NotImplementedError
 
-    def register_wallet(self, wallet: Wallet) -> Tuple[bytes, bytes]:
+    def register_wallet(self, wallet: WalletPolicy) -> Tuple[bytes, bytes]:
         """Registers a wallet policy with the user. After approval returns the wallet id and hmac to be stored on the client.
 
         Parameters
         ----------
-        wallet : Wallet
-            The Wallet policy to register on the device.
+        wallet : WalletPolicy
+            The wallet policy to register on the device.
 
         Returns
         -------
@@ -149,7 +149,7 @@ def register_wallet(self, wallet: Wallet) -> Tuple[bytes, bytes]:
 
     def get_wallet_address(
         self,
-        wallet: Wallet,
+        wallet: WalletPolicy,
         wallet_hmac: Optional[bytes],
         change: int,
         address_index: int,
@@ -160,7 +160,7 @@ def get_wallet_address(
 
         Parameters
         ----------
-        wallet : Wallet
+        wallet : WalletPolicy
             The registered wallet policy, or a standard wallet policy.
 
         wallet_hmac: Optional[bytes]
@@ -183,7 +183,7 @@ def get_wallet_address(
 
         raise NotImplementedError
 
-    def sign_psbt(self, psbt: PSBT, wallet: Wallet, wallet_hmac: Optional[bytes]) -> Mapping[int, bytes]:
+    def sign_psbt(self, psbt: PSBT, wallet: WalletPolicy, wallet_hmac: Optional[bytes]) -> List[Tuple[int, bytes, bytes]]:
         """Signs a PSBT using a registered wallet (or a standard wallet that does not need registration).
 
         Signature requires explicit approval from the user.
@@ -196,16 +196,19 @@ def sign_psbt(self, psbt: PSBT, wallet: Wallet, wallet_hmac: Optional[bytes]) ->
             The non-witness UTXO must be present for both legacy and SegWit inputs, or the hardware wallet will reject
             signing (this will change for Taproot inputs).
 
-        wallet : Wallet
+        wallet : WalletPolicy
             The registered wallet policy, or a standard wallet policy.
 
         wallet_hmac: Optional[bytes]
             For a registered wallet, the hmac obtained at wallet registration. `None` for a standard wallet policy.
 
         Returns
         -------
-        Mapping[int, bytes]
-            A mapping that has as keys the indexes of inputs that the Hardware Wallet signed, and the corresponding signatures as values.
+        List[Tuple[int, bytes, bytes]]
+            A list of tuples returned by the hardware wallets, where each element is a tuple of:
+            - an integer, the index of the input being signed;
+            - a `bytes` array of length 33 (compressed ecdsa pubkey) or 32 (x-only BIP-0340 pubkey), the corresponding pubkey for this signature;
+            - a `bytes` array with the signature.
         """
 
         raise NotImplementedError