1192595 - HTML injection in Dialer app's Suggestion Bar via crafted telephone numbers in user's contact list

Status: RESOLVED FIXED

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

Description

[Muneaki Nishimura]

Attachment: Friend.vcf

Dialer app allows to inject any HTML tags through the suggestion bar. It seems this bug was introduced in Firefox OS v1.1:
https://github.com/mozilla-b2g/gaia/blob/v1.1.0hd/apps/communications/dialer/js/suggestion_bar.js#L140
and it's still there in the latest Firefox OS v2.5 though the code was little bit changed:
https://github.com/mozilla-b2g/gaia/commit/f8a1a15d8e98b0316e8e89682bab9460838fdeed#diff-fbedf9c5945c76df46f13be0f8b33f50R272

Steps to reproduce:
1. Send attached .vcf file to your B2G device through e-mail, MMS and so on.
2. Import the .vcf to your contact list on the device.
3. Open Dialer app and put '090' or '911' then the malicious page is shown on full screen.

In detail, the .vcf file imported your device has one contact entry that name is 'Friend'. It has two mobile numbers one is "911<iframe src=http:a.csrf.jp></iframe>" and another one is "090<iframe src=http:a.csrf.jp>". Here the '090' is a common prefix of mobile phone number in Japan and '911' is (as you know) an emergency number in US. At step 3, when you dial '090' or '911' then the <iframe> in the telephone number field is directly injected to the suggestion bar and the site (http:a.csrf.jp) opens a malicious page on the Dialer. Whenever you dial the number you would be taken to the same malicious page so you are prihibited to make a call to any number started with '090' or '911'.

Comment 1

[Muneaki Nishimura]

Attachment: Reproduced on Firefox OS v1.3 simulator

Comment 2

[Muneaki Nishimura]

Attachment: Reproduced on Flame with the latest Firefox OS v2.5 Nightly (Build ID: 20150807150216)

Comment 3

[Stephanie Ouillon [:arroway] (needinfo me)]

Hello, 

thank you for reporting this Muneaki, I'll investigate.

Comment 4

[Stephanie Ouillon [:arroway] (needinfo me)]

I can reproduce the bug on 1.3 (I tried on the simulator) and on a recent build on aries (dogfooding, build id 20150810211816).

Note that for some reason, I couldn't reproduce it on either flame or aries this morning, and finally got it to work on aries (but not flame, trying different builds). I didn't identify the reason why...

Anyway, Gabriele, would you mind take a look at this or redirect to somebody from the contacts app if necessary?
Thanks!

Comment 5

[Gabriele Svelto [:gsvelto]]

This is definitely a dialer issue so I'll deal with it myself. The relevant code was partially fixed as part of bug 1176193 but I wanted to clean it up completely by removing the use of innerHTML entirely and creating the relevant DOM elements by hand (something that should have been done in the first place). I'll do it here since that's going to address this issue once and for all.

Comment 6

[Julian Hector [:tedd] [:jhector]]

Attachment: Patch to fix Sanitizer.createSafeHTML usage

I also looked into this, because it was odd that it worked on the latest version of Gaia although Sanitizer.createSafeHTML and Sanitizer.unwrapSafeHTML were used. It turned out that the call to createSafeHTML [1] is incorrect.

With this patch, the HTML is correctly escaped and the injection is fixed.

[1] https://github.com/mozilla-b2g/gaia/blob/f8a1a15d8e98b0316e8e89682bab9460838fdeed/apps/communications/dialer/js/suggestion_bar.js#L336

Comment 7

[Julian Hector [:tedd] [:jhector]]

The createSafeHTML function signature looks like this:

> createSafeHTML: function (strings, ...values)

where |values| is an array of values that need to be escaped,
so when calling:

> Sanitizer.createSafeHTML`${startStr}<mark class="ci__mark>${middleStr}</mark>${endStr}`

|startStr|, |middleStr| and |endStr| will be escaped before the template is interpolated.
But the call looked like this:

> Sanitizer.createSafeHTML(`${startStr}<mark class="ci__mark>${middleStr}</mark>${endStr}`)

where the template will be interpolated first and then passed to the sanitizer function, which results in the |values| array to be empty and nothing will be escaped.

Comment 8

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8649991 [details] [diff] [review]
Patch to fix Sanitizer.createSafeHTML usage

Can you make a PR and ask me for review?

Comment 9

[Stephanie Ouillon [:arroway] (needinfo me)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/31579

Gabriele, Julian will be away for some time, so I did the PR with his code.

Comment 10

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

Do you know what versions this affects? Is it only 2.5?

Comment 11

[Stephanie Ouillon [:arroway] (needinfo me)]

Looks like it was introduced before (1.1) and I could reproduce easily in the 1.3 simulatoru.

Comment 12

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8654123 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/31579

Thanks Stephanie, let's land this ASAP and I'll open a follow-up to completely get rid of the innerHTML use and replace it with a document fragment created using the appropriate APIs.

Comment 13

[Gabriele Svelto [:gsvelto]]

This issue has most likely been introduced in bug 838017 when the suggestion bar first landed. Importing contacts was already possible back then so it was most likely possible to exploit this already.

Comment 14

[Gabriele Svelto [:gsvelto]]

FYI I've filed bug 1200576 as a follow-up to this.

Comment 15

[Stephanie Ouillon [:arroway] (needinfo me)]

Comment on attachment 8654123 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/31579

[Security approval request comment]
* How easily could an exploit be constructed based on the patch?

Quite easily if one understands the code

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

No

* Which older supported branches are affected by this flaw?

Back to FxOS 1.1 (different code, but same flaw)

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

* Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I don't know

* How likely is this patch to cause regressions; how much testing does it need?

No much risk.

Comment 16

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

I'm ni? Paul before sec-approval+ here. I've never had anyone request a sec-approval for gaia only code and I'm not sure what we want to do here. The reason for sec-approval is to reduce risks of a zero day on ourselves with a checkin. Since this issue goes back to 1.1, I want his input first.

Comment 17

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

(In reply to Al Billings [:abillings] from comment #16)
> I'm ni? Paul before sec-approval+ here. I've never had anyone request a
> sec-approval for gaia only code and I'm not sure what we want to do here.
> The reason for sec-approval is to reduce risks of a zero day on ourselves
> with a checkin. Since this issue goes back to 1.1, I want his input first.

I think this is ok to check-in. The rating of sec-high is probably overly-conservative here (due to CSP and user interaction) but also as Stephanie says, this patch doesn't point to the exploit method really.

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Julian Hector [:tedd] [:jhector]]

Thanks :arroway for taking care of the PR for me.