1445234 - IPC: crash [@get_gtk_cursor]

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Christoph Diehl [:posidron]]

https://searchfox.org/mozilla-central/source/widget/gtk/nsWindow.cpp#5270

    static GdkCursor *
    get_gtk_cursor(nsCursor aCursor)
    {
        GdkCursor *gdkcursor = nullptr;
        uint8_t newType = 0xff;

*       if ((gdkcursor = gCursorCache[aCursor])) {
            return gdkcursor;
        }
        [...]
    }


==24467==ERROR: AddressSanitizer: SEGV on unknown address 0x7fd0179f6118 (pc 0x7fcc0d227471 bp 0x7ffcd8ff45d0 sp 0x7ffcd8ff4580 T0)
==24467==The signal is caused by a READ memory access.
    #0 0x7fcc0d227470 in get_gtk_cursor /builds/worker/workspace/build/src/widget/gtk/nsWindow.cpp:5270:22
    #1 0x7fcc0d227470 in nsWindow::SetCursor(nsCursor) /builds/worker/workspace/build/src/widget/gtk/nsWindow.cpp:1604
    #2 0x7fcc0cad0341 in mozilla::dom::TabParent::SendRealMouseEvent(mozilla::WidgetMouseEvent&) /builds/worker/workspace/build/src/dom/ipc/TabParent.cpp:1100:17
    #3 0x7fcc0b4955a9 in mozilla::EventStateManager::DispatchCrossProcessEvent(mozilla::WidgetEvent*, nsFrameLoader*, nsEventStatus*) /builds/worker/workspace/build/src/dom/events/EventStateManager.cpp:1308:13
    #4 0x7fcc0b496857 in mozilla::EventStateManager::HandleCrossProcessEvent(mozilla::WidgetEvent*, nsEventStatus*) /builds/worker/workspace/build/src/dom/events/EventStateManager.cpp:1447:5
    #5 0x7fcc0b4aaf83 in mozilla::EventStateManager::DispatchMouseOrPointerEvent(mozilla::WidgetMouseEvent*, mozilla::EventMessage, nsIContent*, nsIContent*) /builds/worker/workspace/build/src/dom/events/EventStateManager.cpp:4176:9
    #6 0x7fcc0b4ae070 in mozilla::EventStateManager::NotifyMouseOver(mozilla::WidgetMouseEvent*, nsIContent*) /builds/worker/workspace/build/src/dom/events/EventStateManager.cpp:4365:5
    #7 0x7fcc0b48bd67 in mozilla::EventStateManager::GenerateMouseEnterExit(mozilla::WidgetMouseEvent*) /builds/worker/workspace/build/src/dom/events/EventStateManager.cpp:4516:9
    #8 0x7fcc0b48633b in mozilla::EventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*) /builds/worker/workspace/build/src/dom/events/EventStateManager.cpp:730:5
    #9 0x7fcc0da1c873 in mozilla::PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*, bool) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:7715:19
    #10 0x7fcc0da1a044 in mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:7364:17
    #11 0x7fcc0d14e61a in nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) /builds/worker/workspace/build/src/view/nsViewManager.cpp:812:14
    #12 0x7fcc0d9f331d in mozilla::PresShell::DispatchSynthMouseMove(mozilla::WidgetGUIEvent*, bool) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:3773:33
    #13 0x7fcc0da09649 in mozilla::PresShell::ProcessSynthMouseMoveEvent(bool) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:5689:12
    #14 0x7fcc0da4a23a in mozilla::PresShell::nsSynthMouseMoveEvent::WillRefresh(mozilla::TimeStamp) /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/PresShell.h:661:16
    #15 0x7fcc0d962847 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1886:12
    #16 0x7fcc0d972f70 in TickDriver /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:340:13
    #17 0x7fcc0d972f70 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:310
    #18 0x7fcc0d972b36 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:332:5
    #19 0x7fcc0d9758ae in RunRefreshDrivers /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:773:5
    #20 0x7fcc0d9758ae in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:686
    #21 0x7fcc0d970957 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:532:20
    #22 0x7fcc0536e685 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1040:14
    #23 0x7fcc0538abf0 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:517:10
    #24 0x7fcc06254eda in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #25 0x7fcc061a1a09 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #26 0x7fcc061a1a09 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #27 0x7fcc061a1a09 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #28 0x7fcc0d1dd96a in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
    #29 0x7fcc114a5c6b in nsAppStartup::Run() /builds/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:288:30
    #30 0x7fcc116bd38c in XREMain::XRE_mainRun() /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4679:22
    #31 0x7fcc116c02dc in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4814:8
    #32 0x7fcc116c1724 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4906:21
    #33 0x4f6d45 in do_main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:231:22
    #34 0x4f6d45 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:304
    #35 0x7fcc25ce91c0 in __libc_start_main /build/glibc-itYbWN/glibc-2.26/csu/../csu/libc-start.c:308
    #36 0x4265bc in _start (/home/worker/firefox/firefox+0x4265bc)

Comment 1

[Andrew McCreight [:mccr8]]

I moved this to event handling because it seems more likely that EventStateManager is calling with bad data than the GTK code is doing something wrong. Though maybe the latter should be bounds checking.

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

I believe the bug is actually here: https://searchfox.org/mozilla-central/source/dom/ipc/TabParent.cpp#1724-1727

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

The correct fix for this will be basically the same as https://hg.mozilla.org/mozilla-central/rev/145c594a51c5

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Bug 1449420 looks like it might have the same root cause.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

I agree with comment #2.

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

Attachment: 1445234.patch

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

So is the crash hypothetical in case the child process is compromised and sends invalid data?
Or how does ASAN work here?
Or is child process sending invalid value even in normal case. That is then something to fix, too.

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

This bug is about a compromised child process.

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8966234 [details] [diff] [review]
1445234.patch

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

The vulnerability it reveals is pretty obvious from the lines changed. This is an IPC vulnerability, so exploitation requires a compromised content process.

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

No, but it's pretty obvious from the lines changed.

Which older supported branches are affected by this flaw?

All of them -- but ESR52 isn't important since it doesn't have a sandbox in need of escaping.

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

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

Should backport cleanly.

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

Unlikely.

Comment 11

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

sec-approval+ for trunk.
I'll "won't fix" it for ESR52 but we'll want a Beta patch nomination so it can go there after it lands on mozilla-central.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

There's actually a small conflict in WidgetMessageUtils.h for the Beta uplift, but it'll be trivial to rebase around. Feel free to request approval when ready.

Comment 13

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

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

Comment 14

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8966234 [details] [diff] [review]
1445234.patch

Approval Request Comment
[Feature/Bug causing the regression]: security bug
[User impact if declined]: vulnerability
[Is this code covered by automated tests?]: not sure truth be told
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not at all
[Why is the change risky/not risky?]: adds additional validation to IPC that will never fail unless someone is trying to exploit firefox
[String changes made/needed]:

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8966234 [details] [diff] [review]
1445234.patch

Fix an IPC sec-high. Approved for 60.0b12.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/65d2876668ea