Closed
Bug 1322971
Opened 6 years ago
Closed 6 years ago
Cell iterators need a read barrier
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
People
(Reporter: jonco, Assigned: jonco)
References
Details
(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main52+][adv-esr45.8+])
Attachments
(3 files)
8.73 KB,
patch
|
sfink
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
8.86 KB,
patch
|
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
6.41 KB,
patch
|
gchang
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
Iterating through allocated cells can hand out pointers to otherwise unreachable cells, and so needs a read barrier in case there's an incremental GC going on. This came up originally in bug 1322420.
Updated•6 years ago
|
Keywords: csectype-uaf,
sec-high
Assignee | ||
Comment 1•6 years ago
|
||
Patch to add a barrier to AreanCellIter. ArenaCellIterImpl is used both during and outside of GC so this needs a dynamic check. It does a full ExposeToActiveJS as we can find gray things in the heap and then hand them back to the caller.
Assignee: nobody → jcoppeard
Attachment #8830414 -
Flags: review?(sphink)
Comment 2•6 years ago
|
||
Hey Steve, this is a security issue. Can you please move this review request to the top of your pile? If not, please redirect to someone else ASAP (and make sure they do have the time!)
Flags: needinfo?(sphink)
Comment 3•6 years ago
|
||
Comment on attachment 8830414 [details] [diff] [review] bug1322420-barrier-cell-iter Review of attachment 8830414 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. I don't have a great system for secure bugs, it seems. Please ping after 48 hours.
Attachment #8830414 -
Flags: review?(sphink) → review+
Updated•6 years ago
|
Flags: needinfo?(sphink)
Assignee | ||
Comment 4•6 years ago
|
||
Comment on attachment 8830414 [details] [diff] [review] bug1322420-barrier-cell-iter [Security approval request comment] How easily could an exploit be constructed based on the patch? Very difficult. 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? At least as far back as 32. If not all supported branches, which bug introduced the flaw? Bug 1001159 introduced this class but the flaw may have been present before then. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? It may require rebasing but the result shouldn't be very different. How likely is this patch to cause regressions; how much testing does it need? Unlikely.
Attachment #8830414 -
Flags: sec-approval?
Comment 5•6 years ago
|
||
sec-approval+ for trunk. We'll want this on affected branches as well if you can nominate patches.
status-firefox51:
--- → wontfix
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox-esr45:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox52:
--- → +
tracking-firefox53:
--- → +
tracking-firefox54:
--- → +
tracking-firefox-esr45:
--- → 52+
Flags: needinfo?(jcoppeard)
Updated•6 years ago
|
Attachment #8830414 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 6•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e919ac282a4b42906d8695a0e1671e8ec2a044c
Comment 7•6 years ago
|
||
Jon, can you please nominate branch patches for all other affected branches?
Assignee | ||
Comment 8•6 years ago
|
||
I mistakenly landed this with the wrong bug number: https://hg.mozilla.org/mozilla-central/rev/4e919ac282a4
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 9•6 years ago
|
||
Approval Request Comment [Feature/Bug causing the regression]: Bug 1001159. [User impact if declined]: Possible crash / security violation. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: This is a fairly simple change that adds a read barrier when iterating cells in arenas. [String changes made/needed]: None.
Attachment #8837584 -
Flags: approval-mozilla-beta?
Attachment #8837584 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 10•6 years ago
|
||
[Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high bug. User impact if declined: Possible crash / security vulnerability. Fix Landed on Version: 54. Risk to taking this patch (and alternatives if risky): Low. String or UUID changes made by this patch: None.
Attachment #8837594 -
Flags: approval-mozilla-esr45?
Assignee | ||
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Group: javascript-core-security → core-security-release
Updated•6 years ago
|
Comment 11•6 years ago
|
||
Comment on attachment 8837584 [details] [diff] [review] bug1322971-backport Approving for Beta and Aurora. I'll wait for release management to make a decision for ESR45.
Attachment #8837584 -
Flags: approval-mozilla-beta?
Attachment #8837584 -
Flags: approval-mozilla-beta+
Attachment #8837584 -
Flags: approval-mozilla-aurora?
Attachment #8837584 -
Flags: approval-mozilla-aurora+
Comment 12•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/d956e48d28df https://hg.mozilla.org/releases/mozilla-beta/rev/6e2dcde91b89
Comment 13•6 years ago
|
||
Comment on attachment 8837594 [details] [diff] [review] bug1322971-esr45 Fix a sec-high. ESR45+.
Attachment #8837594 -
Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Comment 14•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/6e2dcde91b89 https://hg.mozilla.org/releases/mozilla-esr45/rev/a0ead6ef09eb
Comment 15•6 years ago
|
||
Setting qe-verify- based on Jon's assessment on manual testing needs (Comment 9) and the fact that this fix has automated coverage.
Flags: qe-verify-
Updated•6 years ago
|
tracking-firefox-esr52:
? → ---
Whiteboard: [adv-main52+][adv-esr45.8+]
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•