Sanitize HTML fragments created for chrome-privileged documents

RESOLVED FIXED in Firefox 58

Status

()

RESOLVED FIXED
a year ago
12 days ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed, firefox59 fixed, firefox60 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
Ideally, we should apply a fairly strict CSP to chrome documents that blocks inline scripts, since the risk of XSS in chrome documents is much higher than it is in web content. Unfortunately, we currently rely on so much inline JS in our static XUL documents that that's not really feasible in the short term.

So until we have a better solution, we should at least mitigate the risk as much as possible, and simply sanitize any HTML/XML fragments that we create for a chrome document.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
I was hoping this would apply without any friction. And for the most part it did. Except for devtools, where its templating and some of its uses of React don't work with our sanitizer.

For now, I added a chrome-only unsafeSetInnerHTML method and an allowUnsafeHTML attribute to allow devtools code to continue working, but I really don't like that they're doing this kind of thing in chrome-privileged documents.
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
Comment on attachment 8945249 [details]
Bug 1432966: Sanitize HTML fragments created for chrome-privileged documents.

https://reviewboard.mozilla.org/r/215474/#review221142


Static analysis found 3 defects in this patch.
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/shared/gcli/source/lib/gcli/util/util.js:501
(Diff revision 2)
>      exports.clearElement(elem);
>      elem.appendChild(contents);
>      return;
>    }
>  
> -  if ('innerHTML' in elem) {
> +  if ('unsafeSetInnerHTML' in elem) {

Error: Strings must use doublequote. [eslint: quotes]

::: devtools/shared/gcli/source/lib/gcli/util/util.js:504
(Diff revision 2)
>    }
>  
> -  if ('innerHTML' in elem) {
> +  if ('unsafeSetInnerHTML' in elem) {
> +    // FIXME: Stop relying on unsanitized HTML.
> +    elem.unsafeSetInnerHTML(contents);
> +  }

Error: Closing curly brace does not appear on the same line as the subsequent block. [eslint: brace-style]

::: devtools/shared/gcli/source/lib/gcli/util/util.js:505
(Diff revision 2)
>  
> -  if ('innerHTML' in elem) {
> +  if ('unsafeSetInnerHTML' in elem) {
> +    // FIXME: Stop relying on unsanitized HTML.
> +    elem.unsafeSetInnerHTML(contents);
> +  }
> +  else if ('innerHTML' in elem) {

Error: Strings must use doublequote. [eslint: quotes]

Comment 6

a year ago
mozreview-review
Comment on attachment 8945249 [details]
Bug 1432966: Sanitize HTML fragments created for chrome-privileged documents.

https://reviewboard.mozilla.org/r/215474/#review221144


Static analysis found 3 defects in this patch.
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/shared/gcli/source/lib/gcli/util/util.js:501
(Diff revision 3)
>      exports.clearElement(elem);
>      elem.appendChild(contents);
>      return;
>    }
>  
> -  if ('innerHTML' in elem) {
> +  if ('unsafeSetInnerHTML' in elem) {

Error: Strings must use doublequote. [eslint: quotes]

::: devtools/shared/gcli/source/lib/gcli/util/util.js:504
(Diff revision 3)
>    }
>  
> -  if ('innerHTML' in elem) {
> +  if ('unsafeSetInnerHTML' in elem) {
> +    // FIXME: Stop relying on unsanitized HTML.
> +    elem.unsafeSetInnerHTML(contents);
> +  }

Error: Closing curly brace does not appear on the same line as the subsequent block. [eslint: brace-style]

::: devtools/shared/gcli/source/lib/gcli/util/util.js:505
(Diff revision 3)
>  
> -  if ('innerHTML' in elem) {
> +  if ('unsafeSetInnerHTML' in elem) {
> +    // FIXME: Stop relying on unsanitized HTML.
> +    elem.unsafeSetInnerHTML(contents);
> +  }
> +  else if ('innerHTML' in elem) {

Error: Strings must use doublequote. [eslint: quotes]

Comment 7

a year ago
mozreview-review
Comment on attachment 8945249 [details]
Bug 1432966: Sanitize HTML fragments created for chrome-privileged documents.

https://reviewboard.mozilla.org/r/215474/#review221230

::: dom/base/nsDocument.cpp:6194
(Diff revision 3)
> +  return (!nsContentUtils::IsSystemPrincipal(NodePrincipal()) ||
> +          mAllowUnsafeHTML);

Potential follow-up fodder: increasing the scope here to include resource://, maybe moz-extension, and non-about:blank about: pages, even when not system-principal'd.

Updated

a year ago
Attachment #8945249 - Flags: feedback+

Comment 8

a year ago
mozreview-review
Comment on attachment 8945249 [details]
Bug 1432966: Sanitize HTML fragments created for chrome-privileged documents.

https://reviewboard.mozilla.org/r/215474/#review221326

r=me

::: dom/base/nsContentUtils.h:1603
(Diff revision 3)
>                                             nsIDOMDocumentFragment** aReturn);
>    static already_AddRefed<mozilla::dom::DocumentFragment>
>    CreateContextualFragment(nsINode* aContextNode, const nsAString& aFragment,
>                             bool aPreventScriptExecution,
> -                           mozilla::ErrorResult& aRv);
> +                           mozilla::ErrorResult& aRv,
> +                           bool aNeverSanitize = false);

Having args after the ErrorResult is a bit weird.  And having boolean args for something like this is not great; an enum would be better.

It's probably OK to clean that up in a followup.
Attachment #8945249 - Flags: review?(bzbarsky) → review+

Comment 9

a year ago
mozreview-review
Comment on attachment 8945249 [details]
Bug 1432966: Sanitize HTML fragments created for chrome-privileged documents.

https://reviewboard.mozilla.org/r/215474/#review221352

::: devtools/client/responsive.html/components/Browser.js:22
(Diff revision 3)
>  const { getToplevelWindow } = require("../utils/window");
>  
>  const FRAME_SCRIPT = "resource://devtools/client/responsive.html/browser/content.js";
>  
> +// Allow creation of HTML fragments without automatic sanitization, even
> +// though we're in a chrom-privileged document.

Nit: chrom -> chrome

As an aside, I believe this document's use of `innerHTML` can be fixed once DevTools updates to React 16 which supports non-standard DOM attrs.

::: devtools/shared/gcli/source/lib/gcli/util/util.js:503
(Diff revision 3)
>      return;
>    }
>  
> -  if ('innerHTML' in elem) {
> +  if ('unsafeSetInnerHTML' in elem) {
> +    // FIXME: Stop relying on unsanitized HTML.
> +    elem.unsafeSetInnerHTML(contents);

Bug 1429421 intends to eventually remove all of devtools/shared/gcli, so that should cover this one long term.
(Assignee)

Comment 10

a year ago
mozreview-review-reply
Comment on attachment 8945249 [details]
Bug 1432966: Sanitize HTML fragments created for chrome-privileged documents.

https://reviewboard.mozilla.org/r/215474/#review221326

> Having args after the ErrorResult is a bit weird.  And having boolean args for something like this is not great; an enum would be better.
> 
> It's probably OK to clean that up in a followup.

Yeah, I didn't really like it either, but I wanted the arg to be optional. I guess I can could just add another overload, though.

I'll change it from a bool to an enum, too.
(Assignee)

Comment 11

a year ago
mozreview-review-reply
Comment on attachment 8945249 [details]
Bug 1432966: Sanitize HTML fragments created for chrome-privileged documents.

https://reviewboard.mozilla.org/r/215474/#review221230

> Potential follow-up fodder: increasing the scope here to include resource://, maybe moz-extension, and non-about:blank about: pages, even when not system-principal'd.

That might be a good idea. We can pretty easily enable CSP for non-system resource: pages, though (and already have it for moz-extension:, which don't allow unsafe-inline under any circumstances), so it might be better to just do that instead.
(Assignee)

Comment 12

a year ago
mozreview-review-reply
Comment on attachment 8945249 [details]
Bug 1432966: Sanitize HTML fragments created for chrome-privileged documents.

https://reviewboard.mozilla.org/r/215474/#review221352

> Nit: chrom -> chrome
> 
> As an aside, I believe this document's use of `innerHTML` can be fixed once DevTools updates to React 16 which supports non-standard DOM attrs.

Good to know. I'll file a bug for that, then. Thanks.
(Assignee)

Comment 13

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b23adcdd6052d18407617de4ad5f7ed549a21e70
Bug 1432966: Sanitize HTML fragments created for chrome-privileged documents. r=bz f=gijs

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b23adcdd6052
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1433414
Depends on: 1433708
Depends on: 1433659
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #12)
> > As an aside, I believe this document's use of `innerHTML` can be fixed once DevTools updates to React 16 which supports non-standard DOM attrs.
> 
> Good to know. I'll file a bug for that, then. Thanks.

I filed bug 1434155 for this.
Depends on: 1434299
Depends on: 1434304
Depends on: 1434321
Depends on: 1434324
No longer depends on: 1434321
No longer depends on: 1434324
(Assignee)

Comment 17

a year ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) (at work week) from comment #16)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #12)
> > > As an aside, I believe this document's use of `innerHTML` can be fixed once DevTools updates to React 16 which supports non-standard DOM attrs.
> > 
> > Good to know. I'll file a bug for that, then. Thanks.
> 
> I filed bug 1434155 for this.

Thanks
Component: DOM: Security → DOM

Updated

6 months ago
Blocks: 1492041
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.