1208520 - (CVE-2015-7190) Firefox Search on Android allows to open an arbitrary activity with Fennec's privilege

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[Muneaki Nishimura]

Attachment: intent.pdf

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.42 Safari/537.36

Steps to reproduce:

The following commit introduced a security bug that allows any web contents to open an arbitrary activity with Fennec's privilege.
https://hg.mozilla.org/mozilla-central/rev/05645d479d89

This code is used by search widget on Android when a search result navigates to an intent: URL. Then this code directly use given intent: URL for starting activity. This is wrong it should be handled like Bug 851693 (e.g., adding Intent.CATEGORY_BROWSABLE).

The following is a possible exploitation scenario.
1. Open a search engine URL http://mallory.csrf.jp/search.html on Fennec
2. Long tap an text input in a page and register it as a search engine
3. In settings menu, choose it as a default search engine for search widget
4. Searche any words via search widget on the registered search engine, then the search result open a following intent: URL
intent://#Intent;action=org.mozilla.gecko.reportCrash;package=org.mozilla.fennec;component=org.mozilla.fennec/org.mozilla.gecko.CrashReporter;S.minidumpPath=%2fsystem%2fbuild.prop;end
5. Fennec opens the link and the private hidden crash reporter activity is shown by Fennec's own privilege.
6. Then the parameter minidumpPath=%2fsystem%2fbuild.prop given by the content makes crash reporter to open /system/build.prop file on Fennec's privilege
Then the following log may be put on adb if you use an Android device that OS version is before 4.4.
I/GeckoCrashReporter(29235): moving /system/build.prop to /data/data/org.mozilla.fennec/files/mozilla/Crash Reports/pending/build.prop

I attached a PDF introducing steps to reproduce.

Comment 1

[Muneaki Nishimura]

Note that a similar issue was once pointed out by Bug 1182140, but as far as I looked at *public* logs the source code I mentioned above has never been covered.

And about the example I wrote above that opens crash reporter from the search engine is just a possible scenario.
Here the malicious website can issue arbitrary intents, for example, the following intent URL allows a malicious search result page that is hosted via http(s): to launch a user's local file system/build.prop on a tab of Fennec.

intent:%2fsystem%2fbuild.prop#Intent;scheme=file;action=org.mozilla.gecko.BOOKMARK;package=org.mozilla.fennec;component=org.mozilla.fennec/org.mozilla.gecko.BrowserApp;end

Comment 2

[:Margaret Leibovic]

Ally or Mike, could one of you take this?

Comment 3

[Allison Naaktgeboren :ally]

yoink!

Comment 4

[Allison Naaktgeboren :ally]

Attachment: secBugAebitaryPrivs wip

needs testing

Comment 5

[Allison Naaktgeboren :ally]

There is a sizable problem it seems with 5+, the whole shebang crashes. While sucky, is not exploitable there at least.

Comment 6

[Allison Naaktgeboren :ally]

4.3 device also crashing

10-01 13:13:16.175    9824-9824/org.mozilla.fennec_mozilla E/GeckoCrashHandler﹕ >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
    java.lang.SecurityException: Permission Denial: starting Intent { act=org.mozilla.gecko.reportCrash dat=// pkg=org.mozilla.fennec cmp=org.mozilla.fennec/org.mozilla.gecko.CrashReporter (has extras) } from ProcessRecord{433b9d78 9824:org.mozilla.fennec_mozilla/u0a10070} (pid=9824, uid=10070) not exported from uid 10066
            at android.os.Parcel.readException(Parcel.java:1431)
            at android.os.Parcel.readException(Parcel.java:1385)
            at android.app.ActivityManagerProxy.startActivity(ActivityManagerNative.java:1947)
            at android.app.Instrumentation.execStartActivity(Instrumentation.java:1419)
            at android.app.Activity.startActivityForResult(Activity.java:3390)
            at android.app.Activity.startActivityForResult(Activity.java:3351)
            at android.support.v4.app.FragmentActivity.startActivityFromFragment(FragmentActivity.java:829)
            at android.support.v4.app.Fragment.startActivity(Fragment.java:897)
            at org.mozilla.search.PostSearchFragment$ResultsWebViewClient.shouldOverrideUrlLoading(PostSearchFragment.java:144)
            at android.webkit.CallbackProxy.uiOverrideUrlLoading(CallbackProxy.java:261)
            at android.webkit.CallbackProxy.handleMessage(CallbackProxy.java:363)
            at android.os.Handler.dispatchMessage(Handler.java:99)
            at android.os.Looper.loop(Looper.java:137)
            at android.app.ActivityThread.main(ActivityThread.java:5103)
            at java.lang.reflect.Method.invokeNative(Native Method)
            at java.lang.reflect.Method.invoke(Method.java:525)
            at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:737)
            at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
            at dalvik.system.NativeStart.main(Native Method)
10-01 13:13:27.558  10666-10670/org.mozilla.fennec_mozilla D/dalvikvm﹕ GC_CONCURRENT freed 425K, 6% free 9266K/9760K, paused 3ms+3ms, total 30ms
10-01 13:13:27.566  10666-10666/org.mozilla.fennec_mozilla D/TilesManager﹕ Starting TG #0, 0x4163d8e8
10-01 13:13:27.566  10666-10666/org.mozilla.fennec_mozilla D/TilesManager﹕ new EGLContext from framework: 579de278
10-01 13:13:27.566  10666-10666/org.mozilla.fennec_mozilla D/GLWebViewState﹕ Reinit shader
10-01 13:13:27.573  10666-10666/org.mozilla.fennec_mozilla D/GLWebViewState﹕ Reinit transferQueue
10-01 13:13:29.988  10666-10666/org.mozilla.fennec_mozilla D/Telemetry﹕ SendUIEvent: event = search.1 method = homescreen timestamp = 14090867 extras = history
10-01 13:13:37.659  10666-10666/org.mozilla.fennec_mozilla E/PostSearchFragment﹕ Error getting host from URL loading in webview
    java.net.MalformedURLException: Unknown protocol: intent
            at java.net.URL.<init>(URL.java:184)
            at java.net.URL.<init>(URL.java:127)
            at org.mozilla.search.PostSearchFragment$ResultsWebViewClient.shouldOverrideUrlLoading(PostSearchFragment.java:113)
            at android.webkit.CallbackProxy.uiOverrideUrlLoading(CallbackProxy.java:261)
            at android.webkit.CallbackProxy.handleMessage(CallbackProxy.java:363)
            at android.os.Handler.dispatchMessage(Handler.java:99)
            at android.os.Looper.loop(Looper.java:137)
            at android.app.ActivityThread.main(ActivityThread.java:5103)
            at java.lang.reflect.Method.invokeNative(Native Method)
            at java.lang.reflect.Method.invoke(Method.java:525)
            at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:737)
            at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
            at dalvik.system.NativeStart.main(Native Method)

I'm not so convinced there's much here to fix?

Comment 7

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

bug 1168998 did the filtering for intent URIs.

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #6)
> I'm not so convinced there's much here to fix?

I prefer the crash to the exploit.

That being said, I would expect that we can wrap startActivity with a catch SecurityException to prevent this. I think we may get the SecurityException from web pages too though – I filed bug 1210857.

Comment 8

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

Comment on attachment 8667702 [details] [diff] [review]
secBugAebitaryPrivs wip

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

This looks to be how I fixed bug 1168998 so looks reasonable.

If you r? me, I can be more thorough.

Comment 9

[Allison Naaktgeboren :ally]

(In reply to Michael Comella (:mcomella) from comment #7)
> bug 1168998 did the filtering for intent URIs.
> 
> (In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #6)
> > I'm not so convinced there's much here to fix?
> 
> I prefer the crash to the exploit.
> 
> That being said, I would expect that we can wrap startActivity with a catch
> SecurityException to prevent this. I think we may get the SecurityException
> from web pages too though – I filed bug 1210857.

I think I'm fixing both, as a crash isn't awesome either and I can't verify my fix actually works if it crashes before I can verify. :p

Comment 10

[Allison Naaktgeboren :ally]

Attachment: secBugAebitaryPrivs

Comment 11

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

Comment on attachment 8670052 [details] [diff] [review]
secBugAebitaryPrivs

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

If this works for you, this works for me, provided we don't catch Exception, as per my comments below.

Also, I think the comment may be too explicit in terms of security – see my other related comment below for more. But I don't really know all of the procedures we have with security.

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +140,5 @@
>                      Telemetry.sendUIEvent(TelemetryContract.Event.LAUNCH,
>                              TelemetryContract.Method.INTENT, "search-result");
>                  }
>  
> +                // Only open applications which can accept arbitrary data from a browser.

nit: I wouldn't mind this bit in a method.

@@ +142,5 @@
>                  }
>  
> +                // Only open applications which can accept arbitrary data from a browser.
> +                i.addCategory(Intent.CATEGORY_BROWSABLE);
> +                // Prevent site from explicitly opening our internal activities, which can leak data.

I think this comment may be too explicit in terms of security – I think there is some obscurity we usually try to have when these patches don't touch all affected versions.

@@ +154,5 @@
>              } catch (URISyntaxException e) {
>                  Log.e(LOG_TAG, "Error parsing intent URI", e);
> +            } catch (SecurityException e) {
> +                Log.e(LOG_TAG, "Security Error handling arbitary intent content", e);
> +            } catch (Exception e) {

Why are we catching Exception here? Is the error handling covered by SecurityException?

Comment 12

[Allison Naaktgeboren :ally]

Attachment: secBugArbitaryPrivs

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I don't know.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The catch blocks perhaps?

Which older supported branches are affected by this flaw?
All of them, including, I think, ESR.  https://hg.mozilla.org/mozilla-central/rev/05645d479d89 is more than a year old.

If not all supported branches, which bug introduced the flaw?
Bug 1064152

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I imagine we would land this patch on them, and so would not require separate backports?

How likely is this patch to cause regressions; how much testing does it need?
I don't think it's likely to cause regressions, but I do not know to what extent there is test coverage of intents.

Comment 13

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

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #12)
> but I do not know to what extent there is test coverage of intents.

Pretty sure we don't have any.

As we discussed on IRC, I'd like for this patch to catch ActivityNotFoundException rather than Exception before it lands.

Comment 14

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

(In reply to Michael Comella (:mcomella) from comment #13)
> As we discussed on IRC, I'd like for this patch to catch
> ActivityNotFoundException rather than Exception before it lands.

Nevermind – I didn't realize it was a new uploaded patch. :)

Comment 15

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

Comment on attachment 8670575 [details] [diff] [review]
secBugArbitaryPrivs

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

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +152,5 @@
>                  return true;
>              } catch (URISyntaxException e) {
>                  Log.e(LOG_TAG, "Error parsing intent URI", e);
> +            } catch (SecurityException e) {
> +                Log.e(LOG_TAG, "Security Error handling arbitrary intent content", e);

Thinking about it now, I'm afraid logging the exceptions (and not just mentioning it's a security error) will leak user data, namely URIs & other browsing data (in the form of intents). We should just log the statements.

Comment 16

[Allison Naaktgeboren :ally]

rgr. Do you want me to modify the statements themselves? Do you feel they are vague enough?

Comment 17

[Muneaki Nishimura]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #16)
> rgr. Do you want me to modify the statements themselves? Do you feel they
> are vague enough?

I think URL in the log code should be removed since Fennec still supports Android version less than 4.1. On those devices, any other applications declaring READ_LOGS permission can steal sensitive data in the log.

Comment 18

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

We need to figure out a security rating on this issue before it can be approved to land.

Comment 19

[Muneaki Nishimura]

(In reply to Al Billings [:abillings] from comment #18)
> We need to figure out a security rating on this issue before it can be
> approved to land.

I'm not sure how you estimate it's risk however I think it's similar type of flaw to mfsa2010-05.
https://www.mozilla.org/en-US/security/advisories/mfsa2005-10/

By abusing this, an remote attacker can send arbitrary intents for launching activity with the Fennec's privilege.
The risk in Fennec is I commented in the description and Comment 1 of this bug, e.g., launching hidden activities, opening local files with file: scheme etc.

On the other hand, the risk out of Fennec depends on vulnerabilities in the installed application each user.
One significant example is below.
https://securityintelligence.com/apache-cordova-phonegap-vulnerability-android-banking-apps/

This is a cross application scripting vulnerability CVE-2015-3500 in Cordova apps that allows any application can inject JavaScript to Cordova based apps via an intent. If a user installs vulnerable Cordova apps in his/her device, the app can be exploited from Fennec through this flaw.

Comment 20

[Daniel Veditz [:dveditz]]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #12)
> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
> The catch blocks perhaps?

Or the check-in comment containing "Firefox Search on Android allows to open an arbitrary activity with Fennec's privilege"? You could change that to something more functional like "Add missing CATEGORY_BROWSABLE".

> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?
> I imagine we would land this patch on them, and so would not require
> separate backports?

That would be ideal, but have you looked at the code or tried it to make sure that's the case? We don't need to worry about ESR for Fennec, but we might want to uplift this to aurora and beta.

Comment 21

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

Comment on attachment 8670575 [details] [diff] [review]
secBugArbitaryPrivs

Clearing sec-approval? now. As a sec-moderate, this doesn't need sec-approval (now that we've rated it).

Comment 22

[Allison Naaktgeboren :ally]

(In reply to Daniel Veditz [:dveditz] from comment #20)
> (In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #12)
> > Do comments in the patch, the check-in comment, or tests included in the
> > patch paint a bulls-eye on the security problem?
> > The catch blocks perhaps?
> 
> Or the check-in comment containing "Firefox Search on Android allows to open
> an arbitrary activity with Fennec's privilege"? You could change that to
> something more functional like "Add missing CATEGORY_BROWSABLE".

Of course. I know sec-bugs checkin comments are different, but could not find guidance in the wiki for _what_ those differences might be.

Might I suggest amending https://wiki.mozilla.org/Security/Bug_Approval_Process with some guidance about what to title sec bugs for other sec-bug noobs like myself? 

> > Do you have backports for the affected branches? If not, how different, hard
> > to create, and risky will they be?
> > I imagine we would land this patch on them, and so would not require
> > separate backports?
> 
> That would be ideal, but have you looked at the code or tried it to make
> sure that's the case? We don't need to worry about ESR for Fennec, but we
> might want to uplift this to aurora and beta.

Yes I did prior to writing the sec-approval? comment block. This is not an area of the code under much churn. Aurora and Beta's PostSearchFragment's shouldOverrideUrlLoading() are effectively identical to Nightly's and I see nothing in the patch that would give release drivers pause with respect to uplift.

Comment 23

[Allison Naaktgeboren :ally]

(In reply to Muneaki Nishimura from comment #17)
> (In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #16)
> > rgr. Do you want me to modify the statements themselves? Do you feel they
> > are vague enough?
> 
> I think URL in the log code should be removed since Fennec still supports
> Android version less than 4.1. On those devices, any other applications
> declaring READ_LOGS permission can steal sensitive data in the log.

By 'URL' you mean the exception itself, 'e', that was part of the log message in the earlier patch right? Or did you mean something else?

Comment 24

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

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #16)
> rgr. Do you want me to modify the statements themselves? Do you feel they
> are vague enough?

Without the exceptions and urls they provide, I think they're appropriate – they don't release specific urls and there is the potential for enough invalid intent uris that we're not indirectly releasing that info.

Comment 25

[Allison Naaktgeboren :ally]

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

Comment 26

[Muneaki Nishimura]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #23)
> (In reply to Muneaki Nishimura from comment #17)
> > (In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #16)
> > > rgr. Do you want me to modify the statements themselves? Do you feel they
> > > are vague enough?
> > 
> > I think URL in the log code should be removed since Fennec still supports
> > Android version less than 4.1. On those devices, any other applications
> > declaring READ_LOGS permission can steal sensitive data in the log.
> 
> By 'URL' you mean the exception itself, 'e', that was part of the log
> message in the earlier patch right? Or did you mean something else?

Yes, my concerning point is the handling of URISyntaxException (below) in the earlier patch.
Log.e(LOG_TAG, "Error parsing intent URI", e);

Here the parameter 'e' may contain sensitive URL so I think it should not be dumped.
For example, when Fennec parses following broken URL, the input URL itself is put to the logcat.
String url = "intent://SECRET_DATA#Intent;action=FOO;Z.USER=admin;Z.PASSWORD=12345678;end";

E PostSearchFragment: Error parsing intent URI
E PostSearchFragment: java.net.URISyntaxException: unknown EXTRA type at index 39: intent://SECRET_DATA#Intent;action=FOO;Z.USER=admin;Z.PASSWORD=12345678;end
E PostSearchFragment:    at android.content.Intent.parseUri(Intent.java:4722)

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a7a050a4ec47

Comment 28

[Allison Naaktgeboren :ally]

(In reply to Muneaki Nishimura from comment #26)
> (In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #23)
> > (In reply to Muneaki Nishimura from comment #17)
> > > (In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #16)
> > > > rgr. Do you want me to modify the statements themselves? Do you feel they
> > > > are vague enough?
> > > 
> > > I think URL in the log code should be removed since Fennec still supports
> > > Android version less than 4.1. On those devices, any other applications
> > > declaring READ_LOGS permission can steal sensitive data in the log.
> > 
> > By 'URL' you mean the exception itself, 'e', that was part of the log
> > message in the earlier patch right? Or did you mean something else?
> 
> Yes, my concerning point is the handling of URISyntaxException (below) in
> the earlier patch.
> Log.e(LOG_TAG, "Error parsing intent URI", e);
> 
> Here the parameter 'e' may contain sensitive URL so I think it should not be
> dumped.
> For example, when Fennec parses following broken URL, the input URL itself
> is put to the logcat.
> String url =
> "intent://SECRET_DATA#Intent;action=FOO;Z.USER=admin;Z.PASSWORD=12345678;
> end";
> 
> E PostSearchFragment: Error parsing intent URI
> E PostSearchFragment: java.net.URISyntaxException: unknown EXTRA type at
> index 39:
> intent://SECRET_DATA#Intent;action=FOO;Z.USER=admin;Z.PASSWORD=12345678;end
> E PostSearchFragment:    at android.content.Intent.parseUri(Intent.java:4722)

dveditz, abillings, What do you think? I can remove this easily enough, but I'd like us to decide what we want for the pre-existing exception handling (not the new stuff I added in the patch) before I flag for uplift.

Comment 29

[Muneaki Nishimura]

I found a practical attack scenario of this bug for stealing user's cookie database stored in a profile directory of Firefox.
The following URLs are the proof of concept and the demonstration movie I took on my Nexus 6 with Android 6.0.
http://mallory.csrf.jp/crash/
http://mallory.csrf.jp/crash/demo.mp4

This technique requires 10 more screen taps and 3 pseudo crashes, so it would not be affected to the current rating of this bug. In this scenario I abused the CrashReporter activity in Firefox. As I mentioned in Comment 0, this bug cam make CrashReporter move arbitrary files to /data/data/org.mozilla.firefox/files/mozilla/Crash Reports/pending/. If we can move both exploit HTML file and user's cookie database in the same directory, the sqlite file can be stolen via XHR from the exploit code.

Comment 30

[Allison Naaktgeboren :ally]

Comment on attachment 8670575 [details] [diff] [review]
secBugArbitaryPrivs

Approval Request Comment
[Feature/regressing bug #]: Bug 1208520
[User impact if declined]: malicious intents can do nasty things, there's also room for a couple crashes

[Describe test coverage new/current, TreeHerder]:
[Risks and why]: very low. patch is small, calls a couple setters and catches a pair of exceptions
[String/UUID change made/needed]: none

Please do not uplift this attached patch, but the one that landed, https://hg.mozilla.org/mozilla-central/rev/a7a050a4ec47 (see comment 27 for its merge to m-c)

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8670575 [details] [diff] [review]
secBugArbitaryPrivs

Fix a sec issue, taking it. Should be in 42 beta 7.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1d7fc1581e77

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/28efbb818caa