Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add websocket support for c2 detection #28782

Merged
merged 30 commits into from
Dec 11, 2024
Merged

Conversation

AugmentedMode
Copy link
Contributor

@AugmentedMode AugmentedMode commented Nov 27, 2024

Description

This pull request adds WebSocket support to the MetaMask extension's phishing detection functionality. Scammers have started using WebSocket connections for command-and-control (C2) operations to bypass traditional HTTP-based phishing detection. This PR allows the extension to intercept and block WebSocket handshake requests (ws:// and wss://) in addition to HTTP/HTTPS requests.

The key changes include:

  1. Adding WebSocket schemes (ws://*/* and wss://*/*) to the urls filter in background.js.
  2. Updating the manifest.json to include WebSocket permissions in the host_permissions field.

This ensures that malicious WebSocket connections can be detected and blocked.

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3788

Manual testing steps

  1. Navigate to example.com
  2. Initiate a WebSocket connection to a known safe domain (e.g., wss://example.com) and verify it works as expected by going to the console via right clicking and hitting inspect. Then type into the console new WebSocket("https://example.com/")
  3. Attempt a WebSocket connection to a domain flagged as phishing, and verify the connection is blocked and appropriate warnings are displayed by going to the console via right clicking and hitting inspect. Then type into the console new WebSocket("https://walietconnectapi.com/")

Screenshots/Recordings

Before

No support for detecting WebSocket phishing connections.


After

WebSocket phishing connections are detected and blocked during the handshake phase.

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.

@AugmentedMode AugmentedMode requested a review from a team as a code owner November 27, 2024 18:17
@AugmentedMode AugmentedMode marked this pull request as draft November 27, 2024 18:17
Copy link
Contributor

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.

@metamaskbot metamaskbot added the team-product-safety Push issues to Product Safety team label Nov 27, 2024
@AugmentedMode AugmentedMode self-assigned this Nov 27, 2024
@AugmentedMode AugmentedMode marked this pull request as ready for review November 27, 2024 18:26
mindofmar
mindofmar previously approved these changes Nov 27, 2024
Copy link
Contributor

@mindofmar mindofmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@metamaskbot
Copy link
Collaborator

Builds ready [60a3ac7]
Page Load Metrics (1763 ± 94 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint27823361529556267
domContentLoaded15132313173820096
load15562337176319694
domInteractive238337147
backgroundConnect972272010
firstReactRender17170403517
getState55720199
initialActions01000
loadScripts10781827126917182
setupStore669252211
uiStartup175425801991231111
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 23 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@@ -327,7 +327,7 @@ function maybeDetectPhishing(theController) {
return {};
},
{
urls: ['http://*/*', 'https://*/*'],
urls: ['http://*/*', 'https://*/*', 'ws://*/*', 'wss://*/*'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change needs automated tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a few tests!

@metamaskbot
Copy link
Collaborator

Builds ready [d2c7f78]
Page Load Metrics (1614 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint33218881556302145
domContentLoaded13561810157710550
load14001893161411153
domInteractive236639168
backgroundConnect6100342612
firstReactRender1699482814
getState47717199
initialActions01000
loadScripts984134711428541
setupStore68413189
uiStartup16262235186117584
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 23 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@AndrewMohawk
Copy link

Excited for this to land in prod!

@metamaskbot
Copy link
Collaborator

Builds ready [395bf8c]
Page Load Metrics (1896 ± 90 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint16012429190118991
domContentLoaded15752394186718890
load16062432189618790
domInteractive256335115
backgroundConnect879352311
firstReactRender168627178
getState46210943216
initialActions01000
loadScripts11531820142816479
setupStore75511105
uiStartup181527312166221106
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 23 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [dabc7a8]
Page Load Metrics (1974 ± 52 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26821901811504242
domContentLoaded17792171194411656
load18222194197410852
domInteractive259435167
backgroundConnect86329199
firstReactRender16251931
getState812591293617
initialActions01000
loadScripts13371705151910349
setupStore716921
uiStartup20242857226717986
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 23 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@AugmentedMode AugmentedMode requested a review from a team as a code owner December 10, 2024 20:14
@@ -72,5 +72,6 @@
"unresponsive-rpc.test",
"unresponsive-rpc.url",
"user-storage.api.cx.metamask.io",
"www.4byte.directory"
"www.4byte.directory",
"verify.walletconnect.com"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added as MetaMasks Test dApp makes a websocket connnection as shown in the screenshot

Screenshot 2024-12-10 at 3 33 47 PM

@metamaskbot
Copy link
Collaborator

Builds ready [d64c13c]
Page Load Metrics (2032 ± 127 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint171226782032265127
domContentLoaded168026052002255122
load169326292032264127
domInteractive258540168
backgroundConnect97328209
firstReactRender15322152
getState882781453818
initialActions01000
loadScripts127221381560218104
setupStore68312168
uiStartup193030952360338162
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 23 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [fe74a38]
Page Load Metrics (1923 ± 72 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint17172395193214871
domContentLoaded17032386189215273
load17152394192314972
domInteractive2410236178
backgroundConnect96431189
firstReactRender158122147
getState962861273919
initialActions01000
loadScripts13211988149614871
setupStore711911
uiStartup19332676221018890
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 23 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

cryptotavares
cryptotavares previously approved these changes Dec 11, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [28c26bf]
Page Load Metrics (1572 ± 29 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1478166015755928
domContentLoaded1427164715456431
load1448166315726129
domInteractive247446189
backgroundConnect96725178
firstReactRender1676302211
getState55018178
initialActions00000
loadScripts1060125711655426
setupStore65211136
uiStartup1605185517386732
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 23 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Tested with the e2e tests locally, and it appears to work well. I also double-checked that the manifest file changes didn't introduce new warnings, and they do not.

@AugmentedMode AugmentedMode added this pull request to the merge queue Dec 11, 2024
Merged via the queue into main with commit e0f6575 Dec 11, 2024
75 checks passed
@AugmentedMode AugmentedMode deleted the feat/websocket-support branch December 11, 2024 19:37
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2024
@metamaskbot metamaskbot added the release-12.10.1 Issue or pull request that will be included in release 12.10.1 label Dec 11, 2024
Copy link
Contributor

@davidmurdoch davidmurdoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've confirmed that this new code intercepts wss requests and redirects the user to the phishing page. However, the Proceed anyway link on the phishing page now redirects the user to the websocket URL.

Not sure what the "fix" is here, but it is something we should figure out how to handle better.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.10.1 Issue or pull request that will be included in release 12.10.1 team-product-safety Push issues to Product Safety team
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants