Closed Bug 1408349 (CVE-2019-11718) Opened 4 years ago Closed 2 years ago
Possible unsanitized write to inner
HTML for Activity Stream's snippets
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.
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.
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);
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.
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?
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 (email@example.com) 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.
(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.
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?
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.)
(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.
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 :)
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?
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?
this will be fixed when Snippets infrastructure is replaced by ASRouter in meta bug #1432588
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Component: Activity Streams: Newtab → New Tab Page
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main68+]
You need to log in before you can comment on or make changes to this bug.