Closed Bug 1158715 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set
normal

Tracking

(b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S11 (1may)
Tracking Status
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: freddy, Assigned: suchiu)

References

Details

(Keywords: sec-high, wsec-xss, Whiteboard: [stingray-picked(2015/5/19)] [b2g-adv-main2.2-])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
johnhu
: review+
freddy
: review+
Details | Review
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?
Flags: sec-bounty?
Sure, I can take this.
Assignee: nobody → im
Sung,

Please check and fix this bug.
Assignee: im → suchiu
Hi Frederik, I'm not authorized to access bug 1157216, can you give me permission?
Flags: needinfo?(fbraun)
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.
Flags: needinfo?(fbraun)
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?
Flags: needinfo?(fbraun)
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.
Flags: needinfo?(fbraun)
Keywords: sec-high, wsec-xss
Blocks: 1159136
Blocks: 1159137
Attached file Pull Request
Apply Tagged.escapeHTML to the HTML string for selectOneMenu in modal_dialog.js.
Attachment #8598544 - Flags: review?(im)
Attachment #8598544 - Flags: review?(fbraun)
Comment on attachment 8598544 [details] [review]
Pull Request

From a security-perspective, this looks good!
Attachment #8598544 - Flags: review?(fbraun) → review+
Bug 1159136 and Bug 1159137 are the following bugs for eliminating XSS injection in app_modal_dialog and value picker respectively.
Comment on attachment 8598544 [details] [review]
Pull Request

Look good to me. Please ask Evelyn on how to share this issue to partner.
Attachment #8598544 - Flags: review?(im) → review+
rename the issue and change the dependency chain. they should be chained with |see also|.
No longer blocks: 1159136, 1159137
See Also: → 1159136, 1159137
Summary: XSS/HTML injection in TV Smart System app → XSS/HTML injection in TV Smart System app's modal_dialog.js
Since these codes are copied from phone's System app, could you check if we also need to fix there? Thanks.
Flags: needinfo?(suchiu)
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.
Flags: needinfo?(suchiu)
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/e744b32cf71c4ae12b0c54fa4fee4190030af1e4
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S11 (1may)
Flags: sec-bounty? → sec-bounty+
Whiteboard: stingray-picked(2015/5/19)
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?
Flags: needinfo?(938.daisuke)
Whiteboard: stingray-picked(2015/5/19) → stingray-picked(2015/5/19) [b2g-adv-main2.2-]
(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.
Flags: needinfo?(938.daisuke)
Whiteboard: stingray-picked(2015/5/19) [b2g-adv-main2.2-] → [stingray-picked(2015/5/19)] [b2g-adv-main2.2-]
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.
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.