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

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

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.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: add-TypeToRootType-specializations

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.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8482289 [details] [diff] [review]
add-TypeToRootType-specializations

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

Great find!

Comment 3

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/860a8e4ae2fa

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/860a8e4ae2fa

Comment 5

[Benjamin Kerensa [:bkerensa]]

Does this have impact to ESR?

Comment 6

[Daniel Veditz [: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.

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Daniel Veditz [:dveditz] from comment #6)
Sorry about that, and thanks for the link - I can never find that page.

Comment 8

[Jon Coppeard (:jonco)]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please request aurora/beta/esr31 approval when you get a chance :)

Comment 10

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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 :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Also, this applies without conflict all the way to esr31 and is green on Try.

Comment 13

[Jon Coppeard (:jonco)]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Matt Wobensmith [:mwobensmith][:matt:]]

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.