310165 - [FIX]Allow userContent.css to link to local images

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[Andre Sihera]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

I was reading this tutorial on how to customise "userContent.css":

http://www.mozilla.org/support/firefox/tips#lay_linkcursornew

I checked the CSS 2.1 specification and in Section 18.1 "Cursors: the 'cursor'
property" it explains the following syntax:

> Value: [ [<uri> ,]* [ auto | crosshair | default | pointer | move | e-resize
>        | ne-resize | nw-resize | n-resize | se-resize | sw-resize | s-resize
>        | w-resize | text | wait | help | progress ] ] | inherit

Following the example in the specification for specifying URL-based cursors, I
created a file called "test.cur" on my local hard disk, dropped it into the
chrome directory of my profile, and added the following to my userContent.css:

/*
 * Change cursor for links that open in new window
 */
:link[target="_blank"], :visited[target="_blank"],
:link[target="_new"],   :visited[target="_new"]
{
	cursor:  url(test.cur), crosshair;
}

Then I started Firefox. It loaded the cursor fine (at first there were typing
errors that caused parsing errors on load but these disappeared so I know the
code is being parsed). However, on all pages that load I get the following
error:

"Security Error: Content at <some-url> may not load or link to
 file://H:/Firefox/Profile/chrome/test.cur"

This behaviour is open to interpration from the specification. Technically, it
can be argued that generating a "Security Error" on the cursor is defined as
not being able to handle it. Therefore, it should have chosen my second cursor
instead. However, the expected behaviour was to replace the cursor with the
one on my hard disk and produce no error.

There are two bugs that I found that are related to this problem:

- #163410, which talks about inter-frame security.
- #238093, which talks about the about:about page handling

The ability to be able to use local resources to be able to override default
CSS options of websites is a potentially useful feature. However, it appears
that any locally-based content referenced from inside userContent.css will
probably always generate errors (cursors, images, backgrounds, etc.).

Is it possible to remove this error for certain known type of files accessed
from certain areas?


Reproducible: Always

Steps to Reproduce:
1. Close Firefox
2. Create a cursor of your choice (or take the one attached to this report)
3. Drop the cursor file into your chrome directory alongside "userContent.css"
4. Add the following "cursor" directive to your "userContent.css" (assumes your
   file is called "test.cur", as attached to this report):

/*
 * Change cursor for links that open in new window
 */
:link[target="_blank"], :visited[target="_blank"],
:link[target="_new"],   :visited[target="_new"]
{
	cursor:  url(test.cur), crosshair;
}

5. Restart Firefox
6. Go to a page on the net that contains a link that opens in a new window
7. Check JavaScript Console

Actual Results:  
The cursor for links that open in new Windows defaults to the I-beam, and the
following error appears in the JavaScript console for every page accessed that
contains a URL that is opened in a new window:

"Security Error: Content at <some-url> may not load or link to
 file://H:/Firefox/Profile/chrome/test.cur"

Expected Results:  
Either:

1. Subsequent cursors in the CSS list should be rendered (in this case, the
   second "crosshair" cursor should be selected) instead of the I-beam. The
   errors remain in the JavaScript console.

or:

2. The user-defined cursor is displayed as intented and no errors are placed in
   the JavaScript console.

Comment 1

[Andre Sihera]

Attachment: Sample cursor file for reproducing test results

Comment 2

[Andre Sihera]

By the way, the link on the example URL that opens in the new window is the link
marked as "Real Time Disruption Map".

Comment 3

[zug_treno]

Related to bug 38447 comment 159?

Comment 4

[Andre Sihera]

(In reply to comment #3)
> Related to bug 38447 comment 159?

It is exactly related to this comment. Can we presume then that if this bug is
marked as fixed, but I am still seeing the problem, that this is a regression?

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

not a regression, because bug 38447 did not focus on userChrome.css. like that
bug said, this is unlikely to be specific to cursors anyway. resummarizing this bug.

to style system, I think.

Comment 6

[Andre Sihera]

(In reply to comment #5)
> not a regression, because bug 38447 did not focus on userChrome.css. like that
> bug said, this is unlikely to be specific to cursors anyway. resummarizing
this bug.
> 
> to style system, I think.

Did you mean "userContent.css" not "userChrome.css"?

Comment 7

[Jesse Ruderman]

See also bug 204140, "Allow user style sheets to load XBL from file: URLs".

Comment 8

[Adam Guthrie]

Dupe of bug 267248?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm... We do seem to use the document we're resolving style for to do the security check...

Perhaps we need a version of nsContentUtils::CanLoadImage that takes a principal (not just a document)?  And save off a principal associated to the sheet while parsing or something?

Comment 10

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

Yeah, that sounds like the right solution. However, we must make sure that the page can never get access to the userContent.css stylesheet and modify it. Is that something we can guarentee through all our APIs?

Comment 11

[Boris Zbarsky [:bzbarsky]]

At the moment, yes....

Comment 12

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

Could we maybe make it so that any dynamic changes to the stylesheet nukes the principal from it? Unless the principal of the currently running script is the same as the principal of the stylesheet?

Comment 13

[Boris Zbarsky [:bzbarsky]]

We'll need to store the principal in the URI values, not in the sheet.  There's no way to get back from the URI values to the sheet during resolution.  Which makes it kinda hard to clear the pointer if the sheet mutates, actually.

Perhaps we just need a way to mark sheets immutable and do that for user and ua sheets?  Not sure how that'd affect extensions...

Comment 14

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

Maybe we could allow mutations if done by chrome-level script?

Comment 15

[Boris Zbarsky [:bzbarsky]]

We can't even realistically detect that a mutation is done by script, much less what type of script.  Imho.  GetSubjectPrincipal is a dismal thing given C++ access to the methods.

Comment 16

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

yeah, I agree. Another alternative I thought about was to give extensions the an opportunity to tinker with the stylesheet before we mark it immutable. Don't know at all what the interfaces for that would be though.

Comment 17

[Boris Zbarsky [:bzbarsky]]

userContent.css is loaded at startup....

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

This basically propagates the relevant principals through to CanLoadImage (depends on bug 377091 for that) and makes sure that only sufficiently privileged (or same-origin) callers can modify sheets.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 263445 [details] [diff] [review]
Proposed fix

>+  // XXXbz would it make more sense to pass aLoadingPrincipal's URI
>+  // instead of aLoadingDocument's?

Or aReferrerURI?

>+  //-- Security check: Only scripts whose principal subsumes that of the
>+  //   style sheet can modify rule collections.
>+  nsresult rv = SubjectSubsumesInnerPrincipal(PR_FALSE);
>+  NS_ENSURE_SUCCESS(rv, rv);

Don't you also need to add similar security checks to anything that can mutate a rule that already exists if you want this model?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Or aReferrerURI?

Possibly, yes.  Sort of depends on what the API consumers expect...

> Don't you also need to add similar security checks to anything that can mutate
> a rule that already exists if you want this model?

Hmm.  If we require that getting the cssRules needs UniversalBrowserWrite, would that do?  I don't think there's a way to modify rules without doing that.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #20)
> Hmm.  If we require that getting the cssRules needs UniversalBrowserWrite,
> would that do?  I don't think there's a way to modify rules without doing that.

UniversalBrowserWrite?  Wouldn't that break any Web pages doing that?

Comment 22

[Boris Zbarsky [:bzbarsky]]

More precisely, the test is that either the caller's principal subsumes the sheet's principal (in practice that means caller is chrome or the principals are equal) or that the caller has UniversalBrowserWrite (if it's trying to access style sheets cross-domain).

We currently have that check in GetCssRules, just with UniversalBrowserRead.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ah, ok, changing that from Read to Write sounds fine.

Comment 24

[Boris Zbarsky [:bzbarsky]]

I'll do that, sure.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 263445 [details] [diff] [review]
Proposed fix


>   if (appType != nsIDocShell::APP_TYPE_EDITOR) {
>     // Editor apps get special treatment here, editors can load images
>     // from anywhere.

Is this still needed, or was it a hack to let editor load images from its own UA style sheet?

>+  // We don't use aLoadingPrincipal for anything here yet... but we will.
>+

Is there a bug on this?  And maybe an XXX comment?

>+#ifdef DEBUG
>+  nsCOMPtr<nsIContent> thisContent = do_QueryInterface(this);
>+  NS_ASSERTION(thisContent &&
>+               thisContent->NodePrincipal() == aDocument->NodePrincipal(),
>+               "Principal mismatch?");
>+#endif

Are you using aDocument->NodePrincipal() throughout the rest because it's cheaper?  Or some other reason?  Might be worth a comment.
>+  nsresult rv = SubjectSubsumesInnerPrincipal(PR_TRUE);

The previous comments meant this would change to PR_FALSE, right?  Given that, maybe the parameter should be removed?


sr=dbaron

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Is this still needed, or was it a hack to let editor load images from its own
> UA style sheet?

No, that was already handled by the ALLOW_CHROME flag.  This is a hack to let editor load images from file:// into documents that are either about:blank or some web principal (depending on what one is editing).  I'll add a comment to this effect.

> Is there a bug on this?  And maybe an XXX comment?

Bug 377092 basically covers that.  I'll add a comment here.

> aDocument->NodePrincipal() throughout the rest because it's cheaper?

Yeah.  It doesn't require the extra QI.  I'll comment.

> The previous comments meant this would change to PR_FALSE, right? 

Yes.  And yes, the parameter should go.  I'll do that.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to dbaron's comments

Comment 28

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

Comment on attachment 268581 [details] [diff] [review]
Updated to dbaron's comments

>Index: content/base/src/nsContentUtils.cpp
>+  // XXXbz would it make more sense to pass aLoadingPrincipal's URI
>+  // instead of aLoadingDocument's?
>   rv = NS_CheckContentLoadPolicy(nsIContentPolicy::TYPE_IMAGE,
>                                  aURI,
>                                  aLoadingDocument->GetDocumentURI(),
>                                  aContext,
>                                  EmptyCString(), //mime guess
>                                  nsnull,         //extra
>                                  &decision,
>                                  GetContentPolicy());

IMHO yes, otherwise content policies might unintentionally regress this bug, right? I'll make that change before I check in.

r=me with that.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Checked in with that change.