User stylesheet can no longer load XBL from external files

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: emk, Assigned: ckerschb)

Tracking

(Depends on 1 bug, {regression})

43 Branch
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed, firefox47 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

4 years ago
Posted file 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.
Reporter

Comment 1

4 years ago
Posted file userContent.zip
If the binding url is embedded as a data: URL, it will work.
Reporter

Updated

4 years ago
Version: unspecified → 43 Branch
This may have been a semi-purposeful change... though it seems broken to me.  We should allow user stylesheets to do this.
Flags: needinfo?(mozilla)
Flags: needinfo?(jonas)
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?
Flags: needinfo?(jonas)
That seems fine to me, generally speaking...
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
}
Flags: needinfo?(mozilla)
Attachment #8710759 - Flags: review?(jonas)
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
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.
Attachment #8710759 - Flags: review?(jonas) → review-

Updated

3 years ago
Depends on: 1246578
(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?
Attachment #8710759 - Attachment is obsolete: true
Attachment #8725091 - Flags: review?(jonas)
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.
Attachment #8725091 - Flags: review?(jonas) → review-
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
Attachment #8725091 - Attachment is obsolete: true
Attachment #8725479 - Flags: review?(jonas)
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
Attachment #8725479 - Flags: review?(jonas) → review+

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/80549d9e7528
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Jonas, how far should we uplift that fix? 46 and 45? Let me know, I can file the uplift request.
Flags: needinfo?(jonas)
I'll let you decide how far to uplift. You're the module owner :)
Flags: needinfo?(jonas)
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
Attachment #8725479 - Flags: approval-mozilla-beta?
Attachment #8725479 - Flags: approval-mozilla-aurora?
Reporter

Comment 16

3 years ago
(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 on attachment 8725479 [details] [diff] [review]
bug_1232903_allow_triggeringprincipal_systemprincipal.patch

Too late for 45 but ok for 46
Attachment #8725479 - Flags: approval-mozilla-beta?
Attachment #8725479 - Flags: approval-mozilla-beta-
Attachment #8725479 - Flags: approval-mozilla-aurora?
Attachment #8725479 - Flags: approval-mozilla-aurora+
Depends on: 1257650
Blocks: 1257650
No longer depends on: 1257650
You need to log in before you can comment on or make changes to this bug.