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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Emilio Cobos Álvarez [:emilio]]

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>

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Console says:

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

Comment 2

[Brian Grinstead [:bgrins]]

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.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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)

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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?)

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1490793 - Localization DTDs should not be subject to CSP

Comment 7

[Darkspirit]

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.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

(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!

Comment 9

[Bobby Holley (:bholley)]

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?

Comment 10

[Boris Zbarsky [: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.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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?

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.)

Comment 13

[Bobby Holley (:bholley)]

(bz will respond here after his call)

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Christoph, thoughts on comment 14?

Comment 16

[Christoph Kerschbaumer [: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

Comment 17

[Christoph Kerschbaumer [:ckerschb}]

Comment on attachment 9008564 [details]
Bug 1490793 - Localization DTDs should not be subject to CSP

Christoph Kerschbaumer [:ckerschb] has approved the revision.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/840113981a53
Localization DTDs should not be subject to CSP r=ckerschb

Comment 19

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/840113981a53