Merge pull request LedgerHQ#141 from LedgerHQ/paranoidclient

Reject policies that are not correctly implemented in certain app versions

bitcoin_client/CHANGELOG.md

@@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 Dates are in `dd-mm-yyyy` format.
 
+## [0.2.1] - 18-04-2023
+
+### Changed
+- Avoid using miniscript policies containing an `a:` fragment on versions below `2.1.2` of the bitcoin app.
+
+## [0.2.0] - 3-04-2023
+
+This release introduces a breaking change in the return type of the `sign_psbt`method.
+
+### Added
+- Added new `PartialSignature` data class together with support for taproot script signing, which is supported in version `2.1.2` of the bitcoin app.
+
 ## [0.1.2] - 09-01-2023
 
 ### Fixed

bitcoin_client/ledger_bitcoin/__init__.py

@@ -7,7 +7,7 @@
 
 from .wallet import AddressType, WalletPolicy, MultisigWallet, WalletType
 
-__version__ = '0.2.0'
+__version__ = '0.2.1'
 
 __all__ = [
     "Client",

bitcoin_client/ledger_bitcoin/client.py

@@ -2,6 +2,7 @@
 from typing import Tuple, List, Mapping, Optional, Union
 import base64
 from io import BytesIO, BufferedReader
+import re
 
 from .command_builder import BitcoinCommandBuilder, BitcoinInsType
 from .common import Chain, read_uint, read_varint
@@ -51,6 +52,11 @@ def _make_partial_signature(pubkey_augm: bytes, signature: bytes) -> PartialSign
         return PartialSignature(signature=signature, pubkey=pubkey_augm)
 
 
+def _contains_a_fragment(desc_tmp: str) -> bool:
+    """Returns true if the given descriptor template contains the `a:` fragment."""
+    return any('a' in match for match in re.findall(r'[asctdvjnlu]+:', desc_tmp))
+
+
 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
@@ -88,6 +94,8 @@ 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")
 
+        self._validate_policy(wallet)
+
         client_intepreter = ClientCommandInterpreter()
         client_intepreter.add_known_preimage(wallet.serialize())
         client_intepreter.add_known_list([k.encode() for k in wallet.keys_info])
@@ -125,6 +133,8 @@ def get_wallet_address(
         if change != 0 and change != 1:
             raise ValueError("Invalid change")
 
+        self._validate_policy(wallet)
+
         client_intepreter = ClientCommandInterpreter()
         client_intepreter.add_known_list([k.encode() for k in wallet.keys_info])
         client_intepreter.add_known_preimage(wallet.serialize())
@@ -145,6 +155,9 @@ def get_wallet_address(
         return response.decode()
 
     def sign_psbt(self, psbt: Union[PSBT, bytes, str], wallet: WalletPolicy, wallet_hmac: Optional[bytes]) -> List[Tuple[int, PartialSignature]]:
+
+        self._validate_policy(wallet)
+
         psbt = normalize_psbt(psbt)
 
         if psbt.version != 2:
@@ -252,6 +265,15 @@ def sign_message(self, message: Union[str, bytes], bip32_path: str) -> str:
 
         return base64.b64encode(response).decode('utf-8')
 
+    def _validate_policy(self, wallet_policy: WalletPolicy):
+        """Performs any additional checks before we use a wallet policy"""
+        if _contains_a_fragment(wallet_policy.descriptor_template):
+            _, app_version, _ = self.get_version()
+            if app_version in ["2.1.0", "2.1.1"]:
+                # Versions 2.1.0 and 2.1.1 produced incorrect scripts for policies containing
+                # the `a:` fragment.
+                raise RuntimeError("Please update your Ledger Bitcoin app.")
+
 
 def createClient(comm_client: Optional[TransportClient] = None, chain: Chain = Chain.MAIN, debug: bool = False) -> Union[LegacyClient, NewClient]:
     if comm_client is None:

bitcoin_client_js/package.json

@@ -1,6 +1,6 @@
 {
   "name": "ledger-bitcoin",
-  "version": "0.2.0",
+  "version": "0.2.1",
   "description": "Ledger Hardware Wallet Bitcoin Application Client",
   "main": "build/main/index.js",
   "typings": "build/main/index.d.ts",

bitcoin_client_js/src/__tests__/appClient.test.ts

@@ -128,6 +128,11 @@ describe("test AppClient", () => {
     await killProcess(sp);
   });
 
+  it("can retrieve the app's version", async () => {
+    const result = await app.getAppAndVersion();
+    expect(result.name).toEqual("Bitcoin Test");
+    expect(result.version.split(".")[0]).toEqual("2")
+  });
 
   it("can retrieve the master fingerprint", async () => {
     const result = await app.getMasterFingerprint();

bitcoin_client_js/src/lib/appClient.ts

@@ -62,6 +62,14 @@ function makePartialSignature(pubkeyAugm: Buffer, signature: Buffer): PartialSig
   }
 }
 
+/**
+ * Checks whether a descriptor template contains an `a:` fragment.
+ */
+function containsA(descriptorTemplate: string): boolean {
+  const matches = descriptorTemplate.match(/[asctdvjnlu]+:/g) || [];
+  return matches.some(match => match.includes('a'));
+}
+
 /**
  * This class encapsulates the APDU protocol documented at
  * https://github.com/LedgerHQ/app-bitcoin-new/blob/master/doc/bitcoin.md
@@ -106,6 +114,34 @@ export class AppClient {
     return response.slice(0, -2); // drop the status word (can only be 0x9000 at this point)
   }
 
+  /**
+   * Returns an object containing the currently running app's name, version and the device status flags.
+   *
+   * @returns an object with app name, version and device status flags.
+   */
+  public async getAppAndVersion(): Promise<{
+    name: string;
+    version: string;
+    flags: number | Buffer;
+  }> {
+    const r = await this.transport.send(0xb0, 0x01, 0x00, 0x00);
+    let i = 0;
+    const format = r[i++];
+    if (format !== 1) throw new Error("Unexpected response")
+
+    const nameLength = r[i++];
+    const name = r.slice(i, (i += nameLength)).toString("ascii");
+    const versionLength = r[i++];
+    const version = r.slice(i, (i += versionLength)).toString("ascii");
+    const flagLength = r[i++];
+    const flags = r.slice(i, (i += flagLength));
+    return {
+      name,
+      version,
+      flags,
+    };
+  };
+
   /**
    * Requests the BIP-32 extended pubkey to the hardware wallet.
    * If `display` is `false`, only standard paths will be accepted; an error is returned if an unusual path is
@@ -148,6 +184,8 @@ export class AppClient {
     walletPolicy: WalletPolicy
   ): Promise<readonly [Buffer, Buffer]> {
 
+    await this.validatePolicy(walletPolicy);
+
     const clientInterpreter = new ClientCommandInterpreter();
 
     clientInterpreter.addKnownWalletPolicy(walletPolicy);
@@ -198,6 +236,8 @@ export class AppClient {
       throw new Error('Invalid HMAC length');
     }
 
+    await this.validatePolicy(walletPolicy);
+
     const clientInterpreter = new ClientCommandInterpreter();
 
     clientInterpreter.addKnownWalletPolicy(walletPolicy);
@@ -239,6 +279,8 @@ export class AppClient {
     walletHMAC: Buffer | null,
     progressCallback?: () => void
   ): Promise<[number, PartialSignature][]> {
+    await this.validatePolicy(walletPolicy);
+
     if (typeof psbt === 'string') {
       psbt = Buffer.from(psbt, "base64");
     }
@@ -359,6 +401,18 @@ export class AppClient {
 
     return result.toString('base64');
   }
+
+  /* Performs any additional checks on the policy before using it.*/
+  private async validatePolicy(walletPolicy: WalletPolicy) {
+    if (containsA(walletPolicy.descriptorTemplate)) {
+      const appAndVer = await this.getAppAndVersion();
+      if (["2.1.0", "2.1.1"].includes(appAndVer.version)) {
+        // Versions 2.1.0 and 2.1.1 produced incorrect scripts for policies containing
+        // the `a:` fragment.
+        throw new Error("Please update your Ledger Bitcoin app.")
+      }
+    }
+  }
 }
 
 export default AppClient;

bitcoin_client_rs/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "ledger_bitcoin_client"
-version = "0.1.2"
+version = "0.1.3"
 authors = ["Edouard Paris <[email protected]>"]
 edition = "2018"
 description = "Ledger Bitcoin application client"

bitcoin_client_rs/src/async_client.rs

@@ -20,7 +20,7 @@ use crate::{
     error::BitcoinClientError,
     interpreter::{get_merkleized_map_commitment, ClientCommandInterpreter},
     psbt::*,
-    wallet::WalletPolicy,
+    wallet::{contains_a, WalletPolicy},
 };
 
 /// BitcoinClient calls and interprets commands with the Ledger Device.
@@ -68,6 +68,21 @@ impl<T: Transport> BitcoinClient<T> {
         }
     }
 
+    async fn validate_policy(
+        &self,
+        wallet: &WalletPolicy,
+    ) -> Result<(), BitcoinClientError<T::Error>> {
+        if contains_a(&wallet.descriptor_template) {
+            let (_, version, _) = self.get_version().await?;
+            if version == "2.1.0" || version == "2.1.1" {
+                // Versions 2.1.0 and 2.1.1 produced incorrect scripts for policies containing
+                // the `a:` fragment.
+                return Err(BitcoinClientError::UnsupportedAppVersion);
+            }
+        }
+        Ok(())
+    }
+
     /// Returns the currently running app's name, version and state flags
     pub async fn get_version(
         &self,
@@ -138,6 +153,8 @@ impl<T: Transport> BitcoinClient<T> {
         &self,
         wallet: &WalletPolicy,
     ) -> Result<([u8; 32], [u8; 32]), BitcoinClientError<T::Error>> {
+        self.validate_policy(&wallet).await?;
+
         let cmd = command::register_wallet(wallet);
         let mut intpr = ClientCommandInterpreter::new();
         intpr.add_known_preimage(wallet.serialize());
@@ -173,6 +190,8 @@ impl<T: Transport> BitcoinClient<T> {
         address_index: u32,
         display: bool,
     ) -> Result<bitcoin::Address, BitcoinClientError<T::Error>> {
+        self.validate_policy(&wallet).await?;
+
         let mut intpr = ClientCommandInterpreter::new();
         intpr.add_known_preimage(wallet.serialize());
         let keys: Vec<String> = wallet.keys.iter().map(|k| k.to_string()).collect();
@@ -201,6 +220,8 @@ impl<T: Transport> BitcoinClient<T> {
         wallet: &WalletPolicy,
         wallet_hmac: Option<&[u8; 32]>,
     ) -> Result<Vec<(usize, PublicKey, EcdsaSig)>, BitcoinClientError<T::Error>> {
+        self.validate_policy(&wallet).await?;
+
         let mut intpr = ClientCommandInterpreter::new();
         intpr.add_known_preimage(wallet.serialize());
         let keys: Vec<String> = wallet.keys.iter().map(|k| k.to_string()).collect();

bitcoin_client_rs/src/client.rs

@@ -18,7 +18,7 @@ use crate::{
     error::BitcoinClientError,
     interpreter::{get_merkleized_map_commitment, ClientCommandInterpreter},
     psbt::*,
-    wallet::WalletPolicy,
+    wallet::{contains_a, WalletPolicy},
 };
 
 /// BitcoinClient calls and interprets commands with the Ledger Device.
@@ -63,6 +63,18 @@ impl<T: Transport> BitcoinClient<T> {
         }
     }
 
+    fn validate_policy(&self, wallet: &WalletPolicy) -> Result<(), BitcoinClientError<T::Error>> {
+        if contains_a(&wallet.descriptor_template) {
+            let (_, version, _) = self.get_version()?;
+            if version == "2.1.0" || version == "2.1.1" {
+                // Versions 2.1.0 and 2.1.1 produced incorrect scripts for policies containing
+                // the `a:` fragment.
+                return Err(BitcoinClientError::UnsupportedAppVersion);
+            }
+        }
+        Ok(())
+    }
+
     /// Returns the currently running app's name, version and state flags
     pub fn get_version(&self) -> Result<(String, String, Vec<u8>), BitcoinClientError<T::Error>> {
         let cmd = command::get_version();
@@ -129,6 +141,8 @@ impl<T: Transport> BitcoinClient<T> {
         &self,
         wallet: &WalletPolicy,
     ) -> Result<([u8; 32], [u8; 32]), BitcoinClientError<T::Error>> {
+        self.validate_policy(&wallet)?;
+
         let cmd = command::register_wallet(wallet);
         let mut intpr = ClientCommandInterpreter::new();
         intpr.add_known_preimage(wallet.serialize());
@@ -162,6 +176,8 @@ impl<T: Transport> BitcoinClient<T> {
         address_index: u32,
         display: bool,
     ) -> Result<bitcoin::Address, BitcoinClientError<T::Error>> {
+        self.validate_policy(&wallet)?;
+
         let mut intpr = ClientCommandInterpreter::new();
         intpr.add_known_preimage(wallet.serialize());
         let keys: Vec<String> = wallet.keys.iter().map(|k| k.to_string()).collect();
@@ -188,6 +204,8 @@ impl<T: Transport> BitcoinClient<T> {
         wallet: &WalletPolicy,
         wallet_hmac: Option<&[u8; 32]>,
     ) -> Result<Vec<(usize, PublicKey, EcdsaSig)>, BitcoinClientError<T::Error>> {
+        self.validate_policy(&wallet)?;
+
         let mut intpr = ClientCommandInterpreter::new();
         intpr.add_known_preimage(wallet.serialize());
         let keys: Vec<String> = wallet.keys.iter().map(|k| k.to_string()).collect();

bitcoin_client_rs/src/error.rs

@@ -9,6 +9,7 @@ pub enum BitcoinClientError<T: Debug> {
     Interpreter(InterpreterError),
     Device { command: u8, status: StatusWord },
     UnexpectedResult { command: u8, data: Vec<u8> },
+    UnsupportedAppVersion,
 }
 
 impl<T: Debug> From<InterpreterError> for BitcoinClientError<T> {

bitcoin_client_rs/src/wallet.rs

@@ -252,6 +252,25 @@ impl core::fmt::Display for WalletPubKey {
     }
 }
 
+/// Returns true iff `descriptor_template` contains a `a:` fragment
+pub fn contains_a(descriptor_template: &str) -> bool {
+    let allowed_chars = ['a', 's', 'c', 't', 'd', 'v', 'j', 'n', 'l', 'u']; // miniscript wrappers
+    let mut sequence = String::new();
+
+    for (i, ch) in descriptor_template.chars().enumerate() {
+        if allowed_chars.contains(&ch) {
+            sequence.push(ch);
+        } else {
+            if ch == ':' && sequence.contains('a') {
+                return true;
+            }
+            sequence.clear();
+        }
+    }
+
+    false
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -307,4 +326,15 @@ mod tests {
         );
         assert_eq!(wallet.serialize().as_slice().to_hex(), "020c436f6c642073746f726167651fb56c3d5542fa09b3956834a9ff6a1df5c36a38e5b02c63c54b41a9a04403b82602516d2c50a89476ecffeec658057f0110674bbfafc18797dc480c7ed53802f3fb");
     }
+
+    #[test]
+    fn test_contains_a() {
+        assert_eq!(contains_a("wsh(pkh(@0))"), false);
+        assert_eq!(
+            contains_a("wsh(c:andor(pk(@0/**),pk_k(@1/**),and_v(v:pk(@2/**),pk_k(@3/**))))"),
+            false
+        );
+        assert_eq!(contains_a("wsh(a:pkh(@0))"), true);
+        assert_eq!(contains_a("wsh(or_i(and_v(v:pkh(@0/**),older(65535)),or_d(multi(2,@1/**,@3/**),and_v(v:thresh(1,pkh(@4/**),a:pkh(@5/**)),older(64231)))))"), true);
+    }
 }