1190038 - (CVE-2015-8510) HTML injection on homescreen app (with bypassing DOM sanitizer)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Muneaki Nishimura]

Attachment: reproduced.png

1. Prepare a device that Firefox OS 2.5.0.0-prelease is installed.
   I used my Flame with a firmware id 201507311030207.
2. Launch Browser and open http://csrf.jp/manifest/xss.html .
3. Push 'Add to home screen' button and 'Add' button.
   Then a bookmark with a lock icon is created.
4. Long press the lock icon and try to uninstall the bookmark.
5. As attached picture, a confirmation dialog is shown with FM Radio application (if reproduced).

At step 2, the page sets "<s>PoC<iframe src=app://fm.gaiamobile.org/index.html mozbrowser></iframe>" as it's application name throught the web manifest.

When I set iframe tag simply like "<iframe src=app://fm.gaiamobile.org/index.html mozbrowser></iframe>" as an application name then the iframe is correctly removed by DOM sanitizer in Gaia.
But above text pattern, i.e., <s> + <iframe>, can bypass DOM sanitizer and the iframe is executed with homescreen app's privilege.

Comment 1

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

Ben - this looks it is exploited via w3c app support. Can you have a look here? The vuln is probably somewhere else but we may have violated an assumption here.

Comment 2

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

Actually the strings for this issue are here:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/locales/system.en-US.properties#L288

I wonder if this is another bug in our l20n code, since I _think_ this has a sanitizer in it.

Comment 3

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

 FWIW, this bug has been recently introduced since I had to flash from dogfood to dogfood-latest to reproduce this.

Comment 4

[Staś Małolepszy :stas]

I can reproduce this and I can confirm that this is a bug in l10n.js/l20n.js.  We only recursively sanitize the translation if the outer element was found in the source HTML:

https://github.com/mozilla-b2g/gaia/blob/8dba2077f5e7137253fbb3faf10cd0b5f7da25c2/shared/js/l10n.js#L2006

In the test case, <s> is not in the source HTML, so l10n.js doesn't sanitize its children and appends the whole thing to DOM.

I'll fix this.

Comment 5

[Staś Małolepszy :stas]

Attachment: Patch 1

This patch calls the sanitization logic recursively on children of allowed elements which are only present in translations.

I'm not sure what the disclosure policy is so I haven't submited a pull request yet.  Also, I think this bug might be present on 2.2 as well, because bug 994357 was backported.

https://github.com/mozilla-b2g/gaia/commit/3c71bd67292bbb9f898d1045f07cbe0b4e565762

I'll also add a test before I'm ready to land.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Paul Theriault [:pauljt] from comment #2)
> I wonder if this is another bug in our l20n code, since I _think_ this has a
> sanitizer in it.

Do we have other bugs in our l20n code? I don't know of any.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8642437 [details] [diff] [review]
Patch 1

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

Great catch Muneaki!

Comment 8

[Staś Małolepszy :stas]

Paul, what's the best way of landing this on master and 2.2?  Do we have users on 2.2 that we should be careful about here?  Should I simply create a pull request?

Comment 9

[Staś Małolepszy :stas]

Attachment: Patch 2 with a test

Added a test.  Carrying over the r+.  I'm going to sleep and will check back in 7-8 hours.  If you need to land this before then, this is ready to go.  Thanks!

Comment 10

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

(In reply to Staś Małolepszy :stas from comment #8)
> Paul, what's the best way of landing this on master and 2.2?  Do we have
> users on 2.2 that we should be careful about here?  Should I simply create a
> pull request?

I just checked and we don't currently have any devices on 2.2, so I think its ok to land.

Comment 11

[Staś Małolepszy :stas]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/65c731fbe7023a5d27badb48a0f1b27f9170cfb5

Comment 12

[Staś Małolepszy :stas]

Comment on attachment 8642642 [details] [diff] [review]
Patch 2 with a test

[Approval Request Comment]

[Bug caused by] (feature/regressing bug #): bug 994357

[User impact] if declined: XSS is possible via well-prepared name of app, or name of bluetooth device, or name of wifi network (if they're used in translations)

[Testing completed]: tested on device;  added a unit test

[Risk to taking this patch] (and alternatives if risky): very low

[String changes made]: none

Comment 13

[Staś Małolepszy :stas]

Ryan, would you mind helping us with the uplift to 2.2 here?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Doesn't cherry-pick cleanly, please rebase. No need to ni? me when you're done.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

The conflicts were that l20n.js doesn't exist on v2.2. Stas confirmed that it's safe to ignore those changes and I went ahead and pushed the rest.

v2.2: https://github.com/mozilla-b2g/gaia/commit/102f1299e9eafe3760e1deb44d556b5c4f36b5af

Comment 16

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

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6)
> (In reply to Paul Theriault [:pauljt] from comment #2)
> > I wonder if this is another bug in our l20n code, since I _think_ this has a
> > sanitizer in it.
> 
> Do we have other bugs in our l20n code? I don't know of any.

Late response - I meant, "I wonder if this bug is another issue similar to bug 1190137, 1190139 etc'