QSB-068: Disconnecting a video output can cause XScreenSaver to crash

News
1

We have just published Qubes Security Bulletin (QSB) 068: Disconnecting a video output can cause XScreenSaver to crash. The text of this QSB is reproduced below. This QSB and its accompanying signatures will always be available in the Qubes Security Pack (qubes-secpack).

View QSB-068 in the qubes-secpack:

https://github.com/QubesOS/qubes-secpack/blob/master/QSBs/qsb-068-2021.txt

Learn about the qubes-secpack, including how to obtain, verify, and read it:

https://www.qubes-os.org/security/pack/

View all past QSBs:

https://www.qubes-os.org/security/qsb/



             ---===[ Qubes Security Bulletin 068 ]===---

                             2021-06-04


     Disconnecting a video output can cause XScreenSaver to crash


User action required
=====================

Users must install the following specific packages in order to address
the issues discussed in this bulletin:

  For Qubes 4.0, in dom0:
  - xscreensaver 5.45-5

  For Qubes 4.1, in dom0:
  - xscreensaver 5.45-5

These packages will migrate from the security-testing repository to the
current (stable) repository over the next two weeks after being tested
by the community. [1] Once available, the packages are to be installed
via the Qubes Update Tool or its command-line equivalents. [2]

After installing this update, the XScreenSaver daemon process must be
restarted in order for the changes to take effect. This can be done by
restarting dom0, logging out of dom0 then logging back in, or issuing
the following command in a dom0 terminal:

    xscreensaver-command -exit; xscreensaver &


Summary
========

XScreenSaver is the default screen locker in dom0. It tracks which video
outputs are connected to the system in order to blank them properly. In
some specific hardware configurations, disconnecting an output can cause
XScreenSaver to crash, leaving the screen unlocked.

Impact
=======

On hardware configurations with more than 10 video outputs that can be
disconnected, an attacker with physical access to a screen-locked system
may be able to unlock it by physically disconnecting one or more
outputs, bypassing standard screen lock authentication.

Details
========

On X11, screen locking and blanking is done by creating a window that
obscures the whole screen, which is a standard practice. In
XScreenSaver, each such window is assigned a specific property. When a
video output is disconnected, its corresponding blanking window is
destroyed, and its XScreenSaver-specific property is removed so that it
will not be used by `xscreensaver-command` anymore. This is handled by
the `update_screen_layout()` function in the `driver/screens.c` file:

     985 /* Synchronize the contents of si->ssi to the current state of the monitors.
     986    Doesn't change anything if nothing has changed; otherwise, alters and
     987    reuses existing saver_screen_info structs as much as possible.
     988    Returns True if anything changed.
     989  */
     990 Bool
     991 update_screen_layout (saver_info *si)
     992 {
     993   monitor **monitors = scan_monitors (si);
     994   int count = 0;
     995   int good_count = 0;
    ...
    1009   while (monitors[count])
    1010     {
    1011       if (monitors[count]->sanity == S_SANE)
    1012         good_count++;
    1013       count++;
    1014     }
    1015 
    1016   if (si->ssi_count == 0)
    1017     {
    1018       si->ssi_count = 10;
    1019       si->screens = (saver_screen_info *)
    1020         calloc (sizeof(*si->screens), si->ssi_count);
    1021     }
    1022 
    1023   if (si->ssi_count <= good_count)
    1024     {
    1025       si->ssi_count = good_count + 10;
    1026       si->screens = (saver_screen_info *)
    1027         realloc (si->screens, sizeof(*si->screens) * si->ssi_count);
    1028       memset (si->screens + si->nscreens, 0,
    1029               sizeof(*si->screens) * (si->ssi_count - si->nscreens));
    1030     }
    ...
    1092   for (; j < count; j++)
    1093     {
    1094       saver_screen_info *ssi = &si->screens[j];
    1095       if (!ssi->screensaver_window)
    1096         continue;
    1097       fprintf (stderr, "%s: %d: screen now unused, disabling.\n",
    1098                blurb(), j);
    1099       /* Undo store_saver_id() so that xscreensaver-command doesn't attempt
    1100          to communicate with us through this window. It might make more
    1101          sense to destroy the window, but I'm not 100% sure that there are
    1102          no outstanding grabs on it that have yet been transferred.
    1103        */
    1104       XDeleteProperty (si->dpy, ssi->screensaver_window,
    1105                        XA_SCREENSAVER_VERSION);
    1106     }

The initial portion of the function counts how many outputs are defined
(the `count` variable) and how many of them are connected (the
`good_count` variable). Then, the `si->screens` array is allocated or
re-allocated to fit information about connected outputs, with an extra
margin of 10 entries. However, the loop at the end iterates over the
array up to the total number of outputs, not just the ones that are
connected.

If there are 10 or fewer disconnected outputs, this works fine. However,
if there are more than 10, it will access the array beyond its end,
reading unrelated data from memory. It will interpret this data as an
XScreenSaver window ID. If that unrelated data happens to be non-zero
(which is very likely), then the condition at line 1095 will not skip
it, and the `XDeleteProperty` call will operate on that (most likely
invalid) window ID. This, in turn, will cause the XScreenSaver process
to crash, as that's what the error handler is programmed to do (the
`saver_ehandler()` function in the `driver/xscreensaver.c` file).

The error message will look like this:

    ##############################################################################
    
    xscreensaver: 11:17:59: X Error!  PLEASE REPORT THIS BUG.
    xscreensaver: 11:17:59: screen 0/0: 0x2ae, 0x0, 0x6600001
    xscreensaver: 11:17:59: screen 0/1: 0x2ae, 0x0, 0x0
    
    ##############################################################################
    
    X Error of failed request:  BadWindow (invalid Window parameter)
      Major opcode of failed request:  19 (X_DeleteProperty)
      Resource id in failed request:  0x188dba0
      Serial number of failed request:  4284
      Current serial number in output stream:  4286
    
    #######################################################################


The issue affects only XScreenSaver version 5.45. Versions 5.44 and
older, as well as 6.00, are not affected. The XScreenSaver author was
notified about this issue and decided not to publish an advisory, as the
issue does not affect the most recent version.

The Qubes Security Team has decided to address this issue in Qubes OS by
patching this specific bug rather than immediately upgrading to the 6.00
version. The reason is that XScreenSaver 6.00 is a major update with
major architectural changes. As such, it poses an increased risk of
introducing unrelated problems. However, this decision does not preclude
the possibility of updating to XScreenSaver 6.00 at some point in the
future, independently of this particular security patch.

Credits
========

The issue was reported by Mustafa Kuscu. [3]

References
===========

[1] https://www.qubes-os.org/doc/testing/
[2] https://www.qubes-os.org/doc/updating-qubes-os/
[3] https://github.com/QubesOS/qubes-issues/issues/6595

--
The Qubes Security Team
https://www.qubes-os.org/security/
This is a companion discussion topic for the original entry at https://www.qubes-os.org/news/2021/06/04/qsb-068/
2
For users who don't know how to upgrade from `xscreensaver 5.45-4` to `xscreensaver 5.45-5`, the command to execute in the dom0 terminal is : sudo qubes-dom0-update --enablerepo=qubes-dom0-current-testing --action=upgrade xscreensaver

@adw QSB enhancement proposition : IMHO, add (all) the commands to execute. In fact, not all the users read or understand the below included links:

Edit: crossed out text, please don’t follow my above update command, read and follow the QSB instructions.

3

In addition, the specific command for salt/qubesctl would be nice as well.

4

No, this was all entirely intentional. We already considered all of this and decided against it.

We provided the Testing New Releases and Updates | Qubes OS link and did not reproduce the exact instructions because anyone who wants to use security-testing (or any testing repo, for that matter) should understand what they’re doing before enabling it, which means reading the documentation.

We linked to (A) Testing New Releases and Updates | Qubes OS rather than (B) Installing and updating software in dom0 | Qubes OS, because:

  1. (A) provides an overview that explains the role of testing and how it works in the Qubes OS Project.

  2. (A) provides information about how the testing repos work, how updates migrate to current, and how to provide feedback (updates-status).

  3. (A) provides links to instructions for enabling both dom0 and TemplateVM testing repos, whereas (B) is only about dom0. It’s possible that, for some QSBs, the fix may be delivered through packages for TemplateVMs in addition to or instead of dom0. We would probably forget to change this. Better to not have to worry about changing it by linking to the more general document in the first place.

There’s already a note about the command-line equivalents on Updating Qubes OS | Qubes OS with links to documentation including example commands. We don’t think it’s necessary to try to duplicate exact commands in the QSB, as that would likely lead to problems with non-technical users trying to use the commands without sufficient understanding.

5

And that is exactly why we shouldn’t just provide the commands.

6

It may not be obvious if you haven’t been on the other side of user/developer relationship with something like this, but there are a thousand things that can go wrong. To give you just one example:

User blindly copies exact command to install update from security-testing. Update breaks installation in some way. User loses some work, has an emotional breakdown, goes on life mission to crusade against the evils of Qubes for ruining their life, when in reality user had no business installing updates from a testing repo. Had wrong expectations for a testing update. Had no backups. Had no technical ability to troubleshoot, diagnose, or fix own problems without making things worse. Probably did not even realize they were installing a testing update to begin with, because the command was provided in easy-to-copy form without any context.

Again, that was just one example. There are so many ways this can go wrong. Users who can’t even figure out how to read one page written in plain English and click one hyperlink to the get to the command are the last people who should be handed such power. A lot like handing someone with no training a gun to shoot themselves in the foot.

7

Also, this should already be obvious, but users do not need to learn or know any of these commands. Just use the Qubes Update tool, and you will get the updates when they hit the current branch. That is the way it’s intended to work and the way it does work. Regular users are not supposed to be enabling testing repos, especially if they cannot understand or cannot learn what that means or how to do it. The testing repos are for people who are savvy enough to use them and who are willing to help testing things.

8

Hi @adw ,
OK I understand this point of view. I’ll no more provide such commands.