1073577 - Objects can be put into the wrong entry in the new object cache after a minor GC

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

The new object cache is partly keyed based on object address.  The computed entry index is not re-looked up after operations that can GC and cause the key to be moved, so a entry can be inserted in the wrong place.  If a subsequent lookup happens with the same class and kind but with a new key object allocated at the same address as the original key, the wrong entry will be returned.

I'm not entirely sure of the implications of this so I'm marking security sensitive.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: fix-new-object-cache

This patch checks the gc number on the one path where this can happen, and adds an assertion to check the entry index is correct when adding.

Comment 2

[Terrence Cole [:terrence]]

Why does this not also affect the two other newObjectCache usages in jsobj.cpp and the one in jsarray.cpp?

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #2)
The other uses are keyed off either a global object or a type object, and we don't move any of those.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 8496056 [details] [diff] [review]
fix-new-object-cache

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

Ah, right!

Comment 5

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f33f7dc50aa9

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f33f7dc50aa9

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8496056 [details] [diff] [review]
fix-new-object-cache

[Security approval request comment]

I should have requested security approval before landing this one too, so sorry about that.

How easily could an exploit be constructed based on the patch?

Definitely hard.

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

The addition of a check for GC number does indicate there is a problem if GC happens here.  But I think it would still be hard to trigger this.

Which older supported branches are affected by this flaw?

Everything back to FF 32 since we enabled GGC on this.

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

Generational GC enabled on desktop in bug 619558.

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

Not yet.  Also the backport will need to include the fix for bug 1075546.

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

I think it's unlikely but we should probably give it a week for the fuzzers to find any other potential issues.

Comment 8

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

Comment on attachment 8496056 [details] [diff] [review]
fix-new-object-cache

This is too late to make it into 33. We're building soon for the release build. We should get this on Aurora though. Can you make and nominate an Aurora patch?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

(In reply to Al Billings [:abillings] from comment #8)
> This is too late to make it into 33. We're building soon for the release
> build. We should get this on Aurora though. Can you make and nominate an
> Aurora patch?

FYI, this and bug 1075546 apply cleanly to Aurora as-is.

Comment 10

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

Let's get this nominated, approved, and then into Aurora then.

Comment 11

[Jon Coppeard (:jonco)]

Comment on attachment 8496056 [details] [diff] [review]
fix-new-object-cache

Approval Request Comment
[Feature/regressing bug #]: Bug 619558
[User impact if declined]: Potential security vulnerability
[Describe test coverage new/current, TBPL]: On central for 1 week.
[Risks and why]: Low
[String/UUID change made/needed]: None

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8496056 [details] [diff] [review]
fix-new-object-cache

This is too late for 33 but OK for 34.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fef8fb959d28