Last Comment Bug 339918 - needsSecurityCheck() incorrect for top-level scripts
: needsSecurityCheck() incorrect for top-level scripts
[sg:high] xss? requires 340537 (not 1...
: fixed1.8.0.5, fixed1.8.1, regression, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla1.8.1beta1
Assigned To: Blake Kaplan (:mrbkap)
: Andrew Overholt [:overholt]
Depends on: 340537
Blocks: geniter js1.7 343594
  Show dependency treegraph
Reported: 2006-05-31 22:15 PDT by Jesse Ruderman
Modified: 2007-09-03 16:47 PDT (History)
8 users (show)
dveditz: blocking1.7.14-
dveditz: blocking‑aviary1.0.9-
mtschrep: blocking1.8.1+
dveditz: blocking1.8.0.5+
dveditz: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (307 bytes, text/html)
2006-05-31 22:16 PDT, Jesse Ruderman
no flags Details
testcase 2 (__iterator__ on document) (275 bytes, text/html)
2006-05-31 23:49 PDT, Jesse Ruderman
no flags Details
testcase 3 (__iterator__ on document) (449 bytes, text/html)
2006-06-01 00:17 PDT, Jesse Ruderman
no flags Details
companion to testcase 3, also hosted on (506 bytes, text/html)
2006-06-01 00:20 PDT, Jesse Ruderman
no flags Details
Fix for the cross-origin testcase (2.95 KB, patch)
2006-06-02 16:11 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Better fix (4.93 KB, patch)
2006-06-02 17:54 PDT, Blake Kaplan (:mrbkap)
jst: review+
Details | Diff | Splinter Review
Updated (4.26 KB, patch)
2006-06-02 19:41 PDT, Blake Kaplan (:mrbkap)
mrbkap: review+
brendan: superreview+
dveditz: approval1.8.0.5+
mtschrep: approval1.8.1+
Details | Diff | Splinter Review

Description Jesse Ruderman 2006-05-31 22:15:06 PDT
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 2006-05-31 22:16:49 PDT
Created attachment 224017 [details]
Comment 2 Brendan Eich [:brendan] 2006-05-31 23:14:44 PDT
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.

Comment 3 Jesse Ruderman 2006-05-31 23:30:31 PDT
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] 2006-05-31 23:39:37 PDT
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.

Comment 5 Jesse Ruderman 2006-05-31 23:49:42 PDT
Created attachment 224022 [details]
testcase 2 (__iterator__ on document)
Comment 6 Jesse Ruderman 2006-06-01 00:17:06 PDT
Created attachment 224028 [details]
testcase 3 (__iterator__ on document)
Comment 7 Jesse Ruderman 2006-06-01 00:19:24 PDT
Hmm, that testcase confuses Firefox's address bar.
Comment 8 Jesse Ruderman 2006-06-01 00:20:01 PDT
Created attachment 224029 [details]
companion to testcase 3, also hosted on
Comment 9 Jesse Ruderman 2006-06-01 00:37:18 PDT
Address bar confusion --> bug 339928.
Comment 10 Jesse Ruderman 2006-06-01 01:53:26 PDT
For the cross-domain case to work, the script trying to do the has to be top-level.  Weird.
Comment 11 Blake Kaplan (:mrbkap) 2006-06-02 16:11:21 PDT
Created attachment 224257 [details] [diff] [review]
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] 2006-06-02 17:50:36 PDT
DOMI behavior is not a bug.  DOMI sets xpcnativewrappers=no, use it carefully.

Comment 13 Blake Kaplan (:mrbkap) 2006-06-02 17:54:23 PDT
Created attachment 224271 [details] [diff] [review]
Better fix

I missed needsSecurityCheck the first time around.
Comment 14 Johnny Stenback (:jst, 2006-06-02 19:26:24 PDT
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) 2006-06-02 19:41:14 PDT
Created attachment 224284 [details] [diff] [review]
Comment 16 Jesse Ruderman 2006-06-02 20:43:21 PDT
> 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] 2006-06-03 15:23:48 PDT
Comment on attachment 224284 [details] [diff] [review]

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.

Comment 18 Blake Kaplan (:mrbkap) 2006-06-05 11:02:27 PDT
Fix checked into trunk.
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2006-06-06 09:05:48 PDT
This caused a significant Tdhtml regression.  See bug 340537.
Comment 20 Blake Kaplan (:mrbkap) 2006-07-05 14:29:00 PDT
This blocks js1.7 and I think it allows top-level scripts to access properties cross-domain.
Comment 21 Blake Kaplan (:mrbkap) 2006-07-05 14:30:31 PDT
Comment on attachment 224284 [details] [diff] [review]

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] 2006-07-05 18:48:33 PDT
May fix bug 339918
Comment 23 Blake Kaplan (:mrbkap) 2006-07-06 12:49:26 PDT
Fix checked into the 1.8 branch.
Comment 24 Daniel Veditz [:dveditz] 2006-07-06 13:29:02 PDT
Comment on attachment 224284 [details] [diff] [review]

approved for 1.8.0 branch, a=dveditz for drivers
Comment 25 Blake Kaplan (:mrbkap) 2006-07-06 15:14:32 PDT
Fix checked into the 1.8.0 branch.
Comment 26 Daniel Veditz [:dveditz] 2006-07-11 13:22:06 PDT
(In reply to comment #22)
> May fix bug 339918

meant bug 343594

Comment 27 Daniel Veditz [:dveditz] 2006-07-21 00:30:05 PDT
This bug does not affect Firefox 1.0.x or Mozilla 1.7.x
Comment 28 Bob Clary [:bc:] 2006-08-22 10:20:34 PDT
ff2b2 windows, linux alert custom iterator called
not sure I am testing this right.

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

Comment 29 chris hofmann 2007-04-24 15:29:22 PDT
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...

Note You need to log in before you can comment on or make changes to this bug.