Malicious site protection navigation detection #3707

alessandroboron · 2024-12-10T12:37:00Z

Task/Issue URL: https://app.asana.com/0/1206329551987282/1207151848931030
Tech Design URL: https://app.asana.com/0/1206329551987282/1207273224076495/f
Cc: @not-a-rootkit

Description:

This PR adds the navigation logic for detecting a malicious site and navigating to a special error page if the site is malicious.

The original idea in the tech design was to intercept the Request in decidePolicyForNavigationAction and check whether a site is malicious cancelling the request accordingly.

We noticed that the above approach increases the page load time of websites due to the logic check.

I opted for an approach where in decidePolicyForNavigationAction we start the detection task in parallel without waiting and in decidePolicyForNavigationResponse we evaluate the task’s result.

Another approach I thought of was to perform the logic in the background in didStartProvisionalNavigation. The problem with this approach is that is called only for navigation that starts from the main frame so it would not be possible to intercept malicious iFrame URLs.

NOTE
BSK Threat detection is currently mocked. Its integration and tests will follow in an upcoming PR.

Steps to test this PR:

Scenario 1 - Phishing Website should show special error page

Ensure that http://privacy-test-pages.site/security/badware/phishing.html shows the phishing special error page with the copy “This website may be impersonating a legitimate site...".

Scenario 2 - Malware Website should show special error page

Ensure that http://privacy-test-pages.site/security/badware/malware.html shows the malware special error page with the copy “DuckDuckGo blocked this page because it may be distributing malware...".

Scenario 3 - Allow visit a malicious website should not show the error again if navigating back to the site

Visit one of the malicious sites above.
Tap the “Advanced” button
Tap the “Accept Risk and Visit Site” link
Wait for malicious site to load.
Refresh page.
Ensure the website load without showing the error again.
Close tab.
Paste the same URL again.
Ensure the special error page is shown again.

Scenario 4 - Leave a malicious site should close the Tab

Visit one of the malicious sites above.
Tap the “Leave Site” button.
Ensure that the tab is closed.

Definition of Done (Internal Only):

Does this PR satisfy our Definition of Done?

Copy Testing:

Use of correct apostrophes in new copy, ie ’ rather than '

Orientation Testing:

Portrait
Landscape

Device Testing:

OS Testing:

iOS 15
iOS 16
iOS 17

Theme Testing:

Light theme
Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

github-actions · 2024-12-10T12:38:34Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 5c07c05

alessandroboron · 2024-12-10T12:58:04Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageInterfaces/SpecialErrorPageActionHandler.swift

@@ -26,11 +27,14 @@ import Foundation
 /// advanced information related to the error.
 protocol SpecialErrorPageActionHandler {
    /// Handles the action of navigating to the site associated with the error page
-    func visitSite()
+    @MainActor
+    func visitSite(url: URL, errorData: SpecialErrorData)


I’m a bit on the fence with this.
The idea was to have a generic protocol that all the sub-special error handlers should conform to.
For MaliciousSiteProtectionNavigationHandler I need to pass the URL and SpecialErrorData but I don’t need them for the SSL handler. So I feel I’m violating the Interface Segregation principle. what do you think?

could you perhaps create two funcs visitSite, one without params and one with params? Different handlers would be implementing only one function leaving other implementation empty? It's not ideal but the simplest

However, if we encounter more cases like this where we’re stretching these protocols, we might need to reconsider whether it’s appropriate to treat all special errors the same way. For example, SSL and malicious site errors already exhibit different behaviors. I’ll leave it up to you, but perhaps we should treat them differently (not under same logic) from the start. We can discuss further in 2025 :)

alessandroboron · 2024-12-10T12:59:02Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageNavigationHandler+MaliciousSite.swift

+                let response = MaliciousSiteDetectionNavigationResponse(navigationAction: navigationAction, errorData: errorData)
+                return .navigationHandled(.mainFrame(response))
+            } else {
+                // Extract the URL of the source frame (the iframe) that initiated the navigation action


I will look more into iFrame logic when I integrate with BSK

alessandroboron · 2024-12-10T13:00:13Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageNavigationHandler+MaliciousSite.swift

+
+    @MainActor
+    func handleMaliciousExemptions(for navigationType: WKNavigationType, url: URL) {
+        // TODO: check storing redirects


Will address this TODO in upcoming PR

alessandroboron · 2024-12-10T13:01:23Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageNavigationHandler+SSL.swift

@@ -90,7 +94,7 @@ extension SSLErrorPageNavigationHandler: SpecialErrorPageActionHandler {
        Pixel.fire(pixel: .certificateWarningLeaveClicked)
    }

-    func visitSite() {
+    func visitSite(url: URL, errorData: SpecialErrorData) {


See comment https://github.com/duckduckgo/iOS/pull/3707/files#r1878050527

github-actions · 2024-12-18T00:20:23Z

This PR has been inactive for more than 7 days and will be automatically closed 7 days from now.

jaceklyp · 2024-12-20T17:33:33Z

DuckDuckGo/MaliciousSiteProtection/MaliciousSiteProtectionManager.swift

+        case "http://privacy-test-pages.site/security/badware/phishing.html":
+            return .phishing
+        case "http://privacy-test-pages.site/security/badware/malware.html":


should it be http vs https?
also, we should be keeping all urls inside AppURLs

jaceklyp · 2024-12-20T17:45:40Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageInterfaces/SpecialErrorPageActionHandler.swift

@@ -26,11 +27,14 @@ import Foundation
 /// advanced information related to the error.
 protocol SpecialErrorPageActionHandler {
    /// Handles the action of navigating to the site associated with the error page
-    func visitSite()
+    @MainActor
+    func visitSite(url: URL, errorData: SpecialErrorData)


could you perhaps create two funcs visitSite, one without params and one with params? Different handlers would be implementing only one function leaving other implementation empty? It's not ideal but the simplest

jaceklyp · 2024-12-20T17:46:22Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageInterfaces/WebViewNavigationHandling.swift

-    /// - Returns: A Boolean value that indicates whether the navigation action was handled.
-    func handleSpecialErrorNavigation(navigationAction: WKNavigationAction, webView: WKWebView) async -> Bool
+    @MainActor
+    func handleDecidePolicyFor(navigationAction: WKNavigationAction, webView: WKWebView)


I think it should be
func handleDecidePolicy(for navigationAction: WKNavigationAction, webView: WKWebView)

jaceklyp · 2024-12-20T17:46:48Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageInterfaces/WebViewNavigationHandling.swift

+    ///   - webView: The web view from which the navigation request began.
+    /// - Returns: A Boolean value that indicates whether to cancel or allow the navigation.
+    @MainActor
+    func handleDecidePolicyfor(navigationResponse: WKNavigationResponse, webView: WKWebView) async -> Bool


typo, "for" is lowercased, but I'd refer to my previous comment ^

jaceklyp · 2024-12-20T17:49:09Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageNavigationHandler+MaliciousSite.swift

+    ///     the navigation event.
+    ///   - webView: The web view from which the navigation request began.
+    @MainActor
+    func createMaliciousSiteDetectionTask(for navigationAction: WKNavigationAction, webView: WKWebView)


I'd go with makeMalicious*

jaceklyp · 2024-12-20T17:51:55Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageNavigationHandler+MaliciousSite.swift

+            return
+        }
+
+        let threatDetectionTask: Task<MaliciousSiteProtectionNavigationResult, Never> = Task.detached { [weak self] in


why is the Task detached?

jaceklyp · 2024-12-20T17:53:18Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageNavigationHandler+MaliciousSite.swift

+        guard let url = navigationAction.request.url else {
+            return
+        }
+
+        if let aboutBlankURL = URL(string: "about:blank"), url == aboutBlankURL {
+            return
+        }


can we put it under one guard, like so
guard let url = navigationAction.request.url, url != URL(string: "about:blank") else { return }

jaceklyp · 2024-12-20T17:59:36Z

DuckDuckGo/SpecialErrorPage/SpecialErrorPageInterfaces/SpecialErrorPageActionHandler.swift

@@ -26,11 +27,14 @@ import Foundation
 /// advanced information related to the error.
 protocol SpecialErrorPageActionHandler {
    /// Handles the action of navigating to the site associated with the error page
-    func visitSite()
+    @MainActor
+    func visitSite(url: URL, errorData: SpecialErrorData)


However, if we encounter more cases like this where we’re stretching these protocols, we might need to reconsider whether it’s appropriate to treat all special errors the same way. For example, SSL and malicious site errors already exhibit different behaviors. I’ll leave it up to you, but perhaps we should treat them differently (not under same logic) from the start. We can discuss further in 2025 :)

jaceklyp · 2024-12-20T18:05:23Z

DuckDuckGo/TabViewController.swift

-                self.startDownload(with: navigationResponse, decisionHandler: decisionHandler)
-            } cancelHandler: {
+        // If the navigation has been handled by the special error page handler cancel navigating to new content as the special error page will be shown.
+        Task { @MainActor in


Whoa :), this is triggering an alert for me. Making a task on this code could have bigger consequences – we’re essentially returning immediately and skipping the run loop, without a guarantee that other tasks spawned elsewhere won’t interfere with this. Are we sure this is the approach we want to take?

alessandroboron commented Dec 10, 2024

View reviewed changes

alessandroboron force-pushed the alessandro/malicious-site-protection-navigation-detection-async branch from 8f0b90f to b6dbf5c Compare December 10, 2024 13:06

alessandroboron requested a review from jaceklyp December 10, 2024 13:39

github-actions bot added the stale label Dec 18, 2024

alessandroboron force-pushed the alessandro/malicious-site-protection branch from ef5bd3f to e2f654c Compare December 19, 2024 12:36

alessandroboron added 16 commits December 19, 2024 13:37

Add Detection Logic

1e126cd

Integrate MaliciousSiteProtection

3986ec1

Hardcode malicious site detection

0712de4

Use isSpecialErrorPageRequest flag to avoid navigation loops

ecd530f

Clean up due to integration with BSK

3d8c5e4

Add test for malicious site navigation handler

b20db83

Disable adding redirect to examption URLs

0310680

Re-set bypassed threat kind at every page load

c6aef3d

Refactor MaliciousSiteProtectionNavigationHandler

0b52351

Refactor SpecialErrorPageNavigationHandler and handle synchronisation

8925306

Fix typo for Malicious site Navigation Handler method

efb6e54

Update the logic for handling malicious site protection

4de6ac8

Close Tab when leaving malicious site

c7b2d3d

Update BSK version

758a164

Clean up

829bc38

Update BSK after rebasing

5c07c05

alessandroboron force-pushed the alessandro/malicious-site-protection-navigation-detection-async branch from b6dbf5c to 5c07c05 Compare December 19, 2024 12:42

github-actions bot removed the stale label Dec 20, 2024

jaceklyp requested changes Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Malicious site protection navigation detection #3707

Malicious site protection navigation detection #3707

alessandroboron commented Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024 •

edited

Loading

alessandroboron Dec 10, 2024

jaceklyp Dec 20, 2024

jaceklyp Dec 20, 2024

alessandroboron Dec 10, 2024

alessandroboron Dec 10, 2024

alessandroboron Dec 10, 2024

github-actions bot commented Dec 18, 2024

jaceklyp Dec 20, 2024

jaceklyp Dec 20, 2024

jaceklyp Dec 20, 2024

jaceklyp Dec 20, 2024

jaceklyp Dec 20, 2024

jaceklyp Dec 20, 2024

jaceklyp Dec 20, 2024

jaceklyp Dec 20, 2024

jaceklyp Dec 20, 2024

Malicious site protection navigation detection #3707

Are you sure you want to change the base?

Malicious site protection navigation detection #3707

Conversation

alessandroboron commented Dec 10, 2024 • edited Loading

Internal references:

github-actions bot commented Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandroboron commented Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024 •

edited

Loading