1158715 - XSS/HTML injection in TV Smart System app's modal_dialog.js

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::TV::System, defect)

Description

[Frederik Braun [:freddy]]

As suggested in bug 1157216 comment 11 by Muneaki Nishimura:
> Is "tv_apps" an active project (and in scope of your bounty program)?
> 
> I'm not sure it is really exploitable since I don't have execution environment > of tv_apps but following code may have same defect.
> https://github.com/mozilla-b2g/gaia/blob/master/tv_apps/smart-system/js/modal_dialog.js#L379

The patch should escape quotes around the data items in lines 373 and 375:
>      itemsHTML.push(data.options[i].id);
>      …
>      itemsHTML.push(data.options[i].text);


John, can you take this?

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

Sure, I can take this.

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

Sung,

Please check and fix this bug.

Comment 3

[Sung Chiu[:sungchiu]]

Hi Frederik, I'm not authorized to access bug 1157216, can you give me permission?

Comment 4

[Frederik Braun [:freddy]]

Bug 1157216 is a completely different issue. The relevant part form bug 1157216 is coped into comment 0:
We just need to escape quotes around the itemsHTML.push calls that I cited above, assuming that the content may come from untrusted user/network data.

Comment 5

[Sung Chiu[:sungchiu]]

I also found the similar issues in app_modal_dialog and value_picker:
  1. app_modal_dialog -
    https://github.com/mozilla-b2g/gaia/blob/master/tv_apps/smart-system/js/app_modal_dialog.js#L310, where 'title' comes from app name in manifest, and others may create a name with escape characters.
  2. value_picker -
    https://github.com/mozilla-b2g/gaia/blob/master/tv_apps/smart-system/js/value_selector/value_picker.js#L137, where variable _valueDisplayedText may be any characters coming from user data.

Do I have to address those two also?

Comment 6

[Frederik Braun [:freddy]]

It would be ideal if we can get all of those fixed.
We can split this into separate bugs, if this works better for you. But please remember to mark them as security bugs when filing a follow-up.

Comment 7

[Sung Chiu[:sungchiu]]

Attachment: Pull Request

Apply Tagged.escapeHTML to the HTML string for selectOneMenu in modal_dialog.js.

Comment 8

[Frederik Braun [:freddy]]

Comment on attachment 8598544 [details] [review]
Pull Request

From a security-perspective, this looks good!

Comment 9

[Sung Chiu[:sungchiu]]

Bug 1159136 and Bug 1159137 are the following bugs for eliminating XSS injection in app_modal_dialog and value picker respectively.

Comment 10

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8598544 [details] [review]
Pull Request

Look good to me. Please ask Evelyn on how to share this issue to partner.

Comment 11

[John Hu [:johnhu][:johu][:醬糊小弟]]

rename the issue and change the dependency chain. they should be chained with |see also|.

Comment 12

[Evelyn Hung]

Since these codes are copied from phone's System app, could you check if we also need to fix there? Thanks.

Comment 13

[Sung Chiu[:sungchiu]]

Our phone's System app has corrected this part. This patch is the temporary fix for XSS injection. After system app and smart-system app are merged, we will not have this problem.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/e744b32cf71c4ae12b0c54fa4fee4190030af1e4

Comment 15

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/94d10b292cd62cb4650aab0b96499b10bd0e87bd

Comment 16

[Christiane Ruetten [INACTIVE]]

Daisuke, please check whether my whiteboard tag has messed with yours. We have the habit to use square brackets for tag separation. Perhaps you want to adopt a similar scheme, for example using the [stingray-*] namespace?

Comment 17

[Daisuke]

(In reply to Christiane Ruetten [:cr] from comment #16)
> Daisuke, please check whether my whiteboard tag has messed with yours. We
> have the habit to use square brackets for tag separation. Perhaps you want
> to adopt a similar scheme, for example using the [stingray-*] namespace?

Hi Christiane

Sorry for bother you and I modified.
If still not enough, please let me know.

Comment 18

[Christiane Ruetten [INACTIVE]]

No bother, just wanted to make sure your bug queries keep working as expected. I'll modify other stingray tags accordingly when I come across them.