Closed Bug 1590965 Opened 3 years ago Closed 3 years ago

nsClipboard::Observe() is spinning the event loop

Categories

(Core :: Widget: Gtk, defect)

Desktop
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox-esr68 --- wontfix
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

Details

(Keywords: csectype-uaf, sec-moderate, Whiteboard: [adv-main72+r][post-critsmash-triage])

Attachments

(4 files)

nsClipboard::Observe() calls into gtk_clipboard_store(), which runs a nested event loop.
Spinning the event loop permits calls to nsIObserver::remove(), for example, which is forbidden during notifications

This has led to multiple kinds of UaF problems (due to remove not functioning as expected).
e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1506911

ShutdownXPCOM() processes pending events before advancing to "xpcom-shutdown-threads" and so I assume dispatching an event to call gtk_clipboard_store() would suffice.

I wonder whether both "quit-application" and "xpcom-shutdown" need be observed.

"xpcom-shutdown" dates back to https://hg.mozilla.org/mozilla-central/rev/296dd5f8849a#l3.84
"quit-application" dates back to https://hg.mozilla.org/mozilla-central/rev/270a70535807#l1.94

Group: core-security → layout-core-security
Assignee: nobody → karlt
Status: NEW → ASSIGNED

This notifies GTK that the data is no longer available for clipboard_get_cb(),
so that GTK will no longer advertise nor attempt to store the data.

This stores the clipboard even if it was set in a GTK dialog.

Depends on D50764

Attachment #9104082 - Attachment description: Bug 1590965 call gtk_clipboard_store() from an event r?stransky → bug 1590965 call gtk_clipboard_store() from an event r?stransky
Group: layout-core-security → core-security-release

Comment on attachment 9104082 [details]
bug 1590965 call gtk_clipboard_store() from an event r?stransky

Please consider this a request for all patches on this bug.

Beta/Release Uplift Approval Request

  • User impact if declined: Potentially unexpected results on shutdown of browser (parent) process, such as a UaF.
    This would be very hard to exploit and so there is no pressing need for uplift to beta (or esr).
  • Is this code covered by automated tests?: Yes
  • 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: Medium
  • Why is the change risky/not risky? (and alternatives if risky): The logic has changed a bit, which introduces some risk.
  • String changes made/needed: None.
Attachment #9104082 - Flags: approval-mozilla-beta?
Attachment #9104561 - Flags: approval-mozilla-beta?
Attachment #9104562 - Flags: approval-mozilla-beta?
Attachment #9104563 - Flags: approval-mozilla-beta?

Karl, would it be ok to let it ride the 72 train given that there is medium risk introduced by the logic change?

Flags: needinfo?(karlt)

Yes, I think that is a reasonable option here.

Attachment #9104082 - Flags: approval-mozilla-beta?
Attachment #9104561 - Flags: approval-mozilla-beta?
Attachment #9104562 - Flags: approval-mozilla-beta?
Attachment #9104563 - Flags: approval-mozilla-beta?
Regressions: 1593714
Attachment #9104561 - Attachment description: Bug 1590965 clear GtkClipboard on nsIClipboard::EmptyClipboard r?stransky → bug 1590965 clear GtkClipboard on nsIClipboard::EmptyClipboard r?stransky
Whiteboard: [adv-main72+r]
Flags: qe-verify-
Whiteboard: [adv-main72+r] → [adv-main72+r][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.