1355016 - Add support for IsConstructor on wrapped functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

http://searchfox.org/mozilla-central/rev/eace920a0372051a11d8d275bd9b8f14f3024ecd/js/src/proxy/Wrapper.cpp#294-296

This issue is visible when using Reflect.construct:
---
var g = newGlobal();
var w = g.eval("() => {}");
Reflect.construct(Array, [], w);
---

Expected: Throws TypeError
Actual: No TypeError thrown

Comment 1

[Till Schneidereit [:till]]

Attachment: Properly check for constructability of wrapped objects in intrinsic_IsConstructor

I *think* this should be ok: AFAICT checking if the wrapped object is a constructor after doing an unchecked unwrap should have the right behavior. In particular, it should strictly be a more thorough check than we had before.

AFAICT it should be enough to do this in the IsConstructor intrinsic as all other code paths that'd lead to constructing things should already be covered.

bz, asking you for review because I'm not 100% convinced I have the right understanding of the security implications of this.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8867168 [details] [diff] [review]
Properly check for constructability of wrapped objects in intrinsic_IsConstructor

I'm not sure about the security implications either; handing off to bholley.

That said, it's weird to have intrinsic_IsConstructor report a different value from IsConstructor...  Why should this logic be in intrinsic_IsConstructor and not in IsConstructor?

Comment 3

[Till Schneidereit [:till]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #2)
> That said, it's weird to have intrinsic_IsConstructor report a different
> value from IsConstructor...  Why should this logic be in
> intrinsic_IsConstructor and not in IsConstructor?

Not all uses of IsConstructor would usefully do this unwrapping. E.g. the asserts in InternalConstruct want to check if the object in question itself is a constructor. I'll add an IsConstructorUnwrapping and use it in the appropriate places, assuming bholley gives a thumbs up to the general approach.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8867168 [details] [diff] [review]
Properly check for constructability of wrapped objects in intrinsic_IsConstructor

Review of attachment 8867168 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah this is ok - we allow getConstructor on security wrappers in general: http://searchfox.org/mozilla-central/rev/484d2b7f51b7aed035147bbb4a565061659d9278/js/src/jswrapper.h#318

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e32dc83928e5
Properly check for constructability of wrapped objects in intrinsic_IsConstructor. r=bz,bholley

Comment 6

[Till Schneidereit [:till]]

I forgot to land this :(

Comment 7

[André Bargull [:anba]]

There seem to be more places where we need to check for wrapped objects, for example when creating scripted proxies or bound functions:
---
var g = newGlobal();
var w = g.eval("() => {}");
var p = Function.prototype.bind.call(w);
// Also: var p = new Proxy(w, {});
Reflect.construct(Array, [], p);
---

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e32dc83928e5

Comment 9

[Andrew McCreight [:mccr8]]

This is too late for 55.