cross-domain variable detection with scopes (eval, with)

RESOLVED FIXED in mozilla1.4final

Status

()

Core
DOM: Core & HTML
P1
normal
RESOLVED FIXED
15 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: jst)

Tracking

({csectype-sop, fixed1.4})

Trunk
mozilla1.4final
csectype-sop, fixed1.4
Points:
---
Bug Flags:
blocking1.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:mustfix] security [HAVE FIX])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

15 years ago
A script can determine whether a variable is defined in another domain using
scope tricks.  The attacker cannot find the value of the variable.  The impact
is the same as in bug 59208: a script can find out whether you're logged into
nytimes.com and possibly other sites.

1. eval('amz_js_PopWin',frame) 
- "Permission denied" error if amz_js_PopWin exists.
- Uncatchable "undefined" error if amz_js_PopWin does not exist.  Note that this
method never looks in the attacker's context.  (Why is the error uncatchable?)

2. with(frame)amz_js_PopWin
- "Permission denied" error if amz_js_PopWin exists.
- Catchable does-not-exist error, or the value in the attacker's context, if
defined in the attacker's domain.
(Reporter)

Comment 1

15 years ago
Created attachment 91752 [details]
demo: can tell whether another window is amazon.com or not
Please define uncatchable, show the try/catch code that fails.  Try it in the
xpcshell and js shell too.

/be

Comment 3

15 years ago
Here is a synopsis of Jesse's testcase, with his comments:

/*
 * Bug: possible to tell whether a function/variable is defined
 * in another window using eval(expr,frame) or with(frame).
 *
 * In the example, amz_js_PopWin is defined in amazon.com,
 * but foobar is not.
 */

frame = window.open('http://www.amazon.com/');
foobar=8;

// OK
try{alert(frame.amz_js_PopWin)} catch(e){alert(e)}
try{alert(frame.foobar)} catch(e){alert(e)}

// OK
try{alert(eval('frame.amz_js_PopWin'))} catch(e){alert(e)}
try{alert(eval('frame.foobar'))} catch(e){alert(e)} 

// OK, can't call frame.eval
try{alert(frame.eval('amz_js_PopWin'))} catch(e){alert(e)}
try{alert(frame.eval('foobar'))} catch(e){alert(e)}

// "Permission denied" vs. uncatchable (!?) "undefined" error: not good
try{alert(eval('amz_js_PopWin',frame))} catch(e){alert(e)}
try{alert(eval('foobar',frame))} catch(e){alert(e)}  

// "Permission denied" vs. "undefined" error: not good.
try{alert(eval('with(frame)amz_js_PopWin'))} catch(e){alert(e)}
try{alert(eval('with(frame)foobar'))} catch(e){alert(e)}

Comment 4

15 years ago
Amplifying the "not good" sections above. I should note that Jesse's
testcase allows you to either define |foobar = 8| or |foobar = undefined|.
That comes into play in the last line below:


// "Permission denied" vs. uncatchable (!?) "undefined" error: not good
try{alert(eval('amz_js_PopWin',frame))} catch(e){alert(e)}

---> alerts "Permission denied to get property Window.amz_js_PopWin"

try{alert(eval('foobar',frame))} catch(e){alert(e)}  

---> Get error in JS Console "Error: foobar is not defined"
     (i.e. the error was not caught; no alert comes up)



// "Permission denied" vs. "undefined" error: not good.
try{alert(eval('with(frame)amz_js_PopWin'))} catch(e){alert(e)}

---> alerts "Permission denied to get property Window.amz_js_PopWin"

try{alert(eval('with(frame)foobar'))} catch(e){alert(e)}

if you chose to define |foobar = 8| at the beginning of test:
---> alerts "8" 
if you chose to define |foobar = undefined|:
---> alerts "ReferenceError: foobar is not defined" 
We can't bound the downside of this, it depends on how sites are set up to use
scripting. We could potentially reveal something important.
Whiteboard: [sg:blocker]
With the js shell, I see no catchability problems:

js> try{print(eval('foo',{bar:'hi'}))}catch(e){print('<<<'+e+'>>>')}
<<<ReferenceError: foo is not defined>>>
js> try{with({bar:'hi'})print(foo)}catch(e){print('<<<'+e+'>>>')} 
<<<ReferenceError: foo is not defined>>>
js> 


Someone produce a counterexample, or say that this worksforthem as it does for
me in the js shell.  Is the catchability difference seen only in Mozilla?

Assuming the catchability bug has been fixed, or could be fixed, I think one
possible solution is to make the JS engine call a checkAccess callback before
throwing a tell-tale exception.  I can hack up a patch for that, later today.

/be

Comment 7

15 years ago
> Is the catchability difference seen only in Mozilla?

Yes. 

If I try Brendan's test in the JS shell, the error is successfully caught.
If I try Jesse's tests in Mozilla trunk 2002-10-14, it is not caught.
For anyone following this bug: notice the use in both tests of the syntax

                      eval (str, obj)

That is, we are using the second parameter of eval() to specify
the context in which the eval() should execute. In Brendan's test,
|obj| is {bar:'hi'}. In Jesse's test, |obj| is a new browser window.


STEPS TO REPRODUCE
1. Load Jesse's HTML testcase from Comment #1
2. Click on the button "open amazon.com in a new window"
3. A variable named |frame| in the global scope points to this new window
4. Now try these javascript:URLs

A)
javascript: try {alert(eval(1, frame));} catch(e) {alert('<<<'+e+'>>>');}
OUTPUT: alertbox: '1'

B)
javascript: try {alert(eval('window', frame));} catch(e) {alert('<<<'+e+'>>>');}
OUTPUT: alertbox: '[object Window]'

C)
javascript: try {eval('foo', frame);} catch(e) {alert('<<<'+e+'>>>');}
OUTPUT: NO ALERTBOX !!! ERROR IN JS CONSOLE INSTEAD:

        Error: foo is not defined
        Source File: javascript: try {alert(eval('foo', frame));} catch(e)                                                     
                     {alert('<<<'+e+'>>>');}
        Line: 1

D)
javascript: try {alert(eval('foo', window));} catch(e) {alert('<<<'+e+'>>>');}
ALERTBOX: '<<<ReferenceError: foo is not defined  >>>'


The problem is in C): in the context of the new window,
the error is not successfully caught 

By contrast, in D): when we provide |window| as the context,
i.e. the original window, the error IS caught.

Comment 8

15 years ago
Typo above: C) should have the same call to alert() as in D).
The result is the same as I've reported: no alertbox comes up.
Reassigning to brendan, downgrading to mustfix.
Assignee: mstoltz → brendan
Whiteboard: [sg:blocker] → [sg:mustfix]
Fixing TM.

/be
Target Milestone: --- → mozilla1.3beta

Comment 11

15 years ago
close to a patch?
Adding rogerl, he may be interested in this bug as well.

Updated

14 years ago
Status: NEW → ASSIGNED
Target Milestone: mozilla1.3beta → mozilla1.4beta
WORKSFORME, mostly.  Someone please retest and say what you see.

I see amz_js_PopWin is not defined for the left 2nd from bottom button, in an
alert.  I see foobar is not defined in an alert for the right 2nd from bottom. 
I see frame has no properties for the bottom row of two buttons, when I click.

/be
Assignee: brendan → mstoltz
Status: ASSIGNED → NEW
(Reporter)

Comment 14

14 years ago
Brendan, it sounds like you're not clicking the "Open amazon.com in a new
window" button before clicking the other buttons in the testcase.

Comment 15

14 years ago
trying for 1.4 final
Flags: blocking1.4+
Target Milestone: mozilla1.4beta → mozilla1.4final
Dammit, jruderman's right -- I thought frame meant a frame-in-frameset, or an
iframe.  Duh.

There are two problems here:

1.  Somehow, an error is not becoming an exception, but is being reported.
2.  The caps code gives up the name of restricted properties.

Mitch, can you work on 2?  I'll work on 1.  If you want to file a new bug, feel
free -- or let's work here.

/be
I don't quite understand - what do you think the correct behavior should be?
Right now, a script can tell the difference between an undefined property and a
forbidden property that is defined because the error messages are different. I
suppose we should report "forbidden" in both cases for a cross-host access, but
that change won't be in caps - if the property is undefined I don't think we
call caps at all.
(Reporter)

Comment 18

14 years ago
> Right now, a script can tell the difference between an undefined property and 
> a forbidden property that is defined because the error messages are different.

Right, that's the problem.  But it only happens if I play with scopes using
"with" or "eval".  Here's a transcript of using http://www.squarefree.com/shell/
in Firebird 05/22, with the spacing changed:

x=window.open("http://localhost/");
[object Window]

x.submitGoogle   // defined
Permission denied to get property Window.submitGoogle
x.d              // not defined
Permission denied to get property Window.d

with(x) submitGoogle
Permission denied to get property Window.submitGoogle
with(x) d
ReferenceError: d is not defined

eval("submitGoogle", x)
Permission denied to get property Window.submitGoogle
eval("d", x)
["d is not defined" appears in the JavaScript console, not caught by shell]

For both cases (with and eval), I think "Permission denied to add (Window|x) to
scope chain" would be the best behavior.
I don't think the problem is in caps. Assigning back to Brendan for more
analysis. Brendan, please let me know if I need to do anything from my end.
Assignee: mstoltz → brendan
re: comment 18: the problem is not "failure to add some object to the scope
chain" -- the scope chain is determined by ECMA rules, independent of access
control policies.  The problem is the uncatchable exception, and any information
leaked about whether the variable exists or not.  Accepting bug, sorry I was
slow on the uptake here.

/be
Status: NEW → ASSIGNED
Priority: -- → P1
(Reporter)

Comment 21

14 years ago
One way to stop the information leak would be to make with(x) throw an exception
if x is in another domain.  That's why I suggested "Permission denied to add
(Window|x) to scope chain".  It sounds like you have another way to stop the
information leak; that's fine with me.
Created attachment 124604 [details] [diff] [review]
fix for first problem (uncatchable error)

This fixes the uncatchable error, restoring transparency to lazy standard class
initialization via window-level resolve hooks.	The trick is to run such lazy
standard class resolve code (JS_ResolveStandardClass) on the window's context,
not on the caller's context (which may have a different origin).  There's no
danger in so doing, because if we used eager standard class init, we'd do it on
the window's own context at window creation/re-init time.

/be

Updated

14 years ago
Attachment #124604 - Flags: superreview?(heikki)
Attachment #124604 - Flags: review?(jst)

Updated

14 years ago
Component: Security: CAPS → DOM Level 0
OS: Windows 2000 → All
Hardware: PC → All
(Reporter)

Updated

14 years ago
Whiteboard: [sg:mustfix] → [sg:mustfix] security
(Assignee)

Comment 23

14 years ago
Taking bug (per discussion with Brendan) to continue with the remaining part.
Assignee: brendan → jst
Status: ASSIGNED → NEW
Comment on attachment 124604 [details] [diff] [review]
fix for first problem (uncatchable error)

sr=heikki

The only thing I am wondering about is that the resolve now happens inside |if
(JSVAL_IS_STRING(id))|. If jst says that it is ok then I am ok with it.
Attachment #124604 - Flags: superreview?(heikki) → superreview+
Comment on attachment 124604 [details] [diff] [review]
fix for first problem (uncatchable error)

Patch is ok, but jst's take on it will be the money.

/be
Attachment #124604 - Attachment is obsolete: true
Attachment #124604 - Flags: superreview+
Attachment #124604 - Flags: review?(jst)
(Assignee)

Comment 26

14 years ago
Created attachment 124611 [details] [diff] [review]
In addition to the above, do security checks in the  resolve hooks for window and document
(Assignee)

Comment 27

14 years ago
Created attachment 124612 [details] [diff] [review]
diff -w of the above.
(Assignee)

Comment 28

14 years ago
Comment on attachment 124611 [details] [diff] [review]
In addition to the above, do security checks in the  resolve hooks for window and document

Um, that won't do. This'll prevent cross-origin access to window.foo where foo
is the name of a child-frame. New patch coming up.
Attachment #124611 - Attachment is obsolete: true
Comment on attachment 124611 [details] [diff] [review]
In addition to the above, do security checks in the  resolve hooks for window and document

Nit: fix the aweful typo.

Issue: do you want to do the security check for all ids, including int (index)
ones?  I think so.  Don't we have document[0], document[1], etc. referring to
DOM level 0 things?  Or if not, couldn't the user set those to secret
properties that should be locked up by the same-origin model?

/be
(Assignee)

Comment 30

14 years ago
Created attachment 124613 [details] [diff] [review]
Prevent checking for property existance across origins.

This puts the security checks in the resolve hooks to after we've dealt with
all the properties that should be accessable across origins.
(Assignee)

Updated

14 years ago
Attachment #124612 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #124613 - Flags: superreview?(brendan)
Attachment #124613 - Flags: review?(mstoltz)
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Whiteboard: [sg:mustfix] security → [sg:mustfix] security [HAVE FIX]
Comment on attachment 124613 [details] [diff] [review]
Prevent checking for property existance across origins.

>     if (!(flags & JSRESOLVE_ASSIGNING)) {
>-      // If we're resolving for assignment it's not worth calling
>-      // GlobalResolve()
>+      // We're resolving for assignment. It's not worth calling

First sentence should say "We're *not* resolving for assignment" (emphasis
mine).

>+
>+      // Do a security check when resolving upto-this-point unknown

Nit: need another hyphen in "up-to-this-point" -- or, preferably, "heretofore".

>+      // string properties on window objects to prevent detection of a
>+      // property's existance across origins.
>+      rv =
>+        doCheckPropertyAccess(cx, obj, id, wrapper,
>+                              nsIXPCSecurityManager::ACCESS_GET_PROPERTY,
>+                              PR_TRUE);

Is this only for the GET case, or do we need to test (flags &
JSRESOLVE_ASSIGNING) to select ACCESS_SET_PROPERTY?  Sorry, I'm not reading
beyond the context.  But if we need only check GET here because this code is
predicated on !(flags & JSRESOLVE_ASSIGNING), don't we need another
doCheckPropertyAccess call for the SET case?

>     return nsEventRecieverSH::NewResolve(wrapper, cx, obj, id, flags, objp,
>                                          _retval);

timeless may fix this, but he'd thank you if you could fix Receiver's
misspelling -- not sure when, or how many files would be touched, but I had to
mention it.

>+  // Do a security check when resolving upto-this-point unknown string

See above, "heretofore".

sr=brendan@mozilla.org with the above nit-picked or otherwise addressed.

/be
Attachment #124613 - Flags: superreview?(brendan) → superreview+
(Assignee)

Comment 32

14 years ago
> >+      // string properties on window objects to prevent detection of a
> >+      // property's existance across origins.
> >+      rv =
> >+        doCheckPropertyAccess(cx, obj, id, wrapper,
> >+                              nsIXPCSecurityManager::ACCESS_GET_PROPERTY,
> >+                              PR_TRUE);
> 
> Is this only for the GET case, or do we need to test (flags &
> JSRESOLVE_ASSIGNING) to select ACCESS_SET_PROPERTY?  Sorry, I'm not reading
> beyond the context.  But if we need only check GET here because this code is
> predicated on !(flags & JSRESOLVE_ASSIGNING), don't we need another
> doCheckPropertyAccess call for the SET case?

This is only for the GET case, and we don't need this security check in the SET
case since we'll security check in the ::SetProperty() hook in the SET case. I
added a comment about this.

Fixed the nits.
Comment on attachment 124613 [details] [diff] [review]
Prevent checking for property existance across origins.

I looked this over with jst and surprisingly I think I am able to r= this after
spending a bit of time reading up on domci/js code and asking him a bunch of
questions.  I do have a couple minor nits that I'd like to add though:


>+      if (id == sConstructor_id) {
>+        return ResolveConstructor(cx, obj, objp);
>+      }

Add the comment we discussed about why this code is before the child frame
lookup.


>+      // Do a security check when resolving upto-this-point unknown
>+      // string properties on window objects to prevent detection of a
>+      // property's existance across origins.

Nit: "existence" is misspelled.

r=caillon with those fixed.
Attachment #124613 - Flags: review?(mstoltz) → review+
(Assignee)

Comment 34

14 years ago
Fix checked in on the trunk.
Comment on attachment 124613 [details] [diff] [review]
Prevent checking for property existance across origins.

I figured jst would land this on the branch after a trunk shake-out, but wanted
to mark drivers' approval on the patch.

/be
Attachment #124613 - Flags: approval1.4+
(Assignee)

Comment 36

14 years ago
Yeah, I was going to let this bake on the trunk a day or two before landing on
the branch. My initial plan was to land this on the branch on Monday, assuming
all is well, unless there's a reason to land before the Monday morning branch
builds are fired off. If so, let me know and I'll try to land it on Sunday.

Comment 37

14 years ago
a=adt to land this on the 1.4 branch.  Please add the fixed1.4 keyword once this
fix lands.
(Assignee)

Comment 38

14 years ago
Fix checked in on the branch now too.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Keywords: fixed1.4
Resolution: --- → FIXED
(Reporter)

Updated

14 years ago
Group: security
(Reporter)

Updated

4 years ago
Keywords: csec-sop
You need to log in before you can comment on or make changes to this bug.