Closed
Bug 1061214
Opened 10 years ago
Closed 10 years ago
MarkJitExitFrame() doesn't mark some VM wrapper argument types
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla35
Tracking | Status | |
---|---|---|
firefox32 | --- | wontfix |
firefox33 | + | fixed |
firefox34 | + | fixed |
firefox35 | + | fixed |
firefox-esr31 | 33+ | fixed |
b2g-v1.4 | --- | unaffected |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | fixed |
b2g-v2.2 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
Details
(Keywords: sec-high, Whiteboard: [adv-main33+][adv-esr31.2+][b2g-adv-main2.2-])
Attachments
(1 file)
1.74 KB,
patch
|
terrence
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
bkerensa
:
approval-mozilla-esr31+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
The machinery to work out which argument types to mark relies on template specialization, but the default case is not to mark. Unfortunately some types are missing so these don't get marked.
I guess this is a security issue now we use exact rooting, although I think this would be very hard to exploit in practice.
Assignee | ||
Comment 1•10 years ago
|
||
This adds specializations for handles to JSScript, StaticBlockObject and StaticWithObject, and also a Handle<T> specialization that will cause a build error if other unsupported handles are used.
Attachment #8482289 -
Flags: review?(terrence)
Comment 2•10 years ago
|
||
Comment on attachment 8482289 [details] [diff] [review]
add-TypeToRootType-specializations
Review of attachment 8482289 [details] [diff] [review]:
-----------------------------------------------------------------
Great find!
Attachment #8482289 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Blocks: ExactRooting
Comment 4•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox35:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 6•10 years ago
|
||
Jon: security bugs need to follow the sec-approval process before landing
https://wiki.mozilla.org/Security/Bug_Approval_Process
This is in part to answer the questions bkerensa has above. Please retroactively request sec-approval? on the patch which will populate a template of questions for you to answer about the code being patched.
status-b2g-v1.4:
--- → ?
status-b2g-v2.0:
--- → ?
status-b2g-v2.0M:
--- → ?
status-b2g-v2.1:
--- → ?
status-b2g-v2.2:
--- → fixed
status-firefox32:
--- → ?
status-firefox33:
--- → ?
status-firefox34:
--- → ?
status-firefox-esr31:
--- → ?
tracking-firefox35:
--- → +
Flags: needinfo?(dveditz) → needinfo?(jcoppeard)
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #6)
Sorry about that, and thanks for the link - I can never find that page.
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8482289 [details] [diff] [review]
add-TypeToRootType-specializations
[Security approval request comment]
Requesting retroactive approval for this.
How easily could an exploit be constructed based on the patch?
I think this would be 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?
Exact rooting was enabled on desktop in FF28 and for b2g in 33, so I think the following are affected: aurora, beta, release, esr31, b2g 2.1.
If not all supported branches, which bug introduced the flaw?
Bug 753203 (but note this was enabled in desktop and b2g builds at different times).
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No, but should be trivial.
How likely is this patch to cause regressions; how much testing does it need?
Unlikely. This has been present on central for a month.
Attachment #8482289 -
Flags: sec-approval?
Comment 9•10 years ago
|
||
Please request aurora/beta/esr31 approval when you get a chance :)
Comment 10•10 years ago
|
||
Comment on attachment 8482289 [details] [diff] [review]
add-TypeToRootType-specializations
After the fact, sec-approval+. We should get this on Aurora but I believe it is too late for ESR31 and Beta. We're under two weeks from release.
Attachment #8482289 -
Flags: sec-approval? → sec-approval+
Comment 11•10 years ago
|
||
We still have time to get this in for the final 33 build if the nominations and approvals are done expediently. Jon, can you please do the aurora/beta/esr31 approval requests ASAP? Thanks :)
Flags: needinfo?(jcoppeard)
Comment 12•10 years ago
|
||
Also, this applies without conflict all the way to esr31 and is green on Try.
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8482289 [details] [diff] [review]
add-TypeToRootType-specializations
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Bug marked sec-high
User impact if declined: Potential security vulnerability
Fix Landed on Version: FF35
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None
See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Approval Request Comment
[Feature/regressing bug #]: Bug 753203
[User impact if declined]: Potential security vulnerability
[Describe test coverage new/current, TBPL]: This has been on mozilla-central for a month.
[Risks and why]: Low risk
[String/UUID change made/needed]: None
Attachment #8482289 -
Flags: approval-mozilla-esr31?
Attachment #8482289 -
Flags: approval-mozilla-beta?
Attachment #8482289 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(jcoppeard)
Updated•10 years ago
|
Attachment #8482289 -
Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Updated•10 years ago
|
Attachment #8482289 -
Flags: approval-mozilla-beta?
Attachment #8482289 -
Flags: approval-mozilla-beta+
Attachment #8482289 -
Flags: approval-mozilla-aurora?
Attachment #8482289 -
Flags: approval-mozilla-aurora+
Comment 14•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/77dcdf987c19
https://hg.mozilla.org/releases/mozilla-beta/rev/bbc35ec2c90e
https://hg.mozilla.org/releases/mozilla-esr31/rev/be0ae4dbb1ff
Updated•10 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
Whiteboard: [adv-main33+][adv-esr31.2+]
Comment 15•10 years ago
|
||
Marking [qe-verify-] as I don't see any tests or STR to reproduce. If you come up with something, let me know and I'd be happy to test.
Flags: qe-verify-
Updated•9 years ago
|
Whiteboard: [adv-main33+][adv-esr31.2+] → [adv-main33+][adv-esr31.2+][b2g-adv-main2.2-]
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
•