Last Comment Bug 339918 - needsSecurityCheck() incorrect for top-level scripts
: needsSecurityCheck() incorrect for top-level scripts
Status: RESOLVED FIXED
[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)
:
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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 www.squarefree.com (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]
testcase
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.

/be
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.

/be
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 www.squarefree.com
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 for..in 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.

/be
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, jst@mozilla.com) 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]
Updated
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]
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) 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]
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] 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]
Updated

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
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 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.