Fix hazards revealed by adding in missing GCPointers

RESOLVED FIXED in Firefox 37, Firefox OS v2.0

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

({sec-high})

unspecified
mozilla39
x86_64
Linux
sec-high
Points:
---

Firefox Tracking Flags

(firefox35 wontfix, firefox36 wontfix, firefox37+ fixed, firefox38+ fixed, firefox39+ fixed, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main37+])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8565564 [details] [diff] [review]
Fix hazards revealed by adding in new GCPointers
Attachment #8565564 - Flags: review?(terrence)
(Assignee)

Updated

4 years ago
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment on attachment 8565564 [details] [diff] [review]
Fix hazards revealed by adding in new GCPointers

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

LGTM
Attachment #8565564 - Flags: review?(terrence) → review+
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
status-firefox-esr31: --- → affected
status-firefox-esr38: --- → affected
Keywords: sec-high
status-firefox35: affected → wontfix
status-firefox36: affected → wontfix
status-firefox39: --- → affected
Are these hazards dangerous?  If this is actually sec-high, it should have gotten sec-approval prior to landing.
Flags: needinfo?(sphink)
(Assignee)

Comment 6

4 years ago
(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?
Flags: needinfo?(sphink)
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.
https://hg.mozilla.org/mozilla-central/rev/331e34783b63
https://hg.mozilla.org/mozilla-central/rev/e29fabd8fad7
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-b2g-v1.4: --- → ?
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
status-firefox39: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 9

4 years ago
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.
Attachment #8565564 - Flags: sec-approval?
(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.
(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.
status-firefox-esr31: affected → unaffected
status-b2g-v1.4: ? → unaffected
tracking-firefox37: --- → +
tracking-firefox38: --- → +
tracking-firefox39: --- → +
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.
Attachment #8565564 - Flags: sec-approval? → sec-approval+
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?
Flags: needinfo?(sphink)
(Assignee)

Comment 14

4 years ago
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
Flags: needinfo?(sphink)
Attachment #8565564 - Flags: approval-mozilla-beta?
Attachment #8565564 - Flags: approval-mozilla-b2g37?
Attachment #8565564 - Flags: approval-mozilla-b2g34?
Attachment #8565564 - Flags: approval-mozilla-b2g32?
Attachment #8565564 - Flags: approval-mozilla-aurora?
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+
Attachment #8565564 - Flags: approval-mozilla-beta?
Attachment #8565564 - Flags: approval-mozilla-beta+
Attachment #8565564 - Flags: approval-mozilla-aurora?
Attachment #8565564 - Flags: approval-mozilla-aurora+
Comment on attachment 8565564 [details] [diff] [review]
Fix hazards revealed by adding in new GCPointers

sec-high doesn't need any extra b2g approvals
Attachment #8565564 - Flags: approval-mozilla-b2g37?
Attachment #8565564 - Flags: approval-mozilla-b2g34?
Attachment #8565564 - Flags: approval-mozilla-b2g32?
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
status-b2g-v2.0: affected → fixed
status-b2g-v2.1: affected → fixed
status-b2g-v2.2: affected → fixed
status-firefox-esr38: affected → ---
Whiteboard: [adv-main37+]

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.