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

RESOLVED FIXED in Firefox -esr17

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr17 fixed)

Details

Attachments

(6 attachments)

Assignee

Description

7 years ago
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.
I like the sound of that.
Assignee

Updated

7 years ago
Depends on: 803068
Assignee

Comment 5

7 years ago
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.
Assignee

Comment 6

7 years ago
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.
Attachment #678296 - Flags: review?(mrbkap)
Assignee

Comment 9

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

Flagging sfink for the Spidermonkey changes, bz for the DOM changes.
Attachment #678299 - Flags: review?(sphink)
Attachment #678299 - Flags: review?(bzbarsky)
Nice
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 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
Attachment #678299 - Flags: review?(sphink) → review+
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.
Attachment #678299 - Flags: review?(bzbarsky) → review+
Assignee

Updated

7 years ago
Depends on: CVE-2013-0773
Assignee

Updated

7 years ago
No longer depends on: CVE-2013-0773
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?
Attachment #678296 - Flags: review?(mrbkap) → review+
Attachment #678297 - Flags: review?(mrbkap) → review+
Attachment #678298 - Flags: review?(mrbkap) → review+
Assignee

Updated

7 years ago
Blocks: 811152
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
https://hg.mozilla.org/mozilla-central/rev/de9fff3a5232
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Assignee

Comment 18

7 years ago
I think this was an accidental resolution. Reopening on account of the backout.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 19

7 years ago
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
Blocks: 812104
Assignee

Comment 22

7 years ago
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.
Attachment #708058 - Flags: review?(Ms2ger)
Assignee

Comment 23

7 years ago
Attachment #708059 - Flags: review?(Ms2ger)
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)
Attachment #708058 - Flags: review?(Ms2ger) → review+
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.
Attachment #708059 - Flags: review?(Ms2ger) → review+
Assignee

Comment 26

7 years ago
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.
Attachment #708059 - Flags: approval-mozilla-esr17?
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
Attachment #708059 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
You need to log in before you can comment on or make changes to this bug.