dom.ua_widget.enabled breaks video preview in bugs.chromium.org

RESOLVED FIXED in Firefox 64

Status

()

defect
P2
normal
RESOLVED FIXED
11 months ago
5 months ago

People

(Reporter: emilio, Assigned: timdream)

Tracking

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

()

Attachments

(1 attachment)

STR: Go to the url with dom.ua_widget.enabled = true

Expected: video gets previewed

Actual:

XML Parsing Error: undefined entity Location: https://bugs.chromium.org/p/chromium/issues/detail?id=882989 Line Number 10, Column 56:
            <span class="errorLabel" id="errorAborted">&error.aborted;</span>
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Console says:

Content Security Policy: The page’s settings blocked the loading of a resource at chrome://global/locale/videocontrols.dtd (“default-src”).
I wonder if we could somehow inline the videocontrols.dtd entities into the document fragment. I suspect we'd have to somehow preprocess it in so it doesn't break localizer workflows with the external file.
I prefer to keep the patch minimum by overcoming CSP with a check of the DOMParser.forceEnableXULXBL() flag. Have not figured out how that could work though.
Or, given that DOMParser is initialized from UA Widget script, the principal it got should be an expanded principal. If that's true I'll key to that ... (still reading the code)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #4)
> Or, given that DOMParser is initialized from UA Widget script, the principal
> it got should be an expanded principal. If that's true I'll key to that ...
> (still reading the code)

OK. We are not getting an expanded principal in DOMParser.cpp (I guess the principal tie to the window global, which is the document?)
We should permanently see subtitles (or only for a second if bug 1491045 gets fixed):
https://storage.googleapis.com/shaka-demo-assets/_bugs/firefox-cue-remove/text.html
It can be fixed with disabling dom.ua_widget.enabled.
See Also: → 1491066
(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #7)
> We should permanently see subtitles (or only for a second if bug 1491045
> gets fixed):
> https://storage.googleapis.com/shaka-demo-assets/_bugs/firefox-cue-remove/
> text.html
> It can be fixed with disabling dom.ua_widget.enabled.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1491066 for that. Nice catch, thanks!
Just talked to tim about the review.

The basic problem here is that we have the video controls binding using the window's DOMParser constructor over Xrays. The DOMParser uses the window principal rather than the caller principal for parsing, which trips up over CSP. So for the video controls case, we want to use the nsEP, and make that exempt from the CSP if it isn't already.

So we could either make DOMParser always use the caller principal (which changes behavior in the Xray case), or have a separate UAWidget-only API called parseWithCallerPrincipal or something, that does this on a special path. The former is more general, but also riskier. bz, WDYT?
Flags: needinfo?(bzbarsky)
I would prefer it if the privilege-escalating thing was very explicit, so would prefer a separate "parse as me" API, I think.

The other option, of course, is to not be using the window's DOMParser constructor to start with...   But I'm not sure we have a good mechanism for exposing webidl things in a binding global.
Flags: needinfo?(bzbarsky)
It turned out I cannot set the document created in the DOMParser to an expanded principal. I will hit this assertion added in bug 1301123.

https://searchfox.org/mozilla-central/rev/dd965445ec47fbf3cee566eff93b301666bda0e1/dom/base/nsNodeInfoManager.cpp#313-314

I could escalate the permission from nsEP to SystemPrincipal, though, by not creating a DOMParser.loadWithSubjectPrincipal() method but a DOMParser.loadWithSystemPrincipal() method. Do we want to do that?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bobbyholley)
If we want to completely avoid calling DOMParser from JS, we'll need to rethink how UA widgets populate the DOM instead of

https://searchfox.org/mozilla-central/rev/dd965445ec47fbf3cee566eff93b301666bda0e1/toolkit/content/widgets/videocontrols.js#2051-2121

(disregard the comment about Fluent; I am not sure how it will work with a content document so we might still need to load localization resource via DOMParser.)

It's just that, adding anything in C++ that parses another XML for DOM feels like a step backward (from the goal of XBL removal.)
(bz will respond here after his call)
Flags: needinfo?(bobbyholley)
OK.  So the fundamental issue here is this:

> Content Security Policy: The page’s settings blocked the loading of a resource at chrome://global/locale/videocontrols.dtd (“default-src”).

There are two ways we can have a TYPE_DTD load:

1) The DTD URI is not URI_IS_UI_RESOURCE but the doctype public identifier maps to a known DTD we ship with.  This is the "someone used an XHTML doctype" case, basically.  In this case we will do the load with a system triggering principal, so CSP won't block it.

2) The DTD URI is URI_IS_UI_RESOURCE.  In this case we do the load with the page's principal and do our normal content policy, CheckLoadURI, etc checks.  In practice, UI_RESOURCE means "chrome", "moz-icon", or "resource" URI.

My temptation would be to just not do a CSP check at all for TYPE_DTD loads.  The cases when a page can load a DTD from one of those protocols are pretty rare, and it's not exactly clear to me that per spec DTD loads should be happening at all.

(Note that we explicitly exempt DTD loads from the data document content policy, precisely so that they will work in DOMParser, which otherwise is not allowed to load any subresources at all.)
Christoph, thoughts on comment 14?
Flags: needinfo?(bzbarsky) → needinfo?(ckerschb)
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #14)
> My temptation would be to just not do a CSP check at all for TYPE_DTD loads.
> The cases when a page can load a DTD from one of those protocols are pretty
> rare, and it's not exactly clear to me that per spec DTD loads should be
> happening at all.

What Boris said seems correct to me. Please note that we even have a bug on file for determining the right policy for DTD loads (see Bug 1228117).

Regarding this bug however, we shouldn't mess with the Principal like in the proposed patch, but as Boris suggested we could exempt DTD loads from CSP. The right way to do this would be to update the function subjectToCSP() here:
  https://searchfox.org/mozilla-central/source/dom/security/nsCSPService.cpp#51
Flags: needinfo?(ckerschb)
Priority: -- → P2
Attachment #9008564 - Attachment description: Bug 1490793 - Overcoming CSP in UA Widget with AllowXULXBL flag r=bholley → Bug 1490793 - Localization DTDs should not be subject to CSP
Comment on attachment 9008564 [details]
Bug 1490793 - Localization DTDs should not be subject to CSP

Christoph Kerschbaumer [:ckerschb] has approved the revision.
Attachment #9008564 - Flags: review+
Pushed by tchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/840113981a53
Localization DTDs should not be subject to CSP r=ckerschb
See Also: → 1228117
https://hg.mozilla.org/mozilla-central/rev/840113981a53
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.