Closed Bug 1454139 Opened 6 years ago Closed 6 years ago

heap-buffer-overflow in [@ nsAttrAndChildArray::GetAttr]

Categories

(Core :: DOM: Editor, defect, P1)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1454126
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 - fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(5 keywords)

Attachments

(2 files)

testcase.html
495 bytes, text/html
Details
Wallpaperish patch that is consistent with surrounding code.
1.27 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Attached file testcase.html — 
==97199==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6030002a8f90 at pc 0x7f557e6c5a61 bp 0x7fffac06fa40 sp 0x7fffac06fa38
READ of size 8 at 0x6030002a8f90 thread T0
    #0 0x7f557e6c5a60 in AttrSlotIsTaken src/dom/base/nsAttrAndChildArray.h:188:12
    #1 0x7f557e6c5a60 in nsAttrAndChildArray::GetAttr(nsAtom*, int) const src/dom/base/nsAttrAndChildArray.cpp:305
    #2 0x7f558338405d in GetParsedAttr src/obj-firefox/dist/include/mozilla/dom/Element.h:1448:30
    #3 0x7f558338405d in DoMatch<const mozilla::dom::Element, (lambda at src/layout/style/ServoBindings.cpp:969:16)> src/layout/style/ServoBindings.cpp:939
    #4 0x7f558338405d in AttrEquals<const mozilla::dom::Element> src/layout/style/ServoBindings.cpp:972
    #5 0x7f558338405d in Gecko_AttrEquals src/layout/style/ServoBindings.cpp:1193
    #6 0x7f5589232535 in selectors::matching::matches_simple_selector::hb578bbd74364a96d src/servo/components/selectors/matching.rs:716
    #7 0x7f558923149a in selectors::matching::matches_compound_selector::_$u7b$$u7b$closure$u7d$$u7d$::h07fc8293dd207946 src/servo/components/selectors/matching.rs:650
    #8 0x7f558923149a in core::iter::iterator::Iterator::all::_$u7b$$u7b$closure$u7d$$u7d$::hede4686f2e304db4 /checkout/src/libcore/iter/iterator.rs:1536
    #9 0x7f558923149a in core::ops::function::impls::_$LT$impl$u20$core..ops..function..FnMut$LT$A$GT$$u20$for$u20$$RF$$u27$a$u20$mut$u20$F$GT$::call_mut::hdc46e834c66fae85 /checkout/src/libcore/ops/function.rs:261
    #10 0x7f558923149a in core::iter::iterator::Iterator::try_fold::hd6475f108dd112e4 /checkout/src/libcore/iter/iterator.rs:1412
    #11 0x7f558923149a in _$LT$core..iter..Chain$LT$A$C$$u20$B$GT$$u20$as$u20$core..iter..iterator..Iterator$GT$::try_fold::hda63c47decd85386 /checkout/src/libcore/iter/mod.rs:848
    #12 0x7f558923149a in core::iter::iterator::Iterator::all::ha97674678e460f2a /checkout/src/libcore/iter/iterator.rs:1535
    #13 0x7f558923149a in selectors::matching::matches_compound_selector::h284d0e9a9162dc01 src/servo/components/selectors/matching.rs:648
    #14 0x7f558923149a in selectors::matching::matches_complex_selector_internal::h64848bed4cad2aaf src/servo/components/selectors/matching.rs:484
    #15 0x7f558922fcda in selectors::matching::matches_complex_selector::hea5aa589be7ad3c0 src/servo/components/selectors/matching.rs:342
    #16 0x7f558922fcda in selectors::matching::matches_selector::h316fca911f3c9275 src/servo/components/selectors/matching.rs:205
    #17 0x7f558922fcda in _$LT$style..selector_map..SelectorMap$LT$style..stylist..Rule$GT$$GT$::get_matching_rules::h19a18a258a79b8a5 src/servo/components/style/selector_map.rs:243
    #18 0x7f558922f8f8 in _$LT$style..selector_map..SelectorMap$LT$style..stylist..Rule$GT$$GT$::get_all_matching_rules::h7fc207778e5099c9 src/servo/components/style/selector_map.rs:206
    #19 0x7f558921878a in style::stylist::Stylist::push_applicable_declarations::h62f925035d65ad5a src/servo/components/style/stylist.rs:1183
    #20 0x7f558921878a in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::match_primary::h25be2d4cbdb4254a src/servo/components/style/style_resolver.rs:433
    #21 0x7f55892177ad in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::resolve_primary_style::hb32ff60905482457 src/servo/components/style/style_resolver.rs:161
    #22 0x7f55892177ad in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::resolve_style::hd20d538bf9d2844d src/servo/components/style/style_resolver.rs:230
    #23 0x7f558920447f in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::resolve_style_with_default_parents::_$u7b$$u7b$closure$u7d$$u7d$::he175ba30b5e545b3 src/servo/components/style/style_resolver.rs:268
    #24 0x7f558920447f in style::style_resolver::with_default_parent_styles::h22099f5aacfaac02 src/servo/components/style/style_resolver.rs:102
    #25 0x7f558920447f in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::resolve_style_with_default_parents::h8f1661ec3e169a7c src/servo/components/style/style_resolver.rs:267
    #26 0x7f558920447f in style::traversal::compute_style::h94401bbc03c7868f src/servo/components/style/traversal.rs:636
    #27 0x7f55892017fb in style::traversal::recalc_style_at::h8b24ca294242c90e src/servo/components/style/traversal.rs:434
    #28 0x7f55892017fb in _$LT$style..gecko..traversal..RecalcStyleOnly$LT$$u27$recalc$GT$$u20$as$u20$style..traversal..DomTraversal$LT$style..gecko..wrapper..GeckoElement$LT$$u27$le$GT$$GT$$GT$::process_preorder::hfb6ed43eb561144a src/servo/components/style/gecko/traversal.rs:37
    #29 0x7f55892017fb in style::driver::traverse_dom::h220daa4c72117c2c src/servo/components/style/driver.rs:111
    #30 0x7f55892017fb in geckoservo::glue::traverse_subtree::ha3e2bdf2418cee5a src/servo/ports/geckolib/glue.rs:289
    #31 0x7f5589201017 in Servo_TraverseSubtree src/servo/ports/geckolib/glue.rs:347
    #32 0x7f55833c7587 in mozilla::ServoStyleSet::StyleDocument(mozilla::ServoTraversalFlags) src/layout/style/ServoStyleSet.cpp:1014:7
    #33 0x7f5583577393 in mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) src/layout/base/RestyleManager.cpp:2965:20
    #34 0x7f5583530c33 in ProcessPendingRestyles src/layout/base/RestyleManager.cpp:3073:3
    #35 0x7f5583530c33 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4283
    #36 0x7f557e665c24 in FlushPendingNotifications src/obj-firefox/dist/include/nsIPresShell.h:583:5
    #37 0x7f557e665c24 in mozilla::dom::Selection::ScrollIntoView(short, nsIPresShell::ScrollAxis, nsIPresShell::ScrollAxis, int) src/dom/base/Selection.cpp:3575
    #38 0x7f557e671cd3 in mozilla::dom::Selection::ScrollSelectionIntoViewEvent::Run() src/dom/base/Selection.cpp:3481:15
    #39 0x7f55834bf7d6 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1852:13
    #40 0x7f55834c0091 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1882:12
    #41 0x7f55834cae74 in nsRefreshDriver::FinishedWaitingForTransaction() src/layout/base/nsRefreshDriver.cpp:2161:5
    #42 0x7f557dd141a3 in mozilla::layers::ClientLayerManager::DidComposite(unsigned long, mozilla::TimeStamp const&, mozilla::TimeStamp const&) src/gfx/layers/client/ClientLayerManager.cpp:532:32
    #43 0x7f557de2ced3 in mozilla::layers::CompositorBridgeChild::RecvDidComposite(mozilla::layers::LayersId const&, unsigned long const&, mozilla::TimeStamp const&, mozilla::TimeStamp const&) src/gfx/layers/ipc/CompositorBridgeChild.cpp:544:8
    #44 0x7f557cd58229 in mozilla::layers::PCompositorBridgeChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PCompositorBridgeChild.cpp:1402:20
    #45 0x7f557c563bde in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) src/ipc/glue/MessageChannel.cpp:2135:25
    #46 0x7f557c560ba6 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) src/ipc/glue/MessageChannel.cpp:2065:17
    #47 0x7f557c56235c in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) src/ipc/glue/MessageChannel.cpp:1911:5
    #48 0x7f557c5629b8 in mozilla::ipc::MessageChannel::MessageTask::Run() src/ipc/glue/MessageChannel.cpp:1944:15
    #49 0x7f557b686c89 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1096:14
    #50 0x7f557b6a26c0 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #51 0x7f557c56b74a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #52 0x7f557c4c0289 in RunInternal src/ipc/chromium/src/base/message_loop.cc:326:10
    #53 0x7f557c4c0289 in RunHandler src/ipc/chromium/src/base/message_loop.cc:319
    #54 0x7f557c4c0289 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299
    #55 0x7f5582f6ed3a in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:157:27
    #56 0x7f5586ffe06b in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290:30
    #57 0x7f5587206c1c in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4834:22
    #58 0x7f5587209d5c in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4979:8
    #59 0x7f558720b224 in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:5071:21
    #60 0x4f168b in do_main src/browser/app/nsBrowserApp.cpp:231:22
    #61 0x4f168b in main src/browser/app/nsBrowserApp.cpp:304
    #62 0x7f559ada382f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #63 0x420f48 in _start (firefox+0x420f48)

0x6030002a8f92 is located 0 bytes to the right of 18-byte region [0x6030002a8f80,0x6030002a8f92)
allocated by thread T0 here:
    #0 0x4c1c93 in malloc src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x7f557b4c05a1 in nsStringBuffer::Alloc(unsigned long) src/xpcom/string/nsSubstring.cpp:256:22
    #2 0x7f557e939e4d in nsTextFragment::SetTo(char16_t const*, int, bool, bool) src/dom/base/nsTextFragment.cpp:302:11
    #3 0x7f557e5361cf in mozilla::dom::CharacterData::SetTextInternal(unsigned int, unsigned int, char16_t const*, unsigned int, bool, CharacterDataChangeInfo::Details*) src/dom/base/CharacterData.cpp:304:13
    #4 0x7f557e53926e in mozilla::dom::CharacterData::SetText(char16_t const*, unsigned int, bool) src/dom/base/CharacterData.cpp:715:10
    #5 0x7f5583a940f7 in SetText src/obj-firefox/dist/include/mozilla/dom/CharacterData.h:150:12
    #6 0x7f5583a940f7 in nsTextControlFrame::UpdateValueDisplay(bool, bool, nsTSubstring<char16_t> const*) src/layout/forms/nsTextControlFrame.cpp:1326
    #7 0x7f5581760c4d in nsTextEditorState::SetValue(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const*, unsigned int) src/dom/html/nsTextEditorState.cpp:2509:22
    #8 0x7f55815b53f2 in mozilla::dom::HTMLInputElement::SetValueInternal(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const*, unsigned int) src/dom/html/HTMLInputElement.cpp:2867:33
    #9 0x7f558175f9b6 in nsTextEditorState::SetRangeText(nsTSubstring<char16_t> const&, unsigned int, unsigned int, mozilla::dom::SelectionMode, mozilla::ErrorResult&, mozilla::Maybe<unsigned int> const&, mozilla::Maybe<unsigned int> const&) src/dom/html/nsTextEditorState.cpp:1933:35
    #10 0x7f55815d971f in mozilla::dom::HTMLInputElement::SetRangeText(nsTSubstring<char16_t> const&, unsigned int, unsigned int, mozilla::dom::SelectionMode, mozilla::ErrorResult&) src/dom/html/HTMLInputElement.cpp:5919:10
    #11 0x7f55808c1123 in mozilla::dom::HTMLInputElementBinding::setRangeText(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLInputElement*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/HTMLInputElementBinding.cpp:3627:13
    #12 0x7f5580c2b501 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3242:13
    #13 0x7f55874d8ad7 in CallJSNative src/js/src/vm/JSContext-inl.h:290:15
    #14 0x7f55874d8ad7 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:467
    #15 0x7f55874c35cb in CallFromStack src/js/src/vm/Interpreter.cpp:522:12
    #16 0x7f55874c35cb in Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3084
    #17 0x7f55874a9cc7 in js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:417:12
    #18 0x7f55874d8855 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:489:15
    #19 0x7f55874d9ad2 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:535:10
    #20 0x7f5587fc95cd in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2977:12
    #21 0x7f55803ba93e 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:264:37
    #22 0x7f5581394890 in Call<nsISupports *> src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:363:12
    #23 0x7f5581394890 in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) src/dom/events/JSEventHandler.cpp:215
    #24 0x7f558135aa3c in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1121:51
    #25 0x7f558135c21c in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) src/dom/events/EventListenerManager.cpp:1290:20
    #26 0x7f5581346687 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:527:16
    #27 0x7f558134a423 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:933:9
    #28 0x7f558360e218 in nsDocumentViewer::LoadComplete(nsresult) src/layout/base/nsDocumentViewer.cpp:1066:7
    #29 0x7f558679c84b in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) src/docshell/base/nsDocShell.cpp:7217:21
    #30 0x7f5586798c49 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:7010:7
    #31 0x7f55867a054f in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp
    #32 0x7f557d42b0f7 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) src/uriloader/base/nsDocLoader.cpp:1315:3
    #33 0x7f557d42a17a in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:858:14
Flags: in-testsuite?
See Also: → 1454126
CCing stylo folks just because of the stack trace.
Sounds more like a DOM issue, because the loop condition involved there isn't affected by arguments passed in... unless the object itself is corrupt.
I mean, the loop condition in nsAttrAndChildArray::GetAttr.
Yeah, seems unlikely to be style-system related given the test-case. There's something corrupt in the editor state. In particular, notice how each time you fold and unfold the <details>, more garbage appears in the text field.

I can take a look on rr if nobody wins me to that.
Flags: needinfo?(emilio)
This asserts fatally on a debug build:

  Assertion failure: IsText(), at /home/emilio/src/moz/gecko/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/Text.h
Assignee: nobody → emilio
Flags: needinfo?(emilio)
This is a regression from bug 1449404.

The content in:

  https://searchfox.org/mozilla-central/rev/fca4426325624fecbd493c31389721513fc49fef/layout/forms/nsTextControlFrame.cpp#1290

Is not text (it's an HR element), and we mess up from there. The HR is inserted by document.execCommand. Probably that should be looked into. Going to submit a wallpaperish patch.
Blocks: 1449404
Attachment #8967956 - Flags: review?(bzbarsky)
Even with that, the test-case is super-NS_ASSERTION / NS_WARNING happy, but that's less of an issue.
Priority: -- → P1
Comment on attachment 8967920 [details]
testcase.html

Looks like that this is same as bug 1454126 since this inserts <hr> element into the <input> element accidentally. Then, editor related code may crash because a lot of places assume that the DOM tree in <input>/<textarea> elements are always same structure as expected.
Masayuki, do you plan to fix the underlying bug in that bug? (I don't have access to it).

If so and that can land easily, feel free to just dupe this one. Otherwise we can land this I guess. Feel free to also steal the review if you want.
Flags: needinfo?(masayuki)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Masayuki, do you plan to fix the underlying bug in that bug? (I don't have
> access to it).
> 
> If so and that can land easily, feel free to just dupe this one. Otherwise
> we can land this I guess. Feel free to also steal the review if you want.

Yeah, I'm waiting sec-approval now. And I cannot reproduce the crash with patched build.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(masayuki)
Resolution: --- → DUPLICATE
Comment on attachment 8967956 [details] [diff] [review]
Wallpaperish patch that is consistent with surrounding code.

Sorry for the lag here.

I guess before bug 1449404 this would have asserted and returned failure from the method but been safe...

It's probably worth making this change to GetAsText(), just in case another editor bug creeps in.  Though it really sucks to have supposed invariants we can't really rely on.  :(
Attachment #8967956 - Flags: review?(bzbarsky) → review+
Masayuki - do we want to land this patch anyways, as bz suggests?
Flags: needinfo?(emilio)
301 Masayuki, I suspect that was the intention at least. :)
Flags: needinfo?(emilio) → needinfo?(masayuki)
If we need to land it, I think that we should check if it's nullptr in the else block for making clearer where didn't work as expected.

Perhaps, taking this patch must be safer since editor code still might not guarantee that native anonymous tree as expected in some cases and I guess that the patch does not make damage to the performance.  On the other hand, shouldn't we land it from dummy bug# instead of this bug for reducing the zero day attack risk with different path from bug 1454126?.
Flags: needinfo?(masayuki)
re comment 15 - al?
Flags: needinfo?(abillings)
I'm happy for things to land in dummy bugs or with other work in general.
Flags: needinfo?(abillings)
Group: dom-core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: