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; ```
