From 081c90da48177d79b46ff0ba9a65a3c6d1b2f9a7 Mon Sep 17 00:00:00 2001 From: yashaanand Date: Mon, 28 Sep 2026 13:03:13 -0400 Subject: [PATCH 1/2] feat(root): add TSS EdDSA key verification to SDK and Express Adds a boolean key-verification path so a caller that holds its own TSS user signing material can learn up front whether it belongs to a wallet, instead of discovering a mismatch only when a signature fails, and exposes it as an Express route so REST callers get the same answer. - sdk-core: pure helpers in utils/tss/keyVerification parse and shape-check user signing material with constant, non-leaking errors (the shape check keeps malformed input away from the combine routine, whose raw errors interpolate submitter-controlled fields) and recombine it to compare against a commonKeychain - sdk-core: ITssUtils gains verifyKey and supportsVerifyKey; the base implementation reports unsupported and EddsaUtils implements the EdDSA MPCv1 check, so ECDSA/MPCv2/on-chain wallets reject without a keychain request; TypeScript consumers implementing ITssUtils may need to add the new members; Wallet.verifyKey fetches only the user keychain and returns the result object verbatim for the Express response body; walletPassphrase is deliberately not accepted - express: typed route and handler for POST /api/v2/:coin/wallet/:id/verifyKey, with 400s for unsupported wallet types and malformed material; errors with an HTTP status or a transport-level code keep their semantics instead of being masked as a 400; the decode-error formatter now redacts structured values so key material sent with the wrong field type cannot be echoed back; the corresponding server-side mirror route is a follow-up in the server repo Ticket: WAL-1868 --- modules/express/src/clientRoutes.ts | 36 ++- modules/express/src/sensitiveRequestKeys.ts | 13 + modules/express/src/typedRoutes/api/index.ts | 9 + .../src/typedRoutes/api/openapi-index.ts | 4 + .../src/typedRoutes/api/v2/verifyKey.ts | 55 +++++ modules/express/src/typedRoutes/utils.ts | 11 +- .../typedRoutes/formatValidationErrors.ts | 21 ++ .../test/unit/typedRoutes/verifyKey.ts | 165 +++++++++++++ .../src/bitgo/utils/tss/baseTSSUtils.ts | 13 + .../sdk-core/src/bitgo/utils/tss/baseTypes.ts | 2 + .../src/bitgo/utils/tss/eddsa/eddsa.ts | 9 + modules/sdk-core/src/bitgo/utils/tss/index.ts | 1 + .../src/bitgo/utils/tss/keyVerification.ts | 159 +++++++++++++ modules/sdk-core/src/bitgo/wallet/iWallet.ts | 15 ++ modules/sdk-core/src/bitgo/wallet/wallet.ts | 37 +++ .../unit/bitgo/utils/tss/keyVerification.ts | 223 ++++++++++++++++++ .../test/unit/bitgo/wallet/walletVerifyKey.ts | 158 +++++++++++++ 17 files changed, 920 insertions(+), 11 deletions(-) create mode 100644 modules/express/src/sensitiveRequestKeys.ts create mode 100644 modules/express/src/typedRoutes/api/v2/verifyKey.ts create mode 100644 modules/express/test/unit/typedRoutes/verifyKey.ts create mode 100644 modules/sdk-core/src/bitgo/utils/tss/keyVerification.ts create mode 100644 modules/sdk-core/test/unit/bitgo/utils/tss/keyVerification.ts create mode 100644 modules/sdk-core/test/unit/bitgo/wallet/walletVerifyKey.ts diff --git a/modules/express/src/clientRoutes.ts b/modules/express/src/clientRoutes.ts index 3afd7a5efa0..63cc7f3b7a7 100755 --- a/modules/express/src/clientRoutes.ts +++ b/modules/express/src/clientRoutes.ts @@ -56,6 +56,7 @@ import { Config } from './config'; import { ApiResponseError, BitGoExpressError } from './errors'; import { promises as fs } from 'fs'; import { retryPromise } from './retryPromise'; +import { SENSITIVE_REQUEST_KEYS } from './sensitiveRequestKeys'; import { handleCreateSignerMacaroon, handleGetLightningWalletState, @@ -845,6 +846,30 @@ export async function handleV2CreateAddress(req: ExpressApiRouteRequest<'express return result; } +/** + * handle v2 verifyKey - verify that user-held TSS key material belongs to a wallet + * @param req + */ +export async function handleV2VerifyKey(req: ExpressApiRouteRequest<'express.v2.wallet.verifyKey', 'post'>) { + const coin = req.bitgo.coin(req.decoded.coin); + const wallet = await coin.wallets().get({ id: req.decoded.id }); + try { + return await wallet.verifyKey({ prv: req.decoded.prv }); + } catch (e) { + // errors with a meaningful HTTP status (e.g. the keychain fetch inside wallet.verifyKey) + // surface it instead of being masked as a 400 + if (e instanceof Error && typeof (e as ApiResponseError).status === 'number') { + throw e; + } + // transport-level failures (DNS, connection refused, TLS) carry a system `code` instead; + // they are infrastructure errors, not malformed input + if (e instanceof Error && typeof (e as NodeJS.ErrnoException).code === 'string') { + throw e; + } + throw new ApiResponseError(e instanceof Error ? e.message : String(e), 400); + } +} + /** * handle v2 isWalletAddress - verify if an address belongs to a wallet * @param req @@ -1751,16 +1776,6 @@ interface RequestHandler extends express.RequestHandler; } -const SENSITIVE_REQUEST_KEYS = new Set([ - 'password', - 'passphrase', - 'walletpassphrase', - 'prv', - 'privatekey', - 'encryptedprv', - 'secret', -]); - function collectSensitiveRequestValues(value: unknown, values = new Set()): Set { if (Array.isArray(value)) { value.forEach((item) => collectSensitiveRequestValues(item, values)); @@ -2171,6 +2186,7 @@ export function setupAPIRoutes(app: express.Application, config: Config): void { ]); router.post('express.v2.wallet.createAddress', [prepareBitGo(config), typedPromiseWrapper(handleV2CreateAddress)]); + router.post('express.v2.wallet.verifyKey', [prepareBitGo(config), typedPromiseWrapper(handleV2VerifyKey)]); router.post('express.v2.wallet.isWalletAddress', [ prepareBitGo(config), typedPromiseWrapper(handleV2IsWalletAddress), diff --git a/modules/express/src/sensitiveRequestKeys.ts b/modules/express/src/sensitiveRequestKeys.ts new file mode 100644 index 00000000000..b9b4598dab1 --- /dev/null +++ b/modules/express/src/sensitiveRequestKeys.ts @@ -0,0 +1,13 @@ +/** + * Request body keys whose values must never be logged or echoed back to a caller. + * Compared lowercased against incoming keys. + */ +export const SENSITIVE_REQUEST_KEYS = new Set([ + 'password', + 'passphrase', + 'walletpassphrase', + 'prv', + 'privatekey', + 'encryptedprv', + 'secret', +]); diff --git a/modules/express/src/typedRoutes/api/index.ts b/modules/express/src/typedRoutes/api/index.ts index 5df558606aa..6855227dcde 100644 --- a/modules/express/src/typedRoutes/api/index.ts +++ b/modules/express/src/typedRoutes/api/index.ts @@ -31,6 +31,7 @@ import { PostCreateLocalKeyChain } from './v1/createLocalKeyChain'; import { PutConstructPendingApprovalTx } from './v1/constructPendingApprovalTx'; import { PutConsolidateUnspents } from './v1/consolidateUnspents'; import { PostCreateAddress } from './v2/createAddress'; +import { PostVerifyKey } from './v2/verifyKey'; import { PutFanoutUnspents } from './v1/fanoutUnspents'; import { PostOfcSignPayload } from './v2/ofcSignPayload'; import { PostWalletRecoverToken } from './v2/walletRecoverToken'; @@ -233,6 +234,12 @@ export const ExpressV2WalletCreateAddressApiSpec = apiSpec({ }, }); +export const ExpressV2WalletVerifyKeyApiSpec = apiSpec({ + 'express.v2.wallet.verifyKey': { + post: PostVerifyKey, + }, +}); + export const ExpressV2WalletIsWalletAddressApiSpec = apiSpec({ 'express.v2.wallet.isWalletAddress': { post: PostIsWalletAddress, @@ -420,6 +427,7 @@ export type ExpressApi = typeof ExpressPingApiSpec & typeof ExpressV2WalletConsolidateAccountApiSpec & typeof ExpressWalletFanoutUnspentsApiSpec & typeof ExpressV2WalletCreateAddressApiSpec & + typeof ExpressV2WalletVerifyKeyApiSpec & typeof ExpressV2WalletIsWalletAddressApiSpec & typeof ExpressV2AddressDeriveApiSpec & typeof ExpressKeychainLocalApiSpec & @@ -466,6 +474,7 @@ export const ExpressApi: ExpressApi = { ...ExpressWalletConsolidateUnspentsApiSpec, ...ExpressWalletFanoutUnspentsApiSpec, ...ExpressV2WalletCreateAddressApiSpec, + ...ExpressV2WalletVerifyKeyApiSpec, ...ExpressV2WalletConsolidateAccountApiSpec, ...ExpressV2WalletIsWalletAddressApiSpec, ...ExpressV2AddressDeriveApiSpec, diff --git a/modules/express/src/typedRoutes/api/openapi-index.ts b/modules/express/src/typedRoutes/api/openapi-index.ts index 5ed6517afd0..29d4045817c 100644 --- a/modules/express/src/typedRoutes/api/openapi-index.ts +++ b/modules/express/src/typedRoutes/api/openapi-index.ts @@ -4,6 +4,7 @@ import { PostV2Encrypt } from './v2/encrypt'; import { PostGenerateWallet } from './v2/generateWallet'; import { GetV2PingExpress } from './v2/pingExpress'; import { PostWalletSweep } from './v2/walletSweep'; +import { PostVerifyKey } from './v2/verifyKey'; /** * Cumulative OpenAPI batch entrypoint. @@ -27,4 +28,7 @@ export const ExpressOpenApiSpec = apiSpec({ 'express.pingexpress': { get: GetV2PingExpress, }, + 'express.v2.wallet.verifyKey': { + post: PostVerifyKey, + }, }); diff --git a/modules/express/src/typedRoutes/api/v2/verifyKey.ts b/modules/express/src/typedRoutes/api/v2/verifyKey.ts new file mode 100644 index 00000000000..e48c2b2d1cc --- /dev/null +++ b/modules/express/src/typedRoutes/api/v2/verifyKey.ts @@ -0,0 +1,55 @@ +import * as t from 'io-ts'; +import { httpRoute, httpRequest } from '@api-ts/io-ts-http'; +import { BitgoExpressError } from '../../schemas/error'; + +/** + * Path parameters for verifying user key material against a wallet + */ +export const VerifyKeyParams = { + /** Blockchain identifier (e.g., 'tsol', 'tdot', 'tsui') */ + coin: t.string, + /** The wallet ID */ + id: t.string, +} as const; + +/** + * Request body for verifying user key material against a wallet + */ +export const VerifyKeyBody = { + /** + * User TSS signing material, the same string that would be passed as `prv` when signing a + * transaction (e.g. on sendmany); TSS EdDSA MPCv1 wallets only. Private key material: it is + * recombined locally and never sent to the server, and never echoed in error messages. + */ + prv: t.string, +} as const; + +/** + * Response for verifying user key material against a wallet + */ +export const VerifyKeyResponse = { + /** Whether the signing material recombines to the wallet's commonKeychain */ + 200: t.type({ match: t.boolean }), + /** Invalid request parameters, unsupported wallet type, or malformed signing material */ + 400: BitgoExpressError, +} as const; + +/** + * Verify that user-held TSS key material belongs to a wallet + * + * Recombines the shares locally and compares the result against the wallet's commonKeychain, + * answering up front whether the material can sign for this wallet. Supported for TSS EdDSA + * (MPCv1) wallets; other wallet types return a 400. + * + * @operationId express.v2.wallet.verifyKey + * @tag Express + */ +export const PostVerifyKey = httpRoute({ + path: '/api/v2/{coin}/wallet/{id}/verifyKey', + method: 'POST', + request: httpRequest({ + params: VerifyKeyParams, + body: VerifyKeyBody, + }), + response: VerifyKeyResponse, +}); diff --git a/modules/express/src/typedRoutes/utils.ts b/modules/express/src/typedRoutes/utils.ts index 0aa6ed4c5bd..b3c9fd88591 100644 --- a/modules/express/src/typedRoutes/utils.ts +++ b/modules/express/src/typedRoutes/utils.ts @@ -1,6 +1,7 @@ import * as t from 'io-ts'; import { ValidationError } from '../errors'; +import { SENSITIVE_REQUEST_KEYS } from '../sensitiveRequestKeys'; /** * Formats io-ts validation errors into clear, human-readable messages. @@ -25,7 +26,15 @@ export function formatValidationErrors(errors: t.Errors): string { if (error.value === undefined) { messages.push(`Missing required field '${path}'`); } else { - const value = typeof error.value === 'object' ? JSON.stringify(error.value) : String(error.value); + // values on a sensitive path are redacted rather than interpolated: they can carry + // request material a caller must never get echoed back (e.g. key material sent with + // the wrong field type) + const isSensitive = path.split('.').some((segment) => SENSITIVE_REQUEST_KEYS.has(segment.toLowerCase())); + const value = isSensitive + ? '[REDACTED]' + : error.value !== null && typeof error.value === 'object' + ? JSON.stringify(error.value) + : String(error.value); messages.push(`Invalid value for '${path}': expected ${expected}, got '${value}'`); } } diff --git a/modules/express/test/unit/typedRoutes/formatValidationErrors.ts b/modules/express/test/unit/typedRoutes/formatValidationErrors.ts index 2c8f7da8dec..435086360b7 100644 --- a/modules/express/test/unit/typedRoutes/formatValidationErrors.ts +++ b/modules/express/test/unit/typedRoutes/formatValidationErrors.ts @@ -14,6 +14,27 @@ describe('formatValidationErrors', function () { assert.strictEqual(formatValidationErrors(errors), "Invalid value for 'field': expected string, got '123'."); }); + it('should redact values on a sensitive path instead of interpolating them', function () { + const errors: t.Errors = [{ value: { uShare: { seed: 'deadbeef' } }, context: [{ key: 'prv', type: t.string }] }]; + assert.strictEqual(formatValidationErrors(errors), "Invalid value for 'prv': expected string, got '[REDACTED]'."); + }); + + it('should redact a sensitive path regardless of value type', function () { + const errors: t.Errors = [{ value: 'hunter2', context: [{ key: 'walletPassphrase', type: t.number }] }]; + assert.strictEqual( + formatValidationErrors(errors), + "Invalid value for 'walletPassphrase': expected number, got '[REDACTED]'." + ); + }); + + it('should interpolate structured values on a non-sensitive path', function () { + const errors: t.Errors = [{ value: { invalid: 'object' }, context: [{ key: 'webauthnInfo', type: t.string }] }]; + assert.strictEqual( + formatValidationErrors(errors), + 'Invalid value for \'webauthnInfo\': expected string, got \'{"invalid":"object"}\'.' + ); + }); + it('should format nested paths', function () { const errors: t.Errors = [ { diff --git a/modules/express/test/unit/typedRoutes/verifyKey.ts b/modules/express/test/unit/typedRoutes/verifyKey.ts new file mode 100644 index 00000000000..0d639d12158 --- /dev/null +++ b/modules/express/test/unit/typedRoutes/verifyKey.ts @@ -0,0 +1,165 @@ +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import { BitGo } from 'bitgo'; +import { BaseCoin, decodeOrElse } from '@bitgo/sdk-core'; +import { setupAgent } from '../../lib/testutil'; +import { VerifyKeyResponse } from '../../../src/typedRoutes/api/v2/verifyKey'; + +describe('verifyKey', function () { + const agent = setupAgent(); + + const walletId = '68c02f96aa757d9212bd1a536f123456'; + const coin = 'tsol'; + // wallet.verifyKey is stubbed in every test below, so the body value is opaque to these tests + const prv = '{"uShare":"stub-u","bitgoYShare":"stub-y","backupYShare":"stub-b"}'; + + function stubVerifyKey(verifyKeyStub: sinon.SinonStub) { + const mockWallet = { verifyKey: verifyKeyStub }; + const mockCoin = { + wallets: sinon.stub().returns({ get: sinon.stub().resolves(mockWallet) }), + }; + // a partial stand-in: the handler under test only calls coin.wallets().get() + sinon.stub(BitGo.prototype, 'coin').returns(mockCoin as unknown as BaseCoin); + } + + afterEach(function () { + sinon.restore(); + }); + + it('returns 200 { match: true } and passes the prv to wallet.verifyKey', async function () { + const verifyKeyStub = sinon.stub().resolves({ match: true }); + stubVerifyKey(verifyKeyStub); + + const result = await agent + .post(`/api/v2/${coin}/wallet/${walletId}/verifyKey`) + .set('Authorization', 'Bearer test_access_token_12345') + .set('Content-Type', 'application/json') + .send({ prv }); + + assert.strictEqual(result.status, 200); + assert.deepStrictEqual(result.body, { match: true }); + sinon.assert.calledOnceWithExactly(verifyKeyStub, { prv }); + decodeOrElse('express.v2.wallet.verifyKey', VerifyKeyResponse[200], result.body, (errors) => { + throw new Error(`Response did not match expected codec: ${errors}`); + }); + }); + + it('returns 200 { match: false } when the key does not belong to the wallet', async function () { + const verifyKeyStub = sinon.stub().resolves({ match: false }); + stubVerifyKey(verifyKeyStub); + + const result = await agent + .post(`/api/v2/${coin}/wallet/${walletId}/verifyKey`) + .set('Authorization', 'Bearer test_access_token_12345') + .set('Content-Type', 'application/json') + .send({ prv }); + + assert.strictEqual(result.status, 200); + assert.deepStrictEqual(result.body, { match: false }); + }); + + it('returns 400 with the SDK error message when wallet.verifyKey rejects', async function () { + const verifyKeyStub = sinon.stub().rejects(new Error('Key verification is not supported for this wallet type')); + stubVerifyKey(verifyKeyStub); + + const result = await agent + .post(`/api/v2/${coin}/wallet/${walletId}/verifyKey`) + .set('Authorization', 'Bearer test_access_token_12345') + .set('Content-Type', 'application/json') + .send({ prv }); + + assert.strictEqual(result.status, 400); + assert.strictEqual(result.body.message, 'Key verification is not supported for this wallet type'); + }); + + it('returns 400 without calling wallet.verifyKey when the body has no prv', async function () { + const verifyKeyStub = sinon.stub().resolves({ match: true }); + stubVerifyKey(verifyKeyStub); + + const result = await agent + .post(`/api/v2/${coin}/wallet/${walletId}/verifyKey`) + .set('Authorization', 'Bearer test_access_token_12345') + .set('Content-Type', 'application/json') + .send({}); + + assert.strictEqual(result.status, 400); + sinon.assert.notCalled(verifyKeyStub); + }); + + it('propagates the upstream status when the wallet lookup fails', async function () { + const verifyKeyStub = sinon.stub().resolves({ match: true }); + const notFound = Object.assign(new Error('wallet not found'), { status: 404 }); + sinon.stub(BitGo.prototype, 'coin').returns({ + wallets: sinon.stub().returns({ get: sinon.stub().rejects(notFound) }), + } as unknown as BaseCoin); + + const result = await agent + .post(`/api/v2/${coin}/wallet/${walletId}/verifyKey`) + .set('Authorization', 'Bearer test_access_token_12345') + .set('Content-Type', 'application/json') + .send({ prv }); + + assert.strictEqual(result.status, 404); + sinon.assert.notCalled(verifyKeyStub); + }); + + it('propagates a status-bearing error from wallet.verifyKey instead of wrapping it to 400', async function () { + const verifyKeyStub = sinon.stub().rejects(Object.assign(new Error('keychain fetch failed'), { status: 429 })); + stubVerifyKey(verifyKeyStub); + + const result = await agent + .post(`/api/v2/${coin}/wallet/${walletId}/verifyKey`) + .set('Authorization', 'Bearer test_access_token_12345') + .set('Content-Type', 'application/json') + .send({ prv }); + + assert.strictEqual(result.status, 429); + assert.strictEqual(result.body.message, 'keychain fetch failed'); + }); + + it('surfaces a transport-level failure as an infrastructure error rather than a 400', async function () { + const verifyKeyStub = sinon + .stub() + .rejects(Object.assign(new Error('connect ECONNREFUSED'), { code: 'ECONNREFUSED' })); + stubVerifyKey(verifyKeyStub); + + const result = await agent + .post(`/api/v2/${coin}/wallet/${walletId}/verifyKey`) + .set('Authorization', 'Bearer test_access_token_12345') + .set('Content-Type', 'application/json') + .send({ prv }); + + assert.strictEqual(result.status, 500); + assert.strictEqual(result.body.message, 'connect ECONNREFUSED'); + }); + + it('returns 400 with a stringified message when wallet.verifyKey rejects with a non-Error', async function () { + const verifyKeyStub = sinon.stub().callsFake(() => Promise.reject('not an error')); + stubVerifyKey(verifyKeyStub); + + const result = await agent + .post(`/api/v2/${coin}/wallet/${walletId}/verifyKey`) + .set('Authorization', 'Bearer test_access_token_12345') + .set('Content-Type', 'application/json') + .send({ prv }); + + assert.strictEqual(result.status, 400); + assert.strictEqual(result.body.message, 'not an error'); + }); + + it('returns 400 without echoing the material when prv is not a string', async function () { + const verifyKeyStub = sinon.stub().resolves({ match: true }); + stubVerifyKey(verifyKeyStub); + + const result = await agent + .post(`/api/v2/${coin}/wallet/${walletId}/verifyKey`) + .set('Authorization', 'Bearer test_access_token_12345') + .set('Content-Type', 'application/json') + .send({ prv: { uShare: { seed: 'deadbeefdeadbeef' } } }); + + assert.strictEqual(result.status, 400); + assert.ok(!JSON.stringify(result.body).includes('deadbeef'), 'response must not echo the material'); + assert.ok(result.body.error.includes("got '[REDACTED]'"), 'response must show the redaction marker'); + sinon.assert.notCalled(verifyKeyStub); + }); +}); diff --git a/modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts b/modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts index ba91ab4e0b5..d7bf124d0e7 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts @@ -3,6 +3,7 @@ import * as openpgp from 'openpgp'; import { Key, readKey, SerializedKeyPair } from 'openpgp'; import { IBaseCoin, KeychainsTriplet, TransactionParams } from '../../baseCoin'; import { BitGoBase } from '../../bitgoBase'; +import { MethodNotImplementedError } from '../../errors'; import { Keychain, KeyIndices, WebauthnKeyEncryptionInfo } from '../../keychain'; import { getTxRequest } from '../../tss'; import { IWallet } from '../../wallet'; @@ -257,6 +258,18 @@ export default class BaseTssUtils extends MpcUtils implements ITssUtil throw new Error('Method not implemented.'); } + supportsVerifyKey(): boolean { + return false; + } + + async verifyKey(params: { prv: string; commonKeychain: string }): Promise { + // unsupported wallet types are rejected by the `supportsVerifyKey()` gate in Wallet.verifyKey, + // which surfaces the user-facing message; reaching this stub means a direct caller invoked + // verifyKey on an algorithm that does not implement it. + // async so the rejection, rather than a sync throw, reaches callers of a Promise-typed method. + throw new MethodNotImplementedError(); + } + /** * Signs a transaction using TSS for EdDSA and through utilization of custom share generators * diff --git a/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts b/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts index df64526fa4c..a242164f875 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts @@ -979,4 +979,6 @@ export interface ITssUtils { recreateTxRequest(txRequestId: string, decryptedPrv: string, reqId: IRequestTracer): Promise; getTxRequest(txRequestId: string): Promise; supportedTxRequestVersions(): TxRequestVersion[]; + supportsVerifyKey(): boolean; + verifyKey(params: { prv: string; commonKeychain: string }): Promise; } diff --git a/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsa.ts b/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsa.ts index 94247da5bad..6b723e38a51 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsa.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsa.ts @@ -39,6 +39,7 @@ import { import { InvalidTransactionError } from '../../../errors'; import { CreateEddsaBitGoKeychainParams, CreateEddsaKeychainParams, KeyShare, YShare } from './types'; import baseTSSUtils from '../baseTSSUtils'; +import { eddsaUserSigningMaterialMatchesCommonKeychain } from '../keyVerification'; import { BaseEddsaUtils } from './base'; import { KeychainsTriplet, TransactionParams } from '../../../baseCoin'; import { exchangeEddsaCommitments } from '../../../tss/common'; @@ -868,6 +869,14 @@ export class EddsaUtils extends baseTSSUtils { return this.signRequestBase(params, RequestType.message); } + supportsVerifyKey(): boolean { + return true; + } + + async verifyKey(params: { prv: string; commonKeychain: string }): Promise { + return eddsaUserSigningMaterialMatchesCommonKeychain(params); + } + /** * Get the commonPub portion of the commonKeychain. * diff --git a/modules/sdk-core/src/bitgo/utils/tss/index.ts b/modules/sdk-core/src/bitgo/utils/tss/index.ts index fb3efad17cd..c5cf3e36cd5 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/index.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/index.ts @@ -16,6 +16,7 @@ export { ITssUtils, IEddsaUtils, TxRequest, EddsaUnsignedTransaction } from './e export * as BaseTssUtils from './baseTSSUtils'; export * from './baseTypes'; export * from './addressVerification'; +export * from './keyVerification'; export * from './preHashedSignable'; export * from './recipientUtils'; export * from './keyShareEnvelope'; diff --git a/modules/sdk-core/src/bitgo/utils/tss/keyVerification.ts b/modules/sdk-core/src/bitgo/utils/tss/keyVerification.ts new file mode 100644 index 00000000000..141742a554e --- /dev/null +++ b/modules/sdk-core/src/bitgo/utils/tss/keyVerification.ts @@ -0,0 +1,159 @@ +/** + * @prettier + */ +import * as t from 'io-ts'; +import Eddsa, { KeyCombine } from '../../../account-lib/mpc/tss'; +import { UserSigningMaterial } from '../../tss/eddsa/types'; + +const HEX_32_BYTES = /^[0-9a-f]{64}$/i; + +/** 32-byte hex string; every hex field of genuine MPCv1 signing material is fixed-width 32 bytes. */ +const Hex32Bytes = new t.Type( + 'Hex32Bytes', + (u): u is string => typeof u === 'string' && HEX_32_BYTES.test(u), + (u, c) => (typeof u === 'string' && HEX_32_BYTES.test(u) ? t.success(u) : t.failure(u, c)), + t.identity +); + +const UShareCodec = t.type({ + i: t.number, + t: t.number, + n: t.number, + y: Hex32Bytes, + seed: Hex32Bytes, + chaincode: Hex32Bytes, +}); + +/** + * `v` is the optional VSS commitment: absent on shares created without verifiable secret sharing. + * It is declared through `t.partial` rather than a union with `undefined` so that decoding an + * absent `v` does not materialize it as an own property on the share handed to `keyCombine`. + */ +const YShareCodec = t.intersection([ + t.type({ + i: t.number, + j: t.number, + y: Hex32Bytes, + u: Hex32Bytes, + chaincode: Hex32Bytes, + }), + t.partial({ v: Hex32Bytes }), +]); + +const UserSigningMaterialCodec = t.type({ + uShare: UShareCodec, + bitgoYShare: YShareCodec, + backupYShare: YShareCodec, +}); + +/** + * Maps a decode failure to a constant message. Never interpolates `error.value`: the input is + * private key material. This is also why the codecs call `t.failure` without a message and why + * the `validationErrors` reporter in `utils/decode.ts` is not used — both embed the failing value. + */ +function toInvalidKeyMessage(errors: t.Errors): string { + const [error] = errors; + // the head of the context is the root codec itself, so the remaining keys are the field path. + // purely numeric keys are intersection/union member indices rather than field names. + const segments = error.context + .slice(1) + .map((entry) => entry.key) + .filter((key) => !/^\d+$/.test(key)); + if (segments.length === 0) { + return 'Invalid user key - signing material is not an object'; + } + if (segments.length === 1 && (error.value === undefined || error.value === null)) { + return `Invalid user key - missing ${segments[0]}`; + } + return `Invalid user key - ${segments[0]} is not a valid share`; +} + +/** + * Parses the user TSS EdDSA signing material that a client passes as `prv` when signing, + * and rejects anything that does not decode as user signing material. + * + * Decoding is a shape check only, but it keeps malformed input away from the combine routine, + * which throws raw, input-interpolating errors for malformed shares. + * + * The input is private key material, so no error message may interpolate any part of it. + * In particular, JSON.parse errors on Node >= 20 include the raw input in their message, + * which is why parse failures are rethrown with a constant message. + * + * @param prv - stringified user signing material (`uShare`, `bitgoYShare`, `backupYShare`) + */ +export function parseEddsaUserSigningMaterial(prv: string): UserSigningMaterial { + let parsed: unknown; + try { + parsed = JSON.parse(prv); + } catch (e) { + throw new Error('Invalid user key - could not parse signing material'); + } + const decoded = UserSigningMaterialCodec.decode(parsed); + if (decoded._tag === 'Left') { + throw new Error(toInvalidKeyMessage(decoded.left)); + } + return decoded.right as UserSigningMaterial; +} + +/** + * Checks that `uShare.y` is the public point actually derived from `uShare.seed`. + * + * `keyCombine` derives the private scalar from the seed but reconstructs the common keychain from + * the declared `y` fields, so it never compares the two. Without this check a caller who knows + * only the wallet's public commonKeychain can submit genuine public fields alongside a seed they + * do not own and still verify. Re-running `keyShare` with the share's own seed and chaincode + * reproduces the derivation and yields the `y` the seed really commits to. + */ +function uShareSeedMatchesY(MPC: Eddsa, uShare: UserSigningMaterial['uShare']): boolean { + const seedchain = Buffer.concat([Buffer.from(uShare.seed, 'hex'), Buffer.from(uShare.chaincode, 'hex')]); + let derived: UserSigningMaterial['uShare']; + try { + derived = MPC.keyShare(uShare.i, uShare.t, uShare.n, seedchain).uShare; + } catch (e) { + // keyShare rejects an out-of-range index or threshold, which makes the share unusable + return false; + } + return derived.y === uShare.y; +} + +/** + * Verifies that user TSS EdDSA signing material (`prv`) recombines to a given commonKeychain. + * + * Recombines the user's share with the BitGo and backup Y shares — the same computation the + * SDK performs when creating the user keychain or signing — and compares the result against + * the wallet's commonKeychain. Returns false rather than throwing when the keys do not match. + * Shares that pass the shape check but are cryptographically inconsistent are rejected with + * a constant message: the combine routine throws raw errors, some of which interpolate + * submitter-controlled fields, which must not surface through the API. + * + * Recombination alone is not proof of possession, so two further checks run first. The seed is + * bound to `uShare.y`, and the Y shares must carry their VSS commitment `v` — `keyCombine` + * verifies a Y share's secret `u` only when `v` is present, so omitting it skips the check that + * the share is consistent with the public `y` it claims. Genuine shares produced by `keyShare` + * always carry `v`. + * + * @param params.prv - stringified user signing material, as passed as `prv` when signing + * @param params.commonKeychain - the wallet's user keychain commonKeychain + */ +export async function eddsaUserSigningMaterialMatchesCommonKeychain(params: { + prv: string; + commonKeychain: string; +}): Promise { + const signingMaterial = parseEddsaUserSigningMaterial(params.prv); + const MPC = await Eddsa.initialize(); + if (!uShareSeedMatchesY(MPC, signingMaterial.uShare)) { + return false; + } + // keyCombine verifies a Y share's secret `u` against its commitment only when `v` is present, + // so material without it would skip that check entirely + if (signingMaterial.bitgoYShare.v === undefined || signingMaterial.backupYShare.v === undefined) { + return false; + } + let combinedKey: KeyCombine; + try { + combinedKey = MPC.keyCombine(signingMaterial.uShare, [signingMaterial.bitgoYShare, signingMaterial.backupYShare]); + } catch (e) { + throw new Error('Invalid user key - could not combine signing material'); + } + return combinedKey.pShare.y + combinedKey.pShare.chaincode === params.commonKeychain; +} diff --git a/modules/sdk-core/src/bitgo/wallet/iWallet.ts b/modules/sdk-core/src/bitgo/wallet/iWallet.ts index e0130c883b3..d4f3fd66734 100644 --- a/modules/sdk-core/src/bitgo/wallet/iWallet.ts +++ b/modules/sdk-core/src/bitgo/wallet/iWallet.ts @@ -459,6 +459,20 @@ export interface GetUserPrvOptions { walletPassphrase?: string; } +export interface VerifyKeyOptions { + /** + * User TSS signing material: the same string that would be passed as `prv` when signing a + * transaction. TSS EdDSA (MPCv1) wallets only. + */ + prv: string; + reqId?: IRequestTracer; +} + +export interface VerifyKeyResult { + /** Whether the signing material recombines to the wallet's commonKeychain */ + match: boolean; +} + export interface WalletCoinSpecific { tokenFlushThresholds?: any; addressVersion?: number; @@ -1319,6 +1333,7 @@ export interface IWallet { prebuildTransaction(params?: PrebuildTransactionOptions): Promise; signTransaction(params?: WalletSignTransactionOptions): Promise; getUserPrv(params?: GetUserPrvOptions): Promise; + verifyKey(params: VerifyKeyOptions): Promise; prebuildAndSignTransaction(params?: PrebuildAndSignTransactionOptions): Promise; signAndSendTxRequest(params?: SignAndSendTxRequestOptions): Promise; accelerateTransaction(params?: AccelerateTransactionOptions): Promise; diff --git a/modules/sdk-core/src/bitgo/wallet/wallet.ts b/modules/sdk-core/src/bitgo/wallet/wallet.ts index 68b7d8e0daa..26031b41fdd 100644 --- a/modules/sdk-core/src/bitgo/wallet/wallet.ts +++ b/modules/sdk-core/src/bitgo/wallet/wallet.ts @@ -138,6 +138,8 @@ import { UpdateWalletOptions, UpgradeEncryptionOptions, UpgradeEncryptionResult, + VerifyKeyOptions, + VerifyKeyResult, WalletCoinSpecific, WalletData, WalletEcdsaChallenges, @@ -2721,6 +2723,41 @@ export class Wallet implements IWallet { return userPrv; } + /** + * Verify that user-held TSS signing material belongs to this wallet. + * + * Recombines the shares locally and compares the result against the user keychain's + * `commonKeychain`, answering up front whether the material can sign for this wallet + * instead of discovering a mismatch only when a signature fails. Supported for TSS EdDSA + * (MPCv1) wallets; other wallet types are rejected. + * + * `walletPassphrase` is deliberately not accepted: decrypting the BitGo-held `encryptedPrv` + * and comparing it against the BitGo-held keychain is circular, and callers that hold their + * own key material do not need it. + * + * @param params.prv - user signing material, the same string that would be passed as `prv` when signing + * @param params.reqId - request tracer + * @returns whether the recombined key matches the wallet's commonKeychain + */ + async verifyKey(params: VerifyKeyOptions): Promise { + if (this.multisigType() !== 'tss' || !this.tssUtils?.supportsVerifyKey()) { + throw new Error('Key verification is not supported for this wallet type'); + } + const userKeyId = this.keyIds()?.[KeyIndices.USER]; + if (!userKeyId) { + throw new Error('wallet is missing a user keychain id'); + } + const userKeychain = await this.baseCoin.keychains().get({ + id: userKeyId, + reqId: params.reqId, + }); + const { commonKeychain } = userKeychain; + if (!commonKeychain) { + throw new Error('wallet keychain is missing commonKeychain'); + } + return { match: await this.tssUtils.verifyKey({ prv: params.prv, commonKeychain }) }; + } + /** * Get a transaction prebuild from BitGo, validate it, and then decrypt the user key and sign the transaction * @param params diff --git a/modules/sdk-core/test/unit/bitgo/utils/tss/keyVerification.ts b/modules/sdk-core/test/unit/bitgo/utils/tss/keyVerification.ts new file mode 100644 index 00000000000..8e4a1cf5b7a --- /dev/null +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/keyVerification.ts @@ -0,0 +1,223 @@ +import * as assert from 'assert'; +import 'should'; +import { Ed25519Bip32HdTree } from '@bitgo/sdk-lib-mpc'; +import Eddsa from '../../../../../src/account-lib/mpc/tss'; +import { + eddsaUserSigningMaterialMatchesCommonKeychain, + parseEddsaUserSigningMaterial, +} from '../../../../../src/bitgo/utils/tss/keyVerification'; + +describe('TSS EdDSA key verification', function () { + let matchingPrv: string; + let commonKeychain: string; + let otherPrv: string; + let backupStylePrv: string; + + before(async function () { + const hdTree = await Ed25519Bip32HdTree.initialize(); + const MPC = await Eddsa.initialize(hdTree); + + const userKeyShare = MPC.keyShare(1, 2, 3); + const backupKeyShare = MPC.keyShare(2, 2, 3); + const bitgoKeyShare = MPC.keyShare(3, 2, 3); + const combined = MPC.keyCombine(userKeyShare.uShare, [backupKeyShare.yShares[1], bitgoKeyShare.yShares[1]]); + commonKeychain = combined.pShare.y + combined.pShare.chaincode; + matchingPrv = JSON.stringify({ + uShare: userKeyShare.uShare, + bitgoYShare: bitgoKeyShare.yShares[1], + backupYShare: backupKeyShare.yShares[1], + }); + + // an independent key generation for the negative case + const otherUserKeyShare = MPC.keyShare(1, 2, 3); + const otherBackupKeyShare = MPC.keyShare(2, 2, 3); + const otherBitgoKeyShare = MPC.keyShare(3, 2, 3); + otherPrv = JSON.stringify({ + uShare: otherUserKeyShare.uShare, + bitgoYShare: otherBitgoKeyShare.yShares[1], + backupYShare: otherBackupKeyShare.yShares[1], + }); + + // backup-style signing material: the holder is the backup party (i=2), so it carries + // userYShare instead of backupYShare + backupStylePrv = JSON.stringify({ + uShare: backupKeyShare.uShare, + bitgoYShare: bitgoKeyShare.yShares[2], + userYShare: userKeyShare.yShares[2], + }); + }); + + describe('parseEddsaUserSigningMaterial', function () { + it('parses well-formed user signing material', function () { + const parsed = parseEddsaUserSigningMaterial(matchingPrv); + parsed.should.have.property('uShare'); + parsed.should.have.property('bitgoYShare'); + parsed.should.have.property('backupYShare'); + }); + + it('rejects input that is not valid JSON without echoing the input', function () { + const prv = 'not json'; + assert.throws( + () => parseEddsaUserSigningMaterial(prv), + (err: unknown) => { + assert.ok(err instanceof Error); + assert.ok(!err.message.includes(prv), 'error message must not echo the input'); + return true; + } + ); + }); + + it('rejects input that parses to a non-object', function () { + assert.throws(() => parseEddsaUserSigningMaterial('123'), /signing material is not an object/); + }); + + it('rejects material missing any of the three required shares', function () { + const { uShare, bitgoYShare, backupYShare } = parseEddsaUserSigningMaterial(matchingPrv); + assert.throws( + () => parseEddsaUserSigningMaterial(JSON.stringify({ bitgoYShare, backupYShare })), + /missing uShare/ + ); + assert.throws( + () => parseEddsaUserSigningMaterial(JSON.stringify({ uShare, backupYShare })), + /missing bitgoYShare/ + ); + assert.throws( + () => parseEddsaUserSigningMaterial(JSON.stringify({ uShare, bitgoYShare })), + /missing backupYShare/ + ); + }); + + it('rejects backup-style material; only user signing material is accepted', function () { + assert.throws(() => parseEddsaUserSigningMaterial(backupStylePrv), /missing backupYShare/); + }); + + it('rejects shares that are not share-shaped objects', function () { + const { uShare, bitgoYShare, backupYShare } = parseEddsaUserSigningMaterial(matchingPrv); + // array shares are malformed; null shares count as missing + assert.throws( + () => parseEddsaUserSigningMaterial(JSON.stringify({ uShare, bitgoYShare: [], backupYShare })), + /bitgoYShare is not a valid share/ + ); + assert.throws( + () => parseEddsaUserSigningMaterial(JSON.stringify({ uShare, bitgoYShare: null, backupYShare })), + /missing bitgoYShare/ + ); + assert.throws( + () => parseEddsaUserSigningMaterial(JSON.stringify({ uShare: {}, bitgoYShare, backupYShare })), + /uShare is not a valid share/ + ); + }); + + it('rejects shares with malformed fields', function () { + const malformed = JSON.stringify({ + uShare: { i: 1, t: 2, n: 3, y: 'not-hex', seed: 'ab', chaincode: 'cd' }, + bitgoYShare: { i: 1, j: 3, y: 'ab', u: 'cd', chaincode: 'ef', v: '01' }, + backupYShare: { i: 1, j: 2, y: 'ab', u: 'cd', chaincode: 'ef' }, + }); + assert.throws(() => parseEddsaUserSigningMaterial(malformed), /uShare is not a valid share/); + }); + + it('rejects hex fields of the wrong length', function () { + const { uShare, bitgoYShare, backupYShare } = parseEddsaUserSigningMaterial(matchingPrv); + const truncatedChaincode = JSON.stringify({ + uShare: { ...uShare, chaincode: uShare.chaincode.slice(0, 62) }, + bitgoYShare, + backupYShare, + }); + assert.throws(() => parseEddsaUserSigningMaterial(truncatedChaincode), /uShare is not a valid share/); + }); + + it('rejects a non-hex v field on a Y share when present', function () { + const { uShare, bitgoYShare, backupYShare } = parseEddsaUserSigningMaterial(matchingPrv); + const tampered = JSON.stringify({ + uShare, + backupYShare, + bitgoYShare: { ...bitgoYShare, v: 'zz' }, + }); + assert.throws(() => parseEddsaUserSigningMaterial(tampered), /bitgoYShare is not a valid share/); + }); + + it('accepts a Y share without the optional v field', function () { + // v is optional on the YShare type, so parsing must allow it; verification separately + // requires it as proof of possession (see the VSS commitment case below) + const { uShare, bitgoYShare, backupYShare } = parseEddsaUserSigningMaterial(matchingPrv); + const withoutV = { ...bitgoYShare }; + delete withoutV.v; + const vlessPrv = JSON.stringify({ uShare, bitgoYShare: withoutV, backupYShare }); + const parsed = parseEddsaUserSigningMaterial(vlessPrv); + parsed.bitgoYShare.should.not.have.property('v'); + }); + }); + + describe('eddsaUserSigningMaterialMatchesCommonKeychain', function () { + it('returns true when the combined key equals the commonKeychain', async function () { + const match = await eddsaUserSigningMaterialMatchesCommonKeychain({ prv: matchingPrv, commonKeychain }); + match.should.equal(true); + }); + + it('returns false for material from a different key generation', async function () { + const match = await eddsaUserSigningMaterialMatchesCommonKeychain({ prv: otherPrv, commonKeychain }); + match.should.equal(false); + }); + + it('rejects malformed material without echoing the input', async function () { + await assert.rejects(eddsaUserSigningMaterialMatchesCommonKeychain({ prv: 'not json', commonKeychain }), { + message: 'Invalid user key - could not parse signing material', + }); + }); + + it('rejects inconsistent shares with a constant error instead of the raw combine failure', async function () { + const { uShare, bitgoYShare, backupYShare } = parseEddsaUserSigningMaterial(matchingPrv); + const tampered = JSON.stringify({ + uShare, + bitgoYShare, + backupYShare: { ...backupYShare, v: '00'.repeat(32) }, + }); + await assert.rejects(eddsaUserSigningMaterialMatchesCommonKeychain({ prv: tampered, commonKeychain }), { + message: 'Invalid user key - could not combine signing material', + }); + }); + + it('returns false for a wrong seed behind otherwise genuine public fields', async function () { + // keyCombine derives the private scalar from the seed but rebuilds the common keychain from + // the declared y fields, so without an explicit seed/y binding a caller who knows only the + // public commonKeychain could verify against a seed they do not hold + const { uShare, bitgoYShare, backupYShare } = parseEddsaUserSigningMaterial(matchingPrv); + const forged = JSON.stringify({ + uShare: { ...uShare, seed: 'ab'.repeat(32) }, + bitgoYShare, + backupYShare, + }); + const match = await eddsaUserSigningMaterialMatchesCommonKeychain({ prv: forged, commonKeychain }); + match.should.equal(false); + }); + + it('returns false when a Y share omits its VSS commitment', async function () { + // keyCombine only verifies a Y share's secret u when v is present, so material that drops v + // would otherwise pass with an arbitrary u + const { uShare, bitgoYShare, backupYShare } = parseEddsaUserSigningMaterial(matchingPrv); + const bitgoWithoutV = { ...bitgoYShare, u: 'cd'.repeat(32) }; + delete bitgoWithoutV.v; + const forged = JSON.stringify({ + uShare, + bitgoYShare: bitgoWithoutV, + backupYShare, + }); + const match = await eddsaUserSigningMaterialMatchesCommonKeychain({ prv: forged, commonKeychain }); + match.should.equal(false); + }); + + it('returns false when a Y share carries a mismatched secret with its commitment present', async function () { + const { uShare, bitgoYShare, backupYShare } = parseEddsaUserSigningMaterial(matchingPrv); + const forged = JSON.stringify({ + uShare, + bitgoYShare: { ...bitgoYShare, u: 'cd'.repeat(32) }, + backupYShare, + }); + // a present-but-failing commitment is a combine failure, surfaced as the constant error + await assert.rejects(eddsaUserSigningMaterialMatchesCommonKeychain({ prv: forged, commonKeychain }), { + message: 'Invalid user key - could not combine signing material', + }); + }); + }); +}); diff --git a/modules/sdk-core/test/unit/bitgo/wallet/walletVerifyKey.ts b/modules/sdk-core/test/unit/bitgo/wallet/walletVerifyKey.ts new file mode 100644 index 00000000000..318b60da6f2 --- /dev/null +++ b/modules/sdk-core/test/unit/bitgo/wallet/walletVerifyKey.ts @@ -0,0 +1,158 @@ +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import 'should'; +import { Wallet } from '../../../../src/bitgo/wallet/wallet'; +import { Ed25519Bip32HdTree } from '@bitgo/sdk-lib-mpc'; +import Eddsa from '../../../../src/account-lib/mpc/tss'; +import { BitGoBase } from '../../../../src/bitgo/bitgoBase'; +import { IBaseCoin } from '../../../../src/bitgo/baseCoin'; +import { getBitgoMpcGpgPubKey } from '../../../../src/bitgo/tss/bitgoPubKeys'; +import { RequestTracer } from '../../../../src/bitgo/utils/util'; + +const UNSUPPORTED_WALLET_MESSAGE = 'Key verification is not supported for this wallet type'; + +describe('Wallet.verifyKey', function () { + let commonKeychain: string; + let matchingPrv: string; + let otherPrv: string; + + before(async function () { + const hdTree = await Ed25519Bip32HdTree.initialize(); + const MPC = await Eddsa.initialize(hdTree); + + const userKeyShare = MPC.keyShare(1, 2, 3); + const backupKeyShare = MPC.keyShare(2, 2, 3); + const bitgoKeyShare = MPC.keyShare(3, 2, 3); + const combined = MPC.keyCombine(userKeyShare.uShare, [backupKeyShare.yShares[1], bitgoKeyShare.yShares[1]]); + commonKeychain = combined.pShare.y + combined.pShare.chaincode; + matchingPrv = JSON.stringify({ + uShare: userKeyShare.uShare, + bitgoYShare: bitgoKeyShare.yShares[1], + backupYShare: backupKeyShare.yShares[1], + }); + + // an independent key generation for the negative case + const otherUserKeyShare = MPC.keyShare(1, 2, 3); + const otherBackupKeyShare = MPC.keyShare(2, 2, 3); + const otherBitgoKeyShare = MPC.keyShare(3, 2, 3); + otherPrv = JSON.stringify({ + uShare: otherUserKeyShare.uShare, + bitgoYShare: otherBitgoKeyShare.yShares[1], + backupYShare: otherBackupKeyShare.yShares[1], + }); + }); + + let mockBitGo: { post: sinon.SinonStub; setRequestTracer: sinon.SinonStub; fetchConstants: sinon.SinonStub }; + let mockBaseCoin: { + supportsTss: sinon.SinonStub; + getFamily: sinon.SinonStub; + getMPCAlgorithm: sinon.SinonStub; + keychains: sinon.SinonStub; + url: sinon.SinonStub; + }; + let walletData: { id: string; keys: string[]; multisigType: string; multisigTypeVersion?: string }; + let getKeychainStub: sinon.SinonStub; + + beforeEach(function () { + mockBitGo = { + post: sinon.stub(), + setRequestTracer: sinon.stub(), + // the MPCv2/ECDSA utils constructors call setBitgoGpgPubKey, which fetches constants; + // stubbing it avoids a swallowed unhandled rejection in those tests + fetchConstants: sinon.stub().resolves({ + mpc: { bitgoPublicKey: getBitgoMpcGpgPubKey('test', 'nitro', 'mpcv1') }, + }), + }; + + getKeychainStub = sinon.stub().resolves({ id: 'user-key-id', type: 'tss', commonKeychain }); + + mockBaseCoin = { + supportsTss: sinon.stub().returns(true), + getFamily: sinon.stub().returns('sol'), + getMPCAlgorithm: sinon.stub().returns('eddsa'), + keychains: sinon.stub().returns({ get: getKeychainStub }), + url: sinon.stub().returns('/api/v2/sol/wallet/test-wallet-id'), + }; + + walletData = { + id: 'test-wallet-id', + keys: ['user-key-id', 'backup-key-id', 'bitgo-key-id'], + multisigType: 'tss', + }; + }); + + afterEach(function () { + sinon.restore(); + }); + + /** + * Builds the wallet under test from the sinon stand-ins. The casts are safe because the + * Wallet constructor and verifyKey only touch the members stubbed above. + */ + function buildWallet(): Wallet { + return new Wallet(mockBitGo as unknown as BitGoBase, mockBaseCoin as unknown as IBaseCoin, walletData); + } + + it('returns match true for user signing material that belongs to the wallet', async function () { + const wallet = buildWallet(); + + const result = await wallet.verifyKey({ prv: matchingPrv }); + + assert.deepStrictEqual(result, { match: true }); + sinon.assert.calledOnce(getKeychainStub); + assert.strictEqual(getKeychainStub.firstCall.args[0].id, walletData.keys[0]); + }); + + it('forwards the request tracer to the keychain fetch', async function () { + const wallet = buildWallet(); + const reqId = new RequestTracer(); + + const result = await wallet.verifyKey({ prv: matchingPrv, reqId }); + + assert.deepStrictEqual(result, { match: true }); + sinon.assert.calledOnce(getKeychainStub); + assert.strictEqual(getKeychainStub.firstCall.args[0].reqId, reqId); + }); + + it('returns match false for user signing material from a different key generation', async function () { + const wallet = buildWallet(); + + const result = await wallet.verifyKey({ prv: otherPrv }); + + assert.deepStrictEqual(result, { match: false }); + sinon.assert.calledOnce(getKeychainStub); + }); + + it('rejects on-chain multisig wallets without fetching the keychain', async function () { + walletData.multisigType = 'onchain'; + const wallet = buildWallet(); + + await assert.rejects(wallet.verifyKey({ prv: matchingPrv }), { message: UNSUPPORTED_WALLET_MESSAGE }); + sinon.assert.notCalled(getKeychainStub); + }); + + it('rejects TSS EdDSA MPCv2 wallets without fetching the keychain', async function () { + walletData.multisigTypeVersion = 'MPCv2'; + const wallet = buildWallet(); + + await assert.rejects(wallet.verifyKey({ prv: matchingPrv }), { message: UNSUPPORTED_WALLET_MESSAGE }); + sinon.assert.notCalled(getKeychainStub); + }); + + it('rejects TSS ECDSA wallets without fetching the keychain', async function () { + mockBaseCoin.getMPCAlgorithm.returns('ecdsa'); + const wallet = buildWallet(); + + await assert.rejects(wallet.verifyKey({ prv: matchingPrv }), { message: UNSUPPORTED_WALLET_MESSAGE }); + sinon.assert.notCalled(getKeychainStub); + }); + + it('rejects when the user keychain has no commonKeychain', async function () { + getKeychainStub.resolves({ id: 'user-key-id', type: 'tss' }); + const wallet = buildWallet(); + + await assert.rejects(wallet.verifyKey({ prv: matchingPrv }), { + message: 'wallet keychain is missing commonKeychain', + }); + }); +}); From 616898c74a798d975f67e699f6b7fe10c041b1a1 Mon Sep 17 00:00:00 2001 From: yashaanand Date: Mon, 5 Oct 2026 14:05:54 -0400 Subject: [PATCH 2/2] fix(sdk-core): report TSS signing material without VSS commitments as unverifiable Review follow-up on the verifyKey endpoint: material whose Y shares lack their VSS commitment cannot be verified at all, so it now throws a distinct "Unable to verify key" error instead of returning match: false, which genuine material without the commitment would otherwise be misreported as. Documents that a true result is a consistency check, not proof of possession and never an authorization signal, and that callers must handle both the 200 false and the 400 outcomes for unusable material. Ticket: WAL-1868 --- .../src/typedRoutes/api/v2/verifyKey.ts | 12 ++++++++- .../src/bitgo/utils/tss/keyVerification.ts | 17 ++++++++---- modules/sdk-core/src/bitgo/wallet/iWallet.ts | 5 +++- modules/sdk-core/src/bitgo/wallet/wallet.ts | 3 +++ .../unit/bitgo/utils/tss/keyVerification.ts | 26 ++++++++++++++----- 5 files changed, 50 insertions(+), 13 deletions(-) diff --git a/modules/express/src/typedRoutes/api/v2/verifyKey.ts b/modules/express/src/typedRoutes/api/v2/verifyKey.ts index e48c2b2d1cc..d189f33e94b 100644 --- a/modules/express/src/typedRoutes/api/v2/verifyKey.ts +++ b/modules/express/src/typedRoutes/api/v2/verifyKey.ts @@ -30,7 +30,7 @@ export const VerifyKeyBody = { export const VerifyKeyResponse = { /** Whether the signing material recombines to the wallet's commonKeychain */ 200: t.type({ match: t.boolean }), - /** Invalid request parameters, unsupported wallet type, or malformed signing material */ + /** Invalid request parameters, unsupported wallet type, or signing material that is malformed, inconsistent, or cannot be verified */ 400: BitgoExpressError, } as const; @@ -41,6 +41,16 @@ export const VerifyKeyResponse = { * answering up front whether the material can sign for this wallet. Supported for TSS EdDSA * (MPCv1) wallets; other wallet types return a 400. * + * Outcomes: `match: false` means the material is well-formed and self-consistent but + * recombines to a different key. A 400 means the material is malformed, cryptographically + * inconsistent, or lacks the VSS commitments needed to verify it at all. Callers treating + * "this key does not work for this wallet" as one condition must handle both the 200 `false` + * and the 400. + * + * `match: true` is a consistency check for a caller inspecting its own key file, not proof of + * possession: anyone who knows the wallet's public commonKeychain can construct shares that + * pass. Never use the result as an authorization signal. + * * @operationId express.v2.wallet.verifyKey * @tag Express */ diff --git a/modules/sdk-core/src/bitgo/utils/tss/keyVerification.ts b/modules/sdk-core/src/bitgo/utils/tss/keyVerification.ts index 141742a554e..5708cba29b4 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/keyVerification.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/keyVerification.ts @@ -128,9 +128,15 @@ function uShareSeedMatchesY(MPC: Eddsa, uShare: UserSigningMaterial['uShare']): * * Recombination alone is not proof of possession, so two further checks run first. The seed is * bound to `uShare.y`, and the Y shares must carry their VSS commitment `v` — `keyCombine` - * verifies a Y share's secret `u` only when `v` is present, so omitting it skips the check that - * the share is consistent with the public `y` it claims. Genuine shares produced by `keyShare` - * always carry `v`. + * verifies a Y share's secret `u` only when `v` is present, so material without `v` cannot be + * verified at all and throws a distinct error rather than being judged a non-match. Genuine + * shares produced by `keyShare` always carry `v`. + * + * A true result is a consistency check for a caller inspecting its own material, not proof of + * possession, and must never be used as an authorization signal: a caller who knows only the + * wallet's public commonKeychain can construct material that passes — an `uShare` whose seed + * matches its `y`, plus Y shares with a chosen secret `u` and the `v` solved from it, whose + * `y` values sum to the common key. * * @param params.prv - stringified user signing material, as passed as `prv` when signing * @param params.commonKeychain - the wallet's user keychain commonKeychain @@ -145,9 +151,10 @@ export async function eddsaUserSigningMaterialMatchesCommonKeychain(params: { return false; } // keyCombine verifies a Y share's secret `u` against its commitment only when `v` is present, - // so material without it would skip that check entirely + // so material without `v` cannot be verified at all; reject it distinctly instead of judging + // it a non-match, which would misreport genuine material that lacks the commitment if (signingMaterial.bitgoYShare.v === undefined || signingMaterial.backupYShare.v === undefined) { - return false; + throw new Error('Unable to verify key - signing material has no VSS commitment'); } let combinedKey: KeyCombine; try { diff --git a/modules/sdk-core/src/bitgo/wallet/iWallet.ts b/modules/sdk-core/src/bitgo/wallet/iWallet.ts index d4f3fd66734..8bdec9c52c3 100644 --- a/modules/sdk-core/src/bitgo/wallet/iWallet.ts +++ b/modules/sdk-core/src/bitgo/wallet/iWallet.ts @@ -469,7 +469,10 @@ export interface VerifyKeyOptions { } export interface VerifyKeyResult { - /** Whether the signing material recombines to the wallet's commonKeychain */ + /** + * Whether the signing material recombines to the wallet's commonKeychain. A consistency + * check, not proof of possession — never use it as an authorization signal. + */ match: boolean; } diff --git a/modules/sdk-core/src/bitgo/wallet/wallet.ts b/modules/sdk-core/src/bitgo/wallet/wallet.ts index 26031b41fdd..f9782f643ad 100644 --- a/modules/sdk-core/src/bitgo/wallet/wallet.ts +++ b/modules/sdk-core/src/bitgo/wallet/wallet.ts @@ -2731,6 +2731,9 @@ export class Wallet implements IWallet { * instead of discovering a mismatch only when a signature fails. Supported for TSS EdDSA * (MPCv1) wallets; other wallet types are rejected. * + * The result is a consistency check for a caller inspecting its own material, not proof of + * possession, and must never be used as an authorization signal. + * * `walletPassphrase` is deliberately not accepted: decrypting the BitGo-held `encryptedPrv` * and comparing it against the BitGo-held keychain is circular, and callers that hold their * own key material do not need it. diff --git a/modules/sdk-core/test/unit/bitgo/utils/tss/keyVerification.ts b/modules/sdk-core/test/unit/bitgo/utils/tss/keyVerification.ts index 8e4a1cf5b7a..ba5f660e3af 100644 --- a/modules/sdk-core/test/unit/bitgo/utils/tss/keyVerification.ts +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/keyVerification.ts @@ -139,7 +139,7 @@ describe('TSS EdDSA key verification', function () { it('accepts a Y share without the optional v field', function () { // v is optional on the YShare type, so parsing must allow it; verification separately - // requires it as proof of possession (see the VSS commitment case below) + // rejects material without it with a distinct unable-to-verify error (see below) const { uShare, bitgoYShare, backupYShare } = parseEddsaUserSigningMaterial(matchingPrv); const withoutV = { ...bitgoYShare }; delete withoutV.v; @@ -192,9 +192,10 @@ describe('TSS EdDSA key verification', function () { match.should.equal(false); }); - it('returns false when a Y share omits its VSS commitment', async function () { - // keyCombine only verifies a Y share's secret u when v is present, so material that drops v - // would otherwise pass with an arbitrary u + it('rejects material that omits a VSS commitment instead of judging it a non-match', async function () { + // keyCombine only verifies a Y share's secret u when v is present, so material that drops + // v would otherwise pass combine with an arbitrary u; it cannot be verified at all and + // must not be reported as a clean non-match const { uShare, bitgoYShare, backupYShare } = parseEddsaUserSigningMaterial(matchingPrv); const bitgoWithoutV = { ...bitgoYShare, u: 'cd'.repeat(32) }; delete bitgoWithoutV.v; @@ -203,8 +204,21 @@ describe('TSS EdDSA key verification', function () { bitgoYShare: bitgoWithoutV, backupYShare, }); - const match = await eddsaUserSigningMaterialMatchesCommonKeychain({ prv: forged, commonKeychain }); - match.should.equal(false); + await assert.rejects(eddsaUserSigningMaterialMatchesCommonKeychain({ prv: forged, commonKeychain }), { + message: 'Unable to verify key - signing material has no VSS commitment', + }); + }); + + it('rejects otherwise-genuine material without a VSS commitment as unverifiable', async function () { + // shares without v may be genuine (the server did not always return vssProof), so the + // answer must be "cannot verify", never "not this wallet's key" + const { uShare, bitgoYShare, backupYShare } = parseEddsaUserSigningMaterial(matchingPrv); + const bitgoWithoutV = { ...bitgoYShare }; + delete bitgoWithoutV.v; + const vless = JSON.stringify({ uShare, bitgoYShare: bitgoWithoutV, backupYShare }); + await assert.rejects(eddsaUserSigningMaterialMatchesCommonKeychain({ prv: vless, commonKeychain }), { + message: 'Unable to verify key - signing material has no VSS commitment', + }); }); it('returns false when a Y share carries a mismatched secret with its commitment present', async function () {