Closed
Bug 1314053
Opened 8 years ago
Closed 7 years ago
Investigate editor's KungFuDeathGrips for potential security concerns
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: nika, Assigned: masayuki)
References
Details
(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main54-])
Attachments
(5 files, 9 obsolete files)
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.
Assignee | ||
Comment 1•8 years ago
|
||
>> 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.
Updated•8 years ago
|
Group: core-security → dom-core-security
Keywords: csectype-uaf,
sec-high
Comment 2•7 years ago
|
||
Masayuki, is this something you will be able to work on? Thanks.
Flags: needinfo?(masayuki)
Reporter | ||
Comment 4•7 years ago
|
||
(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)
Assignee | ||
Comment 5•7 years ago
|
||
Okay.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5285de9efe3
Assignee | ||
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8b0f262f1b4
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1c74cbb989d
Assignee | ||
Comment 9•7 years ago
|
||
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)
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8826582 -
Flags: review?(bugs)
Assignee | ||
Comment 11•7 years ago
|
||
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)
Assignee | ||
Comment 12•7 years ago
|
||
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)
Assignee | ||
Comment 13•7 years ago
|
||
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 14•7 years ago
|
||
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 15•7 years ago
|
||
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 16•7 years ago
|
||
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 17•7 years ago
|
||
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 18•7 years ago
|
||
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+
Assignee | ||
Comment 19•7 years ago
|
||
(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.
Assignee | ||
Comment 20•7 years ago
|
||
Er, using WidgetEvent* for DetachedFromEditorOrDefaultPrevented makes it simpler. Although, some methods still need to take nsIDOM*Event.
Assignee | ||
Comment 21•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=465c64dc7480
Assignee | ||
Comment 22•7 years ago
|
||
Attachment #8826581 -
Attachment is obsolete: true
Attachment #8827798 -
Flags: review+
Assignee | ||
Comment 23•7 years ago
|
||
Attachment #8826582 -
Attachment is obsolete: true
Attachment #8827799 -
Flags: review+
Assignee | ||
Comment 24•7 years ago
|
||
Attachment #8826583 -
Attachment is obsolete: true
Attachment #8827802 -
Flags: review?(bugs)
Assignee | ||
Comment 25•7 years ago
|
||
Attachment #8826584 -
Attachment is obsolete: true
Attachment #8827803 -
Flags: review+
Assignee | ||
Comment 26•7 years ago
|
||
Attachment #8826585 -
Attachment is obsolete: true
Attachment #8827804 -
Flags: review+
Assignee | ||
Comment 27•7 years ago
|
||
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-
Assignee | ||
Comment 28•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f167bccd6161
Assignee | ||
Comment 29•7 years ago
|
||
Attachment #8827802 -
Attachment is obsolete: true
Attachment #8827810 -
Flags: review?(bugs)
Comment 30•7 years ago
|
||
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-
Assignee | ||
Comment 31•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=819b53d84eb9
Assignee | ||
Comment 32•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=860ec36af238
Assignee | ||
Comment 33•7 years ago
|
||
(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).
Assignee | ||
Comment 34•7 years ago
|
||
Reversed the result of DetachedFromEditorOrDefaultPrevented() and updated the all callers.
Attachment #8827810 -
Attachment is obsolete: true
Attachment #8828346 -
Flags: review?(bugs)
Comment 35•7 years ago
|
||
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-
Comment 36•7 years ago
|
||
And those behavior changes would need tests
Assignee | ||
Comment 37•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2feeb9f093d6
Assignee | ||
Comment 38•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32d8018ce7b6
Comment 39•7 years ago
|
||
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.
Assignee | ||
Comment 40•7 years ago
|
||
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
Assignee | ||
Comment 41•7 years ago
|
||
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)
Assignee | ||
Comment 42•7 years ago
|
||
Attachment #8827803 -
Attachment is obsolete: true
Assignee | ||
Comment 43•7 years ago
|
||
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 44•7 years ago
|
||
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+
Assignee | ||
Comment 45•7 years ago
|
||
(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)
Assignee | ||
Comment 46•7 years ago
|
||
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?
Assignee | ||
Comment 47•7 years ago
|
||
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?
Assignee | ||
Comment 48•7 years ago
|
||
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?
Assignee | ||
Comment 49•7 years ago
|
||
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?
Assignee | ||
Comment 50•7 years ago
|
||
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?
Assignee | ||
Comment 51•7 years ago
|
||
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.
Comment 52•7 years ago
|
||
(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)
Comment 53•7 years ago
|
||
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.
Updated•7 years ago
|
Attachment #8827798 -
Flags: sec-approval? → sec-approval+
Updated•7 years ago
|
Attachment #8827799 -
Flags: sec-approval? → sec-approval+
Updated•7 years ago
|
Attachment #8827804 -
Flags: sec-approval? → sec-approval+
Updated•7 years ago
|
Attachment #8830655 -
Flags: sec-approval? → sec-approval+
Updated•7 years ago
|
Attachment #8830656 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 54•7 years ago
|
||
(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+.
Assignee | ||
Comment 55•7 years ago
|
||
(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!
Assignee | ||
Comment 56•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6bc746dd5c0
Assignee | ||
Comment 57•7 years ago
|
||
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
https://hg.mozilla.org/mozilla-central/rev/b61ab2c333b9 https://hg.mozilla.org/mozilla-central/rev/17760a0340eb https://hg.mozilla.org/mozilla-central/rev/4c29136204f9 https://hg.mozilla.org/mozilla-central/rev/cce7f1f580d5 https://hg.mozilla.org/mozilla-central/rev/75d08dfb899f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•7 years ago
|
Group: dom-core-security → core-security-release
Comment 59•7 years ago
|
||
Do we intend to backport this or let it ride the trains?
Assignee | ||
Comment 60•7 years ago
|
||
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)
Updated•7 years ago
|
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•7 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54-]
Updated•7 years ago
|
status-firefox-esr52:
--- → wontfix
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•