Closed
Bug 823348
Opened 12 years ago
Closed 12 years ago
Security Wrapper Housecleaning
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(14 files)
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
I just discovered that they currently go through IsTransparent stuff on Xray wrapper, which is just wrong (because the waive isn't deep). I'm in the middle of some other stuff, so I'll fix this separately.
(It's a shame - this bug would let us get rid of the IsTransparent() case on XrayWrapper, except that I'm adding a new use for that case in bug 821850).
Assignee | ||
Comment 1•12 years ago
|
||
My patches for this have broadened a bit, so updating the title here.
Blocks: XBL-scopes
Summary: Xray waivers for non-chrome actors should use WaiveXrayWrapper → Security Wrapper Housecleaning
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Hm, there's a browser-chrome failure that I can't reproduce locally. Let's see what happens on mac:
https://tbpl.mozilla.org/?tree=Try&rev=c6fb7ce793bb
Assignee | ||
Comment 4•12 years ago
|
||
Oops, it's "macosx64", not "macosx":
https://tbpl.mozilla.org/?tree=Try&rev=ac3e1ce17133
Assignee | ||
Comment 5•12 years ago
|
||
Fixed that bug, should be ready. Uploading patches and flagging blake for review.
Aside from general refactoring and cleanup, these patches make nsExpandedPrincipal(foo) have a proper asymmetric relationship with foo, so it blocks bug 821850.
Assignee | ||
Comment 6•12 years ago
|
||
Every time the layout of CompartmentPrivate changes, I forget to rebuild in
caps/ and spend half an hour wondering what the heck is going on. :-(
Attachment #695868 -
Flags: review?(mrbkap)
Assignee | ||
Comment 7•12 years ago
|
||
There's a browser-chrome test that does this, which means that _all_ subsequent
browser-chrome tests inherit it. So depending on the ordering of cases in
WrapperFactory, we might end up using a CrossCompartmentWrapper rather than an
XrayWrapper, meaning that stuff like nodePrincipal doesn't work anymore.
The semantics of UniversalXPConnect are now entirely dicatated by what makes
our test suite go green. So let's not force ourselves to bend over backwards
during wrapping to handle this case. And let's fix that stupid test while
we're at it.
Attachment #695869 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•12 years ago
|
||
__scriptOnly__ is unused on mxr and addons-mxr. Morevoer, the current
implementation is totally broken, because we check for NNXOW, which only
happens when a random content JS object ends up in some other cross-origin
scope (via addons, presumably), whereas chrome objects use ChomeObjectWrapper.
I'm soon going to replace SCRIPT_ACCESS_ONLY with checked unwrapping, and mark
all COWs as unsafe to unwrap (see bug 821573 and bug 658909). So let's just kill
this thing here.
Attachment #695870 -
Flags: review?(mrbkap)
Assignee | ||
Comment 9•12 years ago
|
||
I noticed this nonfatal assertion firing, unrelated to my patches. Leaking
the holder is not so great. Let's fix this for real.
Attachment #695871 -
Flags: review?(mrbkap)
Assignee | ||
Comment 10•12 years ago
|
||
There's no reason to be doing a dynamic check here, given that the JSClasses
will never match. Lets be explicit and safe.
Attachment #695872 -
Flags: review?(mrbkap)
Assignee | ||
Comment 11•12 years ago
|
||
wantXrays means that the sandbox wants Xray wrappers even when accessing same-
origin content. The default is true, which Blake says has something to do with
GreaseMonkey and days of old.
This flag never had an effect for chrome, because the chrome->chrome case always
short-circuited to &CrossCompartmentWrapper::singleton. But once we start
respecting the flag as a general-purpose indicator that Xrays should be applied
same-origin, we need to either add a special case in Rewrap or make the flag reflect
reality. The latter seems cleaner and more sane.
However, things are complicated by the fact that there's also a completely different,
orthogonal usage, whereby setting wantXrays to false implicitly waives Xray on the
returned sandbox _and_ on any results returned from evalInSandbox. This is just nuts.
The former can be accomplished by callers manually using .wrappedJSObject, and the
latter by having EvalInSandbox transitively apply waivers from their sandbox arguments.
I've updated the documentation on the MDN page so that it only describes the
reasonable usage. The next step is to get rid of the crazy behavior. I think the
best path of migration is to have wantXrays: false keep implicitly waiving, but
waive return values from EvalInSandbox based on whether the argument was waived. This
patch does that.
Attachment #695874 -
Flags: review?(mrbkap)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #695875 -
Flags: review?(mrbkap)
Assignee | ||
Comment 13•12 years ago
|
||
There's no reason to do this any different than we do for XOWs and such. The
only thing this might conceivably support would be certain chrome XPWNs-as-COWs.
But that would require that they forced a parent in precreate without being
flagged as DOM objects in classinfo. And it's not clear why we'd want to support
that. And we're generally moving away from COWs anyway.
Attachment #695876 -
Flags: review?(mrbkap)
Assignee | ||
Comment 14•12 years ago
|
||
This is generally cleaner, and avoids potentially calling these functions
multiple times when we start moving this stuff around.
Attachment #695878 -
Flags: review?(mrbkap)
Assignee | ||
Comment 15•12 years ago
|
||
It's pretty orthogonal, and makes the critical block more complicated than it
needs to be.
Attachment #695879 -
Flags: review?(mrbkap)
Assignee | ||
Comment 16•12 years ago
|
||
This paves the way for more rule-based selection of wrappers in the common case.
Attachment #695880 -
Flags: review?(mrbkap)
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #695881 -
Flags: review?(mrbkap)
Assignee | ||
Comment 18•12 years ago
|
||
All the casese where we want to waive should now be going through WaiveXrayWrapper.
Attachment #695882 -
Flags: review?(mrbkap)
Assignee | ||
Comment 19•12 years ago
|
||
We tack these onto the tests from bug 812415, adding coverage for
nsExpandedPrincipal and making sure that the waivers are deep.
We also take the opportunity to check the asymmetric security
relationship between a principal and its corresponding nsEP.
Attachment #695883 -
Flags: review?(mrbkap)
Comment 20•12 years ago
|
||
Comment on attachment 695882 [details] [diff] [review]
Part 13 - Stop checking for Xray waivers in the Xray machinery. v1
Review of attachment 695882 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1239,5 @@
>
> bool
> IsTransparent(JSContext *cx, JSObject *wrapper)
> {
> + return false;
So, uh, why keep this function?
Assignee | ||
Comment 21•12 years ago
|
||
(In reply to :Ms2ger from comment #20)
> Comment on attachment 695882 [details] [diff] [review]
> Part 13 - Stop checking for Xray waivers in the Xray machinery. v1
>
> Review of attachment 695882 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: js/xpconnect/wrappers/XrayWrapper.cpp
> @@ +1239,5 @@
> >
> > bool
> > IsTransparent(JSContext *cx, JSObject *wrapper)
> > {
> > + return false;
>
> So, uh, why keep this function?
Because I'm using it again in bug 821850.
Comment 22•12 years ago
|
||
I haven't forgot about this -- I've been super busy with b2g stuff and will be until next week.
Updated•12 years ago
|
Attachment #695868 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #695869 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #695870 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #695871 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #695872 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #695874 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #695875 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #695876 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #695878 -
Flags: review?(mrbkap) → review+
Comment 23•12 years ago
|
||
Comment on attachment 695879 [details] [diff] [review]
Part 10 - Move COW prototype remapping out of wrapper selection. v1
Review of attachment 695879 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with the question answered.
::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ -355,5 @@
> }
> } else if (xpc::IsUniversalXPConnectEnabled(target)) {
> wrapper = &CrossCompartmentWrapper::singleton;
> } else if (originIsChrome) {
> - JSFunction *fun = JS_GetObjectFunction(obj);
Why bother moving this?
@@ +418,5 @@
> + // The prototype chain of COW(obj) looks lke this:
> + //
> + // COW(obj) => COW(foo) => COW(bar) => contentWin.StandardClass.prototype
> + if (wrapper == &ChromeObjectWrapper::singleton) {
> +
Nit: extra newline here.
Attachment #695879 -
Flags: review?(mrbkap) → review+
Comment 24•12 years ago
|
||
Comment on attachment 695880 [details] [diff] [review]
Part 11 - Hoist special cases to the top of WrapperFactory::Rewrap. v1
Review of attachment 695880 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +351,5 @@
> +
> + // If content is accessing a Components object or NAC, we need a special filter,
> + // even if the object is same origin.
> + } else if (IsComponentsObject(obj) && !AccessCheck::isChrome(target)) {
> + wrapper = &FilteringWrapper<CrossCompartmentSecurityWrapper,
This branch and the one below it are over-indented.
Attachment #695880 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 25•12 years ago
|
||
(In reply to Blake Kaplan (:mrbkap) from comment #24)
> Comment on attachment 695879 [details] [diff] [review]
> Part 10 - Move COW prototype remapping out of wrapper selection. v1
>
> Review of attachment 695879 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> r=me with the question answered.
>
> ::: js/xpconnect/wrappers/WrapperFactory.cpp
> @@ -355,5 @@
> > }
> > } else if (xpc::IsUniversalXPConnectEnabled(target)) {
> > wrapper = &CrossCompartmentWrapper::singleton;
> > } else if (originIsChrome) {
> > - JSFunction *fun = JS_GetObjectFunction(obj);
>
> Why bother moving this?
Because I want wrapper computation to be as succinct as possible. I don't consider that check to be very important to someone skimming over our wrapper behavior, so I wanted to move it to the "Junk we do for COWs" section, below.
Comment 26•12 years ago
|
||
Comment on attachment 695881 [details] [diff] [review]
Part 12 - Replace security wrapper enumeration with a more rule-based approach. v1
Review of attachment 695881 [details] [diff] [review]:
-----------------------------------------------------------------
Very nice patch. I'm not entirely convinced that this is easier to understand or to tweak but I see why it's useful.
Attachment #695881 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #695882 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #695883 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 27•12 years ago
|
||
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c356aef54656
This was green on try (comment 2, comment 5), and I've also pushed other try pushes on top of it, such as bug 821850 comment 12.
Comment 28•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7198eb5958a2
https://hg.mozilla.org/mozilla-central/rev/a6059bb2bc6c
https://hg.mozilla.org/mozilla-central/rev/1e063f6c170a
https://hg.mozilla.org/mozilla-central/rev/d8c8bfd7f3e4
https://hg.mozilla.org/mozilla-central/rev/1aa557c6712b
https://hg.mozilla.org/mozilla-central/rev/228181ec5626
https://hg.mozilla.org/mozilla-central/rev/c5d438850b86
https://hg.mozilla.org/mozilla-central/rev/29eae99e14f7
https://hg.mozilla.org/mozilla-central/rev/96b499530382
https://hg.mozilla.org/mozilla-central/rev/588adb5ab729
https://hg.mozilla.org/mozilla-central/rev/43be433d5832
https://hg.mozilla.org/mozilla-central/rev/309a0c60b871
https://hg.mozilla.org/mozilla-central/rev/16afc05bc402
https://hg.mozilla.org/mozilla-central/rev/c356aef54656
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Updated•12 years ago
|
Assignee: nobody → bobbyholley+bmo
You need to log in
before you can comment on or make changes to this bug.
Description
•