Fix #2424: Disabling Synthetic clicks on WebPages #2419

Brandon-T · 2020-03-22T13:51:31Z

Security Review

https://github.com/brave/security/issues/104

Test Plan

Go to reddit or any site with a login
Tap the URL bar and type https://loving-newton-6b489c.netlify.com/
Hit Enter and make sure you do NOT get a pop!
If you see a link on the page and you tap on it, it should ask you are you sure you want to open it.
If you manually type any data URIs into the URL bar, it should give a prompt to ask whether you want to open it or not.

Summary of Changes

Detect synthetic click types and automatically block them.
If this cannot be done, ask the user if they wish to proceed.
If the url can't be opened, then it will just show an error.

Other Browser:

Safari: Attempts to open the URL but gives an error Safari cannot open this URL then proceeds to show the 2 popups.
Chrome: Automatically begins downloading the document
Firefox: Same behaviour as current Brave-iOS.. It shows the two popups but does NOT show the document cookies or any other sensitive information because parent-tab:// is not a valid URL. If it were, we'd be in trouble. In both cases (brave and firefox), it's a minor UXSS attack.

This pull request fixes #2422 && #2424

Submitter Checklist:

Unit Tests are updated to cover new or changed functionality
User-facing strings use NSLocalizableString()

Test Plan:

Screenshots:

Reviewer Checklist:

Issues include necessary QA labels:
- QA/(Yes|No)
- release-notes/(include|exclude)
- bug / enhancement
Necessary security reviews have taken place.
Adequate unit test coverage exists to prevent regressions.
Adequate test plan exists for QA to validate (if applicable).
Issue is assigned to a milestone (should happen at merge time).

iccub

Add:

regular ticket in brave-ios
sec-review ticket
test plan for QA

Code looks good so approving it

Client/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift

jumde · 2020-03-24T18:49:39Z

Client/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift

+
+ //Fallback.. Asks the user whether or not the url should be opened.. but only for `data`
+ if navigationAction.navigationType == .linkActivated && url.scheme == "data" {
+ handleExternalURL(url) { didOpenURL in


data is not an external URL. This code essentially disables data urls (which is tracked as a different issue). I tried this code snippet using this URL: https://jumde.github.io/test/data-url.hml and clicking hello, the data URL cannot be opened.

…nnot be done, it's always best to ask the user if they wish to proceed.

diracdeltas · 2020-03-26T19:09:49Z

Client/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift

+
+ // Prevents synthetically activated links such as: CVE-2017-7089
+ //Follow desktop
+ if url.scheme == "data" {


will this also block window.open('data:...')?

@Brandon-T ^

@diracdeltas I have implemented: https://blog.mozilla.org/security/2017/11/27/blocking-top-level-navigations-data-urls-firefox-59/

It does indeed block window.open. I tested against the latest Firefox-Quantum to make sure our behaviour is the same.

garvankeeley · 2020-03-31T21:42:17Z

Curious what y'all think of an approach like this mozilla-mobile/firefox-ios#6341
On Firefox iOS we are trying to follow the desktop approach of allowing certain data URL media types.

jumde · 2020-03-31T21:50:44Z

@garvankeeley - Same plan: #2424 (comment)

Let us know if we're missing something.

diracdeltas · 2020-04-06T18:34:59Z

any blockers here or can this be merged?

Brandon-T · 2020-04-15T21:27:19Z

Client/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift

+ case WebKit::WebMouseEvent::TwoFingerTap:
+ return WKSyntheticClickTypeTwoFingerTap;
+ }*/
+ return decisionHandler(.cancel)


Please note, the above code is done on purpose. Firefox desktop v75.0 (64-bit), MacOS blocks ALL synthetic clicks (touches/clicks that do not involve user interaction). For example test page which I hosted locally via python's flask framework:

<!DOCTYPE html> <html lang="en"> <head> <title>Test Page</title> </head> <body> <p> <a href="data:text/html,<script>alert('hi');</script>">Should be Blocked</a> <a href="data:text/xhtml,<script>alert('hi');</script>">Should also be Blocked</a> <a href="data:application/xhtml,<script>alert('hi');</script>">How many are we going to block</a> <a href="data:image/svg+xml;base64,eyJ0ZXN0Ijoid2hhdGV2ZXIifQo=">I'm tired of blocking :(</a> <a href="data:video/mp4;base64,eyJ0ZXN0Ijoid2hhdGV2ZXIifQo=">Should do something</a> <a href="data:,">Should render blank plain text</a> <a href="data:application/json,{\"test\":\"yay\"}">Should do something</a> <a href="data:text/json;base64,PGh0bWw+PGJvZHkgc3R5bGU9J2JhY2tncm91bmQtY29sb3I6cmVkJz48L2JvZHk+PC9odG1sPgo=">I'm Valid!</a> <a href="data:text/plain;base64,PGh0bWw+PGJvZHkgc3R5bGU9J2JhY2tncm91bmQtY29sb3I6cmVkJz48L2JvZHk+PC9odG1sPgo=">I'm not plain..</a> <a href="data:application/something;base64,iVBORw0KGg==">Oh boy.. Not sure about this one</a> </p> <script> //Synthetically change the window TopLevelDomain via window.location or window.open setTimeout(function(){ window.location = "data:text/html;base64,PGh0bWw+PGJvZHkgc3R5bGU9J2JhY2tncm91bmQtY29sb3I6cmVkJz48L2JvZHk+PC9odG1sPgo="; //window.open("data:text/html;base64,PGh0bWw+PGJvZHkgc3R5bGU9J2JhY2tncm91bmQtY29sb3I6cmVkJz48L2JvZHk+PC9odG1sPgo="); }, 2000); </script> </body> </html>

jumde · 2020-04-16T17:28:35Z

security review approved 👍

iccub · 2020-04-16T18:38:08Z

Client/Frontend/Browser/DataURIParser.swift

+ mediaType = headers.first ?? "text/plain"
+
+ let dataSegment = uri.index(infoSegment, offsetBy: 1)
+ //data = try! Data(contentsOf: URL(string: uri)!)


Remove this line

iccub · 2020-04-16T18:38:51Z

Client/Frontend/Browser/DataURIParser.swift

+
+ let dataSegment = uri.index(infoSegment, offsetBy: 1)
+ //data = try! Data(contentsOf: URL(string: uri)!)
+ data = Data(base64Encoded: String(uri[dataSegment..<uri.endIndex])) ?? Data()


Do we really want to fallback to an empty data object?
Maybe throwing an error would be better, what do you think?

iccub · 2020-04-16T18:40:03Z

Client/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift

+
+ return decisionHandler(.cancel)
+ } catch {
+ return


log error here

Brandon-T requested review from kylehickinson, jumde, jhreis and iccub March 22, 2020 13:51

iccub approved these changes Mar 23, 2020

View reviewed changes

Client/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift Outdated Show resolved Hide resolved

iccub added the needs security review label Mar 23, 2020

iccub changed the title ~~Fix for Synthetic Clicks and parsing of data scheme URLs - CVE-2017-7089~~ Fix #2422: CVE-2017-7089 Mar 24, 2020

jumde reviewed Mar 24, 2020

View reviewed changes

Client/Frontend/Browser/BrowserViewController/BrowserViewController+WKNavigationDelegate.swift Outdated Show resolved Hide resolved

Brandon-T changed the title ~~Fix #2422: CVE-2017-7089~~ Fix #2422: Disabling Synthetic clicks on WebPages Mar 24, 2020

jumde reviewed Mar 24, 2020

View reviewed changes

Brandon-T changed the title ~~Fix #2422: Disabling Synthetic clicks on WebPages~~ Fix #2422 & #2424: Disabling Synthetic clicks on WebPages Mar 24, 2020

iccub changed the title ~~Fix #2422 & #2424: Disabling Synthetic clicks on WebPages~~ Fix #2424: Disabling Synthetic clicks on WebPages Mar 25, 2020

iccub linked an issue Mar 25, 2020 that may be closed by this pull request

Disable data URLs in top-level navigation #2424

Open

Brandon-T added 3 commits March 25, 2020 15:04

Detect synthetic click types and automatically block them. If this ca…

329447e

…nnot be done, it's always best to ask the user if they wish to proceed.

Added documentation for the syntheticClick API.

140c741

Desktop disabled data URI schemes.

de588d1

diracdeltas reviewed Mar 26, 2020

View reviewed changes

jhreis approved these changes Mar 31, 2020

View reviewed changes

Fixed to only exclude and include specific data-uri's.

8b7bdeb

Brandon-T force-pushed the security/SyntheticClicks branch from 67ed378 to 8b7bdeb Compare April 15, 2020 21:09

Brandon-T requested review from diracdeltas, jhreis, iccub and jumde April 15, 2020 21:10

Fixed bug with commenting.

917dab9

Brandon-T commented Apr 15, 2020

View reviewed changes

iccub suggested changes Apr 16, 2020

View reviewed changes

sssasad approved these changes Nov 9, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2424: Disabling Synthetic clicks on WebPages #2419

Fix #2424: Disabling Synthetic clicks on WebPages #2419

Brandon-T commented Mar 22, 2020 •

edited

iccub left a comment

jumde Mar 24, 2020

diracdeltas Mar 26, 2020

diracdeltas Apr 3, 2020

Brandon-T Apr 15, 2020

garvankeeley commented Mar 31, 2020

jumde commented Mar 31, 2020

diracdeltas commented Apr 6, 2020

Brandon-T Apr 15, 2020 •

edited

jumde commented Apr 16, 2020

iccub Apr 16, 2020

iccub Apr 16, 2020

iccub Apr 16, 2020

Fix #2424: Disabling Synthetic clicks on WebPages #2419

Are you sure you want to change the base?

Fix #2424: Disabling Synthetic clicks on WebPages #2419

Conversation

Brandon-T commented Mar 22, 2020 • edited

Security Review

Test Plan

Summary of Changes

Other Browser:

Submitter Checklist:

Test Plan:

Screenshots:

Reviewer Checklist:

iccub left a comment

Choose a reason for hiding this comment

jumde Mar 24, 2020

Choose a reason for hiding this comment

diracdeltas Mar 26, 2020

Choose a reason for hiding this comment

diracdeltas Apr 3, 2020

Choose a reason for hiding this comment

Brandon-T Apr 15, 2020

Choose a reason for hiding this comment

garvankeeley commented Mar 31, 2020

jumde commented Mar 31, 2020

diracdeltas commented Apr 6, 2020

Brandon-T Apr 15, 2020 • edited

Choose a reason for hiding this comment

jumde commented Apr 16, 2020

iccub Apr 16, 2020

Choose a reason for hiding this comment

iccub Apr 16, 2020

Choose a reason for hiding this comment

iccub Apr 16, 2020

Choose a reason for hiding this comment

Brandon-T commented Mar 22, 2020 •

edited

Brandon-T Apr 15, 2020 •

edited