Address Issue #171: Validate Generated Addresses Against Expected Ones in JS Client Library

bitcoin_client_js/package.json

@@ -1,6 +1,6 @@
 {
   "name": "ledger-bitcoin",
-  "version": "0.2.2",
+  "version": "0.2.3",
   "description": "Ledger Hardware Wallet Bitcoin Application Client",
   "main": "build/main/index.js",
   "typings": "build/main/index.d.ts",
@@ -29,9 +29,11 @@
     "node": ">=14"
   },
   "dependencies": {
+    "@bitcoinerlab/descriptors": "^1.0.2",
+    "@bitcoinerlab/secp256k1": "^1.0.5",
     "@ledgerhq/hw-transport": "^6.20.0",
     "bip32-path": "^0.4.2",
-    "bitcoinjs-lib": "^6.0.1"
+    "bitcoinjs-lib": "^6.1.3"
   },
   "devDependencies": {
     "@ledgerhq/hw-transport-node-speculos-http": "^6.24.1",

bitcoin_client_js/src/__tests__/appClient.test.ts

@@ -260,6 +260,51 @@ describe("test AppClient", () => {
     expect(walletHmac.length).toEqual(32);
   });
 
+  //https://wizardsardine.com/blog/ledger-vulnerability-disclosure/
+  it('can generate a correct address or throw on a:X', async () => {
+    for (const template of [
+      'wsh(and_b(pk(@0/**),a:1))',
+      'wsh(and_b(pk(@0/<0;1>/*),a:1))'
+    ]) {
+      try {
+        const walletPolicy = new WalletPolicy('Fixed Vulnerability', template, [
+          "[f5acc2fd/84'/1'/0']tpubDCtKfsNyRhULjZ9XMS4VKKtVcPdVDi8MKUbcSD9MJDyjRu1A2ND5MiipozyyspBT9bg8upEp7a8EAgFxNxXn1d7QkdbL52Ty5jiSLcxPt1P"
+        ]);
+
+        const automation = JSON.parse(
+          fs
+            .readFileSync(
+              'src/__tests__/automations/register_wallet_accept.json'
+            )
+            .toString()
+        );
+        await setSpeculosAutomation(transport, automation);
+
+        const [walletId, walletHmac] = await app.registerWallet(walletPolicy);
+
+        expect(walletId).toEqual(walletPolicy.getId());
+        expect(walletHmac.length).toEqual(32);
+
+        const address = await app.getWalletAddress(
+          walletPolicy,
+          walletHmac,
+          0,
+          0,
+          false
+        );
+        //version > 2.1.1
+        expect(address).toEqual(
+          'tb1q5lyn9807ygs7pc52980mdeuwl9wrq5c8n3kntlhy088h6fqw4gzspw9t9m'
+        );
+      } catch (error) {
+        //version <= 2.1.1
+        expect(error.message).toMatch(
+          /^Third party address validation mismatch/
+        );
+      }
+    }
+  });
+
   it("can register a miniscript wallet", async () => {
     const walletPolicy = new WalletPolicy(
       "Decaying 3-of-3",
@@ -418,4 +463,4 @@ describe("test AppClient", () => {
     const result = await app.signMessage(Buffer.from(msg, "ascii"), path)
     expect(result).toEqual("H4frM6TYm5ty1MAf9o/Zz9Qiy3VEldAYFY91SJ/5nYMAZY1UUB97fiRjKW8mJit2+V4OCa1YCqjDqyFnD9Fw75k=");
   });
-});
+});

bitcoin_client_js/src/lib/appClient.ts

@@ -1,4 +1,8 @@
+import * as descriptors from '@bitcoinerlab/descriptors';
+import * as secp256k1 from '@bitcoinerlab/secp256k1';
+const { Descriptor } = descriptors.DescriptorsFactory(secp256k1);
 import Transport from '@ledgerhq/hw-transport';
+import { networks } from 'bitcoinjs-lib';
 
 import { pathElementsToBuffer, pathStringToArray } from './bip32';
 import { ClientCommandInterpreter } from './clientCommands';
@@ -62,14 +66,6 @@ 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
@@ -184,8 +180,6 @@ export class AppClient {
     walletPolicy: WalletPolicy
   ): Promise<readonly [Buffer, Buffer]> {
 
-    await this.validatePolicy(walletPolicy);
-
     const clientInterpreter = new ClientCommandInterpreter();
 
     clientInterpreter.addKnownWalletPolicy(walletPolicy);
@@ -205,8 +199,20 @@ export class AppClient {
         `Invalid response length. Expected 64 bytes, got ${response.length}`
       );
     }
+    const walletId = response.subarray(0, 32);
+    const walletHMAC = response.subarray(32);
+
+    // sanity check: derive and validate the first address with a 3rd party
+    const firstAddrDevice = await this.getWalletAddress(
+      walletPolicy,
+      walletHMAC,
+      0,
+      0,
+      false
+    );
+    await this.validateAddress(firstAddrDevice, walletPolicy, 0, 0);
 
-    return [response.subarray(0, 32), response.subarray(32)];
+    return [walletId, walletHMAC];
   }
 
   /**
@@ -236,8 +242,6 @@ export class AppClient {
       throw new Error('Invalid HMAC length');
     }
 
-    await this.validatePolicy(walletPolicy);
-
     const clientInterpreter = new ClientCommandInterpreter();
 
     clientInterpreter.addKnownWalletPolicy(walletPolicy);
@@ -257,7 +261,9 @@ export class AppClient {
       clientInterpreter
     );
 
-    return response.toString('ascii');
+    const address = response.toString('ascii');
+    await this.validateAddress(address, walletPolicy, change, addressIndex);
+    return address;
   }
 
   /**
@@ -279,7 +285,6 @@ export class AppClient {
     walletHMAC: Buffer | null,
     progressCallback?: () => void
   ): Promise<[number, PartialSignature][]> {
-    await this.validatePolicy(walletPolicy);
 
     if (typeof psbt === 'string') {
       psbt = Buffer.from(psbt, "base64");
@@ -402,34 +407,75 @@ export class AppClient {
     return result.toString('base64');
   }
 
-  /* Performs any additional checks on the policy before using it.*/
-  private async validatePolicy(walletPolicy: WalletPolicy) {
-    // TODO: Once an independent implementation of miniscript in JavaScript is available,
-    // we will replace the checks in this section with a generic comparison between the
-    // address produced by the app and the one computed locally (like the python and Rust
-    // clients). Until then, we filter for any known bug.
-
-    let appAndVer = undefined;
-
-    if (containsA(walletPolicy.descriptorTemplate)) {
-      appAndVer = 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.")
-      }
+  /* Performs any additional check on the generated address before returning it.*/
+  private async validateAddress(
+    address: string,
+    walletPolicy: WalletPolicy,
+    change: number,
+    addressIndex: number
+  ) {
+    if (change !== 0 && change !== 1)
+      throw new Error('Change can only be 0 or 1');
+    const isChange: boolean = change === 1;
+    if (addressIndex < 0 || !Number.isInteger(addressIndex))
+      throw new Error('Invalid address index');
+    const appAndVer = await this.getAppAndVersion();
+    let network;
+    if (appAndVer.name === 'Bitcoin Test') {
+      network = networks.testnet;
+    } else if (appAndVer.name === 'Bitcoin') {
+      network = networks.bitcoin;
+    } else {
+      throw new Error(
+        `Invalid network: ${appAndVer.name}. Expected 'Bitcoin Test' or 'Bitcoin'.`
+      );
     }
-
-    if (walletPolicy.descriptorTemplate.includes("thresh(1,")) {
-      appAndVer = appAndVer || await this.getAppAndVersion();
-      if (["2.1.0", "2.1.1", "2.1.2"].includes(appAndVer.version)) {
-        // Versions 2.1.0 and 2.1.1 and "2.1.2" produced incorrect scripts for policies
-        // containing an unusual thresh fragment with k = n = 1, that is "thresh(1,X)".
-        // (The check above has false positives, as it also matches "thresh" fragments
-        // where n > 1; however, better to be overzealous).
-        throw new Error("Please update your Ledger Bitcoin app.")
-      }
+    let expression = walletPolicy.descriptorTemplate;
+    // Replace change:
+    expression = expression.replace(/\/\*\*/g, `/<0;1>/*`);
+    const regExpMN = new RegExp(`/<(\\d+);(\\d+)>`, 'g');
+    let matchMN;
+    while ((matchMN = regExpMN.exec(expression)) !== null) {
+      const [M, N] = [parseInt(matchMN[1], 10), parseInt(matchMN[2], 10)];
+      expression = expression.replace(`/<${M};${N}>`, `/${isChange ? N : M}`);
     }
+    // Replace index:
+    expression = expression.replace(/\/\*/g, `/${addressIndex}`);
+    // Replace origin in reverse order to prevent
+    // misreplacements, e.g., @10 being mistaken for @1 and leaving a 0.
+    for (let i = walletPolicy.keys.length - 1; i >= 0; i--)
+      expression = expression.replace(
+        new RegExp(`@${i}`, 'g'),
+        walletPolicy.keys[i]
+      );
+    let thirdPartyValidationApplicable = true;
+    let thirdPartyGeneratedAddress: string;
+    try {
+      thirdPartyGeneratedAddress = new Descriptor({
+        expression,
+        network
+      }).getAddress();
+    } catch (err) {
+      // Note: @bitcoinerlab/descriptors@1.0.x does not support Tapscript yet.
+      // These are the supported descriptors:
+      //  - pkh(KEY)
+      //  - wpkh(KEY)
+      //  - sh(wpkh(KEY))
+      //  - sh(SCRIPT)
+      //  - wsh(SCRIPT)
+      //  - sh(wsh(SCRIPT)), where
+      // SCRIPT is any of the (non-tapscript) fragments in: https://bitcoin.sipa.be/miniscript/
+      //
+      // Other expressions are not supported and third party validation would not be applicable:
+      thirdPartyValidationApplicable = false;
+    }
+    if (
+      thirdPartyValidationApplicable &&
+      address !== thirdPartyGeneratedAddress
+    )
+      throw new Error(
+        `Third party address validation mismatch: ${address} != ${thirdPartyGeneratedAddress}`
+      );
   }
 }