Closed Bug 1149526 Opened 5 years ago Closed 5 years ago

Check HeapPtrs have GC lifetime

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox37 --- wontfix
firefox38 + fixed
firefox39 + fixed
firefox40 + fixed
firefox-esr31 --- unaffected
firefox-esr38 --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main38+][b2g-adv-main2.2+])

Attachments

(7 files, 1 obsolete file)

The HeapPtr wrapper template is designed to be used only in object which have GC lifetime, i.e. they are only ever part of a GC thing and are destroyed when we sweep that thing.

I added some assertions to test this and found there are a few cases where HeapPtrs are deleted outside of GC sweeping.  This is potentially bad since for a HeapPtr<JSObject*> we could try and mark a store buffer entry that points at it after it has been destroyed.

I don't think we've seen any crashes like this so it's possible that the JSObject ones are only used with tenured objects, but I'm marking this security sensitive just in case.
Here's a first attempt at adding the assertions and fixing the places that broke (these were traced to things being deleted outside of the GC).

This gets the current zone by looking at the value in the HeapPtr<> and checks that we are sweeping that zone if we are on the main thread.  This doesn't work for cross compartment pointers - but maybe these are all HeapSlots?

I'm also wondering if I should add some state in the zone to say 'this zone is currently being swept in the background' to make that check more precise.
Attachment #8586165 - Flags: feedback?(terrence)
Comment on attachment 8586165 [details] [diff] [review]
bug1149526-check-heapptr-lifetime

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

\o/  This is a huge improvement! We should definitely add state to the zone to make this assertion even tighter; I'd even be fine landing this patch as-is and doing that as a followup, if it's more convenient that way.
Attachment #8586165 - Flags: feedback?(terrence) → feedback+
Great!  Here's the updated patch.
Attachment #8586165 - Attachment is obsolete: true
Attachment #8587272 - Flags: review?(terrence)
Comment on attachment 8587272 [details] [diff] [review]
bug1149526-check-heapptr-lifetime v2

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

Looks good!

::: js/src/gc/Zone.h
@@ +309,3 @@
>      bool jitUsingBarriers_;
>  
> +

Extra newline.
Attachment #8587272 - Flags: review?(terrence) → review+
Comment on attachment 8587272 [details] [diff] [review]
bug1149526-check-heapptr-lifetime v2

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

Very difficult.

The issue here is that a HeapPtrObject might be destroyed outside of a GC while it holds a nursery pointer.  It's not clear whether this is possible but it's hard to analyse all the paths that might lead to it so best to be safe.

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 FF32.

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?

They will require merging, and the type inference code has probably changed a bit, but backport patches will be mostly the same.

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

Unlikely.
Attachment #8587272 - Flags: sec-approval?
Comment on attachment 8587272 [details] [diff] [review]
bug1149526-check-heapptr-lifetime v2

sec-approval+. Let's get this nominated on Aurora and Beta too.
Attachment #8587272 - Flags: sec-approval? → sec-approval+
Backed out for static analysis bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f40229bf323f
https://hg.mozilla.org/mozilla-central/rev/3ccd83f8e32b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Please nominate this for Aurora/Beta approval ASAP so we can hopefully get it landed in time for 38b3.
Flags: needinfo?(jcoppeard)
Depends on: 1152623
Depends on: 1152635
AWFY detected a regression/improvement:

First landing: http://arewefastyet.com/regressions/#/regression/139660 (12% regression on misc-f32-markov).
Backout: http://arewefastyet.com/regressions/#/regression/139682 (misc-f32-markov back to normal).

But I have good news. The relanding didn't trigger the regression. So you are clear ;).
Comment on attachment 8587272 [details] [diff] [review]
bug1149526-check-heapptr-lifetime v2

Approval Request Comment
[Feature/regressing bug #]: Bug 619558.
[User impact if declined]: Possible crashes / security vulnerability.
[Describe test coverage new/current, TreeHerder]: On mozilla-central since 9th April.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8587272 - Flags: approval-mozilla-beta?
Attachment #8587272 - Flags: approval-mozilla-aurora?
Comment on attachment 8587272 [details] [diff] [review]
bug1149526-check-heapptr-lifetime v2

Actually given that there are fuzz bugs reported caused by this, let's not do this yet.
Attachment #8587272 - Flags: approval-mozilla-beta?
Attachment #8587272 - Flags: approval-mozilla-aurora?
As evidenced by the fuzz bugs filed, this approach doesn't work too well because it involves trying to the zone of the containing object by examining the referent, which may be been finalized at this point.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
If we use TLS to work out whether the current thread is doing GC sweeping then everything becomes much simpler.  Also, it caught a couple more places where we need to use RelocatablePtr.
Attachment #8590288 - Flags: review?(terrence)
Comment on attachment 8590288 [details] [diff] [review]
bug1149526-use-tls-to-check-heap-ptr-lifetime

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

Yeah, that's much nicer.
Attachment #8590288 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/909e17a7edb7
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
The branch patches still need separate approval to land.
A b2g37 patch is going to be needed here. Not sure about the older B2G branches yet.
Flags: needinfo?(jcoppeard)
Depends on: 1153498
Jon, could you fill the uplift request to the various affected branches? Thanks
Comment on attachment 8590716 [details] [diff] [review]
bug1149526-aurora-patch

Approval Request Comment
[Feature/regressing bug #]: Bug 619558.
[User impact if declined]: Possible crashes / security vulnerability.
[Describe test coverage new/current, TreeHerder]: On mozilla-central since 9th April.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8590716 - Flags: approval-mozilla-aurora?
Comment on attachment 8590759 [details] [diff] [review]
bug1149526-beta-patch

Approval Request Comment
[Feature/regressing bug #]: Bug 619558.
[User impact if declined]: Possible crashes / security vulnerability.
[Describe test coverage new/current, TreeHerder]: On mozilla-central since 9th April.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Attachment #8590759 - Flags: approval-mozilla-beta?
Comment on attachment 8590759 [details] [diff] [review]
bug1149526-beta-patch

Should be in 38 beta 5.
Attachment #8590759 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8590716 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
My needinfo was for B2G rebasing :)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #30)
Oh right, yes I missed that.  Patch coming up.
Flags: needinfo?(jcoppeard)
NOTE: 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 #): Bug 619558.
User impact if declined: Possible crashes / security vulnerability.
Testing completed: On mozilla-central since 9th April.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.
Attachment #8592243 - Flags: approval-mozilla-b2g37?
Comment on attachment 8592243 [details] [diff] [review]
bug1149526-b2g-37-patch

sec-high bugs don't need extra approval for B2G so long as the affected Fx branches are taken care of :)
Attachment #8592243 - Flags: approval-mozilla-b2g37?
I made it about halfway through rebasing this for b2g34 before I hit enough conflicts to call it quits. Might as well look at b2g32 while you're at it since I'm sure that won't be any better.
Flags: needinfo?(jcoppeard)
Flags: needinfo?(jcoppeard)
Whiteboard: [adv-main38+]
Whiteboard: [adv-main38+] → [adv-main38+][b2g-adv-main2.2+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.