Closed Bug 1322390 Opened 3 years ago Closed 3 years ago

TrackUnionStream::RemoveInput mutates mTrackMap[i].mOwnedDirectListeners while iterating over it

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: Nika, Assigned: Nika)

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [post-critsmash-triage])

Attachments

(1 file)

This isn't caught due to the use of range based for loops being used for iterating over the nsTArray.

This was caught by bug 1299489 and will not be a possible form of sec bug once bug 1299489 has landed.

I believe that this code landed in 51 - meaning that this bug is present in Aurora, Beta and Nightly.
Keywords: sec-high
MozReview-Commit-ID: PCLCEiKTFO
Attachment #8817271 - Flags: review?(pehrson)
Comment on attachment 8817271 [details] [diff] [review]
Iterate over a copy of mOwnedDirectListeners when removing them

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

Thanks!
Attachment #8817271 - Flags: review?(pehrson) → review+
Comment on attachment 8817271 [details] [diff] [review]
Iterate over a copy of mOwnedDirectListeners when removing them

[Security approval request comment]
How easily could an exploit be constructed based on the patch? 
I am not familiar enough with the classes which are affected here to know what level of control websites have over this array.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? 
I imagine that someone looking at this patch will be able to identify the security problem which it is fixing fairly easily, as this is a fairly straightforward iterator invalidation error.

Which older supported branches are affected by this flaw?
The code in question was introduced in Firefox 51.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? 
This patch will likely apply directly to all affected branches, and if it doesn't, correcting the patch such that it does should be trivial.

How likely is this patch to cause regressions; how much testing does it need?
This patch is unlikely to cause regressions.
Attachment #8817271 - Flags: sec-approval?
Sec-approval+ for trunk. Please nominate patches for Aurora and Beta since Beta is 51 and if we fix it back to beta, we won't ship this flaw.
Attachment #8817271 - Flags: sec-approval? → sec-approval+
Comment on attachment 8817271 [details] [diff] [review]
Iterate over a copy of mOwnedDirectListeners when removing them

I believe that this patch should apply on both beta and aurora without changes.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1259788
[User impact if declined]: Potential memory bug in Beta/Aurora
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: This just landed in nightly in https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=9f61239c41a0
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No
[Why is the change risky/not risky?]: This change makes a small change which should not change behavior except to avoid a memory safety trap.
[String changes made/needed]: None.
Attachment #8817271 - Flags: approval-mozilla-beta?
Attachment #8817271 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/9f61239c41a0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8817271 [details] [diff] [review]
Iterate over a copy of mOwnedDirectListeners when removing them

Fix a sec-high. Beta51+ and Aurora52+. Should be in 51 beta 7.
Attachment #8817271 - Flags: approval-mozilla-beta?
Attachment #8817271 - Flags: approval-mozilla-beta+
Attachment #8817271 - Flags: approval-mozilla-aurora?
Attachment #8817271 - Flags: approval-mozilla-aurora+
Group: core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.