845862 - Transitive Xray waiving misses accessors in certain cases

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

WaiveXrayWrapper::get{Own,}PropertyDescriptor only waives Xray on desc->value. If the result happens to be an accessor property, the transitive waiving chain will be broken, and we'll revert to Xray (this is safe, so it's a correctness problem rather than a security problem).

This generally isn't noticeable, because most property access ends up going through WaiveXrayWrapper::get, which will waive the result after the getter is invoked. But I'm adding some code that invokes GetPropertyDescriptor on a waived object, and we're breaking in that case.

I think the fix here is to wrap returned accessors in a function that invokes the accessor and then waives the result.  We could reify JSPropertyOps or something, but that's kind of a can of worms. I'm tempted to just not handle them.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Transitively apply waivers for accessor descriptors. v1

Comment 2

[Jeff Walden [:Waldo]]

Yeah, wrapping in another function is definitely the right thing to do.  Agreed about reifying JSPropertyOps, too -- I tried it once, it was a mess.

If at some point we ever make it possible to have accessors directly backed by JSNative, I'd consider reifying, but not before.  Most of the trouble of JSPropertyOp is that it doesn't fit within the accessor mold required by ECMAScript.  But if the function pointer is equally functional as a JS function, there's no impedance mismatch, and it becomes much more plausible.  Still it'd be tricky to get right, and refiying a JSNative to a function could be hairy performance-wise.  It's definitely not something I'd want unless done after, or possibly concurrent with, JSPropertyOp removal.

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 719129 [details] [diff] [review]
Transitively apply waivers for accessor descriptors. v1

Given that mrbkap is overloaded and that Waldo has already looked at this bug, I think I can move the review over. ;-)

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 719129 [details] [diff] [review]
Transitively apply waivers for accessor descriptors. v1

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

No good deed goes unpunished.  And have you *seen* my review queue?  I have to be at least as loaded as mrbkap is.  :-)  Probably shouldn't have looked in the first place.  :-)  :-(  But anyway.

The compartment bit here is the only bit that doesn't look good modulo nits.

::: js/xpconnect/wrappers/WaiveXrayWrapper.cpp
@@ +13,5 @@
>  #include "AccessCheck.h"
>  #include "WrapperFactory.h"
>  
> +using namespace JS;
> +using namespace js;

Why these?  I thought tradition was to not do this, and prefix everything, for clarity, or something.  Well, especially for js::.

@@ +48,5 @@
> +{
> +    // If there's no getter, we have nothing to do.
> +    if (!(desc->attrs & JSPROP_GETTER))
> +        return true;
> +    MOZ_ASSERT(desc->getter);

Add an "Xray around a property that has a setter but no getter?" to help out people who hit this.  (It's barely possible for this to happen, I think with Object.defineProperty(..., { get: undefined, set: undefined }) or somesuch.  Craziness, sure, but at least leave breadcrumbs for the fuzzers to follow.  :-) )

@@ +53,5 @@
> +
> +    // Create our wrapper.
> +    JSObject *baseFun = JS_FUNC_TO_DATA_PTR(JSObject *, desc->getter);
> +    JSFunction *outerFun =
> +      NewFunctionWithReserved(cx, CallAndWaive, 0, 0,

I'd rather see a js:: prefix on this, seeing as it's friend API and we want to get rid of it or figure out some better way to do it at some point.

@@ +57,5 @@
> +      NewFunctionWithReserved(cx, CallAndWaive, 0, 0,
> +                              JS_GetGlobalForObject(cx, baseFun), "CallAndWaive");
> +    if (!outerFun)
> +        return false;
> +    SetFunctionNativeReserved(JS_GetFunctionObject(outerFun), 0, ObjectValue(*baseFun));

Could you pull out this 0 into a |const uint32_t BASE_FUNCTION_SLOT = 0;| and use it in CallAndWaive as well?

It looks like |outerFun| could be from a different compartment, judging by "AndWrap" in the previous function called.  Don't you need some wrapping and/or compartment-entering to handle this?  (Both here and for desc->getter, looks like.)

Comment 5

[Bobby Holley (:bholley)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> No good deed goes unpunished.  And have you *seen* my review queue?  I have
> to be at least as loaded as mrbkap is.  :-)  Probably shouldn't have looked
> in the first place.  :-)  :-(  But anyway.

Yeah, but I personally have like 30 patches in blake's review queue. ;-)
 
> ::: js/xpconnect/wrappers/WaiveXrayWrapper.cpp
> > +using namespace JS;
> > +using namespace js;
> 
> Why these?  I thought tradition was to not do this, and prefix everything,
> for clarity, or something.  Well, especially for js::.

Ok, I'll fix.


> Add an "Xray around a property that has a setter but no getter?" to help out
> people who hit this.  (It's barely possible for this to happen, I think with
> Object.defineProperty(..., { get: undefined, set: undefined }) or somesuch. 
> Craziness, sure, but at least leave breadcrumbs for the fuzzers to follow. 
> :-) )

If it can happen, I'll just check for it.


> Could you pull out this 0 into a |const uint32_t BASE_FUNCTION_SLOT = 0;|
> and use it in CallAndWaive as well?

Sure.

> It looks like |outerFun| could be from a different compartment, judging by
> "AndWrap" in the previous function called.  Don't you need some wrapping
> and/or compartment-entering to handle this?  (Both here and for
> desc->getter, looks like.)

We discussed this on IRC. The patch is ok.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 719129 [details] [diff] [review]
Transitively apply waivers for accessor descriptors. v1

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

Um, pretend I never said anything about the wrapping stuff, it's clearly wrong.  :-)  Nitfix and you're good here.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Transitively apply waivers for accessor descriptors. v2

Comment 8

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=c3cf535a3332

Comment 9

[Bobby Holley (:bholley)]

The more recent try push for this was https://tbpl.mozilla.org/?tree=Try&rev=759cf5bbb6e0

This turned one webconsole test orange because that test was relying on some logic in WebConsoleUtils.jsm to determine whether a getter was "native" or not, and and wrapping all the getters in CallAndWaive made them all appear native, even the scripted ones.

This, in turn, made me realize that my previous solution was may more complicated than it needed to be. In particular, we don't need to wrap things in a callable at all - we just need to waive the getters and setters (if they exist), so that their proxy handler becomes WaiveXrayWrapper, and calling them goes through WaiveXrayWrapper::call, which waives the result.

Patch coming up.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Simpler Approach. v3

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 721473 [details] [diff] [review]
Simpler Approach. v3

Yeah, this is clearly the way to go.

Comment 12

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c6ab0e54917

Comment 13

[(no longer active)]

Backed out because of mochitest-5 failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e60919ded783
https://tbpl.mozilla.org/php/getParsedLog.php?id=21369981&tree=Mozilla-Inbound

Comment 14

[Bobby Holley (:bholley)]

Just a stupid osx-only XUL test that should have been a chrome test. Fixed up and repushed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c6d2adc18ca

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c6d2adc18ca