109113 - Content accessing chrome window, still causing problems

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Mitchell Stoltz (not reading bugmail)]

Even with the fix for 105705, scripts in content can still call a few functions
on the chrome window, and while we haven't found any serious exploits there,
they may exist. Examples to follow.

Comment 1

[Mitchell Stoltz (not reading bugmail)]

Attachment: Example 1 - arg8.html

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Attachment: Example 2 - locals.html

Comment 3

[Mitchell Stoltz (not reading bugmail)]

Attachment: Example 3 - argcrash.html

Comment 4

[Mitchell Stoltz (not reading bugmail)]

From email conversation between mstoltz and brendan:

>>In addition to the fix for bug 105705, I think we want to block access to a
>>function's __parent__ property if that parent is the chrome window. Currently
>>there's no security check that I know of on __parent__. Can we add one? 
>Sure -- the engine will call the DOM's JSClass.checkAccess hook with
>JSACC_PARENT for all such accesses. 

What if the function in question is not part of the DOM? Then the DOMClassInfo
helper won't be called at all. I think that's waht we were seeing with the patch
you wrote - it didn't fix the original problem, at least.



>The potential problem (3) could be handled by the patch I came up with for the
>bug you guys were and still are wrestling with.  See attached, let me know if I
>should check it in (with your r= and sr= of course, and if into the 0.9.6
>frozen trunk, with a driver a=).

(3) is not fixed by your patch. As I said, the CheckAccess callback for
JSACC_CALLER never reaches the DOMClassInfo helper. I think it's just ignored.
How do I make use of the CheckAccess callback called for a JS (not a DOM) function?

Comment 5

[Mitchell Stoltz (not reading bugmail)]

Attachment: arg9a.html - accesses XPConnect using __call__

OK, he's found a real exploit using this method, and nothing yst suggested
fixes this. I think we need to attack this on a number of fronts - block all
cross-frame access to __parent__ and __call__, and probably some other changes
as well.

Comment 6

[Arun Ranganathan]

putting barrowma and sol on cc

Comment 7

[Brendan Eich [:brendan]]

Attachment: ok, dig this: rip out __call__ altogether (it's non-standard and obviously hazardous)

Comment 8

[Brendan Eich [:brendan]]

shaver, jband: my attachment 57127 [details] [diff] [review] removes __call__ utterly (as a property of
function and call objects).  Please r= and sr= for 0.9.6 to help close a
security hole.

/be

Comment 9

[John Bandhauer]

Comment on attachment 57127 [details] [diff] [review]
ok, dig this: rip out __call__ altogether (it's non-standard and obviously hazardous)

sr=jband

Comment 10

[Mitchell Stoltz (not reading bugmail)]

Removing __call__ obviously fixes the problem at hand, but I want to make sure
thare aren't any other exploits lurking in this arguments.callee.caller business.

Is caller a read-only property? What properties or functions does a function
object have? I'd like to block access to caller if caller is a function in chrome.

Comment 11

[Brendan Eich [:brendan]]

>Is caller a read-only property?

No, it can be set, but if set, the overridden value is preserved.  In no case
does the engine itself get the value of caller, so it doesn't matter what value
an evil script might store there.

>What properties or functions does a function object have?

arguments, arity, length, name, caller.  The callee and length properties, per
ECMA-262, appear to be direct properties of each function object, but in our
implementation they inherit from Function.prototype.  Function objects inherit
__proto__, __parent__, and __count__ from Object.prototype.

Note that foo.arguments for a function foo is deprecated and gets you a strict
warning (offtopic, FYI only).

>I'd like to block access to caller if caller is a function in chrome.

That will take another patch.  Just a sec...

/be

Comment 12

[Brendan Eich [:brendan]]

Attachment: rip out __call__, add JS_SetCheckObjectAccessCallback and rt->checkObjectAccess, called for f.caller gets currently

Comment 13

[John Bandhauer]

Isn't that a little too hacky? You've added something very general, but it 
seems to me that it would be hard to use it for anything else. It doesn't seem 
like the callback implementor will be able to descriminate well enough. In this 
case the id is a tinyid that is not exported. Is there some way (that I don't 
know of) to map from the tinyid back to a property name? I see that 
js_FunctionClass *is* exported. So, one can at least tell that the given object 
is a function (if one #includes jsfun.c). Still, it looks like the implementor 
will have to hit this with a big hammer and hope that the engine does not start 
using this machanism for other properties. Am I missing something?

Comment 14

[John Bandhauer]

s/jsfun.c/jsfun.h/

Comment 15

[Brendan Eich [:brendan]]

jband: JS_TypeOfValue(cx, OBJECT_TO_JSVAL(v)) == JSTYPE_FUNCTION may help for
classifying obj.  But you're right, in my haste I blew it and forgot that id was
tiny.  I'll fix to pass ATOM_KEY(cx->runtime->atomState.callerAtom) for the
string-valued id -- or perhaps I should switch the callback to be of type
JSCheckAccessIdOp?

/be

Comment 16

[Brendan Eich [:brendan]]

Attachment: better fix, but jband convinced me to pass the jsval flavor of id, not the jsid flavor.

Comment 17

[John Bandhauer]

Reminder: we discussed that it might be easier for the callback implementor if 
you passed a jsval instead - the jsval of the intern'd string.

Comment 18

[Brendan Eich [:brendan]]

Attachment: best patch

Comment 19

[John Bandhauer]

Comment on attachment 57201 [details] [diff] [review]
best patch

I'm OK with this. But one question...
Should you fill in vp before calling the callback to give the callback a peek
at the result if it wants? I see that js_CheckAccess does that for native
objects.

Comment 20

[Brendan Eich [:brendan]]

jband: sure, anything to get this patch in! :-)  I simply transposed the *vp
setting if-else and the if (cx->runtime->checkObjectAccess) {...} statement. 
Lemme know if you want a new patch attached.

/be

Comment 21

[John Bandhauer]

brendan: I feel the same way :) No new patch needed - I trust you got it right. 
My sr= stands. I suppose that the current security manager will do its own walk 
to decide if the access is allowed or not. But we should certainly be consistent 
here and give the callback the data.

Comment 22

[Mike Shaver (:shaver emeritus)]

Comment on attachment 57201 [details] [diff] [review]
best patch

mmm, could we just this to make __parent__ writable again, and let the Mozilla
setup deny it?

r=shaver.

Comment 23

[Brendan Eich [:brendan]]

mstoltz or jst, can you attach the patch to use JS_SetCheckObjectAccessCallback
from the DOM code, so we can review and then go to drivers with the total sol'n
for 0.9.6?

/be

Comment 24

[Mitchell Stoltz (not reading bugmail)]

Brendan, the JSObject* passed to the callback function when function.caller is
accessed appears to be the function object whose caller property is being
accessed. How would I get the object pointed to by the caller property? That's
the one I need to compare to the caller.

I'll attach what I've got so far. I think I need to put more code into
CheckJSFunctionPropertyAccess (my callback) to get a pointer to the caller object.

Comment 25

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch to NSScriptSecurityManager to use new CheckObjectAccess callback. Doesn't work right yet - need to find the right target object

Comment 26

[Mitchell Stoltz (not reading bugmail)]

Attachment: Revised patch to nsScriptSecurityManager - register callback and disallow content->chrome calls on caller property

This one seems to work and it's ready for review.

Comment 27

[Brendan Eich [:brendan]]

mstoltz: the value of the id'd property (caller, in this case -- you could
assert that JS_GetStringChars(JSVAL_TO_STRING(id)) matches a const jschar[]
containing "caller") is passed to your checkObjectAccess hook in *vp, so you can
see it and maybe fiddle it.

Or so jband suggested I do, but I haven't attached that patch!  Sorry, coming
right up.  Hit me with a version of your patch revised to take advantage of *vp
(thereby avoiding the recursion-stopping flag and the JS_GetProperty call) and
I'll sr tomorrow.  I'll need your r= on the forthcoming js/src patch (which I
think jband will re-sr=).

/be

Comment 28

[Brendan Eich [:brendan]]

Attachment: best-est js/src patch

Comment 29

[Johnny Stenback (:jst)]

Comment on attachment 57904 [details] [diff] [review]
best-est js/src patch

r=jst

Comment 30

[Johnny Stenback (:jst)]

FYI, I will add back the code for the old security check tomorrow but when doing
that I'll disable the optimization. This way the patches in this bug should
actually show the difference between the old optimization and the new one, and
not just new code... Watch out for conflicts.

Comment 31

[John Bandhauer]

Comment on attachment 57904 [details] [diff] [review]
best-est js/src patch

sr=jband

Comment 32

[Mitchell Stoltz (not reading bugmail)]

Attachment: New security check, take 3 - uses Brendan's latest engine patch

This patch incorporates Brendan's suggestion for eliminating the GetProperty
call. Ready for review.

Comment 33

[Mitchell Stoltz (not reading bugmail)]

I thought of one moe change - if getting the security manager service fails, we
should return false, not true. If the callback has been registered then
nsScriptSecurityManager has already been initialized, so there's no legitimate
reason for the getService to fail. I've made that change locally, I will attach
a new patch if necessary.

Comment 34

[Brendan Eich [:brendan]]

Comment on attachment 57947 [details] [diff] [review]
New security check, take 3 - uses Brendan's latest engine patch

mstoltz, what if subject and object principals do not match, but neither is the
system principal?  Shouldn't we deny access in that case, too?

Nits:
- Indent these comments:

+// Currently, this function will be called only when function.caller
+// is accessed. If that changes, we will need to change this function.

- Maybe the callback name should be CheckPropertyAccess or something more
generic than "JSFunction".

/be

Comment 35

[Mitchell Stoltz (not reading bugmail)]

> mstoltz, what if subject and object principals do not match, but neither is the
> system principal?  Shouldn't we deny access in that case, too?

Do you think we should? We allow cross-host access to window objects and a few
of their properties, but then again it's possible that this exploit could be
used to get at some object in the DOM (other than the window) that isn't
otherwise security-checked. Thoughts?

As for the name of the function, right now it serves a specific purpose -
checking the caller property of function objects. A generic name would not be
accurate. If the function ever becomes more generic, then a generic name would
be more appropriate.

Comment 36

[Brendan Eich [:brendan]]

Ok, then the name should be more specific: CheckJSFunctionCallerPropertyAccess
or some such.

If victim.com can be tricked to call some evil.org function, the evil function
could get back to victim.com via caller and access properties.  Either we
already protect the right properties, or there's a bug.  And as you note,
evil.org could get at victim.com's window in common cases (frame spoofing,
etc.).  Ok, sorry to digress -- I'm happy with the patch other than the name and
indentation nits.

/be

Comment 37

[Mitchell Stoltz (not reading bugmail)]

Attachment: New security check, take 4

jst convinced me that we should block all cross-host access through
function.caller to prevent a script from modifying functions on pages from
another host. This makes the new function much simpler, becuase I can call the
standard DOM security check to enforce same-origin. I've also fixed the nits.

Comment 38

[Brendan Eich [:brendan]]

Comment on attachment 58001 [details] [diff] [review]
New security check, take 4

sr=[e-mail] and I think the sr=jst carries over.  Excellent work!

/be

Comment 39

[Brendan Eich [:brendan]]

Cc'ing blizzard for driver approval into 0.9.6.

/be

Comment 40

[Brendan Eich [:brendan]]

I've checked the js/src patch into trunk and branch.  Mitch, can you do likewise
with your patch?

/be

Comment 41

[Brendan Eich [:brendan]]

At blizzard's request, I checked in mstoltz's nsScriptSecurityManager.cpp patch
to the 0.9.6 branch.  I took the liberty of expanding tabs per the Emacs
modeline comment on line 1 of the file.  To get this onto the trunk (once the
trunk stops burning and its leak and perf regressions are fixed!), use

cvs up -j1.154 -j1.154.2.1 nsScriptSecurityManager.cpp

in mozilla/caps/src in a trunk-based tree.

/be

Comment 42

[Jaime Rodriguez, Jr.]

Adding PDT for tracking and 6.2 branch review purposes.

Comment 43

[Mitchell Stoltz (not reading bugmail)]

Checked in on the trunk - leaving open for PDT consideration.

Comment 44

[Jaime Rodriguez, Jr.]

PDT+, pls check this one into the 6.2 branch before 9 am PST.

Comment 45

[Jaime Rodriguez, Jr.]

who feels comfortable checking this one into the 6.2 branch?

Comment 46

[Mitchell Stoltz (not reading bugmail)]

Checked in on 6.2 branch, marking FIxed.

Comment 47

[georgi - hopefully not receiving bugspam]

The testcase ar10-1.html which redefines __proto__ in chrome and is potential
exploit still works for me on the latest nightly on linux.

Comment 48

[Mitchell Stoltz (not reading bugmail)]

Reopening - this doesn't seem to be fixed in today's trunk builds, even though
the fixes above are checked in.

Comment 49

[Mitchell Stoltz (not reading bugmail)]

I tested with a build from the Netscape 6.2 branch on NT, and the problem
appears to be fixed there, but not on the trunk.

Comment 50

[bsharma]

Verified on 2001-11-20-Trunk.

The test case fails.

Comment 51

[Jimmy Lee]

Adding Antonio to Cc.  He is helping with Mac testing.

Comment 52

[bsharma]

Verified on 2001-11-26-6.2.1 build on WinNT, Linux (7.2), Mac OS X, Mac OS 9.1.

All of the above test case passes.

Comment 53

[bsharma]

Verified on 2001-11-29-Trunk build on WinNT.

All of the attached test cases passes.

Comment 54

[bsharma]

Marking as verified FIXED.

Comment 55

[Marek Z. Jeziorek]

edt0.9.4+ per chofmann's e-mail:
"
yes, go ahead.

thanks

chris h."

Comment 56

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

This was checked in onto NS 6.2 branch, which I believe is the same as 0.9.4.
Therefore adding fixed0.9.4 keyword. If I am wrong, please correct.