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

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
blocking-basecamp: --- → ?
(Assignee)

Comment 1

5 years ago
I've confirmed this bug with a testcase.
Summary: (I suspect that) in-process mozapp/mozbrowser does not divide the window name namespace → In-process mozapp/mozbrowser does not divide the window name namespace
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 2

5 years ago
Created attachment 649950 [details] [diff] [review]
Bug 780351 - Tests for ensuring that mozapp divides the window name namespace, but mozbrowser does not.
(Assignee)

Comment 3

5 years ago
Created attachment 649951 [details] [diff] [review]
Patch, v1
Attachment #649951 - Flags: review?(bugs)
(Assignee)

Comment 4

5 years ago
Created attachment 649954 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
Attachment #649950 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 780546

Comment 5

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

># HG changeset patch
># User Justin Lebar <justin.lebar@gmail.com>
>
>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
Attachment #649951 - Flags: review?(bugs) → review-
(Assignee)

Comment 6

5 years ago
> 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

5 years ago
> > 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

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
> 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.
blocking-basecamp: ? → +
(Assignee)

Comment 10

5 years ago
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+.
Attachment #649951 - Flags: review- → review?(bugs)
(Assignee)

Updated

5 years ago
Blocks: 781320
(Assignee)

Comment 11

5 years ago
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.
Attachment #649954 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Depends on: 780761
No longer depends on: 780546

Updated

5 years ago
Attachment #649951 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #649954 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/815ab7dc9b12
https://hg.mozilla.org/mozilla-central/rev/e2e23255b6bf
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Depends on: 783509

Updated

5 years ago
Depends on: 815685
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.