Closed Bug 848031 Opened 7 years ago Closed 7 years ago

about:healthreport performs .innerHTML with unescaped JSON

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 22
Tracking Status
firefox20 --- unaffected
firefox21 --- fixed
firefox22 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected

People

(Reporter: gps, Assigned: gps)

Details

(Keywords: sec-high, Whiteboard: [fhr])

Attachments

(1 file)

https://hg.mozilla.org/mozilla-central/file/c95439870e05/browser/base/content/abouthealthreport/abouthealth.js#l63 performs a |.innerHTML = | with a value that may contain unescaped HTML-like strings. e.g. if the JSON contains a string with "<", the produced HTML will be invalid and the feature won't work.

This is theoretically an attack vector. If untrusted content could result in an arbitrary string being added to the FHR payload, that string could eventually make its way into the DOM of a chrome-privileged page and code execution could ensue.

I don't believe untrusted content can currently cause injection of arbitrary strings into the FHR payload. But, this may not always hold. We should keep this door closed.

Even if there isn't a security issue here, the issue of 'payloads containing "<" fail to render' is a legitimate bug that needs fixed and preferably uplifted to Aurora.
I /think/ this will do it. raw-data is a leaf node, so textContent should be equivalent to innerText. But I'm not a DOM expert.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #721347 - Flags: review?(gavin.sharp)
Attachment #721347 - Flags: review?(gavin.sharp) → review+
Comment on attachment 721347 [details] [diff] [review]
Don't use innerHTML on untrusted data, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): about:healthreport landing
User impact if declined: possible injection of remote code into chrome compartment
Testing completed (on m-c, etc.): Trivial DOM change. Manually verified fix.
Risk to taking this patch (and alternatives if risky): Not much risk.
String or UUID changes made by this patch: None.
Attachment #721347 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/0e67704d6b18

Can we get a test for this?
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Flags: in-testsuite? → in-testsuite-
(In reply to Ryan VanderMeulen [:RyanVM] from comment #4)
> https://hg.mozilla.org/mozilla-central/rev/0e67704d6b18
> 
> Can we get a test for this?

I have a bug open somewhere to track test coverage of FHR's browser components. Can't find it though...

Parts of this page are due to be rewritten shortly. Hopefully tests will be part of the refactor.
Attachment #721347 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: core-security
You need to log in before you can comment on or make changes to this bug.