Closed Bug 1073577 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 + fixed
firefox35 --- fixed
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- disabled
b2g-v2.0M --- disabled
b2g-v2.1 --- disabled
b2g-v2.2 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main34+])

Attachments

(1 file)

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.
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.
Attachment #8496056 - Flags: review?(terrence)
Why does this not also affect the two other newObjectCache usages in jsobj.cpp and the one in jsarray.cpp?
Flags: needinfo?(jcoppeard)
(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.
Flags: needinfo?(jcoppeard)
Comment on attachment 8496056 [details] [diff] [review]
fix-new-object-cache

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

Ah, right!
Attachment #8496056 - Flags: review?(terrence) → review+
Keywords: sec-audit
https://hg.mozilla.org/mozilla-central/rev/f33f7dc50aa9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1075546
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.
Attachment #8496056 - Flags: sec-approval?
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?
Attachment #8496056 - Flags: sec-approval? → sec-approval+
(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.
Let's get this nominated, approved, and then into Aurora then.
Flags: needinfo?(jcoppeard)
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
Attachment #8496056 - Flags: approval-mozilla-beta?
Attachment #8496056 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jcoppeard)
Comment on attachment 8496056 [details] [diff] [review]
fix-new-object-cache

This is too late for 33 but OK for 34.
Attachment #8496056 - Flags: approval-mozilla-beta?
Attachment #8496056 - Flags: approval-mozilla-beta-
Attachment #8496056 - Flags: approval-mozilla-aurora?
Attachment #8496056 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main34+]
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.