needsSecurityCheck() incorrect for top-level scripts

RESOLVED FIXED in mozilla1.8.1beta1

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: mrbkap)

Tracking

(4 keywords)

Trunk
mozilla1.8.1beta1
fixed1.8.0.5, fixed1.8.1, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.14 -
blocking-aviary1.0.9 -
blocking1.8.1 +
blocking1.8.0.5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high] xss? requires 340537 (not 1.7/aviary))

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 224017 [details]
testcase
(Reporter)

Updated

11 years ago
Blocks: 326466
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
(Reporter)

Comment 3

11 years ago
The same thing happens if a document in one domain tries to iterate over the properties of a document in another domain...
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
(Reporter)

Comment 5

11 years ago
Created attachment 224022 [details]
testcase 2 (__iterator__ on document)
(Reporter)

Comment 6

11 years ago
Created attachment 224028 [details]
testcase 3 (__iterator__ on document)
Attachment #224022 - Attachment is obsolete: true
(Reporter)

Comment 7

11 years ago
Hmm, that testcase confuses Firefox's address bar.
(Reporter)

Comment 8

11 years ago
Created attachment 224029 [details]
companion to testcase 3, also hosted on www.squarefree.com
(Reporter)

Comment 9

11 years ago
Address bar confusion --> bug 339928.
(Reporter)

Comment 10

11 years ago
For the cross-domain case to work, the script trying to do the for..in has to be top-level.  Weird.
(Assignee)

Comment 11

11 years ago
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.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #224257 - Flags: superreview?(brendan)
Attachment #224257 - Flags: review?(jst)
(Assignee)

Updated

11 years ago
Component: JavaScript Engine → DOM
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
DOMI behavior is not a bug.  DOMI sets xpcnativewrappers=no, use it carefully.

/be
(Assignee)

Comment 13

11 years ago
Created attachment 224271 [details] [diff] [review]
Better fix

I missed needsSecurityCheck the first time around.
Attachment #224271 - Flags: superreview?(brendan)
Attachment #224271 - Flags: review?(jst)
(Assignee)

Updated

11 years ago
Attachment #224257 - Attachment is obsolete: true
Attachment #224257 - Flags: superreview?(brendan)
Attachment #224257 - Flags: review?(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.
Attachment #224271 - Flags: review?(jst) → review+
(Assignee)

Comment 15

11 years ago
Created attachment 224284 [details] [diff] [review]
Updated
Attachment #224271 - Attachment is obsolete: true
Attachment #224284 - Flags: superreview?(brendan)
Attachment #224284 - Flags: review+
Attachment #224271 - Flags: superreview?(brendan)
(Reporter)

Comment 16

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

Don't use DOM Inspector on untrusted pages?  Eww.
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
Attachment #224284 - Flags: superreview?(brendan) → superreview+
(Assignee)

Comment 18

11 years ago
Fix checked into trunk.
Blocks: 336373
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Depends on: 340537
This caused a significant Tdhtml regression.  See bug 340537.
(Assignee)

Comment 20

11 years ago
This blocks js1.7 and I think it allows top-level scripts to access properties cross-domain.
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
(Assignee)

Comment 21

11 years ago
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.
Attachment #224284 - Flags: approval1.8.1?
Attachment #224284 - Flags: approval1.8.0.6?
May fix bug 339918
Flags: blocking1.8.0.5?
Whiteboard: requires 340537
Summary: __iterator__ defined by web page is called by DOM Inspector → needsSecurityCheck() incorrect for top-level scripts
Whiteboard: requires 340537 → [sg:high] xss? requires 340537
Blocks: 343594

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
Target Milestone: mozilla1.9alpha → mozilla1.8.1beta1

Updated

11 years ago
Attachment #224284 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 23

11 years ago
Fix checked into the 1.8 branch.
Keywords: fixed1.8.1
Comment on attachment 224284 [details] [diff] [review]
Updated

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #224284 - Flags: approval1.8.0.6? → approval1.8.0.5+
Flags: blocking1.8.0.6?
Flags: blocking1.8.0.5?
Flags: blocking1.8.0.5+
(Assignee)

Comment 25

11 years ago
Fix checked into the 1.8.0 branch.
Keywords: fixed1.8.0.5
(In reply to comment #22)
> May fix bug 339918

meant bug 343594

This bug does not affect Firefox 1.0.x or Mozilla 1.7.x
Flags: blocking1.7.14-
Flags: blocking-aviary1.0.9-
Keywords: regression
Whiteboard: [sg:high] xss? requires 340537 → [sg:high] xss? requires 340537 (not 1.7/aviary)

Comment 28

11 years ago
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

10 years ago
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...
Group: security
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.