1052089 - Streamline XPConnect singleton scopes and remove lazy instantiation

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This will be useful for some extra CPOW security that we need.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

This is green.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Sprinkle some assert against using nsContentUtils too early. v1

And earlier version of these patches called nsContentUtils::GetSystemPrincipal()
too early, which returned null, and caused xpc::CreateSandboxObject to create
an nsNullPrincipal. Let's avoid having that happen again.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add a System Principal accessor to nsXPConnect and use it in Sandbox creation. v1

nsContentUtils isn't ready by this point.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Stop using a contractid to create a null principal during sandbox creation. v1

This causes problems when used too early in startup.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Don't require the cx to be in a compartment during sandbox creation. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 5 - Don't fuss around with remote-XUL XBL-scope special cases for sandboxes. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 6 - Make the junk scope invisible to the debugger and Components-less. v1

This prevents the JS engine from trying to fire off debugger notifications and
do other complicated things when we create this thing early in startup in the
upcoming patches.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 7 - Infallibly init singleton scopes in the XPCJSRuntime constructor. v1

We've had some problems with GetJunkScope returning null and causing crashes in
the past, but I'm now pretty convinced that just null-checking it isn't the
answer. Rather than creating it lazily, we should create it at a defined point
in startup. Any problems will then be much more reproducible, and we can track
them down and fix them.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 8 - Rename JunkScope to PrivilegedJunkScope and remove fallibility of singleton scope access. v1

These two things ended up getting mushed together in my tree.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 9 - Swap out the SafeJSContextGlobal for the new UnprivilegedJunkScope. v1

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8474056 [details] [diff] [review]
Part 7 - Infallibly init singleton scopes in the XPCJSRuntime constructor. v1

I'm flagging Bill for review on this stuff because he's the one blocked on it, but I'm also flagging gabor for feedback on the last 3 patches just to make sure he's in the loop.

Comment 14

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8474055 [details] [diff] [review]
Part 6 - Make the junk scope invisible to the debugger and Components-less. v1

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +3557,5 @@
>      if (!mJunkScope) {
>          AutoSafeJSContext cx;
>          SandboxOptions options;
>          options.sandboxName.AssignLiteral("XPConnect Junk Compartment");
> +        options.invisibleToDebugger = true;

why invisible to the debugger?

Comment 15

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8474057 [details] [diff] [review]
Part 8 - Rename JunkScope to PrivilegedJunkScope and remove fallibility of singleton scope access. v1

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

PrivilegedJunkScope sounds super weird... but I don't have any good/better name idea either.

Comment 16

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> why invisible to the debugger?

See comment 9.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8474058 [details] [diff] [review]
Part 9 - Swap out the SafeJSContextGlobal for the new UnprivilegedJunkScope. v1

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +3548,5 @@
>  
> +    // Create the Unprivileged Junk Scope.
> +    SandboxOptions unprivilegedJunkScopeOptions;
> +    unprivilegedJunkScopeOptions.sandboxName.AssignLiteral("XPConnect Junk Compartment");
> +    unprivilegedJunkScopeOptions.invisibleToDebugger = true;

Does this one need Components?

Comment 18

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #17)
> Does this one need Components?

It won't get one regardless of the boolean value, because it's not system or expanded principal.

Thanks for the reviews!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7741d78f083a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d0d758777e46
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9b163c0d80d0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6cb6404e5d91
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/550074532758
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/58e2167bcef6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c4f5dcb57924
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2a2883202fff
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/621470d025e7

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7741d78f083a
https://hg.mozilla.org/mozilla-central/rev/d0d758777e46
https://hg.mozilla.org/mozilla-central/rev/9b163c0d80d0
https://hg.mozilla.org/mozilla-central/rev/6cb6404e5d91
https://hg.mozilla.org/mozilla-central/rev/550074532758
https://hg.mozilla.org/mozilla-central/rev/58e2167bcef6
https://hg.mozilla.org/mozilla-central/rev/c4f5dcb57924
https://hg.mozilla.org/mozilla-central/rev/2a2883202fff
https://hg.mozilla.org/mozilla-central/rev/621470d025e7