Closed Bug 1208956 (CVE-2015-7191) Opened 4 years ago Closed 4 years ago

Universal XSS with browser_fallback_url extra in intent: URL on Fennec

Categories

(Firefox for Android :: General, defect)

44 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 44
Tracking Status
firefox41 --- wontfix
firefox42 + verified
firefox43 + fixed
firefox44 + fixed
firefox-esr38 --- unaffected

People

(Reporter: sdna.muneaki.nishimura, Assigned: mcomella)

Details

(Keywords: sec-high, Whiteboard: [adv-main42+])

Attachments

(2 files, 1 obsolete file)

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.
Flags: sec-bounty?
Have a look at this Mike.
Flags: needinfo?(michael.l.comella)
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
Flags: needinfo?(michael.l.comella) → needinfo?(abillings)
Assignee: nobody → michael.l.comella
I have no idea. I am but a program manager.

We could pull in a fennec developer or two.
Flags: needinfo?(abillings)
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.
Thanks for the info, Muneaki.

Handling only http & https urls seems effective to me and WebURLFinder seems like an overly complicated solution.
This follows Chrome's behavior.
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).
Attachment #8669961 - Flags: review?(nalexander)
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.
Attachment #8669961 - Flags: review?(nalexander) → review+
(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.
Attachment #8669961 - Attachment is obsolete: true
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.
Attachment #8670852 - Flags: review+
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.
Attachment #8670852 - Flags: sec-approval?
So this doesn't affect 41?

Sec-approval+ for trunk. Can you nominate the patch for branches?
Attachment #8670852 - Flags: sec-approval? → sec-approval+
(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?
(In reply to Michael Comella (:mcomella) from comment #14)
> Sure - release (41) too?

Oh, I see 41 was marked WONTFIX - nevermind.
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
Attachment #8670852 - Flags: approval-mozilla-beta?
Attachment #8670852 - Flags: approval-mozilla-aurora?
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.
Attachment #8670852 - Flags: approval-mozilla-beta?
Attachment #8670852 - Flags: approval-mozilla-beta+
Attachment #8670852 - Flags: approval-mozilla-aurora?
Attachment #8670852 - Flags: approval-mozilla-aurora+
Let's try to verify this fix once it lands.
Flags: qe-verify+
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!
Flags: needinfo?(michael.l.comella)
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
Flags: needinfo?(michael.l.comella)
https://hg.mozilla.org/mozilla-central/rev/429dbd2ac169
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Group: firefox-core-security → core-security-release
Can you please give me some guidelines on how can I test that this issue is fixed?
Flags: needinfo?(michael.l.comella)
(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
Flags: needinfo?(michael.l.comella)
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
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main42+]
Alias: CVE-2015-7191
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.
Keywords: sec-criticalsec-high
Group: core-security-release
Hi Liz,

Is the qe-verify+ flag still valid?

Thank you!
Flags: needinfo?(lhenry)
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!
Status: RESOLVED → VERIFIED
Flags: needinfo?(lhenry)
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.