Closed
Bug 800915
Opened 9 years ago
Closed 8 years ago
Refactor wrapper hierarchy so that the result of PUNCTURE can be statically known
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla19
Tracking | Status | |
---|---|---|
firefox-esr17 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(6 files)
14.40 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
7.02 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
8.26 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
100.99 KB,
patch
|
sfink
:
review+
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
17.15 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
6.79 KB,
patch
|
Ms2ger
:
review+
bajaj
:
approval-mozilla-esr17+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
I like the sound of that.
Assignee | ||
Comment 2•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=d1069aff385f
Assignee | ||
Comment 3•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a1d4a6f5c123
Assignee | ||
Comment 4•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=71a65dc79c8c
Assignee | ||
Comment 5•9 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•9 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 7•9 years ago
|
||
Attachment #678297 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #678298 -
Flags: review?(mrbkap)
Assignee | ||
Comment 9•9 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)
![]() |
||
Comment 10•9 years ago
|
||
Nice
Comment 11•9 years ago
|
||
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•9 years ago
|
||
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 13•8 years ago
|
||
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•8 years ago
|
Depends on: CVE-2013-0773
Assignee | ||
Updated•8 years ago
|
Blocks: CVE-2013-0773
No longer depends on: CVE-2013-0773
Comment 14•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #678297 -
Flags: review?(mrbkap) → review+
Updated•8 years ago
|
Attachment #678298 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 15•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/de9fff3a5232
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Assignee | ||
Comment 18•8 years ago
|
||
I think this was an accidental resolution. Reopening on account of the backout.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 19•8 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
Assignee | ||
Comment 20•8 years ago
|
||
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•8 years ago
|
||
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
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 22•8 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•8 years ago
|
||
Attachment #708059 -
Flags: review?(Ms2ger)
Comment 24•8 years ago
|
||
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 25•8 years ago
|
||
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•8 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 27•8 years ago
|
||
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+
Assignee | ||
Comment 28•8 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-esr17/rev/21b73c4b93e1 remote: https://hg.mozilla.org/releases/mozilla-esr17/rev/331ff8fde056
status-firefox-esr17:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•