Bug 1212356 (CVE-2016-5255)

crash in js::PreliminaryObjectArray::sweep()

VERIFIED FIXED in Firefox 48

Status

()

Core
JavaScript: GC
--
critical
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Jukka Jylänki, Assigned: jonco)

Tracking

({crash, sec-moderate})

unspecified
mozilla50
Unspecified
Windows NT
crash, sec-moderate
Points:
---

Firefox Tracking Flags

(firefox47 wontfix, firefox48- verified, firefox49- verified, firefox-esr45 wontfix, firefox50+ verified)

Details

(Whiteboard: [adv-main48+], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
This bug was filed from the Socorro interface and is 
report bp-f43103aa-365d-4657-889e-81d3e2151007.
=============================================================

Came back to this crash. Not sure how to repro. Does the callstack show anything relevant?

Updated

3 years ago
Crash Signature: [@ js::PreliminaryObjectArray::sweep()] → [@ js::PreliminaryObjectArray::sweep()] [@ js::PreliminaryObjectArray::sweep]

Comment 1

3 years ago
str
This happened to me after the following steps:

1. Went to css3test.com
2. Right-clicked the 'background-repeat' element on the page and chose 'Inspect Element' from the context menu
3. Right-clicked the selected element within the markup view and chose 'Edit As HTML'
4. Added '<div></div>' (without quotes) at the end of the editor
5. Pressed Ctrl+Enter
6. Moved mouse over the markup view (Sometimes the crash doesn't happen immediately but after some time moving the mouse around or even reloading the page.)

Crash report:
https://crash-stats.mozilla.com/report/index/832d749a-9fde-4ff7-a170-01b372151027

If e10s is disabled, I get a different crash signature, which is filed in bug 1213270.

Sebastian
See Also: → bug 1213270
See Also: → bug 1278832
Marking s-s because crashing on accessing a weak pointer is likely UAF.
Group: javascript-core-security
Bug 1278832 showed one way in which this crash could be triggered.  While in a incremental GC we traced the heap with the runtime in the 'minor collecting' state and traced an ObjectGroup that needed lazy sweeping.  Because we were in the minor collecting state IsAboutToBeFinalized only checked for the pointer being in the nursery and ignored whether it was in a zone that was being swept.  The result was that the object group was deemed to have been swept although it still contained a dead object pointer.

That was fixed by changing the way the zeal mode heap tracing works to happen outside of a minor collection.  However I think this can happen in the wild too.  All it would take is for a minor GC to happen during an incremental GC with an unswept object group in a stack root.  ObjectGroup::traceChildren will ultimately call maybeSweep which will mark the object as swept without correctly updating any dead object pointers.

I think we should change IsAboutToBeFinalized to check whether tenured objects are dead even during minor collections.
Assignee: nobody → jcoppeard
Created attachment 8770113 [details] [diff] [review]
bug1212356-object-group-sweep

As described above.
Attachment #8770113 - Flags: review?(terrence)
Comment on attachment 8770113 [details] [diff] [review]
bug1212356-object-group-sweep

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

Ouch. Makes sense.
Attachment #8770113 - Flags: review?(terrence) → review+
Comment on attachment 8770113 [details] [diff] [review]
bug1212356-object-group-sweep

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Hard, would require very precise timing of GCs.

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? Everything back to FF31.

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

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

How likely is this patch to cause regressions; how much testing does it need? Unlikely.
Attachment #8770113 - Flags: sec-approval?

Comment 7

2 years ago
This needs a security rating before it can be approved.

If this crash was triggered, how exploitable is it (if you got the timing right)?
status-firefox47: --- → wontfix
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox48: --- → ?
tracking-firefox49: --- → ?
tracking-firefox50: --- → +
tracking-firefox-esr45: --- → ?
(In reply to Al Billings [:abillings] from comment #7)
> This needs a security rating before it can be approved.

Setting sec-moderate given the difficulty of constructing an exploit, although I could be talked into sec-high. 

> If this crash was triggered, how exploitable is it (if you got the timing
> right)?

This bug would allow an attacker to forge an object's "group". Our higher tier compilers use the "group" information in order to decide what optimizations to make on the object. You could potentially use this to trick the compiler into emitting code to manipulate a different sort of object than the one that you actually are holding. In theory you could use this to write wherever you wanted in the heap and from there, the world.

That said, it's multiple (difficult) steps to get there even once you hit the right timing. It's no simple jit-spray. In order to exploit this bug you'd have to understand our heap structure in details (e.g. [1]), but also have strong familiarity with how our type system and compiler work. You'd likely need a full time doppleganger anti-JS team. Actually, that probably already exists, maybe even multiple instances. So, probably exploitable by state-level actors, but not criminal rings? Is that sec-moderate or sec-high?

1- http://phrack.org/issues/69/14.html#article
Keywords: sec-moderate

Comment 9

2 years ago
Comment on attachment 8770113 [details] [diff] [review]
bug1212356-object-group-sweep

Clearing sec-approval? as I agree with the sec-moderate rating. That said, I'd like to see it on Aurora and Beta if you could nominate patches there. Go ahead and check into trunk though.
Attachment #8770113 - Flags: sec-approval?

Updated

2 years ago
status-firefox-esr45: affected → wontfix
tracking-firefox-esr45: ? → ---
https://hg.mozilla.org/mozilla-central/rev/50c41c7027f6
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Group: javascript-core-security → core-security-release
Jon, could you fill the uplift requests? Thanks
Flags: needinfo?(jcoppeard)
Not tracking because of the relatively low volume but happy to take a patch.
tracking-firefox48: ? → -
tracking-firefox49: ? → -
Comment on attachment 8770113 [details] [diff] [review]
bug1212356-object-group-sweep

Approval Request Comment
[Feature/regressing bug #]: Bug 619558.
[User impact if declined]: Possible crash / security vulnerability.
[Describe test coverage new/current, TreeHerder]: On m-c since 15th July.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8770113 - Flags: approval-mozilla-beta?
Attachment #8770113 - Flags: approval-mozilla-aurora?
> [Risks and why]: Low.
Could you explain why it is low risk?
gc is not the most easiest thing in our code.
(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> > [Risks and why]: Low.
> Could you explain why it is low risk?
> gc is not the most easiest thing in our code.

The change makes IsAboutToBeFinalized check whether tenured objects are dead during minor collections, whereas previously this check was skipped.  So it's possible that this could have returned false negatives before.  This is bad as it can lead to UAF.  I don't think the change can lead to anything bad happening as the extra check already happens all the time when this is called outside minor collections.
Comment on attachment 8770113 [details] [diff] [review]
bug1212356-object-group-sweep

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

Thanks for your explanation. Let's take it in 48 beta 10 and aurora.
Attachment #8770113 - Flags: approval-mozilla-beta?
Attachment #8770113 - Flags: approval-mozilla-beta+
Attachment #8770113 - Flags: approval-mozilla-aurora?
Attachment #8770113 - Flags: approval-mozilla-aurora+
Thanks Jon, setting the flags
status-firefox48: affected → fixed
status-firefox49: affected → fixed
Flags: qe-verify+
I managed to reproduce this crash on Firefox 44.0a1 (2015-10-06) and on Windows 10 x64.
The crash is no longer reproducible on Firefox 50.0a1 (2016-07-21), Firefox 49.0a2 (2016-07-21), or on 48.0b10 Firefox, using the STR from Comment 1.
The tests were performed on Windows 10 x64 and on Windows 7 x64.
I am marking the issue Verified Fixed.
Status: RESOLVED → VERIFIED
status-firefox48: fixed → verified
status-firefox49: fixed → verified
status-firefox50: fixed → verified
Flags: qe-verify+

Updated

2 years ago
Alias: CVE-2016-5255
Whiteboard: [adv-main48+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.