404720 - consider dropping support for arguments.callee.caller

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect, P3)

Description

[georgi - hopefully not receiving bugspam]

Attachment: bz6.html - toString() strangeness

had some discussion with brendan about this.

suppose js code expects a string and does some checks.
pass it an object with toString() which checks in what context it is via
|arguments|, returning different stuff effectively bypassing any string
validation. (if you can't get the exact context you can randomly
return correct and exploitable values)

in addition code may modify arguments of calling functions via:
arguments.callee.caller.arguments.callee.caller.arguments[1]='new c';
this modifies the second argument.

example - this looks like deterministic code if it were C/C++:
-------
function validate(a)
{
	if (a=="bz") 
		return true
	else
		return false		
}
function dostuff(a,c)
{
	var b=validate(a);
	if(b) 
		alert('pass: a='+a);
	else
		alert('fail: a='+a);
	alert('now a='+a);
	if(typeof(c)!="undefined") alert(' c='+c)	

}
-------
(also attached)
js reality is if |a| is an object with special |toString|, after the call to |validate|, |b| is |true| and later on |a| may have arbitrary values and if |c| is defined, |c| may be changed by |validate| or |dostuff| via |a|

potential misuse of this:
javascript:var m={};m.watch("value",function(a,b,c)
{alert(11)});sidebar.getInterfaces(m)

pasting above in location executes js and reloading it via hitting enter gives:
Error: uncaught exception: Permission denied to call method UnnamedClass.toString

Comment 1

[Brendan Eich [:brendan]]

I'm in favor, let's lose this old crufty Netscape-ism. Crowder, can you take this bug for 1.9b2? Thanks,

/be

Comment 2

[georgi - hopefully not receiving bugspam]

suggest moz_bug_r_a4 comment on the exploitability of the sidebar watchpoint

Comment 3

[Jesse Ruderman]

http://www.google.com/search?q=arguments.callee.caller shows some use but not much.

CCing Jomel from bug 390488.  Jomel, you can use (new Error).stack instead, right?

Comment 4

[Jeff Walden [:Waldo]]

It depends whether you compare for specific functions on the stack instead of function names.  .stack is enough for diagnostic purposes and error messages, but it's not enough for totally correct functionality dispatch based on an ancestor caller's identity.

I'll be sad to see this go -- I had some awesome times using .caller to poke at Facebook's JS rewriting system -- but I can't say I have any legitimate, might-have-used-in-real-world-code reasons to keep it.  Also, such frameworks will almost inevitably ban caller eventually anyway, or at least be super-careful not to call methods or access properties on user-provided values (in combination with using a singly-recursive function to prevent access up the stack, but even that doesn't stop the arguments mutation -- didn't remember that existed).

Also, another strike on the slightly pedantic side is that walking the stack with .caller is O(n^2), because the property's calculated anew each time by walking from the current frame up the stack.

Word on the street is that of the other browsers only IE supports .caller, which might limit its real-world use.  We've got a bunch in the tree, tho; first step should be to look at those to see what the use cases are.

Comment 5

[John Mellor (Jomel)]

Ah - I'm actually using myFunction.caller in Tab Kit [1] for a serious (if hacky) purpose - whenever a tab is added, I use the call stack to find out what function was responsible for the tab being added, and if it's a known function I use a lookup table to find out how to deal with it (if it's relevant to the current tab I might group them together, if it's not I might move it to the end of the tab bar, etc).
Yes it's ugly, yes it's labor intensive, but it's also just about the only way of doing anything like this, and works well enough in practice :/

While technically (new Error).stack could be parsed to get stack information, that forces you to assume that the syntax of the error stacktrace will always stay the same, which isn't very satisfying...

That said, chrome js can also use Components.stack.caller, so uses in Mozilla code/extensions could probably be changed to use that if necessary.

It can also be handy for debugging though - I don't know if this is done currently, but the fact that it also works in IE could make it a useful way for javascript frameworks to print out debug stacktraces, for example.

[1]: https://addons.mozilla.org/firefox/addon/5447

Comment 6

[georgi - hopefully not receiving bugspam]

f.__proto__.toString=function() {alert('tos');return 'tos'}
makes usage of
|arguments.callee.caller| close to useless against malicious code

Comment 7

[georgi - hopefully not receiving bugspam]

a potential exploit scenario is:

chrome(a,s) {
[1] if (a=="1")
...

if |a| is malicious and |s| is sensitive [1] may set |s| to anything i suppose. 

Comment 8

[Mike Schroepfer]

Restoring wanted flag

Comment 9

[georgi - hopefully not receiving bugspam]

[sg:nse]

Comment 10

[Brian Crowder]

Attachment: Eliminate the "caller" property

Sorry this fell off my radar for a while.  Not quite sure this is what's wanted and I also wonder if more could/should be removed in jsexn.c:InitExnPrivate() which uses caller in a way which is not obvious to me at a glance.

Comment 11

[georgi - hopefully not receiving bugspam]

if you checkin this, have in mind it may break some usage of arguments.callee.caller, so several lines need to be rewritten:
grep -rniI 'arguments.callee.caller' *

Comment 12

[Brian Crowder]

Georgi:  I assume you're referring to the test-suite tests that will be affected by this change?

Comment 13

[georgi - hopefully not receiving bugspam]

hm, i haven't examined where it is used. at top of firefox source i just did:
grep -rniI 'arguments.callee.caller' * 

lxr:
http://lxr.mozilla.org/seamonkey/search?string=arguments.callee.caller
or

Comment 14

[Mike Shaver (:shaver emeritus)]

(In reply to comment #13)
> hm, i haven't examined where it is used. at top of firefox source i just did:
> grep -rniI 'arguments.callee.caller' * 
> 
> lxr:
> http://lxr.mozilla.org/seamonkey/search?string=arguments.callee.caller

That link produces no results for me -- can you attach your grep results, if you think they point to further necessary work?

Comment 15

[georgi - hopefully not receiving bugspam]

the link worked morning my time, now it seems to timeout probably due to server load.

here is my grep:
$grep -rniI 'arguments.callee.caller' * 2>/dev/null
js/src/xpconnect/tests/mochitest/test_bug390488.html:26:    var func = arguments.callee.caller;
toolkit/content/debug.js:81:  var caller = arguments.callee.caller;
toolkit/content/debug.js:106:      caller = caller.arguments.callee.caller;
toolkit/mozapps/extensions/src/nsExtensionManager.js.in:8680:  var temp = aArguments.callee.caller;
toolkit/mozapps/extensions/src/nsExtensionManager.js.in:8685:    temp = temp.arguments.callee.caller;

and these may be out of sync with cvs:
fb-opt-static/dist/bin/components/nsSearchService.js:3174:  var caller = arguments.callee.caller;
fb-opt-static/dist/bin/components/nsSearchService.js:3199:      caller = caller.arguments.callee.caller;
fb-opt-static/dist/bin/components/FeedConverter.js:691:  var caller = arguments.callee.caller;
fb-opt-static/dist/bin/components/FeedConverter.js:716:      caller = caller.arguments.callee.caller;
fb-opt-static/dist/bin/components/WebContentConverter.js:907:  var caller = arguments.callee.caller;
fb-opt-static/dist/bin/components/WebContentConverter.js:932:      caller = caller.arguments.callee.caller;

not sure if they need work

note that such grep-ing doesn't catch cases like:
var a=arguments;
var b=a.callee;

Comment 16

[georgi - hopefully not receiving bugspam]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/debug.js&rev=1.7&mark=81#81
81 joe   1.1   var caller = arguments.callee.caller;

Comment 17

[Brian Crowder]

For me, lxr turns up five instances of use:

/toolkit/mozapps/extensions/src/nsExtensionManager.js.in, line 8680
   -- var temp = aArguments.callee.caller;
/toolkit/mozapps/extensions/src/nsExtensionManager.js.in, line 8685
   -- temp = temp.arguments.callee.caller;
/toolkit/content/debug.js, line 81
   -- var caller = arguments.callee.caller;
/toolkit/content/debug.js, line 106
   -- caller = caller.arguments.callee.caller;
/js/src/xpconnect/tests/mochitest/test_bug390488.html, line 26
   -- var func = arguments.callee.caller;

Extension Manager and debug.js both use it exclusively for generating stack traces (doesn't xpconnect provide a way of doing this?), as does the bug390488 test (cc:ing bz, who wrote that test)

Comment 18

[Brian Crowder]

Perhaps you should only be able to get caller if callee has identical principals?

Comment 19

[georgi - hopefully not receiving bugspam]

afaict the only constructive usage of this construct is to get the stack.

one get the stack via new Error() as per comments.

Comment 20

[John Mellor (Jomel)]

(In reply to comment #19)
> afaict the only constructive usage of this construct is to get the stack.
> 
> one get the stack via new Error() as per comments.
> 

Yes, but as explained in comment #4 and comment #5, (new Error).stack isn't much good for programmatically accessing the stack. (Reasons for doing this include code that needs to change behaviour depending on what's calling it, or simply generating prettier stack traces - (new Error).stack can get quite ugly when looking at a stack of eval calls, since it always includes function arguments).

To prevent cheating toString you can either first check that (typeof obj == "string"), or use strict equality (obj === "bz"). It seems the biggest risk (e.g. comment #7) actually comes from being able to modify aFunction.arguments...

Comment 21

[georgi - hopefully not receiving bugspam]

(In reply to comment #20)
> It seems the biggest risk
> (e.g. comment #7) actually comes from being able to modify
> aFunction.arguments...
> 

sure. if i remember correctly long ago there was a chrome exploit something like this:
in chrome:

chrome(objarg,importanturi) {
objarg gets called
it modifies importanturi to javascript exploit
openDialog(importanturi)

}

it may have been modifying local variables (this `feature' is killed now), not quite sure

Comment 22

[Brian Crowder]

Maybe this problem needs more thought.  Letting go of the bug until those issues are resolved.

Comment 23

[Brendan Eich [:brendan]]

Jesse can probably id the bug georgi is referring to in comment 21.

Cc'ing Bill Edney, as I recall TIBET uses caller.

Not in favor of posted patch -- it leaves callerAtom used in InitExnPrivate as Brian noted. At this point in 1.9, it might be better to neuter mutations of actual parameters via f.arguments for function object f (whether f was expressed via caller or not) -- arguments[i] = x would of course continue to work in an active function where i was less than the number of actuals, but the ancient and bogus foo.arguments support could be restricted to read-only operation.

/be

Comment 24

[Brendan Eich [:brendan]]

Neutering foo.arguments to provide read-only views of actual arguments means wrapping, or adding some kind of write barrier. Too much code required.

Instead we could break bar.arguments compatibility if bar was the result of evaluating foo.caller for some function foo. In other words, a poison pill: getting .caller from a function object sets a flag on the stack, which defeats attempts to write *or read* arguments from the flagged frame.

Walking up the stack via .caller.caller would poison each frame in turn, so that accessing f.arguments for any f equal to a poisoned frame's callee member would return null (probably better than undefined). This would not require much code. Comments welcome.

/be

Comment 25

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 26

[Brendan Eich [:brendan]]

With the patch applied:

js> function g(a,b) { g.caller.arguments[0] = 3; }
js> function f(g) { g(1,2); return g }
js> f(g)
typein:1: TypeError: g.caller.arguments is null

/be

Comment 27

[georgi - hopefully not receiving bugspam]

why keep it at all?

isn't it possible to write a js function that gets |arguments.caller.callee| from |Error()| (Error is trusted)

who will not be happy in this case?

Comment 28

[Jesse Ruderman]

Georgi's comment 21 refers to bug 109113.  There were also a few instances where georgi used arguments.callee.caller.__parent__ to get a reference to a chrome window: bug 105705, bug 290324 comment 17.

Comment 29

[Jesse Ruderman]

I believe (new Error).stack is a more reliable way to get a stack trace than crawling up the caller chain, because the caller chain doesn't deal with recursive functions well.

Comment 30

[georgi - hopefully not receiving bugspam]

looks like people are more interested in who called their code, so adding a property to |Error| may be an option, assuming |Error| is safe

Comment 31

[Brian Crowder]

Is it intended behavior that f also lose access to its own arguments? :

js> function f(g) { print(f.arguments); try { g(1,2); } catch (e) { print("caught " + e) } finally { print(f.arguments) } }
js> f(g)
[object Object]                             
caught TypeError: g.caller.arguments is null
null

Comment 32

[William J. Edney]

Brendan -

Thanks very much for adding me to this bug.

I'm trying to refresh my memory here, so enlighten me if I'm wrong.

At one point, we used 'arguments.caller', which allowed complete access to the
stack, including caller, arguments, etc. This allowed access to the
'activations' themselves. Note that IE, up through the JScript 5.7 engine in
IE7, still allows access to 'arguments.caller'.

This support was removed many ages ago (ECMA E3 committed dropped it back in
late 1999 / early 2000 if memory serves and you removed it from Mozilla. At
that point we stopped relying on it and went a different route (basically
passing 'arguments' into the routines that needed access to the real activation
stack).

When this was removed from Mozilla, you added 'arguments.callee.caller', which
provided a way to walk just the stack of Functions themselves. We do use this,
unless the developer has switched on XPConnect, in which case we use
'Components.stack.caller' (which looks like its the stack of real activations),
and start walking from there.

Please note that we use this capability *ad hoc*, that is outside of the
context of an Error being thrown.

We do not assume the ability to get access to the 'arguments' of each Function
as we walk (since we assumed that each one of these objects was simply the
'dead' Function object), but be aware that, since we 'instance program'
Function objects with private slots that we assume to be read/write, we're
assuming that the objects accessible via this walking process are the same
actual objects that we're manipulating in code.

Given that, I guess we'd be ok with your solution in comment #24, as long as
access to slots on the actual Function objects themselves were still
read/write.

Thanks again for letting me know about this!!

Cheers,

- Bill

Comment 33

[Brendan Eich [:brendan]]

(In reply to comment #31)
> Is it intended behavior that f also lose access to its own arguments?

Yes, it's rum-dumb and effective. No modern JS should use f.arguments, no way no how. It predates ECMA and is an implementation artifact from the First Age, when Balrogs roamed Middle-earth.

I do not propose to yank caller this close to 1.9 being done. For Mozilla 2, sure -- it should be completely removed. I'd like comments on what exploitable surface remains.

/be

Comment 34

[Brian Crowder]

Comment on attachment 303002 [details] [diff] [review]
proposed fix

Looks good to me, then.

Comment 35

[georgi - hopefully not receiving bugspam]

this works from getters. if you know of open bugs where chrome executes user js and there are sensitive arguments in chrome functions probably branch should be fixed sooner.

Comment 36

[georgi - hopefully not receiving bugspam]

hm, there is some protection in chrome against this attack.
when chrome executes getter, get error:
permission denied to get Function.caller

Comment 37

[Brendan Eich [:brendan]]

georgi: so, two thumbs up on the patch?

/be

Comment 38

[georgi - hopefully not receiving bugspam]

(In reply to comment #37)
> georgi: so, two thumbs up on the patch?
> 
> /be
> 

i'll need to test the patch.

not sure what this demonstrates:
js> function g(a,b) { g.caller.arguments[0] = 3; }
js> function f(g) { g(1,2); return g }
js> f(g)
typein:1: TypeError: g.caller.arguments is null

without the patch |arguments.caller| is undefined, afaict the correct usage is arguments.CALLEE.caller


in this comment:
#define JSFRAME_POISON_ARGS  0x1000 /* frame.callee accessed via f.caller, so
                                       censor f.caller.arguments */

|callee| and |caller| seem swapped if i understand the usage of |arguments| correctly.

Comment 39

[georgi - hopefully not receiving bugspam]

the patch seems to stop access to arguments.callee.caller.arguments and this seems to stop redefining arguments. it is still possible to read caller's arguments:

javascript:function g(a,b) {alert("caller[1]="+f[1]+" arg[1]="+arguments.callee.caller[1])} function f(a,b) {var c=3;g(1,1);alert("b="+b)}f(1,5)

Comment 40

[georgi - hopefully not receiving bugspam]

if the only goal of the patch is to stop redefining arguments up the stack it is ok for me.

remaining stuff from the testcase.

if js code accepts js object as argument and assumes it is a string and does some validation, toString() may cheat possibly bypassing validation, i.e. the first time toString() returns something, the second something else:

javascript:var ob={toString: function x() {return Math.random()>= 0.5 ? "1" : "2"}};function f(a) {alert(' =="1" ? '+(a=="1"))};f(ob)

Comment 41

[Igor Bukanov]

(In reply to comment #39)
> the patch seems to stop access to arguments.callee.caller.arguments and this
> seems to stop redefining arguments. it is still possible to read caller's
> arguments:

Thanks for finding this! I do not think that even a read-only access to arguments should be allowed.

Comment 42

[georgi - hopefully not receiving bugspam]

(In reply to comment #41)
> I do not think that even a read-only access to
> arguments should be allowed.
> 

afaict it is for the direct caller only, walking up the stack doesn't seem possible

Comment 43

[georgi - hopefully not receiving bugspam]

btw, the patch doesn't apply cleanly - there is a conflict with the #define so i fixed it by hand.

Comment 44

[Igor Bukanov]

(In reply to comment #25)
> Created an attachment (id=303002) [details]
> proposed fix
> 

With the patch one can not use arguments.callee.caller to discover the argument object of the parent frame. But if a piece of code wants to phish/alter the information in a parent frame, it will most likely wants to do that for a particular function. But then it can call functionReference.arguments directly which continue to work even with the patch applied. 

Disabling functionReference.arguments for non-current frame would cover this case as well. Would it break too much? 

Comment 45

[Brendan Eich [:brendan]]

Attachment: plus what Igor proposed

I'm very uncomfortable shipping this in beta 4. I'm also too busy atm to keep carrying this patch. Suggestions?

/be

Comment 46

[georgi - hopefully not receiving bugspam]

if it is not possible to mess with chrome arguments (some evidence support this) probably this is low risk of exploit. otherwise it should be patched imo.

Comment 47

[Igor Bukanov]

(In reply to comment #45)
> Created an attachment (id=304912) [details]
> plus what Igor proposed

If the goal of patch is to prevent accessing f.arguments whenever f is not a current frame, then there is no need to tag the frame as the function can simply verify that fp->fun is f.

Comment 48

[Brendan Eich [:brendan]]

Igor: good point, I'm not doing the right thing here hacking in haste.

I'm not going to take chances of regressing someone with lots of users who we nevertheless do not hear from in the short time remaining for 1.9.

/be

Comment 49

[georgi - hopefully not receiving bugspam]

offtopic:

does this work as expected, seems counter intuitive to me:

function f(x) {return [x,x+1]};[f,f]=f(1);f

Comment 50

[Jesse Ruderman]

Seems reasonable to me.  [f,f]=f(1) first sets f to 1, then sets f to 2.

Comment 51

[georgi - hopefully not receiving bugspam]

(In reply to comment #50)
> Seems reasonable to me.  [f,f]=f(1) first sets f to 1, then sets f to 2.
> 

i knew this, something else bugged me. now i don't find this counter intuitive.

Comment 52

[Josh Aas]

Can we open this bug up? Looks like security concerns around this are already discussed publicly.

Comment 53

[Jeff Walden [:Waldo]]

Luke and I looked at this, we think we're okay opening this.  Security wrappers help with many of the concerns, and .caller being brokeny is already basically known, and is what contributed to its being a poison pill on strict mode functions.

Comment 54

[Claude Pache]

See also bug 1615704.