Make insertAdjacentHTML more safe in system app error messages

RESOLVED FIXED

Status

Firefox OS
Gaia::System
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: pauljt, Assigned: pauljt)

Tracking

({sec-moderate})

unspecified
x86
Mac OS X
sec-moderate
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, firefox-esr24 unaffected, b2g18 fixed, b2g-v1.1hd fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: Replace innerHTML with something more safe in system app error messages → Make insertAdjacentHTML more safe in system app error messages
(Assignee)

Updated

5 years ago
QA Contact: ptheriault
(Assignee)

Updated

5 years ago
Assignee: nobody → ptheriault
QA Contact: ptheriault
(Assignee)

Comment 1

5 years ago
Created attachment 739494 [details] [diff] [review]
https://github.com/mozilla-b2g/gaia/pull/9311
Attachment #739494 - Flags: review?(21)
(Assignee)

Comment 2

5 years ago
Created patch which fixes the issue in Gaia, but its my first gaia patch, so please let me know if I submitted correctly.
You can either use a usual patch (generated with "git format-patch -U8") or use a PR, so seems good.
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.
Attachment #739494 - Flags: review?(21) → review?(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.
Attachment #739494 - Flags: review?(alive) → review+
(Assignee)

Comment 6

5 years ago
Created attachment 740028 [details] [diff] [review]
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).
Attachment #740028 - Flags: review?(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.
Attachment #740028 - Flags: review?(alive) → review+
(Assignee)

Updated

5 years ago
Keywords: sec-moderate
(Assignee)

Updated

5 years ago
Group: b2g-core-security
(Assignee)

Updated

5 years ago
Blocks: 866886
(Assignee)

Comment 8

5 years ago
This is a really minor change which fixes a significant bug in the system app so I am nominating for leo.
blocking-b2g: --- → leo?

Updated

5 years ago
blocking-b2g: leo? → leo+
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.
blocking-b2g: leo+ → leo?

Updated

4 years ago
blocking-b2g: leo? → leo+
yay, The PR still applies cleanly.

Alive, would you please commit this after checking this still works as expected ?
Flags: needinfo?(alive)
I will uplift.
Flags: needinfo?(alive)
master
https://github.com/mozilla-b2g/gaia/commit/fcda94c9f9776c2ae181f78a3471e012db9e04a1
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g18: --- → affected
Resolution: --- → FIXED
(Assignee)

Comment 13

4 years ago
(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.
afaik this is not uplifted yet.
Flags: needinfo?(jhford)
Uplifted fcda94c9f9776c2ae181f78a3471e012db9e04a1 to:
v1-train: ce39a9234e7763b77652f0fb79d7dfd0ac4a4f23
status-b2g18: affected → fixed
Flags: needinfo?(jhford)
Group: b2g-core-security → core-security
v1.1.0hd: ce39a9234e7763b77652f0fb79d7dfd0ac4a4f23
status-b2g-v1.1hd: --- → fixed
status-firefox-esr24: --- → unaffected
Group: core-security
You need to log in before you can comment on or make changes to this bug.