Closed
Bug 1133909
Opened 10 years ago
Closed 10 years ago
Fix hazards revealed by adding in missing GCPointers
Categories
(Core :: JavaScript: GC, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
People
(Reporter: sfink, Assigned: sfink)
Details
(Keywords: sec-high, Whiteboard: [adv-main37+])
Attachments
(1 file)
5.25 KB,
patch
|
terrence
:
review+
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Attachment #8565564 -
Flags: review?(terrence)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment 2•10 years ago
|
||
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+
Updated•10 years ago
|
status-firefox35:
--- → affected
status-firefox36:
--- → affected
status-firefox37:
--- → affected
status-firefox38:
--- → affected
status-firefox-esr31:
--- → affected
status-firefox-esr38:
--- → affected
Updated•10 years ago
|
Assignee | ||
Comment 3•10 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=54f9adc327db https://hg.mozilla.org/integration/mozilla-inbound/rev/331e34783b63
Comment 4•10 years ago
|
||
Are these hazards dangerous? If this is actually sec-high, it should have gotten sec-approval prior to landing.
Flags: needinfo?(sphink)
Assignee | ||
Comment 5•10 years ago
|
||
check-style fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/e29fabd8fad7
Assignee | ||
Comment 6•10 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)
Comment 7•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/331e34783b63 https://hg.mozilla.org/mozilla-central/rev/e29fabd8fad7
Status: ASSIGNED → RESOLVED
Closed: 10 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
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee | ||
Comment 9•10 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?
Comment 10•10 years ago
|
||
(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•10 years ago
|
||
(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.
Updated•10 years ago
|
Updated•10 years ago
|
Comment 12•10 years ago
|
||
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+
Comment 13•10 years ago
|
||
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•10 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 15•10 years ago
|
||
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 16•10 years ago
|
||
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?
Comment 17•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/62f149b1cfd7 https://hg.mozilla.org/releases/mozilla-beta/rev/3a352baeeca4
Comment 18•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/0a534b3a14b9 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/442332896430
Updated•10 years ago
|
Whiteboard: [adv-main37+]
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•