Use after free JS_ASSERT(arenaHeader()->allocated());

RESOLVED FIXED in Firefox 30

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: gwagner, Assigned: shu)

Tracking

({csectype-uaf, regression, sec-high})

unspecified
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, firefox31 fixed, firefox32 fixed, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, seamonkey2.26 fixed)

Details

(Whiteboard: [adv-main30+])

Attachments

(1 attachment)

Reporter

Description

5 years ago
Seen on nexus 4 with debug gecko and current trunk.

STR:
Go to maps.google.com
press GPS icon a few times and keep zooming and moving around

Program received signal SIGSEGV, Segmentation fault.
0xb5e56b56 in js::gc::Cell::isMarked (color=<optimized out>, this=<optimized out>) at ../../../js/src/gc/Heap.h:1018
1018	    JS_ASSERT(arenaHeader()->allocated());
(gdb) bt
#0  0xb5e56b56 in js::gc::Cell::isMarked (color=<optimized out>, this=<optimized out>) at ../../../js/src/gc/Heap.h:1018
#1  0xb5e5722e in js::gc::Cell::isMarked (this=0xb0684440, color=0) at ../../../js/src/gc/Heap.h:1021
#2  0xb5e5746a in js::gc::IsAboutToBeFinalized<js::UnownedBaseShape> (thingp=<optimized out>) at ../../../js/src/gc/Marking.cpp:388
#3  0xb5fe9cc6 in js::types::TypeCompartment::sweep (this=0xb0984834, fop=<optimized out>) at ../../../js/src/jsinfer.cpp:4166
#4  0xb5fea894 in js::types::TypeZone::sweep (this=0xb0983694, fop=0xbeb836b8, releaseTypes=<optimized out>, oom=0xbeb83677) at ../../../js/src/jsinfer.cpp:4425
#5  0xb5e6ccc2 in JS::Zone::sweep (this=0xb0983400, fop=0xbeb836b8, releaseTypes=false, oom=0xbeb83677) at ../../../js/src/gc/Zone.cpp:119
#6  0xb5fd891e in BeginSweepingZoneGroup (rt=0xb31e4000) at ../../../js/src/jsgc.cpp:3718
#7  0xb5fda0d4 in BeginSweepPhase (rt=0xb31e4000, lastGC=<optimized out>) at ../../../js/src/jsgc.cpp:3822
#8  0xb5fdb7ba in IncrementalCollectSlice (rt=0xb31e4000, budget=<optimized out>, reason=JS::gcreason::ALLOC_TRIGGER, gckind=js::GC_NORMAL) at ../../../js/src/jsgc.cpp:4428
#9  0xb5fdbc92 in GCCycle (rt=0xb31e4000, incremental=<optimized out>, budget=0, gckind=js::GC_NORMAL, reason=JS::gcreason::ALLOC_TRIGGER) at ../../../js/src/jsgc.cpp:4564
#10 0xb5fdbff0 in Collect (rt=0xb31e4000, incremental=<optimized out>, budget=30000, gckind=js::GC_NORMAL, reason=JS::gcreason::ALLOC_TRIGGER) at ../../../js/src/jsgc.cpp:4697
#11 0xb5fdc296 in js::gc::GCIfNeeded (cx=<optimized out>) at ../../../js/src/jsgc.cpp:4840
#12 0xb5fa2aca in js::InvokeInterruptCallback (cx=0xb3cbec60) at ../../../js/src/jscntxt.cpp:1020
#13 0xb3d62d2c in ?? ()
#14 0xb3d62d2c in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb) f 3
#3  0xb5fe9cc6 in js::types::TypeCompartment::sweep (this=0xb0984834, fop=<optimized out>) at ../../../js/src/jsinfer.cpp:4166
4166	                if (IsTypeObjectAboutToBeFinalized(&typeObject))
(gdb) p typeObject
$5 = (js::types::TypeObject *) 0xb0684440
(gdb) p *typeObject
$6 = {<js::gc::BarrieredCell<js::types::TypeObject>> = {<js::gc::Cell> = {<No data fields>}, <No data fields>}, clasp_ = 0x4b4b4b4b, 
  proto_ = {<js::BarrieredPtr<JSObject, unsigned int>> = {{value = 0x4b4b4b4b, other = 1263225675}}, <No data fields>}, 
  singleton_ = {<js::BarrieredPtr<JSObject, unsigned int>> = {{value = 0x4b4b4b4b, other = 1263225675}}, <No data fields>}, static LAZY_SINGLETON = 1, flags_ = 1263225675, 
  addendum = {<js::BarrieredPtr<js::types::TypeObjectAddendum, unsigned int>> = {{value = 0x4b4b4b4b, other = 1263225675}}, <No data fields>}, propertySet = 0x4b4b4b4b, 
  interpretedFunction = {<js::BarrieredPtr<JSFunction, unsigned int>> = {{value = 0x4b4b4b4b, other = 1263225675}}, <No data fields>}, padding = 1263225675}
Assignee

Comment 1

5 years ago
Gregor tested this in person and it seems to work.
Attachment #8411468 - Flags: review?(wmccloskey)
Assignee

Updated

5 years ago
Assignee: nobody → shu
Status: NEW → ASSIGNED
Attachment #8411468 - Flags: review?(wmccloskey) → review+
Assignee

Comment 2

5 years ago
Comment on attachment 8411468 [details] [diff] [review]
Clear TypeCompartment tables.

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Pretty unlikely.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Not really.

> Which older supported branches are affected by this flaw?

aurora, beta, release. I'll let Gregor handle the b2g ones.

> If not all supported branches, which bug introduced the flaw?

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

Should apply cleanly.

> How likely is this patch to cause regressions; how much testing does it need?

Should be none. Testing on m-c after sec-approval should be enough.
Attachment #8411468 - Flags: sec-approval?
Assignee

Comment 3

5 years ago
I'm not sure what sec rating this should have. Can one of the GC guys weigh in?
Use-after-free is sec-high or sec-crit, depending on how hard it is to take advantage of. High enough to bother backporting the fix in any case.
OS: Mac OS X → All
Hardware: x86 → All
Reporter

Comment 5

5 years ago
Paul, can you help with the b2g versions that need to be fixed?
Flags: needinfo?(ptheriault)
Just to be clear, I think bug 897655 is where we regressed.
Assignee

Comment 7

5 years ago
(In reply to Bill McCloskey (:billm) from comment #6)
> Just to be clear, I think bug 897655 is where we regressed.

That's right: https://hg.mozilla.org/releases/mozilla-release/rev/b5e301863e69#l18.12
So double-checked in tree, and yes, affects 1.2 onwards. https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/file/14df3b8bf122/js/src/jscompartment.cpp#l593

I would recommend this as blocking-b2g: 1.3 since it is at least sec-high, maybe sec-critical, with small patch ready to be applied.
blocking-b2g: --- → 1.3?
Flags: needinfo?(ptheriault)
Reporter

Updated

5 years ago
Summary: B2G: use after free JS_ASSERT(arenaHeader()->allocated()); → Use after free JS_ASSERT(arenaHeader()->allocated());
This has sec-approval+ to go in on May 20 or later. We're shipping 29 in a few days and it missed the train for that.
Whiteboard: [checkin on 5/20]
Attachment #8411468 - Flags: sec-approval? → sec-approval+
Assignee

Comment 11

5 years ago
Comment on attachment 8411468 [details] [diff] [review]
Clear TypeCompartment tables.

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 897655
User impact if declined: Crashes?
Testing completed: in person
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.
Attachment #8411468 - Flags: approval-mozilla-release?
Attachment #8411468 - Flags: approval-mozilla-esr24?
Attachment #8411468 - Flags: approval-mozilla-beta?
Attachment #8411468 - Flags: approval-mozilla-b2g28?
Attachment #8411468 - Flags: approval-mozilla-b2g26?
Comment on attachment 8411468 [details] [diff] [review]
Clear TypeCompartment tables.

I don't see the uplift request for aurora. Is that expected? As Al said in comment #10, it is too late for 29 (and for 28 obviously).
Attachment #8411468 - Flags: approval-mozilla-release?
Attachment #8411468 - Flags: approval-mozilla-release-
Attachment #8411468 - Flags: approval-mozilla-beta?
Attachment #8411468 - Flags: approval-mozilla-beta-
Assignee

Comment 13

5 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #12)
> Comment on attachment 8411468 [details] [diff] [review]
> Clear TypeCompartment tables.
> 
> I don't see the uplift request for aurora. Is that expected? As Al said in
> comment #10, it is too late for 29 (and for 28 obviously).

That's why I didn't request it -- I thought that implied that I should wait to request approval for aurora after 05/20.

Anyways I don't know what's going on here and I don't really have time to play the tracking flags game. Can someone just make sure it gets checked in and uplifted to all the right branches?
(In reply to Shu-yu Guo [:shu] from comment #13)

> That's why I didn't request it -- I thought that implied that I should wait
> to request approval for aurora after 05/20.

This is exactly what I figured you would do since you don't have approval to check into trunk until 5/20 and Aurora and Beta then will be different branches than now.
Attachment #8411468 - Flags: approval-mozilla-release-
Attachment #8411468 - Flags: approval-mozilla-beta-
Al or Ryan or somebody will go through and look for these delayed landings in a month, so you shouldn't have to think about it until then.
Assignee

Comment 16

5 years ago
(In reply to Andrew McCreight [:mccr8] from comment #15)
> Al or Ryan or somebody will go through and look for these delayed landings
> in a month, so you shouldn't have to think about it until then.

Cool, thanks!
Taking it in 1.3 per comment #9.
blocking-b2g: 1.3? → 1.3+
Attachment #8411468 - Flags: approval-mozilla-b2g28?
Attachment #8411468 - Flags: approval-mozilla-b2g26?
If this regressed from bug 897655 it should not affect ESR-24. Is the regressing bug wrong or is the esr-24 approval request unnecessary?
Blocks: 897655
Flags: needinfo?(shu)
Keywords: regression
Assignee

Comment 19

5 years ago
(In reply to Daniel Veditz [:dveditz] from comment #18)
> If this regressed from bug 897655 it should not affect ESR-24. Is the
> regressing bug wrong or is the esr-24 approval request unnecessary?

The regression bug is right. The flag I set was unnecessary.
Flags: needinfo?(shu)
Shu,

We need risk analysis of the patch in question here.

What are the areas of code it touches?
Flags: needinfo?(shu)
    To have any that this might make it onto a 1.3 device, we need to have this landed asap. There are two things in the way of this
    - it would mean landing a security fix early in the  desktop FF cycle, which we try to avoid
    - the risk that this patch causes an issue on the 1.3 branch

    For the first one, I've spoken with Al and Dveditz about the risk of landing this early in the cycle, and I think we decided that this was ok for an exception in this case, given the likely complexity involved with discovery and exploitation.

    I can't speak for the second issue though, as I have no idea how likely it is that this bug will cause instability etc. It seems like a relatively straightforward, selfcontained patch, but could I get a comment from someone who knows this code as to whether there is much stability risk of landing this patch on 1.3?
Flags: needinfo?(wmccloskey)
Gregor,

Please assess for technical risks here. Any potential obvious regressions?

Reaching out to you per IRC with shu:

shu> hi, how's it going?
3:37 PM <Preeti> would you be able to help with risk analysis of the bug?
3:37 PM <shu> what is risk analysis
3:37 PM <shu> what kind of risk am i assessing?
3:37 PM <Preeti> code
3:37 PM <Preeti> modules touched
3:37 PM <shu> ah
3:37 PM <Preeti> areas of regression'
3:37 PM <shu> only js is touched
3:37 PM <Preeti> potential regression causes
3:38 PM <shu> outside of that i don't know much about it, i would ask gwagner
3:38 PM <Preeti> k thx
Flags: needinfo?(anygregor)
Assignee

Updated

5 years ago
Flags: needinfo?(shu)
Reporter

Comment 23

5 years ago
This patch has very low risk and it fixes a crash that is very easy to reproduce. We understand the problem and the fix. This is a straight forward bug fix. We should get this in.
Flags: needinfo?(anygregor)
Whiteboard: [checkin on 5/20]
Comment on attachment 8411468 [details] [diff] [review]
Clear TypeCompartment tables.

Not accepting the esr uplift request since ESR is unaffected.
Attachment #8411468 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24-
I was needinfo'd, but I'll just say what Gregor said. This is very low risk and we should take it.
Flags: needinfo?(wmccloskey)
Al, you cleared the [checkin on 5/20] whiteboard thing. Just to be sure, does this mean we should land this now?
Flags: needinfo?(abillings)
Yes, Jan. Paul and others said this needs to land ASAP for b2g timing.
Flags: needinfo?(abillings)
(In reply to Al Billings [:abillings] from comment #27)
> Yes, Jan. Paul and others said this needs to land ASAP for b2g timing.

OK, thanks, let's get this in.
Flags: needinfo?(shu)
https://hg.mozilla.org/integration/mozilla-inbound/rev/64356b36da13

Please request Aurora/Beta approval on this ASAP to expedite it being uplifted everywhere else it needs to go.
Keywords: checkin-needed
Assignee

Updated

5 years ago
Attachment #8411468 - Flags: approval-mozilla-aurora?
Assignee

Updated

5 years ago
Flags: needinfo?(shu)
Comment on attachment 8411468 [details] [diff] [review]
Clear TypeCompartment tables.

[Triage Comment]
As the tracking flags say (and RyanVM), we need it for both 30 & 31 (Beta & Aurora).
Attachment #8411468 - Flags: approval-mozilla-beta+
Attachment #8411468 - Flags: approval-mozilla-aurora?
Attachment #8411468 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/64356b36da13
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Group: javascript-core-security → core-security
Whiteboard: [adv-main30+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.