Closed Bug 863527 Opened 7 years ago Closed 6 years ago

Make insertAdjacentHTML more safe in system app error messages


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

Not set


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

blocking-b2g leo+
Tracking Status
firefox-esr24 --- unaffected
b2g18 --- fixed
b2g-v1.1hd --- fixed


(Reporter: pauljt, Assigned: pauljt)



(Keywords: sec-moderate)


(2 files)

This line is dangerous:

64   AppError.prototype.render = function() {
65'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.
Summary: Replace innerHTML with something more safe in system app error messages → Make insertAdjacentHTML more safe in system app error messages
QA Contact: ptheriault
Assignee: nobody → ptheriault
QA Contact: ptheriault
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]

From the blame I think :alive knows this code better.
Attachment #739494 - Flags: review?(21) → review?(alive)
Comment on attachment 739494 [details] [diff] [review]

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+
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]

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+
Group: b2g-core-security
This is a really minor change which fixes a significant bug in the system app so I am nominating for leo.
blocking-b2g: --- → leo?
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?
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)
Closed: 6 years ago
Resolution: --- → FIXED
(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
Flags: needinfo?(jhford)
Group: b2g-core-security → core-security
v1.1.0hd: ce39a9234e7763b77652f0fb79d7dfd0ac4a4f23
Group: core-security
You need to log in before you can comment on or make changes to this bug.