Re: [PATCH] refpolicy: experimental X policy -v2

[Eamon Walsh <ewalsh@tycho.nsa.gov>]

Christopher J. PeBenito wrote:
> On Tue, 2007-02-13 at 18:28 -0500, Eamon Walsh wrote:
>> This is an experimental policy for use with the X userspace object 
>> manager.  It includes both unconfined and strict policy and is 
>> controlled by a tunable, xwindows_object_manager.  The labeling conf 
>> file in the X.org xserver git (XACE-SELINUX branch) assumes that this 
>> policy is loaded, i.e. the types listed in that file are defined in this 
>> policy.
> 
> Unfortunately I didn't get a chance to look at this until today.  It'll
> take some time to fully understand all this, but I have some notes from
> my initial review inline:
> 
>>  modules/services/xwindows.fc |   13 +
>>  modules/services/xwindows.if |  521 +++++++++++++++++++++++++++++++++++++++++++
>>  modules/services/xwindows.te |   65 +++++
> 
> Eventually this should probably be merged into the xserver module.
> Potentially in a tunable, when that support becomes available.  However,
> for the purposes of vetting the design, a separate module is fine.

I think it's important to distinguish between the policy that governs 
the operation of the X server itself and the policy that governs X 
applications.  Putting everything into xserver may blur that distinction.

> 
>> Index: policy/modules/services/xwindows.if
>> ===================================================================
>> --- policy/modules/services/xwindows.if	(revision 0)
>> +++ policy/modules/services/xwindows.if	(revision 0)
>> @@ -0,0 +1,521 @@
> 
>> +template(`xwindows_property',`
>> +	gen_require(`
>> +		type $1_t;
>> +		type $2;
>> +		attribute xproperty_type;
>> +		attribute $1_xproperty_type;
>> +		attribute $2ype;
>> +		class property all_property_perms;
>> +	')
>> +
>> +	type $1_$2, $1_xproperty_type, $2ype, xproperty_type;
>> +	type_transition $1_t $2:property $1_$2;
>> +')
> 
>> +template(`xwindows_property_set',`
>> +
> 
>> +	attribute $1_xproperty_type;
>> +	xwindows_property($1,client_xproperty_t)
>> +	xwindows_property($1,info_xproperty_t)
>> +	xwindows_property($1,seclabel_xproperty_t)
>> +	xwindows_property($1,rm_xproperty_t)
>> +	xwindows_property($1,wm_xproperty_t)
>> +	xwindows_property($1,clipboard_xproperty_t)
>> +	xwindows_property($1,unknown_xproperty_t)
>> +')
> 
> Is there be a situation where one wouldn't want a full set of
> properties?  If not, I'd rather eliminate xwindows_property(), because
> the name mangling being done with the base property type names (e.g.,
> $2ype) is a little ugly.

Yeah I'll work on cleaning this up.  You're right, the full property set 
should be OK in all cases.

> 
> 
>> +template(`xwindows_windowmgr_client',`
> [cut]
>> +		# can set properties on all windows
>> +		allow $3 domain:window { chprop chproplist };
> 
>> +		# X Windows - extensive control over all windows
>> +		# can query windows for visual information
>> +		allow $3 domain:drawable getattr;
>> +		# can enumerate and change attributes of root window
>> +		allow $3 $1_root_window_t:window { enumerate setattr };
>> +		# can enumerate, set, and change attributes of all non-root windows
>> +		allow $3 domain:window { enumerate getattr setattr };
>> +		# can map and unmap all non-root windows
>> +		allow $3 domain:window { map unmap move ctrllife };
>> +		# can send various events to all non-root windows
>> +		allow $3 domain:window { windowchangeevent clientcomevent };
>> +		# can reparent all non-root windows
>> +		allow $3 domain:window { chparent chstack };
>> +		# can list properties of all non-root windows
>> +		allow $3 domain:window listprop;
> 
>> +		# X Input
>> +		# can change input focus on all windows
>> +		allow $3 domain:window setfocus;
> 
> domain attribute can't be used here, since its not owned by this module.
> Its overly broad too.  One thing to investigate would be adding an
> attribute, something like xserver_xclient_type for all X clients, and
> then use that here.  Then that attribute would be added to a domain,
> perhaps via xserver_user_client_template().

Yes, that's fine.

> 
> 
>> +template(`xwindows_displaymgr_client',`
>> +	gen_require(`
>> +		class xextension use;
>> +	')
> 
>> +	xwindows_basic_client($1,$2,$3,$4)
> 
>> +	tunable_policy(`xwindows_object_manager',`
>> +		# X Protocol Extensions
>> +		allow $3 output_xext_t:xextension use;
>> +
>> +		# allow server grabs
>> +		allow $3 $1_xserver_t:xserver { grab ungrab };
>> +		allow $3 $1_xserver_t:xinput { getattr activegrab };
>> +
>> +		# can move the mouse cursor
>> +		allow $3 $1_xserver_t:xinput warppointer;
>> +
>> +		# can set resource manager properties
>> +		allow $3 $2_rm_xproperty_t:property { write free };
>> +
>> +		# can enumerate windows
>> +		allow $3 $1_root_window_t:window enumerate;
>> +	')
>> +')
> 
> I suspect this might work as part of xserver_user_client_template(), but
> the derived type for the property is going to be a problem.

Why is that?  The derived property types should all be defined in the 
same module.

In this particular case though it's probably safe to say that $1 and $2 
will both be "xdm".

I think a deeper question somewhat related to this is what the prefix on 
the X server's domain should be.  In the policy as written I'm assuming 
that the user domain prefix $2 and X server domain prefix $1 are 
independent.  This is because 1) the xserver runs as xdm_xserver_t when 
started from gdm but user_xserver_t when started by user with startx, 
and 2) because you might want to allow things like sysadm_xdomain_t 
programs to work on user_xserver_t X servers.

It would be useful to standardize on either always running the X server 
under a single domain, or always running it with the user prefixed domain.

> 
> 
>> +template(`xwindows_resourcemgr_client',`
>> +	gen_require(`
>> +		class property all_property_perms;
>> +	')
> 
>> +	tunable_policy(`xwindows_object_manager',`
>> +		# X Properties
>> +		# can read and write resource manager settings
>> +		allow $3 $2_rm_xproperty_t:property { read write };
>> +	')
>> +')
> 
> Not sure why there is no $1.  Do you anticipate this being used outside
> of the one use in the patch?  If not, the rule might as well go in the
> caller.
> 
> 
>> Index: policy/modules/services/xserver.if
>> ===================================================================
>> --- policy/modules/services/xserver.if	(revision 2180)
>> +++ policy/modules/services/xserver.if	(working copy)
>> @@ -90,6 +90,13 @@
>>  	kernel_read_kernel_sysctls($1_xserver_t)
>>  	kernel_write_proc_files($1_xserver_t)
>>  
>> +	# X server userspace object manager
>> +	tunable_policy(`xwindows_object_manager',`
>> +		allow $1_xserver_t self:netlink_audit_socket create;
>> +		allow $1_xserver_t self:netlink_selinux_socket { bind create read };
>> +		allow $1_xserver_t security_t:security { check_context compute_av compute_create };
> 
> This should be:
> 
> send_audit_msgs_pattern($1_xserver_t)
> selinux_validate_context($1_xserver_t)
> selinux_compute_access_vector($1_xserver_t)
> selinux_compute_create_context($1_xserver_t)

Thx, used audit2allow and forgot to go back and look up interfaces.

> 
>> Index: policy/modules/system/unconfined.if
>> ===================================================================
>> --- policy/modules/system/unconfined.if	(revision 2180)
>> +++ policy/modules/system/unconfined.if	(working copy)
>> @@ -31,6 +42,19 @@
>>  	allow $1 self:nscd *;
>>  	allow $1 self:dbus *;
>>  	allow $1 self:passwd *;
>> +	tunable_policy(`xwindows_object_manager',`
>> +		allow $1 self:drawable *;
>> +		allow $1 self:window *;
>> +		allow $1 self:gc *;
>> +		allow $1 self:font *;
>> +		allow $1 self:colormap *;
>> +		allow $1 self:property *;
>> +		allow $1 self:cursor *;
>> +		allow $1 self:xclient *;
>> +		allow $1 self:xserver *;
>> +		allow $1 self:xinput *;
>> +		allow $1 self:xextension *;
>> +	')
> 
> I don't think this needs to be tunable.  It probably should go in an
> xserver_unconfined() too (as should the classes for the other userland
> object managers for that matter).
> 

OK.  Will repost.

-- 
Eamon Walsh <ewalsh@tycho.nsa.gov>
National Security Agency

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.