Tyson Smith [:tsmith] Reporter
	Description • 6 years ago

Test case is being reduced and will be added once complete.

==17501==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000705a00 at pc 0x7f13727d9a88 bp 0x7fff324e36a0 sp 0x7fff324e3698
READ of size 8 at 0x602000705a00 thread T0 (file:// Content)
    #0 0x7f13727d9a87 in Equals src/dom/base/nsAttrName.h:110:50
    #1 0x7f13727d9a87 in AttrArray::GetAttr(nsAtom*, int) const src/dom/base/AttrArray.cpp:42
    #2 0x7f1378aad8da in IsMozEditorBogusNode src/obj-firefox/dist/include/mozilla/dom/Element.h:2026:35
    #3 0x7f1378aad8da in mozilla::TextEditRules::CreateBogusNodeIfNeeded() src/editor/libeditor/TextEditRules.cpp:1539
    #4 0x7f1378aacf04 in mozilla::TextEditRules::Init(mozilla::TextEditor*) src/editor/libeditor/TextEditRules.cpp:155:17
    #5 0x7f1378ac2748 in mozilla::TextEditor::EndEditorInit() src/editor/libeditor/TextEditor.cpp:214:17
    #6 0x7f1378ac50c4 in ~AutoEditInitRulesTrigger src/editor/libeditor/TextEditUtils.cpp:93:28
    #7 0x7f1378ac50c4 in mozilla::TextEditor::Init(nsIDocument&, mozilla::dom::Element*, nsISelectionController*, unsigned int, nsTSubstring<char16_t> const&) src/editor/libeditor/TextEditor.cpp:148
    #8 0x7f1376824f41 in nsTextEditorState::PrepareEditor(nsTSubstring<char16_t> const*) src/dom/html/nsTextEditorState.cpp:1424:25
    #9 0x7f1379518579 in nsTextControlFrame::EnsureEditorInitialized() src/layout/forms/nsTextControlFrame.cpp:315:28
    #10 0x7f137661f8c3 in mozilla::dom::HTMLInputElement::GetEventTargetParent(mozilla::EventChainPreVisitor&) src/dom/html/HTMLInputElement.cpp:3387:25
    #11 0x7f13762d1092 in mozilla::EventTargetChainItem::GetEventTargetParent(mozilla::EventChainPreVisitor&) src/dom/events/EventDispatcher.cpp:495:12
    #12 0x7f13762d7347 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1001:15
    #13 0x7f13762dc6c6 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) src/dom/events/EventDispatcher.cpp
    #14 0x7f137260208d in nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, mozilla::WidgetEvent&, mozilla::EventMessage, mozilla::CanBubble, mozilla::Cancelable, mozilla::Trusted, bool*, mozilla::ChromeOnlyDispatch) src/dom/base/nsContentUtils.cpp:4471:17
    #15 0x7f1376252ed1 in nsresult nsContentUtils::DispatchTrustedEvent<mozilla::WidgetEvent>(nsIDocument*, nsISupports*, mozilla::EventMessage, mozilla::CanBubble, mozilla::Cancelable, bool*, mozilla::ChromeOnlyDispatch) src/dom/base/nsContentUtils.h:1381:12
    #16 0x7f1376252718 in mozilla::AsyncEventDispatcher::Run() src/dom/events/AsyncEventDispatcher.cpp:54:12
    #17 0x7f136ea73902 in mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:337:32
    #18 0x7f136eab0850 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1167:14
    #19 0x7f136eab9565 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #20 0x7f136fc9887e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #21 0x7f136fb9a4cc in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #22 0x7f136fb9a4cc in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #23 0x7f136fb9a4cc in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #24 0x7f13786ba316 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
    #25 0x7f137ca44b7e in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:944:22
    #26 0x7f136fb9a4cc in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #27 0x7f136fb9a4cc in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #28 0x7f136fb9a4cc in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #29 0x7f137ca43c35 in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:770:34
    #30 0x4f5b01 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #31 0x4f5b01 in main src/browser/app/nsBrowserApp.cpp:287
    #32 0x7f139221d82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #33 0x424edc in _start (firefox+0x424edc)

0x602000705a00 is located 10 bytes to the right of 6-byte region [0x6020007059f0,0x6020007059f6)
allocated by thread T0 (file:// Content) here:
    #0 0x4c5623 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x7f1372cfb5df in nsTextFragment::SetTo(char16_t const*, int, bool, bool) src/dom/base/nsTextFragment.cpp:315:37
    #2 0x7f13727eb3a8 in mozilla::dom::CharacterData::SetTextInternal(unsigned int, unsigned int, char16_t const*, unsigned int, bool, CharacterDataChangeInfo::Details*) src/dom/base/CharacterData.cpp:302:13
    #3 0x7f13727ef1f6 in mozilla::dom::CharacterData::SetText(char16_t const*, unsigned int, bool) src/dom/base/CharacterData.cpp:615:10
    #4 0x7f137951be12 in SetText src/obj-firefox/dist/include/mozilla/dom/CharacterData.h:156:12
    #5 0x7f137951be12 in nsTextControlFrame::UpdateValueDisplay(bool, bool, nsTSubstring<char16_t> const*) src/layout/forms/nsTextControlFrame.cpp:1312
    #6 0x7f1376824027 in nsTextEditorState::PrepareEditor(nsTSubstring<char16_t> const*) src/dom/html/nsTextEditorState.cpp:1376:23
    #7 0x7f1379518579 in nsTextControlFrame::EnsureEditorInitialized() src/layout/forms/nsTextControlFrame.cpp:315:28
    #8 0x7f137661f8c3 in mozilla::dom::HTMLInputElement::GetEventTargetParent(mozilla::EventChainPreVisitor&) src/dom/html/HTMLInputElement.cpp:3387:25
    #9 0x7f13762d1092 in mozilla::EventTargetChainItem::GetEventTargetParent(mozilla::EventChainPreVisitor&) src/dom/events/EventDispatcher.cpp:495:12
    #10 0x7f13762d7347 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1001:15
    #11 0x7f13762dc6c6 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) src/dom/events/EventDispatcher.cpp
    #12 0x7f137260208d in nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, mozilla::WidgetEvent&, mozilla::EventMessage, mozilla::CanBubble, mozilla::Cancelable, mozilla::Trusted, bool*, mozilla::ChromeOnlyDispatch) src/dom/base/nsContentUtils.cpp:4471:17
    #13 0x7f1376252ed1 in nsresult nsContentUtils::DispatchTrustedEvent<mozilla::WidgetEvent>(nsIDocument*, nsISupports*, mozilla::EventMessage, mozilla::CanBubble, mozilla::Cancelable, bool*, mozilla::ChromeOnlyDispatch) src/dom/base/nsContentUtils.h:1381:12
    #14 0x7f1376252718 in mozilla::AsyncEventDispatcher::Run() src/dom/events/AsyncEventDispatcher.cpp:54:12
    #15 0x7f136ea73902 in mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:337:32
    #16 0x7f136eab0850 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1167:14
    #17 0x7f136eab9565 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #18 0x7f136fc9887e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #19 0x7f136fb9a4cc in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #20 0x7f136fb9a4cc in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #21 0x7f136fb9a4cc in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #22 0x7f13786ba316 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27

	Makoto Kato [:m_kato] Assignee
	Comment 2 • 6 years ago

This seems to be buffer override due to wrong casting.  This might be regression by bug 1449404. Patch is coming soon.

	Makoto Kato [:m_kato] Assignee
	Comment 4 • 6 years ago

Comment on attachment 9004133 [details] [diff] [review]
Use GetAsText

This is regression by bug 1449404.  Since childContent isn't text node, casting to Text by AsText() is incorrect (childContent is an element, not text node).  Then this cast causes heap corruption.

Also, <hr> element with contenteditable allows execCommand('insertText') unfortunately.  It might have to fix this (to reject this command?) at feature

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 5 • 6 years ago

(In reply to Makoto Kato [:m_kato] from comment #4)
> Also, <hr> element with contenteditable allows execCommand('insertText')
> unfortunately.  It might have to fix this (to reject this command?) at
> feature

It sounds like a remaining bug of this security fix: https://hg.mozilla.org/mozilla-central/rev/4ac461885d8c
Perhaps, when selection is in a void element, we should move up ancestor which can insert the given content. When editing host is a void element, we should do nothing, perhaps.

	Makoto Kato [:m_kato] Assignee
	Comment 6 • 6 years ago

Comment on attachment 9004133 [details] [diff] [review]
Use GetAsText

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

Attacher imagines that setRangeText can cause any issues such as heap corruption if anonymous node into input element isn't text node. But attacher has to insert non-text node into input element's value (as anonymous node) if creating an exploit.  Because anonymous node into input element has only text node in normal situation.  So it needs another bug to create an exploit.

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

No comment in this patch

Which older supported branches are affected by this flaw?

61 or later

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

bug 1449404

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

Yes, it can backport to 61+.

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

No, this patch adds text node check.

	Al Billings [:abillings - ex-MoCo]
	Comment 8 • 6 years ago

This isn't going to get sec-approval until after 62 ships. It is too late in the cycle since we're building the release candidate and are out of betas.

We also need to figure out a security rating.

	Makoto Kato [:m_kato] Assignee
	Comment 9 • 6 years ago

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #5)

> It sounds like a remaining bug of this security fix:
> https://hg.mozilla.org/mozilla-central/rev/4ac461885d8c
> Perhaps, when selection is in a void element, we should move up ancestor
> which can insert the given content. When editing host is a void element, we
> should do nothing, perhaps.

Not related to that bug.  Although I file as bug 1487301 for this strange behaviour bug, I don't add test case in that bug due to same test case required without setRangeText...

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 6 years ago

There's all sorts of other code that expects to only find text or <br> inside text control frames.  Is there a  followup bug on fixing whatever code is violating that invariant?

	Makoto Kato [:m_kato] Assignee
	Comment 11 • 6 years ago

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #10)
> There's all sorts of other code that expects to only find text or <br>
> inside text control frames.  Is there a  followup bug on fixing whatever
> code is violating that invariant?

execCommand('insertText') and etc doesn't still allow for input element even if contenteditable.  (textarea element can allow this command with contenteditable only).  In this test case, execCommand('forwardDelete') sets selection into anonymous text node (bug 1487301 and bug 1487324).

When selection is anonymous text node, GetActiveEditingHost is null, so most case cannot insert any elements inside text control frame.  But it has a change to insert span element, so I file a new bug as bug 1487590.

	Al Billings [:abillings - ex-MoCo]
	Comment 12 • 6 years ago

This isn't going to land for 62. I'm giving sec-approval+ for landing on September 19, two weeks into the next cycle. Please don't land it on trunk before then. At that point, we'll want a Beta patch made and nominated as well.

	Makoto Kato [:m_kato] Assignee
	Comment 13 • 6 years ago

Comment on attachment 9004133 [details] [diff] [review]
Use GetAsText

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1449404

[User impact if declined]:
A exploit can cause heap corruption.

Attacher imagines that setRangeText can cause any issues such as heap corruption if anonymous node into input element isn't text node. But attacher has to insert non-text node into input element's value (as anonymous node) if creating an exploit.  Because anonymous node into input element has only text node in normal situation.  So it needs another bug to create an exploit.

[Is this code covered by automated tests?]:
I attached test but I cannot land this since this is security bug.

[Has the fix been verified in Nightly?]:
No, because this isn't landed yet.

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
Nothing.

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Before fixing this, it uses static cast.  But after fixing this, it checks current node type.

[String changes made/needed]:
No

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 14 • 6 years ago

Makoto, can you land your patch (see comment #12)? I'll approve your uplift for beta after that. Thanks.

	Makoto Kato [:m_kato] Assignee
	Comment 15 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe4edfe3dcba

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 16 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/fe4edfe3dcba

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 17 • 6 years ago

Comment on attachment 9004133 [details] [diff] [review]
Use GetAsText

Uplift approved for 63 beta 9, thanks.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 18 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/d51352e7c73c

	Makoto Kato [:m_kato] Assignee
	Comment 19 • 6 years ago

Update crash test