Subject: [PATCH 1/1] XSELinux: When SELinux is enabled the xserver seg faults

[Peter Hutterer]

On Tue, Jun 12, 2012 at 02:49:52PM +0100, Richard Haines wrote:
> This patch was created using xorg-server-1.12.0 source.
> 
> When using Fedora 17 with xorg-server-1.12.0 and SELinux is enabled
> ('setsebool xserver_object_manager on') the xserver will not load. The X
> log file has a seg fault pointing to XACE/SELinux. Bug 50641 was raised
> (https://bugs.freedesktop.org/show_bug.cgi?id=50641). The patch below is a
> possible fix.
> 
> 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.

tricky one. the question here is: if I put a grab on all keyboard devices,
should it fail if I'm not allowed to access one of them and thus force the
client to re-do on a per-keyboard basis (which has different semantics, btw)
or should I just continue as usual and grab all _but_ the disallowed ones.
I'm actually leaning towards the latter.  Eamon?

as for the patch itself, seems correct for the disallow-all behaviour.
Please fix up the indentation though.
 
> 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.

the keyboard vs pointer distinction is quite arbitrary, it's a legacy and
many modern devices don't fit into either. the webcam is a keyboard in X
because it has no axes but a KEY_CAMERA.

Cheers,
  Peter
> 
> Signed-off-by: Richard Haines <richard_c_haines at 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
> 
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>