105050 - Webpage is loaded in message pane because of "opener"

Status: VERIFIED FIXED

(MailNews Core :: Security, defect, P1)

Description

[Henrik Gemal]

Wow... try this:
1. make sure you dont have any navigator windows running
1. start Mail and News
2. click on a link in a message the produces a new navigator window
3. in the window load: http://gemal.dk/test/3.html

and wupti the website http://gemal.dk is loaded in the message pane of Mail & News!

20011015

Comment 1

[Mitchell Stoltz (not reading bugmail)]

Good one. This could be used for mail spoofing. Henrik, with your permission I'd
like to make this bug Security-Sensitive to protect our users. You'll still be
able to view and comment on it. If you object, just uncheck the
Security-Sensitive box above.

Clearly, a webpage should not be able to change the location of its opener if
the opener is a mail message.

A severity of 'blocker' generally means that this bug blocks someone's tests or
daily work. As I don't think that's the case, I'm changing the severity to Major.

Comment 2

[Bradley Baetz (:bbaetz)]

*** Bug 107952 has been marked as a duplicate of this bug. ***

Comment 3

[Mitchell Stoltz (not reading bugmail)]

Attachment: Fix - don't expose opener property if the opener is a mail message

Comment 4

[Mitchell Stoltz (not reading bugmail)]

Johnny, can you think of any other way for a script on a webpage to get a handle
to a mail message's window object?

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 69388 [details] [diff] [review]
Fix - don't expose opener property if the opener is a mail message

>+    nsCOMPtr<nsIDocShell> openerDocShell;
>+    openerSGO->GetDocShell(getter_AddRefs(openerDocShell));
>+    if (openerDocShell) {
>+      nsCOMPtr<nsIDocShellTreeItem> docShellAsItem(do_QueryInterface(openerDocShell));

Loose the null check of openerDocShell, do_QueryInterface() does that for you.

>+      if (docShellAsItem) {
>+        nsCOMPtr<nsIDocShellTreeItem> openerRootItem;
>+        docShellAsItem->GetRootTreeItem(getter_AddRefs(openerRootItem));
>+        if (openerRootItem) {
>+          nsCOMPtr<nsIDocShell> openerRootDocShell(do_QueryInterface(openerRootItem));

Loose the null check for openerRootItem...

>+          if (openerRootDocShell) {
>+            PRUint32 appType;
>+            nsresult rv = openerRootDocShell->GetAppType(&appType);
>+            if (NS_SUCCEEDED(rv) && appType != nsIDocShell::APP_TYPE_MAIL) {
>+              *aOpener = mOpener;
>+              NS_IF_ADDREF(*aOpener);
>+              return NS_OK;

This would IMO be cleaner if you'd null out *aOpener as the first thing in this
method, and then just set it here and do the NS_IF_ADDREF() and the return as
the last thing below.

>+            }
>+          }
>+        }
>+      }
>+    }
>+  }
>+  *aOpener = nsnull;

i.e. move this to the beginning of the method.

>   return NS_OK;
> }

sr=jst if you fix that.

Comment 6

[Mitchell Stoltz (not reading bugmail)]

Attachment: New patch with the above suggestions

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 69566 [details] [diff] [review]
New patch with the above suggestions

sr=jst

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 69566 [details] [diff] [review]
New patch with the above suggestions

r=heikki

Comment 9

[tor]

The patch looks it will make GetOpener() somewhat expensive - how often is
it called?  lxr only turned up two references, which I find somewhat hard
to believe.

Comment 10

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.9

Comment 11

[Mitchell Stoltz (not reading bugmail)]

Fix checked in.

Comment 12

[Henrik Gemal]

v 20020304

Comment 13

[Johnny Stenback (:jst)]

Tor, the speed of GetOpener() should not be significant, and the changes don't
really make it that expensive. GetOpener() is very rarely used.