>>> My Info: Win7_64, Nightly 49, 32bit, ID 20160429030215 STR: 1. Install Flash 21_0_0_182, make sure it's set to "Ask to Activate" 2. Open attachment 8675621 [details] AR: No icon displayed in <object> and no background ER: Blocked plugin icon and warning background should be displayed in a normal way This is regression from bug 1206961. Regression range: > https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=632811bf4b6e96b54709d125c298493d30576eb8&tochange=bf69c3219b5ef75454640e9eaf087a2650c0c0eb

[Tracking Requested - why for this release]: Christoph, something to do with the XBL bindings we use for blocked plugins in content? Would there be something in the error console showing if there is a specific security check that is failing?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #1) > Christoph, something to do with the XBL bindings we use for blocked plugins > in content? Benjamin, most likely we didn't perform a CheckLoadURIWithPrincipal before converting imageLoader to use asyncOpen2() - but now we do. > Would there be something in the error console showing if there is a specific > security check that is failing? I see the following two errors in the console: Security Error: Content at https://bug1148978.bmoattachments.org/attachment.cgi?id=8675621 may not load or link to chrome://mozapps/skin/plugins/contentPluginActivate.png. Security Error: Content at https://bug1148978.bmoattachments.org/attachment.cgi?id=8675621 may not load or link to chrome://mozapps/skin/plugins/contentPluginClose.png. Obviously we could make those images accessible to content, but I am not sure that is what we actually want to do, also given Bug 1246568. How would you like to proceed?

Here are the intersting arguments when consulting nsContentSecurityManager: doContentSecurityCheck { channelURI: chrome://mozapps/skin/plugins/contentPluginActivate.png loadingPrincipal: https://bug1148978.bmoattachments.org/attachment.cgi?id=8675621 triggeringPrincipal: chrome://mozapps/skin/plugins/pluginProblem.css contentPolicyType: 37 securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS, initalSecurityChecksDone: no enforceSecurity: no }

Christoph, we need to be able to show these images from the plugin XBL binding (which is attached to content). Otherwise the plugin UI is seriously broken (this may affect other forms of the plugin binding, including the default click-to-play version and the plugin-crashed version). I do not have any specific opinion about how that should be achieved.

I don't have strong opinions about how to fix this. We could make the relevant resources accessible to content. We could insert a chrome stylesheet and have that link to the image. The chrome stylesheet would either have to be accessible to content, or we need to inject it into the document as a UA stylesheet. There might be other solutions too. I don't have an opinion about what the preferred solution is.

This is the usual problem: the new setup does CheckLoadURI checks on both triggeringPRincipal and loadingPrincipal (which I still think is wrong). The old setup only did them on triggeringPrincipal. How are we handling this for user stylesheets linking to file:// images right now? Or did we break that too?

If the triggeringPrincipal is the system principal, we don't do any security checks on the loadingPrincipal. Not CheckLoadURI, not CheckMayLoad, not nsIContentPolicies. However, according to comment 3, that's not the case here. The triggeringPrincipal is a codebase-principal containing the URI chrome://mozapps/skin/plugins/pluginProblem.css Are you saying that you don't think we should ever do CheckLoadURI checks on the loadingPrincipal? Or are you saying that you don't think that we should do so if the triggeringPrincipal is the system principal? Or that you don't think that we should do so if the triggeringPrincipal is a chrome URI? Or something else?

I think generally doing CheckLoadURI on the loadingPrincipal is pretty weird and has caused us problems all along.

Can you say why?

Because in lots of cases (user stylesheets, the case here) the loadingPrincipal has very little to do with the load and isn't getting access to the load's data anyway...

(In reply to Boris Zbarsky [:bz] from comment #6) > This is the usual problem: the new setup does CheckLoadURI checks on both > triggeringPRincipal and loadingPrincipal (which I still think is wrong). > The old setup only did them on triggeringPrincipal. It's not quite the usual problem as far as I understand. E.g. we started to allow user stylesheets to load XBL from external files if the TriggeringPrincipal was the SystemPrincipal [1], but that's not quite the case here as Jonas points out in comment 7. Obviously performing security checks only on the TriggeringPrincipal and skipping the security checks on the loadingPrincipal [2] would fix the problem described in comment 0. I don't know what security implications it might have if we start performing doing security checks only on the triggeringPrincipal. Jonas? [1] http://hg.mozilla.org/mozilla-central/rev/116e62eb5e97#l1.43 [2] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#101 > How are we handling this for user stylesheets linking to file:// images > right now? Or did we break that too? I haven't tested, but I would imagine we break those too.

> I haven't tested, but I would imagine we break those too. Well, please test. If we did break that, we certainly need to fix it.

The loadingPrincipal often gets access to the data. For stylesheets, XHR and fonts, it gets most if not all of the response. For script, image and video, the loadingPrincipal gets access to some metadata. I think it's fine that we say that if the systemprincipal triggers a load, that we let that override the normal rules and hope that chrome code knows what it's doing (generally not a safe assumption, but probably the best we can do). I guess I could live with a similar exception for chrome:// and res:// URLs. Does user stylesheets not have a system principal?

> Does user stylesheets not have a system principal? They didn't use to, but maybe we changed that?

Boris: What URI does the UA stylesheet have? I'm quite reluctant to in general skip CheckLoadURI checks on the loading principal. The loadingPrincipal many times gets access to the resource being loaded. Could we make exceptions only when the triggering principal is a chrome:// or resource:// URI?

See comment 15

> Boris: What URI does the UA stylesheet have? You mean user stylesheet? Typically file:// I would think, but I don't know what sort of URIs GreaseMonkey, say, produces.

As discussed, we should skip CheckLoadURIChecks() using the LoadingPricnipal if SEC_ALLOW_CROSS_ORIGIN_* security flag is set. Manually verified that user stylesheets work again.

Gentle ping ;-) Any chance to get this reviewed and landed soon? Ideally the fix should be uplifted to mozilla48 since it regressed there. The next branch date is 6th of June, and an uplift request to close to the branch date will usually be rejected.

Thanks. I hope this can land soon ;-)

Christoph, as I said before, we *really* need this fix (yesterday). Can you please get it landed. Sorry to be pushy. I also don't know in which time zone you are.

[Tracking Requested - why for this release]: This fixes a problem in Thunderbird which was caused by bug 1195173. That bug landed on mozilla48. So as far as I am concerned, Firefox 48 *is* affected although this bug here claims to be a regression of bug 1206961 which landed on mozilla49.

Taking the liberty to request checkin here. The review following comment #19 was without any comments, so this should be good to go. As I stated in the previous comment, we will need uplift to mozilla48 (currently in Aurora), since this fixes a regression from bug 1195173 which caused TB bug 1246500. Without this bug fixed, TB is basically not usable since our style sheets don't get loaded and therefore we can't display a single message correctly.

(In reply to Jorg K (PTO during summer, NI me) from comment #21) > Christoph, as I said before, we *really* need this fix (yesterday). Can you > please get it landed. Sorry to be pushy. I also don't know in which time > zone you are. Agreed, we need this landed before the merge. I see you already set the checkin-needed flag, hopefully someone from the sheriffs can check that in for us soon.

Unfortunately this patch seems to have bitrotted a bit, please update it.

Comment on attachment 8753481 [details] [diff] [review] bug_1269254_skip_checkloadurichecks_on_loadingprincipal.patch Approval Request Comment Also see comment #23. [Feature/regressing bug #]: bug 1195173 [User impact if declined]: Thunderbird 48 can't load its style sheets. Firefox add-ons most likely also affected. [String/UUID change made/needed]: None

Thanks!

Comment on attachment 8753481 [details] [diff] [review] bug_1269254_skip_checkloadurichecks_on_loadingprincipal.patch The risk evaluation wasn't provided. Without it, I cannot approve this patch

Correct. Christoph, can you please fill in the missing bits: Approval Request Comment [Feature/regressing bug #]: bug 1195173 [User impact if declined]: Thunderbird 48 can't load its style sheets. Firefox add-ons most likely also affected. [Describe test coverage new/current, TreeHerder]: ??? [Risks and why]: ??? [String/UUID change made/needed]: None

(In reply to Jorg K (PTO during summer, NI me) from comment #32) > Approval Request Comment > [Feature/regressing bug #]: bug 1195173 > [User impact if declined]: Thunderbird 48 can't load its style sheets. Firefox would block user stylesheets to load chrome:// urls. > [Describe test coverage new/current, TreeHerder]: Manual verification of this Bug and also Bug 1270703, as well as Bug 1246500. > [Risks and why]: Low - we are still performing security checks on the TriggeringPrincipal but skip security checks on the LoadingPrincipal. > [String/UUID change made/needed]: None

ok, thanks. Can we have someone to verify that it is actually fixed in 49?

Works fine in Thunderbird now after this has landed.

Comment on attachment 8753481 [details] [diff] [review] bug_1269254_skip_checkloadurichecks_on_loadingprincipal.patch OK, let's take it!

has problems to apply to aurora: merging dom/security/nsContentSecurityManager.cpp warning: conflicts while merging dom/security/nsContentSecurityManager.cpp! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue

Have you tried the attached patch? Or did you try to transplant this: https://hg.mozilla.org/mozilla-central/rev/0c174794580d The latter won't work (I think).

(In reply to Carsten Book [:Tomcat] from comment #38) > has problems to apply to aurora: > > merging dom/security/nsContentSecurityManager.cpp > warning: conflicts while merging dom/security/nsContentSecurityManager.cpp! > (edit, then use 'hg resolve --mark') > abort: unresolved conflicts, can't continue Thanks!

The patch is in 48. Remove the tracking-firefox48.