1137326 - Avoid compartment iterator invalidation

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Steve Fink [:sfink] [:s:]]

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

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Fix out of bounds error in JS_iterateCompartments,

Ugh. bzexport fail when trying to move to a different bug.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8569987 [details] [diff] [review]
Fix out of bounds error in JS_iterateCompartments,

Carrying over terrence's r+.

Comment 3

[Steve Fink [:sfink] [:s:]]

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?

Comment 4

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

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.

Comment 5

[Steve Fink [:sfink] [:s:]]

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

Comment 6

[Steve Fink [:sfink] [:s:]]

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

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 9

[Steve Fink [:sfink] [:s:]]

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/10eff960b898

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/b897fa86b438
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/ca470cc0a674
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/3fc43021d56f
https://hg.mozilla.org/releases/mozilla-esr31/rev/434bb6ebc748

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/b897fa86b438
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/ca470cc0a674