1542324 - global-buffer-overflow in [@ nsFind::Find]

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found with m-c: 20190405-61540ab18c9b

==13169==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7fd82fae905e at pc 0x7fd82ca977e3 bp 0x7ffd07c1b2f0 sp 0x7ffd07c1b2e8

READ of size 2 at 0x7fd82fae905e thread T0 (file:// Content)
    #0 0x7fd82ca977e2 in nsFind::Find(nsTSubstring<char16_t> const&, nsRange*, nsRange*, nsRange*, nsRange**) /src/toolkit/components/find/nsFind.cpp:624:12
    #1 0x7fd82ca9a85f in nsWebBrowserFind::SearchInFrame(nsPIDOMWindowOuter*, bool, bool*) /src/toolkit/components/find/nsWebBrowserFind.cpp:676:14
    #2 0x7fd82ca987a7 in nsWebBrowserFind::FindNext(bool*) /src/toolkit/components/find/nsWebBrowserFind.cpp:109:8
    #3 0x7fd8226029e5 in nsGlobalWindowOuter::FindOuter(nsTSubstring<char16_t> const&, bool, bool, bool, bool, bool, bool, mozilla::ErrorResult&) /src/dom/base/nsGlobalWindowOuter.cpp:6583:20
    #4 0x7fd824d68246 in mozilla::dom::Window_Binding::find(JSContext*, JS::Handle<JSObject*>, nsGlobalWindowInner*, JSJitMethodCallArgs const&) /src/obj-firefox/dom/bindings/WindowBinding.cpp:6595:21
    #5 0x7fd825c1b988 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /src/dom/bindings/BindingUtils.cpp:3150:13
    #6 0x7fd82d4257c7 in CallJSNative /src/js/src/vm/Interpreter.cpp:442:13
    #7 0x7fd82d4257c7 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:534
    #8 0x7fd82d40dbba in CallFromStack /src/js/src/vm/Interpreter.cpp:593:10
    #9 0x7fd82d40dbba in Interpret(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:3075
    #10 0x7fd82d3efbe8 in js::RunScript(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:422:10
    #11 0x7fd82d426138 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:562:13
    #12 0x7fd82d427d82 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:605:8
    #13 0x7fd82e06de99 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /src/js/src/jsapi.cpp:2621:10
    #14 0x7fd825218e10 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:266:37
    #15 0x7fd8264e8df2 in Call<nsCOMPtr<mozilla::dom::EventTarget> > /src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:363:12
    #16 0x7fd8264e8df2 in mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) /src/dom/events/JSEventHandler.cpp:205
    #17 0x7fd826498c4a in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) /src/dom/events/EventListenerManager.cpp:1045:22
    #18 0x7fd82649b223 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) /src/dom/events/EventListenerManager.cpp:1240:17
    #19 0x7fd82647b3f0 in HandleEvent /src/obj-firefox/dist/include/mozilla/EventListenerManager.h:355:5
    #20 0x7fd82647b3f0 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:349
    #21 0x7fd826479af5 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:587:14
    #22 0x7fd826480383 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /src/dom/events/EventDispatcher.cpp:1046:11
    #23 0x7fd822b6730d in FocusBlurEvent::Run() /src/dom/base/nsFocusManager.cpp:1949:12
    #24 0x7fd822485976 in AddScriptRunner /src/dom/base/nsContentUtils.cpp:5333:13
    #25 0x7fd822485976 in nsContentUtils::AddScriptRunner(nsIRunnable*) /src/dom/base/nsContentUtils.cpp:5339
    #26 0x7fd822b0c187 in nsFocusManager::FireFocusOrBlurEvent(mozilla::EventMessage, nsIPresShell*, nsISupports*, bool, bool, mozilla::dom::EventTarget*) /src/dom/base/nsFocusManager.cpp:2097:5
    #27 0x7fd822b09f27 in nsFocusManager::SendFocusOrBlurEvent(mozilla::EventMessage, nsIPresShell*, mozilla::dom::Document*, nsISupports*, unsigned int, bool, bool, mozilla::dom::EventTarget*) /src/dom/base/nsFocusManager.cpp:2065:3
    #28 0x7fd822aff21d in nsFocusManager::Focus(nsPIDOMWindowOuter*, mozilla::dom::Element*, unsigned int, bool, bool, bool, bool, nsIContent*) /src/dom/base/nsFocusManager.cpp:1880:7
    #29 0x7fd822af1c98 in nsFocusManager::SetFocusInner(mozilla::dom::Element*, int, bool, bool) /src/dom/base/nsFocusManager.cpp:1303:5
    #30 0x7fd822af35cd in nsFocusManager::SetFocus(mozilla::dom::Element*, unsigned int) /src/dom/base/nsFocusManager.cpp:457:3
    #31 0x7fd822844f6d in mozilla::dom::Element::Focus(mozilla::ErrorResult&) /src/dom/base/Element.cpp:349:20
    #32 0x7fd8267f15d7 in mozilla::dom::HTMLInputElement::Focus(mozilla::ErrorResult&) /src/dom/html/HTMLInputElement.cpp:2970:27
    #33 0x7fd8267f0f4b in mozilla::dom::HTMLInputElement::Focus(mozilla::ErrorResult&) /src/dom/html/HTMLInputElement.cpp:2952:22
    #34 0x7fd8228bfd0f in mozilla::dom::nsAutoFocusEvent::Run() /src/dom/base/Document.cpp:8820:15
    #35 0x7fd81e349a56 in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1180:14
    #36 0x7fd81e35171d in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:486:10
    #37 0x7fd81f6b37cf in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:88:21
    #38 0x7fd81f588a9e in RunInternal /src/ipc/chromium/src/base/message_loop.cc:315:10
    #39 0x7fd81f588a9e in RunHandler /src/ipc/chromium/src/base/message_loop.cc:308
    #40 0x7fd81f588a9e in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:290
    #41 0x7fd828b761b3 in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:137:27
    #42 0x7fd82d13b98e in XRE_RunAppShell() /src/toolkit/xre/nsEmbedFunctions.cpp:919:20
    #43 0x7fd81f588a9e in RunInternal /src/ipc/chromium/src/base/message_loop.cc:315:10
    #44 0x7fd81f588a9e in RunHandler /src/ipc/chromium/src/base/message_loop.cc:308
    #45 0x7fd81f588a9e in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:290
    #46 0x7fd82d13ab1c in XRE_InitChildProcess(int, char**, XREChildData const*) /src/toolkit/xre/nsEmbedFunctions.cpp:757:34
    #47 0x5606a4176834 in content_process_main /src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
    #48 0x5606a4176834 in main /src/browser/app/nsBrowserApp.cpp:263
    #49 0x7fd84265782f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #50 0x5606a409bebc in _start (/home/ubuntu/builds/m-c-20190405095028-fuzzing-asan-opt/firefox+0x2debc)

Comment 1

[Andrew McCreight [:mccr8]]

Brad, can you take a look? Thanks.

Comment 2

[Brad Werth [:bradwerth]]

nsFind::Find cannot handle empty search strings. This is normally prevented by nsGlobalWindowOuter::FindOuter which will early exit with an empty string. However, this testcase uses some complicated mechanism to reset the finder->mSearchString while finder->FindNext is already executing, so the string is emptied out before the call to nsFind::Find.

A comprehensive fix will be to make nsFind::Find resilient to empty string searches -- which is easy to do. I'm not clear if the odd behavior of the resetting of the mSearchString should also be investigated. It may cause other sorts of problems.

Comment 3

[Brad Werth [:bradwerth]]

Attachment: Bug 1542324 Part 1: Make nsFind::Find handle empty string searches.

Comment 4

[Brad Werth [:bradwerth]]

Comment on attachment 9060838 [details]
Bug 1542324 Part 1: Make nsFind::Find handle empty string searches.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Challenging. This allows array index -1 reading, which seems not at all useful for code injection and of only limited use for out-of-band reading.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Easy to create for ESR 60.
• How likely is this patch to cause regressions; how much testing does it need?: Needs no additional testing. Handles a narrow case that is already handled in most normal calling patterns. This new case requires odd calling patterns that are unlikely to be discovered in the wild.

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug1542324-esr60.patch

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: A specific calling pattern of window.find could do an array -1 read access. Possible data exposure.
• Fix Landed on Version: 68
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Adds a well-understood early exit case that can't cause harm.
• String or UUID changes made by this patch: none

Comment 6

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

Comment on attachment 9060838 [details]
Bug 1542324 Part 1: Make nsFind::Find handle empty string searches.

sec-approval+ for trunk. This doesn't seem super risky.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/5d03854895fb371b5ed6c90fd42cd0f2d4c122d3
https://hg.mozilla.org/mozilla-central/rev/5d03854895fb

Comment 8

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

Comment on attachment 9060845 [details] [diff] [review]
Bug1542324-esr60.patch

Fix for sec-high issue, let's take it for esr 60.7.

Comment 9

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

Brad, can you also request beta uplift? It is usually best to ship the same fix in corresponding release and esr versions.

Comment 10

[Brad Werth [:bradwerth]]

Comment on attachment 9060838 [details]
Bug 1542324 Part 1: Make nsFind::Find handle empty string searches.

Beta/Release Uplift Approval Request

• User impact if declined: A specific calling pattern of window.find could do an array -1 read access. Possible data exposure.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Adds a well-understood early exit case that can't cause harm.
• String changes made/needed:

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/747d7342cab1

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9060838 [details]
Bug 1542324 Part 1: Make nsFind::Find handle empty string searches.

Uplift accepted for 67 beta 16, thanks.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/59b61e692216