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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

defect
RESOLVED FIXED
13 years ago
2 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

({fixed1.8.1, verified1.7.13, verified1.8.0.2})

Trunk
mozilla1.9alpha1
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
blocking1.8.1 +
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical][rft-dl])

Attachments

(4 attachments, 4 obsolete attachments)

Make the code more compatible with what nsDocShell::LoadURI does
Posted patch Like soSplinter Review
Attachment #211805 - Flags: superreview?(peterv)
Attachment #211805 - Flags: review?(bugmail)
(Assignee)

Updated

13 years ago
Attachment #211805 - Flags: approval-branch-1.8.1?(jst)
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?
Sicking, thoughts on comment 2?

Updated

13 years ago
Blocks: 319858
Flags: blocking1.8.1+
Flags: blocking1.8.0.2+
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
(Assignee)

Updated

13 years ago
Blocks: 328454
(Assignee)

Updated

13 years ago
Blocks: 328469
(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 on attachment 211805 [details] [diff] [review]
Like so

Actually, this is no good.  :(
Attachment #211805 - Flags: superreview?(peterv)
Attachment #211805 - Flags: superreview-
Attachment #211805 - Flags: approval-branch-1.8.1?(jst)
Attachment #211805 - Flags: approval-branch-1.8.1-
(Assignee)

Updated

13 years ago
Group: security
Whiteboard: [sg:critical]
Posted patch Trunk patch (obsolete) — Splinter Review
I think this is the right behavior in the general case...
Attachment #213284 - Flags: superreview?(jst)
Attachment #213284 - Flags: review?(bugmail)
Posted patch Same for the 1.8 branch (obsolete) — Splinter Review
Attachment #213285 - Flags: superreview?(jst)
Attachment #213285 - Flags: review?(bugmail)
Attachment #213285 - Flags: approval1.8.0.2?
Attachment #213285 - Flags: approval-branch-1.8.1?(jst)
Attachment #213285 - Attachment is obsolete: true
Attachment #213286 - Flags: superreview?(jst)
Attachment #213286 - Flags: review?(bugmail)
Attachment #213286 - Flags: approval1.8.0.2?
Attachment #213286 - Flags: approval-branch-1.8.1?(jst)
Attachment #213285 - Flags: superreview?(jst)
Attachment #213285 - Flags: review?(bugmail)
Attachment #213285 - Flags: approval1.8.0.2?
Attachment #213285 - Flags: approval-branch-1.8.1?(jst)
Posted patch Aviary/1.7 version of same (obsolete) — Splinter Review
This one could use an extra-careful look, since the nsIScriptSecurityManager API is so benighted back then.... :(
Attachment #213288 - Flags: superreview?(jst)
Attachment #213288 - Flags: review?(bugmail)
Attachment #213288 - Flags: approval1.7.13?
Attachment #213288 - Flags: approval-aviary1.0.8?
Looks good to me. I'll test it against the known problems on the 1.7 branch in the morning.
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
Attachment #213284 - Flags: superreview?(jst) → superreview+
Attachment #213286 - Flags: superreview?(jst)
Attachment #213286 - Flags: superreview+
Attachment #213286 - Flags: approval-branch-1.8.1?(jst)
Attachment #213286 - Flags: approval-branch-1.8.1+
Attachment #213288 - Flags: superreview?(jst) → superreview+

Comment 12

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

a=timr for drivers
Attachment #213288 - Flags: approval1.7.13?
Attachment #213288 - Flags: approval1.7.13+
Attachment #213288 - Flags: approval-aviary1.0.8?
Attachment #213288 - Flags: approval-aviary1.0.8+

Comment 13

13 years ago
Boris-  Consider Johnny's suggestion and then let's get this landed and built.
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.
Attachment #213288 - Flags: review?(bugmail) → review+
> 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 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.
Attachment #213288 - Attachment is obsolete: true
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.
Attachment #213284 - Flags: review?(bugmail) → review+
Waiting on a= for 1.8.0.2 to land this on both 1.8 branches.
Attachment #213286 - Attachment is obsolete: true
Attachment #213286 - Flags: approval1.8.0.2?
Attachment #213284 - Attachment is obsolete: true
Fixed on trunk.

Also fixed on aviary and 1.7.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
> 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.  :(
(Assignee)

Updated

13 years ago
No longer blocks: 328454
(Assignee)

Updated

13 years ago
Attachment #213347 - Flags: approval1.8.0.2?
Comment on attachment 213347 [details] [diff] [review]
1.8 patch updated to comments

approved for 1.8.0 branch, a=dveditz
Attachment #213347 - Flags: approval1.8.0.2? → approval1.8.0.2+
tested that this fixes bug 319858 and bug 328469 in Moz 1.7.13
Fixed on 1.8 branches.
verified on Linux with dep bug test cases
(Assignee)

Updated

13 years ago
Depends on: 328932
Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)
Whiteboard: [sg:critical] → [sg:critical][rft-dl]
verified on Firefox 1.5.0.2 Linux with dep bug test cases

Comment 29

13 years ago
Adding verified1.8.0.2 based on Tracy's testing on that 1.8.0 branch.
Depends on: 331807
Group: security
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>.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.