Re: [PATCH] SELinux: Always allow FIOCLEX and FIONCLEX

Demi Marie Obenour <[email protected]> Sat, 19 Feb 2022 20:15:59 -0500
Newsgroups	org.kernel.vger.selinux-refpolicy,org.kernel.vger.linux-kernel,org.kernel.vger.selinux
Message-ID	<[email protected]>
On 2/18/22 10:39, Richard Haines wrote:
> On Thu, 2022-02-17 at 18:55 -0500, Demi Marie Obenour wrote:
>> On 2/15/22 15:34, Paul Moore wrote:
>>> On Mon, Feb 14, 2022 at 2:11 AM Jeffrey Vander Stoep
>>> <[email protected]> wrote:
>>>> On Tue, Feb 8, 2022 at 3:18 PM William Roberts
>>>> <[email protected]> wrote:
>>>>>
>>>>> <snip>
>>>>>
>>>>> This is getting too long for me.
>>>>>
>>>>>>>
>>>>>>> I don't have a strong opinion either way.  If one were to
>>>>>>> allow this
>>>>>>> using a policy rule, it would result in a major policy
>>>>>>> breakage.  The
>>>>>>> rule would turn on extended perm checks across the entire
>>>>>>> system,
>>>>>>> which the SELinux Reference Policy isn't written for.  I
>>>>>>> can't speak
>>>>>>> to the Android policy, but I would imagine it would be the
>>>>>>> similar
>>>>>>> problem there too.
>>>>>>
>>>>>> Excuse me if I am wrong but AFAIK adding a xperm rule does
>>>>>> not turn on
>>>>>> xperm checks across the entire system.
>>>>>
>>>>> It doesn't as you state below its target + class.
>>>>>
>>>>>>
>>>>>> If i am not mistaken it will turn on xperm checks only for
>>>>>> the
>>>>>> operations that have the same source and target/target class.
>>>>>
>>>>> That's correct.
>>>>>
>>>>>>
>>>>>> This is also why i don't (with the exception TIOSCTI for
>>>>>> termdev
>>>>>> chr_file) use xperms by default.
>>>>>>
>>>>>> 1. it is really easy to selectively filter ioctls by adding
>>>>>> xperm rules
>>>>>> for end users (and since ioctls are often device/driver
>>>>>> specific they
>>>>>> know best what is needed and what not)
>>>>>
>>>>>>>>> and FIONCLEX can be trivially bypassed unless
>>>>>>>>> fcntl(F_SETFD)
>>>>>>
>>>>>> 2. if you filter ioctls in upstream policy for example like i
>>>>>> do with
>>>>>> TIOSCTI using for example (allowx foo bar (ioctl chr_file
>>>>>> (not
>>>>>> (0xXXXX)))) then you cannot easily exclude additional ioctls
>>>>>> later where source is
>>>>>> foo and target/tclass is bar/chr_file because there is
>>>>>> already a rule in
>>>>>> place allowing the ioctl (and you cannot add rules)
>>>>>
>>>>> Currently, fcntl flag F_SETFD is never checked, it's silently
>>>>> allowed, but
>>>>> the equivalent FIONCLEX and FIOCLEX are checked. So if you
>>>>> wrote policy
>>>>> to block the FIO*CLEX flags, it would be bypassable through
>>>>> F_SETFD and
>>>>> FD_CLOEXEC. So the patch proposed makes the FIO flags behave
>>>>> like
>>>>> F_SETFD. Which means upstream policy users could drop this
>>>>> allow, which
>>>>> could then remove the target/class rule and allow all icotls.
>>>>> Which is easy
>>>>> to prevent and fix you could be a rule in to allowx 0 as
>>>>> documented in the
>>>>> wiki: https://selinuxproject.org/page/XpermRules
>>>>>
>>>>> The questions I think we have here are:
>>>>> 1. Do we agree that the behavior between SETFD and the FIO
>>>>> flags are equivalent?
>>>>>   I think they are.
>>>>> 2. Do we want the interfaces to behave the same?
>>>>>   I think they should.
>>>>> 3. Do upstream users of the policy construct care?
>>>>>   The patch is backwards compat, but I don't want their to be
>>>>> cruft
>>>>> floating around with extra allowxperm rules.
>>>>
>>>> I think this proposed change is fine from Android's perspective.
>>>> It
>>>> implements in the kernel what we've already already put in place
>>>> in
>>>> our policy - that all domains are allowed to use these IOCLTs.
>>>> https://cs.android.com/android/platform/superproject/+/master:system/sepolicy/public/domain.te;l=312
>>>>
>>>> It'll be a few years before we can clean up our policy since we
>>>> need
>>>> to support older kernels, but that's fine.
>>>
>>> Thanks for the discussion everyone, it sounds like everybody is
>>> okay
>>> with the change - that's good.  However, as I said earlier in this
>>> thread I think we need to put this behind a policy capability, how
>>> does POLICYDB_CAPABILITY_IOCTL_CLOEXEC/"ioctl_skip_cloexec" sound
>>> to
>>> everyone?
>>>
>>> Demi, are you able to respin this patch with policy capability
>>> changes?
>>
>> I can try, but this is something I am doing in my spare time and I
>> have no idea what adding a policy capability would involve.  While I
>> have written several policies myself, I believe this is the first
>> time
>> I have dealt with policy capabilities outside of kernel log output.
>> So it will be a while before I can make a patch.  You would probably
>> be
>> able to write a patch far more quickly and easily.
> 
> RESEND: Forgot to add the updates for libsepol (I think it's complete
> now)
> 
> 
> # Adding A New Policy Capability
> 
> - [Kernel Updates](#kernel-updates)
> - [*libsepol* Library Updates](#libsepol-library-updates)
> - [Reference Policy Updates](#reference-policy-updates)
> 
> ## Kernel Updates
> 
> In kernel source update the following three files with the new
> capability:
> 
> ***security/selinux/include/policycap_names.h***
> 
> Add new entry at end of this list:
> 
> ```
> /* Policy capability names */
> const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
> 	...
> 	"genfs_seclabel_symlinks",
> 	"new_polcap_name"
> };
> ```
> 
> ***security/selinux/include/policycap.h***
> 
> Add new entry at end of this list:
> 
> ```
> /* Policy capabilities */
> enum {
> 	...
> 	POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS,
> 	POLICYDB_CAPABILITY_NEW_POLCAP_NAME,
> 	__POLICYDB_CAPABILITY_MAX
> };
> ```
> 
> ***security/selinux/include/security.h***
> 
> Add a new entry that will initialise the new capability:
> 
> ```
> static inline bool selinux_policycap_new_name(void)
> {
> 	struct selinux_state *state = &selinux_state;
> 
> 	return READ_ONCE(state->policycap[POLICYDB_CAPABILITY_NEW_POLCAP_NAME]);
> }
> ```
> 
> Finally in the updated code that utilises the new policy capabilty do
> something like this:
> 
> ```
> if (selinux_policycap_new_name())
> 	do this;
> else
> 	do that;
> ```
> 
> ## *libsepol* Library Updates
> 
> In selinux userspace source update the following two files with the new
> capability:
> 
> ***selinux/libsepol/src/polcaps.c***
> 
> Add new entry at end of this list:
> 
> ```
> static const char * const polcap_names[] = {
> 	...
> 	"genfs_seclabel_symlinks",	/* POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS */
> 	"new_polcap_name",		/* POLICYDB_CAPABILITY_NEW_POLCAP_NAME */
> 	NULL
> };
> ```
> 
> ***selinux/libsepol/include/sepol/policydb/polcaps.h***
> 
> Add new entry at end of this list:
> 
> ```
> /* Policy capabilities */
> enum {
> 	...
> 	POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS,
> 	POLICYDB_CAPABILITY_NEW_POLCAP_NAME,
> 	__POLICYDB_CAPABILITY_MAX
> };
> ```
> 
> ## Reference Policy Updates
> 
> The new policy capability entry is then added to the Reference Policy
> file:
> 
> ***policy/policy_capabilities***
> 
> An example entry that enables the capability in policy is:
> 
> ```
> # A description of the capability
> policycap new_polcap_name;
> ```
> To disable the capability in policy comment out the entry:
> 
> ```
> # A description of the capability
> #policycap new_polcap_name;
> ```

This is going to be a much, MUCH larger patch, and it will be quite a
while before I have the spare time to write it.  I would be fine with
someone else writing it, though.

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
OpenPGP_0xB288B55FFF9C22C1.asc (application/pgp-keys, 4.8 KB)
OpenPGP_signature (application/pgp-signature, 833 B) - not displayed