1408349 - (CVE-2019-11718) Possible unsanitized write to innerHTML for Activity Stream's snippets

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect, P3)

Description

[Mark Banner (:standard8)]

I was looking at upgrading Activity Stream to the latest eslint-plugin-mozilla, which also starts enabling rules from eslint-plugin-no-unsanitized.

It raised an error here:

https://github.com/mozilla/activity-stream/blob/019fdbda6f3a07a4f8cb2beccba8e5ff14926cc8/system-addon/content-src/lib/snippets.js#L253

    // Note that injecting snippets can throw if they're invalid XML.
    snippetsEl.innerHTML = payload;

activity-stream/system-addon/content-src/lib/snippets.js (1/0)
  ✖  253:5  Unsafe assignment to innerHTML  no-unsanitized/property

I couldn't see any code that was sanitizing the payload, but it seems an intentional assignment to innerHTML.

I don't know where the payload is coming from, hence raising the issue to get it documented.

Comment 1

[Kate Hudson :k88hudson]

This is existing code ported from the old aboutHome.js (http://searchfox.org/mozilla-central/source/browser/base/content/abouthome/aboutHome.js#334) which is required for the existing snippets service to run. 

Payloads come from the snippets service, the code for which lives here https://github.com/mozmeao/snippets-service and the documentation for which lives here https://abouthome-snippets-service.readthedocs.io/en/latest/.

We brought this up during our security review and they were aware of it.

Our team would like to move to an approach that receives JSON only from the service and implement all templates in Firefox, however because it would require a rewrite of the entire system for a team that is really small (there are only a couple of folks who work on the service), it was determined that a rewrite for 57 would be out of scope.

That said, I'd like to try to resolve this as soon as possible. Hopefully we'll be able to get the resources to be able to fix it for 58.

Comment 2

[Frederik Braun [:freddy]]

Yes, this is a known weakness that's been carried over from aboutHome.js. The notable difference is that about:home did not contain any sensitive information (e.g., history), but ActivityStream does.

That leads me to the following questions: 
Do we expect active markup like <script> tags in snippets?
If yes, who owns the risk of a compromised snippet service
If no, do we have an HTML/XML sanitizer in ActivityStream that could clean out active markup (like script tags)?

I know we can do something like this in chrome-JS code:

> let uri = NetUtil.newURI(document.baseURI);
> let fragment = parserUtils.parseFragment(stringThatMayContainXSS, parserUtils.SanitizerAllowComments, true, uri, embeddingDiv);
> embeddingDiv.appendChild(fragment);

Comment 3

[Ed Lee :Mardak]

Yes, there's expected to be active markup. Here's an example of snippets payload:

view-source:https://snippets.mozilla.com/show/7314/

Line 69 has a var ABOUTHOME_SNIPPETS = JSON.parse('…

Then ABOUTHOME_SNIPPETS[0].code contains html including css and <script> (inline), in this case:

138: "<script type=\"text/javascript\">"
139: " //<![CDATA["
140: " (function() {"
141: "     var snippet = document.getElementById('snippet');"


Also to be clear about the user content of Activity Stream, it contains top sites urls, bookmarks, and recent history. Additionally, sometimes those are shown with screenshots -- for top sites 560x380 -- that could have been captured of a logged in state, e.g., email.

Comment 4

[Frederik Braun [:freddy]]

Can we still find a way to disable access to the other ActivityStream sections?
What do we expect the scripts to do, generally?
I'm thinking of embedding the snippet HTML into an <iframe sandbox="allow-scripts allow-top-navigation" srcdoc="...">.


NB: This rewrite is a great chance to improve Firefox's security in a meaningful way, by not trusting the snippet server more than we have to.

Comment 5

[Ed Lee :Mardak]

In the past, snippets have been able to take over the whole page if necessary. It looks like typically they are just a rectangular section anchored to the bottom of the screen taking up the whole width. So, maybe the snippets content can be loaded into an iframe, k88hudson?

Comment 6

[Kate Hudson :k88hudson]

The script tags are needed because they contain custom logic related to each individual snippet (e.g. deciding which users to target given profile age, deciding which snippet to randomly select) as well as the code that attaches to various parts of the HTML elements in templates (the submit button, the block button). I do think this functionality could be implemented in Firefox if we do the rewrite to land the templates into a system add-on and make the service return JSON, it will just require some engineering time.

The snippet service is currently managed by :giorgos ([e-mail]) if you need more info about it.

Using an iframe wouldn’t be ideal since the snippet is not always preset / has to appear and hide dynamically on the page, can be variable height, and needs to respond to changes in the Activity Stream UI. Our impression from our initial discussion with security reviewers was that the existing approach was acceptable for this release and could be improved in the future, but we will schedule an additional meeting with them to make sure we’re doing the right thing here.

Comment 7

[Frederik Braun [:freddy]]

(In reply to Kate Hudson :k88hudson from comment #6)
> Using an iframe wouldn’t be ideal since the snippet is not always preset /
> has to appear and hide dynamically on the page, can be variable height, and
> needs to respond to changes in the Activity Stream UI. 

Makes sense.

> Our impression from
> our initial discussion with security reviewers was that the existing
> approach was acceptable for this release and could be improved in the
> future, but we will schedule an additional meeting with them to make sure
> we’re doing the right thing here.

Oh, to be clear, this bug is _not_ about blocking the release.
Just figuring out if this is a good opportunity to how change how things work.

Comment 8

[Mark Banner (:standard8)]

I'm looking to upgrade Activity Stream's eslint-plugin-mozilla version for some other things I'm doing. As part of that, it'll pull in eslint-plugin-no-unsanitized and I'll need to whitelist the issue in the source (// eslint-disable-next-line no-unsanitized/property)

Are we happy with that? Or should we wait until we resolve this?

Comment 9

[Ed Lee :Mardak]

Probably doesn't make that much of a difference, but we could generally disable no-unsanitized/property if we don't want to point directly at that line… (Although someone who really was digging would just easily run with the rule erroring and disabling generally could accidentally miss a new unsanitized added to code.)

Comment 10

[Frederik Braun [:freddy]]

(Taking Dan's needinfo, as I've looked at the code recently)

I think it's OK to whitelist this instance of innerHTML:
The snippet service is an "intendex XSS" that's been known for a while: Snippets authors can write code that will appear for all of our users.
Though the risk is increasing with snippets moving from about:home to ActivityStream, it's still limited to authors on our snippets service.
And btw we're re-reviewing the service and it's publishing process in bug 1408972, which will hopefully allow us to mitigate the risk even more.

Comment 11

[Frederik Braun [:freddy]]

I think here's a likely path forward:

Remove the <div> and use <iframe sandbox="allow-scripts allow-top-navigation" srcdoc="...">.

Due to the new – isolated – security context, the snippets code which will lose direct access to IndexedDB. To retain functionality, all access to gSnippestMap have to be remoted, i.e., turned into a postMessage protocol that proxies access for the snippets iframe.
I have carefully reviewed the usage of IndexedDB and consider them OK to be shared with the potentially harmful snippets code:

This will be the most work, but seems quite manageable. Another bit of work will be around hiding and showing the iframe and styling it appropriately, as Kate mentioned in comment 6.

P.S: I had a chat with Andrei to help me understand how ActivityStream works, but all potential flaws in this proposal  are completely mine :)

Comment 12

[Ed Lee :Mardak]

There was some discussion in Austin with Snippets people about moving to some structured snippets, so activity stream would control the display of this content. k88hudson, do you have a bug open for that design and work?

Comment 13

[:Gijs (he/him)]

Could we host the scripts externally and use CSP and a sanitizer to avoid the use of inline script entirely, relying on a set of allowed-by-CSP https-secured mozilla.org domains to further restrict what script runs even when script does run in the iframe?

Comment 14

[Tim Spurway [:tspurway]]

this will be fixed when Snippets infrastructure is replaced by ASRouter in meta bug #1432588

Comment 15

[:Gijs (he/him)]

(In reply to Tim Spurway [:tspurway] from comment #14)

this will be fixed when Snippets infrastructure is replaced by ASRouter in
meta bug #1432588

Has this happened now?

Comment 16

[Ed Lee :Mardak]

ASRouter powered snippets was preffed on for all channels in bug 1518619. Bug 1540939 will be removing the old snippets code. Additionally bug 1500061 is to tighten up CSP now that we don't inject snippets scripts.

Comment 17

[Ed Lee :Mardak]

The innerHTML was removed in bug 1546190 exporting bug 1540939
https://hg.mozilla.org/mozilla-central/rev/8ff5048f2b90#l16.273

CSP 'unsafe-inline' was removed in bug 1546535 exporting bug 1500061
https://hg.mozilla.org/mozilla-central/rev/01c3e1ae707d#l1.32