1357377 - [meta] Limit intent filters and check incoming intents against whitelist

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, enhancement, P3)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Follow-up from bug 1338867. Especially read Daniel's comment here: bug 1338867 comment 32 (Answering questions from bug 1338867 comment 30).

(A) We are registering an Intent filter for URLs using the javascript:// scheme [1]. There are concerns that this could be or become exploitable. If not needed we should remove this filter (Although note that Chrome registers for javascript:// URLS too [2]).

(B) Intent filters control which URLs are redirected to us by the Android system. However third-party apps (and maybe even web pages through intent://) can dispatch arbitrary intents to us. Our intent handling code does not care about the incoming URL and redirects it to Gecko happily [3][4][5]. It might be advisable to check incoming intents against a whitelist that matches the intent filters.

(C) We allow about:// URLs in intents too. See Daniel's concerns about parameterized about URLs (bug 1338867 comment 32). If not needed we should think about removing this intent filter too (Again, chrome listens to about:// URLs too [6]). This one might be a bit more tricky because there could be things like notifications linking to about:accounts etc.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#100
[2] https://cs.chromium.org/chromium/src/chrome/android/java/AndroidManifest.xml?l=187
[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#2166-2175
[4] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Tabs.java#937
[5] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Tabs.java#966
[6] https://cs.chromium.org/chromium/src/chrome/android/java/AndroidManifest.xml?l=186

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Flagging for triage and adding core team to CC.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

:nalexander, :rnewman: Any concerns or history background from your side? :)

Comment 3

[Nick Alexander :nalexander [he/him]]

See also Bug 1356893, which would be mitigated by the same approach.

With respect to about:accounts, an attacker who could download the contents of about:accounts might be able to determine your email address and signed in status (and, if something has gone seriously wrong with accounts.firefox.com or the WebChannel interaction security, your account credentials).  Per https://bugzilla.mozilla.org/show_bug.cgi?id=1338867#c34, I think we could avoid requiring opening external about:accounts URLs easily enough, and I'm not aware of notifications opening about:accounts -- but Grisha would know better (ni? to him to comment).

There's a lot of valuable discussion about about:accounts in the implementation ticket: https://bugzilla.mozilla.org/show_bug.cgi?id=1191067, including this gem: "... but in the crazy world of the malicious Web, about:accounts feels safer if someone is trying to trick the user in some way."  It's ironic that we're now working around about:accounts being "exploited" more directly than bare URLs!

As to whitelisting, is it the case that Android apps use intent://app-specific-scheme://... to launch a URL in their specific App?  (I doubt it; that might not even be a legal URL to put into an Intent.)  I don't understand the "intent of intent://", if you will -- it's supposed to be a very general mechanism for integrating with the apps on a device, so I think I'd prefer a blacklist of things that we know can be used to attack Fennec rather than a whitelist of things we allow.  Otherwise, what's the point of supporting intent:// at all?

Comment 4

[Richard Newman [:rnewman]]

Nothing further to add, but mkaply might.

Comment 5

[Mike Kaply [:mkaply]]

The main use of intents by partners is an external app using an install referrer intent:

https://wiki.mozilla.org/Mobile/Distribution_Files#Testing_distribution_download

So as long as that still works, I don't have a problem with any changes around this.

Comment 6

[:Gijs (he/him)]

(In reply to Nick Alexander :nalexander from comment #3)
> See also Bug 1356893, which would be mitigated by the same approach.
> 
> With respect to about:accounts

We should just be preventing access to about: pages on the basis of URI flags. This would prevent access to most about: pages besides about:blank and any others that are *explicitly* marked as content-linkable by having both the URI_SAFE_FOR_UNTRUSTED_CONTENT and MAKE_LINKABLE flags.

Comment 7

[:Grisha Kruglov]

(In reply to Nick Alexander :nalexander from comment #3)
> I'm not aware of notifications opening about:accounts

Neither am I. Our notifications at the moment are pending intents linking to a few Fx*activities which then internally open about:accounts... urls when appropriate.

> As to whitelisting, is it the case that Android apps use
> intent://app-specific-scheme://... to launch a URL in their specific App? 
> (I doubt it; that might not even be a legal URL to put into an Intent.)

Check out https://developer.chrome.com/multidevice/android/intents

E.g. we'll want to handle something like intent://fxa-signin?signin=magictoken#Intent;scheme=firefox;package=org.mozilla.firefox;end to deep link into about:accounts?signin=magictoken.

> don't understand the "intent of intent://", if you will -- it's supposed to
> be a very general mechanism for integrating with the apps on a device, so I
> think I'd prefer a blacklist of things that we know can be used to attack
> Fennec rather than a whitelist of things we allow.  Otherwise, what's the
> point of supporting intent:// at all?

An explicit whitelist feels much safer, if only from the "we don't know what we don't know" point of view. I'm not very familiar with the intent ecosystem on android (yet!), so I can't comment on that aspect.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to :Gijs from comment #6)
> (In reply to Nick Alexander :nalexander from comment #3)
> > See also Bug 1356893, which would be mitigated by the same approach.
> > 
> > With respect to about:accounts
> 
> We should just be preventing access to about: pages on the basis of URI
> flags. This would prevent access to most about: pages besides about:blank
> and any others that are *explicitly* marked as content-linkable by having
> both the URI_SAFE_FOR_UNTRUSTED_CONTENT and MAKE_LINKABLE flags.

The point is that it's not possible at this time to disambiguate:

1. about: pages opened "by Fennec" (for FxA integration)
2. about: pages opened by untrusted Android Apps at this time
3. about: pages opened by untrusted web content via intent://about:... URLs

They all arrive to Fennec's Intent handler without disambiguating features.  We can address 1. ourselves, since we control the producer.  We mostly do the correct thing for 2. already; using URI flags to limit this load at the Gecko level is a great idea, if it's not already happening.  3. is the subject for discussion.

Comment 9

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[triage]
we should track this. 
p3 for we don't really see specific use case introducing significant sec risk

Comment 10

[:Gijs (he/him)]

(In reply to Nick Alexander :nalexander from comment #8)
> (In reply to :Gijs from comment #6)
> > (In reply to Nick Alexander :nalexander from comment #3)
> > > See also Bug 1356893, which would be mitigated by the same approach.
> > > 
> > > With respect to about:accounts
> > 
> > We should just be preventing access to about: pages on the basis of URI
> > flags. This would prevent access to most about: pages besides about:blank
> > and any others that are *explicitly* marked as content-linkable by having
> > both the URI_SAFE_FOR_UNTRUSTED_CONTENT and MAKE_LINKABLE flags.
> 
> The point is that it's not possible at this time to disambiguate:
> 
> 1. about: pages opened "by Fennec" (for FxA integration)
> 2. about: pages opened by untrusted Android Apps at this time
> 3. about: pages opened by untrusted web content via intent://about:... URLs
> 
> They all arrive to Fennec's Intent handler without disambiguating features. 
> We can address 1. ourselves, since we control the producer.  We mostly do
> the correct thing for 2. already; using URI flags to limit this load at the
> Gecko level is a great idea, if it's not already happening.  3. is the
> subject for discussion.

Sure. I guess what I'm trying to say is, we can treat all the access to about:* as (2/3), which will pretty much automatically mean that all of it (except ones marked both URI_SAFE_FOR_UNTRUSTED_CONTENT + MAKE_LINKABLE) will be blocked. I don't think (2) or (3) should be treated differently as far as access to about: is concerned - they're both untrusted. Inasmuch as (1) exists, as you note we can (presumably?) avoid going through intent://.

Comment 11

[Gerry Chang [:gchang]]

Track 54+/55+ as sec-high.

Comment 12

[Stephanie Ouillon [:arroway] (needinfo me)]

Wesley, this bug has been marked as sec-high, so it should be prioritized.

Comment 13

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Discussed with the team and this is not an easy path to fix.
Will investigate and see if we can take it in the next sprint. Suggest not to block 54 shipping.
Note: This issue exists across multiple versions.

Comment 14

[Frederik Braun [:freddy]]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #13)
> Discussed with the team and this is not an easy path to fix.
> Will investigate and see if we can take it in the next sprint. Suggest not
> to block 54 shipping.
> Note: This issue exists across multiple versions.

This was 3 weeks ago. Any updates?

Comment 15

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

There are too many items in the team's plate so we were not able to take this before SFO All Hands.
This remains P1 in our list. We'll update soon.

Comment 16

[Gerry Chang [:gchang]]

Too late for 54. Given no actions for the moment, mark 54 won't fix.

Comment 17

[Nevin Chen(Not active on Bugzilla)]

My action item:
If the intent data URI is in whitelist, I'll

Comment 18

[Nevin Chen(Not active on Bugzilla)]

My action item:
If the intent data URI is in whitelist, I'll
1. Check if we can remove javascript:// scheme
2. Only allow intent:// scheme for whitelisted URLs
3. Only allow about:// scheme for whitelisted URLs

Comment 19

[Nevin Chen(Not active on Bugzilla)]

Hi Daniel
Do you know if there's a white list for above purpose?
After consulting with some people I only found tracking protection related whitelist.
But for our purpose I can't find any good fit.
Thanks!

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

The idea behind the whitelist was to only accept schemes we explicitly registered for in the manifest (like http, https, ..). If someone sends a foobar:// intent then we should just drop it and not happily send it to gecko.

Comment 21

[Nevin Chen(Not active on Bugzilla)]

Attachment: 1357377.patch

WIP

Comment 22

[Nevin Chen(Not active on Bugzilla)]

Attachment: 1357377-1.patch

WIP. I think putting the check in LauncherActiivty is not useful cause I can't think of a use case it could be compromised with scheme ( AndroidManifest already protects it, right?) We should protect BrowserApp cause it's exported=true and has no intent filter to it.

But before that, It also need to have some test cases for
1. non-whitelisted scheme from web content.
2. non-whitelisted scheme from other apps.

Comment 23

[Nevin Chen(Not active on Bugzilla)]

I got a release tracking alert today for 56. But I think I don't have the time to prepare for all the test cases for this bug. Suggest to track it for 57.

Comment 24

[Daniel Veditz [:dveditz]]

(In reply to Nevin Chen [:nechen] from comment #19)
> Do you know if there's a white list for above purpose?

I meant a whitelist of schemes or, as Gijs suggested for about urls, doing some programmatic checking for properties of specific ones.

Comment 25

[Stephanie Ouillon [:arroway] (needinfo me)]

Nevin, you worked on a WIP patch a couple months ago, what are your plans? Any chance this could be fixed for 57?

Comment 26

[Nevin Chen(Not active on Bugzilla)]

Thanks for the NI
I'm too busy now. I try to squeeze this into our sprint planning.
NI EPM so he knows it.

Comment 27

[Wesly Huang (EPM)]

Not for Nightly 57.

Joe, we'll need to revisit the priority for Nightly 58 and hopefully can get this in. (then uplift maybe)

(In reply to Nevin Chen [:nechen] from comment #26)
> Thanks for the NI
> I'm too busy now. I try to squeeze this into our sprint planning.
> NI EPM so he knows it.

Comment 28

[Stephanie Ouillon [:arroway] (needinfo me)]

Wesly, do you have any insight regarding your team's future plans to fix this sec bug?

Comment 29

[Wesly Huang (EPM)]

(In reply to Stephanie Ouillon [:arroway] from comment #28)
> Wesly, do you have any insight regarding your team's future plans to fix
> this sec bug?

Sorry for late, Stephanie. We reviewed the plan for 59 Nightly cycle today and I'm afraid this one won't get priority. We may revisit this in the next cycle. Please feel free to let me know shall you have any question/concern. Thanks.

Comment 30

[Frederik Braun [:freddy]]

Wesly, can you help me got this moving along? If only by finding someone else to nudge.

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[triage] sec-high: P1.

Wesly is no longer working on fennec and I'm now the triage owner: please let me know if you have questions! Please note we don't have eng resources on fennec at the moment but Susheel is looking into it (NI).

Comment 32

[:sdaswani]

Mike put it on your critical list.

Comment 33

[Frederik Braun [:freddy]]

(In reply to :sdaswani from comment #32)
> Mike put it on your critical list.


Did you mean to assign this bug to someone or needinfo someone?
This comment of yours is 2 months old :-(

Comment 34

[:sdaswani]

Andreas, is this something we should be working on ASAP?

Comment 35

[Andreas Bovens [:abovens]]

If others flag it as critical, it seems like something we should look into with high priority. Can we split it up in simpler tasks (e.g. start with disallowing javascript:// already, tackle the rest later)?

Comment 36

[:sdaswani]

Putting it into 61, let's see how the Softvision folks break down the work.

Comment 37

[Frederik Braun [:freddy]]

+1 to what Andreas said. disallowing javascript:// would be a good first step.

Comment 38

[:sdaswani]

Adding Petru and Vlad to this so they can evaluate the work, not sure why I couldn't add Andrei.

Comment 39

[:sdaswani]

Frederic since this landed does it change the security rating? https://bugzilla.mozilla.org/show_bug.cgi?id=1480806

Comment 40

[Frederik Braun [:freddy]]

Removing javascript: is good and it does lower the security risk, as we are taking away the danger of executing JS in existing tabs.
We still allow the about protocol which can be problematic on a case-by-case basis, depending on the actual functionality. Most about pages do not perform any actions based on parameters, so I think we can easily call this sec-moderate now.

sec-moderate is generally something that needs another bug to be problematic.
FYI the ratings are explained here: https://wiki.mozilla.org/Security_Severity_Ratings

Comment 41

[Liz Henry (:lizzard)]

Since this was re-rated to sec-moderate, marking this as P2 for the fennec team.
It sounds like this may even be more of a sec-audit bug. Dan, what do you think?

Should we be opening specific followup issues for about pages?

Comment 42

[Chris Peterson [:cpeterson]]

snorp thinks Fenix already does this properly.

WONTFIX'ing this bug for Fennec because we're unlikely to fix it now that Fennec is in ESR 68 maintenance mode.