Bug 50641

This patch will fix the problem but: Should the keyboard be initialised by the 
SELinuxLabelInitial(); routine in Xext/xselinux_hooks.c so that it has a
context. I could not find a way to do this but as device IDs 0 and 1 are
reserved, then this patch seems reasonable!!!


When SELinux is enabled by 'setsebool xserver_object_manager on' the xserver
will not load. This is caused when ProcXIPassiveUngrabDevice
(in Xi/xipassivegrab.c) calls GrabKey (in Xi/exevents.c) with a device ID
of '1'. This is the keyboard that does not have a security context attached to
it.

Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
---
 Xi/exevents.c |    9 +++++----
 dix/grabs.c   |    8 +++++---
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/Xi/exevents.c b/Xi/exevents.c
index f390f67..a8f9cb8 100644
--- a/Xi/exevents.c
+++ b/Xi/exevents.c
@@ -2386,10 +2386,11 @@ GrabKey(ClientPtr client, DeviceIntPtr dev, DeviceIntPtr modifier_device,
 	return rc;
     if (param->this_device_mode == GrabModeSync || param->other_devices_mode == GrabModeSync)
 	access_mode |= DixFreezeAccess;
-    rc = XaceHook(XACE_DEVICE_ACCESS, client, dev, access_mode);
-    if (rc != Success)
-	return rc;
-
+	if (dev->id > 1) {
+	    rc = XaceHook(XACE_DEVICE_ACCESS, client, dev, access_mode);
+    	if (rc != Success)
+			return rc;
+	}
     grab = CreateGrab(client->index, dev, modifier_device, pWin, grabtype,
                       mask, param, type, key, NULL, NULL);
     if (!grab)
diff --git a/dix/grabs.c b/dix/grabs.c
index cc2c946..cc64515 100644
--- a/dix/grabs.c
+++ b/dix/grabs.c
@@ -583,9 +583,11 @@ AddPassiveGrabToList(ClientPtr client, GrabPtr pGrab)
 
     if (pGrab->keyboardMode == GrabModeSync||pGrab->pointerMode == GrabModeSync)
 	access_mode |= DixFreezeAccess;
-    rc = XaceHook(XACE_DEVICE_ACCESS, client, pGrab->device, access_mode);
-    if (rc != Success)
-	return rc;
+	if (pGrab->device->id > 1) {
+	    rc = XaceHook(XACE_DEVICE_ACCESS, client, pGrab->device, access_mode);
+    	if (rc != Success)
+			return rc;
+	}
 
     /* Remove all grabs that match the new one exactly */
     for (grab = wPassiveGrabs(pGrab->window); grab; grab = grab->next)
-- 
1.7.10.2

I've been trying to find a better fix but but could only come up with the one
below. Anyway I'm coming to the conclusion that maybe ProcXIPassiveUngrabDevice
should not be called with device id=1 as 0 and 1 are reserved (according to
the comments in dix/device.c)

I have not managed to find why device id=1 is used or where the call originates
from but ProcXIPassiveUngrabDevice is called 91 times with the same 
parameters before X is loaded. Does anyone have any info that may help.

The simplest fix to get X working with SELinux is to modify the 
Xi/xipassivegrab.c ProcXIPassiveGrabDevice() function:

---
 Xi/xipassivegrab.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/Xi/xipassivegrab.c b/Xi/xipassivegrab.c
index c80da80..b7e8326 100644
--- a/Xi/xipassivegrab.c
+++ b/Xi/xipassivegrab.c
@@ -192,6 +192,8 @@ ProcXIPassiveGrabDevice(ClientPtr client)
 
     for (i = 0; i < stuff->num_modifiers; i++, modifiers++)
     {
+		if (dev->id < 2)
+			continue;
         uint8_t status = Success;
 
         param.modifiers = *modifiers;
-- 
1.7.10.2

The final fix (well mine anyway):

This patch was created using xorg-server-1.12.0 source.

The bug is caused by X calling XaceHook(XACE_DEVICE_ACCESS, client, ...)
with a device ID of '1' that is XIAllMasterDevices. It would also happen
if the device ID = 0 (XIAllDevices).

The only places currently seen calling with a device id=1 are:
GrabKey - in Xi/exevents.c and AddPassiveGrabToList - in dix/grabs.c
These start life in ProcXIPassiveGrabDevice (in Xi/xipassivegrab.c) that
has been called by XIGrabKeycode.

The patch has been tested using the other XI calls that would also impact
this: XIGrabTouchBegin, XIGrabButton, XIGrabFocusIn and XIGrabEnter with
and without the correct permissions (grab and freeze) with no problems.

Both possible classes have to be checked (x_keyboard and x_pointer) as it
is not known whether it is a pointer or keyboard as this info is not
available. To get this info would require a change to the
XaceHook(XACE_DEVICE_ACCESS, client, ..) call to pass an additional
parameter stating the actual devices (that would defeat the objective of
the XIAllMasterDevices and XIAllDevices dev ids).

Note that there are other devices apart from the keyboard and pointer, for
example on the test system: DeviceID: 9 is the Integrated_Webcam_1.3M. As
it is classed as a slave keyboard it is checked.

Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
---
Xext/xselinux_hooks.c |   44 +++++++++++++++++++++++++++++++++++++++-----
1 file changed, 39 insertions(+), 5 deletions(-)

diff --git a/Xext/xselinux_hooks.c b/Xext/xselinux_hooks.c
index 0d4c9ab..c2b21d6 100644
--- a/Xext/xselinux_hooks.c
+++ b/Xext/xselinux_hooks.c
@@ -336,9 +336,17 @@ SELinuxDevice(CallbackListPtr *pcbl, pointer unused, pointer calldata)
     SELinuxAuditRec auditdata = { .client = rec->client, .dev = rec->dev };
     security_class_t cls;
     int rc;
+    DeviceIntPtr dev = NULL;
+    int i = 0;

     subj = dixLookupPrivate(&rec->client->devPrivates, subjectKey);
-    obj = dixLookupPrivate(&rec->dev->devPrivates, objectKey);
+    /*
+     * The XIAllMasterDevices or XIAllDevices do not have devPrivates
+     * entries. Therefore dixLookupPrivate for the object is done later
+     * for these device IDs.
+     */
+    if (rec->dev->id != XIAllDevices && rec->dev->id != XIAllMasterDevices)
+        obj = dixLookupPrivate(&rec->dev->devPrivates, objectKey);

     /* If this is a new object that needs labeling, do it now */
     if (rec->access_mode & DixCreateAccess) {
@@ -356,12 +364,38 @@ SELinuxDevice(CallbackListPtr *pcbl, pointer unused, pointer calldata)
    }
     }

-    cls = IsPointerDevice(rec->dev) ? SECCLASS_X_POINTER : SECCLASS_X_KEYBOARD;
-    rc = SELinuxDoCheck(subj, obj, cls, rec->access_mode, &auditdata);
-    if (rc != Success)
-    rec->status = rc;
+    if (rec->dev->id != XIAllDevices && rec->dev->id != XIAllMasterDevices) {
+           cls = IsPointerDevice(rec->dev) ? SECCLASS_X_POINTER : SECCLASS_X_KEYBOARD;
+           rc = SELinuxDoCheck(subj, obj, cls, rec->access_mode, &auditdata);
+           if (rc != Success)
+            rec->status = rc;
+        return;
+    } else {
+        /*
+         * Device ID must be 0 or 1
+         * We have to check both possible classes as we don't know whether it
+         * was a pointer or keyboard. Therefore all devices are checked for:
+         *         rec->dev->id == XIAllDevices
+         * and only masters for:
+         *         rec->dev->id == XIAllMasterDevices
+         *
+         * An error is returned should any device fail SELinuxDoCheck
+         */
+        for (dev = inputInfo.devices; dev; dev = dev->next, i++) {
+            if (!IsMaster(dev) && rec->dev->id == XIAllMasterDevices)
+                continue;
+              cls = IsPointerDevice(dev) ? SECCLASS_X_POINTER : SECCLASS_X_KEYBOARD;
+            obj = dixLookupPrivate(&dev->devPrivates, objectKey);
+            rc = SELinuxDoCheck(subj, obj, cls, rec->access_mode, &auditdata);
+            if (rc != Success) {
+                rec->status = rc;
+                return;
+            }
+        }
+    }
}

+
static void
SELinuxSend(CallbackListPtr *pcbl, pointer unused, pointer calldata)
{
-- 
1.7.10.2

Richard sent that patch was to xorg-devel for review about 2 hours ago.

Thanks, Richard.

aforementioned patch on list: http://patchwork.freedesktop.org/patch/10621/

Better version, which I'm pretty sure is still valid:

https://patchwork.freedesktop.org/patch/11097/

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/xserver/issues/423.