From e9c1617fc00dbfc7c80c1c047fa4998654a8f089 Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Fri, 20 Dec 2024 08:29:16 +0100 Subject: [PATCH] fix: Sanitize `signTypedDatav3v4` params before calling security API (#12789) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR aims to filter request params before calling security API call if method is `signTypedDatav3v4` ## **Related issues** Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3830 ## **Manual testing steps** 1. Use following payload in the local test-dapp sign typed data v3/v4 ``` // Request the current account addresses from the Ethereum provider const addresses = await window.ethereum.request({ "method": "eth_accounts" }); // Construct the JSON string for eth_signTypedData_v4, including the dynamic owner address const jsonData = { domain: { name: "USD Coin", version: "2", chainId: "1", verifyingContract: "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48" }, types: { EIP712Domain: [ { name: "name", type: "string" }, { name: "version", type: "string" }, { name: "chainId", type: "uint256" }, { name: "verifyingContract", type: "address" } ], Permit: [ { name: "owner", type: "address" }, { name: "spender", type: "address" }, { name: "value", type: "uint256" }, { name: "nonce", type: "uint256" }, { name: "deadline", type: "uint256" } ] }, primaryType: "Permit", message: { owner: addresses[0], spender: "0xa2d86c5ff6fbf5f455b1ba2737938776c24d7a58", value: "115792089237316195423570985008687907853269984665640564039457584007913129639935", nonce: "0", deadline: "115792089237316195423570985008687907853269984665640564039457584007913129639935" } }; ``` 2. Notice that the transaction is considered as malicious (which was not flagged before) ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [X] I've completed the PR template to the best of my ability - [X] I’ve included tests if applicable - [X] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [X] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- app/lib/ppom/ppom-util.test.ts | 42 +++++++++++++++++++++++++++++++++- app/lib/ppom/ppom-util.ts | 27 +++++++++++++++++++--- 2 files changed, 65 insertions(+), 4 deletions(-) diff --git a/app/lib/ppom/ppom-util.test.ts b/app/lib/ppom/ppom-util.test.ts index 89faa23a585..6d0ab6bc03d 100644 --- a/app/lib/ppom/ppom-util.test.ts +++ b/app/lib/ppom/ppom-util.test.ts @@ -3,7 +3,10 @@ import * as SignatureRequestActions from '../../actions/signatureRequest'; // es import * as TransactionActions from '../../actions/transaction'; // eslint-disable-line import/no-namespace import * as NetworkControllerSelectors from '../../selectors/networkController'; // eslint-disable-line import/no-namespace import Engine from '../../core/Engine'; -import PPOMUtil from './ppom-util'; +import PPOMUtil, { + METHOD_SIGN_TYPED_DATA_V3, + METHOD_SIGN_TYPED_DATA_V4, +} from './ppom-util'; // eslint-disable-next-line import/no-namespace import * as securityAlertAPI from './security-alerts-api'; import { isBlockaidFeatureEnabled } from '../../util/blockaid'; @@ -22,6 +25,10 @@ import Logger from '../../util/Logger'; const CHAIN_ID_MOCK = '0x1'; +const SIGN_TYPED_DATA_PARAMS_MOCK_1 = '0x123'; +const SIGN_TYPED_DATA_PARAMS_MOCK_2 = + '{"primaryType":"Permit","domain":{},"types":{}}'; + jest.mock('./security-alerts-api'); jest.mock('../../util/blockaid'); @@ -439,5 +446,38 @@ describe('PPOM Utils', () => { source: SecurityAlertSource.Local, }); }); + + it.each([METHOD_SIGN_TYPED_DATA_V3, METHOD_SIGN_TYPED_DATA_V4])( + 'sanitizes request params if method is %s', + async (method: string) => { + isSecurityAlertsEnabledMock.mockReturnValue(true); + getSupportedChainIdsMock.mockResolvedValue([CHAIN_ID_MOCK]); + + const firstTwoParams = [ + SIGN_TYPED_DATA_PARAMS_MOCK_1, + SIGN_TYPED_DATA_PARAMS_MOCK_2, + ]; + + const unwantedParams = [{}, undefined, 1, null]; + + const params = [...firstTwoParams, ...unwantedParams]; + + const request = { + ...mockRequest, + method, + params, + }; + await PPOMUtil.validateRequest(request, CHAIN_ID_MOCK); + + expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledTimes(1); + expect(validateWithSecurityAlertsAPIMock).toHaveBeenCalledWith( + CHAIN_ID_MOCK, + { + ...request, + params: firstTwoParams, + }, + ); + }, + ); }); }); diff --git a/app/lib/ppom/ppom-util.ts b/app/lib/ppom/ppom-util.ts index 3a7716eb87f..b76d4bf8f4b 100644 --- a/app/lib/ppom/ppom-util.ts +++ b/app/lib/ppom/ppom-util.ts @@ -34,6 +34,8 @@ export interface PPOMRequest { const TRANSACTION_METHOD = 'eth_sendTransaction'; const TRANSACTION_METHODS = [TRANSACTION_METHOD, 'eth_sendRawTransaction']; +export const METHOD_SIGN_TYPED_DATA_V3 = 'eth_signTypedData_v3'; +export const METHOD_SIGN_TYPED_DATA_V4 = 'eth_signTypedData_v4'; const CONFIRMATION_METHODS = Object.freeze([ 'eth_sendRawTransaction', @@ -155,7 +157,7 @@ async function validateWithController( ppomController: PPOMController, request: PPOMRequest, ): Promise { - try{ + try { const response = (await ppomController.usePPOM((ppom) => ppom.validateJsonRpc(request as unknown as Record), )) as SecurityAlertResponse; @@ -166,7 +168,10 @@ async function validateWithController( }; } catch (e) { Logger.log(`Error validating request with PPOM: ${e}`); - return {...SECURITY_ALERT_RESPONSE_FAILED, source: SecurityAlertSource.Local,}; + return { + ...SECURITY_ALERT_RESPONSE_FAILED, + source: SecurityAlertSource.Local, + }; } } @@ -212,9 +217,25 @@ function isTransactionRequest(request: PPOMRequest) { return TRANSACTION_METHODS.includes(request.method); } +function sanitizeRequest(request: PPOMRequest): PPOMRequest { + // This is a temporary fix to prevent a PPOM bypass + if ( + request.method === METHOD_SIGN_TYPED_DATA_V4 || + request.method === METHOD_SIGN_TYPED_DATA_V3 + ) { + if (Array.isArray(request.params)) { + return { + ...request, + params: request.params.slice(0, 2), + }; + } + } + return request; +} + function normalizeRequest(request: PPOMRequest): PPOMRequest { if (request.method !== TRANSACTION_METHOD) { - return request; + return sanitizeRequest(request); } request.origin = request.origin