Hi,
On Tue, Jan 15, 2019 at 5:11 PM Victor Toso <victortoso@xxxxxxxxxx> wrote:
>
> From: Victor Toso <me@xxxxxxxxxxxxxx>
>
> SpiceGtkSession::allow-clipboard-managers property is introduced to
> enable other applications in the Client OS to set or fetch clipboard
> data from a spice-gtk-session that is under keyboard-grab, which is
> expected to be in use by the user.
>
> This option is disabled by default as to avoid:
> 1) Another application in the client to fetch clipboard data while the
> user is interacting with the remote machine;
You're implementing this by leaving the GtkSelectionData empty upon
GtkClipboardGetFunc.
This seems weird to me. By grabbing the clipboard, spice-gtk proclaims
it can provide clipboard data, but if another app asks for this data,
spice-gtk provides none.
It naturally depends on the implementation of the given app, but I
think that in the case of a clipboard manager, user could see empty
entries in the clipboard history, which is imho unwanted. (haven't
tested)
If spice-gtk is not willing to give the data to other apps, it
shouldn't advertise it in the first place, I think.
So I would rather implement it by delaying the grab the same way you
do it in case 2) below.
> 2) Another application to set clipboard data *and* send that to the
> remote machine while the user is *not* interacting with the remote
> machine (without keyboard grab)
I see that you're trying to prevent apps in guest from eavesdropping
on the client clipboard while the user is not interacting with the
remote machine.
But I think there's another point of view: if the given app grabbed
the clipboard, it can also be interpreted as that the app decided to
share its data with other apps. And in that case, I don't see why
spice-gtk should impede the sharing. Note, that e.g. obscured
passwords in browsers cannot be copied in the clipboard (this is an
example of the case when app doesn't want to share its data).
Apart from that, there isn't really any good way to clear the
clipboard afaik, unless you store something new in it. Doesn't seem
like something a lot of users would do. So the sensitive data would
get leaked anyway the moment you click into the remote machine's
window.
So I'm rather inclined to agree with the opinion that this should be
enforced by the desktop, not us.
Cheers,
Jakub
>
> This patch disables (1) and following patch disables (2) if
> allow-clipboard-managers is disabled which is set to be default.
>
> This patch also fixes some indentation found in spice-gtk-session.c
>
> Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
> ---
> src/spice-gtk-session.c | 51 +++++++++++++++++++++++++++++++++++++----
> tools/spicy.c | 6 +++++
> 2 files changed, 53 insertions(+), 4 deletions(-)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index adc72a2..7391e6a 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -59,6 +59,7 @@ struct _SpiceGtkSessionPrivate {
> gboolean clip_hasdata[CLIPBOARD_LAST];
> gboolean clip_grabbed[CLIPBOARD_LAST];
> gboolean clipboard_by_guest[CLIPBOARD_LAST];
> + gboolean back_from_focus_out;
> /* auto-usbredir related */
> gboolean auto_usbredir_enable;
> int auto_usbredir_reqs;
> @@ -66,6 +67,7 @@ struct _SpiceGtkSessionPrivate {
> gboolean keyboard_has_focus;
> gboolean mouse_has_pointer;
> gboolean sync_modifiers;
> + gboolean allow_clipboard_managers;
> };
>
> /**
> @@ -114,6 +116,7 @@ enum {
> PROP_AUTO_USBREDIR,
> PROP_POINTER_GRABBED,
> PROP_SYNC_MODIFIERS,
> + PROP_ALLOW_CLIPBOARD_MANAGER,
> };
>
> static guint32 get_keyboard_lock_modifiers(void)
> @@ -287,6 +290,9 @@ static void spice_gtk_session_get_property(GObject *gobject,
> case PROP_SYNC_MODIFIERS:
> g_value_set_boolean(value, s->sync_modifiers);
> break;
> + case PROP_ALLOW_CLIPBOARD_MANAGER:
> + g_value_set_boolean(value, s->allow_clipboard_managers);
> + break;
> default:
> G_OBJECT_WARN_INVALID_PROPERTY_ID(gobject, prop_id, pspec);
> break;
> @@ -337,6 +343,9 @@ static void spice_gtk_session_set_property(GObject *gobject,
> case PROP_SYNC_MODIFIERS:
> s->sync_modifiers = g_value_get_boolean(value);
> break;
> + case PROP_ALLOW_CLIPBOARD_MANAGER:
> + s->allow_clipboard_managers = g_value_get_boolean(value);
> + break;
> default:
> G_OBJECT_WARN_INVALID_PROPERTY_ID(gobject, prop_id, pspec);
> break;
> @@ -441,6 +450,23 @@ static void spice_gtk_session_class_init(SpiceGtkSessionClass *klass)
> G_PARAM_READWRITE |
> G_PARAM_CONSTRUCT |
> G_PARAM_STATIC_STRINGS));
> + /**
> + * SpiceGtkSession:allow-clipboard-managers
> + *
> + * Allow clipboard managers to set and get clipboard data. With this option
> + * disabled, only user direct actions should change the clipboard data.
> + *
> + * Since: 0.36
> + **/
> + g_object_class_install_property
> + (gobject_class, PROP_ALLOW_CLIPBOARD_MANAGER,
> + g_param_spec_boolean("allow-clipboard-managers",
> + "Allow clipboard managers",
> + "Allow clipboard managers to interact with Clipboard",
> + FALSE,
> + G_PARAM_READWRITE |
> + G_PARAM_CONSTRUCT |
> + G_PARAM_STATIC_STRINGS));
> }
>
> /* ---------------------------------------------------------------- */
> @@ -745,7 +771,15 @@ static void clipboard_get(GtkClipboard *clipboard,
> gulong agent_handler;
> int selection;
>
> - SPICE_DEBUG("clipboard get");
> + if (!s->allow_clipboard_managers &&
> + spice_gtk_session_get_keyboard_has_focus(self)) {
> + SPICE_DEBUG("clipboard get: clipboard-manager=no, ignore request");
> + return;
> + }
> +
> + SPICE_DEBUG("clipboard get: clipboard-manager=%s, keyboard-grab=%s",
> + spice_yes_no(s->allow_clipboard_managers),
> + spice_yes_no(spice_gtk_session_get_keyboard_has_focus(self)));
>
> selection = get_selection_from_clipboard(s, clipboard);
> g_return_if_fail(selection != -1);
> @@ -1277,16 +1311,25 @@ gboolean spice_gtk_session_get_pointer_grabbed(SpiceGtkSession *self)
>
> G_GNUC_INTERNAL
> void spice_gtk_session_set_keyboard_has_focus(SpiceGtkSession *self,
> - gboolean keyboard_has_focus)
> + gboolean keyboard_has_focus)
> {
> + SpiceGtkSessionPrivate *s;
> +
> g_return_if_fail(SPICE_IS_GTK_SESSION(self));
>
> - self->priv->keyboard_has_focus = keyboard_has_focus;
> + s = self->priv;
> + if (!s->keyboard_has_focus && keyboard_has_focus) {
> + s->back_from_focus_out = TRUE;
> + } else {
> + s->back_from_focus_out = FALSE;
> + }
> +
> + s->keyboard_has_focus = keyboard_has_focus;
> }
>
> G_GNUC_INTERNAL
> void spice_gtk_session_set_mouse_has_pointer(SpiceGtkSession *self,
> - gboolean mouse_has_pointer)
> + gboolean mouse_has_pointer)
> {
> g_return_if_fail(SPICE_IS_GTK_SESSION(self));
> self->priv->mouse_has_pointer = mouse_has_pointer;
> diff --git a/tools/spicy.c b/tools/spicy.c
> index 8a6d077..e5bfa17 100644
> --- a/tools/spicy.c
> +++ b/tools/spicy.c
> @@ -801,6 +801,7 @@ static const char *spice_gtk_session_properties[] = {
> "auto-clipboard",
> "auto-usbredir",
> "sync-modifiers",
> + "allow-clipboard-managers",
> };
>
> static const GtkToggleActionEntry tentries[] = {
> @@ -828,6 +829,10 @@ static const GtkToggleActionEntry tentries[] = {
> .name = "sync-modifiers",
> .label = "Sync modifiers",
> .callback = G_CALLBACK(menu_cb_bool_prop),
> + },{
> + .name = "allow-clipboard-managers",
> + .label = "Allow clipboard managers",
> + .callback = G_CALLBACK(menu_cb_bool_prop),
> },{
> .name = "auto-clipboard",
> .label = "Automatic clipboard sharing between host and guest",
> @@ -936,6 +941,7 @@ static char ui_xml[] =
> " <menuitem action='scaling'/>\n"
> " <menuitem action='disable-inputs'/>\n"
> " <menuitem action='sync-modifiers'/>\n"
> +" <menuitem action='allow-clipboard-managers'/>\n"
> " <menuitem action='auto-clipboard'/>\n"
> " <menuitem action='auto-usbredir'/>\n"
> " <menu action='CompressionMenu'>\n"
> --
> 2.20.1
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel