Bug 1366420 (CVE-2017-7840)

Javascript injection / XSS in bookmark export

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: hanno, Assigned: mak)

Tracking

({sec-low})

unspecified
Firefox 58
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox56 wontfix, firefox57 fixed, firefox58 fixed)

Details

(Whiteboard: self-xss [fxsearch][adv-main57+])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Posted file bookmarks.html
It is possible to inject Javascript into the HTML export file of the bookmark manager via the Tags field.

To reproduce:
* Create a bookmark with a tag like " onmouseover="alert(1)" "
* Export bookmarks to HTML via bookmark manager
* open bookmarks.html file and move over the bookmark

This is almost certainly not intended and the tags should be properly escaped. I thought a while whether this would have any security implications. I couldn't come up with a realistic scenario where this matters, but one could construct less realistic scenarios (e.g. a publicly available firefox installation where the bookmarks of the users are regularly exported and uploaded to a webpage). So I'd say it's a very low severity security issue, but still should be fixed.
I'm assuming we've long cleaned up titles and other elements that can be set by the page. If it's only tags this is essentially self-xss, but I worry about how we missed escaping on that field. Are any others affected?
Flags: needinfo?(mak77)
Keywords: sec-low
Whiteboard: self-xss
(Assignee)

Comment 2

2 years ago
hm, cool, I had written a long reply, but then I've lost it due to some recent bugs breaking sessionhistory :(
Btw, let's sum up.

The bug is here:
http://searchfox.org/mozilla-central/rev/a14524a72de6f4ff738a5e784970f0730cea03d8/toolkit/components/places/BookmarkHTMLUtils.jsm#1146
We have an escapeHTMLEntities util, but we didn't apply it to tags (ooops)

HTML is an obsolete format for bookmarks, we keep it for 2 reasons:
1. default bookmarks are still in this format
2. sharing with third party software (yes, an add-on could probably cover this)
The only ways to generate a Bookmarks.html are:
a. manually set autoExportHTML pref
b. go to the Library window and Export Bookmarks

In general we don't strip html from strings, nor we do much sanitization on input. Most services/components are unlikely to sanitize string contents for each of their APIs. The html export case is special and that's why we have escapeHTMLEntities, the json export does escaping by itself for example.

The only strings we get from Content are title and description, both are read from the document, doesn't looks like we do much sanitization of their content on input.
Maybe we should consider having a centralized sanitization method and make every API pass strings through it?
Are we worried only about html exports, or are there other cases that come to your mind we should pay attention to?
Flags: needinfo?(mak77)
Priority: -- → P2
(Reporter)

Comment 3

2 years ago
Given that there's no visible activity on fixing this and it's been open several months I intend to disclose this bug within a week.
(Assignee)

Comment 4

2 years ago
well, it's self xss and sec-low, so it makes sense it didn't get much priority. That said, the fix looks simple enough that it's worth just doing it.

Thank you for the heads-up.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: self-xss → self-xss [fxsearch]
(Assignee)

Comment 5

2 years ago
Posted patch Escape tags (obsolete) — Splinter Review
Attachment #8915088 - Flags: review?(standard8)
(Assignee)

Updated

2 years ago
Flags: in-testsuite+
Comment on attachment 8915088 [details] [diff] [review]
Escape tags

Review of attachment 8915088 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/tests/unit/test_bookmarks_html_escape_entities.js
@@ +58,5 @@
> +  for (let current = xml; current;
> +    current = current.firstChild || current.nextSibling || current.parentNode.nextSibling) {
> +    switch (current.nodeType) {
> +      case Ci.nsIDOMNode.ELEMENT_NODE:
> +      for (let {name, value} of current.attributes) {

nit: this for statement block is wrongly indented.
Attachment #8915088 - Flags: review?(standard8) → review+
(Assignee)

Comment 7

2 years ago
Posted patch Escape tagsSplinter Review
Attachment #8915088 - Attachment is obsolete: true
Attachment #8916221 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/08312cdfb230
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
(Assignee)

Comment 10

2 years ago
Comment on attachment 8916221 [details] [diff] [review]
Escape tags

Approval Request Comment
[Feature/Bug causing the regression]: no regression
[User impact if declined]: possible self-xss
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple change, has a test
[String changes made/needed]: none
Attachment #8916221 - Flags: approval-mozilla-beta?
Comment on attachment 8916221 [details] [diff] [review]
Escape tags

sec-low, low risk fix, beta57+
Attachment #8916221 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: firefox-core-security → core-security-release
(In reply to Marco Bonardo [::mak] from comment #10)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: not yet
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Marco's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Whiteboard: self-xss [fxsearch] → self-xss [fxsearch][adv-main57+]
Alias: CVE-2017-7840
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.