-
-
Notifications
You must be signed in to change notification settings - Fork 184
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: Block Client Side C2 Requests by Managing a hashed C2 Request Blocklist #4526
Conversation
…heck if the c2 request is malicous / needs refactor and more tests
…fuly manages and pulls downn blocklist from client-side-detection-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some changes requested (some are nits).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of comments. Overall looks good, though I am not sure if we should split the logic to fetch the c2 blocklist from the current lists being served by the phishing detection API. The current implementation ensures that we are only fetching the c2 blocklist and updating its contents if:
- for stale update: the stalelist fetch interval (30 days) is met + stalelist and hotlist have returned new data.
- for delta updates: the hotlist fetch interval (5 min) is met + hotlist have returned new data.
(I am highlighting return new data, because we are using the safeFetch
to and setting the no-cache
header. This means that anything other than a 200 is going to return undefined.. and by setting the no-cache
header we are relying on the browser cache (but re-validating it on the origin server). If there are no changes, then we get 304).
The ways to go around this is to create a revalidation process for the c2 blocklist and add it to the maybeUpdateState
method. Or even abstract this list within the phishing detection API.
## Explanation This was a [change made in extension](https://github.com/MetaMask/metamask-extension/pull/26004/files#diff-306c4f2f0516d73535fb38d815249cfa94dff9cda38e4108a9bc6c0a16c9ca8aR30). This field is now nullable. It does not change any implementation, just types. ## References https://consensyssoftware.atlassian.net/browse/NOTIFY-940 ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/profile-sync-controller` - **CHANGED**: UserStorageController `isProfileSyncingEnabled` field to be nullable. ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate
…k explorer domains (#4552) ## Explanation This is so we can easily reuse these block explorers for all platforms that consume notifications. We may start moving more shared logic to shared libraries for this same reason. ## References https://consensyssoftware.atlassian.net/browse/NOTIFY-941 ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/notification-services-controller` - **ADDED**: New constant for the block explorers for chains we support notifications for. ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate
## Explanation Added product-safety to own phishing-controller package ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Mark Stacey <[email protected]>
## Explanation <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> This PR exposes NFT `collections` api through NFT controller. ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? For example: * Fixes #12345 * Related to #67890 --> Related to: MetaMask/MetaMask-planning#2507 Extension PR using this PR's preview build: MetaMask/metamask-extension#25692 ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/assets-controllers` - **ADDED**: Add `fetchNftCollectionMetadata` to `NFTController` api ## Checklist - [X] I've updated the test suite for new or updated code as appropriate - [X] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [X] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Elliot Winkler <[email protected]>
## Explanation In various packages, we have a package listed as both a peer dependency and as a dependency. This was done by mistake, the dependency entries aren't actually used in practice, as we require the package to be installed as a "peer" package anyway. It's also incorrect to list other controllers as a dependency in this case because we don't know which version we need. In these cases we need it to match the version setup with the global messenger, which we don't know. ## References None ## Changelog Changelogs updated in diff ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate
## Explanation Profile sync SDK was not exporting any method for connecting to snap. This PR exports a method to connect to snap after initializing the SDK
This is the release candidate for `v180.0.0`: - `@metamask/[email protected]` (major) - `@metamask/[email protected]` (major) - `@metamask/[email protected]` (patch) - `@metamask/[email protected]` (patch) - `@metamask/[email protected]` (patch) - `@metamask/[email protected]` (patch) - `@metamask/[email protected]` (patch) - `@metamask/[email protected]` (patch) - `@metamask/[email protected]` (minor) - `@metamask/[email protected]` (minor) - `@metamask/[email protected]` (patch) - `@metamask/[email protected]` (patch) - `@metamask/[email protected]` (patch) - `@metamask/[email protected]` (minor) - `@metamask/[email protected]` (major) - `@metamask/[email protected]` (major) - `@metamask/[email protected]` (patch) - `@metamask/[email protected]` (patch) - `@metamask/[email protected]` (patch) - Closes #3651 - Unblocks new releases in core. - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Elliot Winkler <[email protected]> Co-authored-by: Mark Stacey <[email protected]>
try { | ||
const url = new URL(urlString); | ||
|
||
const hash = sha256Hash(url.hostname.toLowerCase()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of the entries in the list being hashes, rather than communicating the hostnames in clear-text?
If the purpose is to obscure the blocked domain names from public view, isn't at minimum a frequently rotating salt necessary to mitigate trivial rainbow table attacks etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @legobeat !
your correct in that the purpose is to obscure the domains. We acknowledge that scammers could reverse this method to check if their domain is on this list. The real only way to solve for this is by to heavily obscurating the c2 check in something like a PPOM for client side detection which might be something we build in the future. For now our team thinks it best to push this out and let the scammers pivot as we further build out metamasks client side detection capability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that it offers no notable friction at all for anyone (incl attackers) who wants to check for specific domains (and only minimal friction for attackers to reverse the majority of the list, considering domain lists are readily available online): What is the expected upside of hashing at all, vs just listing the names in clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…the c2 diff blocklist within maybeUpdate
… be used within the extension
@metamaskbot publish-preview |
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
…ask/core into feat/client-side-detection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
"@noble/hashes": "^1.4.0", | ||
"@types/punycode": "^2.1.0", | ||
"eth-phishing-detect": "^1.2.0", | ||
"ethereum-cryptography": "^2.1.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of these dependencies should be noted as CHANGED
entries in the changelog, possibly as breaking changes.
Explanation
This PR implements new logic to manage a Command and Control (C2) request blocklist within the Phishing Controller. It introduces updates to both the PhishingController and PhishingDetector to efficiently handle and validate URLs against a hashed request blocklist.
Solution
The proposed changes add a requestBlocklist to the Phishing Controller and Phishing Detector. This includes methods for updating the blocklist, checking if a request URL's domain is blocked, and incorporating these checks into the existing phishing detection workflow. This ensures that any requests to known malicious C2 domains are identified and blocked.
Notable Changes
Introduction of requestBlocklist to list types.
Implementation of isBlockedRequest in PhishingController.
Update to #updateStalelist to fetch and include the request blocklist.
Addition of isMaliciousRequestDomain in PhishingDetector for URL validation against the blocklist.
Changelog
@metamask/phishing-controller
requestBlocklist
type to ListTypes.isBlockedRequest
method to PhishingController.requestBlocklist
in#updateStalelist
.isMaliciousRequestDomain
method to PhishingDetector.requestBlocklist
in PhishingDetector configuration.sha256
andtoHex
imports fromethereum-cryptography
.sha256Hash
function to generate SHA-256 hash of a domain.