246923 - links in nested frame fail, give a false NS_ERROR_DOM_PROP_ACCESS_DENIED

Status: RESOLVED FIXED

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

Description

[Magnus Melin [:mkmelin]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040608 MultiZilla/1.6.3.0e
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040608 MultiZilla/1.6.3.0e

On the given URL nested frames are used. (One frame contains another framed
page.) If you open the the inner frame URL http://www.hut.fi/~mkmelin/vn/
directly the links will work.

Reproducible: Always
Steps to Reproduce:
On the given URL, click on any of the left column navigation links. They won't
open, and JavaScript console outputs

Error: uncaught exception: [Exception... "Access to property denied"  code:
"1010" nsresult: "0x805303f2 (NS_ERROR_DOM_PROP_ACCESS_DENIED)"  location:
"http://www.hut.fi/~mkmelin/vn/menu.html Line: 39"]

Actual Results:  
The links should work. 

Expected Results:  
Links whould work, as they do in moz (up to) 1.7rc3.

This problem is present in firefox 0.9, also in trunk builds 20040615 of both
the suite and firefox. 

Yes, I know about same origin policy, but this should not be an issue here.

Comment 1

[Magnus Melin [:mkmelin]]

This is also present in 1.7 final, but not in 1.7rc3, tested both linux and
windows. I think this is pretty bad, as not being able to follow links severely
cripples the browser.

Comment 2

[Johnny Stenback (:jst)]

This is not as designed, this happened due to a problem with the last-minute fix
for bug 246923, the change was too restrictive. I'm working on a fix for this.

A workaround for this particular problem might be to redirect
http://www.helsinki.fi/jarj/vasa/ to http://www.hut.fi/~mkmelin/vn/. That way,
the whole frameset is from the same origin, and your links would work again.

Comment 3

[Johnny Stenback (:jst)]

Attachment: Trunk fix.

This makes the criteria for whether we permit a load from a link click or
targetted form submission the same as if the load comes from JS, something that
should've been done in the initial change that caused this bug, but didn't
happen due to time pressure.

This also fixes the bug that caused this particular bug, which was that the
code that compared if the running JS came from a docshell within the targets
root docshell didn't get the root from the docshell where the JS came from, and
compared the actual docshell that the JS was running in to the root of the
target, and not the root of the docshell where the JS was running.

This is somewhat scary in that this code now sometimes passes non-nsIPrincipal
objects as the owner of a URI load. I've digged through the code and tried to
make sure that the owner is reset to the principal where needed, but it's still
not pretty. But hey, this is docshell code, so... :-)

Comment 4

[Johnny Stenback (:jst)]

For the record, this was caused by bug 246448.

Comment 5

[Darin Fisher]

Comment on attachment 151410 [details] [diff] [review]
Trunk fix.

This patch makes sense to me.  It's a hefty change to the docshell code, but it
feels like the right thing to do.  The biggest risk I guess is what code
outside of docshell will do with the nsIChannel::owner attribute.


>Index: docshell/base/nsDocShell.cpp

>+        nsIDocShellTreeItem *tmp = item;
>+        item->GetSameTypeParent(&tmp);
>+        item.swap(tmp);
>+    } while (item);

nit: looks to me as though you do not need to initialize |tmp|
since GetSameTypeParent sets it to NULL on entry to the method.


>+nsDocShell::GetCurrentDocumentPrincipal(nsIPrincipal** aPrincipal)
...
>+        docViewer->GetDocument(getter_AddRefs(document));

nit: what happens if this returns a null document?  (looks like
the old code did not have any null checks or |rv| checks either.)


>+    *aPrincipal = document->GetPrincipal();
>+    NS_IF_ADDREF(*aPrincipal);
> }

looks like we could crash.  maybe add an ASSERTION if this should
never happen.


sr=darin

Comment 6

[Johnny Stenback (:jst)]

Attachment: Bring the old frame origin check back to life, and improve it some. And make the new CheckLoadingPermissions() only deal with JS loads.

Dveditz and I found out that there's already code in the docshell that checks
wether a targetted load should be permitted or not, that change went in as the
fix for bug 13871 in late 2001, only to be disabled by some code reordering 6
months later (bug 65777).

This patch brings the original check back to life, but that check didn't fix
the case where JS tried to change the location of a frame within a frameset it
didn't own. This fixes that as well.

The changes to nsWebShell are merely a backout of the initial changes that
caused this bug, and now that we figured out a better way to prevent (or
redirect to a new window, rather) targetted links and form submissions, the JS
part got much simpler, and no more scary owner-is-not-always-an-nsIPrincipal
changes.

Comment 7

[Johnny Stenback (:jst)]

I forgot to add the null check for document in
nsDocShell::GetCurrentDocumentOwner() that darin mentioned. I've added that
locally, so that's now crash safe.

Comment 8

[Daniel Veditz [:dveditz]]

nominating for the aviary branch

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 151493 [details] [diff] [review]
Bring the old frame origin check back to life, and improve it some. And make the new CheckLoadingPermissions() only deal with JS loads.

>Index: docshell/base/nsDocShell.cpp
>+                    nsIDocShellTreeItem *t = nsnull;
>+                    tmp->GetSameTypeParent(&t);
>+                    tmp.swap(t);

This leaks, t should be a nsCOMPtr<>

>+        nsIDocShellTreeItem *tmp = item;
>+        item->GetSameTypeParent(&tmp);
>+        item.swap(tmp);

Same here. (Not sure why you bother to initialize *tmp.)

>+    // The caller is not from the same origin as this item, or any if
>+    // this items ancestors. Only permit loading content if both are
>+    // part of the same window, assuming we can find the window of the
>+    // caller.
> 
>     nsCOMPtr<nsIJSContextStack> stack =
>         do_GetService("@mozilla.org/js/xpc/ContextStack;1");
>     if (!stack) {
>         return rv;
>     }

What's rv at this point? I guess it's some sort of error from the call to
CheckSameOriginPrincipal (else you'd've returned already) but you should
be explicit that you're blocking the load with an error. Otherwise it risks
someone adding code that sets rv in between.

Or use the rv-setting form of do_GetService() and check rv instead of stack.

>     JSContext *cx = nsnull;
>     stack->Peek(&cx);
> 
>     if (!cx) {
>         // No caller docshell reachable, disallow load.
> 
>         return rv;

ditto here (sorry I didn't catch these in bug 246448).

r=dveditz with those changes.

Comment 10

[Darin Fisher]

Comment on attachment 151493 [details] [diff] [review]
Bring the old frame origin check back to life, and improve it some. And make the new CheckLoadingPermissions() only deal with JS loads.

sr=darin with dveditz's fixes.

Comment 11

[Johnny Stenback (:jst)]

Fix checked in on the trunk.

Comment 12

[Johnny Stenback (:jst)]

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

Comment 13

[Johnny Stenback (:jst)]

Attachment: Same thing for the aviary/1.7 branch.

Comment 14

[Hermann Schwab]

I assume this bugfix caused regression bug 248753 on Mozilla, 2004062309
working, 2004062323 failing, Firefox 2004062408 failing.

Instead of opening a popup and loading an image into it, an empty popup is
created, a new window is created, and the image loaded in the new window.
testcase: http://bugzilla.mozilla.org/attachment.cgi?id=151784&action=view

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 151493 [details] [diff] [review]
Bring the old frame origin check back to life, and improve it some. And make the new CheckLoadingPermissions() only deal with JS loads.

a=dveditz for 1.7.1

Comment 16

[Johnny Stenback (:jst)]

Fixed on the trunk, 1.7 branch, and aviary branch.

Comment 17

[Magnus Melin [:mkmelin]]

Changing the URL, since I put in a workaround to break out of inner frames for
June builds in cases "debug" parameter is not given. With debug parameter it
functions as before.

Comment 18

[Luis Alberto]

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

Comment 19

[Stig Nygaard]

As far as I remember this was fixed on the 1.7.x branch already *before* the
1.7.1 release, but the fix doesn't seems to be included in either 1.7.1, 1.7.2
or 1.7.3 (or Firefox 1.0PR1). Why? I've been running nightly builds from the
1.7.x branch the latest months because this bug prevent me from using some sites
I visit regulary. I haven't found any problems with these branch-nightlies, but
are there some major regression known with this fix which prevents it from being
included in the actually release-versions?
BTW. I know 1.7.1 and 1.7.2 was rushed out very quickly due to security bugs, so
maybe the included fixes in this builds were choosen very conservative, but I've
got the impression thar 1.7.3 (and Firefox 1.0PR1) were planned in a long time.

Anyway, this bug is marked fixed, but ain't fixed in any official release even
though there has been quite a few releases since the bug was fixed (and even
though it has been fixed in the 1.7.x branch nightlies in a long time).

Comment 20

[Magnus Melin [:mkmelin]]

Works fine in Firefox 1.0PR1. In 1.7.1-3 there were only security fixes.

Comment 21

[Stig Nygaard]

#20, Magnus

You're right, it DOES actually works in Firefox PR1. I would have sworn it
didn't last time I tried. Sorry for the spam, and thanks for the quick respons:-)

(BTW, shouldn't the checkins for 1.7.3 and Firefox 1.0PR be pretty much
synchronized? Or it's only the 1.7.x nightlies which are?)

Comment 22

[Magnus Melin [:mkmelin]]

Firefox 1.0PR is made of the aviary branch. Things fixed on the aviary branch
don't automatically get fixed on the 1.7 branch (and the other way around). 

Comment 23

[Mike Shaver (:shaver emeritus)]

(Perhaps confusingly, 1.7.[123] weren't cut from the 1_7_BRANCH, but instead
from the 1.7(.0) release minibranch, based only on the security fixes.  An
upcoming 1.7.x release will feature all these good bits, and align very tightly
with firefox's 1.0.)

Comment 24

[Michael Lefevre]

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

Comment 25

[Ion CIONCA]

(In reply to comment #19)
...
> Anyway, this bug is marked fixed, but ain't fixed in any official release even
> though there has been quite a few releases since the bug was fixed (and even
> though it has been fixed in the 1.7.x branch nightlies in a long time).

Any news concerning this "bug" ? fixed or not yet ? 

The Swiss Post refuses to support Mozilla and Firefox for their on-line credit
card paiment because of this "bug"

Thnx

Ion CIONCA
__________
Ion CIONCA, web & database development
DIT/KIS (Knowledge Information Services), EPFL, 1015 Lausanne, Switzerland
phone: [phone], fax: [phone]
mailto:[e-mail], http://kis.epfl.ch/page44412.html

Comment 26

[Magnus Melin [:mkmelin]]

It's fixed. The fix is in Moz 1.7.5 or firefox 1.0 and up. 

Comment 27

[Fabian]

In reply to comments #25 #26:

it ist still not possible to use the Swiss Post on-line credit card payment.
Firefox browser (Debian Linux) 1.0.4 can't navigate back to main window after 
having finished payment in popup window.

They say it does not work because of this bug since firefox 0.9 and mozilla 1.7:
<http://bugzilla.mozilla.org/show_bug.cgi?id=246923>

So I tried firefox 0.8 und mozilla 1.7: does not work either.

Regards,

Fabian Lienert
[e-mail]