HTML injection in Communications app

RESOLVED FIXED

Status

defect
RESOLVED FIXED
4 years ago
3 years ago

People

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

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(b2g-v2.2 ?, b2g-master fixed)

Details

Attachments

(4 attachments)

Posted image vulnerability 1
User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2383.0 Safari/537.36

Steps to reproduce:

1. Open "Browser" app and download vCard from http://mallory.csrf.jp/fxa/xss.vcf
2. Launch "Contacts" app and open "Settings" page.
3. Push "Import Contacts" button and choose "Menory card"
4. Repeat step 2 & 3 and push "Done" button in the Setting menu
5. "Contacts" list page is shown on the screen
6. Choose one of the contact list entry starting with "<s>FIRSTNAME</s>..."
7. Edit the entry by pushing edit icon on the right-top on the screen
8. Update company name e.g., abc and push "Update" button on the right-top button
9. Here, you can show the "Duplicates found" popup and then the HTML tags in the FIRSTNAME and LASTNAME are not properly escaped and it works. (vulnerability 1)
10. Return to the home screen and start "Phone" app
11. Push "999" on the screen, then the candidates contact entries are displayed on the screen
12. However, HTML tags in phone number displayed automatically is not  are not properly escaped and it works. (vulnerability 2)



Actual results:

In vulnerability 1 & 2 in above procesure, there are two HTML injection possibilities.


Expected results:

In vulnerability 1 & 2 in above procesure, HTML tags should be escaped properly.
Posted image vulnerability 2
Hey - I can help out here.

Fernando, Francisco - Here is a first patch which will solve the escaping for the list items in the contact matching page.

The approach is to use the template strings as suggested by our security guys. I can also fix the rest of the issues in this bug assuming you are ok with this approach. We should probably consider phasing out the template.js file in favor of using tagged template strings. Please take a look and let me know what you think. Thanks!
Attachment #8602597 - Flags: review?(francisco)
Attachment #8602597 - Flags: review?(fernando.campo)
Assigning to myself for now. I'll help out with some of these issues.
Assignee: nobody → kgrandon
Doug or Etienne - could either of you guys review this? Thanks!
Attachment #8602613 - Flags: review?(etienne)
Attachment #8602613 - Flags: review?(drs)
Comment on attachment 8602597 [details] [review]
[gaia] Pull Request - Fix contact matching item escaping

Tried on the phone and working for me.

Thanks Kevin!
Attachment #8602597 - Flags: review?(fernando.campo)
Attachment #8602597 - Flags: review?(francisco) → review+
Comment on attachment 8602613 [details] [review]
[gaia] Pull Request - Fix dialer suggestion escaping

Thanks!

My 2 cents:
I think we should lazy load `tagged.js` with the `suggestion_bar.js` file. And there might be a follow up to file about the call history and the callscreen app.

But mainly, redirecting to Gabriele as a heads-up :)
Attachment #8602613 - Flags: review?(etienne) → review?(gsvelto)
Comment on attachment 8602613 [details] [review]
[gaia] Pull Request - Fix dialer suggestion escaping

Ouch, I didn't realize we were using innerHTML for this. This sounds like a good candidate for a healthy refactor (well, large swathes of the dialer code do).
Attachment #8602613 - Flags: review?(gsvelto) → review+
(In reply to Etienne Segonzac (:etienne) from comment #6)
> Comment on attachment 8602613 [details] [review]
> [gaia] Pull Request - Fix dialer suggestion escaping
> 
> My 2 cents:
> I think we should lazy load `tagged.js` with the `suggestion_bar.js` file.
> And there might be a follow up to file about the call history and the
> callscreen app.

Thanks for taking a look. This makes sense and I've gone ahead and made the change as requested.
Comment on attachment 8602613 [details] [review]
[gaia] Pull Request - Fix dialer suggestion escaping

Looks good, though Gabriele's review is enough here.
Attachment #8602613 - Flags: review?(drs) → review+
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.