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 >>> <jeffv@xxxxxxxxxx> wrote: >>>> On Tue, Feb 8, 2022 at 3:18 PM William Roberts >>>> <bill.c.roberts@xxxxxxxxx> 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)
Attachment:
OpenPGP_0xB288B55FFF9C22C1.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
