1168998 - Filter intent:// URIs use to prevent malicious attacks against the browser & installed apps

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

As detailed in [1], browsers opening intent:// URIs can have their activities opened via the intent:// URI. On other browsers, this lead to leaked user information but could potentially be more dangerous.

Additionally, linking to any arbitrary app can also dangerous, so the Intent.CATEGORY_BROWSABLE [2] category should be used to ensure the apps that are linked to are prepared to be linked from the browser.

The suggested fix in [1] is:

> // convert intent scheme URL to intent object
> Intent intent = Intent.parseUri(uri);
> // forbid launching activities without BROWSABLE category
> intent.addCategory("android.intent.category.BROWSABLE");
> // forbid explicit call
> intent.setComponent(null);
> // forbid intent with selector intent
> intent.setSelector(null);
> // start the activity by the intent
> context.startActivityIfNeeded(intent, -1);

I'll do some research into the side effects here.

[1]: http://www.mbsd.jp/Whitepaper/IntentScheme.pdf
[2]: https://developer.android.com/reference/android/content/Intent.html#CATEGORY_BROWSABLE

Comment 1

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

mbrubeck linked me to [1] for the security of Intents in general - may not be directly relevant here.

[1]: http://www.eecs.berkeley.edu/~daw/papers/intents-mobisys11.pdf

Comment 2

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

Actually, the issue here is not just intent:// URIs but all of uses Context.startActivity [1], Context.startActivityForResult [2], and Context.startActivities (unused) - we should audit these. The cleanest solution would be to move all of these interactions into a wrapper class (i.e. StartActivityUtils) to prevent this from occuring in the future.

Also, I believe we can write custom rules for Android Lint and maybe prevent these methods from accidentally being used in the future.

[1]: https://mxr.mozilla.org/mozilla-central/search?string=startactivity&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
[2]: [3]: https://mxr.mozilla.org/mozilla-central/search?string=startactivityforresult&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 3

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

(In reply to Michael Comella (:mcomella) from comment #2)
> Actually, the issue here is not just intent:// URIs but all of uses
> Context.startActivity [1], Context.startActivityForResult [2], and

Actually, I didn't think that through - it's just intent:// URIs, as in the linked article - any web content we may be passing around.

To be thorough, we should inspect all uses of Context.startActivity* to ensure they're not being called with an external URL, however.

Comment 4

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

It would appear that we open intent:// URIs using `GeckoAppShell.openUriExternal` [1]. We should also guard IntentHelper.openForResult, which comes from the same flow as IntentHelper.open -> GeckoAppShell.openUriExternal [2]. I can't follow all the details through Gecko but I discovered this code path by adding some log statements, then clicking the URIs on my test page [3]. It's not perfect science but following the Gecko code pathways is extremely challenging.

[1]: https://mxr.mozilla.org/mozilla-central/search?string=openuriexternal&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/HelperApps.jsm#198

[3]: https://people.mozilla.org/~mcomella/test/intent_uri.html

Comment 5

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

Attachment: Filter intent:// URIs

Comment 6

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

Testing strategy: I used a test page [1] to open test Activities at various specificity levels [2].

By testing against a build without this patch, I confirmed:
  * The Browsable category is now required for Activities to open
  * components set in urls are correctly nulled (though we currently prevent this from being specified with [3], though I expect that to be removed soon).

For time reasons, I did not test selector, but assume it is also set properly.

[1]: https://www.dropbox.com/s/1j6sjyyn90r9lnp/browsable.html?dl=0
[2]: https://github.com/mcomella/AndySandbox-studio/tree/browsable_tests

[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java?rev=22652196dd36#1203

Comment 7

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

(In reply to Michael Comella (:mcomella) from comment #6)
>   * components set in urls are correctly nulled (though we currently prevent
> this from being specified with [3], though I expect that to be removed soon).

Note that I also removed [3] and tested to make sure things work as expected.

Comment 8

[:Margaret Leibovic]

Comment on attachment 8616912 [details] [diff] [review]
Filter intent:// URIs

Review of attachment 8616912 [details] [diff] [review]:
-----------------------------------------------------------------

Nice find.

Comment 9

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

https://hg.mozilla.org/integration/fx-team/rev/51b3b983b6b3

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/51b3b983b6b3

Comment 11

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

Adding WG9s for additional context regarding bug 1175915.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

This shouldn't have been checked in without a security rating and sec-approval+.

https://wiki.mozilla.org/Security/Bug_Approval_Process

We need a rating on this now because I'm writing the Firefox 41 security advisories. Can you suggest a security rating for this? Seems like a sec-moderate or sec-high, depending on the actual result for Mozilla products.

Comment 13

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

(In reply to Al Billings [:abillings] from comment #12)
> Can you suggest a security rating for this? Seems like a
> sec-moderate or sec-high, depending on the actual result for Mozilla
> products.

There is no known exploit for Mozilla products – this is pre-emptive based on paper in comment 0 that details exploits against other browsers. iirc, in other browsers, the users' private browser data was stolen (e.g. cookies) so I'd say sec-high (i.e. "Theft of arbitrary files from local system"). I'll mark for sec-approval.

Comment 14

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

Comment on attachment 8616912 [details] [diff] [review]
Filter intent:// URIs

It's worth noting that the code from this patch has changed multiple times. bug 1182695 comment 0 contains a good summary of what has changed here.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
  For exploiters in the know, adding CATEGORY_BROWSABLE is probably a sure-fire way to know that something has changed – it's basically there to prevent exploits. So pretty easily, assuming there is an exploit to be constructed (i.e. it's browser-by-browser).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
  Yeah – the patch comment can be fairly obvious if you know about the CATEGORY_BROWSABLE exploit.

Which older supported branches are affected by this flaw?
  Anything before this landed.

If not all supported branches, which bug introduced the flaw?
  Presumably, links with custom URI schemes have this same issue (it's not outlined in the paper) and if so, it's been around for a long time. However, if the problem is only with intent:// uris, that was implemented in bug 851693.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
  No. This only affects intent:// uris which landed in the same version so there's no branch patch to be made.

How likely is this patch to cause regressions; how much testing does it need?
  It caused a few but I fixed them – see bug 1182695.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8616912 [details] [diff] [review]
Filter intent:// URIs

Per discussions with Dan Veditz, we made this a sec-want and it can just go in. Clearing the security advisory request.