380474 - XSS by loading a target site in the middle of calling addEventListener()

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[moz_bug_r_a4]

A page load initiated in a function can be processed in the middle of that
function.

  function f() {
    var d = w.document;
    w.location = url;
    w.document == d; // --> true
    alert(1); // or sync XMLHttpRequest, etc.
    w.document == d; // --> false
  }

By using this behavior, an attacker can add an event listener to a target site. 
While addEventListener() is being called on a window, at the time of
conversions of arguments, it's possible to load a target site in the window
that addEventListener() is being called on.  When this happens, execution of
addEventListener() continues on the target site.

a) w.addEventListener(type, listener, ...);

In this case, type.toString() and listener.QueryInterface() are called in
nsEventReceiverSH::AddEventListenerHelper().

b) Components.lookupMethod(w, "addEventListener")(type, listener, ...);

In this case, type.toString() and listener.QueryInterface() are called in
XPCWrappedNative::CallMethod().

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - a + QueryInterface()

This works on trunk, fx2.0.0.x and fx1.5.0.x.

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - b + toString()

This works on trunk, fx2.0.0.x and fx1.5.0.x.

Comment 3

[Johnny Stenback (:jst)]

Attachment: Fix.

This fixes both testcases in this bug by ensuring we use the inner window in the right places. The trick question here now is what other places do we need to do the same thing in...

Comment 4

[Brendan Eich [:brendan]]

The problem could be phrased in terms of types and data flow:

1. Mark those methods that are allowed to run on an outer, which should be a small subset of all window methods, as "$outerok" or some such. In normal builds this macro expands to nothing.

2. Mark all the methods that must affect only an inner window as "$inneronly". We hope this is a small-ish subset too. We should not mark methods that already call OBJ_TO_INNER_OBJECT. Perhaps this marking could be automated: all window methods that don't call OBJ_TO_INNER_OBJECT and are not $outerok are by default $inneronly.

3. Run an Oink-based tool, not sure whether CQual++ could be used off-the-shelf, to find all flows from $outerok to $inneronly.

Taras, what do you think?

/be

Comment 5

[(dormant account)]

I think you cqual(or hacked variant of it) should be able to do this. As usual the hard part is specifying exactly what should be marked and how.
This doesn't need control flow, so it might be the first problem solvable by cqual.

1) Which methods qualify as window methods? Is there a class that contains them? Looks like you want to mark those with $outerok manually, correct?

2) Likewise, how do I obtain a list of those methods?

3) May be able to write a new tool so it does the marking and (maybe) even feeds it to cqual.

Comment 6

[Johnny Stenback (:jst)]

Fix checked in. #2 is the million dollar question, in a way. I'm all for thorwing tools at this problem but it won't be trivial to get a reasonably correct list of what methods are ok to use inner/outer etc. We'll need a new bug on file for that work, I'm closing this bug for now.

Comment 7

[Daniel Veditz [:dveditz]]

Will this patch work for the 1.8 branches? Do we need something else?

Comment 8

[Johnny Stenback (:jst)]

This should work for branches.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 269780 [details] [diff] [review]
Fix.

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers.

Please land ASAP!

Comment 10

[Johnny Stenback (:jst)]

Attachment: Branch version.

This is the same thing as the trunk version except that I can't use the macro OBJ_TO_INNER_OBJECT on the branches due to linking problems from it using internal JS engine functions that are not exported and reachable outside of the JS engine. So this patch expands the macro in the two places where I needed it and uses JS_GET_CLASS() instead of OBJ_GET_CLASS() which pulls in the dependency on the internal function.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 271764 [details] [diff] [review]
Branch version.

moving approval to correct patch.

Comment 12

[Johnny Stenback (:jst)]

Fixed on branches.

Comment 13

[juan becerra [:juanb]]

Does does not manifest itself in Tbird 15012 or 15013 (while going through Tbird 15013 bug verifications).

Comment 14

[juan becerra [:juanb]]

Verified fixed on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5) Gecko/2007071317 Firefox/2.0.0.5