Ignore invalid GUI related features and options

resolves: QubesOS/qubes-issues#7730

.gitlab-ci.yml

@@ -7,7 +7,7 @@ include:
   project: QubesOS/qubes-continuous-integration
 checks:pylint:
   before_script:
-  - sudo dnf install -y python3-rpm
+  - sudo dnf install -y python3-rpm python3-xlib
   - pip3 install --quiet -r ci/requirements.txt
   script:
   - export PATH="$HOME/.local/bin:$PATH"
@@ -19,15 +19,15 @@ checks:tests:
   after_script:
   - ci/codecov-wrapper
   before_script:
-  - sudo dnf install -y openssl python3-rpm sequoia-sqv
+  - sudo dnf install -y openssl python3-rpm sequoia-sqv python3-xlib libnotify
+    xorg-x11-server-Xvfb
   - pip3 install --quiet -r ci/requirements.txt
   - git config --global --add safe.directory "$PWD"
   script:
   - python3 setup.py build
   # simulate "normal" qubes env for most tests
-  - export DISPLAY=:0
-  - ./run-tests
-  - python3 -m coverage xml
+  - xvfb-run ./run-tests
+  - xvfb-run python3 -m coverage xml
   stage: checks
   tags:
   - docker

debian/control

@@ -28,6 +28,7 @@ Depends:
  qubes-repo-templates,
  scrypt,
  qubes-rpm-oxide (>= 0.2.3),
+ python3-xlib
  ${python:Depends},
  ${python3:Depends},
  ${misc:Depends},

qubesadmin/tests/tools/qvm_start_daemon.py

@@ -27,7 +27,9 @@
 
 import qubesadmin.tests
 import qubesadmin.tools.qvm_start_daemon
-from  qubesadmin.tools.qvm_start_daemon import GUI_DAEMON_OPTIONS
+from qubesadmin.tools.qvm_start_daemon import GUI_DAEMON_OPTIONS
+from qubesadmin.tools.qvm_start_daemon import validator_key_sequence, \
+        validator_trayicon_mode, validator_color
 import qubesadmin.vm
 
 
@@ -93,7 +95,7 @@ def setup_common_args(self):
             ('test-vm', 'admin.vm.property.Get', 'xid', None)] = \
             b'0\x00default=99 type=int 99'
 
-        for name, _kind in GUI_DAEMON_OPTIONS:
+        for name, _kind, _validator in GUI_DAEMON_OPTIONS:
             self.app.expected_calls[
                 ('test-vm', 'admin.vm.feature.Get',
                  'gui-' + name.replace('_', '-'), None)] = \
@@ -754,3 +756,33 @@ def test_070_send_monitor_layout_all(self):
                                unittest.mock.call(vm2, monitor_layout)])
         mock_get_monior_layout.assert_called_once_with()
         self.assertAllCalled()
+
+    def test_071_validators(self):
+        self.assertFalse(validator_key_sequence(123))
+        self.assertFalse(validator_key_sequence('Super-X'))
+        self.assertTrue(validator_key_sequence('Ctrl-shift-F12'))
+        self.assertFalse(validator_key_sequence('F13'))
+        self.assertFalse(validator_trayicon_mode(True))
+        self.assertTrue(validator_trayicon_mode('bg'))
+        self.assertTrue(validator_trayicon_mode('tint+border1'))
+        self.assertFalse(validator_trayicon_mode('shadow'))
+        self.assertFalse(validator_color(321))
+        self.assertTrue(validator_color('0x123456'))
+        self.assertTrue(validator_color('0XFFF'))
+        self.assertTrue(validator_color('lime'))
+        self.assertFalse(validator_color('scarlet'))
+        self.assertFalse(validator_color('octarine'))
+
+    def test_072_feature_validation(self):
+        self.setup_common_args()
+
+        self.app.expected_calls[
+            ('gui-vm', 'admin.vm.feature.Get',
+             'gui-default-override-redirect', None)] = \
+                 b'0\x00ask'
+
+        _args, config = self.run_common_args()
+        self.assertEqual(config, '''\
+global: {
+}
+''')

qubesadmin/tools/qvm_start_daemon.py

@@ -39,22 +39,75 @@
 import qubesadmin.vm
 from qubesadmin.tools import xcffibhelpers
 
+# pylint: disable=wrong-import-position,wrong-import-order
+from Xlib import X, XK
+from Xlib.display import Display
+from Xlib.error import DisplayConnectionError
+
 GUI_DAEMON_PATH = '/usr/bin/qubes-guid'
 PACAT_DAEMON_PATH = '/usr/bin/pacat-simple-vchan'
 QUBES_ICON_DIR = '/usr/share/icons/hicolor/128x128/devices'
 
+def validator_key_sequence(sequence: str) -> bool:
+    """ xside.c key sequence validation is not case sensitive and supports more
+    choices than Global Config's limited choices, so we replicate it here """
+    if not isinstance(sequence, str):
+        return False
+    while '-' in sequence:
+        modifier, sequence = sequence.split('-', 1)
+        if not modifier.lower() in \
+                ['shift', 'ctrl', 'alt', 'mod1', 'mod2', 'mod3', 'mod4']:
+            return False
+    try:
+        display = Display()
+        result = display.keysym_to_keycode(XK.string_to_keysym(sequence))
+        display.close()
+        return result != X.NoSymbol
+    except DisplayConnectionError:
+        return False
+
+def validator_trayicon_mode(mode: str) -> bool:
+    """ xside.c tray mode validation is replicated here """
+    if not isinstance(mode, str):
+        return False
+    if mode in ['bg', 'border1', 'border2', 'tint']:
+        return True
+    if mode.startswith('tint'):
+        if mode[4:] in ['+border1', '+border2', '+saturation50', '+whitehack']:
+            return True
+    return False
+
+def validator_color(color: str) -> bool:
+    """ xside.c `parse_color` validation code is replicated here """
+    if not isinstance(color, str):
+        return False
+    if re.match(r"^0[xX](?:[0-9a-fA-F]{3}){1,2}$", color.strip()):
+        # Technically `parse_color` could parse values such as `0x0`
+        # Xlib.xobject.colormap conventions & standards are different.
+        return True
+    try:
+        display = Display()
+        result = display.screen().default_colormap.alloc_named_color(color)
+        display.close()
+        if result is not None:
+            return True
+    except DisplayConnectionError:
+        return False
+    return False
+
 GUI_DAEMON_OPTIONS = [
-    ('allow_fullscreen', 'bool'),
-    ('override_redirect_protection', 'bool'),
-    ('override_redirect', 'str'),
-    ('allow_utf8_titles', 'bool'),
-    ('secure_copy_sequence', 'str'),
-    ('secure_paste_sequence', 'str'),
-    ('windows_count_limit', 'int'),
-    ('trayicon_mode', 'str'),
-    ('window_background_color', 'str'),
-    ('startup_timeout', 'int'),
-    ('max_clipboard_size', 'int'),
+    ('allow_fullscreen', 'bool', (lambda x: isinstance(x, bool))),
+    ('override_redirect_protection', 'bool', (lambda x: isinstance(x, bool))),
+    ('override_redirect', 'str', (lambda x: x in ['allow', 'disable'])),
+    ('allow_utf8_titles', 'bool', (lambda x: isinstance(x, bool))),
+    ('secure_copy_sequence', 'str', validator_key_sequence),
+    ('secure_paste_sequence', 'str', validator_key_sequence),
+    ('windows_count_limit', 'int', (lambda x: isinstance(x, int) and x > 0)),
+    ('trayicon_mode', 'str', validator_trayicon_mode),
+    ('window_background_color', 'str', validator_color),
+    ('startup_timeout', 'int', (lambda x: isinstance(x, int) and x >= 0)),
+    ('max_clipboard_size', 'int', \
+            (lambda x: isinstance(x, int) and 256 <= x <= 256000)),
 ]
 
 formatter = logging.Formatter(
@@ -108,12 +161,12 @@ def retrieve_gui_daemon_options(vm, guivm):
 
     options = {}
 
-    for name, kind in GUI_DAEMON_OPTIONS:
-        feature_value = vm.features.get(
-            'gui-' + name.replace('_', '-'), None)
+    for name, kind, validator in GUI_DAEMON_OPTIONS:
+        feature = 'gui-' + name.replace('_', '-')
+        feature_value = vm.features.get(feature, None)
         if feature_value is None:
-            feature_value = guivm.features.get(
-                'gui-default-' + name.replace('_', '-'), None)
+            feature = 'gui-default-' + name.replace('_', '-')
+            feature_value = guivm.features.get(feature, None)
         if feature_value is None:
             continue
 
@@ -126,6 +179,15 @@ def retrieve_gui_daemon_options(vm, guivm):
         else:
             assert False, kind
 
+        if not validator(value):
+            message = f"{vm.name}: Ignoring invalid feature:\n" \
+                      f"{feature}={feature_value}"
+            log.error(message)
+            if not sys.stdout.isatty():
+                subprocess.run(['notify-send', '-a', 'Qubes GUI Daemon', \
+                    '--icon', 'dialog-warning', message], check=False)
+            continue
+
         options[name] = value
     return options
 
@@ -141,7 +203,7 @@ def serialize_gui_daemon_options(options):
         '',
         'global: {',
     ]
-    for name, kind in GUI_DAEMON_OPTIONS:
+    for name, kind, validator in GUI_DAEMON_OPTIONS:
         if name in options:
             value = options[name]
             if kind == 'bool':
@@ -153,6 +215,14 @@ def serialize_gui_daemon_options(options):
             else:
                 assert False, kind
 
+            if not validator(value):
+                message = f"Ignoring invalid GUI property:\n{name}={value}"
+                log.error(message)
+                if not sys.stdout.isatty():
+                    subprocess.run(['notify-send', '-a', 'Qubes GUI Daemon', \
+                        '--icon', 'dialog-warning', message], check=False)
+                continue
+
             lines.append('  {} = {};'.format(name, serialized))
     lines.append('}')
     lines.append('')

rpm_spec/qubes-core-admin-client.spec.in

@@ -42,6 +42,7 @@ Requires:   python%{python3_pkgversion}-qubesdb
 Requires:   python%{python3_pkgversion}-rpm
 Requires:   python%{python3_pkgversion}-tqdm
 Requires:   python%{python3_pkgversion}-pyxdg
+Requires:   python%{python3_pkgversion}-xlib
 Conflicts:  qubes-core-dom0 < 4.3.0
 Conflicts:  qubes-manager < 4.0.6