IonBuilder scripts not traced for off thread compilation tasks

RESOLVED FIXED in Firefox -esr45

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

({csectype-uaf, sec-high})

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

Firefox Tracking Flags

(firefox48- wontfix, firefox49+ wontfix, firefox-esr4550+ fixed, firefox50+ fixed, firefox51+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main50+][adv-esr45.5+])

Attachments

(3 attachments)

Assignee

Description

3 years ago
decoder found a fuzz bug where the pre-barrier verifier was crashing with a UAF of a dead JSScript, probably involving threading.

Looking around it seems that we don't trace the JSScript pointers embedded in an IonBuilder when that's used to represent and off main thread compilation task.  We do cancel ion compilations when we start sweeping (in JitCompartment::sweep) but I'm not sure that's sufficient to prevent problems.

For comparision, all GC things in ParseTask are wrapped in PersistentRooted.

Here's a patch to trace pointers in the helper thread state as roots instead.  I converted the ParseTask ones too as it's more efficient to trace them directly than add three elements to the persistent rooted list for every task.

One issue that showed up was that the helper thread state is shared between all runtimes in the process, so we need to only trace those tasks associated with the tracer's runtime.

What are your thoughts on this?  I'm asking for feedback, but feel free to r+ if you think it's a good idea.
Attachment #8778291 - Flags: feedback?(terrence)
Attachment #8778291 - Flags: feedback?(terrence) → feedback+
Do we need the private comments if the bug itself is a hidden security bug? Are those comments supposed to stay hidden when the bug is eventually unhidden?

guessing at rating based on description in comment 2 ("taken as a whole it's absolutely terrifying").
Flags: needinfo?(terrence)
I replied privately mostly because Jan's message was private. Looking closer, I think it makes sense to keep them private: we can solve this specific fuzz bug without necessarily solving the larger architectural issue. OTOH, my private comment does not actually point out any specific problems, only a host of potential problems.

I think the right thing would be to actually file a new bug for the architectural issue, since that's going to be a long slog, if we even want to go that route. As to the severity of the initial filing, I don't think sec-high fits as we've no proof that there are actually any failures here and this is mostly about backstopping. I think. Requesting ni? from jonco to see if he has more to add.
Flags: needinfo?(terrence) → needinfo?(jcoppeard)
Comment on attachment 8778291 [details] [diff] [review]
trace-helper-thread-tasks

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

I think the only reason I didn't r+ this patch initially was that it f? and I failed at basic English comprehension. Please land it!
Attachment #8778291 - Flags: review+
Assignee

Comment 6

3 years ago
Comment on attachment 8778291 [details] [diff] [review]
trace-helper-thread-tasks

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Difficult, requires incremental GC and off thread compilation to happen together at just the right (wrong) time.

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? I think this has been broken right back to bug 774253.

If not all supported branches, which bug introduced the flaw? Bug 774253.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be straightforward.

How likely is this patch to cause regressions; how much testing does it need? Unlikely.
Flags: needinfo?(jcoppeard)
Attachment #8778291 - Flags: sec-approval?
I am afraid this is going to be too late for 49, can we land that for 50? Thanks
Tracking 51+ for this sec high issue.
This is too late for 49 so I'm giving sec-approval+ for trunk checkin but only on September 20 or after (two weeks into the next cycle).
Whiteboard: [checkin on 9/20]
Attachment #8778291 - Flags: sec-approval? → sec-approval+
Track for 49+/50+ as this is sec-high.
https://hg.mozilla.org/mozilla-central/rev/488c4ea38e16
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
This wasn't supposed to land before 20-September?
Depends on: 1301496
Whiteboard: [checkin on 9/20]
Yeah, this should not have landed yet, per comment 9.
Assignee

Comment 14

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
Sorry, I didn't notice that it wasn't an approval for immediate landing and landed it anyway.  Would you like me to back this out?
Flags: needinfo?(abillings)
Backing it out is useless. The point of sec-approval is not to expose a security issue too early by landing it in a public repo. Once it is landed, it is exposed.
Flags: needinfo?(abillings)
Jon, please request Beta and ESR45 approval on this when you get a chance (assuming you don't see this until after 50 uplifts to Beta).
Flags: needinfo?(jcoppeard)
Assignee

Comment 17

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
Sure.  We will need to backport bug 1301496 on top of this as well.
Group: javascript-core-security → core-security-release
Blocks: 1301343
Hi Jon, Terrence, should we uplift this fix to Beta50? I just checked bug 1301496 has already landed on 51.
Flags: needinfo?(terrence.d.cole)
Flags: needinfo?(terrence.d.cole)
I think we can probably uplift this. Will leave it to Jon for a final determination.
Assignee

Comment 20

3 years ago
Approval Request Comment
[Feature/regressing bug #]: Bug 774253.
[User impact if declined]: Possible crash / security vulnerability.
[Describe test coverage new/current, TreeHerder]: On m-c since September 8th.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8795248 - Flags: review+
Attachment #8795248 - Flags: approval-mozilla-beta?
Assignee

Comment 21

3 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: 51
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.
Attachment #8795249 - Flags: review+
Attachment #8795249 - Flags: approval-mozilla-esr45?
Comment on attachment 8795248 [details] [diff] [review]
bug1292590-beta

Sec-high, Beta50+
Attachment #8795248 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8795249 [details] [diff] [review]
bug1292590-esr45

Sec-high, ESR45.5+
Attachment #8795249 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main50+][adv-esr45.5+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.