800915 - Refactor wrapper hierarchy so that the result of PUNCTURE can be statically known

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

See bug 792280 comment 17 for the motivation here. Basically, I want to simplify all the checked unwrapping stuff so that we can know whether a wrapper is safe to unwrap (from a security perspective) just by statically examining it.

This will also involve a bit of an overdue refactor of the proxy hierarchy.

Comment 1

[Luke Wagner [:luke]]

I like the sound of that.

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

Green! Uploading patches and flagging for review.

The big win of these patches is that we no longer need a cx to determine whether we can safely unwrap an object. This has a major impact of the typed array API.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 1 - Clarify and refine the semantics of SecurityWrapper so that it is used if and only if unwrapping is unsafe. v1

The naming scheme for Xray typedefs is the concatenation of the tuple:
({SC,}, {Security,Permissive}, Xray, {XPCWN,DOM}). This is admittedly a bit
much, but I think it's still better than explicitly doing the "typdef Foo Xray"
everywhere. Moreover, once the new DOM bindings are done, the last component
in the tuple will go away.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add infrastructure to flag security wrappers as unsafe to unwrap. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 3 - Reimplement PUNCTURE consumers in terms of isSafeToUnwrap() and remove PUNCTURE API. v3

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove the cx parameter and simplify various APIs. v1

If callers want to throw, it's now their responsibility.

Flagging sfink for the Spidermonkey changes, bz for the DOM changes.

Comment 10

[Luke Wagner [:luke]]

Nice

Comment 11

[Steve Fink [:sfink] [:s:]]

Comment on attachment 678298 [details] [diff] [review]
Part 3 - Reimplement PUNCTURE consumers in terms of isSafeToUnwrap() and remove PUNCTURE API. v3

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

::: js/src/jswrapper.cpp
@@ +202,5 @@
>      GET(DirectProxyHandler::enumerate(cx, wrapper, props));
>  }
>  
>  /*
> + * Ordinarily, the convert trap would require unwrapping  However, the default

Missing period.

Comment 12

[Steve Fink [:sfink] [:s:]]

Comment on attachment 678299 [details] [diff] [review]
Part 4 - Remove the cx parameter and simplify various APIs. v1

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

::: js/src/jswrapper.cpp
@@ -96,2 @@
>  {
> -    RootedObject obj(cx, objArg);

Leave the parameter a RawObject. We use that as a marker to indicate that we've audited the code and determined that the parameter does not need to be rooted.

You can still rename it to plain |obj|, though.

@@ -103,5 @@
>      }
>  }
>  
>  JS_FRIEND_API(JSObject *)
> -js::UnwrapOneChecked(JSContext *cx, HandleObject obj)

Change obj from HandleObject to RawObject instead of JSObject*, please.

Though this is a little confusing -- I guess you changed the body of this function in an earlier patch in this bug? Because it use to do a scary

  handler->enter(cx, obj, JSID_VOID, Wrapper::PUNCTURE, &rvOnFailure)

for which it seems like a HandleObject is a better idea. Lemme dig through the other patches...

Ah, ok, this now just checks a flag. So RawObject is fine.

::: js/src/jswrapper.h
@@ +251,5 @@
>  // the security wrapper has the opportunity to veto the unwrap. Since checked
>  // code should never be unwrapping outer window wrappers, we always stop at
>  // outer windows.
>  JS_FRIEND_API(JSObject *)
> +UnwrapObjectChecked(JSObject *obj);

RawObject

@@ +256,5 @@
>  
>  // Unwrap only the outermost security wrapper, with the same semantics as
>  // above. This is the checked version of Wrapper::wrappedObject.
>  JS_FRIEND_API(JSObject *)
> +UnwrapOneChecked(JSObject *obj);

RawObject

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 678299 [details] [diff] [review]
Part 4 - Remove the cx parameter and simplify various APIs. v1

Followup is fine for all of the following nits:

>+++ b/content/base/src/nsDOMBlobBuilder.cpp
> BlobSet::AppendArrayBuffer(JSObject* aBuffer, JSContext *aCx)

Nix the now-unused aCx argument, please.

>+++ b/content/base/src/nsDOMDataChannel.cpp
>@@ -312,19 +312,19 @@ nsDOMDataChannel::GetSendParams(nsIVariant* aData, nsCString& aStringOut,

This also no longer needs aCx.  And neither does the Send() API method, and it can go away in the IDL.

>+++ b/content/base/src/nsXMLHttpRequest.cpp
>@@ -2666,18 +2666,18 @@ GetRequestBody(nsIVariant* aBody, nsIInputStream** aResult, uint64_t* aContentLe
>       ac.construct(cx, obj);

This is no longer needed, since we do nothing with cx while ac is in scope.

In fact, I believe ac itself is no longer needed, and at first glance cx is also now unused and all the code that gets and pushes it can go away, together with the nsCxPusher.

>+++ b/content/canvas/src/CanvasRenderingContext2D.cpp
>@@ -4341,17 +4341,17 @@ CanvasRenderingContext2D::PutImageData(JSContext* cx,

This can stop taking a JSContext argument, with the corresponding change to Bindings.conf, yay.

>+++ b/content/canvas/src/WebGLContextGL.cpp
>@@ -4866,34 +4866,34 @@ WebGLContext::TexImage2D(JSContext* cx, WebGLenum target, WebGLint level,

This can stop taking a JSContext argument (both overloads of it).

>@@ -5020,33 +5020,33 @@ WebGLContext::TexSubImage2D(JSContext* cx, WebGLenum target, WebGLint level,

Likewise.

r=me on the parts outside js/src, with the above either dealt with or followups filed.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 678296 [details] [diff] [review]
Part 1 - Clarify and refine the semantics of SecurityWrapper so that it is used if and only if unwrapping is unsafe. v1

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1728,5 @@
>  }
>  
> +/*
> + * The Permissive / Security variants should be used depending on whether the
> + * wrapper is guranteed to subsume the wrappee (i.e. - whether it is safe from

I'm not sure if it's just me, but I got caught up on the wrapper unwrapping the callee. I think it would be clearer to say the wrapper's compartment is guaranteed to subsume the wrappee's compartment. Does that actually change anything?

Comment 15

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c7499faaec23
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8e3d976d5dc5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c567df2244f5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/de9fff3a5232

Comment 16

[[Back August 16] Ryan VanderMeulen]

Unfortunately, this had to be backed out for build bustage on OSX, Android, and b2g.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f60b494448f8

https://tbpl.mozilla.org/php/getParsedLog.php?id=16972844&tree=Mozilla-Inbound

Undefined symbols for architecture i386:
  "xpc::XrayWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>, xpc::XPCWrappedNativeXrayTraits>::XrayWrapper(unsigned int)", referenced from:
      __GLOBAL__I_a in FilteringWrapper.o
  "xpc::XrayWrapper<js::SecurityWrapper<js::Wrapper>, xpc::XPCWrappedNativeXrayTraits>::~XrayWrapper()", referenced from:
      xpc::FilteringWrapper<xpc::XrayWrapper<js::SecurityWrapper<js::Wrapper>, xpc::XPCWrappedNativeXrayTraits>, xpc::LocationPolicy>::~FilteringWrapper() in FilteringWrapper.o
      xpc::FilteringWrapper<xpc::XrayWrapper<js::SecurityWrapper<js::Wrapper>, xpc::XPCWrappedNativeXrayTraits>, xpc::LocationPolicy>::~FilteringWrapper() in FilteringWrapper.o
  "xpc::XrayWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>, xpc::DOMXrayTraits>::~XrayWrapper()", referenced from:
      xpc::FilteringWrapper<xpc::XrayWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>, xpc::DOMXrayTraits>, xpc::CrossOriginAccessiblePropertiesOnly>::~FilteringWrapper() in FilteringWrapper.o
      xpc::FilteringWrapper<xpc::XrayWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>, xpc::DOMXrayTraits>, xpc::CrossOriginAccessiblePropertiesOnly>::~FilteringWrapper() in FilteringWrapper.o
  "xpc::XrayWrapper<js::SecurityWrapper<js::Wrapper>, xpc::XPCWrappedNativeXrayTraits>::XrayWrapper(unsigned int)", referenced from:
      __GLOBAL__I_a in FilteringWrapper.o
  "xpc::XrayWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>, xpc::XPCWrappedNativeXrayTraits>::~XrayWrapper()", referenced from:
      xpc::FilteringWrapper<xpc::XrayWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>, xpc::XPCWrappedNativeXrayTraits>, xpc::CrossOriginAccessiblePropertiesOnly>::~FilteringWrapper() in FilteringWrapper.o
      xpc::FilteringWrapper<xpc::XrayWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>, xpc::XPCWrappedNativeXrayTraits>, xpc::CrossOriginAccessiblePropertiesOnly>::~FilteringWrapper() in FilteringWrapper.o
      xpc::FilteringWrapper<xpc::XrayWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>, xpc::XPCWrappedNativeXrayTraits>, xpc::LocationPolicy>::~FilteringWrapper() in FilteringWrapper.o
      xpc::FilteringWrapper<xpc::XrayWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>, xpc::XPCWrappedNativeXrayTraits>, xpc::LocationPolicy>::~FilteringWrapper() in FilteringWrapper.o
  "xpc::XrayWrapper<js::SecurityWrapper<js::CrossCompartmentWrapper>, xpc::DOMXrayTraits>::XrayWrapper(unsigned int)", referenced from:
      __GLOBAL__I_a in FilteringWrapper.o
ld: symbol(s) not found for architecture i386

Comment 17

[Ed Morley [:emorley]]

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

Comment 18

[Bobby Holley (:bholley)]

I think this was an accidental resolution. Reopening on account of the backout.

Comment 19

[Bobby Holley (:bholley)]

After hours of bootstrapping a b2g emulator build, fighting with the compiler, and template munging, let's try again:

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

Comment 20

[Bobby Holley (:bholley)]

Evergreen. Pushed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ca4fd0c6c3d5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a9353f2e3d50
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/850c0b306b6e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f8620fc3f18f

Comment 21

[[Back August 16] Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/ca4fd0c6c3d5
https://hg.mozilla.org/mozilla-central/rev/a9353f2e3d50
https://hg.mozilla.org/mozilla-central/rev/850c0b306b6e
https://hg.mozilla.org/mozilla-central/rev/f8620fc3f18f

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 1 - ESR17 backport. v1

This is a very tricky backport, because there's are various changes which make
the patch not apply directly. Specifically:

1 - ESR17 Still has DOMProxy Xrays, which didn't exist for the original patch.
2 - ESR17 doesn't have the Xray refactoring from bug 761695, which means that we
    have to do explicitly set usingXray = true whenever we select an XPWN Xray.
3 - ESR17 still has the DirectWrapper/IndirectWrapper crap.

Ms2ger said he's willing to take a stab at reviewing this.

Commit message of the original patch:

The naming scheme for Xray typedefs is the concatenation of the tuple:
({SC,}, {Security,Permissive}, Xray, {XPCWN,DOM}). This is admittedly a bit
much, but I think it's still better than explicitly doing the "typdef Foo Xray"
everywhere. Moreover, once the new DOM bindings are done, the last component
in the tuple will go away.

Comment 23

[Bobby Holley (:bholley)]

Attachment: Part 2 - ESR17 backport. v1

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 708058 [details] [diff] [review]
Part 1 - ESR17 backport. v1

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

No obvious bugs.

::: js/xpconnect/wrappers/FilteringWrapper.cpp
@@ +150,5 @@
> +#define NNXOW FilteringWrapper<CrossCompartmentSecurityWrapper, CrossOriginAccessiblePropertiesOnly>
> +#define LW    FilteringWrapper<SCSecurityXrayXPCWN, LocationPolicy>
> +#define XLW   FilteringWrapper<SecurityXrayXPCWN, LocationPolicy>
> +#define CW FilteringWrapper<SameCompartmentSecurityWrapper, ComponentsObjectPolicy>
> +#define XCW FilteringWrapper<CrossCompartmentSecurityWrapper, ComponentsObjectPolicy>

Indentation follows a rule I don't understand (but so did the original patch)

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 708059 [details] [diff] [review]
Part 2 - ESR17 backport. v1

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

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +321,5 @@
> +                        JSCompartment *origin, JSCompartment *target)
> +{
> +    typedef FilteringWrapper<CrossCompartmentSecurityWrapper, OnlyIfSubjectIsSystem> XSOW;
> +
> +    if (AccessCheck::isChrome(target)) {

bholley tells me that the missing xpc::IsUniversalXPConnectEnabled call is intentional.

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 708059 [details] [diff] [review]
Part 2 - ESR17 backport. v1

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

This is a piece of security wrapper infrastructure that we're coming to rely on more and more, and it's starting to be a problem when backporting security fixes. In particular, we need this to backport bug 809652.

User impact if declined:

Inability to backport various security fixes
 
Fix Landed on Version:

Mozilla19

Risk to taking this patch (and alternatives if risky):

Risky patch in general. Not really any alternatives if we want to take the sec fixes. We should give it plenty of time to bake.
 
String or UUID changes made by this patch: 

None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 27

[bhavana bajaj [:bajaj]]

Comment on attachment 708059 [details] [diff] [review]
Part 2 - ESR17 backport. v1

Considering there are no real alternatives and this is needed for 809652 and may be needed in future to backport security fixes like these, there is no better time than now, approving on esr17.

Please make sure to land by 2/11 & ping QA(mwobensmith) to assist with any additional verification needed here or 809652

Comment 28

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-esr17/rev/21b73c4b93e1
remote:   https://hg.mozilla.org/releases/mozilla-esr17/rev/331ff8fde056