Mitchell Stoltz (not reading bugmail) Assignee
	Description • 23 years ago

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.

	Mitchell Stoltz (not reading bugmail) Assignee
	Comment 4 • 23 years ago

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?

	Mitchell Stoltz (not reading bugmail) Assignee
	Comment 5 • 23 years ago

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.

	Arun Ranganathan
	Comment 6 • 23 years ago

putting barrowma and sol on cc

	Brendan Eich [:brendan]
	Comment 8 • 23 years ago

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

	John Bandhauer
	Comment 9 • 23 years ago

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

sr=jband

	Mitchell Stoltz (not reading bugmail) Assignee
	Comment 10 • 23 years ago

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.

	Brendan Eich [:brendan]
	Comment 11 • 23 years ago

>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

	John Bandhauer
	Comment 13 • 23 years ago

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?

	John Bandhauer
	Comment 14 • 23 years ago

s/jsfun.c/jsfun.h/

	Brendan Eich [:brendan]
	Comment 15 • 23 years ago

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

	John Bandhauer
	Comment 17 • 23 years ago

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.

	John Bandhauer
	Comment 19 • 23 years ago

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.

	Brendan Eich [:brendan]
	Comment 20 • 23 years ago

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

	John Bandhauer
	Comment 21 • 23 years ago

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.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 22 • 23 years ago

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.

	Brendan Eich [:brendan]
	Comment 23 • 23 years ago

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

	Mitchell Stoltz (not reading bugmail) Assignee
	Comment 24 • 23 years ago

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.

	Mitchell Stoltz (not reading bugmail) Assignee
	Comment 26 • 23 years ago

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

	Brendan Eich [:brendan]
	Comment 27 • 23 years ago

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

	Johnny Stenback (:jst)
	Comment 29 • 23 years ago

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

r=jst

	Johnny Stenback (:jst)
	Comment 30 • 23 years ago

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.

	John Bandhauer
	Comment 31 • 23 years ago

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

sr=jband

	Mitchell Stoltz (not reading bugmail) Assignee
	Comment 32 • 23 years ago

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

	Mitchell Stoltz (not reading bugmail) Assignee
	Comment 33 • 23 years ago

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.

	Brendan Eich [:brendan]
	Comment 34 • 23 years ago

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

	Mitchell Stoltz (not reading bugmail) Assignee
	Comment 35 • 23 years ago

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

	Brendan Eich [:brendan]
	Comment 36 • 23 years ago

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

	Mitchell Stoltz (not reading bugmail) Assignee
	Comment 37 • 23 years ago

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.

	Brendan Eich [:brendan]
	Comment 38 • 23 years ago

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

sr=brendan@mozilla.org and I think the sr=jst carries over.  Excellent work!

/be

	Brendan Eich [:brendan]
	Comment 39 • 23 years ago

Cc'ing blizzard for driver approval into 0.9.6.

/be

	Brendan Eich [:brendan]
	Comment 40 • 23 years ago

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

/be

	Brendan Eich [:brendan]
	Comment 41 • 23 years ago

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

	Jaime Rodriguez, Jr.
	Comment 42 • 23 years ago

Adding PDT for tracking and 6.2 branch review purposes.

	Mitchell Stoltz (not reading bugmail) Assignee
	Comment 43 • 23 years ago

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

	Jaime Rodriguez, Jr.
	Comment 44 • 23 years ago

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

	Jaime Rodriguez, Jr.
	Comment 45 • 23 years ago

who feels comfortable checking this one into the 6.2 branch?

	Mitchell Stoltz (not reading bugmail) Assignee
	Comment 46 • 23 years ago

Checked in on 6.2 branch, marking FIxed.

	georgi - hopefully not receiving bugspam
	Comment 47 • 23 years ago

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

	Mitchell Stoltz (not reading bugmail) Assignee
	Comment 48 • 23 years ago

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

	Mitchell Stoltz (not reading bugmail) Assignee
	Comment 49 • 23 years ago

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.

	bsharma
	Comment 50 • 23 years ago

Verified on 2001-11-20-Trunk.

The test case fails.

	Jimmy Lee
	Comment 51 • 23 years ago

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

	bsharma
	Comment 52 • 23 years ago

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.

	bsharma
	Comment 53 • 23 years ago

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

All of the attached test cases passes.

	bsharma
	Comment 54 • 23 years ago

Marking as verified FIXED.

	Marek Z. Jeziorek
	Comment 55 • 23 years ago

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

thanks

chris h."

	Heikki Toivonen (remove -bugzilla when emailing directly)
	Comment 56 • 23 years ago

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.