1924108 - (CVE-2025-5269) UAF nsWindow @ nsGlobalWindowInner::GetWorkspaceID()

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Looks like mShell is e5e5. mShell is a raw ptr, so this could make sense.
Note: only a single crash with this stack in the last 6 months, so likely very hard to hit, or might even be due to something bizarre like a bit-flip causing it to free something early; who knows.

(the signature will have a ton of crashes, but if you filter on proto_signature you find only one)

Crash report: https://crash-stats.mozilla.org/report/index/bc80ba67-6144-4321-a768-4dc150240921

Reason:

SIGSEGV / SI_KERNEL

Top 10 frames:

0  libgtk-3.so.0  libgtk-3.so.0@0x354326
1  libxul.so  nsWindow::GetToplevelGdkWindow() const  widget/gtk/nsWindow.cpp:6942
1  libxul.so  nsWindow::GetWorkspaceID(nsTSubstring<char16_t>&)  widget/gtk/nsWindow.cpp:2872
2  libxul.so  nsGlobalWindowInner::GetWorkspaceID(nsTSubstring<char16_t>&)  dom/base/nsGlobalWindowInner.cpp:7014
2  libxul.so  mozilla::dom::Window_Binding::getWorkspaceID(JSContext*, JS::Handle<JSObject*...  dom/bindings/WindowBinding.cpp:7795
3  libxul.so  mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::May...  dom/bindings/BindingUtils.cpp:3268
4  libxul.so  CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::...  js/src/vm/Interpreter.cpp:487
4  libxul.so  js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstru...  js/src/vm/Interpreter.cpp:581
4  libxul.so  InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason)  js/src/vm/Interpreter.cpp:648
4  libxul.so  js::CallFromStack(JSContext*, JS::CallArgs const&, js::CallReason)  js/src/vm/Interpreter.cpp:653

Comment 1

[Andrew McCreight [:mccr8]]

Bug 1923019 also has GetToplevelGdkWindow in the stack.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

It crashes because nsGlobalWindowInner::GetWorkspaceID() calls nsWindow::GetWorkspaceID() on released nsWindow:
https://searchfox.org/mozilla-central/rev/abe6e3de6850eca664e968484f8ecfc53487ea42/dom/base/nsGlobalWindowInner.cpp#7012

Comment 3

[Andrew McCreight [:mccr8]]

I don't think this is a DOM:Core issue. The relevant inner window code is here:

  if (nsCOMPtr<nsIWidget> widget = GetMainWidget()) {
    return widget->GetWorkspaceID(workspaceID);

That all seems perfectly fine to me. widget should hold the nsIWidget alive.

I was thinking that the widget must be alive when it was returned by GetMainWidget because we would have addrefed it, but if the addref got devirtualized by LTO maybe it could be done on a poisoned freed value? (GetMainWidget returns an already addrefed value, so it would be addrefed in a function that might be able to statically tell the specific addref implementation).

But it stills seems to me like the most likely cause here is that nsWindow::mShell is pointing at a freed object in nsWindow::GetToplevelGdkWindow(), because it is a weak pointer.

Looking through the different implementations of GetMainWidget, nsWebBrowser::GetMainWidget can return mParentWidget which is a weak reference.

nsChromeTreeOwner::GetMainWidget returns mAppWindow->mWindow. mAppWindow isn't a strong reference but mWindow is so that should be okay? Dereferencing a freed value would crash right there. nsContentTreeOwner::GetMainWidget is similar.

Comment 4

[Andrew McCreight [:mccr8]]

Why do you think the nsWindow is freed and not the GtkWidget*? Thanks.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

(In reply to Andrew McCreight [:mccr8] from comment #3)

But it stills seems to me like the most likely cause here is that nsWindow::mShell is pointing at a freed object in nsWindow::GetToplevelGdkWindow(), because it is a weak pointer.

We explicitly clear mShell on init and also on nsWindow::Destroy() here:
https://searchfox.org/mozilla-central/rev/18f09bdf36a62ea7079c018301f1d257f71f655b/widget/gtk/nsWindow.cpp#655

the only way how mShell can get a5a5 signature is that whole nsWindow is released.

gtk_widget_get_window() looks like:

GdkWindow*                                
gtk_widget_get_window (GtkWidget *widget)
{
  g_return_val_if_fail (GTK_IS_WIDGET (widget), NULL);

  return widget->priv->window;
}

so it may be possible that widget->priv is 5a5a5a but that means the widget is deleted somehow without our notice but I have no idea how that can happen as we own it.

But okay, I'll try to add destroy handler listener to it so we know for sure.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

With https://phabricator.services.mozilla.com/D226885 landed in case of widget/toolkit bug we should see crashes in nsWindow::GtkWidgetDestroyHandler() - we assert if we're releasing live widget.

Comment 7

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

Here's where we're crashing -- it looks like it's actually the expansion of GTK_IS_WIDGET:

    73b58ef54300:       f3 0f 1e fa             endbr64
    73b58ef54304:       55                      push   %rbp
    73b58ef54305:       48 89 e5                mov    %rsp,%rbp
    73b58ef54308:       53                      push   %rbx
    73b58ef54309:       48 89 fb                mov    %rdi,%rbx   // rbx = widget
    73b58ef5430c:       48 83 ec 08             sub    $0x8,%rsp
    73b58ef54310:       67 e8 0a c9 fe ff       addr32 call 73b58ef40c20   // probably gtk_widget_get_type
    73b58ef54316:       48 85 db                test   %rbx,%rbx
    73b58ef54319:       74 35                   je     73b58ef54350
    73b58ef5431b:       48 89 c6                mov    %rax,%rsi
    73b58ef5431e:       48 8b 03                mov    (%rbx),%rax   // load bad value here
    73b58ef54321:       48 85 c0                test   %rax,%rax
    73b58ef54324:       74 05                   je     73b58ef5432b
    73b58ef54326:       48 39 30                cmp    %rsi,(%rax)   // crash here

But that just brings me to the same conclusion as comment #5: instead of widget->priv->window it's __inst->g_class->g_type (in the macro _G_TYPE_CIT), where __inst is the same pointer as widget and appears to be freed.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

It's possible that nsWindow is not refcounted properly in widget/gtk code so we unref it/release while other components hold reference to it.

I wonder if we may work better with Destroy() - right now we call it from ~nsWindow(). Do we know when the window is supposed to be destroyed? May we remove Destroy() from ~nsWindow() and assert/warn if non-destroyed window is released?

Comment 9

[Martin Stránský [:stransky] (ni? me)]

Bug 1929846 has a patch which asserts on nightly/beta if we're releasing live nsWindow.

Comment 10

[Edgar Chen [:edgar] (OOO 08/03 ~ 08/07)]

(As per the above comments, switching to Widget: GTK for now. Feel free to bounce it back if you think it’s a DOM issue)

Comment 11

[:Gijs (he/him)]

(In reply to Martin Stránský [:stransky] (ni? me) from comment #9)

Bug 1929846 has a patch which asserts on nightly/beta if we're releasing live nsWindow.

This now has crashes, also from release (?) https://crash-stats.mozilla.org/signature/?moz_crash_reason=~mIsDestroyed&signature=nsWindow%3A%3AGtkWidgetDestroyHandler&date=%3E%3D2024-12-17T10%3A45%3A00.000Z&date=%3C2025-03-17T10%3A45%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1#reports , e.g. this one .

Does this help with diagnosis?

Comment 12

[Martin Stránský [:stransky] (ni? me)]

(In reply to :Gijs (he/him) from comment #11)

(In reply to Martin Stránský [:stransky] (ni? me) from comment #9)

Bug 1929846 has a patch which asserts on nightly/beta if we're releasing live nsWindow.

This now has crashes, also from release (?) https://crash-stats.mozilla.org/signature/?moz_crash_reason=~mIsDestroyed&signature=nsWindow%3A%3AGtkWidgetDestroyHandler&date=%3E%3D2024-12-17T10%3A45%3A00.000Z&date=%3C2025-03-17T10%3A45%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1#reports , e.g. this one .

Does this help with diagnosis?

Yes, that's awesome. So the crash is caused by D&D popup which is released after D&D operation before we can do so. It's because we put mShell of D&D window to toplevel D&D widget container owned by Gtk which is destroyed after drop and takes mShell with it.

It may be easily fixed by adding extra reference to mShell.

Comment 13

[Martin Stránský [:stransky] (ni? me)]

So it's really Gtk bug.

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1924108 [Linux] Delete nsWindow from mShell::destroy signal r?emilio

Comment 15

[Martin Stránský [:stransky] (ni? me)]

I think this is pretty rare corner case - we explicitly tell Gtk not to release/dispose icon widget but it does it according to the crash log.
IMHO it's better to delete the popup entirely in such case, it's on D&D drop and it's going to be deleted anyway.

Comment 16

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 9474504 [details]
Bug 1924108 [Linux] Delete nsWindow from mShell::destroy signal r?emilio

Security Approval Request

• How easily could an exploit be constructed based on the patch?: No idea how to reproduce, looks like very rare bug in Gtk3/D&D code.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: 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?: not risky
• How likely is this patch to cause regressions; how much testing does it need?: no regressions, we'll destroy a window which is going to be closed anyway.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: No

Comment 17

[Tom Ritter [:tjr]]

Comment on attachment 9474504 [details]
Bug 1924108 [Linux] Delete nsWindow from mShell::destroy signal r?emilio

Approved to land and request uplift

Comment 18

[BugBot [:suhaib / :marco]]

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:stransky, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 19

[Martin Stránský [:stransky] (ni? me)]

I don't know how secure revisions are handled. Shall I request commit by Lando? Or set any flag to land it?
Thanks.

Comment 20

[Martin Stránský [:stransky] (ni? me)]

Please also land to 138.0.

Comment 21

[Tom Ritter [:tjr]]

Typically yes, after the approval is granted, you can proceed to land it normally via Lando.

Late in the cycle, you should check the uplift deadline, - when it is within a day or same-day you want to avoid a situation where you land it but for some reason it can't be uplifted, so you would request uplift before landing and in the uplift request explain you didn't land it because you didn't want to accidentally cause a problem.

In this instance I can say that uplift shouldn't be a problem and even if it was the risk is very very low, so you can land it via lando normally and request uplift at the same time.

Comment 22

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 9474504 [details]
Bug 1924108 [Linux] Delete nsWindow from mShell::destroy signal r?emilio

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Firefox UAF crash during D&D operation.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• 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): Release nsWindow internals before mShell is deleted.
• String changes made/needed:
• Is Android affected?: Unknown

Comment 23

[Dianna Smith [:diannaS]]

:stransky did you mean to trigger this for landing? needs to land today if it wants to make it for 138.

Comment 24

[Martin Stránský [:stransky] (ni? me)]

(In reply to Dianna Smith [:diannaS] from comment #23)

:stransky did you mean to trigger this for landing? needs to land today if it wants to make it for 138.

Yes.

Comment 25

[Dianna Smith [:diannaS]]

:stransky sorry for the misunderstanding. I meant that this has not landed. Did you need assistance with that? happy to trigger it via lando if you need me to.

Comment 26

[Emilio Cobos Álvarez [:emilio]]

I queued it for landing.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5292260c6898
[Linux] Delete nsWindow from mShell::destroy signal r=emilio

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5292260c6898

Comment 29

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/3628d513810c

Comment 30

[Dianna Smith [:diannaS]]

Comment on attachment 9474504 [details]
Bug 1924108 [Linux] Delete nsWindow from mShell::destroy signal r?emilio

Approved for 138.0rc1

Comment 31

[Dianna Smith [:diannaS]]

Comment on attachment 9474504 [details]
Bug 1924108 [Linux] Delete nsWindow from mShell::destroy signal r?emilio

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: UAF crash
• User impact if declined: Firefox UAF crash during D&D operation.
• Fix Landed on Version: 139
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Release nsWindow internals before mShell is deleted.

Comment 32

[Dianna Smith [:diannaS]]

Comment on attachment 9474504 [details]
Bug 1924108 [Linux] Delete nsWindow from mShell::destroy signal r?emilio

Approved for 128.10esr

Comment 33

[Dianna Smith [:diannaS]]

Comment on attachment 9474504 [details]
Bug 1924108 [Linux] Delete nsWindow from mShell::destroy signal r?emilio

Actually this requires a rebased patch in order to uplift to esr128. (possibly because it requires bug 1927090 to also be uplifted).
:Stransky if you can provide a rebased patch for esr128 i can land it before building the esr RC candidate.

Comment 34

[Dianna Smith [:diannaS]]

this being sec-moderate and being on a time crunch for building RC, I Will probably hold off and uplift this to esr next cycle

Comment 35

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1924108 [Linux] Delete nsWindow from mShell::destroy signal for esr128 r?emilio

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Is there any kind of testing that QA can do to help verify this fix (both from the standpoint of verifying the fix itself and from the standpoint of looking for any functional regressions) that would be helpful on ESR128?

Comment 37

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/132f595d259b

Comment 38

[Martin Stránský [:stransky] (ni? me)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #36)

Is there any kind of testing that QA can do to help verify this fix (both from the standpoint of verifying the fix itself and from the standpoint of looking for any functional regressions) that would be helpful on ESR128?

yes, do D&D operation over titlebar - move tabs, create new window etc.

Comment 39

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I've spent some time D&D lots of tabs, creating new windows by D&D, back and forth and it seems stable. Tested on Firefox 138.0.4, Firefox 139.0, Firefox 140.0a1 and Firefox 128.11.0ESR all on Ubuntu 24.04.