Closed Bug 1445234 Opened 3 years ago Closed 3 years ago

IPC: crash [@get_gtk_cursor]

Categories

(Core :: DOM: Content Processes, defect)

All
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 + fixed
firefox61 + fixed

People

(Reporter: posidron, Assigned: Alex_Gaynor)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [adv-main60+][post-critsmash-triage])

Attachments

(1 file)

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)
Group: core-security → network-core-security
Component: Widget: Gtk → Event Handling
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.
Group: network-core-security → dom-core-security
The correct fix for this will be basically the same as https://hg.mozilla.org/mozilla-central/rev/145c594a51c5
Bug 1449420 looks like it might have the same root cause.
See Also: → 1449420
I agree with comment #2.
Component: Event Handling → DOM: Content Processes
Duplicate of this bug: 1449420
Attached patch 1445234.patchSplinter Review
Assignee: nobody → agaynor
Attachment #8966234 - Flags: review?(bugs)
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.
This bug is about a compromised child process.
Attachment #8966234 - Flags: review?(bugs) → review+
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.
Attachment #8966234 - Flags: sec-approval?
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.
Attachment #8966234 - Flags: sec-approval? → sec-approval+
Keywords: checkin-needed
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.
Flags: needinfo?(agaynor)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a2385811ffdc
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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]:
Flags: needinfo?(agaynor)
Attachment #8966234 - Flags: approval-mozilla-beta?
Comment on attachment 8966234 [details] [diff] [review]
1445234.patch

Fix an IPC sec-high. Approved for 60.0b12.
Attachment #8966234 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: dom-core-security → core-security-release
Whiteboard: [adv-main60+]
Flags: qe-verify-
Whiteboard: [adv-main60+] → [adv-main60+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.