1366420 - (CVE-2017-7840) Javascript injection / XSS in bookmark export

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Hanno Boeck]

Attachment: 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.

Comment 1

[Daniel Veditz [:dveditz]]

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?

Comment 2

[Marco Bonardo [:mak]]

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?

Comment 3

[Hanno Boeck]

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.

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Marco Bonardo [:mak]]

Attachment: Escape tags

Comment 6

[Mark Banner (:standard8)]

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.

Comment 7

[Marco Bonardo [:mak]]

Attachment: Escape tags

Comment 8

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08312cdfb2304264e6871357fe2e6e7831272d21

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08312cdfb230

Comment 10

[Marco Bonardo [:mak]]

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

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8916221 [details] [diff] [review]
Escape tags

sec-low, low risk fix, beta57+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c4235f5a700c

Comment 13

[Andrei Vaida [:avaida]]

(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.