Problems with strict-mode caller poisoning
Andreas Rossberg wrote:
Consider the following code:
function f() { "use strict"; g() } function g() { var caller = Object.getOwnPropertyDescriptor(g, "caller").value }With the current spec, this code would legally give g the strict function f as its caller. In ecmascript#310, Allen proposes the obvious fix, which is to special case [[GetOwnProperty]] instead of [[Get]] for function objects in 15.3.5.4. In fact, that is what both V8 and FF already implement.
SpiderMonkey in FF does this but not by layering on top of an exact implementation equivalent of GetOwnProperty. Still, it's close enough (more below).
However, we recently discovered an issue with that semantics. Namely, it causes Object.is{Sealed,Frozen} and Object.{seal,freeze} to spuriously throw when applied to the wrong function at the wrong time. Consider:
d8> function g() { Object.seal(g) } d8> function f() { "use strict"; g() } d8> f() (d8):1: TypeError: Illegal access to a strict mode caller function.
(Interestingly, Firefox does not throw on that example, so I'm not sure what semantics it actually implements.)
It looks like SpiderMonkey's Object.{seal,freeze,isSealed,isFrozen} implementations do not call [[GetOwnProperty]] on each property. Cc'ing Jeff Walden.
ES6 is redoing the spec internal methods and helpers, so layering exactly as ES5 did is probably a mistake.
What can we do here? There does not seem to be a clean fix, only more hacks on top of hacks. It is a bit of a bummer for our implementation of Object.observe, which wants an isFrozen check on its callback.
Thoughts?
One thought: the spec (ES5 at least, not sure where ES6 drafts are on redoing internal methods) customized [[Get]] for function objects to avoid the value of caller leaking out. Reflecting on the property descriptor for caller of a strict-mode function is ok so long as one doesn't look at .value.
So it seems to me premature to throw on [[GetOwnProperty]] of a strict function's 'caller'. It would be more precise, and avoid the problem you're hitting, to return a property descriptor with a censored .value, or a poisoned-pill throwing-accessor .value.
On 11/16/2012 07:06 AM, Brendan Eich wrote:
d8> function g() { Object.seal(g) } d8> function f() { "use strict"; g() } d8> f() (d8):1: TypeError: Illegal access to a strict mode caller function.
(Interestingly, Firefox does not throw on that example, so I'm not sure what semantics it actually implements.)
It looks like SpiderMonkey's Object.{seal,freeze,isSealed,isFrozen} implementations do not call [[GetOwnProperty]] on each property. Cc'ing Jeff Walden.
They do, sort of, but .caller doesn't fit into the [[GetOwnProperty]] system well in SpiderMonkey because, on functions that are not themselves strict (or bound), it's neither a data property nor an accessor property. That this happens to not complain vociferously is nearly happenstance at best. (I suspect it did at one time, actually, and I bet I know the bug that changed it, but it doesn't really matter in the end.)
So it seems to me premature to throw on [[GetOwnProperty]] of a strict function's 'caller'. It would be more precise, and avoid the problem you're hitting, to return a property descriptor with a censored .value, or a poisoned-pill throwing-accessor .value.
"premature to throw on [GetOwnProperty] on a function whose caller is strict", I assume you meant. That seems right to me. Since caller is a time-variant characteristic, it seems right for the property to be an accessor, probably existing solely on Function.prototype, and to defer all the strictness checks to when the function provided as |this| is actually invoked.
Such a property is no different from anything already in the language, so I can't see how it would at all interfere with Object.observe semantics or implementation.
On 11/16/2012 01:19 PM, Jeff Walden wrote:
and to defer all the strictness checks to when the function provided as |this| is actually invoked.
Er, that should be "and to have the 'caller' [[Get]] function check the strictness of the |this| function provided to it when that [[Get]] function is called". Too many function referents in play in all this discussion. :-\
On 11/16/2012 01:19 PM, Jeff Walden wrote:
Too many function referents in play in all this discussion. :-\
Indeed!
Care to update ecmascript#310 with a pellucid proposal along these lines? :-)
On 16 November 2012 22:19, Jeff Walden <jwalden+es at mit.edu> wrote:
On 11/16/2012 07:06 AM, Brendan Eich wrote:
So it seems to me premature to throw on [[GetOwnProperty]] of a strict function's 'caller'. It would be more precise, and avoid the problem you're hitting, to return a property descriptor with a censored .value, or a poisoned-pill throwing-accessor .value.
That may be plausible, but requires making the 'value' property an accessor, and hence breaks with the idea that descriptors are just "records". But maybe that is OK for this hack? We should at least be careful to define it such that the meaning and behaviour of the descriptor does not vary in time, which would be weird at best. I.e., its return value and/or poisoning has to be determined once when [[GetOwnProperty]] is executed.
"premature to throw on [GetOwnProperty] on a function whose caller is strict", I assume you meant. That seems right to me. Since caller is a time-variant characteristic, it seems right for the property to be an accessor, probably existing solely on Function.prototype, and to defer all the strictness checks to when the function provided as |this| is actually invoked.
I'm not sure I follow, are you talking about the 'caller' property itself now or the 'value' property of its descriptor?
The problem with 'caller' itself is that the spec does not (and doesn't want to) spec it for non-strict functions, so it cannot prescribe it to be an accessor. All would be fine, I suppose, if it was.
If you are talking about the descriptor's 'value' property then I strongly oppose making that vary in time. A time varying descriptor would be weird at best. Fortunately, it's not necessary either.
Such a property is no different from anything already in the language, so I can't see how it would at all interfere with Object.observe semantics or implementation.
See above, non-strict 'caller' is special in that the spec does not try define it but yet guard against it with special, er, measures.
On Nov 20, 2012, at 4:01 AM, Andreas Rossberg wrote:
On 16 November 2012 22:19, Jeff Walden <jwalden+es at mit.edu> wrote:
On 11/16/2012 07:06 AM, Brendan Eich wrote:
So it seems to me premature to throw on [[GetOwnProperty]] of a strict function's 'caller'. It would be more precise, and avoid the problem you're hitting, to return a property descriptor with a censored .value, or a poisoned-pill throwing-accessor .value.
That may be plausible, but requires making the 'value' property an accessor, and hence breaks with the idea that descriptors are just "records". But maybe that is OK for this hack? We should at least be careful to define it such that the meaning and behaviour of the descriptor does not vary in time, which would be weird at best. I.e., its return value and/or poisoning has to be determined once when [[GetOwnProperty]] is executed.
Yes, property descriptor records can't act like accessors. They are just specification internal records that indicate that a set of values is being passed around. But we can censor the value that goes into the record. To me this seems like a sufficient solution for dealing with the security issue. It deviates from what was specified in ES5.1 but that is buggy and I don't think a change from throwing to returning null for the caller would create much havoc
"premature to throw on [GetOwnProperty] on a function whose caller is strict", I assume you meant. That seems right to me. Since caller is a time-variant characteristic, it seems right for the property to be an accessor, probably existing solely on Function.prototype, and to defer all the strictness checks to when the function provided as |this| is actually invoked.
I'm not sure I follow, are you talking about the 'caller' property itself now or the 'value' property of its descriptor?
The problem with 'caller' itself is that the spec does not (and doesn't want to) spec it for non-strict functions, so it cannot prescribe it to be an accessor. All would be fine, I suppose, if it was.
If you are talking about the descriptor's 'value' property then I strongly oppose making that vary in time. A time varying descriptor would be weird at best. Fortunately, it's not necessary either.
Such a property is no different from anything already in the language, so I can't see how it would at all interfere with Object.observe semantics or implementation.
See above, non-strict 'caller' is special in that the spec does not try define it but yet guard against it with special, er, measures.
/Andreas
+1
Allen Wirfs-Brock wrote:
On 11/16/2012 07:06 AM, Brendan Eich wrote:
So it seems to me premature to throw on [[GetOwnProperty]] of a strict function's 'caller'. It would be more precise, and avoid the problem you're hitting, to return a property descriptor with a censored .value,
Censored, yay. :-P
or a poisoned-pill throwing-accessor .value.
That may be plausible, but requires making the 'value' property an accessor, and hence breaks with the idea that descriptors are just "records". But maybe that is OK for this hack? We should at least be careful to define it such that the meaning and behaviour of the descriptor does_not_ vary in time, which would be weird at best. I.e., its return value and/or poisoning has to be determined once when [[GetOwnProperty]] is executed.
Yes, property descriptor records can't act like accessors. They are just specification internal records that indicate that a set of values is being passed around. But we can censor the value that goes into the record.
Is this going into the bug?
On 20 November 2012 17:26, Allen Wirfs-Brock <allen at wirfs-brock.com> wrote:
Yes, property descriptor records can't act like accessors. They are just specification internal records that indicate that a set of values is being passed around. But we can censor the value that goes into the record. To me this seems like a sufficient solution for dealing with the security issue. It deviates from what was specified in ES5.1 but that is buggy and I don't think a change from throwing to returning null for the caller would create much havoc
+1. I just implemented this in V8, and we will see how it goes in the wild.
Interestingly, none of the 97 tests in test262 that are specifically concerned with 15.3.5.4 fail after this change 8-}. It seems that they are broken in at least two ways: allowing a falsey value for .caller, and assuming that a global function would be non-strict even if the global scope is already strict.
Believe you're correct on the former, but perhaps not the latter=)
E.g.: 6 /** 7 * @path ch15/15.3/15.3.5/15.3.5.4/15.3.5.4_2-1gs.js 8 * @description Strict mode - checking access to strict function caller from strict function (FunctionDeclaration defined within strict mode) 9 * @onlyStrict 10 * @negative TypeError 11 */ 12 13 14 "use strict"; 15 function f() { 16 return gNonStrict(); 17 } 18 f(); 19 20 21 function gNonStrict() { 22 return gNonStrict.caller; 23 }
is globally scoped strict mode and passes only when a TypeError gets thrown indicating strict mode is in effect.
Dave
+1. I just implemented this in V8, and we will see how it goes in the wild.
Interestingly, none of the 97 tests in test262 that are specifically
concerned with 15.3.5.4 fail after this change 8-}. It seems that they are broken in at least two ways: allowing a falsey value for .caller, and assuming that a global function would be non-strict even if the global scope is already strict.
On 29 November 2012 00:16, Dave Fugate <dave.fugate at gmail.com> wrote:
Believe you're correct on the former, but perhaps not the latter=)
E.g.: 6 /** 7 * @path ch15/15.3/15.3.5/15.3.5.4/15.3.5.4_2-1gs.js 8 * @description Strict mode - checking access to strict function caller from strict function (FunctionDeclaration defined within strict mode) 9 * @onlyStrict 10 * @negative TypeError 11 */ 12 13 14 "use strict"; 15 function f() { 16 return gNonStrict(); 17 } 18 f(); 19 20 21 function gNonStrict() { 22 return gNonStrict.caller; 23 }
is globally scoped strict mode and passes only when a TypeError gets thrown indicating strict mode is in effect.
The bug with this test (and others) is that gNonStrict is not a non-strict function, its name notwithstanding. Hence the test throws for the wrong reason, namely because strict-function.caller is a poisoned getter, not because of Sec 15.3.5.4, which it is supposed to test.
The naming 'gNonStrict' here refers to the function not containing a "use strict" declaration itself, not that it's subject to strict mode. Sorry this intent wasn't clearer.
Section 15.3.5.4 step 2 in my copy of ES5 es5.github.com/#x15.3.5.4
reads: If P is *"caller" and v is a strict mode Function object, throw a * TypeError es5.github.com/#x15.11.6.5 exception.
Is something other than Function's [[Get]] really supposed to be called in this snippet? E.g., 13.2.19.b. If so, seems like they're still valid test cases, only they apply to step 1 of 15.3.5.4, not step 2?
On 29 November 2012 06:06, Dave Fugate <dave.fugate at gmail.com> wrote:
The naming 'gNonStrict' here refers to the function not containing a "use strict" declaration itself, not that it's subject to strict mode. Sorry this intent wasn't clearer.
Section 15.3.5.4 step 2 in my copy of ES5 reads: If P is "caller" and v is a strict mode Function object, throw a TypeError exception.
Is something other than Function's [[Get]] really supposed to be called in this snippet? E.g., 13.2.19.b. If so, seems like they're still valid test cases, only they apply to step 1 of 15.3.5.4, not step 2?
I suppose so, but was that the intention? Either way, there currently is no test that actually tests step 2.
The intention was definitely to test step 2 which this particular test doesn't hit. Looks like other 'step 2' tests do though:
6 <http://hg.ecmascript.org/tests/test262/file/53c4ade82d14/test/suite/ch15/15.3/15.3.5/15.3.5.4/15.3.5.4_2-2gs.js#l6>
/**
7 <http://hg.ecmascript.org/tests/test262/file/53c4ade82d14/test/suite/ch15/15.3/15.3.5/15.3.5.4/15.3.5.4_2-2gs.js#l7>
-
@path ch15/15.3/15.3.5/15.3.5.4/15.3.5.4_2-2gs.js
-
@description Strict mode - checking access to strict function caller from non-strict function (FunctionDeclaration includes strict directive prologue)
-
@noStrict
-
@negative TypeError
function f() {
15 <http://hg.ecmascript.org/tests/test262/file/53c4ade82d14/test/suite/ch15/15.3/15.3.5/15.3.5.4/15.3.5.4_2-2gs.js#l15>
"use strict";
16 <http://hg.ecmascript.org/tests/test262/file/53c4ade82d14/test/suite/ch15/15.3/15.3.5/15.3.5.4/15.3.5.4_2-2gs.js#l16>
return gNonStrict();
17 <http://hg.ecmascript.org/tests/test262/file/53c4ade82d14/test/suite/ch15/15.3/15.3.5/15.3.5.4/15.3.5.4_2-2gs.js#l17>
}
18 <http://hg.ecmascript.org/tests/test262/file/53c4ade82d14/test/suite/ch15/15.3/15.3.5/15.3.5.4/15.3.5.4_2-2gs.js#l18>
f();
19 <http://hg.ecmascript.org/tests/test262/file/53c4ade82d14/test/suite/ch15/15.3/15.3.5/15.3.5.4/15.3.5.4_2-2gs.js#l19>
20 <http://hg.ecmascript.org/tests/test262/file/53c4ade82d14/test/suite/ch15/15.3/15.3.5/15.3.5.4/15.3.5.4_2-2gs.js#l20>
21 <http://hg.ecmascript.org/tests/test262/file/53c4ade82d14/test/suite/ch15/15.3/15.3.5/15.3.5.4/15.3.5.4_2-2gs.js#l21>
function gNonStrict() {
22 <http://hg.ecmascript.org/tests/test262/file/53c4ade82d14/test/suite/ch15/15.3/15.3.5/15.3.5.4/15.3.5.4_2-2gs.js#l22>
return gNonStrict.caller || gNonStrict.caller.throwTypeError;
23 <http://hg.ecmascript.org/tests/test262/file/53c4ade82d14/test/suite/ch15/15.3/15.3.5/15.3.5.4/15.3.5.4_2-2gs.js#l23>
}
The assumption being made here as you've pointed out is that 'caller' must evaluate to 'true' for this to work. Otherwise, the test will erroneously fail if an implementer set 'caller' to true for some weird reason.
To play devil's advocate, is there any JS engine out there who sets 'caller' to anything other than a function object or is there a way to make a function object evaluate to falsie? I think at worst this test deserves the '@bestPractice' decoration=)
Should be: 'caller' to false :)
Or to null, which is exactly what the new semantics decided to do. ;)
Consider the following code:
With the current spec, this code would legally give g the strict function f as its caller. In ecmascript#310, Allen proposes the obvious fix, which is to special case [[GetOwnProperty]] instead of [[Get]] for function objects in 15.3.5.4. In fact, that is what both V8 and FF already implement.
However, we recently discovered an issue with that semantics. Namely, it causes Object.is{Sealed,Frozen} and Object.{seal,freeze} to spuriously throw when applied to the wrong function at the wrong time. Consider:
d8> function g() { Object.seal(g) }
d8> function f() { "use strict"; g() }
d8> f() (d8):1: TypeError: Illegal access to a strict mode caller function.
(Interestingly, Firefox does not throw on that example, so I'm not sure what semantics it actually implements.)
What can we do here? There does not seem to be a clean fix, only more hacks on top of hacks. It is a bit of a bummer for our implementation of Object.observe, which wants an isFrozen check on its callback.
Thoughts?
Consider the following code: function f() { "use strict"; g() } function g() { var caller = Object.getOwnPropertyDescriptor(g, "caller").value } With the current spec, this code would legally give g the strict function f as its caller. In https://bugs.ecmascript.org/show_bug.cgi?id=310, Allen proposes the obvious fix, which is to special case [[GetOwnProperty]] instead of [[Get]] for function objects in 15.3.5.4. In fact, that is what both V8 and FF already implement. However, we recently discovered an issue with that semantics. Namely, it causes Object.is{Sealed,Frozen} and Object.{seal,freeze} to spuriously throw when applied to the wrong function at the wrong time. Consider: d8> function g() { Object.seal(g) } d8> function f() { "use strict"; g() } d8> f() (d8):1: TypeError: Illegal access to a strict mode caller function. (Interestingly, Firefox does not throw on that example, so I'm not sure what semantics it actually implements.) What can we do here? There does not seem to be a clean fix, only more hacks on top of hacks. It is a bit of a bummer for our implementation of Object.observe, which wants an isFrozen check on its callback. Thoughts? /Andreas