1380824 - Assertion failure: uint32_t(startOffset) <= startParent->Length() && uint32_t(endOffset) <= endParent->Length() [@nsContentSubtreeIterator::Init]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Assertion failure: uint32_t(startOffset) <= startParent->Length() && uint32_t(endOffset) <= endParent->Length(), at /home/worker/workspace/build/src/dom/base/nsContentIterator.cpp:1293

#0 0x7f42e789cada in nsContentSubtreeIterator::Init(nsIDOMRange*) src/dom/base/nsContentIterator.cpp:1292:3
#1 0x7f42e781d573 in mozilla::dom::Selection::SelectFrames(nsPresContext*, nsRange*, bool) src/dom/base/Selection.cpp:1847:9
#2 0x7f42e781ff46 in mozilla::dom::Selection::Repaint(nsPresContext*) src/dom/base/Selection.cpp:2008:19
#3 0x7f42eadfd922 in mozilla::PresShell::RepaintSelection(short) src/layout/base/PresShell.cpp:1671:26
#4 0x7f42ea9d7579 in mozilla::EditorBase::FinalizeSelection() src/editor/libeditor/EditorBase.cpp:4977:24
#5 0x7f42e796c5c0 in nsFocusManager::ContentRemoved(nsIDocument*, nsIContent*) src/dom/base/nsFocusManager.cpp:861:23
#6 0x7f42e9372687 in mozilla::EventStateManager::ContentRemoved(nsIDocument*, nsIContent*) src/dom/events/EventStateManager.cpp:5024:9
#7 0x7f42eae11894 in mozilla::PresShell::ContentRemoved(nsIDocument*, nsIContent*, nsIContent*, int, nsIContent*) src/layout/base/PresShell.cpp:4485:38
#8 0x7f42e7a1bb20 in nsNodeUtils::ContentRemoved(nsINode*, nsIContent*, int, nsIContent*) src/dom/base/nsNodeUtils.cpp:226:3
#9 0x7f42e79d7895 in nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&) src/dom/base/nsINode.cpp:1934:5
#10 0x7f42e77af8c5 in mozilla::dom::FragmentOrElement::RemoveChildAt(unsigned int, bool) src/dom/base/FragmentOrElement.cpp:1242:5
#11 0x7f42e79d8c87 in nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) src/dom/base/nsINode.cpp:2424:5
#12 0x7f42e7ecaca2 in mozilla::dom::NodeBinding::replaceChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/NodeBinding.cpp:945:45
#13 0x7f42e90aed5e in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3060:13
#14 0x7f42edfb6dc1 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:293:15
#15 0x7f42edfb68dd in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:470:16
#16 0x7f42edfb7805 in InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:515:12
#17 0x7f42edfac4da in Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3060:18
#18 0x7f42edf97ca8 in js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:410:12
#19 0x7f42edfb6860 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:488:15
#20 0x7f42edfb7805 in InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:515:12
#21 0x7f42edfb7a1c in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:534:10
#22 0x7f42ee7e022b in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2948:12
#23 0x7f42e8ba2455 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:260:37
#24 0x7f42e93ece2b in void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:362:12
#25 0x7f42e93eb65b in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) src/dom/events/JSEventHandler.cpp:215:12
#26 0x7f42e93c717f in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1141:51
#27 0x7f42e93c847d in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) src/dom/events/EventListenerManager.cpp:1311:20
#28 0x7f42e93b39eb in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:315:17
#29 0x7f42e93b30af in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:464:16
#30 0x7f42e93b4b8d in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:824:9
#31 0x7f42eaeafaf9 in nsDocumentViewer::LoadComplete(nsresult) src/layout/base/nsDocumentViewer.cpp:1104:7
#32 0x7f42ed3b6b9d in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) src/docshell/base/nsDocShell.cpp:7698:21
#33 0x7f42ed3b429a in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:7496:7
#34 0x7f42ed3b82ef in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:7393:13
#35 0x7f42e6ae4abf in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) src/uriloader/base/nsDocLoader.cpp:1299:3
#36 0x7f42e6ae4119 in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:860:14
#37 0x7f42e6ae1bbd in nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:749:9
#38 0x7f42e6ae3329 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp:631:5
#39 0x7f42e6ae3ccc in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp:487:14
#40 0x7f42e5713f77 in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) src/netwerk/base/nsLoadGroup.cpp:629:28
#41 0x7f42e793bc27 in nsDocument::DoUnblockOnload() src/dom/base/nsDocument.cpp:8927:18
#42 0x7f42e793b98b in nsDocument::UnblockOnload(bool) src/dom/base/nsDocument.cpp:8849:9
#43 0x7f42e7921336 in nsDocument::DispatchContentLoadedEvents() src/dom/base/nsDocument.cpp:5369:3
#44 0x7f42e7998515 in mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0>::Run() src/obj-firefox/dist/include/nsThreadUtils.h:1187:13
#45 0x7f42e557b5c2 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1437:14
#46 0x7f42e5581170 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:489:10
#47 0x7f42e60e5e25 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
#48 0x7f42e6032247 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:320:10
#49 0x7f42e60320d9 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:293:3
#50 0x7f42ea8c165a in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:156:27
#51 0x7f42eda464d1 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:287:30
#52 0x7f42edba20c2 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4595:22
#53 0x7f42edba3d1f in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4778:8
#54 0x7f42edba4c08 in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4873:21
#55 0x4ecaf8 in do_main(int, char**, char**) src/browser/app/nsBrowserApp.cpp:237:22
#56 0x4ec410 in main src/browser/app/nsBrowserApp.cpp:310:16
#57 0x7f430340082f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
#58 0x41e144 in _start (m-c-1499788810-asan-debug/firefox+0x41e144)

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Goes back further than mozregression can bisect.

Comment 2

[Andrew Overholt [:overholt]]

Jet, does anyone on your team have time to take a look? Tentatively marking as a sec bug just in case.

Comment 3

[Andrew McCreight [:mccr8]]

Sounds like a bounds check assertion.

Comment 4

[Brian Birtles (:birtles)]

Kato-san, do you mind having a look at this?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, looks like that mutation observer uses nsContentIterator. That won't work as expected because ContentRemoved() is called when the node is removed from its parent but the node still have its parent. So, this does wrong thing...:
https://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/dom/base/nsFocusManager.cpp#809,861

Comment 6

[Makoto Kato [:m_kato]]

Humm, we might have to check whether start and end of range are into document...

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Makoto Kato [:m_kato] from comment #6)
> Humm, we might have to check whether start and end of range are into
> document...

I don't know the order of mutation observers, it should be always in since when modifying DOM tree, nsRange adjusts its range from mutation observer. The point of this is, removing node parent's nsINode::IndexOf() returns -1, but the node's nsINode::GetParent() returns old parent instead of nullptr.

Comment 8

[Makoto Kato [:m_kato]]

Attachment: Call RepaintSelection out of script blocker.

FinalizeSelection might be called from ContentRemoved even if selection isn't updated.  So we need to call RepaintSelection after updated it.

Comment 9

[Makoto Kato [:m_kato]]

(this is assertion, not crash)

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8896838 [details] [diff] [review]
Call RepaintSelection out of script blocker.

>+class RepaintSelectionRunner final : public Runnable {
>+public:
>+  explicit RepaintSelectionRunner(nsISelectionController* aSelectionController)
>+    : mozilla::Runnable("RepaintSelectionRunner")

Is this |mozilla::| really necessary?

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8896838 [details] [diff] [review]
Call RepaintSelection out of script blocker.

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

Too difficult and this might not be security bug.

When using content iterator, this checks whether range is valid.  If it is invalid, it hits assertion on debug build.  After assertion check (on release build, no assertion), offset will change to end offset of node instead.  So since content iterator doesn't return valid node, although frame selection for painting won't work well, it doesn't access invalid index.  (If content iterator has another bug that that methods doesn't check error, developer might create exploit, but this cannot find it)

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

Don't call SelectionController->RepaintSelection during ContentRemoved.  Because selection data might not be updated.

Which older supported branches are affected by this flaw?

At least, this occurs on esr52.

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

I cannot find it, but this is from old.

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

Yes, it is easy.

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

If there is a regression, selection isn't repainted.  But I don't think that this occurs.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8896838 [details] [diff] [review]
Call RepaintSelection out of script blocker.

sec-approval+ for trunk. We'll want branch patches made and nominated.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6a65de867d00

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you request uplift to beta? Thanks.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

And for esr, also.

Comment 16

[Makoto Kato [:m_kato]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14)
> Can you request uplift to beta? Thanks.

This is assertion issue, not crash.  This bug marks as security issue by comment #3, we don't access out of index.  (See Comment #11 for details)

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Sorry, I don't understand your comment. 
Do you mean this doesn't apply to beta? Comment 11 and comment 12 both sound like uplift would be a good idea.

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8896838 [details] [diff] [review]
Call RepaintSelection out of script blocker.

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

[User impact if declined]:
On debug build, it hits assertion by specific HTML content

[Is this code covered by automated tests?]:
No, since this bug is marked as security issue

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes.  No crash/assertion with attached test_case.html

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

[Is the change risky?]:
Low

[Why is the change risky/not risky?]:
Repaint selection after running script instead of repaint immediately.

[String changes made/needed]:
No

Comment 19

[Makoto Kato [:m_kato]]

(for ESR52, this need rebase this)

Comment 20

[Makoto Kato [:m_kato]]

Attachment: For ESR52

Comment 21

[Makoto Kato [:m_kato]]

Comment on attachment 8901661 [details] [diff] [review]
For ESR52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined:
On debug build, it hits assertion by specific HTML content
 
Fix Landed on Version:
57

Risk to taking this patch (and alternatives if risky): 
Low.  Repaint selection after running script instead of repaint immediately.

String or UUID changes made by this patch: 
No

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8896838 [details] [diff] [review]
Call RepaintSelection out of script blocker.

Fix a sec-high. Beta56+ & ESR52+.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6562cd359748

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/fbddb5cdd3c7