From: Demi Marie Obenour <demiobenour@gmail.com>
To: Richard Haines <richard_c_haines@btinternet.com>,
Paul Moore <paul@paul-moore.com>
Cc: William Roberts <bill.c.roberts@gmail.com>,
Dominick Grift <dominick.grift@defensec.nl>,
Chris PeBenito <chpebeni@linux.microsoft.com>,
Stephen Smalley <stephen.smalley.work@gmail.com>,
Eric Paris <eparis@parisplace.org>,
SElinux list <selinux@vger.kernel.org>,
Linux kernel mailing list <linux-kernel@vger.kernel.org>,
selinux-refpolicy@vger.kernel.org,
Jeffrey Vander Stoep <jeffv@google.com>
Subject: Re: [PATCH] SELinux: Always allow FIOCLEX and FIONCLEX
Date: Sat, 19 Feb 2022 20:15:59 -0500 [thread overview]
Message-ID: <6d6378bf-7ac7-108c-a896-14c81832c364@gmail.com> (raw)
In-Reply-To: <141da74b176cd3bae74a8a81226c661c032631dc.camel@btinternet.com>
[-- Attachment #1.1.1: Type: text/plain, Size: 7527 bytes --]
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@google.com> wrote:
>>>> On Tue, Feb 8, 2022 at 3:18 PM William Roberts
>>>> <bill.c.roberts@gmail.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.
>
> 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 #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 4963 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2022-02-20 1:16 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <4df50e95-6173-4ed1-9d08-3c1c4abab23f@gmail.com>
[not found] ` <CAHC9VhSjTqT-4TMxBnQOQHkj+djONihfeoPVyy1egrZY2t10XA@mail.gmail.com>
[not found] ` <c8a616e4-26a6-af51-212c-31dca0e265cd@gmail.com>
2022-01-26 22:41 ` [PATCH] SELinux: Always allow FIOCLEX and FIONCLEX Paul Moore
2022-01-30 3:40 ` Demi Marie Obenour
2022-02-01 17:26 ` Paul Moore
2022-02-02 10:13 ` Demi Marie Obenour
2022-02-03 23:44 ` Paul Moore
2022-02-04 13:48 ` Chris PeBenito
2022-02-05 11:19 ` Dominick Grift
2022-02-05 13:13 ` Demi Marie Obenour
2022-02-08 14:17 ` William Roberts
2022-02-08 15:47 ` Chris PeBenito
2022-02-08 16:47 ` Dominick Grift
2022-02-08 23:44 ` David Laight
2022-02-14 7:11 ` Jeffrey Vander Stoep
2022-02-15 20:34 ` Paul Moore
2022-02-17 15:04 ` Christian Göttsche
2022-02-17 22:25 ` Paul Moore
2022-02-17 23:55 ` Demi Marie Obenour
2022-02-18 15:06 ` Richard Haines
2022-02-18 15:39 ` Richard Haines
2022-02-20 1:15 ` Demi Marie Obenour [this message]
2022-02-07 17:00 ` William Roberts
2022-02-07 17:08 ` Demi Marie Obenour
2022-02-07 18:35 ` William Roberts
2022-02-07 21:12 ` Demi Marie Obenour
2022-02-07 21:42 ` William Roberts
2022-02-07 21:50 ` William Roberts
2022-02-08 0:01 ` Paul Moore
2022-02-08 14:05 ` William Roberts
2022-02-08 16:26 ` Paul Moore
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6d6378bf-7ac7-108c-a896-14c81832c364@gmail.com \
--to=demiobenour@gmail.com \
--cc=bill.c.roberts@gmail.com \
--cc=chpebeni@linux.microsoft.com \
--cc=dominick.grift@defensec.nl \
--cc=eparis@parisplace.org \
--cc=jeffv@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=richard_c_haines@btinternet.com \
--cc=selinux-refpolicy@vger.kernel.org \
--cc=selinux@vger.kernel.org \
--cc=stephen.smalley.work@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).