diff --git a/packages/eth-json-rpc-middleware/CHANGELOG.md b/packages/eth-json-rpc-middleware/CHANGELOG.md index a6823832a4..3e2467f183 100644 --- a/packages/eth-json-rpc-middleware/CHANGELOG.md +++ b/packages/eth-json-rpc-middleware/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Add strict validation for `eth_sendTransaction` and `eth_signTransaction` params — reject requests whose params contain extraneous top-level keys or nest beyond `MAX_TRANSACTION_PARAM_DEPTH` (10). Prevents downstream normalization / PPOM WASM from crashing on deeply-nested junk fields and silently bypassing security scans. ([#9482](https://github.com/MetaMask/core/pull/9482)) - 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..198133b3cd 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_PARAM_DEPTH, resemblesAddress, validateAndNormalizeKeyholder, validateParams, + validateTransactionParams, validateTypedMessageKeys, } from './validation'; @@ -278,4 +280,127 @@ describe('Validation Utils', () => { }); }); }); + + describe('validateTransactionParams', () => { + const VALID_FROM = '0xbe93f9bacbcffc8ee6663f2647917ed7a20a57bb'; + const VALID_TO = '0xdAC17F958D2ee523a2206206994597C13D831ec7'; + + it('does not throw for minimal valid params', () => { + expect(() => + validateTransactionParams({ from: VALID_FROM }), + ).not.toThrow(); + }); + + it('does not throw for the full allowlisted 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 input.'); + }); + + it('throws for an extraneous top-level key', () => { + expect(() => + validateTransactionParams({ + from: VALID_FROM, + to: VALID_TO, + extraKey: 'unexpected', + }), + ).toThrow('Invalid input.'); + }); + + it('throws for the incident repro payload (deeply-nested junk field)', () => { + 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 input.'); + }); + + it('throws when an allowlisted field nests beyond the depth limit', () => { + let deep: Record = { leaf: true }; + for (let i = 0; i < MAX_TRANSACTION_PARAM_DEPTH + 5; i++) { + deep = { nested: deep }; + } + + expect(() => + validateTransactionParams({ + from: VALID_FROM, + data: deep as unknown as string, + }), + ).toThrow('Invalid input.'); + }); + + it('throws when a deeply-nested array exceeds the depth limit', () => { + let deepArray: unknown = 'leaf'; + for (let i = 0; i < MAX_TRANSACTION_PARAM_DEPTH + 5; i++) { + deepArray = [deepArray]; + } + + expect(() => + validateTransactionParams({ + from: VALID_FROM, + accessList: deepArray as never, + }), + ).toThrow('Invalid input.'); + }); + + it('does not throw when params sit exactly at the depth limit', () => { + let deep: unknown = 'leaf'; + for (let i = 0; i < MAX_TRANSACTION_PARAM_DEPTH - 1; i++) { + deep = { nested: deep }; + } + + expect(() => + validateTransactionParams({ + from: VALID_FROM, + data: deep, + }), + ).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..04af3e6c6c 100644 --- a/packages/eth-json-rpc-middleware/src/utils/validation.ts +++ b/packages/eth-json-rpc-middleware/src/utils/validation.ts @@ -234,3 +234,101 @@ export function validateTypedMessageKeys(data: string): void { } } } + +/** + * Top-level keys explicitly permitted on `eth_sendTransaction` and + * `eth_signTransaction` params. Any additional top-level key causes the + * request to be rejected before it reaches downstream consumers such as PPOM + * or the Security Alerts API. + * + * Derived from the dapp-facing subset of `TransactionParams` in + * `@metamask/transaction-controller`. Internal-only fields + * (`estimateGasError`, `estimatedBaseFee`, `estimateSuggested`, + * `estimateUsed`, `gasUsed`) are intentionally omitted — dapps should not be + * able to inject them. + */ +export const ALLOWED_TRANSACTION_PARAM_KEYS = new Set([ + 'accessList', + 'authorizationList', + 'chainId', + 'data', + 'from', + 'gas', + 'gasLimit', + 'gasPrice', + 'maxFeePerGas', + 'maxPriorityFeePerGas', + 'nonce', + 'to', + 'type', + 'value', +]); + +/** + * Maximum nesting depth permitted anywhere inside a transaction params + * object. Legitimate params (including `accessList` and + * `authorizationList`) are at most ~4 levels deep. Anything beyond this is + * treated as a denial-of-service attempt against downstream normalization + * (which recurses and can overflow the call stack in native/WASM code). + */ +export const MAX_TRANSACTION_PARAM_DEPTH = 10; + +/** + * Recursively checks that a value does not nest beyond + * `MAX_TRANSACTION_PARAM_DEPTH`. + * + * @param value - The value to check. + * @param depth - The current depth. Callers should pass `0`. + * @throws rpcErrors.invalidInput() if the value nests too deeply. + */ +function assertMaxDepth(value: unknown, depth: number): void { + if (depth > MAX_TRANSACTION_PARAM_DEPTH) { + throw rpcErrors.invalidInput(); + } + + if (value === null || typeof value !== 'object') { + return; + } + + if (Array.isArray(value)) { + for (const item of value) { + assertMaxDepth(item, depth + 1); + } + return; + } + + for (const key of Object.getOwnPropertyNames( + value as Record, + )) { + assertMaxDepth((value as Record)[key], depth + 1); + } +} + +/** + * Validates that `eth_sendTransaction` / `eth_signTransaction` params contain + * only spec-defined top-level keys and no excessively-nested structures. + * + * This guards against malicious dapps attaching deeply-nested junk fields + * (e.g. `{ from, to, data, test: { b: { b: { b: /* ~1200 levels *\/ } } } }`) + * that would otherwise crash downstream normalization or PPOM WASM with a + * `RangeError: Maximum call stack size exceeded`, bypassing security checks. + * + * @param params - The transaction params object supplied by the dapp. + * @throws rpcErrors.invalidInput() if params is not a plain object, contains + * an extraneous top-level key, or nests beyond `MAX_TRANSACTION_PARAM_DEPTH`. + */ +export function validateTransactionParams(params: unknown): void { + if (params === null || typeof params !== 'object' || Array.isArray(params)) { + throw rpcErrors.invalidInput(); + } + + const hasExtraneousKey = Object.keys(params).some( + (key) => !ALLOWED_TRANSACTION_PARAM_KEYS.has(key), + ); + + if (hasExtraneousKey) { + throw rpcErrors.invalidInput(); + } + + assertMaxDepth(params, 0); +} diff --git a/packages/eth-json-rpc-middleware/src/wallet.test.ts b/packages/eth-json-rpc-middleware/src/wallet.test.ts index a9964c0d3a..25ed91e977 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, @@ -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 input.'); + }); + + it('throws for the incident repro payload with deeply-nested junk', 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 input.'); + }); + it('should not override other request params', async () => { const getAccounts = async (): Promise => testAddresses.slice(0, 2); @@ -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 input.'); + }); + + it('throws for the incident repro payload with deeply-nested junk', 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 input.'); + }); }); 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`.