Address LedgerHQ#171: Compare generated addresses with expected ones

Author: landabaso

bitcoin_client_js/package.json

@@ -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,36 @@ 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 () => {
+    try {
+      const walletPolicy = new WalletPolicy('Fixed Vulnerability', 'wsh(and_b(pk(@0/**),a:1))', [
+        "[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 +448,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';
@@ -184,8 +188,6 @@ export class AppClient {
     walletPolicy: WalletPolicy
   ): Promise<readonly [Buffer, Buffer]> {
 
-    await this.validatePolicy(walletPolicy);
-
     const clientInterpreter = new ClientCommandInterpreter();
 
     clientInterpreter.addKnownWalletPolicy(walletPolicy);
@@ -236,8 +238,6 @@ export class AppClient {
       throw new Error('Invalid HMAC length');
     }
 
-    await this.validatePolicy(walletPolicy);
-
     const clientInterpreter = new ClientCommandInterpreter();
 
     clientInterpreter.addKnownWalletPolicy(walletPolicy);
@@ -257,7 +257,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 +281,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,12 +403,69 @@ export class AppClient {
     return result.toString('base64');
   }
 
+  /* Performs any additional check on the genetated 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');
+    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'.`
+      );
+    }
+    let expression = walletPolicy.descriptorTemplate;
+    for (let i = 0; i < walletPolicy.keys.length; i++) {
+      const keyPath = walletPolicy.keys[i] + '/' + change + '/' + addressIndex;
+      expression = expression
+        .replace('@' + i + '/**', keyPath)
+        .replace('@' + i + '/<0;1>', keyPath);
+    }
+    let thirdPartyValidationApplicable = true;
+    let thirdPartyGeneratedAddress: string;
+    try {
+      thirdPartyGeneratedAddress = new Descriptor({
+        expression,
+        network
+      }).getAddress();
+    } catch(err) {
+      // Note: @bitcoinerlab/[email protected] 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) {
+      if (address !== thirdPartyGeneratedAddress) {
+        throw new Error(
+          `Third party address validation mismatch: ${address} != ${thirdPartyGeneratedAddress}`
+        );
+      }
+    } else {
+      await this.validatePolicy(walletPolicy);
+    }
+  }
+
   /* 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;