diff --git a/packages/eth-json-rpc-middleware/CHANGELOG.md b/packages/eth-json-rpc-middleware/CHANGELOG.md index a6823832a4..8a7c2f19fb 100644 --- a/packages/eth-json-rpc-middleware/CHANGELOG.md +++ b/packages/eth-json-rpc-middleware/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **BREAKING:** Add strict validation for `eth_sendTransaction` and `eth_signTransaction` params ([#9482](https://github.com/MetaMask/core/pull/9482)) + - Reject requests whose params do not match the transaction schema (extraneous top-level keys, ill-typed fields such as non-hex `to`/`data`, malformed `accessList` / `authorizationList` entries) or exceed `MAX_TRANSACTION_PARAMS_SIZE_BYTES` when serialized + - Prevents downstream normalization / PPOM WASM from crashing on deeply-nested junk fields or padded payloads and silently bypassing security scans + - Bump `@metamask/utils` from `^11.9.0` to `^11.11.0` ([#9074](https://github.com/MetaMask/core/pull/9074)) - Bump `@metamask/json-rpc-engine` from `^10.2.4` to `^10.5.0` ([#8661](https://github.com/MetaMask/core/pull/8661), [#8746](https://github.com/MetaMask/core/pull/8746), [#8753](https://github.com/MetaMask/core/pull/8753)) - Bump `@metamask/message-manager` from `^14.1.1` to `^14.1.2` ([#8755](https://github.com/MetaMask/core/pull/8755)) diff --git a/packages/eth-json-rpc-middleware/src/index.test.ts b/packages/eth-json-rpc-middleware/src/index.test.ts index ff6ab44352..beece96550 100644 --- a/packages/eth-json-rpc-middleware/src/index.test.ts +++ b/packages/eth-json-rpc-middleware/src/index.test.ts @@ -296,6 +296,7 @@ describe('index module', () => { "createWalletMiddleware": [Function], "providerAsMiddleware": [Function], "providerAsMiddlewareV2": [Function], + "validateTransactionParams": [Function], } `); }); diff --git a/packages/eth-json-rpc-middleware/src/index.ts b/packages/eth-json-rpc-middleware/src/index.ts index 8efff9e55d..a933d1669d 100644 --- a/packages/eth-json-rpc-middleware/src/index.ts +++ b/packages/eth-json-rpc-middleware/src/index.ts @@ -35,4 +35,5 @@ export { } from './methods/wallet-get-supported-execution-permissions'; export * from './providerAsMiddleware'; export * from './retryOnEmpty'; +export { validateTransactionParams } from './utils/validation'; export * from './wallet'; diff --git a/packages/eth-json-rpc-middleware/src/utils/validation.test.ts b/packages/eth-json-rpc-middleware/src/utils/validation.test.ts index 230b476a4c..585162c93d 100644 --- a/packages/eth-json-rpc-middleware/src/utils/validation.test.ts +++ b/packages/eth-json-rpc-middleware/src/utils/validation.test.ts @@ -5,9 +5,11 @@ import { any, validate } from '@metamask/superstruct'; import type { WalletMiddlewareKeyValues } from '../wallet'; import { + MAX_TRANSACTION_PARAMS_SIZE_BYTES, resemblesAddress, validateAndNormalizeKeyholder, validateParams, + validateTransactionParams, validateTypedMessageKeys, } from './validation'; @@ -278,4 +280,190 @@ describe('Validation Utils', () => { }); }); }); + + describe('validateTransactionParams', () => { + const VALID_FROM = '0xbe93f9bacbcffc8ee6663f2647917ed7a20a57bb'; + const VALID_TO = '0xdac17f958d2ee523a2206206994597c13d831ec7'; + + beforeEach(() => { + const actual = jest.requireActual<{ + validate: typeof validate; + }>('@metamask/superstruct'); + validateMock.mockImplementation(actual.validate); + }); + + it('does not throw for minimal valid params', () => { + expect(() => + validateTransactionParams({ from: VALID_FROM }), + ).not.toThrow(); + }); + + it('does not throw for the full valid param set', () => { + expect(() => + validateTransactionParams({ + accessList: [ + { + address: VALID_TO, + storageKeys: ['0x00', '0x01'], + }, + ], + authorizationList: [ + { + chainId: '0x1', + address: VALID_TO, + nonce: '0x0', + }, + ], + chainId: '0x1', + data: '0x095ea7b3', + from: VALID_FROM, + gas: '0x5208', + gasLimit: '0x5208', + gasPrice: '0x1', + maxFeePerGas: '0x2', + maxPriorityFeePerGas: '0x1', + nonce: '0x0', + to: VALID_TO, + type: '0x2', + value: '0x0', + }), + ).not.toThrow(); + }); + + it.each([ + ['null', null], + ['undefined', undefined], + ['a string', 'not-an-object'], + ['a number', 42], + ['a boolean', true], + ['an array', [{ from: VALID_FROM }]], + ])('throws when params is %s', (_label, value) => { + expect(() => validateTransactionParams(value)).toThrow(/Invalid params/u); + }); + + it('throws for an extraneous top-level key', () => { + expect(() => + validateTransactionParams({ + from: VALID_FROM, + to: VALID_TO, + extraKey: 'unexpected', + }), + ).toThrow(/Invalid params/u); + }); + + it('throws when params contain an extraneous key with a deeply-nested value', () => { + let junk: Record = {}; + for (let i = 0; i < 1200; i++) { + junk = { b: junk }; + } + + expect(() => + validateTransactionParams({ + from: VALID_FROM, + to: VALID_TO, + value: '0x0', + data: '0x095ea7b3', + test: junk, + }), + ).toThrow(/Invalid params/u); + }); + + it('rejects an extraneous top-level key without walking its value (JSON.stringify never called)', () => { + const stringifySpy = jest.spyOn(JSON, 'stringify'); + + const params = { + from: VALID_FROM, + to: VALID_TO, + test: { + get b(): never { + throw new Error('subtree must not be walked'); + }, + }, + }; + + let thrown: unknown; + try { + validateTransactionParams(params); + } catch (error) { + thrown = error; + } + const stringifyCallsAtRejection = stringifySpy.mock.calls.length; + stringifySpy.mockRestore(); + + expect(thrown).toBeInstanceOf(Error); + expect(stringifyCallsAtRejection).toBe(0); + }); + + it('throws when a typed field has the wrong type', () => { + expect(() => + validateTransactionParams({ + from: VALID_FROM, + to: { nested: 'not-an-address' }, + }), + ).toThrow(/Invalid params/u); + }); + + it('throws when `data` is not a hex string', () => { + expect(() => + validateTransactionParams({ + from: VALID_FROM, + data: 1234 as unknown as string, + }), + ).toThrow(/Invalid params/u); + }); + + it('throws when `accessList` entries are malformed', () => { + expect(() => + validateTransactionParams({ + from: VALID_FROM, + accessList: [{ address: 'not-hex', storageKeys: 'not-an-array' }], + }), + ).toThrow(/Invalid params/u); + }); + + it('throws for a data-padding attack that passes the schema', () => { + const padded = `0x${'00'.repeat(MAX_TRANSACTION_PARAMS_SIZE_BYTES)}`; + + expect(() => + validateTransactionParams({ + from: VALID_FROM, + to: VALID_TO, + data: padded, + }), + ).toThrow('Invalid input.'); + }); + + it('throws for an accessList-padding attack that passes the schema', () => { + const padded = Array.from( + { length: Math.ceil(MAX_TRANSACTION_PARAMS_SIZE_BYTES / 64) }, + () => ({ + address: VALID_TO, + storageKeys: [`0x${'00'.repeat(32)}`], + }), + ); + + expect(() => + validateTransactionParams({ + from: VALID_FROM, + to: VALID_TO, + accessList: padded, + }), + ).toThrow('Invalid input.'); + }); + + it('does not throw for a legitimate multi-entry accessList well under the size limit', () => { + const entries = Array.from({ length: 16 }, () => ({ + address: VALID_TO, + storageKeys: [`0x${'11'.repeat(32)}`, `0x${'22'.repeat(32)}`], + })); + + expect(() => + validateTransactionParams({ + from: VALID_FROM, + to: VALID_TO, + accessList: entries, + }), + ).not.toThrow(); + }); + }); }); diff --git a/packages/eth-json-rpc-middleware/src/utils/validation.ts b/packages/eth-json-rpc-middleware/src/utils/validation.ts index de3782fa67..96093c6923 100644 --- a/packages/eth-json-rpc-middleware/src/utils/validation.ts +++ b/packages/eth-json-rpc-middleware/src/utils/validation.ts @@ -1,7 +1,13 @@ import { TYPED_MESSAGE_SCHEMA } from '@metamask/eth-sig-util'; import { providerErrors, rpcErrors } from '@metamask/rpc-errors'; import type { Struct, StructError } from '@metamask/superstruct'; -import { validate } from '@metamask/superstruct'; +import { + array, + object, + optional, + string, + validate, +} from '@metamask/superstruct'; import type { Hex } from '@metamask/utils'; import type { WalletMiddlewareContext } from '../wallet'; @@ -234,3 +240,72 @@ export function validateTypedMessageKeys(data: string): void { } } } + +export const TransactionParamsStruct = object({ + accessList: optional( + array(object({ address: string(), storageKeys: array(string()) })), + ), + authorizationList: optional( + array( + object({ + address: string(), + chainId: string(), + nonce: string(), + r: optional(string()), + s: optional(string()), + yParity: optional(string()), + }), + ), + ), + chainId: optional(string()), + data: optional(string()), + from: string(), + gas: optional(string()), + gasLimit: optional(string()), + gasPrice: optional(string()), + maxFeePerGas: optional(string()), + maxPriorityFeePerGas: optional(string()), + nonce: optional(string()), + to: optional(string()), + type: optional(string()), + value: optional(string()), +}); + +// Upper bound derived from the largest valid eth_sendTransaction payload: +// EIP-3860 caps initcode at 49,152 bytes → hex-encoded in 'data' field ≈ 98 KB of JSON. +// 200 KB is ~2× that ceiling, giving clear headroom above any protocol-legal +// transaction while blocking the padding attacks this cap defends against. +// TODO(CONF-1662): tighten once P99 production data is available. +export const MAX_TRANSACTION_PARAMS_SIZE_BYTES = 200 * 1024; + +/** + * Validates `eth_sendTransaction` / `eth_signTransaction` params against the + * standard transaction schema and rejects payloads whose serialized size + * exceeds `MAX_TRANSACTION_PARAMS_SIZE_BYTES`. + * + * Guards against two attack shapes: + * - Structural: extraneous top-level keys or ill-typed fields (e.g. + * `{ from, to, test: { b: { b: ... × 1200 } } }`) that would crash + * downstream normalization / PPOM WASM with `RangeError: Maximum call + * stack size exceeded`, silently bypassing security checks. Superstruct's + * `object()` rejects unknown keys by name without accessing their values, + * so hostile nested subtrees are never traversed. + * - Size: valid-shaped but oversized payloads (e.g. `data` padded with + * millions of hex zeros) that exhaust memory in downstream code. + * + * @param params - The transaction params object supplied by the dapp. + * @throws rpcErrors.invalidParams() if params is an array or exceeds the + * serialized size limit. + * @throws rpcErrors.invalidInput() if params fails schema validation + * (wrong type, extraneous top-level key, or malformed nested field). + */ +export function validateTransactionParams(params: unknown): void { + validateParams(params, TransactionParamsStruct); + + if ( + new TextEncoder().encode(JSON.stringify(params)).byteLength > + MAX_TRANSACTION_PARAMS_SIZE_BYTES + ) { + throw rpcErrors.invalidInput(); + } +} diff --git a/packages/eth-json-rpc-middleware/src/wallet.test.ts b/packages/eth-json-rpc-middleware/src/wallet.test.ts index a9964c0d3a..a7eecb2481 100644 --- a/packages/eth-json-rpc-middleware/src/wallet.test.ts +++ b/packages/eth-json-rpc-middleware/src/wallet.test.ts @@ -1,5 +1,6 @@ import { MessageTypes, TypedMessage } from '@metamask/eth-sig-util'; import { JsonRpcEngineV2 } from '@metamask/json-rpc-engine/v2'; +import type { Json } from '@metamask/utils'; import type { MessageParams, @@ -116,7 +117,7 @@ describe('wallet', () => { params: [txParams], }); await expect(engine.handle(payload)).rejects.toThrow( - new Error('Invalid parameters: must provide an Ethereum address.'), + 'Invalid parameters: must provide an Ethereum address.', ); }); @@ -150,6 +151,64 @@ describe('wallet', () => { ); }); + it('throws when params contain an extraneous top-level key', async () => { + const getAccounts = async (): Promise => + testAddresses.slice(0, 2); + const processTransaction = async (): Promise => testTxHash; + const engine = JsonRpcEngineV2.create({ + middleware: [ + createWalletMiddleware({ getAccounts, processTransaction }), + ], + }); + const payload = { + method: 'eth_sendTransaction', + params: [ + { + from: testAddresses[0], + to: testAddresses[1], + extraKey: 'unexpected', + }, + ], + }; + + await expect( + engine.handle(...createHandleParams(payload)), + ).rejects.toThrow(/Invalid params/u); + }); + + it('throws when params contain deeply nested invalid data', async () => { + const getAccounts = async (): Promise => + testAddresses.slice(0, 2); + const processTransaction = async (): Promise => testTxHash; + const engine = JsonRpcEngineV2.create({ + middleware: [ + createWalletMiddleware({ getAccounts, processTransaction }), + ], + }); + + let junk: Json = {}; + for (let i = 0; i < 1200; i++) { + junk = { b: junk }; + } + + const payload = { + method: 'eth_sendTransaction', + params: [ + { + from: testAddresses[0], + to: testAddresses[1], + value: '0x0', + data: '0x095ea7b3', + test: junk, + }, + ] as Json[], + }; + + await expect( + engine.handle(...createHandleParams(payload)), + ).rejects.toThrow(/Invalid params/u); + }); + it('should not override other request params', async () => { const getAccounts = async (): Promise => testAddresses.slice(0, 2); @@ -257,7 +316,7 @@ describe('wallet', () => { await expect( engine.handle(...createHandleParams(payload)), ).rejects.toThrow( - new Error('Invalid parameters: must provide an Ethereum address.'), + 'Invalid parameters: must provide an Ethereum address.', ); }); @@ -287,6 +346,64 @@ describe('wallet', () => { 'The requested account and/or method has not been authorized by the user.', ); }); + + it('throws when params contain an extraneous top-level key', async () => { + const getAccounts = async (): Promise => + testAddresses.slice(0, 2); + const processSignTransaction = async (): Promise => testTxHash; + const engine = JsonRpcEngineV2.create({ + middleware: [ + createWalletMiddleware({ getAccounts, processSignTransaction }), + ], + }); + const payload = { + method: 'eth_signTransaction', + params: [ + { + from: testAddresses[0], + to: testAddresses[1], + extraKey: 'unexpected', + }, + ], + }; + + await expect( + engine.handle(...createHandleParams(payload)), + ).rejects.toThrow(/Invalid params/u); + }); + + it('throws when params contain deeply nested invalid data', async () => { + const getAccounts = async (): Promise => + testAddresses.slice(0, 2); + const processSignTransaction = async (): Promise => testTxHash; + const engine = JsonRpcEngineV2.create({ + middleware: [ + createWalletMiddleware({ getAccounts, processSignTransaction }), + ], + }); + + let junk: Json = {}; + for (let i = 0; i < 1200; i++) { + junk = { b: junk }; + } + + const payload = { + method: 'eth_signTransaction', + params: [ + { + from: testAddresses[0], + to: testAddresses[1], + value: '0x0', + data: '0x095ea7b3', + test: junk, + }, + ] as Json[], + }; + + await expect( + engine.handle(...createHandleParams(payload)), + ).rejects.toThrow(/Invalid params/u); + }); }); describe('signTypedData', () => { diff --git a/packages/eth-json-rpc-middleware/src/wallet.ts b/packages/eth-json-rpc-middleware/src/wallet.ts index b220be16e0..eed733a639 100644 --- a/packages/eth-json-rpc-middleware/src/wallet.ts +++ b/packages/eth-json-rpc-middleware/src/wallet.ts @@ -23,6 +23,7 @@ import { normalizeTypedMessage, parseTypedMessage } from './utils/normalize'; import { resemblesAddress, validateAndNormalizeKeyholder as validateKeyholder, + validateTransactionParams, validateTypedDataForPrototypePollution, validateTypedDataV1ForPrototypePollution, validateTypedMessageKeys, @@ -249,6 +250,7 @@ export function createWalletMiddleware({ } const params = request.params[0] as TransactionParams | undefined; + validateTransactionParams(params); const txParams: TransactionParams = { ...params, // Not using nullish coalescing, since `params` may be `null`. @@ -282,6 +284,7 @@ export function createWalletMiddleware({ } const params = request.params[0] as TransactionParams | undefined; + validateTransactionParams(params); const txParams: TransactionParams = { ...params, // Not using nullish coalescing, since `params` may be `null`.