1133909 - Fix hazards revealed by adding in missing GCPointers

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

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

Bug 1132744 added in some new and renamed GCPointers, which revealed a few undetected hazards. I am not marking the dependency since currently bug 1132744 is public.

Comment 1

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

Attachment: Fix hazards revealed by adding in new GCPointers

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8565564 [details] [diff] [review]
Fix hazards revealed by adding in new GCPointers

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

LGTM

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54f9adc327db

https://hg.mozilla.org/integration/mozilla-inbound/rev/331e34783b63

Comment 4

[Andrew McCreight [:mccr8]]

Are these hazards dangerous?  If this is actually sec-high, it should have gotten sec-approval prior to landing.

Comment 5

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

check-style fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/e29fabd8fad7

Comment 6

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

(In reply to Andrew McCreight [:mccr8] from comment #4)
> Are these hazards dangerous?  If this is actually sec-high, it should have
> gotten sec-approval prior to landing.

Oh, sorry. I thought that was only sec-crit.

Are they dangerous? Hm. There are 2 real hazards here. They're both UAF of gcthings. The Symbols one looks like it would give you very limited read-only access. The JitCode one would interpret 3 words as pointers and free them. I don't know how bad that is considered?

Comment 7

[Andrew McCreight [:mccr8]]

No, it is for sec-high and sec-crit.

Thanks for the explanation, that does sound bad, though difficult to exploit.  I guess just fill out the sec-approval thing and Al can set flags and whatever else.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/331e34783b63
https://hg.mozilla.org/mozilla-central/rev/e29fabd8fad7

Comment 9

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

Comment on attachment 8565564 [details] [diff] [review]
Fix hazards revealed by adding in new GCPointers

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

If you recognize that GC problems are nasty, it wouldn't be hard to trigger a crash. An exploit seems difficult, but it'd probably be useful in combination with other holes.

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

No

Which older supported branches are affected by this flaw?

Looks like gecko 32 and up.

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

I would guess bug 1066322 (Fx36) for the Symbol part, and bug 976446 (Fx32) for the jitcode part.

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

I haven't tried making them yet, but they should be nearly identical and low risk. The most likely thing to cause conflicts is only shutting up a false positive, so it can be dropped if it causes trouble.

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

It's very low risk. It has a high probability of fixing the actual flaws, since they were things that the static analysis should be able to catch but couldn't because we renamed some things underneath it. Taking those renamings into account, the analysis is happy.

Comment 10

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

(In reply to Steve Fink [:sfink, :s:] from comment #9)

> Which older supported branches are affected by this flaw?
> 
> Looks like gecko 32 and up.

 I see that Terrence Cole marked this as affecting 31. Is this actually on 32 up and ESR31 is unaffected? It sounds like this is the case.

Comment 11

[Terrence Cole [:terrence]]

(In reply to Al Billings [:abillings] from comment #10)
> (In reply to Steve Fink [:sfink, :s:] from comment #9)
> 
> > Which older supported branches are affected by this flaw?
> > 
> > Looks like gecko 32 and up.
> 
>  I see that Terrence Cole marked this as affecting 31. Is this actually on
> 32 up and ESR31 is unaffected? It sounds like this is the case.

Sorry, Steve is correct: I thought we landed the JitCode change earlier than 32.

Comment 12

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

Comment on attachment 8565564 [details] [diff] [review]
Fix hazards revealed by adding in new GCPointers

sec-approval+ for trunk.

We should take this on Aurora and Beta as well.

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Given that this has already landed on m-c, I would prefer to take the fix in Beta 2 if possible.

Steve - Can you request uplift early on Monday?

Comment 14

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

Comment on attachment 8565564 [details] [diff] [review]
Fix hazards revealed by adding in new GCPointers

Approval Request Comment
[Feature/regressing bug #]: bug 1066322 (Fx36) for the Symbol part, and bug 976446 for the jitcode
[User impact if declined]: potentially exploitable UAF
[Describe test coverage new/current, TreeHerder]: on central
[Risks and why]: Safe. These changes have trivial effects during normal operation. (Minor reordering of independent operations.)
[String/UUID change made/needed]: none

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8565564 [details] [diff] [review]
Fix hazards revealed by adding in new GCPointers

Let's get this sec-high fix on upstream branches. Beta+ Aurora+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8565564 [details] [diff] [review]
Fix hazards revealed by adding in new GCPointers

sec-high doesn't need any extra b2g approvals

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/62f149b1cfd7
https://hg.mozilla.org/releases/mozilla-beta/rev/3a352baeeca4

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3a352baeeca4
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/0a534b3a14b9
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/442332896430

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/0a534b3a14b9
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/442332896430