From 0868aa1b9256dc980e603cab6edbadf2554cce19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Luis=20Landabaso=20D=C3=ADaz?= Date: Mon, 17 Jul 2023 11:19:46 +0200 Subject: [PATCH 1/4] Address #171: Compare generated addresses with expected ones --- bitcoin_client_js/package.json | 4 +- .../src/__tests__/appClient.test.ts | 32 +++++++- bitcoin_client_js/src/lib/appClient.ts | 78 ++++++++++++++++--- 3 files changed, 102 insertions(+), 12 deletions(-) diff --git a/bitcoin_client_js/package.json b/bitcoin_client_js/package.json index 2099ada4c..f09a7cf6c 100644 --- a/bitcoin_client_js/package.json +++ b/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", diff --git a/bitcoin_client_js/src/__tests__/appClient.test.ts b/bitcoin_client_js/src/__tests__/appClient.test.ts index 960772d90..53012fbd1 100644 --- a/bitcoin_client_js/src/__tests__/appClient.test.ts +++ b/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="); }); -}); \ No newline at end of file +}); diff --git a/bitcoin_client_js/src/lib/appClient.ts b/bitcoin_client_js/src/lib/appClient.ts index ddfa169b6..172154af4 100644 --- a/bitcoin_client_js/src/lib/appClient.ts +++ b/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 { - 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/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) { + 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; From bd527178127c20740392b07c0acda3c68417dd42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Luis=20Landabaso=20D=C3=ADaz?= Date: Thu, 20 Jul 2023 07:36:45 +0200 Subject: [PATCH 2/4] Addressed review comments: validate addresses at getWalletAddress and end of registerWallet; validateAddress replaces validatePolicy; bumped package version; correctly deal with format (added a test using this format); Fixed incomplete keys in registerWallet test --- bitcoin_client_js/package.json | 2 +- .../src/__tests__/appClient.test.ts | 65 ++++++++------ bitcoin_client_js/src/lib/appClient.ts | 89 ++++++++----------- 3 files changed, 79 insertions(+), 77 deletions(-) diff --git a/bitcoin_client_js/package.json b/bitcoin_client_js/package.json index f09a7cf6c..362df99f0 100644 --- a/bitcoin_client_js/package.json +++ b/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", diff --git a/bitcoin_client_js/src/__tests__/appClient.test.ts b/bitcoin_client_js/src/__tests__/appClient.test.ts index 53012fbd1..a1784646b 100644 --- a/bitcoin_client_js/src/__tests__/appClient.test.ts +++ b/bitcoin_client_js/src/__tests__/appClient.test.ts @@ -262,31 +262,46 @@ describe("test AppClient", () => { //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/); + 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/ + ); + } } }); diff --git a/bitcoin_client_js/src/lib/appClient.ts b/bitcoin_client_js/src/lib/appClient.ts index 172154af4..763d9bbef 100644 --- a/bitcoin_client_js/src/lib/appClient.ts +++ b/bitcoin_client_js/src/lib/appClient.ts @@ -66,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 @@ -207,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]; } /** @@ -412,8 +416,9 @@ export class AppClient { ) { 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'); + throw new Error('Invalid address index'); const appAndVer = await this.getAppAndVersion(); let network; if (appAndVer.name === 'Bitcoin Test') { @@ -426,12 +431,22 @@ export class AppClient { ); } 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); + // 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: + for (let i = 0; i < walletPolicy.keys.length; i++) + expression = expression.replace( + new RegExp(`@${i}`, 'g'), + walletPolicy.keys[i] + ); let thirdPartyValidationApplicable = true; let thirdPartyGeneratedAddress: string; try { @@ -439,7 +454,7 @@ export class AppClient { expression, network }).getAddress(); - } catch(err) { + } catch (err) { // Note: @bitcoinerlab/descriptors@1.0.x does not support Tapscript yet. // These are the supported descriptors: // - pkh(KEY) @@ -453,41 +468,13 @@ export class AppClient { // 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) { - - 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.") - } - } - - 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.") - } - } + if ( + thirdPartyValidationApplicable && + address !== thirdPartyGeneratedAddress + ) + throw new Error( + `Third party address validation mismatch: ${address} != ${thirdPartyGeneratedAddress}` + ); } } From 3e79af496ef2c104d2fdaa535ad695432ecc9c07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Luis=20Landabaso=20D=C3=ADaz?= Date: Thu, 20 Jul 2023 08:07:55 +0200 Subject: [PATCH 3/4] Fix issue with key replacement in address derivation Revised the key replacement logic in the `validateAddress` function to prevent misinterpretation of key indices. The loop now iterates in reverse order to avoid scenarios where, for example, @10 is mistakenly replaced as @1, leaving an extra 0. This change ensures that the correct keys are replaced when deriving the wallet address. --- bitcoin_client_js/src/lib/appClient.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/bitcoin_client_js/src/lib/appClient.ts b/bitcoin_client_js/src/lib/appClient.ts index 763d9bbef..e4d52ce23 100644 --- a/bitcoin_client_js/src/lib/appClient.ts +++ b/bitcoin_client_js/src/lib/appClient.ts @@ -441,8 +441,9 @@ export class AppClient { } // Replace index: expression = expression.replace(/\/\*/g, `/${addressIndex}`); - // Replace origin: - for (let i = 0; i < walletPolicy.keys.length; i++) + // 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] From 89ab75164584aced526498c011e1d29b16e59f10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Luis=20Landabaso=20D=C3=ADaz?= Date: Wed, 26 Jul 2023 22:45:38 +0200 Subject: [PATCH 4/4] Fix typo --- bitcoin_client_js/src/lib/appClient.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bitcoin_client_js/src/lib/appClient.ts b/bitcoin_client_js/src/lib/appClient.ts index e4d52ce23..bb369779f 100644 --- a/bitcoin_client_js/src/lib/appClient.ts +++ b/bitcoin_client_js/src/lib/appClient.ts @@ -407,7 +407,7 @@ export class AppClient { return result.toString('base64'); } - /* Performs any additional check on the genetated address before returning it.*/ + /* Performs any additional check on the generated address before returning it.*/ private async validateAddress( address: string, walletPolicy: WalletPolicy,