shhnjk commented 7 years ago

Currently, spec allows use of custom protocols in sandboxed iframe. Which could be used to escape sandbox (see https://www.brokenbrowser.com/abusing-of-protocols/) or launch application from sandboxed iframe (mailto:, acrobat:, etc). I think custom protocols should be disabled in sandboxed iframe.

annevk commented 6 years ago

cc @whatwg/security

mikewest commented 6 years ago

I wouldn't be terribly sad if we locked this down to some extent in sandboxes. We'd need to collect some metrics regarding protocol usage inside sandboxed frames; I imagine it's low, though it probably breaks some enterprise app-launching mechanisms and ads use cases that navigate to specific app store entries (and it seems like we could deal with those by tying it to a sandbox flag).

bzbarsky commented 6 years ago

Definitely need data.

Also, "custom protocols" could mean at least two things: (1) protocols delegated to the OS and (2) protocols registered with https://html.spec.whatwg.org/multipage/system-state.html#custom-handlers. Do we want to treat them identically?

jonathanKingston commented 6 years ago

Also, "custom protocols" could mean at least two things

The risks are likely pretty similar in terms of security right?

Would allow-top-navigation permit the sandbox iframe doing this also?

annevk commented 6 years ago

The risks are likely pretty similar in terms of security right?

Not really, one causes navigation within the browser, the other launches native code, potentially in a compromising manner (there have been a number of attacks like this on iOS, iirc).

jonathanKingston commented 6 years ago

We have had exploits via feed:// which probably wouldn't considered native but handled through a completely other code path to normal navigations.

Would there be a risk we would then expose when a browser has a client on mailto rather than gmail?

bzbarsky commented 6 years ago

The risks are likely pretty similar in terms of security right?

As Anne says, no, because registerProtocolHandler bits are all in-browser. That said, should we make sure we sandbox the resulting page?

We have had exploits via feed://

That's a third case: (3) non-standard protocols supported by the browser directly

or so...

Would there be a risk we would then expose when a browser has a client on mailto rather than gmail?

Yes.

jonathanKingston commented 6 years ago

That's a third case: (3) non-standard protocols supported by the browser directly

I also think this might be an edge case we can ignore, given that I just removed it and registering pages into odd code paths like this should be closed where possible.

As Anne says, no, because registerProtocolHandler bits are all in-browser. That said, should we make sure we sandbox the resulting page?

Well they would have to be top level navigations anyway? I don't know 100% how we handle all these cases.

bzbarsky commented 6 years ago

given that I just removed it

Browsers still support all sorts of non-standard protocol bits (about:, chrome:, etc, etc).

Well they would have to be top level navigations anyway?

That's orthogonal to sandboxing. Just like a window.open from a sandboxed page ends up being sandboxed, though toplevel.

gijsk commented 6 years ago

As a first step, could we block navigations to any non-same-protocol, non-http(s)/ws(s) URIs that do not originate from user interaction?

patrickkettner commented 6 years ago

just chiming in with Edge feedback We would implement this if added to the spec, though not for a while. If we did implement, we would almost certainly add a group policy as a switch

mthiesse commented 4 years ago

I think allow-popups-to-escape-sandbox would be the right attribute for allowing sandbox-escaping external protocols.

Here's an example for Android that launches the calculator app from a sandboxed iframe: https://mthiesse-test.glitch.me/external-protocol.html

annevk commented 4 years ago

I think if you can navigate the top-level browsing context (be it auxiliary or top-level of current) it's fine, but in general we should stop supporting navigating embedded browsing contexts to non-fetch schemes. (And we should act as if Cross-Origin-Opener-Policy happened. Either due to the non-fetch scheme or the redirect to the non-fetch scheme.)

annevk commented 4 years ago

5289 might obviate the need for this somewhat.

mikewest commented 3 years ago

I'm supportive of doing something with regard to limitations on navigations to non-fetch schemes in nested contexts. It's probably worth adding some telemetry to track that? @mthiesse might know if we already have histograms in Chromium?

As a more narrow, short-term step: what would y'all think about adjusting https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-url-scheme to at least take sandbox flags into account, and to disallow external handling of a protocol during navigation unless allow-top-navigation or allow-popups is present, or if both allow-top-navigation-by-user-activation along with transient activation?

johannhof commented 3 years ago

Hey @mikewest,

I'm supportive of doing something with regard to limitations on navigations to non-fetch schemes in nested contexts. It's probably worth adding some telemetry to track that? @mthiesse might know if we already have histograms in Chromium?

@trikolon and I have been looking at this on the Firefox side recently and we're also very interested in restricting external protocol navigations further, especially in iframes. For example we've implemented our version of the permission prompts that Edge folks previously added to Blink (and would like to wire them up with Permissions Policy, see #6111). We're working on other ways to reduce annoyance and attack surface by requiring user activation more often and I'm happy to chat about that if you're interested (maybe in a separate issue).

We have also added telemetry in the 85 release (and are currently adding some more) so we'll have data for Firefox in the coming weeks.

As a more narrow, short-term step: what would y'all think about adjusting https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-url-scheme to at least take sandbox flags into account, and to disallow external handling of a protocol during navigation unless allow-top-navigation or allow-popups is present, or if both allow-top-navigation-by-user-activation along with transient activation?

From Firefox side we think that this is a good step and would implement this change, assuming that our telemetry doesn't hint at massive breakage (which doesn't seem likely).

mikewest commented 3 years ago

Sorry I lost track of this:

@johannhof: Did y'all have a chance to analyze data from Firefox's telemetry? @mthiesse: Do you know if we have metrics for this in Chromium?

mthiesse commented 3 years ago

I don't believe we have any metrics for this.

johannhof commented 3 years ago

I also lost track of this, sorry. We do have telemetry now, but, uh, it's sort of bad news. For a long time usage was extremely low but it's been growing quite a lot since around version 88 (released on April 19) for some reason. And unfortunately we can't even say what the protocol is because it's not in our allow-list of known protocols we record.

This is the curve in the data for release (which is not publicly accessible):

Interestingly this seems to correlate with a significant drop in same-origin iframes opening unidentified protocol handlers vs. other protocol handlers between 88 and 89. I haven't precisely run the numbers but the loss here is in the same ballpark as the gain from the above graph. The red line is for "other" protocols, and it's the only one in decline (ignore version 90):

You can see the same trends on the public dashboard for pre-release: https://mzl.la/2VKfDCl

So, unless we accidentally changed something in our reporting there seems to be a major service with an unknown protocol handler that just added a sandbox attribute to their iframes.

We were thinking we could try to identify this service by turning this protection on in our Nightly channel and see if anyone comes complaining. If our data is correct then at least 2000 people should be annoyed by that.

johannhof commented 3 years ago

An update that our updated telemetry data shows that this service is very likely Microsoft Teams (i.e. something sandboxed opening msteams: URLs). I'll try to reach out via our channels with Microsoft to figure out if they're in control of that iframe and could add allow-popups or something similar.

ArthurSonzogni commented 3 years ago

Hello, I have seen several bugs opened gravitating around this topic. So I would like to propose this change for Chrome: https://chromium-review.googlesource.com/c/chromium/src/+/3162979

TLDR: gatting sandboxed iframe navigation toward external protocol behind any of:

allow-popups
allow-top-navigation
allow-top-navigation-with-user-gesture (+ user gesture)

The rational is that iframe navigation toward external protocol are navigating the page toward a different application. So, this is not really an iframe navigation anymore. This can be seen somehow as a popup or a top-level navigation. Extending the scope of sandbox toward those might make sense. Developers are not expecting sandboxed iframe to open application.

Would you have any feedback?

Note 1: For now, I don't think this will apply to Android Webview, because external protocols are not supported, but we let the embedding App to intercept the requests. So continuing to let the embedding Android app decide is probably better than introducing a breaking change.
Note 2: Contrary to the patch, we might decide to ship only metrics without enforcing it for now. We will decide in code/feature review.

johannhof commented 3 years ago

Hi @ArthurSonzogni, thanks for the update. As I mentioned previously we're supportive of this on Firefox side, we'd have shipped this already but have so far been blocked by the above mentioned Teams breakage. I was planning to bring this issue in front of their team next week. It's great to know that Chrome is also planning on moving forward.

ArthurSonzogni commented 3 years ago

Great to know! For now, I will ship only metrics to measure how many page would be affected and maybe find what are the biggest legitimate users. I will keep a way to remotely enable the enforcement in case the compat issue is resolved quickly.

Independently of the compat issue, please let me know if you want to make any change to the particular shape of the proposed change to the specification.

ArthurSonzogni commented 3 years ago

Enabling this change would impact ~0.014% pages.

This is still a bit high for a removal. I took a look at who are going to be the most impacted website, and this is indeed coming from https://teams.microsoft.com origin. This confirms what @johannhof already found. Out of curiosity, @johannhof, did you received any update from Microsoft Teams so far?

That's aggregated UKM with noise and I don't have enough count to get additional origins so far. I will probably get more later in beta/stable.

johannhof commented 3 years ago

@ArthurSonzogni we reached out to them privately but didn't get a response yet, maybe we can bring someone on their team into this issue... @hpsin would you happen to know a Teams engineer who can help us with this? Thanks!

hpsin commented 3 years ago

I've dropped a note internally and included you Johann, thank you for the heads up.

ArthurSonzogni commented 2 years ago

I've dropped a note internally and included you Johann, thank you for the heads up.

Thanks @hpsin! As far as I can see on the metrics, the pages broken by this new restrictions are still largely dominated by https://teams.microsoft.com . I don't have access to the bug you mentioned, can I ask you for its status?

FYI I warned the second most impacted website https:// txxx.com and asked if this would impact them positively (breaking malicious ads) or negatively (breaking their website).

I don't have more website to contact for now, because count is low and the metrics I have access are aggregated / noised / clamped to preserve privacy. I will likely get more in a week when Chrome M97 will reach stable. I will contact the few major website once I know who they are. Once https://teams.microsoft.com usage is down, I will enable the feature by default.

ydogandjiev commented 2 years ago

This change will break in-production experiences for millions of Teams users who leverage embedded Teams Apps to complete their day to day work and studies. Is there a way for us to re-enable custom protocols (or some allowed list of these) in our embedded iframes? If not, can we please make sure that's added before rolling out this feature. Iframes are used for a lot more than just ads on the web; can we please stop hamstringing their functionality without providing configurable options for the parent page to turn off or adjust these new limitations?

ArthurSonzogni commented 2 years ago

@ydogandjiev Thanks for the update! We are waiting on usage to go down for https://teams.microsoft.com before enabling this security mitigation. You can be reassured ;-) There are ways for the parent to allow those navigation from the sandboxed frame. They can add allow-top-navigation sandbox flag for instance. There are other possibilities, I will let you take a look at the specification above. One can also use postMessage and implement their own logic, if using sandbox flags doesn't fit.

Could you help us figure out what are the blockers for this website?

ArthurSonzogni commented 2 years ago

Developers from https:// txxx.com (the second largest impacted website) have been super reactive to my questions. They confirm they would be positively impacted by this change:

Reply

``` Dear Arthur, Thank you very much for your personal attention to our project. These technologies are used by advertisers for malicious activity. We could not even imagine that this is still possible in modern browsers, and we are very grateful to you for this update. ```

ydogandjiev commented 2 years ago

@ydogandjiev Thanks for the update! We are waiting on usage to go down for https://teams.microsoft.com before enabling this security mitigation. You can be reassured ;-) There are ways for the parent to allow those navigation in the sandboxed frame.

Thank you for the reassuring reply, @ArthurSonzogni! Please understand that Teams uses iframes in ways that few other web apps do. We have built an entire platform that leverages iframes to host other 1P and 3P web apps inside of Teams in order to complete powerful end-to-end user scenarios. And developers have built thousands of apps on top of this platform to enhance the productivity of our shared customers. You can read more about the parts of our Platform that use iframes here:

They can add allow-top-navigation sandbox flag for instance.

This is not an option for us because we do not want to allow our embedded apps to re-navigate the top level page away from Teams. Doing so could allow a malicious Teams App to take the user to an impostor Teams site and spoof them into providing their corporate credentials to it. Even if not used for malicious reasons, the power to re-navigate the top level window would allow Teams Apps to negatively impact our customers' experience with Teams.

One can also use postMessage and implement their own logic, if using sandbox flags doesn't fit.

We already have an executeDeepLink API in our Client SDK that can do this; however, we've only guided our app developers to use this API when executing Teams-specific deep links. Forcing them to call this API for every single protocol deep link in their page is going to be a huge burden on our developers. One of the main goals of our platform from the start has been to keep Teams Apps as close to standard web apps as possible. Every time we ask developers to do something custom in Teams, we are forcing them to move away from web standards and ultimately undermining these standards in the long term (especially if the Teams Platform continues to be successful).

There are other possibilities, I will let you take a look at the specification above.

I don't think anything other than a mechanism for allowing custom protocol execution from within an iframe will address our concerns here. Please do not push this change without providing such a mechanism. Doing so will create significant d-sat for millions of Microsoft Teams users.

Could you help us figure out what are the blockers for this website?

As described above, we have built a platform for Microsoft Teams (and soon the rest of M365) which leverages iframes to host web apps built by other Microsoft organizations and partners from across the industry (e.g. Asana, Atlassian, monday.com, SurveyMonkey, etc.). Many of these embedded apps leverage custom protocol links to complete their scenarios. For example, the Help app uses ms-settings: links to take users to the Windows Settings app so they can fix configuration problems. If you block custom protocol execution from inside of iframes then many of these apps will break. The only way to roll out such a change without impacting hundreds of apps and millions of their users would be to allow the top-level page (i.e. Teams) to have oversight and control over what protocols can be executing from the iframes we create. That way, we can build our own mechanism for deciding which apps can execute which custom protocols and give our enterprise admins oversight.

We are still rolling out the fixes for all the issues caused by the last major Chromium change to iframes (that didn't take our scenarios into consideration): https://dev.chromium.org/Home/chromium-security/deprecating-permissions-in-cross-origin-iframes

Please don't do this to us and our customers again!

mikewest commented 2 years ago

Hi @ydogandjiev!

Even if not used for malicious reasons, the power to re-navigate the top level window would allow Teams Apps to negatively impact our customers' experience with Teams.

To me, accepting the danger of top-level navigation should also require accepting the danger of navigation to protocols which spawn activity in native applications. While I recognize that Teams has built functionality around this behavior, it doesn't seem reasonable to me for that functionality to take precedence over the more general concern that I hope we'd all share around privilege escalation.

The only way to roll out such a change without impacting hundreds of apps and millions of their users would be to allow the top-level page (i.e. Teams) to have oversight and control over what protocols can be executing from the iframes we create.

Doesn't something like the executeDeepLink API you mentioned earlier provide exactly this control, by routing potentially-dangerous requests through to an ancestor that's a) not sandboxed, and b) able to make policy decisions about which protocols a specific frame is expected to be able to execute?

While I understand that it would require developers to do some additional work to support the new model, it's something you could ease into by first relaxing your sandbox as @ArthurSonzogni suggested above, and then tighten the sandbox again as your developer community adjusts.

From a web platform perspective, that forces Teams to accept the additional risk of a weakened sandbox in order to support its specific needs, while providing default protection to the rest of the web. That seems like the right outcome to me.

ydogandjiev commented 2 years ago

To me, accepting the danger of top-level navigation should also require accepting the danger of navigation to protocols which spawn activity in native applications. While I recognize that Teams has built functionality around this behavior, it doesn't seem reasonable to me for that functionality to take precedence over the more general concern that I hope we'd all share around privilege escalation.

I don't see how the two are at all related. The browser asks for confirmation before allowing a custom protocol link to be executed whereas top-level navigation doesn't do that. In our Electron-based Desktop Client, we have a list of protocols that we allow our apps to execute and block all others. We have never heard of a custom protocol issue from either our customers, developers, or bounty hunters.

Unlike untrusted ads, Teams vets all the embedded apps deployed to our global catalog and give our customers full control over the ones that they want to enable for use in their organization. However, that doesn't mean we blindly trust the code running in these apps. The promises we make to our customers are that:

Tenant admins will have full control over which embedded apps can be used by members of their organization
Embedded apps will not be able to access Teams and other enterprise data without the necessary user and/or admin consent
Embedded apps will be clearly identified to the user as different from Teams
Embedded apps will not affect the core Teams experience itself (i.e. chats & meetings)

If we allowed top-level navigation we wouldn't be able to keep half of these promises to our customers. As I indicated in my previous response, this is not an option for us. Why is allowing top-level navigation the only option here? Why can't this feature also provide a way for top level pages to specify exactly what protocols they want to allow the iframe to execute? For example, via an allow-protocols attribute:

<iframe allow-protocols="tel ms-settings ms-teams"> </iframe>

To achieve your security goals, you could still block all custom protocols by default and require developers to explicitly enable the ones they want to allow. Is this approach something which has been considered here?

Doesn't something like the executeDeepLink API you mentioned earlier provide exactly this control, by routing potentially-dangerous requests through to an ancestor that's a) not sandboxed, and b) able to make policy decisions about which protocols a specific frame is expected to be able to execute?

So your advise is to tell our thousands of developers that anywhere they have a simple anchor tag with an href pointing to a custom protocol link they now need to jump through extra hoops just to make that work in their Teams App? That seems completely unacceptable and will ultimately result in a lot of developer d-sat and a long tail of bugs related to this issue in our apps. It also means more divergence from the way standard web apps work. Do you really want Microsoft pulling developers away from these standards like we did back in the day of IE6? Please don't force us to do that again; it serves no good for anyone across the industry.

mikewest commented 2 years ago

Hey @ydogandjiev!

Unlike untrusted ads, Teams vets all the embedded apps deployed to our global catalog and give our customers full control over the ones that they want to enable for use in their organization.

It seems like we're coming at this from different perspectives. I appreciate the work y'all have surely done to understand the set of applications deployed to your global catalog, and the controls you've surely put in place to prevent bad outcomes. While those controls are likely to provide Teams with some protection against bad actors, they're unlikely to be effective at protecting developers generally from the activities of embedded content.

If we allowed top-level navigation we wouldn't be able to keep half of these promises to our customers.

I think you wouldn't be able to keep 4, insofar as the frame could navigate the Teams experience from one Teams page to another. The rest seem independent from sandbox behavior. And, really, 4 also seems difficult to uphold, insofar as these frames can presumably navigate themselves or their descendents to Teams' URLs on the one hand, or initiate navigation to the msteams: protocol on the other.

Why is allowing top-level navigation the only option here?

It's not the only option. It is an option that would work immediately, with code browser vendors have already written, tested, and shipped.

<iframe allow-protocols="tel ms-settings ms-teams"> </iframe>

This is certainly a plausible approach. It's also a bigger change with more implications for the platform. There's some work to be done to determine how it would interface with {Permissions,Document} Policy on the one hand, and HTML on the other. As small examples: would this policy cascade through to frames embedded in this frame (like sandbox does)? Would it apply to windows popped up from the frame (as sandbox does)? What protocols do we allow regardless of the attributes' presence (presumably http and https? What about urn: for bundles? Or any of HTML's safelisted schemes that might be registered?

There are absolutely answers we could define for the questions this approach would raise. It's not clear to me that this feature is worth spending the time on, but perhaps someone at Microsoft is interested in sketching this proposal out in more detail?

(An alternative alternative would be to mint a new sandbox attribute, allow-custom-protocol-navigation or similar. That seems like less work, though it's certainly also less granular.)

So your advise is to tell our thousands of developers that anywhere they have a simple anchor tag with an href pointing to a custom protocol link they now need to jump through extra hoops just to make that work in their Teams App?

If you replace "simple anchor tag with an href pointing to a custom protocol link" with "sandbox escape", then the extra hoops sound pretty reasonable.

It also means more divergence from the way standard web apps work.

From the browser's perspective, custom protocols cause arbitrary code-execution with implementation-defined UX that depends upon native applications running outside the web's context. Electron is likewise pretty explicitly adopting a threat model quite different from the web's. Appealing to "standard web apps" and to the value of the standards process in their defense seems somehow out of place.

mikewest commented 2 years ago

/cc @ericlaw1979 re:Edge's interest in the above.

ydogandjiev commented 2 years ago

Hey @mikewest!

While I recognize that Teams has built functionality around this behavior, it doesn't seem reasonable to me for that functionality to take precedence over the more general concern that I hope we'd all share around privilege escalation.

We at Microsoft share all of these concerns as well. All we are asking for here is a solution that closes these privilege escalation vectors without breaking our existing Teams Apps and degraded experience for millions of students, teachers, office workers, and other Teams users. At least with the device permissions changes, a mechanism was provided for the parent page to re-enable the functionality. But here, it seems like you are choosing to not even provide such a mechanism. Why is that?

If you replace "simple anchor tag with an href pointing to a custom protocol link" with "sandbox escape", then the extra hoops sound pretty reasonable.

Most of our app developers today are not going out of their way to escape the sandboxed iframe we host them in. I don't think many of them will care about this change until it breaks their apps and our customers start opening incident tickets.

(An alternative alternative would be to mint a new sandbox attribute, allow-custom-protocol-navigation or similar. That seems like less work, though it's certainly also less granular.)

Yes, this would at least allow us to maintain the current functionality of our product. Of course, a more granular option that specifies the exact protocols we want to allow or lets the top level page provide a callback which can make the determination would be ideal.

This is certainly a plausible approach. It's also a bigger change with more implications for the platform. There's some work to be done to determine how it would interface with {Permissions,Document} Policy on the one hand, and HTML on the other. As small examples: would this policy cascade through to frames embedded in this frame (like sandbox does)? Would it apply to windows popped up from the frame (as sandbox does)? What protocols do we allow regardless of the attributes' presence (presumably http and https? What about urn: for bundles? Or any of HTML's safelisted schemes that might be registered?

I would expect a new allow-protocols attribute to also affect any nested iframes and popups (so as to prevent the hosted app from bypassing the restrictions we set). As for deciding which protocols are allowed by default, isn't that something you've had to determine as part of the "disable custom protocols" change? I would assume that list is the same. Having said that, I am really not a browser engineer so my perspectives here are mostly those of a web app develop. I am currently on break for the Holidays but will try to find some folks from the Edge team who can help chime in here too.

ArthurSonzogni commented 2 years ago

(back from vacation)

Thank you very much @mikewest!

It feel unlikely to me, someone to be willing to take the time to ship a whole new feature like allow-protocols, with the only incentive being to unblock this. The inheritance and the interactions with many other features seems complex to define and to implement (as outlined above). It would represent a serious technical debt to maintain once shipped. That's an interesting concept though to explore if someone is willing to.

On the other side, minting a new "allow-custom-protocol-navigation" sandbox flags seems extremely easy and its scope seems limited. If this can unblock shipping this security mitigation, I can work on a spec PR and the implementations. First, I would like to know what others think about it? In particular: @annevk and/or @cdumez.

My thought about the sandbox flag

1. How would this interact with `"allow-top-navigation"` / `"allow-top-navigation-by-user-activation"` / `"allow-popups"`? I guess `"allow-top-navigation"` & co should supersedes it. Allowing navigations should be strictly laxer than allowing the ones toward an external application. Nevertheless, I feel slightly uncomfortable being more permissive with external protocols than the supported ones `¯\_(ツ)_/¯` Then, this would require only adding one line to the specification: ``` If browsing context is not a top-level browsing context, and none of: - sandboxFlags contains the "allow-top-navigation-by-user-activation" token, and has hasTransientActiviation is true. - sandboxFlags contains the "allow-top-navigation" token. - sandboxFlags contains the "allow-popups" token. - sandboxFlags contains the "allow-custom-protocols-navigation" token. is true, then return without invoking the external software package. ``` 2. What if the initiator is in the main frame? This is a case where we don't block external protocol in the [spec](https://github.com/whatwg/html/pull/7124), if we do block, this would impact more than the current ~0.014% pages, without being significantly higher I suppose. If we don't block, the name "allow-custom-protocol-navigation" would be slightly confusing. I guess it is okay, because "allow-top-navigation" works similarly: we never prevent a top-level document from navigating the main frame.

annevk commented 2 years ago

@ArthurSonzogni I tend to agree with your assessment, though allowing custom protocols from opaque origins might still be tricky UI-wise. This should probably move into its own issue at this point?

ArthurSonzogni commented 2 years ago

Thanks!

[...] might still be tricky UI-wise. This should probably move into its own issue at this point?

This is tricky, but this is the current behavior. @annevk I am wondering if you are talking about moving the idea of "allow-custom-protocol-navigation" on its own topic, or you are talking about discussing if opaque origin can navigate toward custom protocols.

I guess this is the second and we should continue discussing the first here?

annevk commented 2 years ago

They kinda seem like the same topic to me given the current status of the specification, but I meant the first. This issue is closed after all.

ArthurSonzogni commented 2 years ago

Hey @ydogandjiev, both Safari (link) and Chrome (link) implemented the new restriction and the allow-custom-protocol-navigation sandbox flag.

About Chromium, here the schedule:

M100: Devtool warning only.
M103: Enforced.

See the associated release dates on: https://chromiumdash.appspot.com/schedule

Could you please confirm someone working on Microsoft Team will appropriately handle this, if it is not already done? I guess by adding the allow-top-navigation-to-custom-protocols sandbox flag where it matters to you?

This can be tested early using one of:

google-chrome --enable-features=SandboxExternalProtocolBlockedWarning
google-chrome --enable-features=SandboxExternalProtocolBlocked

ydogandjiev commented 2 years ago

Thanks @ArthurSonzogni! I have scheduled the work to make the necessary sandbox attribute changes in my team's backlog. Appreciate all the work you've done here to accommodate our use case and those of our app developers/customers.

ArthurSonzogni commented 2 years ago

@ydogandjiev Thanks! Both Firefox and Safari enabled it. I am going to enable it by default in Chrome today for M103 as it was scheduled.

I still see: https://teams.microsoft.com and https://teams.live.com to be the two top impacted websites on 2022-04-17 by far. M103 canary users will start to be blocked in 2-3 days after I land the patch. Please let me know if anything.

ydogandjiev commented 2 years ago

Hey @ArthurSonzogni, it took us a while to get the change through our security review process. We are only picking up the work in our current sprint which means that it won't hit production until end of May. Please consider holding off on enabling the feature by default until then.

ArthurSonzogni commented 2 years ago

@ydogandjiev End of may sounds good. This means this will be ready for M103 beta:

M103 Branch Thu, May 12, 2022 M103 Beta Promotion Thu, May 26, 2022 M103 Stable Release Tue, Jun 21, 2022

I can delay landing the patch for a few days, to avoid annoying canary users. However, I need to land it as scheduled for M103, before the branch cut.

ydogandjiev commented 2 years ago

Thanks @ArthurSonzogni! The corresponding change in Teams has been checked in and will be rolling out in our May release train.

ericlaw1979 commented 2 years ago

The original fix for this in Teams was to add allow-top-navigation-by-user-activation which was effective for Safari, but not for Chrome. The reason, I suspect, has to do with how Chrome attributes user activation. If you use JavaScript to create a frame targeting some-custom-protocol:, Chrome deems that frame as not having had a user-activation, and it blocks the protocol invocation; it does not consider the activation state of the document that created the frame. allow-top-navigation-by-user-activation does allow the protocol in Chrome if you have a link in a sandboxed frame and the user clicks on that link.

In contrast, Safari appears to allow the invocation in the initial-nav-of-a-subframe scenario-- the activation of the parent document allows the launch of the protocol for the iframe.

Teams will need to set allow-top-navigation-to-custom-protocols instead to fix their code.

Test cases can be found at https://webdbg.com/test/protocol/#test-sandboxed

ArthurSonzogni commented 2 years ago

Thanks Eric!

I can't really tell which user activation behavior is right... I am unaware of any existing mechanisms to forward it from the parent to the iframe's document in Chrome. It might exist, but I am unfamiliar with this feature. I will ask knowledgeable folks. Your explanation is really useful!

I am quite surprised allow-top-navigation-to-custom-protocols was not used on MsTeams, given that it was requested/discussed here, and created mostly to fit MsTeam constraints ;-)

Anyway, it looks like it is going to be used at the end.

Was the problem discovered by MsTeam's developers while verifying adding sandboxing flag was correctly working? If this was discovered more recently by users, when do you expect the second patch to become effective?

ericlaw1979 commented 2 years ago

@ArthurSonzogni - It turns out that Teams has two scenarios where this restriction comes into play

When Teams is hosting other applets (like e.g. OneNote) that potentially want to open their native clients (this is the scenario ydogandjiev was fixing, and he did indeed use a-t-n-t-c-p), and
When Teams' "Launcher" webpage is trying to launch the native Teams client to join a meeting.

That second scenario (which seems likely to be the primary scenario, in terms of volume and what y'all were seeing in telemetry) was using a-t-n-b-u-a, which worked in Safari but not Chrome/Edge. We've requested that the owners update the Launcher page to use the a-t-n-t-c-p directive to avoid the lack of a required gesture.

(To be honest, I'm not sure why a sandbox is being used at all in the Launcher scenario; AFAIK, the entire flow is 1st party content under Teams' control).

spackmat commented 2 years ago

(To be honest, I'm not sure why a sandbox is being used at all in the Launcher scenario; AFAIK, the entire flow is 1st party content under Teams' control).

To add some more question marks on this: Reload the page in a responsive view (link any smartphone selected in devtools) and observe the iframe vanish at all and the buttons showing up on the main document. Doing that was the first known workaround for those poor desktop Safari users that could not join Teams meetings anymore via the link in invitation emails. So why is that sandboxed iframe there at all? We'll never know.

Currently, the join page uses "allow-top-navigation-to-custom-protocols" so that nasty problem is gone there for newer browsers supporting that.

whatwg / html

Disable custom protocols in sandboxed iframe #2191

5289 might obviate the need for this somewhat.