1102204 - (CVE-2015-2744) Remote HTML tag injection in Gaia Search app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Search, defect)

Description

[Muneaki Nishimura]

Attachment: search.png

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

Steps to reproduce:

1. Start Firefox OS 2.1/2.2.
2. Open Search(Browser) app and launch following link.
http://alice.csrf.jp/http.php?s=308&u=https://www.google.co.jp/search?q=%3Ciframe+mozbrowser+remote+mozapp%3D%22app%3A%2F%2Ffm.gaiamobile.org%2Fmanifest.webapp%22+src%3D%22app%3A%2F%2Ffm.gaiamobile.org%2Findex.html%22%3E
3. Search app. shows the search result of <iframe...>
4. Push HOME button and show home screen
5. Click "Browser" icon and launch Search app again


Actual results:

The built-in FM Radio app. is shown on the Search app. (see attached image)


Expected results:

FM Radio app. is not shown and a simple text "<iframe mozbrowser ..." is shown on the Search app.

Comment 1

[Daniel Veditz [:dveditz]]

I can confirm this in a nightly (2.2) b2g desktop build.

Paul: how bad do you think this is, security-wise? I guess it depends on why this is happening (what's not getting escaped) but maybe you could get scripts run in the search app. What privs does that have? Hosting an app within the search app is strange but doesn't seem all that harmful -- it's not doing much there and if you click on it you actually load the previous search, the events don't seem to get through to the embedded app.

Hopefully CSP would stop any attempts to load things that could try to reach out into the search app's context. Either what you load is a different origin, or if it's same-origin (data: url, for example) then it'll inherit the search.app's CSP.

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

This sounds pretty serious - you shouldnt normally be able to embed apps unless you are the system. If this is HTML injection into system then this is critical. CSP will make attacks harder. I'll need to do a bit of digging to figure what is actually happening here though.

Cc'ing some folks who can help.

Comment 3

[Paul Theriault [:pauljt] (no longer reading bugmail)]

[Blocking Requested - why for this release]: I'm flagging this just as a heads up since its likely to be a blocking security issue. 

I'll report back as soon as I figure out what is going on.

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

This is injection into the system app I think, though it might be within a mozbrowser frame (not what context browser windows run in). In inspector I see the following:


<section id="history" class="local" aria-labelledby="history-header">

    <h3></h3>
    <ul role="listbox"></ul>
    <div class="result" data-url="https://www.google.co.jp/search?q=%3Ciframe%20mozbrowser%20r….webapp%22%20src=%22app://fm.gaiamobile.org/index.html%22%3E" role="link" aria-label="<iframe mozbrowser remote mozapp="app://fm.gaiamobile.org/ma…ebapp" src="app://fm.gaiamobile.org/index.html"> - Google 検索"></div>
    <h3></h3>

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

So I think what is actually happening here is that the search app is requesting a screenshot of a URL:

https://github.com/mozilla-b2g/gaia/blob/v2.1/apps/search/js/newtab.js#L42

Comment 6

[Muneaki Nishimura]

It seems the cause of the bug is here.
https://github.com/mozilla-b2g/gaia/blob/master/apps/search/js/providers/provider.js#L98

Search app has 'webapps-manage' permission so it can also launch other application like System app.

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Ah yes, missed that. Not sure how config ends up with that value but iterating through in the debugger, at some point, title.innerHTML becomes:

<iframe mozbrowser="" remote="" mozapp="app://fm.gaiamobile.org/manifest.webapp" src="app://fm.gaiamobile.org/index.html"> - Google 検索</iframe>

Kevin/Dale/Ben, can we just replace innerHTML with textContent here? (pinging all three since I think 2 of you are away).

Comment 8

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Paul Theriault [:pauljt] from comment #7)
> Ah yes, missed that. Not sure how config ends up with that value but

Oh because that is the title of the page after the google redirect... :/

Comment 9

[Muneaki Nishimura]

One more similar bug in the search app.

1. Open following URL by Search app and show the search result page.
https://www.google.co.jp/search?q=%3Ciframe%20mozbrowser%20remote%20mozapp=%22app://fm.gaiamobile.org/manifest.webapp%22%20src=%22app://fm.gaiamobile.org/index.html%22%3E
2. Push "..." button on the left side of location bar.
3. Push "Show Windows".
4. FM application is embedded on the tab list page.

Comment 10

[Dale Harvey (:daleharvey)]

Yeh that was a bad miss, will switch it and write up a test for it

Comment 11

[Frederik Braun [:freddy]]

There's two instances of innerHTML right next to each other, Dale, line 98 and 100. Can you make sure you fix both and add a test for the titles being escaped?

Comment 12

[Frederik Braun [:freddy]]

Sorry, test sentence makes no sense.
We don't want escaping, we want textContent instead of innerHTML.

Comment 13

[Dale Harvey (:daleharvey)]

Will do

One thing, I remember last time there was a security bug I screwed up with the commit message, is there guidelines about the commit message etc for security related bugs?

Comment 14

[Kevin Grandon :kgrandon]

Looks like Dale is on this, thanks Dale.

(In reply to Dale Harvey (:daleharvey) from comment #13)
> One thing, I remember last time there was a security bug I screwed up with
> the commit message, is there guidelines about the commit message etc for
> security related bugs?

From what I recall it's best to avoid pull requests during the review cycle (use a patch instead until it's ready to land). There's possibly additional guidelines for the actual commit message, but I'm not sure about those.

Comment 15

[Frederik Braun [:freddy]]

I just checked and these innerHTML lines are also in 1.4, so we need fixing for multiple branches.
The bug approval process is explained here <https://wiki.mozilla.org/Security/Bug_Approval_Process>.

I'd suggest this bug is sec-high or sec-moderate. Let's assume "high" for the sake of the approval process.

Comment 16

[Kevin Grandon :kgrandon]

(In reply to Frederik Braun [:freddyb] from comment #15)
> I just checked and these innerHTML lines are also in 1.4, so we need fixing
> for multiple branches.

I could be wrong, but I don't believe this code is *actually* exposed until 2.0. I think it sits there and runs in the background, but it should be fully disabled in 1.4. In 2.0 we expose the rocketbar on the home screen only, and the code is present, but it's exposed in a different way, so I'm not sure if it's reproducible in 2.0.

So although it's *in* the codebase, I'm not sure if it actually does anything in 1.4/2.0 releases. Changing the flag to '?' so we can investigate this.

Comment 17

[Dale Harvey (:daleharvey)]

Attachment: 1102204.patch

Comment 18

[Dale Harvey (:daleharvey)]

We never touch this code in 1.4, in 2.0 we never enabled the browser history which is the only consumer of this code so again its not used.

Comment 19

[Kevin Grandon :kgrandon]

Comment on attachment 8526819 [details] [diff] [review]
1102204.patch

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

Looks good to me, thanks!

Comment 20

[Dale Harvey (:daleharvey)]

Comment on attachment 8526819 [details] [diff] [review]
1102204.patch

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Bug in original implementation
[User impact] if declined: Severe security risk
[Testing completed]: Automated testing added
[Risk to taking this patch] (and alternatives if risky): Low risk, small change
[String changes made]:

Comment 21

[Dale Harvey (:daleharvey)]

Will ask for approval for 2.1 then do try runs and land straight away

Comment 22

[Frederik Braun [:freddy]]

(In reply to Dale Harvey (:daleharvey) from comment #18)
> We never touch this code in 1.4, in 2.0 we never enabled the browser history
> which is the only consumer of this code so again its not used.

Comment 9 mentions this being in the title of browser windows, when doing the "view all windows" thing from within the browser. Sure it's still just 2.1?

Comment 23

[Dale Harvey (:daleharvey)]

This specific bug yes, that is related but a different part of the code, it should be filed seperately

Comment 25

[Muneaki Nishimura]

(In reply to Dale Harvey (:daleharvey) from comment #23)
> This specific bug yes, that is related but a different part of the code, it
> should be filed seperately

Sorry, Comment #9 is not in Search app. but in System app.
I reported it as a comment of Bug 1101158, so you can ignore it.

Comment 26

[Kevin Grandon :kgrandon]

(In reply to Muneaki Nishimura from comment #25)
> Sorry, Comment #9 is not in Search app. but in System app.
> I reported it as a comment of Bug 1101158, so you can ignore it.

Can someone add Dale and myself to the CC of Bug 1101158? It looks relevant, thanks.

Comment 27

[Frederik Braun [:freddy]]

(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #26)
> Can someone add Dale and myself to the CC of Bug 1101158? It looks relevant,
> thanks.

Done

Comment 28

[Frederik Braun [:freddy]]

The "approval-gaia-v2.1?" does not flag someone in particular, Dale. Do we need to include someone specific to get this going?

Comment 29

[Kevin Grandon :kgrandon]

Comment on attachment 8526819 [details] [diff] [review]
1102204.patch

I think these are generally being triaged by either Fabrice or Bhavana. I don't think it's necessary to add them to the flag, but I'll go ahead and do so.

I think it was just requested on Friday, so I assume they just haven't gotten to do a triage yet.

Comment 30

[Dale Harvey (:daleharvey)]

Yeh the approval tags are triaged wether or not they reference anyone, should be got to soon

However I gave the test a try run and its failing on tbpl, its possibly related to a harness issue that is keeping the other tests .skipped (basically any test that tries to use the rocket bar then inspect the search app fails), I previously worked on the bug in https://bugzilla.mozilla.org/show_bug.cgi?id=1078739 and fixed it, the test works locally for my fine but fails every time on treeherder

Kevin of Ben can steal this if it needs to land soon, but I am on PTO till monday, I done some debugging today but cant find an easy cause for the failures (faily run is at https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=799d3857aaf8, doing another run @ https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=ac8b8e272a06)

Comment 31

[Frederik Braun [:freddy]]

Thanks Dale for taking a look though you're on PTO!

I'm unassigning you to reflect that we need someone else to continue here.

Comment 32

[Kevin Grandon :kgrandon]

Comment on attachment 8526819 [details] [diff] [review]
1102204.patch

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

Clearing uplift requests until we have a green test run then. I might take a look at this today.

Comment 33

[Kevin Grandon :kgrandon]

Attachment: 0001-Bug-1102204-Use-textContent-instead-of-innerHTML-for.patch

Here is a new patch with a unit test instead of marionette test. Going to give up on the marionette test for now because I believe our tests which use history items are currently disabled. Carrying review - I just added a small unit test.

Comment 34

[Kevin Grandon :kgrandon]

Guess I'll get this landed. Thanks for the work here Dale.

Comment 35

[Kevin Grandon :kgrandon]

Comment on attachment 8527827 [details] [diff] [review]
0001-Bug-1102204-Use-textContent-instead-of-innerHTML-for.patch

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

Requesting uplift on this before landing as it's a security issue. See comment 20 for the approval form.

Comment 36

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Here is the pull request to master.

Comment 37

[bhavana bajaj [:bajaj]]

Approving so it can land in parallel, we'll need to be sure to watch the trees.

Comment 38

[Kevin Grandon :kgrandon]

Landed in master: https://github.com/mozilla-b2g/gaia/commit/6906fd049b7527950f811a5d097d10e2da901c93

Comment 39

[Kevin Grandon :kgrandon]

In v2.1: https://github.com/mozilla-b2g/gaia/commit/b711909aab9a5cb84cba45175b909ad7f7b63842