1232903 - User stylesheet can no longer load XBL from external files

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: binding.zip

Steps to reproduce:
1. Open Firefox's profile folder.
2. Create "chrome" directory if not exist.
3. Open the "chrome" folder.
4. Copy userContent.css and binding.xml from the attachment.
5. Restart Firefox if it is already open.
6. Visit bugzilla.mozilla.org

Actual result:
Blank page is displayed. Browser Console will have the following message:
Security Error: Content at https://bugzilla.mozilla.org/ may not load data from file:///C:/Users/xxx/AppData/Roaming/Mozilla/Firefox/Profiles/a4qb23x6.default/chrome/binding.xml.

Expected result:
An alert dialog saying "hello, world" should be displayed.

Looks like this is a fallout from bug 1195162.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: userContent.zip

If the binding url is embedded as a data: URL, it will work.

Comment 2

[Boris Zbarsky [:bzbarsky]]

This may have been a semi-purposeful change... though it seems broken to me.  We should allow user stylesheets to do this.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm fine with that, but we'd need to come up with some understandable policy.

Should we simply say that any resource can be loaded into any document if the triggering-principal is the system principal?

Comment 4

[Boris Zbarsky [:bzbarsky]]

That seems fine to me, generally speaking...

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1232903_allow_triggeringprincipal_systemprincipal.patch

First, thanks :emk for the testcase!

Jonas, Boris, as discussed we can bail out of ContentSecurityChecks if the TriggeringPrincipal is the systemPrincipal, but we have to set the intialSecurityChecksDone-flag, otherwise assertions like [1] will trigger.

For the record, I also pasted all of the relevant arguments when calling doContentSecurityCheck() underneath. The only thing left to do is to update the docs to reflect that change. What would we like in the docs?

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsBaseChannel.cpp#640

doContentSecurityCheck {
  channelURI: file:///home/ckerschb/.mozilla/firefox/nldnkpqg.default/chrome/binding.xml
  loadingPrincipal: https://bugzilla.mozilla.org/
  triggeringPrincipal: SystemPrincipal
  contentPolicyType: 9
  securityMode: 1
  initalSecurityChecksDone: no
  enforceSecurity: no
}

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8710759 [details] [diff] [review]
bug_1232903_allow_triggeringprincipal_systemprincipal.patch

Review of attachment 8710759 [details] [diff] [review]:
-----------------------------------------------------------------

I definitely feel like this is a scary change. Some form of additional opt-in might be a good idea. But this is your call.

However this won't work for redirects since we still end up enforcing security there.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1232903_allow_triggeringprincipal_systemprincipal.patch

(In reply to Jonas Sicking (:sicking) from comment #6)
> I definitely feel like this is a scary change. Some form of additional
> opt-in might be a good idea. But this is your call.

As discussed in our meeting today, we can make the change less scary by bailing early only if the contentPolicyType is TYPE_XBL. No need to introduce an additional security flag.
 
> However this won't work for redirects since we still end up enforcing
> security there.

Why would that not work? I don't see why at the moment. The triggeringPrincipal would still be System and also the contentPolicyType woulnd't change and would still be TYPE_XBL, no?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8725091 [details] [diff] [review]
bug_1232903_allow_triggeringprincipal_systemprincipal.patch

Review of attachment 8725091 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/security/nsContentSecurityManager.cpp
@@ +362,5 @@
> +  // TriggeringPrincipal is the Systemprincipal.
> +  if (nsContentUtils::IsSystemPrincipal(loadInfo->TriggeringPrincipal()) &&
> +      loadInfo->GetExternalContentPolicyType() == nsIContentPolicy::TYPE_XBL) {
> +    return loadInfo->SetInitialSecurityCheckDone(true);
> +  }

This code doesn't run on redirects. So redirects would still get blocked. This should live in nsContentSecurityManager::CheckChannel which is now the function which is called for both the initial request and for redirects.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1232903_allow_triggeringprincipal_systemprincipal.patch

Jonas, given [1] we should probably always allow the load if the triggeringPrincipal is the systemPrincipal.

I moved code into |CheckChannel()| so it's also evaluated for redirects. I verified expected behavior described in Comment 0.

Let's do this!

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1246500#c19

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8725479 [details] [diff] [review]
bug_1232903_allow_triggeringprincipal_systemprincipal.patch

Review of attachment 8725479 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed.

::: dom/security/nsContentSecurityManager.cpp
@@ +478,5 @@
> +  // Allow the load if TriggeringPrincipal is the SystemPrincipal which
> +  // is e.g. necessary to allow user user stylesheets to load XBL from
> +  // external files.
> +  if (nsContentUtils::IsSystemPrincipal(loadInfo->TriggeringPrincipal())) {
> +    return loadInfo->SetInitialSecurityCheckDone(true);

No need to call SetInitialSecurityCheckDone(). nsContentSecurityManager::doContentSecurityCheck will do that.

Just return NS_OK

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80549d9e7528

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80549d9e7528

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Jonas, how far should we uplift that fix? 46 and 45? Let me know, I can file the uplift request.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'll let you decide how far to uplift. You're the module owner :)

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8725479 [details] [diff] [review]
bug_1232903_allow_triggeringprincipal_systemprincipal.patch

Approval Request Comment
Currently we block user stylesheets to load XBL from external files - with this patch we are going to fix that.

[Feature/regressing bug #]:
Bug 1195162 - Use channel->ascynOpen2 dom/xbl/nsXBLService.cpp which landed in 43.

[User impact if declined]:
I am not sure how often users actually load XBL from external files, I would assume not too often.

[Describe test coverage new/current, TreeHerder]:
Manual testing from Comment 0.

[Risks and why]:
low, because we bail out of security checks only if the loadingprincipal and the triggernprincipal are different where the triggeringprincipal is the systemprincipal which should only happen for stylesheet loads in that particular case.
 
[String/UUID change made/needed]:
no

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15)
> [User impact if declined]:
> I am not sure how often users actually load XBL from external files, I would
> assume not too often.

I don't claim I'm typical :), but I have XBL in my user stylesheet in my regular profile.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8725479 [details] [diff] [review]
bug_1232903_allow_triggeringprincipal_systemprincipal.patch

Too late for 45 but ok for 46

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4e25ce8c02e7