From d9b7022832ed38d8661b842add34caa97f340d31 Mon Sep 17 00:00:00 2001 From: Rushil Kapoor Date: Wed, 28 Jun 2023 11:58:29 +0530 Subject: [PATCH 1/2] feat(abstract-utxo): always use bitcoin network for validating secondary key signatures Key signatures are always generated with the bitcoin network because it is not always known which coin they will be used with. Without this change altcoin signatures would fail verification in some contexts. BG-79423 --- modules/abstract-utxo/src/abstractUtxoCoin.ts | 8 ++++++-- .../test/v2/unit/coins/abstractUtxoCoin.ts | 19 +++++++++++++++++-- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/modules/abstract-utxo/src/abstractUtxoCoin.ts b/modules/abstract-utxo/src/abstractUtxoCoin.ts index 833aa8ac68..522c212b31 100644 --- a/modules/abstract-utxo/src/abstractUtxoCoin.ts +++ b/modules/abstract-utxo/src/abstractUtxoCoin.ts @@ -102,6 +102,7 @@ const { getExternalChainCode, isChainCode, scriptTypeForChain, outputScripts } = type Unspent = bitgo.Unspent; type RootWalletKeys = bitgo.RootWalletKeys; + export interface VerifyAddressOptions extends BaseVerifyAddressOptions { chain: number; index: number; @@ -677,7 +678,7 @@ export abstract class AbstractUtxoCoin extends BaseCoin { * @param {VerifyKeySignaturesOptions} params * @return {{backup: boolean, bitgo: boolean}} */ - protected verifyKeySignature(params: VerifyKeySignaturesOptions): boolean { + public verifyKeySignature(params: VerifyKeySignaturesOptions): boolean { // first, let's verify the integrity of the user key, whose public key is used for subsequent verifications const { userKeychain, keychainToVerify, keySignature } = params; if (!userKeychain) { @@ -695,10 +696,13 @@ export abstract class AbstractUtxoCoin extends BaseCoin { // verify the signature against the user public key assert(userKeychain.pub); const publicKey = bip32.fromBase58(userKeychain.pub).publicKey; + // Due to interface of `bitcoinMessage`, we need to convert the public key to an address. + // Note that this address has no relationship to on-chain transactions. We are + // only interested in the address as a representation of the public key. const signingAddress = utxolib.address.toBase58Check( utxolib.crypto.hash160(publicKey), utxolib.networks.bitcoin.pubKeyHash, - this.network + utxolib.networks.bitcoin ); // BG-5703: use BTC mainnet prefix for all key signature operations diff --git a/modules/bitgo/test/v2/unit/coins/abstractUtxoCoin.ts b/modules/bitgo/test/v2/unit/coins/abstractUtxoCoin.ts index edbdfc6949..4ff7b7ecd7 100644 --- a/modules/bitgo/test/v2/unit/coins/abstractUtxoCoin.ts +++ b/modules/bitgo/test/v2/unit/coins/abstractUtxoCoin.ts @@ -1,7 +1,7 @@ import * as utxolib from '@bitgo/utxo-lib'; import * as should from 'should'; import * as sinon from 'sinon'; -import { UnexpectedAddressError, VerificationOptions } from '@bitgo/sdk-core'; +import { Keychain, UnexpectedAddressError, VerificationOptions } from '@bitgo/sdk-core'; import { TestBitGo } from '@bitgo/sdk-test'; import { BitGo } from '../../../../src/bitgo'; import { @@ -261,7 +261,8 @@ describe('Abstract UTXO Coin:', () => { }; // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const sign = async (key, keychain) => (await coin.signMessage(keychain, key.pub!)).toString('hex'); + const sign = async (key, keychain, coinToSignFor = coin) => + (await coinToSignFor.signMessage(keychain, key.pub!)).toString('hex'); const signUser = (key) => sign(key, userKeychain); const signOther = (key) => sign(key, otherKeychain); const passphrase = 'test_passphrase'; @@ -581,5 +582,19 @@ describe('Abstract UTXO Coin:', () => { coinMock.restore(); bitcoinMock.restore(); }); + + it('should verify key signature of ZEC', async () => { + const zecCoin = bitgo.coin('tzec') as AbstractUtxoCoin; + const userKeychain = await zecCoin.keychains().create(); + const otherKeychain = await zecCoin.keychains().create(); + + await zecCoin + .verifyKeySignature({ + userKeychain: userKeychain as unknown as Keychain, + keychainToVerify: otherKeychain as unknown as Keychain, + keySignature: await sign(userKeychain, otherKeychain, zecCoin), + }) + .should.be.true(); + }); }); }); From b9fa5c57ae6207974612c96c4cf0941665703a4e Mon Sep 17 00:00:00 2001 From: Rushil Kapoor Date: Fri, 30 Jun 2023 10:24:28 +0530 Subject: [PATCH 2/2] feat(abstract-utxo): move keysSignatures test to separate file Loop over all utxo coins BG-79423 --- modules/abstract-utxo/src/abstractUtxoCoin.ts | 29 +++++++++++++---- .../test/v2/unit/coins/abstractUtxoCoin.ts | 19 ++---------- .../test/v2/unit/coins/utxo/keySignatures.ts | 31 +++++++++++++++++++ .../sdk-core/src/bitgo/baseCoin/baseCoin.ts | 22 +++++++++++++ .../sdk-core/src/bitgo/baseCoin/iBaseCoin.ts | 8 +++++ 5 files changed, 86 insertions(+), 23 deletions(-) create mode 100644 modules/bitgo/test/v2/unit/coins/utxo/keySignatures.ts diff --git a/modules/abstract-utxo/src/abstractUtxoCoin.ts b/modules/abstract-utxo/src/abstractUtxoCoin.ts index 522c212b31..34c9613d56 100644 --- a/modules/abstract-utxo/src/abstractUtxoCoin.ts +++ b/modules/abstract-utxo/src/abstractUtxoCoin.ts @@ -281,9 +281,9 @@ export interface RecoverFromWrongChainOptions { } export interface VerifyKeySignaturesOptions { - userKeychain?: Keychain; - keychainToVerify?: Keychain; - keySignature?: string; + userKeychain: { pub?: string }; + keychainToVerify: { pub?: string }; + keySignature: string; } export interface VerifyUserPublicKeyOptions { @@ -702,6 +702,9 @@ export abstract class AbstractUtxoCoin extends BaseCoin { const signingAddress = utxolib.address.toBase58Check( utxolib.crypto.hash160(publicKey), utxolib.networks.bitcoin.pubKeyHash, + // we do not pass `this.network` here because it would fail for zcash + // the bitcoinMessage library decodes the address and throws away the first byte + // because zcash has a two-byte prefix, verify() decodes zcash addresses to an invalid pubkey hash utxolib.networks.bitcoin ); @@ -744,7 +747,13 @@ export abstract class AbstractUtxoCoin extends BaseCoin { if (!keySignature) { throw new Error(`missing required custom change ${KeyIndices[keyIndex].toLowerCase()} keychain signature`); } - if (!this.verifyKeySignature({ userKeychain, keychainToVerify, keySignature })) { + if ( + !this.verifyKeySignature({ + userKeychain: userKeychain as { pub: string }, + keychainToVerify: keychainToVerify as { pub: string }, + keySignature, + }) + ) { debug('failed to verify custom change %s key signature!', KeyIndices[keyIndex].toLowerCase()); return false; } @@ -814,8 +823,16 @@ export abstract class AbstractUtxoCoin extends BaseCoin { // let's verify these keychains const keySignatures = parsedTransaction.keySignatures; if (!_.isEmpty(keySignatures)) { - const verify = (key, pub) => - this.verifyKeySignature({ userKeychain: keychains.user, keychainToVerify: key, keySignature: pub }); + const verify = (key, pub) => { + if (!keychains.user || !keychains.user.pub) { + throw new Error('missing user keychain'); + } + return this.verifyKeySignature({ + userKeychain: keychains.user as { pub: string }, + keychainToVerify: key, + keySignature: pub, + }); + }; const isBackupKeySignatureValid = verify(keychains.backup, keySignatures.backupPub); const isBitgoKeySignatureValid = verify(keychains.bitgo, keySignatures.bitgoPub); if (!isBackupKeySignatureValid || !isBitgoKeySignatureValid) { diff --git a/modules/bitgo/test/v2/unit/coins/abstractUtxoCoin.ts b/modules/bitgo/test/v2/unit/coins/abstractUtxoCoin.ts index 4ff7b7ecd7..edbdfc6949 100644 --- a/modules/bitgo/test/v2/unit/coins/abstractUtxoCoin.ts +++ b/modules/bitgo/test/v2/unit/coins/abstractUtxoCoin.ts @@ -1,7 +1,7 @@ import * as utxolib from '@bitgo/utxo-lib'; import * as should from 'should'; import * as sinon from 'sinon'; -import { Keychain, UnexpectedAddressError, VerificationOptions } from '@bitgo/sdk-core'; +import { UnexpectedAddressError, VerificationOptions } from '@bitgo/sdk-core'; import { TestBitGo } from '@bitgo/sdk-test'; import { BitGo } from '../../../../src/bitgo'; import { @@ -261,8 +261,7 @@ describe('Abstract UTXO Coin:', () => { }; // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const sign = async (key, keychain, coinToSignFor = coin) => - (await coinToSignFor.signMessage(keychain, key.pub!)).toString('hex'); + const sign = async (key, keychain) => (await coin.signMessage(keychain, key.pub!)).toString('hex'); const signUser = (key) => sign(key, userKeychain); const signOther = (key) => sign(key, otherKeychain); const passphrase = 'test_passphrase'; @@ -582,19 +581,5 @@ describe('Abstract UTXO Coin:', () => { coinMock.restore(); bitcoinMock.restore(); }); - - it('should verify key signature of ZEC', async () => { - const zecCoin = bitgo.coin('tzec') as AbstractUtxoCoin; - const userKeychain = await zecCoin.keychains().create(); - const otherKeychain = await zecCoin.keychains().create(); - - await zecCoin - .verifyKeySignature({ - userKeychain: userKeychain as unknown as Keychain, - keychainToVerify: otherKeychain as unknown as Keychain, - keySignature: await sign(userKeychain, otherKeychain, zecCoin), - }) - .should.be.true(); - }); }); }); diff --git a/modules/bitgo/test/v2/unit/coins/utxo/keySignatures.ts b/modules/bitgo/test/v2/unit/coins/utxo/keySignatures.ts new file mode 100644 index 0000000000..4c03cc2dd6 --- /dev/null +++ b/modules/bitgo/test/v2/unit/coins/utxo/keySignatures.ts @@ -0,0 +1,31 @@ +import * as assert from 'assert'; +import { AbstractUtxoCoin } from '@bitgo/abstract-utxo'; +import { Keychain } from '@bitgo/sdk-core'; + +import { utxoCoins } from './util'; + +function describeWithCoin(coin: AbstractUtxoCoin) { + describe(`verifyKeySignatures for ${coin.getChain()}`, function () { + it('should verify key signature of ZEC', async () => { + const userKeychain = await coin.keychains().create(); + const backupKeychain = await coin.keychains().create(); + const bitgoKeychain = await coin.keychains().create(); + + const signatures = await coin.createKeySignatures( + userKeychain.prv, + { pub: backupKeychain.pub as string }, + { pub: bitgoKeychain.pub as string } + ); + + assert.ok( + await coin.verifyKeySignature({ + userKeychain: userKeychain as unknown as Keychain, + keychainToVerify: backupKeychain as unknown as Keychain, + keySignature: signatures.backup, + }) + ); + }); + }); +} + +utxoCoins.forEach((coin) => describeWithCoin(coin)); diff --git a/modules/sdk-core/src/bitgo/baseCoin/baseCoin.ts b/modules/sdk-core/src/bitgo/baseCoin/baseCoin.ts index 420f4473a0..466023d1df 100644 --- a/modules/sdk-core/src/bitgo/baseCoin/baseCoin.ts +++ b/modules/sdk-core/src/bitgo/baseCoin/baseCoin.ts @@ -227,6 +227,28 @@ export abstract class BaseCoin implements IBaseCoin { return signMessage(message, bip32.fromBase58(key.prv), utxolib.networks.bitcoin); } + /** + * Create signatures for the backup and bitgo keys using the user key. + * We can verify the signatures when fetching the keys from wallet-platform later. + * Currently only `AbstractUtxoCoin` implements and uses the complementary `verifyKeySignature` method. + * @param prv - the user private key + * @param backupKeychain - contains the backup public key + * @param bitgoKeychain - contains the bitgo public key + */ + public async createKeySignatures( + prv: string, + backupKeychain: { pub: string }, + bitgoKeychain: { pub: string } + ): Promise<{ + backup: string; + bitgo: string; + }> { + return { + backup: (await this.signMessage({ prv }, backupKeychain.pub)).toString('hex'), + bitgo: (await this.signMessage({ prv }, bitgoKeychain.pub)).toString('hex'), + }; + } + /** * Decompose a raw transaction into useful information. * @param options - coin-specific diff --git a/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts b/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts index 0ed4f37c38..23b3a713fa 100644 --- a/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts +++ b/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts @@ -438,6 +438,14 @@ export interface IBaseCoin { baseUnitsToBigUnits(baseUnits: string | number): string; bigUnitsToBaseUnits(bigUnits: string | number): string; signMessage(key: { prv: string }, message: string): Promise; + createKeySignatures( + prv: string, + backupKeychain: { pub: string }, + bitgoKeychain: { pub: string } + ): Promise<{ + backup: string; + bitgo: string; + }>; explainTransaction(options: Record): Promise | undefined>; verifyTransaction(params: VerifyTransactionOptions): Promise; verifyAddress(params: VerifyAddressOptions): Promise;