339918 - needsSecurityCheck() incorrect for top-level scripts

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Steps to reproduce:
1. Load the testcase.
2. Open DOM Inspector.
3. Click the HTML element in the node tree pane.
4. Select "JavaScript Object" mode in the right pane.

Result: "Custom __iterator__ called" alert appears, and the normal properties of the object aren't listed in DOM Inspector.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Brendan Eich [:brendan]]

This is a bug in DOM inspector -- it arguably should isolate itself from the page's use of new features such as the iteration protocol.

I'm not sure why you filed this against the JS engine.

/be

Comment 3

[Jesse Ruderman]

The same thing happens if a document in one domain tries to iterate over the properties of a document in another domain...

Comment 4

[Brendan Eich [:brendan]]

How does that cross-site scripting exploit work?  Can you get at another window's document object in order to iterate it?  It ought to be off limits by the same origin policy.  A testcase would be good, since the DOM inspector case doesn't demonstrate this claim.

/be

Comment 5

[Jesse Ruderman]

Attachment: testcase 2 (__iterator__ on document)

Comment 6

[Jesse Ruderman]

Attachment: testcase 3 (__iterator__ on document)

Comment 7

[Jesse Ruderman]

Hmm, that testcase confuses Firefox's address bar.

Comment 8

[Jesse Ruderman]

Attachment: companion to testcase 3, also hosted on www.squarefree.com

Comment 9

[Jesse Ruderman]

Address bar confusion --> bug 339928.

Comment 10

[Jesse Ruderman]

For the cross-domain case to work, the script trying to do the for..in has to be top-level.  Weird.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix for the cross-origin testcase

I haven't tested the DOMI testcase yet, but this patch fixes the cross-origin problem by doing security checks even when the only script running on cx is the top level script with no function object.

Comment 12

[Brendan Eich [:brendan]]

DOMI behavior is not a bug.  DOMI sets xpcnativewrappers=no, use it carefully.

/be

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better fix

I missed needsSecurityCheck the first time around.

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 224271 [details] [diff] [review]
Better fix

- In needsSecurityCheck():

   cached_win_cx = cx;
   cached_win_wrapper = wrapper;
-  cached_win_needs_check = PR_TRUE;

There's a few early returns after this code that relies on the static cached_win_needs_check being set to true here. So what you probably want it so just leave this line in, and add the line where you're setting it to false.
   
r=jst with that changed.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated

Comment 16

[Jesse Ruderman]

> DOMI behavior is not a bug.  DOMI sets xpcnativewrappers=no, use it carefully.

Don't use DOM Inspector on untrusted pages?  Eww.

Comment 17

[Brendan Eich [:brendan]]

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

sr=me, thanks for fixing. Make this block the js1.7 bug please, and collect any other blockers so we can get their patches landed on the 1.8 branch in due course.

/be

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 19

[Boris Zbarsky [:bzbarsky]]

This caused a significant Tdhtml regression.  See bug 340537.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

This blocks js1.7 and I think it allows top-level scripts to access properties cross-domain.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

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

If we take this, we're also going to want the patch for bug 340537 to fix the perf regression this introduces.

Comment 22

[Daniel Veditz [:dveditz]]

May fix bug 339918

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branch.

Comment 24

[Daniel Veditz [:dveditz]]

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

approved for 1.8.0 branch, a=dveditz for drivers

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8.0 branch.

Comment 26

[Daniel Veditz [:dveditz]]

(In reply to comment #22)
> May fix bug 339918

meant bug 343594

Comment 27

[Daniel Veditz [:dveditz]]

This bug does not affect Firefox 1.0.x or Mozilla 1.7.x

Comment 28

[Bob Clary [:bc] (inactive)]

https://bugzilla.mozilla.org/attachment.cgi?id=224017
ff2b2 windows, linux alert custom iterator called

https://bugzilla.mozilla.org/attachment.cgi?id=224028&action=view
not sure I am testing this right.

Jesse, can you verify this is fixed on firefox beta 2?

Comment 29

[chris hofmann]

pvnick is doing a bit of research on XSS and also gathering up bugs with security related test cases to help add to the regression/certification test suites.  adding him to the cc list in these...