780351 - In-process mozapp/mozbrowser does not divide the window name namespace

Status: RESOLVED FIXED

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

Description

[Justin Lebar (not reading bugmail)]

I haven't confirmed this yet, but I have a strong suspicion that in-process mozapp/mozbrowser is not a window name namespace barrier.  That is, if two in-process apps call window.open with the same window name, the second one will load content into the first one.  Ouch.

I think this blocks bug 766873; what Tim is doing there is calling window.open from two different in-process apps and getting a collision on the window name.  (There may be a separate issue around principals also blocking bug 766873; I'm not sure yet.)

This blocks inasmuch as launching popups from in-process mozbrowser/mozapp blocks.  At the moment, it looks like we may want popups from some in-process apps (that's bug 766873); I don't know if the plan is to make everything except the browser app out of process for V1, and I don't know whether the plan is realistic.

There are other problems (if this bug is not invalid!), around in-process browser frames.  Anyway, we should fix this.

Comment 1

[Justin Lebar (not reading bugmail)]

I've confirmed this bug with a testcase.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Bug 780351 - Tests for ensuring that mozapp divides the window name namespace, but mozbrowser does not.

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Tests, v1

I suspect these tests will go randomorange for the same reason as the test in bug 780546 goes randomorange, so not marking for review until I figure that out.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 649951 [details] [diff] [review]
Patch, v1

># HG changeset patch
># User Justin Lebar <[e-mail]>
>
>Bug 780351 - Don't let code in different apps access each others' windows.
>
>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>index 48fb2dc..9cc4619 100644
>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>@@ -2844,25 +2844,49 @@ nsDocShell::CanAccessItem(nsIDocShellTreeItem* aTargetItem,
>     // Bug 13871:  Prevent frameset spoofing
>     // Bug 103638: Targets with same name in different windows open in wrong
>     //             window with javascript
>     // Bug 408052: Adopt "ancestor" frame navigation policy
> 
>     // Now do a security check
>     //
>     // Allow navigation if
>+    //  0) aAccessingItem and aTargetItem have the same is-in-browser-frame
>+    //     state and are part of the same app.
>     //  1) aAccessingItem can script aTargetItem or one of its ancestors in
>     //     the frame hierarchy or
>     //  2) aTargetItem is a top-level frame and aAccessingItem is its descendant
>     //  3) aTargetItem is a top-level frame and aAccessingItem can target
>     //     its opener per rule (1) or (2).
> 
>+    nsCOMPtr<nsIDocShell> targetDS = do_QueryInterface(aTargetItem);
>+    nsCOMPtr<nsIDocShell> accessingDS = do_QueryInterface(aAccessingItem);
>+    if (!!targetDS != !!accessingDS) {
>+        // We must be able to convert both or neither to nsIDocShell.
>+        return false;
>+    }
>+
>+    if (targetDS && accessingDS) {
>+        bool targetInBrowser = false, accessingInBrowser = false;
>+        targetDS->GetIsInBrowserElement(&targetInBrowser);
>+        accessingDS->GetIsInBrowserElement(&accessingInBrowser);
>+
>+        PRUint32 targetAppId = 0, accessingAppId = 0;
>+        targetDS->GetAppId(&targetAppId);
>+        accessingDS->GetAppId(&accessingAppId);
>+
>+        if (targetInBrowser != accessingInBrowser ||
>+            targetAppId != accessingAppId) {
>+            return false;
>+        }
>+    }
So this works only when we don't have nested browserelements. 
Also, does this work when window name is used in some iframe inside browserelement?

>     if (aTargetItem == aAccessingItem) {
>         // A frame is allowed to navigate itself.
>-        return true;  
>+        return true;
>     }
Shouldn't you keep this before your changes

Comment 6

[Justin Lebar (not reading bugmail)]

> So this works only when we don't have nested browserelements. 

Correct.  We do not support nested browser elements anywhere in Gecko.  Our whole security model is built up around the assumption that we do not have nested browser elements, and that the tuple (app-id, is-browser-inside-app) is sufficient to identify the data-jar/security namespace of a window.

> Also, does this work when window name is used in some iframe inside browserelement?

What do you mean?

>>     if (aTargetItem == aAccessingItem) {
>>         // A frame is allowed to navigate itself.
>>-        return true;  
>>+        return true;
>>     }
> Shouldn't you keep this before your changes

It's just a trailing whitespace change.  If you'd prefer that I did not include it in this patch, I will remove it.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

> > Shouldn't you keep this before your changes
> 
> It's just a trailing whitespace change.  If you'd prefer that I did not
> include it in this patch, I will remove it.
I mean the if (aTargetItem == aAccessingItem) part.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Justin Lebar [:jlebar] from comment #6)
> > Also, does this work when window name is used in some iframe inside browserelement?
> 
> What do you mean?

I was thinking variations of http://mozilla.pettay.fi/moztests/iframecontainer.html
Say, if you have <iframe name="bar"> in the app which has also browserelement, and then the
page inside browser element has <iframe> which contains a link which targets bar.

Comment 9

[Justin Lebar (not reading bugmail)]

> I mean the if (aTargetItem == aAccessingItem) part.

Oh, I can move it above, sure.  It's functionally the same, because X.isBrowser == X.isBrowser, but I agree it's cleaner.

> Say, if you have <iframe name="bar"> in the app which has also browserelement, and then the
> page inside browser element has <iframe> which contains a link which targets bar.

If I understand correctly, you're thinking

  <iframe mozapp>
    <iframe name="bar">
    <iframe mozbrowser>
      <iframe>
        <a target="bar">

?  In this case, the <a> should not be able to target bar.  That's what the code does.

Comment 10

[Justin Lebar (not reading bugmail)]

Comment on attachment 649951 [details] [diff] [review]
Patch, v1

Aside from moving the aTargetItem check above the check I added, I don't know what else I need to do here to get r+.

Comment 11

[Justin Lebar (not reading bugmail)]

Comment on attachment 649954 [details] [diff] [review]
Tests, v1

I ran these tests 100 times locally (on a machine which was manifesting the orange in bug 780761), and didn't see any failures.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/815ab7dc9b12
https://hg.mozilla.org/mozilla-central/rev/e2e23255b6bf