Closed Bug 1137326 Opened 5 years ago Closed 5 years ago

Avoid compartment iterator invalidation

Categories

(Core :: JavaScript: GC, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 --- wontfix
firefox37 + fixed
firefox38 + fixed
firefox39 + fixed
firefox-esr31 37+ fixed
b2g-v1.4 --- fixed
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: sfink, Assigned: sfink)

References

Details

(Keywords: csectype-bounds, sec-high, Whiteboard: [adv-main37+][adv-esr31.6+])

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1120655 +++

Separate bug for the patch "Fix out of bounds error in JS_iterateCompartments" from bug 1120655 so that it can land before the other parts.
Ugh. bzexport fail when trying to move to a different bug.
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment on attachment 8569987 [details] [diff] [review]
Fix out of bounds error in JS_iterateCompartments,

Carrying over terrence's r+.
Attachment #8569987 - Flags: review+
Comment on attachment 8569987 [details] [diff] [review]
Fix out of bounds error in JS_iterateCompartments,

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

Difficult. Maybe you could do it with some iframe juggling? But then you'd need to have an iterator going too, which doesn't really happen from user code. Theoretically possible, I guess, but I don't know how to do it.

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

The patch name says "out of bounds error".

Which older supported branches are affected by this flaw?

I would guess this goes way back. Perhaps not as far as the introduction of compartments in FF4, but it doesn't depend on moving GC so it might come before that.

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?

They will be semantically the same, but they might need to be done to different code once we move past the introduction of zones. (I looked at beta; this patch should apply there unchanged.) They won't be any riskier than the patch to the current source in terms of introducing new problems, and it's low risk.

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

Fairly unlikely. It switches a rare failure mode from being an out of bounds access into a safe but incomplete iteration. I doubt any of our tests cover it; you'd probably need to run about:memory while closing tabs or something?
Attachment #8569987 - Flags: sec-approval?
Summary: Make the analysis detect compartment iterator invalidation → Avoid compartment iterator invalidation
Comment on attachment 8569987 [details] [diff] [review]
Fix out of bounds error in JS_iterateCompartments,

sec-approval+. We should get this on branches as well once it is on trunk.
Attachment #8569987 - Flags: sec-approval? → sec-approval+
Comment on attachment 8569987 [details] [diff] [review]
Fix out of bounds error in JS_iterateCompartments,

Approval Request Comment
[Feature/regressing bug #]: compartments (FF4 timeframe)
[User impact if declined]: difficult to exploit out of bounds access, probably not exposed to content code
[Describe test coverage new/current, TreeHerder]: just landed on mozilla-inbound
[Risks and why]: low risk. Turns an out of bound access into a safe but incomplete iteration.
[String/UUID change made/needed]: none
Attachment #8569987 - Flags: approval-mozilla-beta?
Attachment #8569987 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/cde1ceac0788
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Attachment #8569987 - Flags: approval-mozilla-beta?
Attachment #8569987 - Flags: approval-mozilla-beta+
Attachment #8569987 - Flags: approval-mozilla-aurora?
Attachment #8569987 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/2a92625b924e
https://hg.mozilla.org/releases/mozilla-beta/rev/10eff960b898

Looks like we need an esr31 nomination here too? Also, setting in-testsuite? here presuming it can be changed to + once bug 1120655 lands.
Flags: needinfo?(sphink)
Flags: in-testsuite?
Comment on attachment 8569987 [details] [diff] [review]
Fix out of bounds error in JS_iterateCompartments,

[Approval Request Comment]
Same as the other a? for aurora,beta.
Flags: needinfo?(sphink)
Attachment #8569987 - Flags: approval-mozilla-esr31?
Attachment #8569987 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Whiteboard: [adv-main37+][adv-esr31.6+]
Group: javascript-core-security
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.