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

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

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.

Comment 1

[Jesse Ruderman]

Attachment: demo: can tell whether another window is amazon.com or not

Comment 2

[Brendan Eich [:brendan]]

Please define uncatchable, show the try/catch code that fails.  Try it in the
xpcshell and js shell too.

/be

Comment 3

[Phil Schwartau]

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

[Phil Schwartau]

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" 

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Brendan Eich [:brendan]]

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

[Phil Schwartau]

> 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

[Phil Schwartau]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Reassigning to brendan, downgrading to mustfix.

Comment 10

[Brendan Eich [:brendan]]

Fixing TM.

/be

Comment 11

[chris hofmann]

close to a patch?

Comment 12

[Mitchell Stoltz (not reading bugmail)]

Adding rogerl, he may be interested in this bug as well.

Comment 13

[Brendan Eich [:brendan]]

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

Comment 14

[Jesse Ruderman]

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

[chris hofmann]

trying for 1.4 final

Comment 16

[Brendan Eich [:brendan]]

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

Comment 17

[Mitchell Stoltz (not reading bugmail)]

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.

Comment 18

[Jesse Ruderman]

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

Comment 19

[Mitchell Stoltz (not reading bugmail)]

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.

Comment 20

[Brendan Eich [: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

Comment 21

[Jesse Ruderman]

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.

Comment 22

[Brendan Eich [:brendan]]

Attachment: 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

Comment 23

[Johnny Stenback (:jst)]

Taking bug (per discussion with Brendan) to continue with the remaining part.

Comment 24

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 25

[Brendan Eich [:brendan]]

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

Comment 26

[Johnny Stenback (:jst)]

Attachment: In addition to the above, do security checks in the resolve hooks for window and document

Comment 27

[Johnny Stenback (:jst)]

Attachment: diff -w of the above.

Comment 28

[Johnny Stenback (:jst)]

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.

Comment 29

[Brendan Eich [:brendan]]

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

Comment 30

[Johnny Stenback (:jst)]

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

Comment 31

[Brendan Eich [:brendan]]

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=[e-mail] with the above nit-picked or otherwise addressed.

/be

Comment 32

[Johnny Stenback (:jst)]

> >+      // 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 33

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 34

[Johnny Stenback (:jst)]

Fix checked in on the trunk.

Comment 35

[Brendan Eich [:brendan]]

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

Comment 36

[Johnny Stenback (:jst)]

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

[Samir Gehani]

a=adt to land this on the 1.4 branch.  Please add the fixed1.4 keyword once this
fix lands.

Comment 38

[Johnny Stenback (:jst)]

Fix checked in on the branch now too.