1208956 - (CVE-2015-7191) Universal XSS with browser_fallback_url extra in intent: URL on Fennec

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Muneaki Nishimura]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.99 Safari/537.36

Steps to reproduce:

The following source code for Fennec allows universal XSS attack with the browser_fallback_url extra in intent: URL.
http://lxr.mozilla.org/mozilla-central/source/mobile/android/base/IntentHelper.java#186

Please see the live demo by Fennec.
https://mallory.csrf.jp/intent/fallback.html

In this page, parent document injects script code "alert(document.domain)" to nsa.gov opened in the iframe like this.
<iframe src="https://www.nsa.gov" onload="this.src='intent://any#Intent;scheme=httpz;S.browser_fallback_url=javascript%3Aalert%28document.domain%29;end'">


Actual results:

The injected code to nsa.gov via "S.browser_fallback_url=javascript%3Aalert%28document.domain%29" in the intent: URL is executed.


Expected results:

The injected code should not be executed.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Have a look at this Mike.

Comment 2

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

We open the uri via [1]:
  window.location.href = uri

With the fallback uri directly from the Intent [2].

Al, what is the proper way to sterilize a uri when opening it?

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/components/ContentDispatchChooser.js?rev=4a151c796844#68
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/IntentHelper.java?rev=b32b5a757f62#186

Comment 3

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

I have no idea. I am but a program manager.

We could pull in a fennec developer or two.

Comment 4

[Muneaki Nishimura]

Chromium allows to use only http: and https: scheme URL for fallback navigation.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/java/src/org/chromium/chrome/browser/UrlUtilities.java&q=isvalidfori&sq=package:chromium&type=cs&l=50

In Fennec, ShareDialog uses WebURLFinder for checking that given URL is safe web URL to open.
http://lxr.mozilla.org/mozilla-central/source/mobile/android/base/overlays/ui/ShareDialog.java#229

I think it can be used here.

Comment 5

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

Thanks for the info, Muneaki.

Handling only http & https urls seems effective to me and WebURLFinder seems like an overly complicated solution.

Comment 6

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

Attachment: Only open http* scheme in intent fallback uris

This follows Chrome's behavior.

Comment 7

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

Comment on attachment 8669961 [details] [diff] [review]
Only open http* scheme in intent fallback uris

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

Caveats:
* Nick, do you think WebURLFinder is preferable?
* The error message for non-http* uris kind of sucks ("Oops. Fennec can't load this page for some reason" on my local build).

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8669961 [details] [diff] [review]
Only open http* scheme in intent fallback uris

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

r+ with issues addressed.

As for WebURLFinder, it's too optimistic.  We want to be strict in what we accept here.

As for error messages: perhaps compare to Chrome?  In general, I care not at all.  We want to block the load; we need to message the property developer, not the user, and we can't do that.

::: mobile/android/base/IntentHelper.java
@@ +186,5 @@
>          // For this flow, we follow Chrome's lead:
>          //   https://developer.chrome.com/multidevice/android/intents
>          if (intent.hasExtra(EXTRA_BROWSER_FALLBACK_URL)) {
>              final String fallbackUrl = intent.getStringExtra(EXTRA_BROWSER_FALLBACK_URL);
> +            String urlToLoad;

nit: final?

@@ +188,5 @@
>          if (intent.hasExtra(EXTRA_BROWSER_FALLBACK_URL)) {
>              final String fallbackUrl = intent.getStringExtra(EXTRA_BROWSER_FALLBACK_URL);
> +            String urlToLoad;
> +            try {
> +                final String scheme = new URI(fallbackUrl).getScheme().toLowerCase(Locale.US);

Are we guaranteed to have a scheme?  http://docs.oracle.com/javase/7/docs/api/java/net/URI.html suggests not: getScheme() can be null.

Comment 9

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

(In reply to Nick Alexander :nalexander from comment #8)
> As for error messages: perhaps compare to Chrome?

Chrome doesn't show error pages – they take no action when you click the link.

> > +            String urlToLoad;
> 
> nit: final?

You can't use `final` in a try/catch because the variable may already be assigned to in the `catch` block.

> > +                final String scheme = new URI(fallbackUrl).getScheme().toLowerCase(Locale.US);
> 
> Are we guaranteed to have a scheme? 

Nope, will fix.

Comment 10

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

Attachment: Only open http* scheme in intent fallback uris

This follows Chrome's behavior.

Comment 11

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

Comment on attachment 8670852 [details] [diff] [review]
Only open http* scheme in intent fallback uris

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

Carry r+ forward.

Comment 12

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

Comment on attachment 8670852 [details] [diff] [review]
Only open http* scheme in intent fallback uris

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
  Probably pretty easily. We start to explicitly check http/https here so anyone who thinks about schemes as exploit vectors will immediately run to javascript://

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
  Yes – the checkin comment is pretty obvious.

Which older supported branches are affected by this flaw?
  Looks like 42+.

If not all supported branches, which bug introduced the flaw?
  bug 851693.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
  No, but I expect the patch to apply cleanly (and thus any branch patches will likely be easy to create).

How likely is this patch to cause regressions; how much testing does it need?
  It's a small change that that has already been audited for NullPointerExceptions and tested locally so I think it's unlikely to cause regressions.

Comment 13

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

So this doesn't affect 41?

Sec-approval+ for trunk. Can you nominate the patch for branches?

Comment 14

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

(In reply to Al Billings [:abillings] from comment #13)
> So this doesn't affect 41?

Sorry, I made a mistake – this was implemented in bug 1168980, which also landed in 41 so this does affect 41. Still, I don't think any significant changes were made.

Thanks for checking in!

> Sec-approval+ for trunk. Can you nominate the patch for branches?

Sure - release (41) too?

Comment 15

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

(In reply to Michael Comella (:mcomella) from comment #14)
> Sure - release (41) too?

Oh, I see 41 was marked WONTFIX - nevermind.

Comment 16

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

Comment on attachment 8670852 [details] [diff] [review]
Only open http* scheme in intent fallback uris

Approval Request Comment
[Feature/regressing bug #]: bug 1168980.
[User impact if declined]: Users clicking on intent URIs with fallback urls (which appear no differently in content from regular uris) are affected by the security vulnerability described in comment 0.

[Describe test coverage new/current, TreeHerder]: Local testing
[Risks and why]:
  * While we're using the same page load path, we're loading new error pages which could result in unexpected behavior in edge cases (e.g. I previously fixed an issue when we first introduced this code where we would load the new page in the selected tab rather than the context into which it was loaded so if the link was in a hidden iframe, we'd just override the tab content rather than not displaying it at all) 
  * We're doing some String manipulations which are prone to NullPointerExceptions & upper/lowercase errors

[String/UUID change made/needed]: None

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8670852 [details] [diff] [review]
Only open http* scheme in intent fallback uris

Approved for uplift to aurora and beta. Has had local testing.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Let's try to verify this fix once it lands.

Comment 19

[Carsten Book [:Tomcat]]

Hi Michael, since i do uplifts i was pinged by sylveste for this bug. 

Seems this didn't landed on m-i/fx-team / m-c so far.

I tried to checkin the attached patch myself (kind of self-checkin-needed :) but this failed with :

Tomcats-MacBook-Pro-2:fx-team Tomcat$ hg qimport /sheriffs/patch/bug1208956.patch && hg qpush && hg qrefresh -e 
adding bug1208956.patch to series file
applying bug1208956.patch
patching file mobile/android/base/IntentHelper.java
Hunk #1 FAILED at 18
Hunk #2 FAILED at 43
2 out of 3 hunks FAILED -- saving rejects to file mobile/android/base/IntentHelper.java.rej

could you take a look, thanks!

Comment 20

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

Attachment: Patch m-c rebased

Coordinated on IRC.

This patch broke from [1], which is only on m-c.

[1]: http://hg.mozilla.org/mozilla-central/diff/a3891c6b14ab/mobile/android/base/IntentHelper.java

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/429dbd2ac169

thanks michael!

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ba087659bfe7
https://hg.mozilla.org/releases/mozilla-beta/rev/2bfd512a01af

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/429dbd2ac169

Comment 24

[u421692]

Can you please give me some guidelines on how can I test that this issue is fixed?

Comment 25

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

(In reply to Mihai Pop from comment #24)
> Can you please give me some guidelines on how can I test that this issue is
> fixed?

You can build a page with an intent url [1] that has a fallback url. If the app is not installed, the fallback url should be opened – only fallback urls with http & https schemes should open. Note that unspecified encoding are not allowed.

[1]: https://developer.chrome.com/multidevice/android/intents

However, I already have some samples:

(In reply to Muneaki Nishimura from comment #0)
> Please see the live demo by Fennec.
> https://mallory.csrf.jp/intent/fallback.html

This may work though I can't remember.

My intent uri test page [2] should work. Specifically, there are tests for "fallback url", "fallback url w/ file://", and "fallback url w/ no scheme".

[2]: https://people.mozilla.org/~mcomella/test/uri.html

Comment 26

[u421692]

I've tested this on Firefox 42 Beta 6 and Firefox 42 Beta 8
Tested on the following pages:
https://mallory.csrf.jp/intent/fallback.html
https://people.mozilla.org/~mcomella/test/uri.html

Using https://mallory.csrf.jp/intent/fallback.html :
On Firefox 42 Beta 6 the injected code is executed, www.nsa.gov pop-up is displayed
On Firefox 42 Beta 8 the injected code is NOT executed, www.nsa.gov pop-up is NOT displayed (logcat: W/GeckoIntentHelper(9954): Fallback URI uses unsupported scheme: javascript)

Using https://people.mozilla.org/~mcomella/test/uri.html :
On Firefox 42 Beta 6 tapping on the 4th, 5th, 6th link on the page, opens the pages
On Firefox 42 Beta 8 tapping on the 4th, 5th, 6th link on the page opens the about:neterror page:
4th link: W/GeckoIntentHelper(10563): Fallback URI uses unsupported scheme: file
5th link: W/GeckoIntentHelper(10563): Fallback URI uses unsupported scheme: null
6th link: W/GeckoIntentHelper(10563): Exception parsing fallback URI

Marking this verified as fixed on Firefox 42 Beta 8

Comment 27

[Daniel Veditz [:dveditz]]

Unless we know of a way for content to load a privileged frame and inject into it, an unprivileged UXSS is usually rated sec-high.

Comment 28

[miralobontiu]

Hi Liz,

Is the qe-verify+ flag still valid?

Thank you!

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

No, it looks like this was verified in comment 26. So, at that point, the bug Status should have been changed to VERIFIED. I'll do that now. Thanks!