From 48b0fdbc29e8de8cd6c22d4faf638bbc463d53a4 Mon Sep 17 00:00:00 2001 From: Aaron Cox Date: Mon, 16 Jan 2023 19:08:29 -0500 Subject: [PATCH] Ensure all signatures returned by beforeSign hooks are valid This can be disabled with the `validatePluginSignatures` option, it is enabled by default to ensure that multiple mutations in the `beforeSign` hooks don't invalidate any signatures they may create. --- src/session.ts | 55 ++++++++++++++++- src/transact.ts | 3 + test/tests/transact.ts | 135 ++++++++++++++++++++++++++++++++++++++++- 3 files changed, 190 insertions(+), 3 deletions(-) diff --git a/src/session.ts b/src/session.ts index 09173a87..086a4a27 100644 --- a/src/session.ts +++ b/src/session.ts @@ -5,6 +5,7 @@ import { NameType, PermissionLevel, PermissionLevelType, + Serializer, Signature, SignedTransaction, } from '@greymass/eosio' @@ -77,6 +78,7 @@ export interface SessionOptions { permissionLevel?: PermissionLevelType | string transactPlugins?: AbstractTransactPlugin[] transactPluginsOptions?: TransactPluginsOptions + validatePluginSignatures?: boolean walletPlugin: WalletPlugin } @@ -90,6 +92,7 @@ export class Session { readonly permissionLevel: PermissionLevel readonly transactPlugins: TransactPlugin[] readonly transactPluginsOptions: TransactPluginsOptions = {} + readonly validatePluginSignatures: boolean = true readonly wallet: WalletPlugin constructor(options: SessionOptions) { @@ -125,6 +128,9 @@ export class Session { 'Either a permissionLevel or actor/permission must be provided when creating a new Session.' ) } + if (options.validatePluginSignatures !== undefined) { + this.validatePluginSignatures = options.validatePluginSignatures + } this.wallet = options.walletPlugin } @@ -292,6 +298,7 @@ export class Session { // Create response template to this transact call const result: TransactResult = { chain: this.chain, + keys: [], request, resolved: undefined, revisions: new TransactRevisions(request), @@ -314,6 +321,12 @@ export class Session { const willBroadcast = options && typeof options.broadcast !== 'undefined' ? options.broadcast : this.broadcast + // Whether all signatures generated + const willValidatePluginSignatures = + options && typeof options.validatePluginSignatures !== 'undefined' + ? options.validatePluginSignatures + : this.validatePluginSignatures + // Run the `beforeSign` hooks for (const hook of context.hooks.beforeSign) { // Get the response of the hook by passing a clonied request. @@ -326,12 +339,29 @@ export class Session { if (allowModify) { request = await this.updateRequest(request, response.request, abiCache) } - // If signatures were returned, append them - if (response.signatures) { + + // If signatures were returned, record them in the response. + if (response.signatures?.length) { + // Merge new signatures alongside existing signatures into the TransactResult. result.signatures = [...result.signatures, ...response.signatures] + + // Recover the keys used to generate the signatures at the time of the request. + const recoveredKeys = response.signatures.map((signature) => { + const requestTransaction = request.getRawTransaction() + const requestDigest = requestTransaction.signingDigest(this.chain.id) + return signature.recoverDigest(requestDigest) + }) + + // Merge newly discovered keys into the TransactResult. + result.keys = [...result.keys, ...recoveredKeys] } } + // Validate all the signatures returned by the plugins against the current request + if (willValidatePluginSignatures) { + this.validateBeforeSignSignatures(context, request, result) + } + // Resolve the SigningRequest and assign it to the TransactResult result.request = request result.resolved = await context.resolve(request, expireSeconds) @@ -362,5 +392,26 @@ export class Session { return result } + validateBeforeSignSignatures( + context: TransactContext, + request: SigningRequest, + result: TransactResult + ): void { + const requestTransaction = request.getRawTransaction() + const requestDigest = requestTransaction.signingDigest(this.chain.id) + const publicKeys = Serializer.objectify(result.keys) + result.signatures.forEach((signature) => { + const recoveredKey = signature.recoverDigest(requestDigest) + const verified = signature.verifyDigest(requestDigest, recoveredKey) + if (!verified || !publicKeys.includes(String(recoveredKey))) { + throw new Error( + `A signature (${signature}) provided by a beforeSign hook using ` + + `a key (${recoveredKey}) has been invalidated, likely due to the transaction ` + + `being modified by another hook after the signature was created. To disable ` + + `this error, set validatePluginSignatures equal to false.` + ) + } + }) + } } export {AbstractTransactPlugin} diff --git a/src/transact.ts b/src/transact.ts index 381ae17b..2bc5e491 100644 --- a/src/transact.ts +++ b/src/transact.ts @@ -5,6 +5,7 @@ import { Checksum256Type, Name, PermissionLevel, + PublicKey, Serializer, Signature, } from '@greymass/eosio' @@ -226,6 +227,8 @@ export class TransactRevisions { export interface TransactResult { /** The chain that was used. */ chain: ChainDefinition + /** The public keys used to sign. */ + keys: PublicKey[] /** The SigningRequest representation of the transaction. */ request: SigningRequest /** The ResolvedSigningRequest of the transaction */ diff --git a/test/tests/transact.ts b/test/tests/transact.ts index fff8df0e..772b8887 100644 --- a/test/tests/transact.ts +++ b/test/tests/transact.ts @@ -1,7 +1,18 @@ import {assert} from 'chai' import zlib from 'pako' -import {Action, Name, PermissionLevel, Serializer, Signature, TimePointSec} from '@greymass/eosio' +import { + Action, + Name, + PermissionLevel, + PrivateKey, + Serializer, + Signature, + SignatureType, + Struct, + TimePointSec, + Transaction, +} from '@greymass/eosio' import {ResolvedSigningRequest, SigningRequest} from 'eosio-signing-request' import SessionKit, { @@ -321,6 +332,128 @@ suite('transact', function () { ) }) }) + suite('validatePluginSignatures', function () { + test('default: true (check and throw after mutations invalidate signatures)', async function () { + const {action, session} = await mockData() + const randomKeyCosignerHook = async ( + request: SigningRequest, + context: TransactContext + ) => { + const randomName = Name.from('') + const randomKey = PrivateKey.generate('K1') + class noop extends Struct { + static abiName = 'noop' + static abiFields = [] + } + const newAction = Action.from({ + account: 'greymassnoop', + name: 'noop', + authorization: [ + { + actor: randomName, + permission: 'cosign', + }, + ], + data: noop.from({}), + }) + const info = await context.client.v1.chain.get_info() + const header = info.getTransactionHeader() + const newTransaction = Transaction.from({ + ...header, + actions: [newAction, ...request.getRawActions()], + }) + const newRequest = await SigningRequest.create( + {transaction: newTransaction, chainId: session.chain.id}, + context.esrOptions + ) + const digest = newTransaction.signingDigest(info.chain_id) + const signature: SignatureType = randomKey.signDigest(digest) + return { + request: newRequest, + signatures: [signature], + } + } + const debugPlugin = { + register(context) { + context.addHook(TransactHookTypes.beforeSign, randomKeyCosignerHook) + context.addHook(TransactHookTypes.beforeSign, randomKeyCosignerHook) + }, + } + let error + await session + .transact( + {action}, + { + broadcast: false, + transactPlugins: [debugPlugin], + } + ) + .catch((e) => { + error = e + }) + assert.instanceOf( + error, + Error, + 'Expected an Error to be thrown by validatePluginSignatures.' + ) + }) + test('override: false', async function () { + const {action, session} = await mockData() + const randomKeyCosignerHook = async ( + request: SigningRequest, + context: TransactContext + ) => { + const randomName = Name.from('') + const randomKey = PrivateKey.generate('K1') + class noop extends Struct { + static abiName = 'noop' + static abiFields = [] + } + const newAction = Action.from({ + account: 'greymassnoop', + name: 'noop', + authorization: [ + { + actor: randomName, + permission: 'cosign', + }, + ], + data: noop.from({}), + }) + const info = await context.client.v1.chain.get_info() + const header = info.getTransactionHeader() + const newTransaction = Transaction.from({ + ...header, + actions: [newAction, ...request.getRawActions()], + }) + const newRequest = await SigningRequest.create( + {transaction: newTransaction, chainId: session.chain.id}, + context.esrOptions + ) + const digest = newTransaction.signingDigest(info.chain_id) + const signature: SignatureType = randomKey.signDigest(digest) + return { + request: newRequest, + signatures: [signature], + } + } + const debugPlugin = { + register(context) { + context.addHook(TransactHookTypes.beforeSign, randomKeyCosignerHook) + context.addHook(TransactHookTypes.beforeSign, randomKeyCosignerHook) + }, + } + const result = await session.transact( + {action}, + { + broadcast: false, + transactPlugins: [debugPlugin], + validatePluginSignatures: false, + } + ) + assetValidTransactResponse(result) + }) + }) suite('transactPlugins', function () { test('inherit', async function () { const {action} = await mockData()