Closed Bug 1542324 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Find Toolbar, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 67+ fixed
firefox67 + fixed
firefox68 --- fixed

People

(Reporter: tsmith, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-bounds, sec-high, testcase, Whiteboard: [adv-main67+][adv-esr60.7+])

Attachments

(3 files)

Attached file 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)
Flags: in-testsuite?

Brad, can you take a look? Thanks.

Flags: needinfo?(bwerth)
Keywords: sec-high

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.

Assignee: nobody → bwerth
Flags: needinfo?(bwerth)

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.
Attachment #9060838 - Flags: sec-approval?

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
Attachment #9060845 - Flags: approval-mozilla-esr60?

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.

Attachment #9060838 - Flags: sec-approval? → sec-approval+
Group: core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Comment on attachment 9060845 [details] [diff] [review]
Bug1542324-esr60.patch

Fix for sec-high issue, let's take it for esr 60.7.
Attachment #9060845 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+

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

Flags: needinfo?(bwerth)

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:
Flags: needinfo?(bwerth)
Attachment #9060838 - Flags: approval-mozilla-beta?

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

Uplift accepted for 67 beta 16, thanks.

Attachment #9060838 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [adv-main67+][adv-esr60.7+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.