Message-ID: <847acf98ac223ccb3bc34b3d38c1389c12ca27d8.camel@btinternet.com>
Date:   Fri, 18 Feb 2022 15:06:20 +0000
From:   Richard Haines <richard_c_haines@...nternet.com>
To:     Demi Marie Obenour <demiobenour@...il.com>,
        Paul Moore <paul@...l-moore.com>
Cc:     William Roberts <bill.c.roberts@...il.com>,
        Dominick Grift <dominick.grift@...ensec.nl>,
        Chris PeBenito <chpebeni@...ux.microsoft.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Eric Paris <eparis@...isplace.org>,
        SElinux list <selinux@...r.kernel.org>,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>,
        selinux-refpolicy@...r.kernel.org,
        Jeffrey Vander Stoep <jeffv@...gle.com>
Subject: Re: [PATCH] SELinux: Always allow FIOCLEX and FIONCLEX

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@...gle.com> wrote:
> > > On Tue, Feb 8, 2022 at 3:18 PM William Roberts
> > > <bill.c.roberts@...il.com> 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.

This should help:

# Adding A New Policy Capability

- [Kernel Updates](#kernel-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;
```

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

Powered by blists - more mailing lists