Investigate editor's KungFuDeathGrips for potential security concerns

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Nika, Assigned: masayuki)

Tracking

({sec-audit})

Trunk
mozilla54
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main54-])

Attachments

(5 attachments, 9 obsolete attachments)

6.29 KB, patch
masayuki
: review+
dveditz
: sec-approval+
Details | Diff | Splinter Review
19.83 KB, patch
masayuki
: review+
dveditz
: sec-approval+
Details | Diff | Splinter Review
1.80 KB, patch
masayuki
: review+
dveditz
: sec-approval+
Details | Diff | Splinter Review
30.02 KB, patch
smaug
: review+
dveditz
: sec-approval+
Details | Diff | Splinter Review
20.80 KB, patch
dveditz
: sec-approval+
Details | Diff | Splinter Review
It would be a good idea to make sure that no functions depend on the value of, for example, mEditor, not changing thus causing the kungFuDeathGrips to not be keeping it alive anymore.

See bug 1018486 comment 122.
>> nsEditorEventListener::HandleEvent(nsIDOMEvent* aEvent)
> 
> No, this isn't quite right.  This method calls other methods that use mEditor, even though it doesn't use it itself.
> 
> Presumably this means we need to push the deathgrip into the callees that actually use mEditor, or have multiple deathgrips, or pass around the deathgrip value, or this deathgrip is not needed, or something.

I think that moving the deathgrip to the callees is the right approach.
Group: core-security → dom-core-security
Masayuki, is this something you will be able to work on? Thanks.
Flags: needinfo?(masayuki)
Or maybe Michael, because he filed this bug.
Flags: needinfo?(michael)
(In reply to Andrew McCreight [:mccr8] from comment #3)
> Or maybe Michael, because he filed this bug.

I originally filed this bug because I determined that I didn't understand enough about the KungFuDeathGrips within Editor to determine their safety, so I don't think I would be the best person to handle this bug.
Flags: needinfo?(michael)
Okay.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Before cleaning up EditorEventListener, we need to make each event handler method take nsIDOMUIEvent or its subclasse because checking method should take nsIDOMUIEvent.
Attachment #8826581 - Flags: review?(bugs)
EditorEventListener doesn't check if mEditorBase is available even after it's removed from the editor.  If it becomes nullptr, i.e., it's detached from editor, it shouldn't continue to handle event.
Attachment #8826583 - Flags: review?(bugs)
So, EditorEventListener should grab mEditorBase in smaller scope as far as possible.  And each event listener method shouldn't access mEditorBase directly at calling its method since it might be changed to another instance.
Attachment #8826584 - Flags: review?(bugs)
Currently, EventListenerManager calls WidgetEvent::PreventDefault() when the status is nsEventStatus_eConsumeNoDefault. That causes unexpected state of events.  So, WidgetEvent should do nothing when it's not cancelable but PreventDefault() is called.
Attachment #8826585 - Flags: review?(bugs)
Comment on attachment 8826581 [details] [diff] [review]
part.1 Change EditorBase::UpdateIMEComposition()'s argument from nsIDOMEvent* to WidgetCompositionEvent*

>@@ -776,17 +776,19 @@ EditorEventListener::HandleText(nsIDOMEv
>     return NS_OK;
>   }
> 
>   // if we are readonly or disabled, then do nothing.
>   if (mEditorBase->IsReadonly() || mEditorBase->IsDisabled()) {
>     return NS_OK;
>   }
> 
>-  return mEditorBase->UpdateIMEComposition(aTextEvent);
>+  WidgetCompositionEvent* compositionChangeEvent =
>+    aTextEvent->WidgetEventPtr()->AsCompositionEvent();
>+  return mEditorBase->UpdateIMEComposition(compositionChangeEvent);
> }
EditorEventListener::HandleText is a bit hard to follow. One needs to look at IsAcceptableInputEvent implementation and the callers of HandleText to see that 
aTextEvent->WidgetEventPtr()->AsCompositionEvent(); never returns null here.
Could you add a comment why aTextEvent->WidgetEventPtr()->AsCompositionEvent() is expected to return non-null here.

>+TextEditor::UpdateIMEComposition(WidgetCompositionEvent* aCompsitionChangeEvent)
> {
>-  MOZ_ASSERT(aDOMTextEvent, "aDOMTextEvent must not be nullptr");
>+  MOZ_ASSERT(aCompsitionChangeEvent,
>+             "aCompsitionChangeEvent must not be nullptr");
> 
aCompositionChangeEvent here and elsewhere, not aCompsitionChangeEvent

>-  WidgetCompositionEvent* compositionChangeEvent =
>-    aDOMTextEvent->WidgetEventPtr()->AsCompositionEvent();
>-  NS_ENSURE_TRUE(compositionChangeEvent, NS_ERROR_INVALID_ARG);
>-  MOZ_ASSERT(compositionChangeEvent->mMessage == eCompositionChange,
>-             "The internal event should be eCompositionChange");
>+  if (NS_WARN_IF(!aCompsitionChangeEvent)) {
>+    return NS_ERROR_INVALID_ARG;
>+  }
ok, we still have a null check here, good.
Attachment #8826581 - Flags: review?(bugs) → review+
Comment on attachment 8826582 [details] [diff] [review]
part.2 Change EditorBase::IsAcceptableInputEvent()'s argument from nsIDOMEvent* to WidgetGUIEvent*

s/FALSE/false/ in a comment.



I don't still understand what these event handling changes have to do with KungFuDeathGrips handling, but perhaps latter patches explain that :)
Attachment #8826582 - Flags: review?(bugs) → review+
Comment on attachment 8826583 [details] [diff] [review]
part.3 EditorEventListener should check if it's removed during a call of editor's method

>   nsAutoString eventType;
>   aEvent->GetType(eventType);
>   // We should accept "focus" and "blur" event even if it's synthesized with
>   // wrong interface for compatibility with older Gecko.
>   if (eventType.EqualsLiteral("focus")) {
>-    return Focus(aEvent);
>+    nsCOMPtr<nsIDOMUIEvent> uiEvent = do_QueryInterface(aEvent);
>+    return Focus(uiEvent);
>   }
>   if (eventType.EqualsLiteral("blur")) {
>-    return Blur(aEvent);
>+    nsCOMPtr<nsIDOMUIEvent> uiEvent = do_QueryInterface(aEvent);
>+    return Blur(uiEvent);
>   }
The comment above talks about synthesized events using wrong interface, but here you force one to use UIEvent.
This is not backwards compatible.  Either remove the comment and make this code use proper focus/blur events only, or keep using Event interface.
If the former, explain why that is ok.

>+  if (!mEditorBase->IsAcceptableInputEvent(keypressEvent) ||
>+      !CanEditorHandleEvent(aKeyEvent)) {
>     return NS_OK;
>   }
Hmm, this is a bit hard to follow. When is one supposed to call IsAcceptableInputEvent
and when CanEditorHandleEvent. The method names similar things - not the same, but similar.
Could we rename CanEditorHandleEvent to, hmm, DetachedFromEditorOrDefaultPrevented() ? I know that it long and horrible name
but at least one wouldn't need to look at each time what CanEditorHandleEvent means.


Why is it ok to remove kungFuDeathGrip in EditorEventListener::HandleEvent? What guarantees the editor object itself stays alive long enough?
Do we not regress https://bugzilla.mozilla.org/show_bug.cgi?id=798677 because we've added Disconnect to EditorEventListener and because the patch adds CanEditorHandleEvent calls?
Could you explain?
Attachment #8826583 - Flags: review?(bugs) → review-
Comment on attachment 8826585 [details] [diff] [review]
part.5 WidgetEvent should mark event as consumed if it's not cancelable

I wonder if we could assert in WidgetEvent level when one is trying to call its PreventDefault*() methods on non-cancelable event. But I guess this is fine too. Hopefully nothing relies on the current behavior internally :/

Event::PreventDefaultInternal already has 
if (!mEvent->mFlags.mCancelable) {
so JS calls wouldn't end up in WidgetEvent level to trigger possible assertion.
Attachment #8826585 - Flags: review?(bugs) → review+
Comment on attachment 8826584 [details] [diff] [review]
part.4 EditorEventListener should grab mEditorBase in smaller scope as far as possible and shouldn't access it directly during handling an event

Ah, this explains why the kungfuDeathGrip can be removed
Attachment #8826584 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (review request backlog because of a work week) from comment #15)
> I don't still understand what these event handling changes have to do with
> KungFuDeathGrips handling, but perhaps latter patches explain that :)

I don't like to create overload methods of CanEditorHandleEvent(renamed to DetachedFromEditorOrDefaultPrevented).  Therefore, I wanted to change each event listener method's argument to nsIDOMUIEvent.  Then, I need to append |AsEvent()| calls. Instead, using Widget*Event can reduce the code in a line and changing nsIDOMEvent to Widget*Event isn't so risky. That's the reason why I changed these methods to use Widget*Event.

(In reply to Olli Pettay [:smaug] (review request backlog because of a work week) from comment #16)
> Comment on attachment 8826583 [details] [diff] [review]
> part.3 EditorEventListener should check if it's removed during a call of
> editor's method
> 
> >   nsAutoString eventType;
> >   aEvent->GetType(eventType);
> >   // We should accept "focus" and "blur" event even if it's synthesized with
> >   // wrong interface for compatibility with older Gecko.
> >   if (eventType.EqualsLiteral("focus")) {
> >-    return Focus(aEvent);
> >+    nsCOMPtr<nsIDOMUIEvent> uiEvent = do_QueryInterface(aEvent);
> >+    return Focus(uiEvent);
> >   }
> >   if (eventType.EqualsLiteral("blur")) {
> >-    return Blur(aEvent);
> >+    nsCOMPtr<nsIDOMUIEvent> uiEvent = do_QueryInterface(aEvent);
> >+    return Blur(uiEvent);
> >   }
> The comment above talks about synthesized events using wrong interface, but
> here you force one to use UIEvent.
> This is not backwards compatible.  Either remove the comment and make this
> code use proper focus/blur events only, or keep using Event interface.
> If the former, explain why that is ok.

Nice catch! This is really my mistake. In the new patch, Focus() and Blur() will take nsIDOMEvent and not using DetachedFromEditorOrDefaultPrevented() for now. I think that we should try to remove the odd case handling later since odd focus event shouldn't cause changing selection, e.g., shouldn't cause setting selection limit to active editing host without focus.

> Why is it ok to remove kungFuDeathGrip in EditorEventListener::HandleEvent?

Sorry, I accidentally removed it from part.3, I'll move the removing code to part.4.

(In reply to Olli Pettay [:smaug] (review request backlog because of a work week) from comment #17)
> Comment on attachment 8826585 [details] [diff] [review]
> part.5 WidgetEvent should mark event as consumed if it's not cancelable
> 
> I wonder if we could assert in WidgetEvent level when one is trying to call
> its PreventDefault*() methods on non-cancelable event. But I guess this is
> fine too. Hopefully nothing relies on the current behavior internally :/
> 
> Event::PreventDefaultInternal already has 
> if (!mEvent->mFlags.mCancelable) {
> so JS calls wouldn't end up in WidgetEvent level to trigger possible
> assertion.

I wonder, we may have a bug that Event.defaultPrevented may return true from non-cancelable event after storing events into variables. So, part.5 should completely fix this issue.
Er, using WidgetEvent* for DetachedFromEditorOrDefaultPrevented makes it simpler. Although, some methods still need to take nsIDOM*Event.
Comment on attachment 8827802 [details] [diff] [review]
part.3 EditorEventListener should check if it's removed during a call of editor's method

Oops, HandleEvent() already has |internalEvent|.
Attachment #8827802 - Flags: review?(bugs) → review-
Comment on attachment 8827810 [details] [diff] [review]
part.3 EditorEventListener should check if it's removed during a call of editor's method

>+bool
>+EditorEventListener::DetachedFromEditorOrDefaultPrevented(
>+                       WidgetEvent* aWidgetEvent) const
>+{
>+  if (NS_WARN_IF(!aWidgetEvent) || !mEditorBase) {
>+    return false;
>+  }
>+  if (aWidgetEvent->DefaultPrevented()) {
>+    return false;
This should return true. And should return true also in the first return. Otherwise the method name doesn't make sense.
Do we not have any test to catch this?

>-  bool defaultPrevented;
>-  aKeyEvent->AsEvent()->GetDefaultPrevented(&defaultPrevented);
>-  if (defaultPrevented) {
>+  if (!mEditorBase->IsAcceptableInputEvent(keypressEvent) ||
>+      !DetachedFromEditorOrDefaultPrevented(keypressEvent)) {
So this has ! before DetachedFromEditorOrDefaultPrevented because of the bug in 
DetachedFromEditorOrDefaultPrevented's implementation

>     return NS_OK;
>   }
> 
>   nsresult rv = mEditorBase->HandleKeyPressEvent(aKeyEvent);
>   NS_ENSURE_SUCCESS(rv, rv);
>-
>-  aKeyEvent->AsEvent()->GetDefaultPrevented(&defaultPrevented);
>-  if (defaultPrevented) {
>+  if (!DetachedFromEditorOrDefaultPrevented(keypressEvent)) {
And here and elsewhere
Attachment #8827810 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] (review request backlog because of a work week) from comment #30)
> Comment on attachment 8827810 [details] [diff] [review]
> part.3 EditorEventListener should check if it's removed during a call of
> editor's method
> 
> >+bool
> >+EditorEventListener::DetachedFromEditorOrDefaultPrevented(
> >+                       WidgetEvent* aWidgetEvent) const
> >+{
> >+  if (NS_WARN_IF(!aWidgetEvent) || !mEditorBase) {
> >+    return false;
> >+  }
> >+  if (aWidgetEvent->DefaultPrevented()) {
> >+    return false;
> This should return true. And should return true also in the first return.
> Otherwise the method name doesn't make sense.
> Do we not have any test to catch this?

Oh, sorry, I didn't realize that the meaning is reversed.

Fortunately, my mistake (not changed HTMLEditorEventListener) was detected by mochitests (comment 31).
Reversed the result of DetachedFromEditorOrDefaultPrevented() and updated the all callers.
Attachment #8827810 - Attachment is obsolete: true
Attachment #8828346 - Flags: review?(bugs)
Comment on attachment 8828346 [details] [diff] [review]
part.3 EditorEventListener should check if it's removed during a call of editor's method


> EditorEventListener::MouseClick(nsIDOMMouseEvent* aMouseEvent)
> {
>+  if (NS_WARN_IF(!aMouseEvent)) {
>+    return NS_OK;
>+  }
>   // nothing to do if editor isn't editable or clicked on out of the editor.
>   WidgetMouseEvent* clickEvent =
>     aMouseEvent->AsEvent()->WidgetEventPtr()->AsMouseEvent();
>-  if (mEditorBase->IsReadonly() || mEditorBase->IsDisabled() ||
>+  if (DetachedFromEditorOrDefaultPrevented(clickEvent) ||
>+      mEditorBase->IsReadonly() || mEditorBase->IsDisabled() ||
So this changes the behavior. The old code doesn't check defaultPrevented. Why is this change ok?
Could you please make that kind of functional changes in separate bug. That would make regression tracking simpler.
So, just check !mEditorBase here, not DetachedFromEditorOrDefaultPrevented

...
>   // Notifies clicking on editor to IMEStateManager even when the event was
>   // consumed.

especially because we have this comment.

>   if (EditorHasFocus()) {
>     nsPresContext* presContext = GetPresContext();
>     if (presContext) {
>       IMEStateManager::OnClickInEditor(presContext, GetFocusedRootContent(),
>                                        aMouseEvent);
>-     }
>+      if (DetachedFromEditorOrDefaultPrevented(clickEvent)) {
>+        return NS_OK;
>+      }
>+    }
>   }
> 
>-  bool preventDefault;
>-  nsresult rv = aMouseEvent->AsEvent()->GetDefaultPrevented(&preventDefault);
>-  if (NS_FAILED(rv) || preventDefault) {
>-    // We're done if 'preventdefault' is true (see for example bug 70698).
>-    return rv;
>-  }
You change the behavior here too. Why is DetachedFromEditorOrDefaultPrevented() call inside if (EditorHasFocus()) { ?
Please move it to the place where if (NS_FAILED(rv) || preventDefault) { is currently


> EditorEventListener::NotifyIMEOfMouseButtonEvent(
>                        nsIDOMMouseEvent* aMouseEvent)
> {
>+  MOZ_ASSERT(aMouseEvent);
>+
>   if (!EditorHasFocus()) {
>     return false;
>   }
> 
>-  bool defaultPrevented;
>-  nsresult rv = aMouseEvent->AsEvent()->GetDefaultPrevented(&defaultPrevented);
>-  NS_ENSURE_SUCCESS(rv, false);
>-  if (defaultPrevented) {
>-    return false;
>-  }
ok, this can be removed because OnMouseButtonEventInEditor ends up checking defaultPrevented


>-EditorEventListener::HandleText(nsIDOMEvent* aTextEvent)
>+EditorEventListener::HandleChangeComposition(
>+                       WidgetCompositionEvent* aCompositionChangeEvent)
> {
>-  WidgetCompositionEvent* compositionChangeEvent =
>-    aTextEvent->WidgetEventPtr()->AsCompositionEvent();
>-  if (!mEditorBase->IsAcceptableInputEvent(compositionChangeEvent)) {
>+  if (DetachedFromEditorOrDefaultPrevented(aCompositionChangeEvent) ||
Again change to the behavior. Please move to some other bug, and possibly just add
!mEditorBase ||
if needed


> EditorEventListener::DragEnter(nsIDOMDragEvent* aDragEvent)
> {
>-  NS_ENSURE_TRUE(aDragEvent, NS_OK);
>+  if (NS_WARN_IF(!aDragEvent) ||
>+      DetachedFromEditorOrDefaultPrevented(
>+        aDragEvent->AsEvent()->WidgetEventPtr())) {
>+    return NS_OK;
>+  }
also  here


> EditorEventListener::DragExit(nsIDOMDragEvent* aDragEvent)
> {
>-  NS_ENSURE_TRUE(aDragEvent, NS_OK);
>+  if (NS_WARN_IF(!aDragEvent) ||
>+      DetachedFromEditorOrDefaultPrevented(
>+        aDragEvent->AsEvent()->WidgetEventPtr())) {
>+    return NS_OK;
>+  }
and here

>+EditorEventListener::HandleStartComposition(
>+                       WidgetCompositionEvent* aCompositionStartEvent)
> {
>-  WidgetCompositionEvent* compositionStart =
>-    aCompositionEvent->WidgetEventPtr()->AsCompositionEvent();
>-  if (!mEditorBase->IsAcceptableInputEvent(compositionStart)) {
>+  if (DetachedFromEditorOrDefaultPrevented(aCompositionStartEvent) ||
again, changing behavior


>+EditorEventListener::HandleEndComposition(
>+                       WidgetCompositionEvent* aCompositionEndEvent)
> {
>-  WidgetCompositionEvent* compositionEnd =
>-    aCompositionEvent->WidgetEventPtr()->AsCompositionEvent();
>-  if (!mEditorBase->IsAcceptableInputEvent(compositionEnd)) {
>+  if (DetachedFromEditorOrDefaultPrevented(aCompositionEndEvent) ||
and here

>+EditorEventListener::Focus(WidgetEvent* aFocusEvent)
> {
>-  NS_ENSURE_TRUE(aEvent, NS_OK);
>+  if (DetachedFromEditorOrDefaultPrevented(aFocusEvent)) {
>+    return NS_OK;
>+  }
focus event shouldn't be cancelable, so this should be fine I guess
> HTMLEditorEventListener::MouseDown(nsIDOMMouseEvent* aMouseEvent)
> {
>   WidgetMouseEvent* mousedownEvent =
>     aMouseEvent->AsEvent()->WidgetEventPtr()->AsMouseEvent();
> 
>   HTMLEditor* htmlEditor = GetHTMLEditor();
>   // Contenteditable should disregard mousedowns outside it.
>   // IsAcceptableInputEvent() checks it for a mouse event.
>-  if (!htmlEditor->IsAcceptableInputEvent(mousedownEvent)) {
>+  if (DetachedFromEditorOrDefaultPrevented(mousedownEvent) ||
>+      !htmlEditor->IsAcceptableInputEvent(mousedownEvent)) {
Again, changing possibly the behavior

>         if (isContextClick && !HTMLEditUtils::IsImage(node)) {
>           selection->Collapse(parent, offset);
>         } else {
>           htmlEditor->SelectElement(element);
>         }
>+        if (DetachedFromEditorOrDefaultPrevented(mousedownEvent)) {
>+          return NS_OK;
>+        }
possibly changing behavior.


Sorry, I wasn't thinking these behavior changes before, but I really think such changes shouldn't be done in this
security bug.
Attachment #8828346 - Flags: review?(bugs) → review-
And those behavior changes would need tests
Has there actually been any cases here which might fix some possible security issue.
Just thinking whether we need to port the fixes to branches and is even the security rating right.
I don't have any actual scenario to attack. Basically, HandleEvent() grabs mEditorBase. So, actual issue is, there are no null-check of mEditorBase in some places. If users hit that actually, there must be crash reports.

Looks like there are some bugs which will be fixed by the patches:
https://crash-stats.mozilla.com/signature/?product=Firefox&signature=mozilla%3A%3AEditorEventListener%3A%3AMouseClick&date=%3E%3D2017-01-01T05%3A44%3A00.000Z&date=%3C2017-01-24T05%3A44%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1
This adds new method DetachedFromEditor() to check if mEditorBase is nullptr (for making the meaning clearer, I like wrap it with the method).

And then, I checked all of this patch again if I change behavior accidentally.

Note that the nullptr check fixes the crash bugs mentioned in the previous comment. So, if we won't uplift them but we should fix the crash bugs in branches, I'll create simple patches for them separately.
Attachment #8828346 - Attachment is obsolete: true
Attachment #8830655 - Flags: review?(bugs)
FYI: I believe that the intention of the reporter is part.4, but I still don't find any security risk scenarios with that. Only mouse event handlers and focus event handlers call multiple methods of mEditorBase, but mEditorBase is never released during calls of them (could become nullptr, though, but it causes safe crash).
Comment on attachment 8830655 [details] [diff] [review]
part.3 EditorEventListener should check if it's removed during a call of editor's method

Hmm, I wonder if use of Get*DOMEventTarget() is actually a bit error prone.
Though, I guess not worse than the current widgetevent->mTarget usage.
Attachment #8830655 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #44)
> Hmm, I wonder if use of Get*DOMEventTarget() is actually a bit error prone.
> Though, I guess not worse than the current widgetevent->mTarget usage.

Should we get rid of them in follow up bug? But they restrict the targets to proper kind nodes. So, somebody may need them.

Or do you worry about their names?
Flags: needinfo?(bugs)
Comment on attachment 8827798 [details] [diff] [review]
part.1 Change EditorBase::UpdateIMEComposition()'s argument from nsIDOMEvent* to WidgetCompositionEvent* (r=smaug)

[Security approval request comment]
This is preparation for following patches to avoid to create a lot of overload methods.
Attachment #8827798 - Flags: sec-approval?
Comment on attachment 8827799 [details] [diff] [review]
part.2 Change EditorBase::IsAcceptableInputEvent()'s argument from nsIDOMEvent* to WidgetGUIEvent* (r=smaug)

[Security approval request comment]
This is preparation for following patches to avoid to create a lot of overload methods.
Attachment #8827799 - Flags: sec-approval?
Comment on attachment 8830655 [details] [diff] [review]
part.3 EditorEventListener should check if it's removed during a call of editor's method

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

This fixes nullptr reference of mEditorBase. mEditorBase may have already been detached from the event listener when an event is fired or handling an event. So, if somebody digs up every editor methods called by each event handler, it might be found. But perhaps, it's not so easy.

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

Yes, it explain that this patch adds nullptr checks.

Which older supported branches are affected by this flaw?

I think that this fixes ancient crash bugs (bug 1315450, bug 1115055, their bug numbers are not so small, though). 

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

With using |hg import|, I needed to merge pert.2 and part.4 on Beta branch, but perhaps, they can be merged with |hg graft|.

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

This and other patches touch all event handlers of editor (<input>, <textarea>, <foo contenteditable> and designMode), so, some of them must be tested by mochitests, but perhaps, not all.

Probably, if there are some regressions, they'll be reported soon because testers cannot do something in editor.
Attachment #8830655 - Flags: sec-approval?
Comment on attachment 8830656 [details] [diff] [review]
part.4 EditorEventListener should grab mEditorBase in smaller scope as far as possible and shouldn't access it directly during handling an event (r=smaug)

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

I think that this is what the reporter wanted to fix in this bug. If a event handler calls 2 or more editor methods and one of them causes detaching the editor from the event listener, then, mEditorBase becomes nullptr. Then, crashes at calling a method of mEditorBase. So, it may be possible to find how to make it crash, but I don't think it can be used for attack.

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

Yes, the commit message explains what this patch does.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

See the previous request.

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

I think that this is not risky because this patch just replaces from mEditorBase to editorBase which is local variable for grabbing mEditorBase's instance before calling a method of it.
Attachment #8830656 - Flags: sec-approval?
Comment on attachment 8827804 [details] [diff] [review]
part.5 WidgetEvent should mark event as consumed if it's not cancelable (r=smaug)

[Security approval request comment]
This is additional fix for new oranges which are caused by new |MOZ_ASSERT(!a*Event->DefaultPrevented(), ...)| in EditorEventListener (added by part.3). This is fixing existing event handling bug, not directly related to this bug.
Attachment #8827804 - Flags: sec-approval?
My idea was posted to comment 43. I think that we don't need to uplift them due to risk management if there are no scenario to attack with using nullptr reference which are fixed by part.3 and part.4.
(In reply to Masayuki Nakano [:masayuki] from comment #45)
> (In reply to Olli Pettay [:smaug] from comment #44)
> > Hmm, I wonder if use of Get*DOMEventTarget() is actually a bit error prone.
> > Though, I guess not worse than the current widgetevent->mTarget usage.
> 
> Should we get rid of them in follow up bug? But they restrict the targets to
> proper kind nodes. So, somebody may need them.
> 
> Or do you worry about their names?
It is just the generic problem that when to use ->mTarget and when GetDOMEventTarget().
Not really about this bug.



And if we haven't seen anything sec-high/critical related in this bug, this doesn't need to be a security bug. 
Possible null pointer error could be fixed in a separate bug, so that tracking its landing in branches is easy.
Flags: needinfo?(bugs)
The vast majority of crashes in nsEditorEventListener are null derefs, which wouldn't be a security problem. If you remove those there are a small number of concerning crashes (60 in the last 3 months). None of those carry the telltale UAF poison value, but of course they might be from a UAF nonetheless. Some are "exec" access violations which are potentially serious. Of course those could be different from the crashes this set of fixes are expected to fix.

https://crash-stats.mozilla.com/search/?signature=~mozilla%3A%3AEditorEventListener%3A%3A&address=%21%400x%5B0248%5D&product=Firefox&date=%3E%3D2016-10-27T23%3A33%3A39.000Z&date=%3C2017-01-27T23%3A33%3A39.000Z&_sort=version&_sort=-address&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=address&_columns=reason#crash-reports

None of those crashes are before Firefox 50. Firefox 49 was Release the first bit of that period so that's probably significant and not just a random outcome of it being a low-volume event.

Calling this sec-audit, and go ahead and land on nightly. We should spawn a separate investigation (or more) into the crashes I linked to above.
Blocks: 1018486
No longer depends on: 1018486
Attachment #8827798 - Flags: sec-approval? → sec-approval+
Attachment #8827799 - Flags: sec-approval? → sec-approval+
Attachment #8827804 - Flags: sec-approval? → sec-approval+
Attachment #8830655 - Flags: sec-approval? → sec-approval+
Attachment #8830656 - Flags: sec-approval? → sec-approval+
(In reply to Olli Pettay [:smaug] from comment #52)
> (In reply to Masayuki Nakano [:masayuki] from comment #45)
> > (In reply to Olli Pettay [:smaug] from comment #44)
> > > Hmm, I wonder if use of Get*DOMEventTarget() is actually a bit error prone.
> > > Though, I guess not worse than the current widgetevent->mTarget usage.
> > 
> > Should we get rid of them in follow up bug? But they restrict the targets to
> > proper kind nodes. So, somebody may need them.
> > 
> > Or do you worry about their names?
> It is just the generic problem that when to use ->mTarget and when
> GetDOMEventTarget().
> Not really about this bug.

Okay, thanks.

> And if we haven't seen anything sec-high/critical related in this bug, this
> doesn't need to be a security bug. 
> Possible null pointer error could be fixed in a separate bug, so that
> tracking its landing in branches is easy.

Indeed, before landing them, I'll fix bug 1315450 and bug 1115055 in 52+.
(In reply to Daniel Veditz [:dveditz] from comment #53)
> Some are "exec" access violations which are potentially
> serious. Of course those could be different from the crashes this set of
> fixes are expected to fix.
> 
> https://crash-stats.mozilla.com/search/
> ?signature=~mozilla%3A%3AEditorEventListener%3A%3A&address=%21%400x%5B0248%5D
> &product=Firefox&date=%3E%3D2016-10-27T23%3A33%3A39.000Z&date=%3C2017-01-
> 27T23%3A33%3A39.000Z&_sort=version&_sort=-address&_sort=-
> date&_facets=signature&_columns=date&_columns=signature&_columns=product&_col
> umns=version&_columns=build_id&_columns=platform&_columns=address&_columns=re
> ason#crash-reports

Thanks, as far as I checked above, some of them needs some fix of callers. So, not fixed by the patches for this.

> None of those crashes are before Firefox 50. Firefox 49 was Release the
> first bit of that period so that's probably significant and not just a
> random outcome of it being a low-volume event.

The reason is, the class names under editor/ were renamed from ns* to mozilla::* at 50. So, you can see similar crash reports when you search with nsEditorEventListener.

> Calling this sec-audit, and go ahead and land on nightly. We should spawn a
> separate investigation (or more) into the crashes I linked to above.

Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/b61ab2c333b9b85a883b8d8957ed0c83fca3ec69
Bug 1314053 part.1 Change EditorBase::UpdateIMEComposition()'s argument from nsIDOMEvent* to WidgetCompositionEvent* r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/17760a0340eb92dce57d17adf77104e09525dab5
Bug 1314053 part.2 Change EditorBase::IsAcceptableInputEvent()'s argument from nsIDOMEvent* to WidgetGUIEvent* r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c29136204f9cae9c0b0b99082fb813a9cd51b5b
Bug 1314053 part.3 EditorEventListener should check if it's removed during a call of editor's method r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/cce7f1f580d5a73beebd500845e000987a28a4d9
Bug 1314053 part.4 EditorEventListener should grab mEditorBase in smaller scope as far as possible and shouldn't access it directly during handling an event r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/75d08dfb899fb74018ac3e925f0bae04d0704fda
Bug 1314053 part.5 WidgetEvent shouldn't mark event as consumed if it's not cancelable r=smaug
Group: dom-core-security → core-security-release
Do we intend to backport this or let it ride the trains?
Flags: needinfo?(masayuki)
I think that they should ride the train because the patches can fix only nullptr reference bugs but they are fixed and uplifted by separated bugs. So, we shouldn't take risk of uplift for them.
Flags: needinfo?(masayuki)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.