Closed Bug 1432966 Opened 3 years ago Closed 3 years ago

Sanitize HTML fragments created for chrome-privileged documents

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox58 --- fixed
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(1 file)

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.
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 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 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 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.
Attachment #8945249 - Flags: feedback+
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 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.
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.
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.
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b23adcdd6052d18407617de4ad5f7ed549a21e70
Bug 1432966: Sanitize HTML fragments created for chrome-privileged documents. r=bz f=gijs
https://hg.mozilla.org/mozilla-central/rev/b23adcdd6052
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1433414
Depends on: 1433708
Depends on: 1433659
Blocks: 1434086
(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
(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
See Also: → 1437147
Component: DOM: Security → DOM
Blocks: 1492041
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.