Integrate LavaDome private key protection by weizman · Pull Request #22381 · MetaMask/metamask-extension

weizman · 2023-12-21T20:06:01Z

Description

New LavaDome tool can help to better hide the private key from potential malicious code running in the app when the user asks to see it.

Related issues

None

Manual testing steps

This PR changes the "show private key" dialog, not in terms of appearance, and not in terms of interaction but only in terms of "behind the scenes" security, so nothing should change in terms of manual interaction with this feature - that is the most important thing to check here QA-wise.

One thing that is changed, is that the private key text is no longer selectable as it was before, and now the only way to copy it is to use the copy-to-clipboard button.

That is on purpose, allowing selection of sensitive information such as the private key is unsafe, and disablement of selection capabilities of sensitive info is done intentionally to enhance security.

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

socket-security · 2023-12-21T20:15:00Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@lavamoat/lavadome-core@0.0.10	None	`0`	22.5 kB	weizman
npm/@lavamoat/lavadome-react@0.0.10	Transitive: environment	`+9`	3.61 MB	weizman

View full report↗︎

socket-security · 2023-12-21T20:15:03Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

…tamask-extension into weizman/try-lavadome

weizman · 2024-01-09T16:36:26Z

Once LavaMoat/LavaDome#18 is merged and published, eab7582 should work too

metamaskbot · 2024-01-29T05:57:29Z

Builds ready [65c6f87]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (852 ± 46 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	97	154	118	16	8
		domContentLoaded	10	55	20	13	6
		load	736	1184	852	97	46
		domInteractive	10	55	20	13	6

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 156 Bytes (0.00%)
ui: 4.29 KiB (0.06%)
common: 0 Bytes (0.00%)

danjm

The code changes look good to me. I will approve when we get another QA pass

danjm · 2024-01-31T17:21:31Z

actually, I will QA this myself

danjm · 2024-01-31T17:33:39Z

QA passed, all following scenarios have been tested:

user can still reveal their private key
user can still copy their private key using the copy button
user cannot copy their private key any other way (i.e. the can't select the text and copy it)
private key cannot be seen if the html is inspected with the browser tools

These tests continue to pass, and I can also see the implemented message upon clicking the private key

metamaskbot · 2024-01-31T17:52:16Z

Builds ready [1394bad]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (798 ± 55 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	85	168	112	19	9
		domContentLoaded	9	45	20	11	5
		load	704	1206	798	114	55
		domInteractive	9	45	19	11	5

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 156 Bytes (0.00%)
ui: 5.07 KiB (0.07%)
common: 0 Bytes (0.00%)

georgewrmarshall

LGTM! Tested and it works well. Nice use of component-library components. Left 2 non-blocking comments

georgewrmarshall · 2024-01-31T23:02:13Z

ui/components/multichain/account-details/account-details-key.js

          style={{ wordBreak: 'break-word' }}
        >
-          {privateKey}
+          <span onClick={() => setShowDisableSelectWarn(true)}>


nit: could we reduce html bloat and increase the click area of the text slightly by moving the onClick to the wrapping Text component?

georgewrmarshall · 2024-01-31T23:04:05Z

ui/components/multichain/account-details/account-details-key.js

-          onClick={() => handlePrivateKeyCopy(privateKey)}
+          onClick={() =>
+            setShowDisableSelectWarn(false) || handlePrivateKeyCopy(privateKey)
+          }


non-blocking: To improve the accessibility here we could add an ariaLabel={t('copyPrivateKey')} which would need to be created in messages.json. ariaLabel is a a required prop for the ButtonIcon component

try lavadome

179f94a

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Dec 21, 2023

weizman added 2 commits December 21, 2023 22:55

mistake

041140a

bump

3a7a3fe

weizman changed the title ~~wip try lavadome~~ WIP - try integrating LavaDome Dec 22, 2023

weizman added 15 commits January 9, 2024 11:33

Merge branch 'develop' into weizman/try-lavadome

95b7005

bump lavadome

33360ee

update policies

fc55f42

revert build policy

6b9beff

revert build policy

a8456b1

revert

58463c3

revert

93bc1a4

Merge branch 'develop' into weizman/try-lavadome

5e1d098

bump v

b8d758d

Merge branch 'weizman/try-lavadome' of https://github.com/metamask/me…

6362e17

…tamask-extension into weizman/try-lavadome

lint

8627da1

Merge branch 'develop' into weizman/try-lavadome

809e68a

lint

472923d

Merge branch 'weizman/try-lavadome' of https://github.com/metamask/me…

68982e9

…tamask-extension into weizman/try-lavadome

support test mode

eab7582

weizman added 7 commits January 10, 2024 14:41

support lavadome debug for tests

5db302a

lint

29de0a4

update policy

1a3b35a

fix policy

91c8854

strip distraction support to fix tests

e1d1796

lint

ab9ba85

remove new lines

aeea938

weizman added 8 commits January 23, 2024 18:34

Merge branch 'develop' into weizman/try-lavadome

4e8ec9e

dedupe and rebuild policies

5fe1ae0

Merge remote-tracking branch 'origin/develop' into weizman/try-lavadome

90b428b

polivy

8e19fcb

Update policy.json

30d8118

Update policy.json

d545037

newline

ee81f52

newline

65c6f87

darkwing previously approved these changes Jan 30, 2024

View reviewed changes

weizman dismissed darkwing’s stale review via 889e9d6 January 31, 2024 16:57

add selection disabled warn

2cc62b4

weizman force-pushed the weizman/try-lavadome branch from 44a2bc7 to 2cc62b4 Compare January 31, 2024 17:02

weizman added 2 commits January 31, 2024 19:15

fix test

308d2ce

lint

1394bad

danjm reviewed Jan 31, 2024

View reviewed changes

weizman requested review from danjm and darkwing January 31, 2024 17:25

danjm approved these changes Jan 31, 2024

View reviewed changes

weizman removed the DO-NOT-MERGE Pull requests that should not be merged label Jan 31, 2024

georgewrmarshall approved these changes Jan 31, 2024

View reviewed changes

weizman changed the title ~~Introduce LavaDome~~ Integrate LavaDome private key protection Feb 1, 2024

weizman merged commit 030d8cb into develop Feb 1, 2024

weizman deleted the weizman/try-lavadome branch February 1, 2024 07:19

github-actions bot locked and limited conversation to collaborators Feb 1, 2024

github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Feb 1, 2024

metamaskbot added the release-11.11.0 Issue or pull request that will be included in release 11.11.0 label Feb 1, 2024

Conversation

weizman commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

socket-security bot commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

socket-security bot commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weizman commented Jan 9, 2024

Uh oh!

metamaskbot commented Jan 29, 2024

Uh oh!

danjm left a comment

Choose a reason for hiding this comment

Uh oh!

danjm commented Jan 31, 2024

Uh oh!

danjm commented Jan 31, 2024

Uh oh!

metamaskbot commented Jan 31, 2024

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

georgewrmarshall Jan 31, 2024

Choose a reason for hiding this comment

Uh oh!

weizman Feb 1, 2024

Choose a reason for hiding this comment

Uh oh!

georgewrmarshall Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weizman Feb 1, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

weizman commented Dec 21, 2023 •

edited

Loading

socket-security bot commented Dec 21, 2023 •

edited

Loading

socket-security bot commented Dec 21, 2023 •

edited

Loading

georgewrmarshall Jan 31, 2024 •

edited

Loading