From fa692b3af04c038a3a261df432abcf8be5259f55 Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Wed, 21 Feb 2024 15:17:43 +0100 Subject: [PATCH] Enforce JSON-RPC response size limits (#2201) Enforces a size limit on JSON-RPC responses from Snaps, throwing an error if the object is larger than 64 MB. 64 MB was chosen because it seems to be the limit set by Chrome for postMessage between the extension and dapps: https://source.chromium.org/chromium/chromium/src/+/main:extensions/renderer/api/messaging/messaging_util.cc;l=65?q=%22Message%20length%20exceeded%20maximum%20allowed%20length%22&ss=chromium --------- Co-authored-by: Maarten Zuidhoorn --- .../coverage.json | 8 ++-- .../common/BaseSnapExecutor.test.browser.ts | 38 +++++++++++++++++++ .../src/common/BaseSnapExecutor.ts | 19 +++++----- .../src/common/utils.test.ts | 20 ++++++++++ .../src/common/utils.ts | 34 ++++++++++++++++- 5 files changed, 104 insertions(+), 15 deletions(-) diff --git a/packages/snaps-execution-environments/coverage.json b/packages/snaps-execution-environments/coverage.json index 183928de47..3df0bef368 100644 --- a/packages/snaps-execution-environments/coverage.json +++ b/packages/snaps-execution-environments/coverage.json @@ -1,6 +1,6 @@ { - "branches": 81.04, - "functions": 90.13, - "lines": 90.73, - "statements": 90.11 + "branches": 80.66, + "functions": 90.19, + "lines": 90.83, + "statements": 90.2 } diff --git a/packages/snaps-execution-environments/src/common/BaseSnapExecutor.test.browser.ts b/packages/snaps-execution-environments/src/common/BaseSnapExecutor.test.browser.ts index bdf74d29bc..e51710d6c3 100644 --- a/packages/snaps-execution-environments/src/common/BaseSnapExecutor.test.browser.ts +++ b/packages/snaps-execution-environments/src/common/BaseSnapExecutor.test.browser.ts @@ -2007,6 +2007,44 @@ describe('BaseSnapExecutor', () => { }); }); + it('throws when trying to respond with value that is too large', async () => { + const CODE = ` + module.exports.onRpcRequest = () => '1'.repeat(100_000_000); + `; + + const executor = new TestSnapExecutor(); + await executor.executeSnap(1, MOCK_SNAP_ID, CODE, []); + + expect(await executor.readCommand()).toStrictEqual({ + jsonrpc: '2.0', + id: 1, + result: 'OK', + }); + + await executor.writeCommand({ + jsonrpc: '2.0', + id: 2, + method: 'snapRpc', + params: [ + MOCK_SNAP_ID, + HandlerType.OnRpcRequest, + MOCK_ORIGIN, + { jsonrpc: '2.0', method: '', params: [] }, + ], + }); + + expect(await executor.readCommand()).toStrictEqual({ + jsonrpc: '2.0', + id: 2, + error: { + code: -32603, + message: + 'JSON-RPC responses must be JSON serializable objects smaller than 64 MB.', + stack: expect.any(String), + }, + }); + }); + it('throws when receiving an invalid RPC request', async () => { const executor = new TestSnapExecutor(); diff --git a/packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts b/packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts index 8f0f266408..5fe1459dbf 100644 --- a/packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts +++ b/packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts @@ -26,8 +26,6 @@ import type { Json, } from '@metamask/utils'; import { - isObject, - isValidJson, assert, isJsonRpcRequest, hasProperty, @@ -49,6 +47,7 @@ import { sanitizeRequestArguments, proxyStreamProvider, withTeardown, + isValidResponse, } from './utils'; import { ExecuteSnapRequestArgumentsStruct, @@ -301,27 +300,27 @@ export class BaseSnapExecutor { }); } - async #notify(requestObject: Omit) { - if (!isValidJson(requestObject) || !isObject(requestObject)) { + async #notify(notification: Omit) { + if (!isValidResponse(notification)) { throw rpcErrors.internal( - 'JSON-RPC notifications must be JSON serializable objects', + 'JSON-RPC notifications must be JSON serializable objects smaller than 64 MB.', ); } await this.#write({ - ...requestObject, + ...notification, jsonrpc: '2.0', }); } - async #respond(id: JsonRpcId, requestObject: Record) { - if (!isValidJson(requestObject) || !isObject(requestObject)) { + async #respond(id: JsonRpcId, response: Record) { + if (!isValidResponse(response)) { // Instead of throwing, we directly respond with an error. // This prevents an issue where we wouldn't respond when errors were non-serializable await this.#write({ error: serializeError( rpcErrors.internal( - 'JSON-RPC responses must be JSON serializable objects.', + 'JSON-RPC responses must be JSON serializable objects smaller than 64 MB.', ), ), id, @@ -331,7 +330,7 @@ export class BaseSnapExecutor { } await this.#write({ - ...requestObject, + ...response, id, jsonrpc: '2.0', }); diff --git a/packages/snaps-execution-environments/src/common/utils.test.ts b/packages/snaps-execution-environments/src/common/utils.test.ts index b6883ccddb..2acacc371a 100644 --- a/packages/snaps-execution-environments/src/common/utils.test.ts +++ b/packages/snaps-execution-environments/src/common/utils.test.ts @@ -2,6 +2,7 @@ import { BLOCKED_RPC_METHODS, assertEthereumOutboundRequest, assertSnapOutboundRequest, + isValidResponse, } from './utils'; describe('assertSnapOutboundRequest', () => { @@ -77,3 +78,22 @@ describe('assertEthereumOutboundRequest', () => { ); }); }); + +describe('isValidResponse', () => { + it('returns false if the value is not an object', () => { + // @ts-expect-error Intentionally using bad params + expect(isValidResponse('foo')).toBe(false); + }); + + it('returns false if the value is not JSON serializable', () => { + expect(isValidResponse({ foo: BigInt(0) })).toBe(false); + }); + + it('returns false if the value is too large', () => { + expect(isValidResponse({ foo: '1'.repeat(100_000_000) })).toBe(false); + }); + + it('returns true if the value is a valid JSON object', () => { + expect(isValidResponse({ foo: 'bar' })).toBe(true); + }); +}); diff --git a/packages/snaps-execution-environments/src/common/utils.ts b/packages/snaps-execution-environments/src/common/utils.ts index 4786155721..471139a54b 100644 --- a/packages/snaps-execution-environments/src/common/utils.ts +++ b/packages/snaps-execution-environments/src/common/utils.ts @@ -1,9 +1,21 @@ import type { StreamProvider } from '@metamask/providers'; import type { RequestArguments } from '@metamask/providers/dist/BaseProvider'; import { rpcErrors } from '@metamask/rpc-errors'; -import { assert, assertStruct, getSafeJson, JsonStruct } from '@metamask/utils'; +import { + assert, + assertStruct, + getJsonSize, + getSafeJson, + isObject, + JsonStruct, +} from '@metamask/utils'; import { log } from '../logging'; + +// 64 MB - we chose this number because it is the size limit for postMessage +// between the extension and the dapp enforced by Chrome. +const MAX_RESPONSE_JSON_SIZE = 64_000_000; + /** * Make proxy for Promise and handle the teardown process properly. * If the teardown is called in the meanwhile, Promise result will not be @@ -174,3 +186,23 @@ export function sanitizeRequestArguments(value: unknown): RequestArguments { const json = JSON.parse(JSON.stringify(value)); return getSafeJson(json) as RequestArguments; } + +/** + * Check if the input is a valid response. + * + * @param response - The response. + * @returns True if the response is valid, otherwise false. + */ +export function isValidResponse(response: Record) { + if (!isObject(response)) { + return false; + } + + try { + // If the JSON is invalid this will throw and we should return false. + const size = getJsonSize(response); + return size < MAX_RESPONSE_JSON_SIZE; + } catch { + return false; + } +}