863527 - Make insertAdjacentHTML more safe in system app error messages

Status: RESOLVED FIXED

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

Description

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

This line is dangerous:
http://mxr.mozilla.org/gaia/source/apps/system/js/window.js#65

64   AppError.prototype.render = function() {
65     this.app.frame.insertAdjacentHTML('beforeend', this.view());
66     this.closeButton =

This was the cause of bug 854849, but in that bug the approach has been to sanitize app titles and other details to disallow HTML in general, to try to prevent issues such as this across the board. 

I still think we should either remove the insertAdjacentHTML call here, or be sure to escape/sanitize the untrusted input to prevent future injection attacks.

Comment 1

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

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

Comment 2

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

Created patch which fixes the issue in Gaia, but its my first gaia patch, so please let me know if I submitted correctly.

Comment 3

[Julien Wajsberg [:julienw]]

You can either use a usual patch (generated with "git format-patch -U8") or use a PR, so seems good.

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 739494 [details] [diff] [review]
https://github.com/mozilla-b2g/gaia/pull/9311

From the blame I think :alive knows this code better.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 739494 [details] [diff] [review]
https://github.com/mozilla-b2g/gaia/pull/9311

Thanks for patching this! I'll update my rendering function from now on.

*But please fix the lint error of window.js
 See travis log for more info.

Comment 6

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

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

Fixed linter error. (in new PR since I broke the old branch and couldnt figure out how to un-break it).

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 740028 [details] [diff] [review]
https://github.com/mozilla-b2g/gaia/pull/9326

r+ but bug comment should be "Bug XXXXXX - what-you-do"
You could use git rebase to rewording the comment and then push -f to the same branch of your repo.

Comment 8

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

This is a really minor change which fixes a significant bug in the system app so I am nominating for leo.

Comment 9

[Julien Wajsberg [:julienw]]

Paul, you should ask checkin-needed when you're ready to merge.

I think you'll likely need a rebase here, hopefully there won't be too many conflicts.

Comment 10

[Julien Wajsberg [:julienw]]

yay, The PR still applies cleanly.

Alive, would you please commit this after checking this still works as expected ?

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

I will uplift.

Comment 12

[(inactive after 6/18) Alive Kuo [:alive]]

master
https://github.com/mozilla-b2g/gaia/commit/fcda94c9f9776c2ae181f78a3471e012db9e04a1

Comment 13

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

(In reply to Julien Wajsberg [:julienw] (PTO 15th -> 28th July) from comment #10)
> yay, The PR still applies cleanly.
> 
> Alive, would you please commit this after checking this still works as
> expected ?


Awesome thanks - sorry I didn't get a chance to look at this.

Comment 14

[Julien Wajsberg [:julienw]]

afaik this is not uplifted yet.

Comment 15

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted fcda94c9f9776c2ae181f78a3471e012db9e04a1 to:
v1-train: ce39a9234e7763b77652f0fb79d7dfd0ac4a4f23

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: ce39a9234e7763b77652f0fb79d7dfd0ac4a4f23