From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from jazzhorn.ncsc.mil (mummy.ncsc.mil [144.51.88.129]) by tarius.tycho.ncsc.mil (8.13.1/8.13.1) with SMTP id l2KMRWwD008424 for ; Tue, 20 Mar 2007 18:27:32 -0400 Message-ID: <46005FD3.8020203@tycho.nsa.gov> Date: Tue, 20 Mar 2007 18:27:31 -0400 From: Eamon Walsh MIME-Version: 1.0 To: "Christopher J. PeBenito" CC: selinux@tycho.nsa.gov Subject: Re: [PATCH] refpolicy: experimental X policy -v2 References: <45B938EE.8010303@tycho.nsa.gov> <45D2498C.3090902@tycho.nsa.gov> <1172602429.11157.48.camel@sgc> In-Reply-To: <1172602429.11157.48.camel@sgc> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Sender: owner-selinux@tycho.nsa.gov List-Id: selinux@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 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.