221428 - [FIX]Define security policy for stylesheets [was: Security check for same-origin access mistreats data: URIs]

Status: RESOLVED FIXED

(Core :: Security, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Testcase coming up.  The short story is that the fix for bug 135267 makes it
impossible to script a stylesheet loaded via a data: uri, since that in fact has
a different origin than the page.

I'm not sure how CheckSameOrigin() should work exactly and whether it would be
safe for it to succeed when the uri is a data: uri (since presumably cases when
data: is running script would use the principal, and data: should inherit the
page principal).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 2

[Brendan Eich [:brendan]]

Shouldn't the origin of the data: URL be the origin of the page that references
it via an attribute value; or if it was set dynamicly, the origin of the script
that set it?

/be

Comment 3

[Boris Zbarsky [:bzbarsky]]

That's also possible, yes... the problem is that there is no way to get that
information from a URI object...

Comment 4

[Darin Fisher]

shouldn't you have a reference to the nsIChannel used to load the document?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Perhaps, but how does that help here?  The data: url can come from a stylesheet
(eg @import) too...

Comment 6

[Darin Fisher]

bz: well, since nsIChannel::owner holds a reference to the nsIPrincipal, if you
can get ahold of the nsIChannel for the document from which the data: URL was
extracted, then you can just use the principal from that nsIChannel to set the
principal on the nsDataChannel.  (i don't know off hand if the principal's can
be shared, or if a new principal must be constructed with the same origin, but
that detail should be inconsequential i would think.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Like I said, the data: can come from a stylesheet, not a document....  And from
the data: uri itself I don't see how I can get at the principal.

I suppose stylesheets could keep a pointer to their principal, but that may be a
little wasteful of memory..

Comment 8

[Darin Fisher]

i was using "document" loosely to include the stylesheet "document" ;-)

>I suppose stylesheets could keep a pointer to their principal, but that may be a
>little wasteful of memory..

this seems like the right solution to me.  is it really that bloaty to keep a
reference in the stylesheet to the nsIPrincipal used when loading the stylesheet?

Comment 9

[Boris Zbarsky [:bzbarsky]]

So I think we _do_ want to have stylesheets store their principal...  Just need to do it.

Comment 10

[Boris Zbarsky [:bzbarsky]]

What about stylesheets with javascript: URIs?  Should that script run with the loader's principal?  Or will that be a problem for myspace and company?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Another question.  When doing a security check for a stylesheet load, should we use the principal of the stylesheet, or the principal of the document that loaded the stylesheet?  Or some intersection (meet in brendan's terms)?  Or should we simply set the principal of the sheet to the intersection to start with, except for user/ua sheets?

Note that bug 204140 and bug 310165 are basically requesting that we use the sheet's principal.

Requesting blocking, since this bug is sort of turning into a "define a security policy for stylesheets" bug, which is something I think we should absolutely do for 1.9.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Patch will depend on bug 369244 to not execute JS...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Checkpoint

Attached patch basically works; once the dependencies land I'll write some tests and then look for reviews.

Propagating the principal to the non-@import loads is going to be a separate bug (like bug 204140 and bug 310165).  For now I'm just trying to get the principal for the stylesheet right without allowing "@import url('javascript:...');" to execute.

Comment 14

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

Boris, since you started working on this you win the grand price :)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Ready for review

Again, first-come, first-served.  ;)

Summary of changes:
- Add a principal to style sheet inners
- Make stylesheets always have inners and principals and remove null checks
- Propagate principals through the CSSLoader
- Change the CSSLoader's "same URI means it's the same sheet" setup to match on
  both URI and originating principal.  Not that the latter should really change 
  much...

Comment 16

[Peter Van der Beken [:peterv]]

I have started reviewing.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Peter, should I count that as an r+sr?  Or do you want Jonas to look at this too?

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 256892 [details] [diff] [review]
Ready for review

Yeah, I can do r+sr.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip

Comment 20

[Boris Zbarsky [:bzbarsky]]

Checked in.