327078 - [FIX]Shouldn't set owner to chrome JS principal for subframe loads

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Make the code more compatible with what nsDocShell::LoadURI does

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 2

[Boris Zbarsky [:bzbarsky]]

Actually, nsLocation::CheckURL seems to have the same issue... 

Perhaps we should just consolidate this check in docshell (always ignore the owner if it's system)?  Or something?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Sicking, thoughts on comment 2?

Comment 4

[Johnny Stenback (:jst)]

(In reply to comment #2)
> Actually, nsLocation::CheckURL seems to have the same issue... 
> 
> Perhaps we should just consolidate this check in docshell (always ignore the
> owner if it's system)?  Or something?
> 

Yeah, seems like we'd want to do that. I had a look at the patch and it looks good, I could sr if you attach a new patch that consolidates this code.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 211805 [details] [diff] [review]
Like so

Actually, this is no good.  :(

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk patch

I think this is the right behavior in the general case...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Same for the 1.8 branch

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Er, actually for the 1.8 branch

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Aviary/1.7 version of same

This one could use an extra-careful look, since the nsIScriptSecurityManager API is so benighted back then.... :(

Comment 10

[Daniel Veditz [:dveditz]]

Looks good to me. I'll test it against the known problems on the 1.7 branch in the morning.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 213284 [details] [diff] [review]
Trunk patch

+  nsCOMPtr<nsIURI> referrer;
+  
+  if (principal == sysPrin) {
+    // We're a chrome node.  Belt and braces -- inherit the principal for this
+    // load instead of just forcing the system principal.  That way if we have
+    // something loaded already the principal used will be that of what we
+    // already have loaded.
+    loadInfo->SetInheritOwner(PR_TRUE);
+
+    // Also, in this case we don't set a referrer, just in case.
+  } else {
+    // We'll use our principal, not that of the document loaded inside us.
+    // This is very important; needed to prevent XSS attacks on documents
+    // loaded in subframes!
+    loadInfo->SetOwner(principal);
+
+    rv = principal->GetURI(getter_AddRefs(referrer));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    loadInfo->SetReferrer(referrer);
+  }

referrer is only used in the else case here, so you could move the declaration in there as well.

Other then that, this looks right to me. sr=jst

Comment 12

[Tim Riley [:timr]]

Comment on attachment 213288 [details] [diff] [review]
Aviary/1.7 version of same

a=timr for drivers

Comment 13

[Tim Riley [:timr]]

Boris-  Consider Johnny's suggestion and then let's get this landed and built.

Comment 14

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

Comment on attachment 213288 [details] [diff] [review]
Aviary/1.7 version of same

>Index: content/base/src/nsFrameLoader.cpp
...
>   if (!referrer) {
>-    // We're not being called form script, tell the docshell
>-    // to inherit an owner from the current document.
>-
>-    loadInfo->SetInheritOwner(PR_TRUE);
>-
>+    // system principals, dammit
>     referrer = base_uri;
>   }

Why base_uri? That seems very wrong since content can set that at will. Shouldn't this be doc->GetDocumentURI()?

I'm not entierly sure when this can happen though. How would a principal not have a uri?

Other then that, looks good.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> That seems very wrong since content can set that at will.

Except this is chrome, not content.

> How would a principal not have a uri?

When it's an nsSystemPrincipal.

I can still use the documentURI there if you'd prefer.  Probably safer that way...

I'll land this in a bit, I guess.  Need to get home first.

Comment 16

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

Comment on attachment 213288 [details] [diff] [review]
Aviary/1.7 version of same

Ah, i see, system principals doesn't have a uri. Still seems like GetDocumentURI is the right thing to call.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Aviary/1.7 patch updated to comments

Comment 18

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

Comment on attachment 213284 [details] [diff] [review]
Trunk patch

This looks good. What about the nsLocation comment though? It sounds like a *really* good idea if we can just let the docshell do the security checks. Trying to check in all places that can cause loads is just a bad design that is bound to lead to holes.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8 patch updated to comments

Waiting on a= for 1.8.0.2 to land this on both 1.8 branches.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk patch updated to comments

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Also fixed on aviary and 1.7.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> What about the nsLocation comment though?

We should document somewhere that using window.location from C++ is unsafe and that it shouldn't be done....

> It sounds like a *really* good idea if we can just let the docshell do the
> security checks.

That would be great, if we could figure out what security checks it should do.  In particular, the CheckLoadURI check is a pain in the behind -- what principal should it use? Docshell doesn't really have a good way to tell.  :(

This is something we've talked about on and off and so far no one's thought of a reasonable solution.  :(

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 213347 [details] [diff] [review]
1.8 patch updated to comments

approved for 1.8.0 branch, a=dveditz

Comment 24

[Daniel Veditz [:dveditz]]

tested that this fixes bug 319858 and bug 328469 in Moz 1.7.13

Comment 25

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branches.

Comment 26

[Tracy Walker [:tracy]]

verified on Linux with dep bug test cases

Comment 27

[Dave Liebreich [:davel]]

Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)

Comment 28

[Tracy Walker [:tracy]]

verified on Firefox 1.5.0.2 Linux with dep bug test cases

Comment 29

[Jay Patel [:jay]]

Adding verified1.8.0.2 based on Tracy's testing on that 1.8.0 branch.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Now that this is all fixed everywhere, I'd like to add some of the thinking that led to the current setup to this bug:

There are at least five relevant principals around when nsFrameLoader::LoadURI is called:

A)  The nodePrincipal of mOwnerContent
B)  The principal of the document in mDocShell
C)  The principal returned by GetSubjectPrincipal
D)  The principal of whatever is causing the load to happen (whatever that
    means; more below).
E)  The principal corresponding to the URI/channel being loaded.

There are two things that we need to do with a principal in LoadURI:

1)  CheckLoadURI call
2)  Set an owner on the docshell loadinfo.  This is used as the principal in
    which javascript: URIs and data: URIs run when loaded.

For #2 the options are actually to either set an explicit owner (to A, B, or C, D), to set the "inherit owner" flag (which effectively uses B), or to set neither (which uses E unless the subject principal is system, in which case it uses B).

What we did before this patch landed is:

For (1) we use C if it's not null, and A otherwise.
For (2) we use C if it's not null, and set the inherit owner boolean
        to true otherwise.

This is bogus on several levels.  Most obviously, if there's some random JS on the stack that doesn't mean that we should be running subframes with those URIs with the principal of that JS.  For example, if chrome sets innerHTML on a content node to a string containing a subframe with such a URI, we do NOT want to use the chrome principal for the subframe load or the checkLoadURI check.

The problem is that there are some cases in which I can't tell what principals to use.  Here are the possible use cases I can think of:

I)  Parsing a document.  LoadFrame() is called from either SetAttr or
    BindToTree.  I posit that in both cases we should be using principal
    A for both (1) and (2), no matter what the JS stack looks like.

II) BindToTree called not from parsing (eg appendChild via DOM).
    What principals should we use?  Can we even tell this BindToTree call apart
    from the other one?

III) SetAttr called not from parsing (eg setAttribute via DOM).  What
     principals should we use?  We can tell this case apart from parsing via
     the aNotify arg to SetAttr, I think.

There are two reasons to not use principal A throughout.  First of all, if principal A is chrome we could have some moron extension messing with the src attribute in XUL.  We wouldn't really want to run the resulting javascript: URI with the extension's permissions in the scope of a random web page, right?  I think in this case we want to use principal A (same as principal C) for the checkLoadURI check, and use principal B as the principal in which to run the js URL.

On the other hand, if evilsite has goodsite in a subframe and sets the src of the subframe to a javascript: URI, we do want to use principal A (which in this case happens to coincide with principal C) as the principal for the js URL.  The CheckLoadURI check should use principal C....

So what this patch does is:

1)  Always use principal A for the CheckLoadURI check.
2)  Use principal A as the principal to run with, unless it's the system
    principal, in which case we use principal B (via setting "inherit" to
    true).

See also the comment at <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsFrameLoader.cpp&rev=1.68#150>.