1212356 - (CVE-2016-5255) crash in js::PreliminaryObjectArray::sweep()

Status: VERIFIED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jukka Jylänki]

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?

Comment 1

[Sebastian Zartner [:sebo]]

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

Comment 2

[Jon Coppeard (:jonco)]

Marking s-s because crashing on accessing a weak pointer is likely UAF.

Comment 3

[Jon Coppeard (:jonco)]

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.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1212356-object-group-sweep

As described above.

Comment 5

[Terrence Cole [:terrence]]

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

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

Ouch. Makes sense.

Comment 6

[Jon Coppeard (:jonco)]

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.

Comment 7

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

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

Comment 8

[Terrence Cole [:terrence]]

(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

Comment 9

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

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.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/50c41c7027f6

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Jon, could you fill the uplift requests? Thanks

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Not tracking because of the relatively low volume but happy to take a patch.

Comment 13

[Jon Coppeard (:jonco)]

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.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

> [Risks and why]: Low.
Could you explain why it is low risk?
gc is not the most easiest thing in our code.

Comment 15

[Jon Coppeard (:jonco)]

(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 16

[Gerry Chang [:gchang]]

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.

Comment 17

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8c9f7b597430
https://hg.mozilla.org/releases/mozilla-beta/rev/ef30d84237a8

Comment 18

[Carsten Book [:Tomcat]]

Thanks Jon, setting the flags

Comment 19

[Mihai Boldan, Desktop QA [:mboldan]]

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.