Closed
Bug 1166932
Opened 9 years ago
Closed 9 years ago
[e10s]&[non-e10s] - all tabs crash when quickly typing several chars into Facebook's "To:" messenger field
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
VERIFIED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
e10s | m7+ | --- |
firefox38 | --- | unaffected |
firefox38.0.5 | --- | unaffected |
firefox39 | --- | unaffected |
firefox40 | + | verified |
firefox41 | + | verified |
firefox-esr31 | --- | unaffected |
firefox-esr38 | --- | unaffected |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.1S | --- | unaffected |
b2g-v2.2 | --- | unaffected |
b2g-master | --- | fixed |
People
(Reporter: kjozwiak, Assigned: ehsan.akhgari)
References
Details
(Keywords: sec-critical)
Crash Data
Attachments
(3 files)
294.44 KB,
image/gif
|
Details | |
1.54 KB,
patch
|
roc
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
983 bytes,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
Selecting the "To:" field under Facebook's messenger and quickly typing in several characters right after you've loaded the page will crash all the tabs that are currently opened in fx. I can reproduce this consistently and added a quick .gif to illustrate the crash. (in the gif, I quickly select the "To:" field and type in "sonj") OS's: ----- * OSX 10.10.3 x64 - Reproduced several times * Win 8.1 x64 VM - Reproduced several times STR: ---- - login into facebook and select "Messenger -> See All" - Select "New Message" - as soon as the page loads, quickly select the "To:" field and start typing in a name (should instantly crash) Might be related to bug # 1165369 as I also got the same crash report once when reproducing this: (the three listed below are more common) * https://crash-stats.mozilla.com/report/index/fc94ce3d-fd40-4e87-b451-24b342150520 Crash Reports: -------------- - https://crash-stats.mozilla.com/report/index/c94af882-c8dd-4835-924c-fa5a02150520 [OSX] - https://crash-stats.mozilla.com/report/index/be243602-a4a0-4a65-9397-ef2b72150520 [Windows]
Comment 1•9 years ago
|
||
Does the issue happen on FF40 too?
Reporter | ||
Comment 2•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #1) > Does the issue happen on FF40 too? Yup, reproduced it with fx40 using the same steps that are listed above. - Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-05-20-00-40-04-mozilla-aurora/ - Crash: https://crash-stats.mozilla.com/report/index/e420af19-13a7-4fcd-9062-a85452150520
status-firefox40:
--- → affected
Updated•9 years ago
|
Assignee: nobody → mrbkap
tracking-e10s:
--- → m7+
Reporter | ||
Comment 3•9 years ago
|
||
Quickly checked to see if bug # 1166742 might have fixed the crash, unfortunately it's still crashing with today's m-c. - Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-05-21-03-02-04-mozilla-central/ - Crash: https://crash-stats.mozilla.com/report/index/3177a6f5-3ef7-419e-90ac-0698f2150521
Comment 4•9 years ago
|
||
[Tracking Requested - why for this release]: The nsEditor::NotifyEditorObservers signature is #2 in yesterday's Dev Edition 40 data.
tracking-firefox40:
--- → ?
Comment 5•9 years ago
|
||
I can reproduce the crash on Nightly41.0a1(201-05-21) without e10s. Browser crashes with crash reporter. bp-91965820-e5c5-4d19-9514-c1dba2150521
Summary: [e10s] - all tabs crash when quickly typing several chars into Facebook's "To:" messenger field → [e10s]&[non-e10s] - all tabs crash when quickly typing several chars into Facebook's "To:" messenger field
Comment 6•9 years ago
|
||
(In reply to Kamil Jozwiak [:kjozwiak] from comment #2) > (In reply to Olli Pettay [:smaug] from comment #1) > > Does the issue happen on FF40 too? > > Yup, reproduced it with fx40 using the same steps that are listed above. So very much looks like a regression from bug 1154701. I assume one of those for (i = 0; i < array.length; ++i) -> for (item: array) changes caused this. those are rather different ways to iterate array, especially from security point of view.
Flags: needinfo?(ehsan)
Flags: needinfo?(ayg)
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7) > This is very likely a use after free. And it is. We're accessing what an iterator points to after we mutate the array to remove the item.
Flags: needinfo?(ehsan)
Keywords: sec-critical
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8608951 -
Flags: review?(roc)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8608952 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8608951 [details] [diff] [review] Part 1: Copy the editor observers array before iterating over it (Note: these approvals are being requested for both of the patches here.) [Security approval request comment] How easily could an exploit be constructed based on the patch? The first patch makes the issue pretty obvious, but it's impossible to fix this issue in another way. It is not easy to go from the patch to an exploit, however. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? See above. Which older supported branches are affected by this flaw? Only Nightly and Aurora. If not all supported branches, which bug introduced the flaw? Bug 1154701 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I'd prefer to fix this, the backout may be regression prone. How likely is this patch to cause regressions; how much testing does it need? It technically changes the behavior for running the edit actions, but I don't expect it to be very regression prone. At any rate, we do have some baking time. Approval Request Comment [Feature/regressing bug #]: Bug 1154701. [User impact if declined]: Comment 0. [Describe test coverage new/current, TreeHerder]: Locally tested. [Risks and why]: See above. [String/UUID change made/needed]: None.
Attachment #8608951 -
Flags: sec-approval?
Attachment #8608951 -
Flags: approval-mozilla-aurora?
Attachment #8608951 -
Flags: review?(roc) → review+
Attachment #8608952 -
Flags: review?(roc) → review+
Jesse, maybe some fuzzing coverage is needed here?
Flags: needinfo?(jruderman)
Updated•9 years ago
|
status-firefox39:
--- → unaffected
status-firefox-esr31:
--- → unaffected
status-firefox-esr38:
--- → unaffected
Comment 15•9 years ago
|
||
Comment on attachment 8608951 [details] [diff] [review] Part 1: Copy the editor observers array before iterating over it Checkin all the things!
Attachment #8608951 -
Flags: sec-approval?
Attachment #8608951 -
Flags: sec-approval+
Attachment #8608951 -
Flags: approval-mozilla-aurora?
Attachment #8608951 -
Flags: approval-mozilla-aurora+
Comment 16•9 years ago
|
||
(Replying bug 1162360 comment 41) I think I was indeed trying to solve the same issue there, but just by reading the descriptions I don't know if these patches work for my use case. I explicitly choose not to copy the array before iterating it because we do need to notify the observers added while the loop is being run on. Should we instead landing my patch in this bug? Anyhow, I am going to read the patches here and try them too on Monday.
Comment 17•9 years ago
|
||
> I explicitly choose not to copy the array before iterating it because we do need to notify
> the observers added while the loop is being run on.
I agree with this.
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (working slowly due to bad health condition) from comment #17) > > I explicitly choose not to copy the array before iterating it because we do need to notify > > the observers added while the loop is being run on. > > I agree with this. Which observers do we need to run? If you know of one where this will end up doing the wrong behavior, please file a new bug for it and needinfo me on it so that we can discuss that. Thanks!
Comment 19•9 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #18) > Which observers do we need to run? If you know of one where this will end > up doing the wrong behavior, please file a new bug for it and needinfo me on > it so that we can discuss that. Thanks! Let me re-phrase bug 1162360 comment 11: On B2G, we would have 3 observers in the observers array 0: nsTextInputListener 1: FormAssistant (in forms.js) 2: IMEContentObserver The part 1 patch of bug 1162360 is applied, FormAssistant.EditAction will eventually cause IMEContentObserver at 2 to be destroyed and a new one added being assigned in its place. A range-based for loop will attempt to call on the old one, hence the crash. we will ask RemoveEditorObserver do things differently: when the old one got replaced, we will replace it with a nullptr so it still keep its place. The newly added IMEContentObserver will be assigned in the 3rd place. 0: nsTextInputListener 1: FormAssistant (in forms.js) 2: nullptr (IMEContentObserver removed cause by action in FormAssistant) 3: IMEContentObserver (added cause by action in FormAssistant) And a traditional for loop will ensure we'll always run to to the very end so the added one gets notified. nullptr will get cleaned up once the loop is finished. The first patch here (copying the array) will not prevent the crash in the case mentioned, and it will miss the newly added IMEContentObserver.
Comment 20•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/13b7d0d6301a https://hg.mozilla.org/mozilla-central/rev/6d79c472fe57
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 21•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/60418e3204f8 https://hg.mozilla.org/releases/mozilla-aurora/rev/1642ba0f72b4
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → unaffected
status-b2g-master:
--- → fixed
status-firefox38:
--- → unaffected
status-firefox38.0.5:
--- → unaffected
Flags: in-testsuite?
Comment 22•9 years ago
|
||
Ehsan, could you respond to comment 19 and advice how to move bug 1162360 forward since this is landed? Should I just rebase that patch against this one?
Flags: needinfo?(ehsan)
Comment 23•9 years ago
|
||
I was wrong at comment 19; after pulling the latest m-c and include the fix here, the patch there no longer crash the emulator on test. I am still not sure if the patch here is the right behavior though, but without a test failure I would like to land the first patch once the depended gaia test fixes is merged. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e59f08d4cb79
Assignee | ||
Comment 24•9 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #19) > The first patch here (copying the array) will not prevent the crash in the > case mentioned, and it will miss the newly added IMEContentObserver. The problem with your patch #2 on that bug is that it will cause us to call EditAction() on observers that were not registered at the time that the editing operation happened, which is incorrect in the general case for other observers. Couldn't you call the EditAction manually when you register the observer as part of another observer's EditAction()? (FWIW, we can move this discussion to bug 1162360, I think.)
Flags: needinfo?(ehsan)
Reporter | ||
Comment 25•9 years ago
|
||
Went through verification using the following builds: - http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-05-26-03-02-02-mozilla-central/ - http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-05-26-00-40-04-mozilla-aurora/ Tested with the following OS's: - Win 8.1 x64 VM - PASSED - OSX 10.10.3 - PASSED - Ubuntu 14.0 - PASSED I can't reproduce the crash anymore, went through both m-c and m-a on the above OS's without any problems. Ran through the case in comment #0 several times without any problems.
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Group: core-security-release
Flags: needinfo?(jruderman)
Assignee | ||
Updated•4 years ago
|
Flags: in-testsuite? → in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•