Skip to content

Commit

Permalink
xside: Introduce override_redirect protection
Browse files Browse the repository at this point in the history
Prior to this commit, an application (malicious or not) could create
a very large window with the override_redirect attribute set. If the
window in question was large enough to prevent the user from interacting
with the window manager and/or Qubes OS widgets, it was impossible to
terminate the application and/or the hosting VM via regular means.

Hence, this commit introduces a simple protection measure against very
large windows that have the override_redirect attribute set. The
protection works by unsetting the override_redirect attribute for
windows that attempt to cover more than 90% of the screen. Doing so
allows the user to move and/or minimize the windows in question.

When the protection takes effect for the first time, the user is warned
once with a persistent notification about what just happened and is
informed of a way to disable this protection on a per-VM basis.
("persistent" notifications need to be clicked on to be dismissed.)

The protection feature can be disabled via /etc/qubes/guid.conf in dom0,
and this commit introduces an example in the aforementioned file along
with an explanation to help users.

(cherry picked from commit 8d2f822)

QubesOS/qubes-issues#4705
  • Loading branch information
m-v-b authored and marmarek committed May 17, 2020
1 parent b885b8d commit f40a8fc
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 13 deletions.
19 changes: 19 additions & 0 deletions gui-daemon/guid.conf
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
global: {
# default values
#allow_fullscreen = false;
#override_redirect_protection = true;
#allow_utf8_titles = false;
# Change the lines below to change the secure copy and paste keyboard shortcuts
# Before changing secure_(copy|paste)_sequence below because you need
Expand Down Expand Up @@ -34,5 +35,23 @@ VM: {
#example-media-vm: {
# allow_fullscreen = true;
#};
# Example of a VM that may create very large windows that
# have the override_redirect flag set. This flag allows a
# window to unconditionally cover all other windows and
# causes the window manager to make it impossible to
# minimize or hide the window in question.
#
# Qubes OS will prevent a window having the override_redirect
# flag set from covering more than 90% of the screen as a
# protection measure. The protection measure unsets this
# flag and lets the window manager (and hence the user)
# control the window.
#
# If this causes issues with a VM's or an application's usage,
# please adapt this example to disable this protection for
# a specific VM.
#example-vm-with-large-unusual-windows: {
# override_redirect_protection = false;
#}
};

79 changes: 66 additions & 13 deletions gui-daemon/xside.c
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ static void release_mapped_mfns(Ghandles * g, struct windowdata *vm_window);
static void print_backtrace(void);
static void parse_cmdline_prop(Ghandles *g);

static void show_error_message (Ghandles * g, const char *msg)
static void show_message(Ghandles *g, const char *prefix, const char *msg,
gint timeout)
{
char message[1024];
NotifyNotification *notify;
Expand All @@ -110,8 +111,9 @@ static void show_error_message (Ghandles * g, const char *msg)
fprintf(stderr, "Failed to init notification subsystem\n");
return;
}
snprintf(message, sizeof message, "ERROR(%s): %s", g->vmname, msg);
snprintf(message, sizeof message, "%s(%s): %s", prefix, g->vmname, msg);
notify = notify_notification_new(message, NULL, g->cmdline_icon);
notify_notification_set_timeout(notify, timeout);
if (!notify_notification_show(notify, NULL)) {
fprintf(stderr, "Failed to send notification\n");
}
Expand All @@ -122,6 +124,11 @@ static void show_error_message (Ghandles * g, const char *msg)
notify_uninit();
}

static void show_error_message(Ghandles *g, const char *msg)
{
show_message(g, "ERROR", msg, NOTIFY_EXPIRES_DEFAULT);
}

/* ask user when VM sent invalid message */
static int ask_whether_verify_failed(Ghandles * g, const char *cond)
{
Expand Down Expand Up @@ -1116,6 +1123,53 @@ static int force_on_screen(Ghandles * g, struct windowdata *vm_window,
return do_move;
}

static void set_override_redirect(Ghandles * g, struct windowdata *vm_window,
int req_override_redirect)
{
static int warning_shown;
uint64_t avail, desired;
const char * warning_msg = "This VM has attempted to create a very large window "
"in a manner that would have prevented you from closing it and regaining "
"the control of Qubes OS\'s graphical user interface.\n\n"
"As a protection measure, the \"override_redirect\" flag of the window "
"in question has been unset. If this creates unexpected issues in the "
"handling of this VM\'s windows, please set \"override_redirect_protection\" "
"to \"false\" for this VM in /etc/qubes/guid.conf to disable this protection "
"measure and restart the VM.\n\n"
"This message will only appear once per VM per session. Please click on this "
"notification to close it.";

req_override_redirect = !!req_override_redirect;

avail = (uint64_t) g->root_width * (uint64_t) g->root_height;

This comment has been minimized.

Copy link
@DemiMarie

DemiMarie May 25, 2020

Contributor

@marmarek could this overflow? Or does X already block creating such absurdly large windows?

This comment has been minimized.

Copy link
@marmarek

marmarek May 25, 2020

Member

Bigger than 2^64 pixels, I believe so. Anyway, this isn't really about large window, but physical screen size (something outside of VM control). The one below is about VM window size which we enforce to be max 16k * 6k.

desired = (uint64_t) vm_window->width * (uint64_t) vm_window->height;

if (g->override_redirect_protection && req_override_redirect &&
desired > ((avail * MAX_OVERRIDE_REDIRECT_PERCENTAGE) / 100)) {
req_override_redirect = 0;

if (g->log_level > 0)
fprintf(stderr,
"%s unset override_redirect for "
"local 0x%x remote 0x%x, "
"window w=%u h=%u, root w=%d h=%d\n",
__func__,
(unsigned) vm_window->local_winid,
(unsigned) vm_window->remote_winid,
vm_window->width, vm_window->height,
g->root_width, g->root_height);

/* Show a message to the user, but do this only once. */
if (!warning_shown) {
show_message(g, "WARNING", warning_msg,
NOTIFY_EXPIRES_NEVER);
warning_shown = 1;
}
}

vm_window->override_redirect = req_override_redirect;
}

/* handle local Xserver event: XConfigureEvent
* after some checks/fixes send to relevant window in VM */
static void process_xevent_configure(Ghandles * g, const XConfigureEvent * ev)
Expand Down Expand Up @@ -1182,7 +1236,7 @@ static void handle_configure_from_vm(Ghandles * g, struct windowdata *vm_window)
{
struct msg_configure untrusted_conf;
int x, y;
unsigned width, height, override_redirect;
unsigned width, height;
int conf_changed;

read_struct(g->vchan, untrusted_conf);
Expand All @@ -1204,10 +1258,6 @@ static void handle_configure_from_vm(Ghandles * g, struct windowdata *vm_window)
width = untrusted_conf.width;
height = untrusted_conf.height;
VERIFY(width > 0 && height > 0);
if (untrusted_conf.override_redirect > 0)
override_redirect = 1;
else
override_redirect = 0;
x = max(-MAX_WINDOW_WIDTH,
min((int) untrusted_conf.x, MAX_WINDOW_WIDTH));
y = max(-MAX_WINDOW_HEIGHT,
Expand All @@ -1218,7 +1268,7 @@ static void handle_configure_from_vm(Ghandles * g, struct windowdata *vm_window)
conf_changed = 1;
else
conf_changed = 0;
vm_window->override_redirect = override_redirect;
set_override_redirect(g, vm_window, !!(untrusted_conf.override_redirect));

/* We do not allow a docked window to change its size, period. */
if (vm_window->is_docked) {
Expand Down Expand Up @@ -1776,10 +1826,7 @@ static void handle_create(Ghandles * g, XID window)
min((int) untrusted_crt.x, MAX_WINDOW_WIDTH));
vm_window->y = max(-MAX_WINDOW_HEIGHT,
min((int) untrusted_crt.y, MAX_WINDOW_HEIGHT));
if (untrusted_crt.override_redirect)
vm_window->override_redirect = 1;
else
vm_window->override_redirect = 0;
set_override_redirect(g, vm_window, !!(untrusted_crt.override_redirect));
parent = untrusted_crt.parent;
/* sanitize end */
vm_window->remote_winid = window;
Expand Down Expand Up @@ -2229,7 +2276,7 @@ static void handle_map(Ghandles * g, struct windowdata *vm_window)
} else
vm_window->transient_for = NULL;

vm_window->override_redirect = !!(untrusted_txt.override_redirect);
set_override_redirect(g, vm_window, !!(untrusted_txt.override_redirect));
attr.override_redirect = vm_window->override_redirect;
XChangeWindowAttributes(g->display, vm_window->local_winid,
CWOverrideRedirect, &attr);
Expand Down Expand Up @@ -3003,6 +3050,7 @@ static void load_default_config_values(Ghandles * g)
g->paste_seq_mask = ControlMask | ShiftMask;
g->paste_seq_key = XK_v;
g->allow_fullscreen = 0;
g->override_redirect_protection = 1;
g->startup_timeout = 45;
g->audio_low_latency = 1;
g->trayicon_mode = TRAY_TINT;
Expand Down Expand Up @@ -3087,6 +3135,11 @@ static void parse_vm_config(Ghandles * g, config_setting_t * group)
g->allow_fullscreen = config_setting_get_bool(setting);
}

if ((setting =
config_setting_get_member(group, "override_redirect_protection"))) {
g->override_redirect_protection = config_setting_get_bool(setting);
}

if ((setting =
config_setting_get_member(group, "audio_low_latency"))) {
g->audio_low_latency = config_setting_get_bool(setting);
Expand Down
5 changes: 5 additions & 0 deletions gui-daemon/xside.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
/* default width of forced colorful border */
#define BORDER_WIDTH 2

/* maximum percentage of the screen that an override_redirect window can cover */
#define MAX_OVERRIDE_REDIRECT_PERCENTAGE 90

/* this makes any X11 error fatal (i.e. cause exit(1)). This behavior was the
* case for a long time before introducing this option, so nothing really have
* changed */
Expand Down Expand Up @@ -177,6 +180,8 @@ struct _global_handles {
pid_t kill_on_connect; /* pid to kill when connection to gui agent is established */
int allow_utf8_titles; /* allow UTF-8 chars in window title */
int allow_fullscreen; /* allow fullscreen windows without decoration */
int override_redirect_protection; /* disallow override_redirect windows to cover more than
MAX_OVERRIDE_REDIRECT_PERCENTAGE percent of the screen */
int copy_seq_mask; /* modifiers mask for secure-copy key sequence */
KeySym copy_seq_key; /* key for secure-copy key sequence */
int paste_seq_mask; /* modifiers mask for secure-paste key sequence */
Expand Down

0 comments on commit f40a8fc

Please sign in to comment.