877478 - Remove JSContext-specific security managers

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

These don't do anything anymore AFAICT, and the whole GetAppropriateSecurityManager dance doesn't play well with XPCCallContext removal. Let's just make everything use the default security manager (which itself doesn't do all that much).

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove FullTrustSecMan junk. v1

This stuff shouldn't be necessary anymore. The default security manager should
do the right thing given for script running in the scope of a BackstagePass.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove SetSecurityManagerForJSContext and related APIs. v1

The only consumers here were the shells, which we've now fixed.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Move all consumers of GetAppropriateSecurityManager to GetDefaultSecurityManager and rm the former. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove the machinery for security managers in XPCContext. v1

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 755706 [details] [diff] [review]
Part 1 - Remove FullTrustSecMan junk. v1

Hm, looks like this is orange. I'll look into it tomorrow.

Comment 7

[Bobby Holley (:bholley)]

Sigh, the orange is actually indicative of a security bug that our test suite depends on. :-(

Comment 8

[Bobby Holley (:bholley)]

So, the basic issue here is that GetAppropriateSecurityManager actually returns null if !CallerTypeIsJavaScript(), which depends on whether NATIVE_CALLER or JS_CALLER was set on the ccx. But that flag close to meaningless, and there are lots of places where we pass NATIVE_CALLER that can be triggered by JS.

In this case, the big issue is the call to WrapNativeToJSVal in PrepareForUnwrapping creates a ccx with NATIVE_CALLER, effectively meaning that CanCreateInstance checks always succeed. So this means that, if content can get a reference to some chrome-only non-PreCreate (wrapper-per-scope) XPCOM object, wrapping it into the content compartment will succeed, creating a brand new instance of a forbidden object.

This means, for example, that if chrome passes Components.classes to a content window, that content window can use it. The same holds for any other XPCOM object that isn't supposed to be created for content and has one reflector per scope.

Marking as sec-moderate for now, since we don't yet know of an exploit vector. But moz_bug_r_a4 might be able to make this high or crit.

Comment 9

[Bobby Holley (:bholley)]

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

Comment 10

[Bobby Holley (:bholley)]

Spent another day fighting the test suite, let's see if this one's green:
https://tbpl.mozilla.org/?tree=Try&rev=fb4b9a00a6cc

Comment 11

[Bobby Holley (:bholley)]

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

Comment 12

[Bobby Holley (:bholley)]

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

Comment 13

[Bobby Holley (:bholley)]

This is green modulo two failures from the patches in bug 878325. I don't think I actually depend on those patches at this point, so I'm just going to remove them from the queue for now.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 1 - Move crashtests that now error-out to chrome tests. v1

These crashtests now throw, because we run XUL tests in content, and they create
forbidden objects and fail the CanCreateWrapper check. Unfortunately, the
crashtest harness isn't set up to catch onerror. And even if they were, they
wouldn't be testing what they wanted to, because all of the relevant work here
happens in onload.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add a SpecialPowers API for wrapping callbacks and callback objects. v1

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 3 - Fix mochitests with problematic callbacks to use the new API. v1

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 4 - Have SpecialPowers.addObserver modify the observe callback to wrap arguments. v1

|subject| is quite often an XPCOM object that's not accessible to content.

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 5 - Fix test to use SpecialPowers.{add,remove}Observer. v1

This way, we get the callback wrapping behavior for free.

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 6 - Fix tests that depend on creating verboten instances in content. v1

PConnect generally throws when trying to create instances of non-DOM objects
in content. Due to some bugs this has historically worked in certain cases, but
we're fixing those now. So we need to fix the tests that do this sort of thing.

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 7 - Remove FullTrustSecMan junk. v1

This stuff shouldn't be necessary anymore. The default security manager should
do the right thing given for script running in the scope of a BackstagePass.

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 8 - Remove SetSecurityManagerForJSContext and related APIs. v1

The only consumers here were the shells, which we've now fixed.

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 9 - Move all consumers of GetAppropriateSecurityManager to GetDefaultSecurityManager and rm the former. v1

Comment 23

[Bobby Holley (:bholley)]

Attachment: Part 10 - Remove the machinery for security managers in XPCContext. v1

Comment 24

[Bobby Holley (:bholley)]

Attachment: Part 1 - Move crashtests that now error-out to chrome tests. v2

Fixed a small bug in the crashtest move.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 757624 [details] [diff] [review]
Part 2 - Add a SpecialPowers API for wrapping callbacks and callback objects. v1

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

::: testing/specialpowers/content/specialpowersAPI.js
@@ +477,5 @@
> +   * function strips any wrappers if they exist and compare the underlying
> +   * values.
> +   */
> +  compare: function(a, b) {
> +    return unwrapIfWrapped(a) == unwrapIfWrapped(b);

===?

Comment 26

[Bobby Holley (:bholley)]

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

Comment 27

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/832e5392108f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/80aa03b09bfe
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fff2111dd033
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e4b59706ed40
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa378955f9f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f7c0d5b3c6c5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/274244ab9059
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f6eb97809d91
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2a2ad2ce8a4c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/888bbc708061

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/832e5392108f
https://hg.mozilla.org/mozilla-central/rev/80aa03b09bfe
https://hg.mozilla.org/mozilla-central/rev/fff2111dd033
https://hg.mozilla.org/mozilla-central/rev/e4b59706ed40
https://hg.mozilla.org/mozilla-central/rev/8aa378955f9f
https://hg.mozilla.org/mozilla-central/rev/f7c0d5b3c6c5
https://hg.mozilla.org/mozilla-central/rev/274244ab9059
https://hg.mozilla.org/mozilla-central/rev/f6eb97809d91
https://hg.mozilla.org/mozilla-central/rev/2a2ad2ce8a4c
https://hg.mozilla.org/mozilla-central/rev/888bbc708061

Comment 29

[Al Billings [:abillings - ex-MoCo]]

I'm going to assume ESR is unaffected by this issue. Is this correct?

Comment 30

[Bobby Holley (:bholley)]

(In reply to Al Billings [:abillings] from comment #29)
> I'm going to assume ESR is unaffected by this issue. Is this correct?

No, everything is affected, more or less.

Comment 31

[Lukas Blakk [:lsblakk] use ?needinfo]

Affected, but since it's sec-moderate this falls outside of the scope for ESR landing criteria.  Not tracking.