854480 - Remove usage of old-style security wrapper unwrapping

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Now that GWNOJO is gone, I think we can get rid of XPCWrapper::Unwrap and friends and replace it with usage of js::UnwrapObjectChecked. This better reflects the modern security model and should generally be good defense-in-depth.

The main difference at present is that js::UnwrapObjectChecked refuses to unwrap COWs, whereas XPCWrapper::Unwrap will unwrap them. I think I have this covered by a special check in XPCConvert, but I might need to temporarily sprinkle that magic in a few other places as well.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove old-style unwrapping from dom/bindings bindings. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove old-style unwrapping from nsDOMClassInfo. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove old-style unwrapping from QuickStubs. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove old-style unwrapping from XPCWrappedJSClass. v1

I don't claim to really understand why we can end up with a security wrapper
here, but this change should be equivalent.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 5 - Remove old-style unwrapping from GetNativeOfWrapper. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 6 - Remove old-style unwrapping infrastructure. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 7 - Remove SCRIPT_ACCESS_ONLY. v1

Comment 9

[Bobby Holley (:bholley)]

(I've fixed the two try oranges in these patches btw)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 731009 [details] [diff] [review]
Part 1 - Remove old-style unwrapping from dom/bindings bindings. v1

> +++ b/dom/bindings/BindingUtils.h

>+    obj = js::UnwrapObjectChecked(obj, /* stopAtOuter = */ false);

This no longer has the SOW issues we used to have?

>+      // ObjectClassIs for this stuff and we can just kill this code.

Do please file a bug on that?

r=me

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 731012 [details] [diff] [review]
Part 4 - Remove old-style unwrapping from XPCWrappedJSClass. v1

I don't remember why I added that unwrap call there anymore... I made the change in the bug that added SOWs, and that stuff has all changed enough by now that we might be able to remove it.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #10)
> This no longer has the SOW issues we used to have?

UnwrapObjectChecked is now dynamic for SOWs (though only when XBL scopes are disabled).


> >+      // ObjectClassIs for this stuff and we can just kill this code.
> 
> Do please file a bug on that?

Filed bug 856833.

Comment 13

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/623c8b11f31f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b8a91b194ba6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/544c73301d31
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/83ae75a544fb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/18257899ab4b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a6653dadb7f8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/64771564b5bc

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/623c8b11f31f
https://hg.mozilla.org/mozilla-central/rev/b8a91b194ba6
https://hg.mozilla.org/mozilla-central/rev/544c73301d31
https://hg.mozilla.org/mozilla-central/rev/83ae75a544fb
https://hg.mozilla.org/mozilla-central/rev/18257899ab4b
https://hg.mozilla.org/mozilla-central/rev/a6653dadb7f8
https://hg.mozilla.org/mozilla-central/rev/64771564b5bc