220421 - window.open from link onclick gets security error?

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Brendan Eich [:brendan]]

Testcase as attachment coming up, but it's trivial:

<a href="http://www.apr.com/" onClick="window.open(this.href); return false">APR</a>

I reduced this from a real search results page from http://www.mlslistings.com/.
 If you click on the APR link, you'll see a new window that loads www.apr.com as
expected, and the return false cancels the load of that url in the current
page's window, as expected.  But in the JS console, I see

Security Error: Content at file:///tmp/t.html may not load data from
http://www.apr.com/.

(I put the testcase in /tmp/t.html.)

There's a variation involving target=_blank (which is what the real page does)
that I need to test next.  Hope this isn't a false alarm.

/be

Comment 1

[Brendan Eich [:brendan]]

Ok, the relevant change was not target="_blank" or alt="...".  Just change the
test input to look like this (which I'll attach next):

<a href="http://www.apr.com/" onClick="var w=window.open(this.href); w.focus();
return false">APR</a>

The w.focus() call results in this error (which the $@%*! JS console will not
let me copy -- why not?):

Error: uncaught exception: Permission denied to get proprety Window.focus

the uncaught exception apparently causes the onClick (idiosyncratic or HTML
traditional spelling preserved from testcase) handler to terminate abruptly, not
to return false or anything akin.  That results in the href URL loading in the
window of the page containing the link.  (That's ok, I think.)

After the focus security exception comes the same

Security Error: Content at file:///tmp/t.html may not load data from
http://www.apr.com/.

as reported in comment 0.

When did this regress?  What is causing the "Content at ... may not load data
from ..." when there is no attempt to get a property in the first testcase, show
in comment 0?  IOW, the w.focus access should be granted, but is not -- that's a
bug. But what is the "data" being loaded in the testcase without any w.focus
call?  No exception should be given in either case.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: combined testcase page

Two links: "APR" (whose onclick calls w.focus()) and "APR (no focus)".

/be

Comment 3

[Brendan Eich [:brendan]]

Note how when clicking on "APR (no focus)", the return false or something
equivalent does execute, canceling the load of the href in the link's window. 
You get only the Security Error: ... entry in the JS console.

When clicking on the "APR" link, you get both the Window.focus uncaught
exception, which bypasses the false return from onclick, and then you get the
Security Error.

This says that the bogus Security Error about load[ing] data is happening after
the link's onclick runs to completion, returns false, and cancels the href load
in the link's window.  When would that Security Error be thrown, if it's after
onclick returns?

/be

Comment 4

[Brendan Eich [:brendan]]

Could use some testing to find out whether this afflicts 1.4.x too -- Asa?

/be

Comment 5

[Brendan Eich [:brendan]]

Magic 8-ball says caillon is our first best hope.

/be

Comment 6

[Darin Fisher]

using:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5a) Gecko/20030708 Mozilla Firebird/0.6

when i click on the "APR" link, i do not see "Error: uncaught exception:
Permission denied to get proprety Window.focus" in the JS console.

Comment 7

[Anne (:annevk)]

Verified
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6a) Gecko/20030925

comment 6, try a recent build.

Comment 8

[Brendan Eich [:brendan]]

I'm running Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030827.

/be

Comment 9

[Boris Zbarsky [:bzbarsky]]

So... first off, I do _not_ see this in my tip build (from today) with my normal
profile.  So there's maybe something profile dependent here, which is likely why
darin does not see it.

With a vanilla profile, I see the bug with a tip build and with builds back to
2003-05-05-22.  Build 2003-05-04-22 does not show the problem.  The
ever-so-helpful bonsai gives us:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&&ortby=Date&date=explicit&mindate=2003-05-04+22%3A00%3A00&maxdate=2003-05-05+22%3A00%3A00&cvsroot=%2Fcvsroot

and the obvious culprit is Mitch's fix for bug 159424.  In particular, the 
nsGlobalWindow.cpp changes (revision 592 of that file) add a call to
CheckSameOriginURI(), which does its own error reporting if the origins are
different.  Perhaps it should not do that, or take a boolean flag indicating
whether to treat an origin mismatch as error-reporting-worthy?  In this case an
origin mismatch is nothing out of the ordinary; it just means we have to clear
the window scope.

Comment 10

[Brendan Eich [:brendan]]

bz: thanks for digging.

Ugh.  It seems bug 159424's patch also regressed bug 211719.  I think it's fine
to tighten the scope-clearing logic for security reasons, but a same-origin
check that bites window.open where no variable is set on the new window (where
the return value of window.open is discarded, as in the (no focus) testcase) is
flat wrong.

Why is the focus method locked up?  That must be a different change, perhaps
subject to a pref.

/be

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Why is the focus method locked up?

Did you ever disallow raising and lowering windows in this profile with an
oldish build?  More to the point, what are the capability.policy.* prefs in your
prefs.js?

Comment 12

[Brendan Eich [:brendan]]

Nominating bug to block 1.5.  Untested patch in a minute.

bz: sure enough, my prefs.js has

user_pref("capability.policy.default.Window.focus", "noAccess");

Did I do that?  I trust it's not the default, but I don't remember adding it. 
Maybe I did in a fit of pique at some evil website.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: please help test this

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 132261 [details] [diff] [review]
please help test this

I won't be able to test this weekend, but here are some comments:

>Index: caps/idl/nsIScriptSecurityManager.idl

>+    /**
>+     * Utility method for comparing two URIs.  For security purposes, two URIs
>+     * are equivalent if their scheme, host, and port are equal.

I'm trying to think of a way to avoid adding a new method here.  But if as you
say this is wanted as a quick fix, it should suffice for that.	I'll think
about it for the trunk.  Anyway, add something to say it returns true if the
URIs are equivalent, false otherwise.


>+     */
>+    [noscript] boolean securityCompareURIs(in nsIURI aSubjectURI,
>+                                           in nsIURI aObjectURI);



>Index: caps/src/nsScriptSecurityManager.cpp

>-/* Static function for comparing two URIs - for security purposes,
>- * two URIs are equivalent if their scheme, host, and port are equal.
>- */
>-/*static*/ nsresult
>+nsresult

Shouldn't this be NS_IMETHODIMP?  I don't think this will compile on all
platforms as-is.


> nsScriptSecurityManager::SecurityCompareURIs(nsIURI* aSourceURI,
>                                              nsIURI* aTargetURI,
>                                              PRBool* result)
> {
>     nsresult rv;
>     *result = PR_FALSE;
> 
>     if (aSourceURI == aTargetURI)
>Index: dom/src/base/nsGlobalWindow.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v
>retrieving revision 1.619
>diff -p -u -8 -r1.619 nsGlobalWindow.cpp
>--- dom/src/base/nsGlobalWindow.cpp	27 Sep 2003 04:18:14 -0000	1.619
>+++ dom/src/base/nsGlobalWindow.cpp	27 Sep 2003 18:35:22 -0000
>@@ -523,31 +523,30 @@ GlobalWindowImpl::SetNewDocument(nsIDOMD
> 
>       if (treeItem) {
>         PRInt32 itemType = nsIDocShellTreeItem::typeContent;
>         treeItem->GetItemType(&itemType);
> 
>         isContentWindow = itemType != nsIDocShellTreeItem::typeChrome;
>       }
> 
>-      PRBool isAboutBlank = PR_FALSE;
>       nsCAutoString url;
>       docURL->GetSpec(url);
>-      isAboutBlank = url.Equals(NS_LITERAL_CSTRING("about:blank"));
>+      PRBool isAboutBlank = url.Equals(NS_LITERAL_CSTRING("about:blank"));
> 
>       PRBool isSameOrigin = PR_FALSE;
>       if (isAboutBlank && mOpenerScriptURL) {
>         nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(mDocShell));
>         if (webNav) {
>           nsCOMPtr<nsIURI> newDocURL;
>           webNav->GetCurrentURI(getter_AddRefs(newDocURL));
>           if (newDocURL && sSecMan) {
>-            isSameOrigin =
>-              NS_SUCCEEDED(sSecMan->CheckSameOriginURI(mOpenerScriptURL,
>-                                                       newDocURL));
>+            sSecMan->SecurityCompareURIs(mOpenerScriptURL,
>+                                         newDocURL,
>+                                         &isSameOrigin);

Does this need to also check the return value?	There are some cases (which
granted are unlikely) where SecurityCompareURIs does fail.  Though I guess the
implementation seems to return a false value in those cases....

r=caillon for 1.5 final with comments addressed.

Comment 15

[Brendan Eich [:brendan]]

Attachment: fixed NS_IMETHODIMP gaffe, added comment about return value

Thanks, Chris.

I don't see a problem extending nsIScriptSecurityManager, even if only with a
utility method.  I'll hold off on trunk checkin if you like, but it seems
better to me to land this on the trunk, then on the 1.5 branch -- and make
further improvements on the trunk during 1.6a.

However you slice it, nsGlobalWindow.cpp wants to compare origins without an
error report.  The method to do this existed, but was not exposed via the
interface.  It could arguably be non-virtual, but since we virtualize
CheckSameOriginURI and other methods so that there could be other security
managers, with different notions of "origin", it seems right to virtualize the
predicate exposed here.

The other option, alluded to by bz, would add a flag to suppress the error
report.  I don't like that on aesthetic and dominant-case caller code bloat
grounds, and the predicate method was already factored, so I didn't add any
method prolog/epilog overhead, only the vtbl extension and NS_IMETHODIMP costs.


/be

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 132277 [details] [diff] [review]
fixed NS_IMETHODIMP gaffe, added comment about return value

jst is welcome to sr= too.  Drivers are looking at this for 1.5, and time is
very short.

/be

Comment 17

[Brendan Eich [:brendan]]

Forgot to say that I checked SecurityCompareURIs and it doesn't muff the return
value in error cases.  I'm in favor of simplifying caller code based on
knowledge of sanity there.  If nsIScriptSecurityManager were frozen and we had
various impls of it from different, possibly distributed owners, I'd worry more.
 As it is, though, Mozilla suffers from way too much redundant nsresult testing.

/be

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 132277 [details] [diff] [review]
fixed NS_IMETHODIMP gaffe, added comment about return value

Ok, sounds good.  r=caillon.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Did I do that?  I trust it's not the default, but I don't remember adding it. 

The old "scripts and windows" ui (circa 0.8 or 0.9) set that pref if you
disabled raising windows... Nowadays it sets a different pref, one that just
makes us silently do nothing instead of throwing.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132277 [details] [diff] [review]
fixed NS_IMETHODIMP gaffe, added comment about return value

sr=bzbarsky, but I also cannot test this at the moment (my only semi-viable
build is tied up in an attempt to fix a crash regression).

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 132277 [details] [diff] [review]
fixed NS_IMETHODIMP gaffe, added comment about return value

bz: that's it -- my profile dates from that era, and I did set that option. 
Thanks, all makes sense now.

Going for 1.5 approval.

/be

Comment 22

[Brendan Eich [:brendan]]

Checked into trunk.

/be

Comment 23

[Christopher Aillon (sabbatical, not receiving bugmail)]

Oops, I forgot that CompareSecurityURIs was called from other things in caps as
a static.  They need to be updated as well to call this stuff on an instance of
the secman.  nsScriptSecurityManager::GetScriptSecurityManager() will work.

Comment 24

[Brendan Eich [:brendan]]

caillon: right, thanks.  (Pay no attention to intermediate checkins made in
haste to fix redness!).

/be

Comment 25

[Brendan Eich [:brendan]]

Attachment: 1.5 branch version of patch

Comment 26

[Brendan Eich [:brendan]]

Checked into the 1.5 branch too.

/be