Skip to content

Commit

Permalink
fix: Sanitize signTypedDatav3v4 params before calling security API (#…
Browse files Browse the repository at this point in the history
…12789)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

This PR aims to filter request params before calling security API call
if method is `signTypedDatav3v4`

## **Related issues**

Fixes: MetaMask/MetaMask-planning#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**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **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.
  • Loading branch information
OGPoyraz authored Dec 20, 2024
1 parent 3b17cd3 commit e9c1617
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 4 deletions.
42 changes: 41 additions & 1 deletion app/lib/ppom/ppom-util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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');

Expand Down Expand Up @@ -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,
},
);
},
);
});
});
27 changes: 24 additions & 3 deletions app/lib/ppom/ppom-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -155,7 +157,7 @@ async function validateWithController(
ppomController: PPOMController,
request: PPOMRequest,
): Promise<SecurityAlertResponse> {
try{
try {
const response = (await ppomController.usePPOM((ppom) =>
ppom.validateJsonRpc(request as unknown as Record<string, unknown>),
)) as SecurityAlertResponse;
Expand All @@ -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,
};
}
}

Expand Down Expand Up @@ -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
Expand Down

0 comments on commit e9c1617

Please sign in to comment.