Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow shortcut for multiple use of clipboard data #159

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Allow shortcut for multiple use of clipboard data #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions gui-daemon/guid.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ global: {
#
# secure_copy_sequence = "Ctrl-Shift-c";
# secure_paste_sequence = "Ctrl-Shift-v";
#
# Secure paste key sequence clears global clipboard after pasting. User could
# set another key sequence for multiple-use of clipboard data. This could be
# potentially a security risk and is disabled by default.
#
# secure_multipaste_sequence = "None";

# Limit number of windows
#
Expand Down
31 changes: 26 additions & 5 deletions gui-daemon/xside.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -976,7 +976,7 @@ static Time get_clipboard_xevent_timestamp(bool logging) {

/* fetch clippboard content from file */
/* lock already taken in is_special_keypress() */
static void get_qubes_clipboard(Ghandles *g, char **data, int *len)
static void get_qubes_clipboard(Ghandles *g, char **data, int *len, bool multipaste)
{
FILE *file;
*len = 0;
Expand Down Expand Up @@ -1041,7 +1041,16 @@ static void get_qubes_clipboard(Ghandles *g, char **data, int *len)
fclose(file);
metadata.sent_size = *len;
metadata.successful = true;
clear_clipboard(&metadata);
if (multipaste) {
// triggering notification by updating file modification time
if (utimensat(0, QUBES_CLIPBOARD_FILENAME, NULL, 0) == -1) {
alimirjamali marked this conversation as resolved.
Show resolved Hide resolved
show_error_message(g, "secure multi-paste: failed to update modification time of file " QUBES_CLIPBOARD_FILENAME);
} else {
metadata.cleared = false;
save_clipboard_metadata(&metadata);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be quite confusing IMO. Here, you save metadata as after paste operation (paste_action=1), but later this will use it as after copy operation for another paste. The confusion is especially about "vmname" field, as it no longer matches the source of the current clipboard content. As it's written right now, it incidentally works, because you gui-daemon still uses the separate "source" file for policy evaluation, but it's very easy to miss this subtle difference (for example if gui-daemon or something else would be modified to use metadata file for the source vm information when pasting).

Maybe for multipaste it should leave most of the metadata unchanged (just update xevent_timestamp)? Or do not reuse "vmname" field for the name where the clipboard was pasted and instead add a new field for that (used also for paste+wipe)? The latter will potentially allow even more informative notification, as you'll have both source and target name at the same time.

As for copy_action / paste_action fields, I'm not sure how multi-paste should handle them. One of the options is to set both to true in this case. But I guess if properly documented, copy_action=false, paste_action=true may also work (together with cleared=false) may also work. Or even change this to a single "last_action" field?

Anyway, it would be good to write down somewhere (extend QubesOS/qubes-doc#1434? or a comment in xside.h?) all possible states, and what values different fields may have at that time and what do they mean. And also specify which fields may be missing and how to interpret it (see below). IIUC there are the following states:

Right now I see for example that copy made by gui-daemon and by qui-clipboard widgets result in a different metadata files (widget doesn't set "oversized_request"). This may be okay if we have documentation what that means (in this case, that it should be interpreted as oversized_request=false).

}
} else
clear_clipboard(&metadata);
}

/* This is specific to Microsoft Windows and non-X11 compliant OS */
Expand Down Expand Up @@ -1513,8 +1522,13 @@ static int is_special_keypress(Ghandles * g, const XKeyEvent * ev, XID remote_wi
}

/* paste */
if (((int)ev->state & SPECIAL_KEYS_MASK) == g->paste_seq_mask
&& ev->keycode == XKeysymToKeycode(g->display, g->paste_seq_key)) {
bool multipaste = false;
if (((int)ev->state & SPECIAL_KEYS_MASK) == g->multipaste_seq_mask
&& ev->keycode == XKeysymToKeycode(g->display, g->multipaste_seq_key)) {
multipaste = true;
}
if (multipaste || (((int)ev->state & SPECIAL_KEYS_MASK) == g->paste_seq_mask
&& ev->keycode == XKeysymToKeycode(g->display, g->paste_seq_key))) {
if (ev->type != KeyPress)
return 1;
inter_appviewer_lock(g, 1);
Expand All @@ -1541,7 +1555,7 @@ static int is_special_keypress(Ghandles * g, const XKeyEvent * ev, XID remote_wi
hdr.type = MSG_CLIPBOARD_DATA;
if (g->log_level > 0)
fprintf(stderr, "secure paste\n");
get_qubes_clipboard(g, &data, &len);
get_qubes_clipboard(g, &data, &len, multipaste);
if (len > 0) {
/* MSG_CLIPBOARD_DATA used to use the window field to pass the length
of the blob, be aware when working with old implementations. */
Expand Down Expand Up @@ -4345,6 +4359,8 @@ static void load_default_config_values(Ghandles * g)
g->copy_seq_key = XK_c;
g->paste_seq_mask = ControlMask | ShiftMask;
g->paste_seq_key = XK_v;
g->multipaste_seq_mask = 0;
g->multipaste_seq_key = NoSymbol;
g->clipboard_buffer_size = DEFAULT_CLIPBOARD_BUFFER_SIZE;
g->allow_fullscreen = 0;
g->override_redirect_protection = 1;
Expand Down Expand Up @@ -4416,6 +4432,11 @@ static void parse_vm_config(Ghandles * g, config_setting_t * group)
parse_key_sequence(config_setting_get_string(setting),
&g->paste_seq_mask, &g->paste_seq_key);
}
if ((setting =
config_setting_get_member(group, "secure_multipaste_sequence"))) {
parse_key_sequence(config_setting_get_string(setting),
&g->multipaste_seq_mask, &g->multipaste_seq_key);
}

if ((setting =
config_setting_get_member(group, "max_clipboard_size"))) {
Expand Down
2 changes: 2 additions & 0 deletions gui-daemon/xside.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,8 @@ struct _global_handles {
KeySym copy_seq_key; /* key for secure-copy key sequence */
int paste_seq_mask; /* modifiers mask for secure-paste key sequence */
KeySym paste_seq_key; /* key for secure-paste key sequence */
int multipaste_seq_mask; /* modifiers mask for secure-multipaste key sequence */
KeySym multipaste_seq_key; /* key for secure-multipaste key sequence */
unsigned int clipboard_buffer_size; /* maximum clipboard size limit */
int qrexec_clipboard; /* 0: use GUI protocol to fetch/put clipboard, 1: use qrexec */
int use_kdialog; /* use kdialog for prompts (default on KDE) or zenity (default on non-KDE) */
Expand Down