MarkJitExitFrame() doesn't mark some VM wrapper argument types

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

({sec-high})

unspecified
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox32 wontfix, firefox33+ fixed, firefox34+ fixed, firefox35+ fixed, firefox-esr3133+ fixed, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [adv-main33+][adv-esr31.2+][b2g-adv-main2.2-])

Attachments

(1 attachment)

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.
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 on attachment 8482289 [details] [diff] [review]
add-TypeToRootType-specializations

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

Great find!
Attachment #8482289 - Flags: review?(terrence) → review+
Keywords: sec-high
https://hg.mozilla.org/mozilla-central/rev/860a8e4ae2fa
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Does this have impact to ESR?
Flags: needinfo?(dveditz)
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.1: --- → ?
Flags: needinfo?(dveditz) → needinfo?(jcoppeard)
(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)
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 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+
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)
Also, this applies without conflict all the way to esr31 and is green on Try.
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)
Attachment #8482289 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Attachment #8482289 - Flags: approval-mozilla-beta?
Attachment #8482289 - Flags: approval-mozilla-beta+
Attachment #8482289 - Flags: approval-mozilla-aurora?
Attachment #8482289 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main33+][adv-esr31.2+]
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-
Whiteboard: [adv-main33+][adv-esr31.2+] → [adv-main33+][adv-esr31.2+][b2g-adv-main2.2-]
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.