Closed
Bug 1432966
Opened 6 years ago
Closed 6 years ago
Sanitize HTML fragments created for chrome-privileged documents
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla60
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•6 years 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•6 years 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•6 years 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•6 years 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•6 years ago
|
Attachment #8945249 -
Flags: feedback+
Comment 8•6 years 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•6 years 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•6 years 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•6 years 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•6 years 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•6 years 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b23adcdd6052
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 15•6 years ago
|
||
https://hg.mozilla.org/releases/mozilla-release/rev/c2db4a50dc5c93b44852d9a5201f7ec062ecc6cb https://hg.mozilla.org/releases/mozilla-beta/rev/64737c752ac4af4766ad6f82720818521f3aca24
status-firefox58:
--- → fixed
status-firefox59:
--- → fixed
(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.
Assignee | ||
Comment 17•6 years 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
Updated•6 years ago
|
Component: DOM: Security → DOM
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•