Note: There are a few cases of duplicates in user autocompletion which are being worked on.

OOM Crash [@ nsCOMArray<nsISelectionListener>::operator[]] due to unhandled alloc failure in nsTypedSelection::NotifySelectionListeners

RESOLVED FIXED in Firefox 12

Status

()

Core
Layout
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Assigned: Mats Palmgren (vacation - back in August))

Tracking

(Blocks: 1 bug, {assertion, crash})

Trunk
mozilla13
x86_64
Linux
assertion, crash
Points:
---

Firefox Tracking Flags

(firefox12 fixed, firefox13 fixed, firefox-esr1012+ fixed)

Details

(Whiteboard: [sg:critical][qa-], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Tested on m-c revision 8ea5c983743f: The code in nsTypedSelection::NotifySelectionListeners does not properly handle an OOM condition (I assume it's while cloning selectionListeners) that leads to the following assertion and crash:

###!!! ASSERTION: nsVoidArray::FastElementAt: index out of range: '0 <= aIndex && aIndex < Count()', file ../../dist/include/nsVoidArray.h, line 78

Program received signal SIGSEGV, Segmentation fault.
nsCOMArray<nsISelectionListener>::operator[] (this=0x7fffffff8620, aIndex=0) at ../../dist/include/nsCOMArray.h:186
186             return ObjectAt(aIndex);
#0  nsCOMArray<nsISelectionListener>::operator[] (this=0x7fffffff8620, aIndex=0) at ../../dist/include/nsCOMArray.h:186
#1  0x00002aaaac7382ed in nsTypedSelection::NotifySelectionListeners (this=0x4e90470) at /srv/repos/browser/mozilla-central/layout/generic/nsSelection.cpp:5712
#2  0x00002aaaac738408 in nsTypedSelection::EndBatchChanges (this=<optimized out>) at /srv/repos/browser/mozilla-central/layout/generic/nsSelection.cpp:5734
#3  0x00002aaaacbb44e2 in nsEditor::DoTransaction (this=0x5c71bb0, aTxn=0x5c72210) at /srv/repos/browser/mozilla-central/editor/libeditor/base/nsEditor.cpp:716
#4  0x00002aaaacbacea0 in nsEditor::InsertNode (this=0x5c71bb0, aNode=0x5c72108, aParent=0x4e90668, aPosition=0) at /srv/repos/browser/mozilla-central/editor/libeditor/base/nsEditor.cpp:1404
#5  0x00002aaaacba81cd in CreateBogusNodeIfNeeded (aSelection=0x4e90470, this=0x5c71ee0) at /srv/repos/browser/mozilla-central/editor/libeditor/text/nsTextEditRules.cpp:1168
#6  nsTextEditRules::CreateBogusNodeIfNeeded (this=0x5c71ee0, aSelection=0x4e90470) at /srv/repos/browser/mozilla-central/editor/libeditor/text/nsTextEditRules.cpp:1115
#7  0x00002aaaacba97eb in Init (aEditor=<optimized out>, this=0x5c71ee0) at /srv/repos/browser/mozilla-central/editor/libeditor/text/nsTextEditRules.cpp:139
=> 0x2aaaac736e73 <nsCOMArray<nsISelectionListener>::operator[](int) const+73>: mov    0x8(%rax,%rbx,8),%rax
rax            0x0      0
rbx            0x0      0

(gdb) f 1
#1  0x00002aaaac7382ed in nsTypedSelection::NotifySelectionListeners (this=0x4fef960) at /srv/repos/browser/mozilla-central/layout/generic/nsSelection.cpp:5712
5712        nsISelectionListener* thisListener = selectionListeners[i];
(gdb) p selectionListeners
$1 = {<nsCOMArray_base> = {mArray = {mImpl = 0x0}}, <No data fields>}


The backtrace of the failing allocation is as follows:

#0 /srv/repos/browser/mozilla-central/objdir-ff-gcc64dbg/dist/bin/libmozalloc.so(moz_malloc+0x5f)
#1 nsVoidArray::SizeTo(int) at objdir-ff-gcc64dbg/xpcom/build/nsVoidArray.cpp:231
#2 nsVoidArray::Count() const at xpcom/glue/nsVoidArray.h:67
#3 nsCOMPtr<nsIPresShell>::begin_assignment() at objdir-ff-gcc64dbg/dist/include/nsCOMPtr.h:1241 
#4 nsTypedSelection::EndBatchChanges() at layout/generic/nsSelection.cpp:5737
#5 nsEditor::DoTransaction(nsITransaction*) at editor/libeditor/base/nsEditor.cpp:702
#6 nsEditor::InsertNode(nsIDOMNode*, nsIDOMNode*, int) at editor/libeditor/base/nsEditor.cpp:1404
#7 nsTextEditRules::CreateBogusNodeIfNeeded(nsISelection*) at editor/libeditor/text/nsTextEditRules.cpp:1169
#8 nsTextEditRules::Init(nsPlaintextEditor*) at editor/libeditor/text/nsTextEditRules.cpp:140
#9 nsPlaintextEditor::EndEditorInit() at editor/libeditor/text/nsPlaintextEditor.cpp:221
#10 ~nsAutoEditInitRulesTrigger at editor/libeditor/text/nsTextEditUtils.cpp:123
#11 nsPlaintextEditor::Init(nsIDOMDocument*, nsIContent*, nsISelectionController*, unsigned int) at editor/libeditor/text/nsPlaintextEditor.cpp:159
#12 nsTextEditorState::PrepareEditor(nsAString_internal const*) at content/html/content/src/nsTextEditorState.cpp:1225
#13 nsTextEditorState::GetEditor() at content/html/content/src/nsTextEditorState.cpp:1010
#14 ns_if_addref<nsIEditor*> at objdir-ff-gcc64dbg/dist/include/nsISupportsUtils.h:93
#15 /srv/repos/browser/mozilla-central/objdir-ff-gcc64dbg/dist/bin/libxul.so(NS_InvokeByIndex_P+0x23a)


I'm marking this as s-s for now because it's not clear to me what mSelectionListeners contains exactly, especially if the attacker can control how many listeners are in there (which could turn the crash into a controlled read). However, even in that case, exploitation is likely very hard because creating a controlled OOM in that location is surely not trivial.
Assignee: nobody → matspal
Created attachment 604662 [details] [diff] [review]
fix

Check that we copied the whole array.

I'm also removing the null-check inside the loop since
nsTypedSelection::AddSelectionListener rejects null and mSelectionListeners
isn't inserted to from anywhere else.

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=b147664bf01a
Attachment #604662 - Flags: review?(bzbarsky)
'thisListener' used to be the result of a QI at some point so that's where the
null-check came from:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsSelection.cpp&rev=3.1&root=/cvsroot#1849

Comment 3

5 years ago
Comment on attachment 604662 [details] [diff] [review]
fix

r=me
Attachment #604662 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb3752c717c
Whiteboard: [inbound]
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/eeb3752c717c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
This looks like a null deref unless I'm reading over this too quickly. If I've gotten that wrong please nominate for the ESR branch (and ask for approval on the patch).
status-firefox-esr10: --- → wontfix
status-firefox13: --- → fixed
Whiteboard: [sg:dos null deref]
It's not a null deref.  We're reading beyond the array buffer end and interpreting
that as a nsISelectionListener object and calling a virtual function on that
bogus object.
Whiteboard: [sg:dos null deref] → [sg:critical]
Comment on attachment 604662 [details] [diff] [review]
fix

see comment 7.  Low risk.
Attachment #604662 - Flags: approval-mozilla-beta?
status-firefox-esr10: wontfix → ?
tracking-firefox-esr10: --- → ?

Comment 9

5 years ago
Comment on attachment 604662 [details] [diff] [review]
fix

[Triage Comment]
Low risk sg: crit fix. Please land on m-b and prepare a patch for the ESR.
Attachment #604662 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

5 years ago
status-firefox-esr10: ? → affected
tracking-firefox-esr10: ? → 12+
https://hg.mozilla.org/releases/mozilla-beta/rev/8486e1b8a256
status-firefox12: --- → fixed
Comment on attachment 604662 [details] [diff] [review]
fix

The patch also applies to esr10.
Attachment #604662 - Flags: approval-mozilla-esr10?
Comment on attachment 604662 [details] [diff] [review]
fix

low risk, go ahead and land on ESR.
Attachment #604662 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Is there anything QA can do to verify this fix?
Whiteboard: [sg:critical] → [sg:critical][qa?]
https://hg.mozilla.org/releases/mozilla-esr10/rev/076f467733b4
status-firefox-esr10: affected → fixed
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #13)
> Is there anything QA can do to verify this fix?

Christian triggered this crash with a OOM testing tool, presumably it's
repeatable.
(Reporter)

Comment 16

5 years ago
There is no portable solution yet to trigger this crash because my tool uses absolute addresses to indicate which functions to cause failure in. I'm not entirely sure if there can be any way to test this in a portable way without instrumenting the particular code in question.
Christian, can you verify that this is fixed on trunk and, if you have builds, 12?
qa- for verification given recent comments. Christian is there any assistance you can give in verifying this for Firefox 12, 13 and ESR 10.0.4?
Whiteboard: [sg:critical][qa?] → [sg:critical][qa-]
(Reporter)

Comment 19

5 years ago
Unfortunately there is no direct way for me to verify this, other than performing another full run of my scripts on the initial test/url which takes really long.

The reason for this is that there is no portable way (portable across builds) to indicate where an OOM condition should be triggered. I might be working on that in the future but right now, we can't verify this directly.
Okay, thanks Christian. Does this go for all OOM crashes?
(Reporter)

Comment 21

5 years ago
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #20)
> Okay, thanks Christian. Does this go for all OOM crashes?

At least for those that I filed blocking bug 687256, yes.

For other OOM crashes (e.g. those that Bob Clary files), it is usually also very hard to verify a fix, because OOM conditions are very fragile, so not seeing a crash anymore does not mean it is fixed.

The only OOM conditions that can somewhat be verified fixed are those triggered in the JS shell, either by a fuzz test or those involving -A parameter. The parameter varies with builds too, but it's possible to cover a certain range for that parameter to ensure the issue is gone, and I do have scripts for that.
Group: core-security
You need to log in before you can comment on or make changes to this bug.