921454 - js_TransplantObjectWithWrapper is untested and broken

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

This is a spin-off from bug 914618 (see bug 914618 comment 5).

The basic problem is that js_TransplantObjectWithWrapper creates a DeadObjectProxy to swap in with the near-dead reflector. But this proxy needs to have the same background-finalize-ity as the reflector, which can be either. So we need a mechanism to specify the right allocKind when creating the DeadObjectProxy.

Doing this elegantly requires some refactoring that depends on the work I did in bug 921448. But I'm also going to make a hacky patch so that we can avoid carrying that baggage to branches.

Exploiting this would require a SOW-bypass (which, admittedly, exist on trunk), as well as some pretty deep GC magic. I'm going to call this sec-high.

Comment 1

[Bobby Holley (:bholley)]

So, upon fixing the initial assertion, I've run into two more. As it turns out, the same-compartment-security-wrapper transplant path is totally busted, and unexercised in our test suite:

http://people.mozilla.org/~choller/firefox/coverage/mc-coverage-20130210-29dd80c95b7d/js/xpconnect/wrappers/WrapperFactory.cpp.gcov.html

I'm a bit surprised by this. It's true that we no longer hit this code in situations where we used to, since (a) Location no longer has a same-compartment-security-wrapper and (b) Nodes (which can have SOWs) are on new-bindings, which means they don't get categorically reparented during document.open.

Nonetheless, I'd still think that adopting a <video> control or something would hit this. Or do we tear down all bindings in the subtree _before_ reparenting a node?

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

(In reply to Bobby Holley (:bholley) from comment #1)
> As it turns
> out, the same-compartment-security-wrapper transplant path is totally
> busted, and unexercised in our test suite:
Turn it into a MOZ_CRASH() and see if anybody notices? ;)

Comment 4

[Bobby Holley (:bholley)]

That's actually a great idea for a branch patch.

Comment 5

[Bobby Holley (:bholley)]

Actually, I think it's probably worth just doing the MOZ_CRASH everywhere, even on trunk. Note that there are still other NAC-related things that _do_ need to work, but I think we can probably get by without transplanting, which will eliminate a pretty large amount of untested code.

At the same time though, I've just done all this work to get this stuff working, and I think it's probably worth getting into the tree in case we need to back out the MOZ_CRASH. So here's what I'm going to do, in order:

1 - Land the code that I have on trunk and let it bake with the test coverage.
2 - Land a MOZ_CRASH on trunk, and remove the test coverage that would trigger it. Let that bake.
3 - Uplift the MOZ_CRASH to branches.

I'll upload the patches to do (1) now.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 1 - Allow chromeOnlyContent to work for non-chrome XBL documents. v1

Given that we turn off XBL for the web, this just affects remote XUL/XBL, in
particular our tests. And it's expedient for NAC testing to be able to test
with an in-content XBL binding.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 2 - Skip unwrap safety assertion for cross-compartment SOWs. v1

Currently this stuff just asserts when you pass NAC across compartments. And
the logic for this stuff is complicated enough that we don't gain a whole
lot from duplicating it.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 3 - Refactor proxy creation to use an options object. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 4 - Add an option for explicitly specifying background finalization type. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 5 - Match the background-finalize-ity of the DeadObjectProxy with that of origobj in js_TransplantObjectWithWrapper. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 6 - Use background-finalize-ity forwarding for all wrappers, not just CCWs. v1

The current setup allows mismatches for same compartment wrappers, which causes
us to assert when transplanting them.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 7 - Invert faulty assertion. v1

This assertion is pretty clearly bogus, and only exists because, as it turns
out, we never had any test coverage for transplanting SCSW objects with
waivers. Even in the world when this stuff landed (bug 773962, which was well
before the removal of Location SCSWs in bug 808608 and the conversion of Nodes
to new bindings), we apparently never tested the waiver path. :-(

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 8 - Tests for NAC. v1

Comment 14

[Jon Coppeard (:jonco)]

Comment on attachment 811927 [details] [diff] [review]
Part 4 - Add an option for explicitly specifying background finalization type. v1

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

::: js/src/jsproxy.cpp
@@ +3168,5 @@
>  
>      NewObjectKind newKind =
>          (clasp == &OuterWindowProxyObject::class_ || options.singleton()) ? SingletonObject : GenericObject;
>      gc::AllocKind allocKind = gc::GetGCObjectKind(clasp);
> +    bool backgroundFinalize = options.backgroundFinalizeSet() ? options.backgroundFinalize()

This allows the caller to override the background finalize state specified by the handler.  This is only safe if we are making something that could be background finalized foreground finalized instead.

(The issue is that some things need to be finalized in the foreground, because their finalizer interacts with the main thread in some way.  However we want to finalize as many things as possible in the background so we can do it off the main thread.)

I don't think this is a problem for what you're doing here because the DeadProxyObject is background finalized by default and these changes will make it foreground finalized if it's necessary to swap it with something else that's foreground finalized (if I understood correctly).

However, I think this means that instead of specifying an option for either background or foreground finalization, we need an option to force foreground finalization if set, or do the existing behaviour if not.

On a side note, BaseProxyHandler::finalizeInBackground() should probably have been called canFinalizeInBackground() to express that foreground finalization is also ok if necessary.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 4 - Add an option for explicitly forcing foreground finalization. v2

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 5 - Match the background-finalize-ity of the DeadObjectProxy with that of origobj in js_TransplantObjectWithWrapper. v2

Comment 17

[Jon Coppeard (:jonco)]

Comment on attachment 811989 [details] [diff] [review]
Part 4 - Add an option for explicitly forcing foreground finalization. v2

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

Looks good!

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Bobby Holley (:bholley) from comment #1)
> Nonetheless, I'd still think that adopting a <video> control or something
> would hit this. Or do we tear down all bindings in the subtree _before_
> reparenting a node?

I'm not sure how torn down they need to be, but we do null out the node's binding member when it gets adopted. Is that enough?

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 811925 [details] [diff] [review]
Part 2 - Skip unwrap safety assertion for cross-compartment SOWs. v1

I'm a little skeptical of the argument "it's too complicated so it isn't worth it to check it" but I'd be happy to see that happen in a followup.

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 811989 [details] [diff] [review]
Part 4 - Add an option for explicitly forcing foreground finalization. v2

Requesting sec-approval for all these patches.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily. It would require an additional exploit for getting ahold of NAC (like bug 914618).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Not directly.

Which older supported branches are affected by this flaw?

all


Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Basically, my plan is to land this on trunk, and then land bug 922009 on trunk (and all branches) shortly thereafter. The idea is that we can probably get away with MOZ_CRASHing if this ever happens, but we want to have a story in case that breaks some extension in a bad way, and it would be nice to bake that story on m-c for a little bit before we remove all this machinery. So I think we should just check this in.

How likely is this patch to cause regressions; how much testing does it need?

Not super likely.

Comment 21

[Bobby Holley (:bholley)]

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

Comment 22

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

Comment on attachment 811989 [details] [diff] [review]
Part 4 - Add an option for explicitly forcing foreground finalization. v2

sec-approval+ for trunk.

Comment 23

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c55b7450097b

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/72797e672d54
https://hg.mozilla.org/mozilla-central/rev/c03caed5328f
https://hg.mozilla.org/mozilla-central/rev/6a194198c85d
https://hg.mozilla.org/mozilla-central/rev/7ea98711b26c
https://hg.mozilla.org/mozilla-central/rev/fb163ed3947e
https://hg.mozilla.org/mozilla-central/rev/52bd45873c87
https://hg.mozilla.org/mozilla-central/rev/896e70eccffb
https://hg.mozilla.org/mozilla-central/rev/c55b7450097b

Comment 25

[Tracy Walker [:tracy]]

is there anything desktop QA can do here in regards to verification?

Comment 26

[Bobby Holley (:bholley)]

(In reply to [:tracy] Tracy Walker - QA Mentor from comment #25)
> is there anything desktop QA can do here in regards to verification?

Nope.

Comment 27

[Daniel Veditz [:dveditz]]

This has baked for 3 weeks now, is it time to land this on Fx26 while it's still Aurora or just wait until after the merge to beta on Monday? Ditto bug 922009

Comment 28

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #27)
> This has baked for 3 weeks now, is it time to land this on Fx26 while it's
> still Aurora or just wait until after the merge to beta on Monday? Ditto bug
> 922009

We should uplift bug 922009, and not this bug.