Bug 231727

Summary: New spec: Block external protocol handler in sandboxed frames
Product: WebKit Reporter: Arthur Sonzogni <arthursonzogni>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, cdumez, ews-watchlist, ggaren, hi, mkwst, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=237269
Bug Depends on: 232769, 236516    
Bug Blocks:    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch none

Description Arthur Sonzogni 2021-10-14 01:51:12 PDT
Implementation bug about a new specification:
spec: https://github.com/whatwg/html/pull/7124#pullrequestreview-778826909
whatwg/html bug: https://github.com/whatwg/html/issues/2191

Developers are surprised that sandboxed iframe can navigate and/or
redirect the user toward an external application.

General iframe navigation in the sandboxed iframe are not blocked
normally, because they stay within the iframe. However they can be seen
as a popup or a top-level navigation when it opens an external
application. In this case, it makes sense to extend the scope of sandbox
flags, and block malvertisers.

This feature gates access to external protocol from sandboxed iframe behind any of:
- allow-popup
- allow-top-level-navigation
- allow-top-level-navigation-by-user-activation + UserGesture.
Comment 1 Radar WebKit Bug Importer 2021-10-21 01:52:21 PDT
<rdar://problem/84498192>
Comment 2 Chris Dumez 2021-11-03 17:12:05 PDT
Created attachment 443258 [details]
Patch
Comment 3 Tim Horton 2021-11-03 20:10:18 PDT
Comment on attachment 443258 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443258&action=review

Vaguely worried about ads that want to pop to the App Store. Also about third party app compatibility. But no concrete examples of either.

> Source/WebKit/ChangeLog:13
> +        an external apps.

“An external apps”

> Source/WebKit/UIProcess/WebPageProxy.cpp:5359
> +        WEBPAGEPROXY_RELEASE_LOG_ERROR(Process, "Ignoring request to load this main resource because it has a custom protocol and comes from a sandbox iframe");

“Sandboxed”? Also should we log to the inspector too?
Comment 4 Chris Dumez 2021-11-04 08:19:19 PDT
Created attachment 443305 [details]
Patch
Comment 5 Chris Dumez 2021-11-04 11:15:39 PDT
Created attachment 443323 [details]
Patch
Comment 6 Chris Dumez 2021-11-05 14:26:11 PDT
Created attachment 443438 [details]
Patch
Comment 7 Chris Dumez 2021-11-05 15:23:40 PDT
The iOS API test failure is caused by Bug 232769.
Comment 8 Chris Dumez 2021-11-05 22:41:00 PDT
Created attachment 443478 [details]
Patch
Comment 9 Chris Dumez 2021-11-09 07:35:38 PST
Ping review?
Comment 10 Brent Fulgham 2021-11-09 08:18:11 PST
Comment on attachment 443478 [details]
Patch

R=me
Comment 11 EWS 2021-11-09 08:40:57 PST
Committed r285501 (244025@main): <https://commits.webkit.org/244025@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443478 [details].
Comment 12 Arthur Sonzogni 2021-11-09 08:55:51 PST
Hi Chris,

Some data I gathered on Chrome about enabling this.
This would impact ~0.015% pages.

Among them, a good quarter is triggered from https://teams.microsoft.com with the msteam: protocol. They have been notified about it here:
https://github.com/whatwg/html/issues/2191#issuecomment-952283644
and we hope this will contribute to reduce the number of impacted pages.

I just wanted to let you know this is still not enforced by default in Chrome, so you may get some websites complaining about it. On the other side, I am expecting a part of this is coming from malicious ads, but I don't have any estimations, only some reports that this is maliciously used in the wild.

Getting Safari to ship this would greatly help me enabling this in Chrome too, so I am very happy! I just wanted to warn you about the risk and I will let you decide if 0.015% is acceptable.