Closed Bug 921454 Opened 6 years ago Closed 6 years ago

js_TransplantObjectWithWrapper is untested and broken

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- wontfix
firefox27 + fixed
firefox-esr17 --- wontfix
firefox-esr24 26+ wontfix
b2g18 --- wontfix

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: sec-high, Whiteboard: [qa-][to be fixed in old branches by bug 922009][adv-main27+][embargo until B2G1.1 EOL])

Attachments

(8 files, 2 obsolete files)

1.31 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.70 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
13.45 KB, patch
jonco
: review+
Details | Diff | Splinter Review
2.38 KB, patch
jonco
: review+
Details | Diff | Splinter Review
1.44 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.08 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.65 KB, patch
jonco
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
3.12 KB, patch
jonco
: review+
Details | Diff | Splinter Review
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.
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?
Flags: needinfo?(mrbkap)
Summary: Assertion failure: IsBackgroundFinalized(...) == IsBackgroundFinalized(...) when transplanting NAC → js_TransplantObjectWithWrapper is untested and broken
(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? ;)
That's actually a great idea for a branch patch.
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.
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.
Attachment #811924 - Flags: review?(bugs)
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.
Attachment #811925 - Flags: review?(mrbkap)
The current setup allows mismatches for same compartment wrappers, which causes
us to assert when transplanting them.
Attachment #811930 - Flags: review?(jcoppeard)
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. :-(
Attachment #811931 - Flags: review?(mrbkap)
Attachment #811932 - Flags: review?(mrbkap)
Blocks: 922009
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.
Attachment #811927 - Flags: review?(jcoppeard)
Attachment #811927 - Attachment is obsolete: true
Attachment #811989 - Flags: review?(jcoppeard)
Attachment #811926 - Flags: review?(jcoppeard) → review+
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!
Attachment #811989 - Flags: review?(jcoppeard) → review+
Attachment #811990 - Flags: review?(jcoppeard) → review+
Attachment #811930 - Flags: review?(jcoppeard) → review+
Attachment #811924 - Flags: review?(bugs) → review+
(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?
Flags: needinfo?(mrbkap)
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.
Attachment #811925 - Flags: review?(mrbkap) → review+
Attachment #811931 - Flags: review?(mrbkap) → review+
Attachment #811932 - Flags: review?(mrbkap) → review+
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.
Attachment #811989 - Flags: sec-approval?
Comment on attachment 811989 [details] [diff] [review]
Part 4 - Add an option for explicitly forcing foreground finalization. v2

sec-approval+ for trunk.
Attachment #811989 - Flags: sec-approval? → sec-approval+
is there anything desktop QA can do here in regards to verification?
(In reply to [:tracy] Tracy Walker - QA Mentor from comment #25)
> is there anything desktop QA can do here in regards to verification?

Nope.
Whiteboard: [qa-]
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
(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.
Flags: needinfo?(bobbyholley+bmo)
Whiteboard: [qa-] → [qa-][to be fixed in old branches by bug 922009]
Whiteboard: [qa-][to be fixed in old branches by bug 922009] → [qa-][to be fixed in old branches by bug 922009][adv-main27+][embargo until B2G1.1 EOL]
Group: core-security
You need to log in before you can comment on or make changes to this bug.