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

fix: Cherry-pick: Fix c2 detection bypass by supporting all network requests types #28087

Open
wants to merge 1 commit into
base: Version-v12.6.0
Choose a base branch
from

Conversation

AugmentedMode
Copy link
Contributor

@AugmentedMode AugmentedMode commented Oct 24, 2024

Description

Cherry pick: #28057

This update addresses a bypass that allowed scammers to bypass C2 detection by using alternative network request types to communicate with their Command and Control (C2) servers. Previously, we only listened for a limited set of request types (e.g., main_frame, sub_frame, xmlhttprequest), which left the system exposed to other methods of calling C2s.

With this fix, we now listen to all network request types and cross-check them against our client-side blocklist, ensuring better coverage and preventing these types of bypasses.

Changes:

Updated maybeDetectPhishing in background.js to listen for all network requests by removing restrictions on request types.

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to a website known to be on the C2 domain blocklist. For now we made our test website https://develop.d3bkcslj57l47p.amplifyapp.com/ have a malicious C2 Request that is on our blocklist.
  2. Attempt to interact with the site.
  3. Verify that on visiting the website you get redirected to the Metamask phishing page.
  4. Repeat with a site that is not on the blocklist to confirm normal operation.

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.

<!--
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 update addresses a bypass that allowed scammers to bypass C2
detection by using alternative network request types to communicate with
their Command and Control (C2) servers. Previously, we only listened for
a limited set of request types (e.g., main_frame, sub_frame,
xmlhttprequest), which left the system exposed to other methods of
calling C2s.

With this fix, we now listen to all network request types and
cross-check them against our client-side blocklist, ensuring better
coverage and preventing these types of bypasses.

Changes:

Updated maybeDetectPhishing in background.js to listen for all network
requests by removing restrictions on request types.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28057?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **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
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.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-extension/blob/develop/.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.
@AugmentedMode AugmentedMode requested a review from a team as a code owner October 24, 2024 18:20
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.

@github-actions github-actions bot added the team-product-safety Push issues to Product Safety team label Oct 24, 2024
@AugmentedMode AugmentedMode self-assigned this Oct 24, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [5f93667]
Page Load Metrics (1884 ± 214 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint163437021889442212
domContentLoaded158627441799249120
load163537211884445214
domInteractive27149472814
backgroundConnect8100383213102
firstReactRender51197973115
getState482232613
initialActions01000
loadScripts11781769132915072
setupStore1082262411
uiStartup178138892126468225
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -50 Bytes (-0.00%)
  • ui: 1.06 KiB (0.01%)
  • common: 0 Bytes (0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-product-safety Push issues to Product Safety team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants