Disable eth_sign by default, allow users to toggle it back on #17308

409H · 2023-01-20T02:27:45Z

Explanation

After looking at various phishing kits (and de-obfuscating), there are enormous trends between different kits that they all abuse eth_sign to ask users to inadvertently sign a transaction (as MetaMask UI is opaque to an on-chain transaction being signed). You can read more about the attack here.

I'd estimate that the abuse of eth_sign is near equal to the abuse of Wyvern and Seaport contracts in a manner to steal NFTs through creating orders for 0 eth to a specific beneficiary.

In short: eth_sign has been consistently abused by phishers/scammers to steal funds and the case may be that only power-users who use very legacy dapps may need to still use eth_sign.

Disable eth_sign by default - throw an ethErrors.rpc.methodNotFound exception
Allow the user to enable eth_sign through "Advanced Settings"

RPC Method preferences are stored within rpcMethodPreferences in the store state, allow for us to develop this further if someone wants to lock down rpc calls into MetaMask

Screenshots/Screencaps

Manual Testing Steps

Go to https://metamask.github.io/test-dapp/
Open Developer Tools
Click "Sign" button under the heading "Eth Sign"
See the exception thrown and no prompt to sign within MetaMask
Go into MetaMask > Settings > Advanced Settings
Toggle eth_sign to be enabled
goto: 1

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

…n advanced settings to renable support for eth_sign

github-actions · 2023-01-20T02:28:04Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

holantonela · 2023-01-20T12:51:05Z

The content of the toggle should be:

Title: Allow eth_sign requests
Description: Turn this on to enable dapps to request you eth_sign requests. eth_sign is an open-ended signing method that allows signing an arbitrary hash, which means it can be used to sign transactions, or any other data, making it a dangerous phishing risk. Only sign eth_sign requests if you are able to read what you are signing and you trust the origin.
Default: OFF

@coreyjanssen could we have your review here?

coreyjanssen · 2023-01-20T15:52:00Z

revised content here (made it a bit more user-friendly):

Title: Allow eth_sign requests

Description: Turn this on to let dapps request your signature using eth_sign requests. eth_sign is an open-ended signing method that lets you sign an arbitrary hash, making it a dangerous phishing risk. Only sign eth_sign requests if you can read what you are signing and trust the origin of the request.

Default: OFF

@holantonela

409H · 2023-01-20T16:16:45Z

@holantonela @coreyjanssen thank you, i have pushed the copy change in 37d0ba3

…hin store

jpuri · 2023-01-24T18:11:35Z

I think if an eth_sign request comes to extension we should show message to user to enable it in settings, else it may be broken experience for those we are using it currently.

app/scripts/controllers/preferences.js

holantonela · 2023-01-24T19:14:18Z

I think if an eth_sign request comes to extension we should show message to user to enable it in settings, else it may be broken experience for those we are using it currently.

Good point. But, if we ask users to enable it the right away, we are missing the point of making it optional. After we release, we can learn more things about how users use or not eth_sign.

In the case we want to prompt users to change defaults, we can offer users the option to save the origin domain in a safe list so if they trust the origin, they allow them to trigger eth_sign requests even when it is disabled (and other perks too). This should be a local list managed in Settings > General by all the accounts.

…sign * origin/develop: (142 commits) Use network provider state, instead of CurrencyRateController state, to select 'nativeCurrency' (MetaMask#17450) [e2e]Add e2e test for deleting custom network (MetaMask#17254) Fix MetaMask#17362 - Ensure NFT icons are correct (MetaMask#17440) Fix MetaMask#17385 - Provide autohide callback for ActionMessage (MetaMask#17392) button link housekeeping (MetaMask#16885) Bump lavapack version to 5 (MetaMask#17431) feature: convert shared/modules/contract-utils.test.js to typescript (MetaMask#17435) Refactor confirm page container component from class to functional component (MetaMask#16907) Test parameter change cache miss (MetaMask#17346) Document callback-style background calls as deprecated (MetaMask#17376) Improving getMemoizedMetadataContractName selector (MetaMask#17432) [GridPlus] Bumps `gridplus-sdk` to v2.4.1 (MetaMask#16847) Put hardware wallets behind an HARDWARE_WALLETS_MV3 flag (MetaMask#17354) Consolidated all component import paths (MetaMask#17368) updated new links for the stories (MetaMask#17369) Fix MetaMask#17388 - Remove dismiss button from NFT notification (MetaMask#17389) Remove a notification for falling back from STX to regular swaps (MetaMask#17374) Feat/15086/add banner severities component (MetaMask#17307) fix icon names (MetaMask#17391) Fix Playwright install step (MetaMask#17415) ...

holantonela · 2023-01-30T14:55:15Z

This PR closes #17491

jpuri

Looks good 👍

seaona · 2023-01-31T15:02:10Z

test/e2e/tests/eth-sign.spec.js

@@ -31,10 +59,18 @@ describe('Eth sign', function () {
        await driver.fill('#password', 'correct horse battery staple');
        await driver.press('#password', driver.Key.ENTER);

+        // Enable eth_sign


Instead of enabling eth_sign with Settings, we could skip all the steps below by enabling it from the state. This will make the test faster and complies with the standards followed on e2e.

Detailed explanation: on fixture-builder file, we could add the state for eth_sign and set it to true on a function below, in the same way it's done with other preferences.

Then on this test, we can just enable that fixture like this example:

.withPreferencesController({ preferences: { showFiatInTestnets: true, },

Thanks @seaona - changed in c51da5e

Wait for the notification popup to close, leaving 2 window handles the extension and the test dapp

Enable eth_sign method

409H added 3 commits January 20, 2023 02:07

Added translation for eth sign toggle

b2d12fa

Disabled the ability to call eth_sign by default. Added ability withi…

fb05c17

…n advanced settings to renable support for eth_sign

Add test case for eth_sign being enabled and disabled

d6d25e4

409H requested a review from a team as a code owner January 20, 2023 02:27

409H requested a review from segun January 20, 2023 02:27

409H changed the title ~~Feature/toggle ethsign~~ Disable eth_sign by default, allow users to toggle it back on Jan 20, 2023

Modified copy

37d0ba3

409H added 6 commits January 20, 2023 16:20

Moved rpc method preference to its own object within store

e293176

Complete work on moving rpc method preference into its own object wit…

9e4c1d3

…hin store

Fix with prettier

5e24c02

Fix lint

a80b31c

Fix a unit test

3d85ba4

Fix test

9dda6f0

holantonela mentioned this pull request Jan 24, 2023

Improve warning text for eth_sign #11337

Closed

4 tasks

jpuri reviewed Jan 24, 2023

View reviewed changes

app/scripts/controllers/preferences.js Show resolved Hide resolved

409H added 3 commits January 25, 2023 19:11

Renamed function and object keys to be more intuitive

aece3d9

Fix e2e test

04a3997

holantonela added team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead type-security and removed type-security team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels Jan 30, 2023

holantonela linked an issue Jan 30, 2023 that may be closed by this pull request

Disable eth_sign by default #17491

Closed

jpuri previously approved these changes Jan 31, 2023

View reviewed changes

seaona reviewed Jan 31, 2023

View reviewed changes

danjm added the release-blocker This bug is blocking the next release label Feb 2, 2023

bschorchit mentioned this pull request Feb 3, 2023

Disable eth_sign by default MetaMask/metamask-mobile#5676

Closed

Enabled eth_sign through preferences fixture

c51da5e

409H dismissed jpuri’s stale review via c51da5e February 3, 2023 16:23

409H and others added 2 commits February 3, 2023 16:25

Merge branch 'develop' into feature/toggle-ethsign

780bddf

Fix lint

36513b1

jpuri previously approved these changes Feb 5, 2023

View reviewed changes

Fix e2e test

f681030

Wait for the notification popup to close, leaving 2 window handles the extension and the test dapp

PeterYinusa dismissed jpuri’s stale review via f681030 February 6, 2023 11:59

PeterYinusa and others added 5 commits February 6, 2023 12:31

Fix e2e test

3159e7a

Fix unit test

649ed18

Enable eth_sign method

Merge branch 'develop' into feature/toggle-ethsign

b8f2453

Lint fix

0053de2

Merge branch 'develop' into feature/toggle-ethsign

eabb212

brad-decker approved these changes Feb 6, 2023

View reviewed changes

danjm approved these changes Feb 6, 2023

View reviewed changes

danjm merged commit b4ecc4c into MetaMask:develop Feb 6, 2023

github-actions bot locked and limited conversation to collaborators Feb 6, 2023

PeterYinusa added the rc-cherry-picked label Feb 6, 2023

409H deleted the feature/toggle-ethsign branch April 8, 2023 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable eth_sign by default, allow users to toggle it back on #17308

Disable eth_sign by default, allow users to toggle it back on #17308

409H commented Jan 20, 2023 •

edited by wzrdk3lly

Loading

github-actions bot commented Jan 20, 2023

holantonela commented Jan 20, 2023 •

edited

Loading

coreyjanssen commented Jan 20, 2023

409H commented Jan 20, 2023

jpuri commented Jan 24, 2023

holantonela commented Jan 24, 2023

holantonela commented Jan 30, 2023

jpuri left a comment

seaona Jan 31, 2023

409H Feb 3, 2023

Disable eth_sign by default, allow users to toggle it back on #17308

Disable eth_sign by default, allow users to toggle it back on #17308

Conversation

409H commented Jan 20, 2023 • edited by wzrdk3lly Loading

Explanation

Screenshots/Screencaps

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jan 20, 2023

holantonela commented Jan 20, 2023 • edited Loading

coreyjanssen commented Jan 20, 2023

409H commented Jan 20, 2023

jpuri commented Jan 24, 2023

holantonela commented Jan 24, 2023

holantonela commented Jan 30, 2023

jpuri left a comment

Choose a reason for hiding this comment

seaona Jan 31, 2023

Choose a reason for hiding this comment

409H Feb 3, 2023

Choose a reason for hiding this comment

409H commented Jan 20, 2023 •

edited by wzrdk3lly

Loading

holantonela commented Jan 20, 2023 •

edited

Loading