1173944 - Object subclasses are missing barriers again

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

I fixed this a few months ago, but could not come up with any way to assert correct behavior without doing an indirect call in a fast path. Well, it regressed again, so I think we have to add the call despite the fact that this is going to be a substantial slowdown in debug. This is just too subtle to get right reliably and too dangerous to let slip.

I've got a compile-time debug only assertion compiling in all normal builds, but it seems our test infra is not directly pulling in the spidermonkey shared object, so I'm having trouble getting the tests linking there. I will post the patch when I get it building.

Also, doing the research to figure out what branches are affected is going to require custom backports of the assertion, and therefore, be extremely time-consuming. I will set tracking flags and upload backport patches as I work through the branches. Uhg.

Comment 1

[Terrence Cole [:terrence]]

Attachment: share_GCMethods_for_object_subclasses-vWIP.diff

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8621225 [details] [diff] [review]
share_GCMethods_for_object_subclasses-vWIP.diff

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

Seems like a good thing to have. We hashed over some different ways to do this on IRC.

Comment 3

[Terrence Cole [:terrence]]

Attachment: share_GCMethods_for_object_subclasses-v0.diff

I think this is more or less ready. The prior try run broke because of wrong include orders, but the tests were all green and at least one platform built successfully. New try run up at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce0d0c7e420b.

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8622692 [details] [diff] [review]
share_GCMethods_for_object_subclasses-v0.diff

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

Crazy stuff. If the compiler and your tests are good with it, I'm good with it.

::: js/public/RootingAPI.h
@@ +604,5 @@
>      static ThingRootKind rootKind() { return T::rootKind(); }
>  };
>  
> +#if defined(DEBUG) && defined(EXPORT_JS_API)
> +// The problem is that templates do not have any concept of class heirarchy.

*hierarchy

Comment 5

[Terrence Cole [:terrence]]

Rating this as sec moderate. It would potentially allow an attacker to read unexpected memory, but only from the nursery.

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 8622692 [details] [diff] [review]
share_GCMethods_for_object_subclasses-v0.diff

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The missing barriers are obvious enough to me, but the fact that we haven't seen any exploits depending on them after 2 years with GGC enabled makes me think that attackers are still focused on lower hanging fruit.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, if you're paying attention and know what to look for.

Which older supported branches are affected by this flaw?
Unclear. There are at least 4 object subclasses implicated on tip, but not all of them existed, or were used with RelocatablePtr. Backports are going to have to be customized anyway, so I figured I'd just keep backporting until I hit the end.

If not all supported branches, which bug introduced the flaw?
Various.

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

How likely is this patch to cause regressions; how much testing does it need?
These shouldn't need significant testing.

Comment 7

[Terrence Cole [:terrence]]

Attachment: share_GCMethods_for_object_subclasses-vAurora.diff

A backport for Aurora -- almost identical to what's on tip.

Comment 8

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

Comment on attachment 8622692 [details] [diff] [review]
share_GCMethods_for_object_subclasses-v0.diff

Sec-moderate rated issue so it can just go into trunk without sec-approval. Only sec-criticals and sec-highs that affect more than trunk need approval. 

https://wiki.mozilla.org/Security/Bug_Approval_Process

Check 'er in.

Comment 9

[Terrence Cole [:terrence]]

Attachment: internalize_needsPostBarrier_check-v0.diff

I was finally able to get the prior solution linking everywhere except for SM(cgc). Which is weird, since that's a shell build.

Instead of continuing to try to shoehorn a static assert where C++ really doesn't want one, let's do something better. Specifically, let's make a new needsPostBarrier on InternalGCMethods that dispatches to T::, allowing us to use the type system as intended.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5780cd757bda

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8623321 [details] [diff] [review]
internalize_needsPostBarrier_check-v0.diff

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

::: js/public/RootingAPI.h
@@ +621,3 @@
>          if (!v)
>              return nullptr;
> +        MOZ_ASSERT(uintptr_t(v) > 32, "invald on tagged pointers");

*invalid

Er... but that assert seems a little strange. Maybe it should be:

  MOZ_ASSERT((uintptr_t(v) & 31) == 0, "invalid on tagged pointers");

Right now, it seems like a tagged non-nullptr would make it through without asserts.

Comment 11

[Terrence Cole [:terrence]]

That's actually correct, awful as it is -- we expect tagged null pointers to pass through there. But do see my response to Jon at https://bugzilla.mozilla.org/show_bug.cgi?id=1174873#c4.

Comment 12

[Terrence Cole [:terrence]]

Okay, so I finally managed to land and stick bug 1175642, which, instead of hacking things up even more, rewrites the interface in a somewhat cleaner way, at the cost of some increased complexity in the implementation. It does totally prevent the issue here, however. So I guess we need backports for 40 and lower now.

Comment 13

[Terrence Cole [:terrence]]

I guess this slipped through the cracks.

Thinking about it harder, we don't even know that there was a problem except in 41, briefly. And it was explicitly fixed a few releases before that. Given that the only releases still using the prior code are esr38 and b2g37, and given that even making the assertion link is non-trivial, and given that we've never actually seen a missing-barrier exploit in the wild (or in captivity), I would like to *not* uplift the patch here.

Comment 14

[Terrence Cole [:terrence]]

This should be completely fixed in all release branches now.