SELinux: Always allow FIOCLEX and FIONCLEX

Commit Message

Demi Marie Obenour Jan. 25, 2022, 9:34 p.m. UTC

These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
always allows too.  Furthermore, a failed FIOCLEX could result in a file
descriptor being leaked to a process that should not have access to it.

Signed-off-by: Demi Marie Obenour <[e-mail]>
---
 security/selinux/hooks.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Paul Moore Jan. 25, 2022, 10:27 p.m. UTC | #1

On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
<[e-mail]> wrote:
>
> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
> always allows too.  Furthermore, a failed FIOCLEX could result in a file
> descriptor being leaked to a process that should not have access to it.
>
> Signed-off-by: Demi Marie Obenour <[e-mail]>
> ---
>  security/selinux/hooks.c | 5 +++++
>  1 file changed, 5 insertions(+)

I'm not convinced that these two ioctls should be exempt from SELinux
policy control, can you explain why allowing these ioctls with the
file:ioctl permission is not sufficient for your use case?  Is it a
matter of granularity?

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5b6895e4fc29..8f3b2f15c1f3 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3728,6 +3728,11 @@ static int selinux_file_ioctl(struct file *file, unsigned int cmd,
>                 error = file_has_perm(cred, file, FILE__GETATTR);
>                 break;
>
> +       /* must always succeed */
> +       case FIOCLEX:
> +       case FIONCLEX:
> +               break;
> +
>         case FS_IOC_SETFLAGS:
>         case FS_IOC_SETVERSION:
>                 error = file_has_perm(cred, file, FILE__SETATTR);

--
paul-moore.com

Demi Marie Obenour Jan. 25, 2022, 10:50 p.m. UTC | #2

On 1/25/22 17:27, Paul Moore wrote:
> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
> <[e-mail]> wrote:
>>
>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
>> descriptor being leaked to a process that should not have access to it.
>>
>> Signed-off-by: Demi Marie Obenour <[e-mail]>
>> ---
>>  security/selinux/hooks.c | 5 +++++
>>  1 file changed, 5 insertions(+)
> 
> I'm not convinced that these two ioctls should be exempt from SELinux
> policy control, can you explain why allowing these ioctls with the
> file:ioctl permission is not sufficient for your use case?  Is it a
> matter of granularity?

FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
files.  If I want to allow them with SELinux policy, I have to grant
*:ioctl to all processes and use xperm rules to determine what ioctls
are actually allowed.  That is incompatible with existing policies and
needs frequent maintenance when new ioctls are added.

Furthermore, these ioctls do not allow one to do anything that cannot
already be done by fcntl(F_SETFD), and (unless I have missed something)
SELinux unconditionally allows that.  Therefore, blocking these ioctls
does not improve security, but does risk breaking userspace programs.
The risk is especially great because in the absence of SELinux, I
believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
a file descriptor can be leaked to a child process that should not have
access to it, but which SELinux allows access to.  Userspace
SELinux-naive sandboxes are one way this could happen.  Therefore,
blocking FIOCLEX may *create* a security issue, and it cannot solve one.

Paul Moore Jan. 26, 2022, 10:41 p.m. UTC | #3

On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
<[e-mail]> wrote:
> On 1/25/22 17:27, Paul Moore wrote:
> > On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
> > <[e-mail]> wrote:
> >>
> >> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
> >> always allows too.  Furthermore, a failed FIOCLEX could result in a file
> >> descriptor being leaked to a process that should not have access to it.
> >>
> >> Signed-off-by: Demi Marie Obenour <[e-mail]>
> >> ---
> >>  security/selinux/hooks.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >
> > I'm not convinced that these two ioctls should be exempt from SELinux
> > policy control, can you explain why allowing these ioctls with the
> > file:ioctl permission is not sufficient for your use case?  Is it a
> > matter of granularity?
>
> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
> files.  If I want to allow them with SELinux policy, I have to grant
> *:ioctl to all processes and use xperm rules to determine what ioctls
> are actually allowed.  That is incompatible with existing policies and
> needs frequent maintenance when new ioctls are added.
>
> Furthermore, these ioctls do not allow one to do anything that cannot
> already be done by fcntl(F_SETFD), and (unless I have missed something)
> SELinux unconditionally allows that.  Therefore, blocking these ioctls
> does not improve security, but does risk breaking userspace programs.
> The risk is especially great because in the absence of SELinux, I
> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
> a file descriptor can be leaked to a child process that should not have
> access to it, but which SELinux allows access to.  Userspace
> SELinux-naive sandboxes are one way this could happen.  Therefore,
> blocking FIOCLEX may *create* a security issue, and it cannot solve one.

I can see you are frustrated with my initial take on this, but please
understand that excluding an operation from the security policy is not
something to take lightly and needs discussion.  I've added the
SELinux refpolicy list to this thread as I believe their input would
be helpful here.

--
paul-moore.com

Demi Marie Obenour Jan. 30, 2022, 3:40 a.m. UTC | #4

On 1/26/22 17:41, Paul Moore wrote:
> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
> <[e-mail]> wrote:
>> On 1/25/22 17:27, Paul Moore wrote:
>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
>>> <[e-mail]> wrote:
>>>>
>>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
>>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
>>>> descriptor being leaked to a process that should not have access to it.
>>>>
>>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
>>>> ---
>>>>  security/selinux/hooks.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>
>>> I'm not convinced that these two ioctls should be exempt from SELinux
>>> policy control, can you explain why allowing these ioctls with the
>>> file:ioctl permission is not sufficient for your use case?  Is it a
>>> matter of granularity?
>>
>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
>> files.  If I want to allow them with SELinux policy, I have to grant
>> *:ioctl to all processes and use xperm rules to determine what ioctls
>> are actually allowed.  That is incompatible with existing policies and
>> needs frequent maintenance when new ioctls are added.
>>
>> Furthermore, these ioctls do not allow one to do anything that cannot
>> already be done by fcntl(F_SETFD), and (unless I have missed something)
>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
>> does not improve security, but does risk breaking userspace programs.
>> The risk is especially great because in the absence of SELinux, I
>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
>> a file descriptor can be leaked to a child process that should not have
>> access to it, but which SELinux allows access to.  Userspace
>> SELinux-naive sandboxes are one way this could happen.  Therefore,
>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
> 
> I can see you are frustrated with my initial take on this, but please
> understand that excluding an operation from the security policy is not
> something to take lightly and needs discussion.  I've added the
> SELinux refpolicy list to this thread as I believe their input would
> be helpful here.

Absolutely it is not something that should be taken lightly, though I
strongly believe it is correct in this case.  Is one of my assumptions
mistaken?

Paul Moore Feb. 1, 2022, 5:26 p.m. UTC | #5

On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
<[e-mail]> wrote:
> On 1/26/22 17:41, Paul Moore wrote:
> > On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
> > <[e-mail]> wrote:
> >> On 1/25/22 17:27, Paul Moore wrote:
> >>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
> >>> <[e-mail]> wrote:
> >>>>
> >>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
> >>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
> >>>> descriptor being leaked to a process that should not have access to it.
> >>>>
> >>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
> >>>> ---
> >>>>  security/selinux/hooks.c | 5 +++++
> >>>>  1 file changed, 5 insertions(+)
> >>>
> >>> I'm not convinced that these two ioctls should be exempt from SELinux
> >>> policy control, can you explain why allowing these ioctls with the
> >>> file:ioctl permission is not sufficient for your use case?  Is it a
> >>> matter of granularity?
> >>
> >> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
> >> files.  If I want to allow them with SELinux policy, I have to grant
> >> *:ioctl to all processes and use xperm rules to determine what ioctls
> >> are actually allowed.  That is incompatible with existing policies and
> >> needs frequent maintenance when new ioctls are added.
> >>
> >> Furthermore, these ioctls do not allow one to do anything that cannot
> >> already be done by fcntl(F_SETFD), and (unless I have missed something)
> >> SELinux unconditionally allows that.  Therefore, blocking these ioctls
> >> does not improve security, but does risk breaking userspace programs.
> >> The risk is especially great because in the absence of SELinux, I
> >> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
> >> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
> >> a file descriptor can be leaked to a child process that should not have
> >> access to it, but which SELinux allows access to.  Userspace
> >> SELinux-naive sandboxes are one way this could happen.  Therefore,
> >> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
> >
> > I can see you are frustrated with my initial take on this, but please
> > understand that excluding an operation from the security policy is not
> > something to take lightly and needs discussion.  I've added the
> > SELinux refpolicy list to this thread as I believe their input would
> > be helpful here.
>
> Absolutely it is not something that should be taken lightly, though I
> strongly believe it is correct in this case.  Is one of my assumptions
> mistaken?

My concern is that there is a distro/admin somewhere which is relying
on their SELinux policy enforcing access controls on these ioctls and
removing these controls would cause them a regression.

Demi Marie Obenour Feb. 2, 2022, 10:13 a.m. UTC | #6

On 2/1/22 12:26, Paul Moore wrote:
> On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
> <[e-mail]> wrote:
>> On 1/26/22 17:41, Paul Moore wrote:
>>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
>>> <[e-mail]> wrote:
>>>> On 1/25/22 17:27, Paul Moore wrote:
>>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
>>>>> <[e-mail]> wrote:
>>>>>>
>>>>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
>>>>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
>>>>>> descriptor being leaked to a process that should not have access to it.
>>>>>>
>>>>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
>>>>>> ---
>>>>>>  security/selinux/hooks.c | 5 +++++
>>>>>>  1 file changed, 5 insertions(+)
>>>>>
>>>>> I'm not convinced that these two ioctls should be exempt from SELinux
>>>>> policy control, can you explain why allowing these ioctls with the
>>>>> file:ioctl permission is not sufficient for your use case?  Is it a
>>>>> matter of granularity?
>>>>
>>>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
>>>> files.  If I want to allow them with SELinux policy, I have to grant
>>>> *:ioctl to all processes and use xperm rules to determine what ioctls
>>>> are actually allowed.  That is incompatible with existing policies and
>>>> needs frequent maintenance when new ioctls are added.
>>>>
>>>> Furthermore, these ioctls do not allow one to do anything that cannot
>>>> already be done by fcntl(F_SETFD), and (unless I have missed something)
>>>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
>>>> does not improve security, but does risk breaking userspace programs.
>>>> The risk is especially great because in the absence of SELinux, I
>>>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
>>>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
>>>> a file descriptor can be leaked to a child process that should not have
>>>> access to it, but which SELinux allows access to.  Userspace
>>>> SELinux-naive sandboxes are one way this could happen.  Therefore,
>>>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
>>>
>>> I can see you are frustrated with my initial take on this, but please
>>> understand that excluding an operation from the security policy is not
>>> something to take lightly and needs discussion.  I've added the
>>> SELinux refpolicy list to this thread as I believe their input would
>>> be helpful here.
>>
>> Absolutely it is not something that should be taken lightly, though I
>> strongly believe it is correct in this case.  Is one of my assumptions
>> mistaken?
> 
> My concern is that there is a distro/admin somewhere which is relying
> on their SELinux policy enforcing access controls on these ioctls and
> removing these controls would cause them a regression.

I obviously do not have visibility into all systems, but I suspect that
nobody is actually relying on this.  Setting and clearing CLOEXEC via
fcntl is not subject to SELinux restrictions, so blocking FIOCLEX
and FIONCLEX can be trivially bypassed unless fcntl(F_SETFD) is
blocked by seccomp or another LSM.  Clearing close-on-exec can also be
implemented with dup2(), and setting it can be implemented with dup3()
and F_DUPFD_CLOEXEC (which SELinux also allows).  In short, I believe
that unconditionally allowing FIOCLEX and FIONCLEX may fix real-world
problems, and that it is highly unlikely that anyone is relying on the
current behavior.

Paul Moore Feb. 3, 2022, 11:44 p.m. UTC | #7

On Wed, Feb 2, 2022 at 5:13 AM Demi Marie Obenour <[e-mail]> wrote:
> On 2/1/22 12:26, Paul Moore wrote:
> > On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
> > <[e-mail]> wrote:
> >> On 1/26/22 17:41, Paul Moore wrote:
> >>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
> >>> <[e-mail]> wrote:
> >>>> On 1/25/22 17:27, Paul Moore wrote:
> >>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
> >>>>> <[e-mail]> wrote:
> >>>>>>
> >>>>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
> >>>>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
> >>>>>> descriptor being leaked to a process that should not have access to it.
> >>>>>>
> >>>>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
> >>>>>> ---
> >>>>>>  security/selinux/hooks.c | 5 +++++
> >>>>>>  1 file changed, 5 insertions(+)
> >>>>>
> >>>>> I'm not convinced that these two ioctls should be exempt from SELinux
> >>>>> policy control, can you explain why allowing these ioctls with the
> >>>>> file:ioctl permission is not sufficient for your use case?  Is it a
> >>>>> matter of granularity?
> >>>>
> >>>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
> >>>> files.  If I want to allow them with SELinux policy, I have to grant
> >>>> *:ioctl to all processes and use xperm rules to determine what ioctls
> >>>> are actually allowed.  That is incompatible with existing policies and
> >>>> needs frequent maintenance when new ioctls are added.
> >>>>
> >>>> Furthermore, these ioctls do not allow one to do anything that cannot
> >>>> already be done by fcntl(F_SETFD), and (unless I have missed something)
> >>>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
> >>>> does not improve security, but does risk breaking userspace programs.
> >>>> The risk is especially great because in the absence of SELinux, I
> >>>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
> >>>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
> >>>> a file descriptor can be leaked to a child process that should not have
> >>>> access to it, but which SELinux allows access to.  Userspace
> >>>> SELinux-naive sandboxes are one way this could happen.  Therefore,
> >>>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
> >>>
> >>> I can see you are frustrated with my initial take on this, but please
> >>> understand that excluding an operation from the security policy is not
> >>> something to take lightly and needs discussion.  I've added the
> >>> SELinux refpolicy list to this thread as I believe their input would
> >>> be helpful here.
> >>
> >> Absolutely it is not something that should be taken lightly, though I
> >> strongly believe it is correct in this case.  Is one of my assumptions
> >> mistaken?
> >
> > My concern is that there is a distro/admin somewhere which is relying
> > on their SELinux policy enforcing access controls on these ioctls and
> > removing these controls would cause them a regression.
>
> I obviously do not have visibility into all systems, but I suspect that
> nobody is actually relying on this.  Setting and clearing CLOEXEC via
> fcntl is not subject to SELinux restrictions, so blocking FIOCLEX
> and FIONCLEX can be trivially bypassed unless fcntl(F_SETFD) is
> blocked by seccomp or another LSM.  Clearing close-on-exec can also be
> implemented with dup2(), and setting it can be implemented with dup3()
> and F_DUPFD_CLOEXEC (which SELinux also allows).  In short, I believe
> that unconditionally allowing FIOCLEX and FIONCLEX may fix real-world
> problems, and that it is highly unlikely that anyone is relying on the
> current behavior.

I understand your point, but I remain concerned about making a kernel
change for something that can be addressed via policy.  I'm also
concerned that in the nine days this thread has been on both the mail
SELinux developers and refpolicy lists no one other than you and I
have commented on this patch.  In order to consider this patch
further, I'm going to need to see comments from others, preferably
those with a background in supporting SELinux policy.

Also, while I'm sure you are already well aware of this, I think it is
worth mentioning that SELinux does apply access controls when file
descriptors are inherited across an exec() boundary.

Chris PeBenito Feb. 4, 2022, 1:48 p.m. UTC | #8

On 2/3/2022 18:44, Paul Moore wrote:
> On Wed, Feb 2, 2022 at 5:13 AM Demi Marie Obenour <[e-mail]> wrote:
>> On 2/1/22 12:26, Paul Moore wrote:
>>> On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
>>> <[e-mail]> wrote:
>>>> On 1/26/22 17:41, Paul Moore wrote:
>>>>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
>>>>> <[e-mail]> wrote:
>>>>>> On 1/25/22 17:27, Paul Moore wrote:
>>>>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
>>>>>>> <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
>>>>>>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
>>>>>>>> descriptor being leaked to a process that should not have access to it.
>>>>>>>>
>>>>>>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
>>>>>>>> ---
>>>>>>>>   security/selinux/hooks.c | 5 +++++
>>>>>>>>   1 file changed, 5 insertions(+)
>>>>>>>
>>>>>>> I'm not convinced that these two ioctls should be exempt from SELinux
>>>>>>> policy control, can you explain why allowing these ioctls with the
>>>>>>> file:ioctl permission is not sufficient for your use case?  Is it a
>>>>>>> matter of granularity?
>>>>>>
>>>>>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
>>>>>> files.  If I want to allow them with SELinux policy, I have to grant
>>>>>> *:ioctl to all processes and use xperm rules to determine what ioctls
>>>>>> are actually allowed.  That is incompatible with existing policies and
>>>>>> needs frequent maintenance when new ioctls are added.
>>>>>>
>>>>>> Furthermore, these ioctls do not allow one to do anything that cannot
>>>>>> already be done by fcntl(F_SETFD), and (unless I have missed something)
>>>>>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
>>>>>> does not improve security, but does risk breaking userspace programs.
>>>>>> The risk is especially great because in the absence of SELinux, I
>>>>>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
>>>>>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
>>>>>> a file descriptor can be leaked to a child process that should not have
>>>>>> access to it, but which SELinux allows access to.  Userspace
>>>>>> SELinux-naive sandboxes are one way this could happen.  Therefore,
>>>>>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
>>>>>
>>>>> I can see you are frustrated with my initial take on this, but please
>>>>> understand that excluding an operation from the security policy is not
>>>>> something to take lightly and needs discussion.  I've added the
>>>>> SELinux refpolicy list to this thread as I believe their input would
>>>>> be helpful here.
>>>>
>>>> Absolutely it is not something that should be taken lightly, though I
>>>> strongly believe it is correct in this case.  Is one of my assumptions
>>>> mistaken?
>>>
>>> My concern is that there is a distro/admin somewhere which is relying
>>> on their SELinux policy enforcing access controls on these ioctls and
>>> removing these controls would cause them a regression.
>>
>> I obviously do not have visibility into all systems, but I suspect that
>> nobody is actually relying on this.  Setting and clearing CLOEXEC via
>> fcntl is not subject to SELinux restrictions, so blocking FIOCLEX
>> and FIONCLEX can be trivially bypassed unless fcntl(F_SETFD) is
>> blocked by seccomp or another LSM.  Clearing close-on-exec can also be
>> implemented with dup2(), and setting it can be implemented with dup3()
>> and F_DUPFD_CLOEXEC (which SELinux also allows).  In short, I believe
>> that unconditionally allowing FIOCLEX and FIONCLEX may fix real-world
>> problems, and that it is highly unlikely that anyone is relying on the
>> current behavior.
> 
> I understand your point, but I remain concerned about making a kernel
> change for something that can be addressed via policy.  I'm also
> concerned that in the nine days this thread has been on both the mail
> SELinux developers and refpolicy lists no one other than you and I
> have commented on this patch.  In order to consider this patch
> further, I'm going to need to see comments from others, preferably
> those with a background in supporting SELinux policy.
> 
> Also, while I'm sure you are already well aware of this, I think it is
> worth mentioning that SELinux does apply access controls when file
> descriptors are inherited across an exec() boundary.


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.


--
Chris PeBenito

Dominick Grift Feb. 5, 2022, 11:19 a.m. UTC | #9

Chris PeBenito <[e-mail]> writes:

> On 2/3/2022 18:44, Paul Moore wrote:
>> On Wed, Feb 2, 2022 at 5:13 AM Demi Marie Obenour <[e-mail]> wrote:
>>> On 2/1/22 12:26, Paul Moore wrote:
>>>> On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
>>>> <[e-mail]> wrote:
>>>>> On 1/26/22 17:41, Paul Moore wrote:
>>>>>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
>>>>>> <[e-mail]> wrote:
>>>>>>> On 1/25/22 17:27, Paul Moore wrote:
>>>>>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>
>>>>>>>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
>>>>>>>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
>>>>>>>>> descriptor being leaked to a process that should not have access to it.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>   security/selinux/hooks.c | 5 +++++
>>>>>>>>>   1 file changed, 5 insertions(+)
>>>>>>>>
>>>>>>>> I'm not convinced that these two ioctls should be exempt from SELinux
>>>>>>>> policy control, can you explain why allowing these ioctls with the
>>>>>>>> file:ioctl permission is not sufficient for your use case?  Is it a
>>>>>>>> matter of granularity?
>>>>>>>
>>>>>>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
>>>>>>> files.  If I want to allow them with SELinux policy, I have to grant
>>>>>>> *:ioctl to all processes and use xperm rules to determine what ioctls
>>>>>>> are actually allowed.  That is incompatible with existing policies and
>>>>>>> needs frequent maintenance when new ioctls are added.
>>>>>>>
>>>>>>> Furthermore, these ioctls do not allow one to do anything that cannot
>>>>>>> already be done by fcntl(F_SETFD), and (unless I have missed something)
>>>>>>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
>>>>>>> does not improve security, but does risk breaking userspace programs.
>>>>>>> The risk is especially great because in the absence of SELinux, I
>>>>>>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
>>>>>>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
>>>>>>> a file descriptor can be leaked to a child process that should not have
>>>>>>> access to it, but which SELinux allows access to.  Userspace
>>>>>>> SELinux-naive sandboxes are one way this could happen.  Therefore,
>>>>>>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
>>>>>>
>>>>>> I can see you are frustrated with my initial take on this, but please
>>>>>> understand that excluding an operation from the security policy is not
>>>>>> something to take lightly and needs discussion.  I've added the
>>>>>> SELinux refpolicy list to this thread as I believe their input would
>>>>>> be helpful here.
>>>>>
>>>>> Absolutely it is not something that should be taken lightly, though I
>>>>> strongly believe it is correct in this case.  Is one of my assumptions
>>>>> mistaken?
>>>>
>>>> My concern is that there is a distro/admin somewhere which is relying
>>>> on their SELinux policy enforcing access controls on these ioctls and
>>>> removing these controls would cause them a regression.
>>>
>>> I obviously do not have visibility into all systems, but I suspect that
>>> nobody is actually relying on this.  Setting and clearing CLOEXEC via
>>> fcntl is not subject to SELinux restrictions, so blocking FIOCLEX
>>> and FIONCLEX can be trivially bypassed unless fcntl(F_SETFD) is
>>> blocked by seccomp or another LSM.  Clearing close-on-exec can also be
>>> implemented with dup2(), and setting it can be implemented with dup3()
>>> and F_DUPFD_CLOEXEC (which SELinux also allows).  In short, I believe
>>> that unconditionally allowing FIOCLEX and FIONCLEX may fix real-world
>>> problems, and that it is highly unlikely that anyone is relying on the
>>> current behavior.
>> I understand your point, but I remain concerned about making a
>> kernel
>> change for something that can be addressed via policy.  I'm also
>> concerned that in the nine days this thread has been on both the mail
>> SELinux developers and refpolicy lists no one other than you and I
>> have commented on this patch.  In order to consider this patch
>> further, I'm going to need to see comments from others, preferably
>> those with a background in supporting SELinux policy.
>> Also, while I'm sure you are already well aware of this, I think it
>> is
>> worth mentioning that SELinux does apply access controls when file
>> descriptors are inherited across an exec() boundary.
>
>
> 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.

If i am not mistaken it will turn on xperm checks only for the
operations that have the same source and target/target class.

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)

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)

>
>
> --
> Chris PeBenito
>

Demi Marie Obenour Feb. 5, 2022, 1:13 p.m. UTC | #10

On 2/5/22 06:19, Dominick Grift wrote:
> Chris PeBenito <[e-mail]> writes:
> 
>> On 2/3/2022 18:44, Paul Moore wrote:
>>> On Wed, Feb 2, 2022 at 5:13 AM Demi Marie Obenour <[e-mail]> wrote:
>>>> On 2/1/22 12:26, Paul Moore wrote:
>>>>> On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
>>>>> <[e-mail]> wrote:
>>>>>> On 1/26/22 17:41, Paul Moore wrote:
>>>>>>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
>>>>>>> <[e-mail]> wrote:
>>>>>>>> On 1/25/22 17:27, Paul Moore wrote:
>>>>>>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
>>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>>
>>>>>>>>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
>>>>>>>>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
>>>>>>>>>> descriptor being leaked to a process that should not have access to it.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>>   security/selinux/hooks.c | 5 +++++
>>>>>>>>>>   1 file changed, 5 insertions(+)
>>>>>>>>>
>>>>>>>>> I'm not convinced that these two ioctls should be exempt from SELinux
>>>>>>>>> policy control, can you explain why allowing these ioctls with the
>>>>>>>>> file:ioctl permission is not sufficient for your use case?  Is it a
>>>>>>>>> matter of granularity?
>>>>>>>>
>>>>>>>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
>>>>>>>> files.  If I want to allow them with SELinux policy, I have to grant
>>>>>>>> *:ioctl to all processes and use xperm rules to determine what ioctls
>>>>>>>> are actually allowed.  That is incompatible with existing policies and
>>>>>>>> needs frequent maintenance when new ioctls are added.
>>>>>>>>
>>>>>>>> Furthermore, these ioctls do not allow one to do anything that cannot
>>>>>>>> already be done by fcntl(F_SETFD), and (unless I have missed something)
>>>>>>>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
>>>>>>>> does not improve security, but does risk breaking userspace programs.
>>>>>>>> The risk is especially great because in the absence of SELinux, I
>>>>>>>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
>>>>>>>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
>>>>>>>> a file descriptor can be leaked to a child process that should not have
>>>>>>>> access to it, but which SELinux allows access to.  Userspace
>>>>>>>> SELinux-naive sandboxes are one way this could happen.  Therefore,
>>>>>>>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
>>>>>>>
>>>>>>> I can see you are frustrated with my initial take on this, but please
>>>>>>> understand that excluding an operation from the security policy is not
>>>>>>> something to take lightly and needs discussion.  I've added the
>>>>>>> SELinux refpolicy list to this thread as I believe their input would
>>>>>>> be helpful here.
>>>>>>
>>>>>> Absolutely it is not something that should be taken lightly, though I
>>>>>> strongly believe it is correct in this case.  Is one of my assumptions
>>>>>> mistaken?
>>>>>
>>>>> My concern is that there is a distro/admin somewhere which is relying
>>>>> on their SELinux policy enforcing access controls on these ioctls and
>>>>> removing these controls would cause them a regression.
>>>>
>>>> I obviously do not have visibility into all systems, but I suspect that
>>>> nobody is actually relying on this.  Setting and clearing CLOEXEC via
>>>> fcntl is not subject to SELinux restrictions, so blocking FIOCLEX
>>>> and FIONCLEX can be trivially bypassed unless fcntl(F_SETFD) is
>>>> blocked by seccomp or another LSM.  Clearing close-on-exec can also be
>>>> implemented with dup2(), and setting it can be implemented with dup3()
>>>> and F_DUPFD_CLOEXEC (which SELinux also allows).  In short, I believe
>>>> that unconditionally allowing FIOCLEX and FIONCLEX may fix real-world
>>>> problems, and that it is highly unlikely that anyone is relying on the
>>>> current behavior.
>>> I understand your point, but I remain concerned about making a
>>> kernel
>>> change for something that can be addressed via policy.  I'm also
>>> concerned that in the nine days this thread has been on both the mail
>>> SELinux developers and refpolicy lists no one other than you and I
>>> have commented on this patch.  In order to consider this patch
>>> further, I'm going to need to see comments from others, preferably
>>> those with a background in supporting SELinux policy.
>>> Also, while I'm sure you are already well aware of this, I think it
>>> is
>>> worth mentioning that SELinux does apply access controls when file
>>> descriptors are inherited across an exec() boundary.
>>
>>
>> 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.
> 
> If i am not mistaken it will turn on xperm checks only for the
> operations that have the same source and target/target class.

Correct, but to emulate my patch one would need to use xperm rules
for all source and target classes.

William Roberts Feb. 7, 2022, 5 p.m. UTC | #11

On Mon, Feb 7, 2022 at 9:08 AM Paul Moore <[e-mail]> wrote:
>
> On Wed, Feb 2, 2022 at 5:13 AM Demi Marie Obenour <[e-mail]> wrote:
> > On 2/1/22 12:26, Paul Moore wrote:
> > > On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
> > > <[e-mail]> wrote:
> > >> On 1/26/22 17:41, Paul Moore wrote:
> > >>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
> > >>> <[e-mail]> wrote:
> > >>>> On 1/25/22 17:27, Paul Moore wrote:
> > >>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
> > >>>>> <[e-mail]> wrote:
> > >>>>>>
> > >>>>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
> > >>>>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
> > >>>>>> descriptor being leaked to a process that should not have access to it.
> > >>>>>>
> > >>>>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
> > >>>>>> ---
> > >>>>>>  security/selinux/hooks.c | 5 +++++
> > >>>>>>  1 file changed, 5 insertions(+)
> > >>>>>
> > >>>>> I'm not convinced that these two ioctls should be exempt from SELinux
> > >>>>> policy control, can you explain why allowing these ioctls with the
> > >>>>> file:ioctl permission is not sufficient for your use case?  Is it a
> > >>>>> matter of granularity?
> > >>>>
> > >>>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
> > >>>> files.  If I want to allow them with SELinux policy, I have to grant
> > >>>> *:ioctl to all processes and use xperm rules to determine what ioctls
> > >>>> are actually allowed.  That is incompatible with existing policies and
> > >>>> needs frequent maintenance when new ioctls are added.
> > >>>>
> > >>>> Furthermore, these ioctls do not allow one to do anything that cannot
> > >>>> already be done by fcntl(F_SETFD), and (unless I have missed something)
> > >>>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
> > >>>> does not improve security, but does risk breaking userspace programs.
> > >>>> The risk is especially great because in the absence of SELinux, I
> > >>>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
> > >>>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
> > >>>> a file descriptor can be leaked to a child process that should not have
> > >>>> access to it, but which SELinux allows access to.  Userspace
> > >>>> SELinux-naive sandboxes are one way this could happen.  Therefore,
> > >>>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
> > >>>
> > >>> I can see you are frustrated with my initial take on this, but please
> > >>> understand that excluding an operation from the security policy is not
> > >>> something to take lightly and needs discussion.  I've added the
> > >>> SELinux refpolicy list to this thread as I believe their input would
> > >>> be helpful here.
> > >>
> > >> Absolutely it is not something that should be taken lightly, though I
> > >> strongly believe it is correct in this case.  Is one of my assumptions
> > >> mistaken?
> > >
> > > My concern is that there is a distro/admin somewhere which is relying
> > > on their SELinux policy enforcing access controls on these ioctls and
> > > removing these controls would cause them a regression.
> >
> > I obviously do not have visibility into all systems, but I suspect that
> > nobody is actually relying on this.  Setting and clearing CLOEXEC via
> > fcntl is not subject to SELinux restrictions, so blocking FIOCLEX
> > and FIONCLEX can be trivially bypassed unless fcntl(F_SETFD) is
> > blocked by seccomp or another LSM.  Clearing close-on-exec can also be
> > implemented with dup2(), and setting it can be implemented with dup3()
> > and F_DUPFD_CLOEXEC (which SELinux also allows).  In short, I believe
> > that unconditionally allowing FIOCLEX and FIONCLEX may fix real-world
> > problems, and that it is highly unlikely that anyone is relying on the
> > current behavior.
>
> I understand your point, but I remain concerned about making a kernel
> change for something that can be addressed via policy.  I'm also
> concerned that in the nine days this thread has been on both the mail
> SELinux developers and refpolicy lists no one other than you and I
> have commented on this patch.  In order to consider this patch
> further, I'm going to need to see comments from others, preferably
> those with a background in supporting SELinux policy.
>

AFAIK/AFAICT Android makes no reference to F_SETFD, and tracing the code
does seem to be ignored, and the code for FIOCLEX FIONCLEX calls into
the same kernel routine set_close_on_exec().
Considering that Android's bionic contains support for "e" flag to
fopen, and it's
used in a lot of places, makes me more sure the check is skipped for F_SETFD

However, Android does make reference to FIOCLEX FIONCLEX and every
domain has it enabled:
domain.te:allowxperm domain { file_type fs_type domain dev_type }:{
dir notdevfile_class_set blk_file } ioctl { FIOCLEX FIONCLEX };
domain.te:allowxperm domain tun_device:chr_file ioctl { FIOCLEX FIONCLEX };

Refpolicy doesn't use xperm AFAICT.

I stayed quiet, I wouldn't ack on this myself, but the premise seems
correct and we
can safely drop this. Note that I didn't review the code. But we need
to ensure we handle
policy correctly and not break anything. I'm not sure what the
compilers are doing
for validation of policy macro values, but we would probably want to
mark it deprecated,
but still allow loading of old compiled policies.

> Also, while I'm sure you are already well aware of this, I think it is
> worth mentioning that SELinux does apply access controls when file
> descriptors are inherited across an exec() boundary.
>
> --
> paul-moore.com

Demi Marie Obenour Feb. 7, 2022, 5:08 p.m. UTC | #12

On 2/7/22 12:00, William Roberts wrote:
> On Mon, Feb 7, 2022 at 9:08 AM Paul Moore <[e-mail]> wrote:
>>
>> On Wed, Feb 2, 2022 at 5:13 AM Demi Marie Obenour <[e-mail]> wrote:
>>> On 2/1/22 12:26, Paul Moore wrote:
>>>> On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
>>>> <[e-mail]> wrote:
>>>>> On 1/26/22 17:41, Paul Moore wrote:
>>>>>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
>>>>>> <[e-mail]> wrote:
>>>>>>> On 1/25/22 17:27, Paul Moore wrote:
>>>>>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>
>>>>>>>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
>>>>>>>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
>>>>>>>>> descriptor being leaked to a process that should not have access to it.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>  security/selinux/hooks.c | 5 +++++
>>>>>>>>>  1 file changed, 5 insertions(+)
>>>>>>>>
>>>>>>>> I'm not convinced that these two ioctls should be exempt from SELinux
>>>>>>>> policy control, can you explain why allowing these ioctls with the
>>>>>>>> file:ioctl permission is not sufficient for your use case?  Is it a
>>>>>>>> matter of granularity?
>>>>>>>
>>>>>>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
>>>>>>> files.  If I want to allow them with SELinux policy, I have to grant
>>>>>>> *:ioctl to all processes and use xperm rules to determine what ioctls
>>>>>>> are actually allowed.  That is incompatible with existing policies and
>>>>>>> needs frequent maintenance when new ioctls are added.
>>>>>>>
>>>>>>> Furthermore, these ioctls do not allow one to do anything that cannot
>>>>>>> already be done by fcntl(F_SETFD), and (unless I have missed something)
>>>>>>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
>>>>>>> does not improve security, but does risk breaking userspace programs.
>>>>>>> The risk is especially great because in the absence of SELinux, I
>>>>>>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
>>>>>>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
>>>>>>> a file descriptor can be leaked to a child process that should not have
>>>>>>> access to it, but which SELinux allows access to.  Userspace
>>>>>>> SELinux-naive sandboxes are one way this could happen.  Therefore,
>>>>>>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
>>>>>>
>>>>>> I can see you are frustrated with my initial take on this, but please
>>>>>> understand that excluding an operation from the security policy is not
>>>>>> something to take lightly and needs discussion.  I've added the
>>>>>> SELinux refpolicy list to this thread as I believe their input would
>>>>>> be helpful here.
>>>>>
>>>>> Absolutely it is not something that should be taken lightly, though I
>>>>> strongly believe it is correct in this case.  Is one of my assumptions
>>>>> mistaken?
>>>>
>>>> My concern is that there is a distro/admin somewhere which is relying
>>>> on their SELinux policy enforcing access controls on these ioctls and
>>>> removing these controls would cause them a regression.
>>>
>>> I obviously do not have visibility into all systems, but I suspect that
>>> nobody is actually relying on this.  Setting and clearing CLOEXEC via
>>> fcntl is not subject to SELinux restrictions, so blocking FIOCLEX
>>> and FIONCLEX can be trivially bypassed unless fcntl(F_SETFD) is
>>> blocked by seccomp or another LSM.  Clearing close-on-exec can also be
>>> implemented with dup2(), and setting it can be implemented with dup3()
>>> and F_DUPFD_CLOEXEC (which SELinux also allows).  In short, I believe
>>> that unconditionally allowing FIOCLEX and FIONCLEX may fix real-world
>>> problems, and that it is highly unlikely that anyone is relying on the
>>> current behavior.
>>
>> I understand your point, but I remain concerned about making a kernel
>> change for something that can be addressed via policy.  I'm also
>> concerned that in the nine days this thread has been on both the mail
>> SELinux developers and refpolicy lists no one other than you and I
>> have commented on this patch.  In order to consider this patch
>> further, I'm going to need to see comments from others, preferably
>> those with a background in supporting SELinux policy.
>>
> 
> AFAIK/AFAICT Android makes no reference to F_SETFD, and tracing the code
> does seem to be ignored, and the code for FIOCLEX FIONCLEX calls into
> the same kernel routine set_close_on_exec().
> Considering that Android's bionic contains support for "e" flag to
> fopen, and it's
> used in a lot of places, makes me more sure the check is skipped for F_SETFD
> 
> However, Android does make reference to FIOCLEX FIONCLEX and every
> domain has it enabled:
> domain.te:allowxperm domain { file_type fs_type domain dev_type }:{
> dir notdevfile_class_set blk_file } ioctl { FIOCLEX FIONCLEX };
> domain.te:allowxperm domain tun_device:chr_file ioctl { FIOCLEX FIONCLEX };
> 
> Refpolicy doesn't use xperm AFAICT.
> 
> I stayed quiet, I wouldn't ack on this myself, but the premise seems
> correct and we
> can safely drop this. Note that I didn't review the code. But we need
> to ensure we handle
> policy correctly and not break anything. I'm not sure what the
> compilers are doing
> for validation of policy macro values, but we would probably want to
> mark it deprecated,
> but still allow loading of old compiled policies.

Loading of policies is not impacted.  My patch simply skips the
checks for FIOCLEX and FIONCLEX, instead unconditionally allowing the
operation.  This is actually *more* selective than anything that can
be done via policy, as my patch checks the entire ioctl number whereas
policy can only check the low 16 bits.  As such, it is safer than using
policy to allow FIOCLEX and FIONCLEX system-wide: if my patch causes an
ioctl to be allowed, it is guaranteed that that ioctl will change the
close-on-exec flag and have no other effect.

William Roberts Feb. 7, 2022, 6:35 p.m. UTC | #13

On Mon, Feb 7, 2022 at 11:09 AM Demi Marie Obenour
<[e-mail]> wrote:
>
> On 2/7/22 12:00, William Roberts wrote:
> > On Mon, Feb 7, 2022 at 9:08 AM Paul Moore <[e-mail]> wrote:
> >>
> >> On Wed, Feb 2, 2022 at 5:13 AM Demi Marie Obenour <[e-mail]> wrote:
> >>> On 2/1/22 12:26, Paul Moore wrote:
> >>>> On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
> >>>> <[e-mail]> wrote:
> >>>>> On 1/26/22 17:41, Paul Moore wrote:
> >>>>>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
> >>>>>> <[e-mail]> wrote:
> >>>>>>> On 1/25/22 17:27, Paul Moore wrote:
> >>>>>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
> >>>>>>>> <[e-mail]> wrote:
> >>>>>>>>>
> >>>>>>>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
> >>>>>>>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
> >>>>>>>>> descriptor being leaked to a process that should not have access to it.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
> >>>>>>>>> ---
> >>>>>>>>>  security/selinux/hooks.c | 5 +++++
> >>>>>>>>>  1 file changed, 5 insertions(+)
> >>>>>>>>
> >>>>>>>> I'm not convinced that these two ioctls should be exempt from SELinux
> >>>>>>>> policy control, can you explain why allowing these ioctls with the
> >>>>>>>> file:ioctl permission is not sufficient for your use case?  Is it a
> >>>>>>>> matter of granularity?
> >>>>>>>
> >>>>>>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
> >>>>>>> files.  If I want to allow them with SELinux policy, I have to grant
> >>>>>>> *:ioctl to all processes and use xperm rules to determine what ioctls
> >>>>>>> are actually allowed.  That is incompatible with existing policies and
> >>>>>>> needs frequent maintenance when new ioctls are added.
> >>>>>>>
> >>>>>>> Furthermore, these ioctls do not allow one to do anything that cannot
> >>>>>>> already be done by fcntl(F_SETFD), and (unless I have missed something)
> >>>>>>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
> >>>>>>> does not improve security, but does risk breaking userspace programs.
> >>>>>>> The risk is especially great because in the absence of SELinux, I
> >>>>>>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
> >>>>>>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
> >>>>>>> a file descriptor can be leaked to a child process that should not have
> >>>>>>> access to it, but which SELinux allows access to.  Userspace
> >>>>>>> SELinux-naive sandboxes are one way this could happen.  Therefore,
> >>>>>>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
> >>>>>>
> >>>>>> I can see you are frustrated with my initial take on this, but please
> >>>>>> understand that excluding an operation from the security policy is not
> >>>>>> something to take lightly and needs discussion.  I've added the
> >>>>>> SELinux refpolicy list to this thread as I believe their input would
> >>>>>> be helpful here.
> >>>>>
> >>>>> Absolutely it is not something that should be taken lightly, though I
> >>>>> strongly believe it is correct in this case.  Is one of my assumptions
> >>>>> mistaken?
> >>>>
> >>>> My concern is that there is a distro/admin somewhere which is relying
> >>>> on their SELinux policy enforcing access controls on these ioctls and
> >>>> removing these controls would cause them a regression.
> >>>
> >>> I obviously do not have visibility into all systems, but I suspect that
> >>> nobody is actually relying on this.  Setting and clearing CLOEXEC via
> >>> fcntl is not subject to SELinux restrictions, so blocking FIOCLEX
> >>> and FIONCLEX can be trivially bypassed unless fcntl(F_SETFD) is
> >>> blocked by seccomp or another LSM.  Clearing close-on-exec can also be
> >>> implemented with dup2(), and setting it can be implemented with dup3()
> >>> and F_DUPFD_CLOEXEC (which SELinux also allows).  In short, I believe
> >>> that unconditionally allowing FIOCLEX and FIONCLEX may fix real-world
> >>> problems, and that it is highly unlikely that anyone is relying on the
> >>> current behavior.
> >>
> >> I understand your point, but I remain concerned about making a kernel
> >> change for something that can be addressed via policy.  I'm also
> >> concerned that in the nine days this thread has been on both the mail
> >> SELinux developers and refpolicy lists no one other than you and I
> >> have commented on this patch.  In order to consider this patch
> >> further, I'm going to need to see comments from others, preferably
> >> those with a background in supporting SELinux policy.
> >>
> >
> > AFAIK/AFAICT Android makes no reference to F_SETFD, and tracing the code
> > does seem to be ignored, and the code for FIOCLEX FIONCLEX calls into
> > the same kernel routine set_close_on_exec().
> > Considering that Android's bionic contains support for "e" flag to
> > fopen, and it's
> > used in a lot of places, makes me more sure the check is skipped for F_SETFD
> >
> > However, Android does make reference to FIOCLEX FIONCLEX and every
> > domain has it enabled:
> > domain.te:allowxperm domain { file_type fs_type domain dev_type }:{
> > dir notdevfile_class_set blk_file } ioctl { FIOCLEX FIONCLEX };
> > domain.te:allowxperm domain tun_device:chr_file ioctl { FIOCLEX FIONCLEX };
> >
> > Refpolicy doesn't use xperm AFAICT.
> >
> > I stayed quiet, I wouldn't ack on this myself, but the premise seems
> > correct and we
> > can safely drop this. Note that I didn't review the code. But we need
> > to ensure we handle
> > policy correctly and not break anything. I'm not sure what the
> > compilers are doing
> > for validation of policy macro values, but we would probably want to
> > mark it deprecated,
> > but still allow loading of old compiled policies.
>
> Loading of policies is not impacted.  My patch simply skips the
> checks for FIOCLEX and FIONCLEX, instead unconditionally allowing the
> operation.  This is actually *more* selective than anything that can
> be done via policy, as my patch checks the entire ioctl number whereas
> policy can only check the low 16 bits.  As such, it is safer than using
> policy to allow FIOCLEX and FIONCLEX system-wide: if my patch causes an
> ioctl to be allowed, it is guaranteed that that ioctl will change the
> close-on-exec flag and have no other effect.
>

What I meant by my comment is that patching the kernel is only 1/2 the
problem. We
still need to coordinate with existing policies to deprecate that out,
but since it's just
Android (AFIAK), that's pretty simple to do. I just want to make sure
we don't leave
confusing cruft floating around. I looked more at how they do xperms
in Android, and it's just
an m4 macro to a number. So we would want to coordinate a patch into the kernel
with a patch that drops that from Android policy.

It's part of their "public api" for device makers, but we can bump the
API version
and drop that IIUC their changes from "project treble".

> --
> Sincerely,
> Demi Marie Obenour (she/her/hers)

Demi Marie Obenour Feb. 7, 2022, 9:12 p.m. UTC | #14

On 2/7/22 13:35, William Roberts wrote:
> On Mon, Feb 7, 2022 at 11:09 AM Demi Marie Obenour
> <[e-mail]> wrote:
>>
>> On 2/7/22 12:00, William Roberts wrote:
>>> On Mon, Feb 7, 2022 at 9:08 AM Paul Moore <[e-mail]> wrote:
>>>>
>>>> On Wed, Feb 2, 2022 at 5:13 AM Demi Marie Obenour <[e-mail]> wrote:
>>>>> On 2/1/22 12:26, Paul Moore wrote:
>>>>>> On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
>>>>>> <[e-mail]> wrote:
>>>>>>> On 1/26/22 17:41, Paul Moore wrote:
>>>>>>>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
>>>>>>>> <[e-mail]> wrote:
>>>>>>>>> On 1/25/22 17:27, Paul Moore wrote:
>>>>>>>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
>>>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
>>>>>>>>>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
>>>>>>>>>>> descriptor being leaked to a process that should not have access to it.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
>>>>>>>>>>> ---
>>>>>>>>>>>  security/selinux/hooks.c | 5 +++++
>>>>>>>>>>>  1 file changed, 5 insertions(+)
>>>>>>>>>>
>>>>>>>>>> I'm not convinced that these two ioctls should be exempt from SELinux
>>>>>>>>>> policy control, can you explain why allowing these ioctls with the
>>>>>>>>>> file:ioctl permission is not sufficient for your use case?  Is it a
>>>>>>>>>> matter of granularity?
>>>>>>>>>
>>>>>>>>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
>>>>>>>>> files.  If I want to allow them with SELinux policy, I have to grant
>>>>>>>>> *:ioctl to all processes and use xperm rules to determine what ioctls
>>>>>>>>> are actually allowed.  That is incompatible with existing policies and
>>>>>>>>> needs frequent maintenance when new ioctls are added.
>>>>>>>>>
>>>>>>>>> Furthermore, these ioctls do not allow one to do anything that cannot
>>>>>>>>> already be done by fcntl(F_SETFD), and (unless I have missed something)
>>>>>>>>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
>>>>>>>>> does not improve security, but does risk breaking userspace programs.
>>>>>>>>> The risk is especially great because in the absence of SELinux, I
>>>>>>>>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
>>>>>>>>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
>>>>>>>>> a file descriptor can be leaked to a child process that should not have
>>>>>>>>> access to it, but which SELinux allows access to.  Userspace
>>>>>>>>> SELinux-naive sandboxes are one way this could happen.  Therefore,
>>>>>>>>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
>>>>>>>>
>>>>>>>> I can see you are frustrated with my initial take on this, but please
>>>>>>>> understand that excluding an operation from the security policy is not
>>>>>>>> something to take lightly and needs discussion.  I've added the
>>>>>>>> SELinux refpolicy list to this thread as I believe their input would
>>>>>>>> be helpful here.
>>>>>>>
>>>>>>> Absolutely it is not something that should be taken lightly, though I
>>>>>>> strongly believe it is correct in this case.  Is one of my assumptions
>>>>>>> mistaken?
>>>>>>
>>>>>> My concern is that there is a distro/admin somewhere which is relying
>>>>>> on their SELinux policy enforcing access controls on these ioctls and
>>>>>> removing these controls would cause them a regression.
>>>>>
>>>>> I obviously do not have visibility into all systems, but I suspect that
>>>>> nobody is actually relying on this.  Setting and clearing CLOEXEC via
>>>>> fcntl is not subject to SELinux restrictions, so blocking FIOCLEX
>>>>> and FIONCLEX can be trivially bypassed unless fcntl(F_SETFD) is
>>>>> blocked by seccomp or another LSM.  Clearing close-on-exec can also be
>>>>> implemented with dup2(), and setting it can be implemented with dup3()
>>>>> and F_DUPFD_CLOEXEC (which SELinux also allows).  In short, I believe
>>>>> that unconditionally allowing FIOCLEX and FIONCLEX may fix real-world
>>>>> problems, and that it is highly unlikely that anyone is relying on the
>>>>> current behavior.
>>>>
>>>> I understand your point, but I remain concerned about making a kernel
>>>> change for something that can be addressed via policy.  I'm also
>>>> concerned that in the nine days this thread has been on both the mail
>>>> SELinux developers and refpolicy lists no one other than you and I
>>>> have commented on this patch.  In order to consider this patch
>>>> further, I'm going to need to see comments from others, preferably
>>>> those with a background in supporting SELinux policy.
>>>>
>>>
>>> AFAIK/AFAICT Android makes no reference to F_SETFD, and tracing the code
>>> does seem to be ignored, and the code for FIOCLEX FIONCLEX calls into
>>> the same kernel routine set_close_on_exec().
>>> Considering that Android's bionic contains support for "e" flag to
>>> fopen, and it's
>>> used in a lot of places, makes me more sure the check is skipped for F_SETFD
>>>
>>> However, Android does make reference to FIOCLEX FIONCLEX and every
>>> domain has it enabled:
>>> domain.te:allowxperm domain { file_type fs_type domain dev_type }:{
>>> dir notdevfile_class_set blk_file } ioctl { FIOCLEX FIONCLEX };
>>> domain.te:allowxperm domain tun_device:chr_file ioctl { FIOCLEX FIONCLEX };
>>>
>>> Refpolicy doesn't use xperm AFAICT.
>>>
>>> I stayed quiet, I wouldn't ack on this myself, but the premise seems
>>> correct and we
>>> can safely drop this. Note that I didn't review the code. But we need
>>> to ensure we handle
>>> policy correctly and not break anything. I'm not sure what the
>>> compilers are doing
>>> for validation of policy macro values, but we would probably want to
>>> mark it deprecated,
>>> but still allow loading of old compiled policies.
>>
>> Loading of policies is not impacted.  My patch simply skips the
>> checks for FIOCLEX and FIONCLEX, instead unconditionally allowing the
>> operation.  This is actually *more* selective than anything that can
>> be done via policy, as my patch checks the entire ioctl number whereas
>> policy can only check the low 16 bits.  As such, it is safer than using
>> policy to allow FIOCLEX and FIONCLEX system-wide: if my patch causes an
>> ioctl to be allowed, it is guaranteed that that ioctl will change the
>> close-on-exec flag and have no other effect.
>>
> 
> What I meant by my comment is that patching the kernel is only 1/2 the
> problem. We
> still need to coordinate with existing policies to deprecate that out,
> but since it's just
> Android (AFIAK), that's pretty simple to do. I just want to make sure
> we don't leave
> confusing cruft floating around. I looked more at how they do xperms
> in Android, and it's just
> an m4 macro to a number. So we would want to coordinate a patch into the kernel
> with a patch that drops that from Android policy.

The kernel patch needs to come first, but there is no urgency at all
for the Android policy patch.  The existing Android policy will work
fine with a patched kernel.  Removing the allowxperms for FIOCLEX and
FIONCLEX will require ensuring that doing so does not make some domains
not subject to xperm rules, and therefore allow ioctls that would
previously have been forbidden.

William Roberts Feb. 7, 2022, 9:42 p.m. UTC | #15

+ NNK and Dan

On Mon, Feb 7, 2022 at 3:12 PM Demi Marie Obenour <[e-mail]> wrote:
>
> On 2/7/22 13:35, William Roberts wrote:
> > On Mon, Feb 7, 2022 at 11:09 AM Demi Marie Obenour
> > <[e-mail]> wrote:
> >>
> >> On 2/7/22 12:00, William Roberts wrote:
> >>> On Mon, Feb 7, 2022 at 9:08 AM Paul Moore <[e-mail]> wrote:
> >>>>
> >>>> On Wed, Feb 2, 2022 at 5:13 AM Demi Marie Obenour <[e-mail]> wrote:
> >>>>> On 2/1/22 12:26, Paul Moore wrote:
> >>>>>> On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
> >>>>>> <[e-mail]> wrote:
> >>>>>>> On 1/26/22 17:41, Paul Moore wrote:
> >>>>>>>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
> >>>>>>>> <[e-mail]> wrote:
> >>>>>>>>> On 1/25/22 17:27, Paul Moore wrote:
> >>>>>>>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
> >>>>>>>>>> <[e-mail]> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
> >>>>>>>>>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
> >>>>>>>>>>> descriptor being leaked to a process that should not have access to it.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
> >>>>>>>>>>> ---
> >>>>>>>>>>>  security/selinux/hooks.c | 5 +++++
> >>>>>>>>>>>  1 file changed, 5 insertions(+)
> >>>>>>>>>>
> >>>>>>>>>> I'm not convinced that these two ioctls should be exempt from SELinux
> >>>>>>>>>> policy control, can you explain why allowing these ioctls with the
> >>>>>>>>>> file:ioctl permission is not sufficient for your use case?  Is it a
> >>>>>>>>>> matter of granularity?
> >>>>>>>>>
> >>>>>>>>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
> >>>>>>>>> files.  If I want to allow them with SELinux policy, I have to grant
> >>>>>>>>> *:ioctl to all processes and use xperm rules to determine what ioctls
> >>>>>>>>> are actually allowed.  That is incompatible with existing policies and
> >>>>>>>>> needs frequent maintenance when new ioctls are added.
> >>>>>>>>>
> >>>>>>>>> Furthermore, these ioctls do not allow one to do anything that cannot
> >>>>>>>>> already be done by fcntl(F_SETFD), and (unless I have missed something)
> >>>>>>>>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
> >>>>>>>>> does not improve security, but does risk breaking userspace programs.
> >>>>>>>>> The risk is especially great because in the absence of SELinux, I
> >>>>>>>>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
> >>>>>>>>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
> >>>>>>>>> a file descriptor can be leaked to a child process that should not have
> >>>>>>>>> access to it, but which SELinux allows access to.  Userspace
> >>>>>>>>> SELinux-naive sandboxes are one way this could happen.  Therefore,
> >>>>>>>>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
> >>>>>>>>
> >>>>>>>> I can see you are frustrated with my initial take on this, but please
> >>>>>>>> understand that excluding an operation from the security policy is not
> >>>>>>>> something to take lightly and needs discussion.  I've added the
> >>>>>>>> SELinux refpolicy list to this thread as I believe their input would
> >>>>>>>> be helpful here.
> >>>>>>>
> >>>>>>> Absolutely it is not something that should be taken lightly, though I
> >>>>>>> strongly believe it is correct in this case.  Is one of my assumptions
> >>>>>>> mistaken?
> >>>>>>
> >>>>>> My concern is that there is a distro/admin somewhere which is relying
> >>>>>> on their SELinux policy enforcing access controls on these ioctls and
> >>>>>> removing these controls would cause them a regression.
> >>>>>
> >>>>> I obviously do not have visibility into all systems, but I suspect that
> >>>>> nobody is actually relying on this.  Setting and clearing CLOEXEC via
> >>>>> fcntl is not subject to SELinux restrictions, so blocking FIOCLEX
> >>>>> and FIONCLEX can be trivially bypassed unless fcntl(F_SETFD) is
> >>>>> blocked by seccomp or another LSM.  Clearing close-on-exec can also be
> >>>>> implemented with dup2(), and setting it can be implemented with dup3()
> >>>>> and F_DUPFD_CLOEXEC (which SELinux also allows).  In short, I believe
> >>>>> that unconditionally allowing FIOCLEX and FIONCLEX may fix real-world
> >>>>> problems, and that it is highly unlikely that anyone is relying on the
> >>>>> current behavior.
> >>>>
> >>>> I understand your point, but I remain concerned about making a kernel
> >>>> change for something that can be addressed via policy.  I'm also
> >>>> concerned that in the nine days this thread has been on both the mail
> >>>> SELinux developers and refpolicy lists no one other than you and I
> >>>> have commented on this patch.  In order to consider this patch
> >>>> further, I'm going to need to see comments from others, preferably
> >>>> those with a background in supporting SELinux policy.
> >>>>
> >>>
> >>> AFAIK/AFAICT Android makes no reference to F_SETFD, and tracing the code
> >>> does seem to be ignored, and the code for FIOCLEX FIONCLEX calls into
> >>> the same kernel routine set_close_on_exec().
> >>> Considering that Android's bionic contains support for "e" flag to
> >>> fopen, and it's
> >>> used in a lot of places, makes me more sure the check is skipped for F_SETFD
> >>>
> >>> However, Android does make reference to FIOCLEX FIONCLEX and every
> >>> domain has it enabled:
> >>> domain.te:allowxperm domain { file_type fs_type domain dev_type }:{
> >>> dir notdevfile_class_set blk_file } ioctl { FIOCLEX FIONCLEX };
> >>> domain.te:allowxperm domain tun_device:chr_file ioctl { FIOCLEX FIONCLEX };
> >>>
> >>> Refpolicy doesn't use xperm AFAICT.
> >>>
> >>> I stayed quiet, I wouldn't ack on this myself, but the premise seems
> >>> correct and we
> >>> can safely drop this. Note that I didn't review the code. But we need
> >>> to ensure we handle
> >>> policy correctly and not break anything. I'm not sure what the
> >>> compilers are doing
> >>> for validation of policy macro values, but we would probably want to
> >>> mark it deprecated,
> >>> but still allow loading of old compiled policies.
> >>
> >> Loading of policies is not impacted.  My patch simply skips the
> >> checks for FIOCLEX and FIONCLEX, instead unconditionally allowing the
> >> operation.  This is actually *more* selective than anything that can
> >> be done via policy, as my patch checks the entire ioctl number whereas
> >> policy can only check the low 16 bits.  As such, it is safer than using
> >> policy to allow FIOCLEX and FIONCLEX system-wide: if my patch causes an
> >> ioctl to be allowed, it is guaranteed that that ioctl will change the
> >> close-on-exec flag and have no other effect.
> >>
> >
> > What I meant by my comment is that patching the kernel is only 1/2 the
> > problem. We
> > still need to coordinate with existing policies to deprecate that out,
> > but since it's just
> > Android (AFIAK), that's pretty simple to do. I just want to make sure
> > we don't leave
> > confusing cruft floating around. I looked more at how they do xperms
> > in Android, and it's just
> > an m4 macro to a number. So we would want to coordinate a patch into the kernel
> > with a patch that drops that from Android policy.
>
> The kernel patch needs to come first, but there is no urgency at all
> for the Android policy patch.  The existing Android policy will work
> fine with a patched kernel.

Yes it will work, no one said it wouldn't.
**If we make the change in the kernel, we should also do the cleanup
in policies.**
No cruft left behind, no dead rules.
We shouldn't take a patch here without ensuring that AOSP has a clean
path forward.
and putting a patch through for review gets us buy in and lets them
know about the
kernel change. This isn't about "technically it works". It's about community and
notice to AOSP. We give them a policy patch + link to the kernel patch, it keeps
everyone happy. But that's my opinion, let's just ask them (CC'd).

Dan/Nick do you guys care about these dead ioctl rules after this patch? How
would you like to proceed? Do you have any concerns we're not aware of?

>  Removing the allowxperms for FIOCLEX and
> FIONCLEX will require ensuring that doing so does not make some domains
> not subject to xperm rules, and therefore allow ioctls that would
> previously have been forbidden.

Yes, but this is very solvable. The set of xperms shouldn't change for
an affected
domain with the exception of FIOCLEX and FIONCLEX. sesearch will give
you that, I don't
know if sediff ever got updated for xperms.

> --
> Sincerely,
> Demi Marie Obenour (she/her/hers)

William Roberts Feb. 7, 2022, 9:50 p.m. UTC | #16

On Mon, Feb 7, 2022 at 3:42 PM William Roberts <[e-mail]> wrote:
>
> + NNK and Dan
- nnk and Dan.
+ Jeff
Let me try again, looks like Nick left, not sure about Dan.
Jeff, can you look this over?

>
> On Mon, Feb 7, 2022 at 3:12 PM Demi Marie Obenour <[e-mail]> wrote:
> >
> > On 2/7/22 13:35, William Roberts wrote:
> > > On Mon, Feb 7, 2022 at 11:09 AM Demi Marie Obenour
> > > <[e-mail]> wrote:
> > >>
> > >> On 2/7/22 12:00, William Roberts wrote:
> > >>> On Mon, Feb 7, 2022 at 9:08 AM Paul Moore <[e-mail]> wrote:
> > >>>>
> > >>>> On Wed, Feb 2, 2022 at 5:13 AM Demi Marie Obenour <[e-mail]> wrote:
> > >>>>> On 2/1/22 12:26, Paul Moore wrote:
> > >>>>>> On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
> > >>>>>> <[e-mail]> wrote:
> > >>>>>>> On 1/26/22 17:41, Paul Moore wrote:
> > >>>>>>>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
> > >>>>>>>> <[e-mail]> wrote:
> > >>>>>>>>> On 1/25/22 17:27, Paul Moore wrote:
> > >>>>>>>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
> > >>>>>>>>>> <[e-mail]> wrote:
> > >>>>>>>>>>>
> > >>>>>>>>>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
> > >>>>>>>>>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
> > >>>>>>>>>>> descriptor being leaked to a process that should not have access to it.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
> > >>>>>>>>>>> ---
> > >>>>>>>>>>>  security/selinux/hooks.c | 5 +++++
> > >>>>>>>>>>>  1 file changed, 5 insertions(+)
> > >>>>>>>>>>
> > >>>>>>>>>> I'm not convinced that these two ioctls should be exempt from SELinux
> > >>>>>>>>>> policy control, can you explain why allowing these ioctls with the
> > >>>>>>>>>> file:ioctl permission is not sufficient for your use case?  Is it a
> > >>>>>>>>>> matter of granularity?
> > >>>>>>>>>
> > >>>>>>>>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
> > >>>>>>>>> files.  If I want to allow them with SELinux policy, I have to grant
> > >>>>>>>>> *:ioctl to all processes and use xperm rules to determine what ioctls
> > >>>>>>>>> are actually allowed.  That is incompatible with existing policies and
> > >>>>>>>>> needs frequent maintenance when new ioctls are added.
> > >>>>>>>>>
> > >>>>>>>>> Furthermore, these ioctls do not allow one to do anything that cannot
> > >>>>>>>>> already be done by fcntl(F_SETFD), and (unless I have missed something)
> > >>>>>>>>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
> > >>>>>>>>> does not improve security, but does risk breaking userspace programs.
> > >>>>>>>>> The risk is especially great because in the absence of SELinux, I
> > >>>>>>>>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
> > >>>>>>>>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
> > >>>>>>>>> a file descriptor can be leaked to a child process that should not have
> > >>>>>>>>> access to it, but which SELinux allows access to.  Userspace
> > >>>>>>>>> SELinux-naive sandboxes are one way this could happen.  Therefore,
> > >>>>>>>>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
> > >>>>>>>>
> > >>>>>>>> I can see you are frustrated with my initial take on this, but please
> > >>>>>>>> understand that excluding an operation from the security policy is not
> > >>>>>>>> something to take lightly and needs discussion.  I've added the
> > >>>>>>>> SELinux refpolicy list to this thread as I believe their input would
> > >>>>>>>> be helpful here.
> > >>>>>>>
> > >>>>>>> Absolutely it is not something that should be taken lightly, though I
> > >>>>>>> strongly believe it is correct in this case.  Is one of my assumptions
> > >>>>>>> mistaken?
> > >>>>>>
> > >>>>>> My concern is that there is a distro/admin somewhere which is relying
> > >>>>>> on their SELinux policy enforcing access controls on these ioctls and
> > >>>>>> removing these controls would cause them a regression.
> > >>>>>
> > >>>>> I obviously do not have visibility into all systems, but I suspect that
> > >>>>> nobody is actually relying on this.  Setting and clearing CLOEXEC via
> > >>>>> fcntl is not subject to SELinux restrictions, so blocking FIOCLEX
> > >>>>> and FIONCLEX can be trivially bypassed unless fcntl(F_SETFD) is
> > >>>>> blocked by seccomp or another LSM.  Clearing close-on-exec can also be
> > >>>>> implemented with dup2(), and setting it can be implemented with dup3()
> > >>>>> and F_DUPFD_CLOEXEC (which SELinux also allows).  In short, I believe
> > >>>>> that unconditionally allowing FIOCLEX and FIONCLEX may fix real-world
> > >>>>> problems, and that it is highly unlikely that anyone is relying on the
> > >>>>> current behavior.
> > >>>>
> > >>>> I understand your point, but I remain concerned about making a kernel
> > >>>> change for something that can be addressed via policy.  I'm also
> > >>>> concerned that in the nine days this thread has been on both the mail
> > >>>> SELinux developers and refpolicy lists no one other than you and I
> > >>>> have commented on this patch.  In order to consider this patch
> > >>>> further, I'm going to need to see comments from others, preferably
> > >>>> those with a background in supporting SELinux policy.
> > >>>>
> > >>>
> > >>> AFAIK/AFAICT Android makes no reference to F_SETFD, and tracing the code
> > >>> does seem to be ignored, and the code for FIOCLEX FIONCLEX calls into
> > >>> the same kernel routine set_close_on_exec().
> > >>> Considering that Android's bionic contains support for "e" flag to
> > >>> fopen, and it's
> > >>> used in a lot of places, makes me more sure the check is skipped for F_SETFD
> > >>>
> > >>> However, Android does make reference to FIOCLEX FIONCLEX and every
> > >>> domain has it enabled:
> > >>> domain.te:allowxperm domain { file_type fs_type domain dev_type }:{
> > >>> dir notdevfile_class_set blk_file } ioctl { FIOCLEX FIONCLEX };
> > >>> domain.te:allowxperm domain tun_device:chr_file ioctl { FIOCLEX FIONCLEX };
> > >>>
> > >>> Refpolicy doesn't use xperm AFAICT.
> > >>>
> > >>> I stayed quiet, I wouldn't ack on this myself, but the premise seems
> > >>> correct and we
> > >>> can safely drop this. Note that I didn't review the code. But we need
> > >>> to ensure we handle
> > >>> policy correctly and not break anything. I'm not sure what the
> > >>> compilers are doing
> > >>> for validation of policy macro values, but we would probably want to
> > >>> mark it deprecated,
> > >>> but still allow loading of old compiled policies.
> > >>
> > >> Loading of policies is not impacted.  My patch simply skips the
> > >> checks for FIOCLEX and FIONCLEX, instead unconditionally allowing the
> > >> operation.  This is actually *more* selective than anything that can
> > >> be done via policy, as my patch checks the entire ioctl number whereas
> > >> policy can only check the low 16 bits.  As such, it is safer than using
> > >> policy to allow FIOCLEX and FIONCLEX system-wide: if my patch causes an
> > >> ioctl to be allowed, it is guaranteed that that ioctl will change the
> > >> close-on-exec flag and have no other effect.
> > >>
> > >
> > > What I meant by my comment is that patching the kernel is only 1/2 the
> > > problem. We
> > > still need to coordinate with existing policies to deprecate that out,
> > > but since it's just
> > > Android (AFIAK), that's pretty simple to do. I just want to make sure
> > > we don't leave
> > > confusing cruft floating around. I looked more at how they do xperms
> > > in Android, and it's just
> > > an m4 macro to a number. So we would want to coordinate a patch into the kernel
> > > with a patch that drops that from Android policy.
> >
> > The kernel patch needs to come first, but there is no urgency at all
> > for the Android policy patch.  The existing Android policy will work
> > fine with a patched kernel.
>
> Yes it will work, no one said it wouldn't.
> **If we make the change in the kernel, we should also do the cleanup
> in policies.**
> No cruft left behind, no dead rules.
> We shouldn't take a patch here without ensuring that AOSP has a clean
> path forward.
> and putting a patch through for review gets us buy in and lets them
> know about the
> kernel change. This isn't about "technically it works". It's about community and
> notice to AOSP. We give them a policy patch + link to the kernel patch, it keeps
> everyone happy. But that's my opinion, let's just ask them (CC'd).
>

Jeff?

> Dan/Nick do you guys care about these dead ioctl rules after this patch? How
> would you like to proceed? Do you have any concerns we're not aware of?
>
> >  Removing the allowxperms for FIOCLEX and
> > FIONCLEX will require ensuring that doing so does not make some domains
> > not subject to xperm rules, and therefore allow ioctls that would
> > previously have been forbidden.
>
> Yes, but this is very solvable. The set of xperms shouldn't change for
> an affected
> domain with the exception of FIOCLEX and FIONCLEX. sesearch will give
> you that, I don't
> know if sediff ever got updated for xperms.
>
> > --
> > Sincerely,
> > Demi Marie Obenour (she/her/hers)

Paul Moore Feb. 8, 2022, 12:01 a.m. UTC | #17

On Mon, Feb 7, 2022 at 4:51 PM William Roberts <[e-mail]> wrote:
>
> On Mon, Feb 7, 2022 at 3:42 PM William Roberts <[e-mail]> wrote:
> >
> > + NNK and Dan
> - nnk and Dan.
> + Jeff
> Let me try again, looks like Nick left, not sure about Dan.
> Jeff, can you look this over?

FWIW, I'm still not convinced merging this kernel patch is something
we want to do, so please don't assume that it's a done deal on the
kernel side.

> > On Mon, Feb 7, 2022 at 3:12 PM Demi Marie Obenour <[e-mail]> wrote:
> > >
> > > On 2/7/22 13:35, William Roberts wrote:
> > > > On Mon, Feb 7, 2022 at 11:09 AM Demi Marie Obenour
> > > > <[e-mail]> wrote:
> > > >>
> > > >> On 2/7/22 12:00, William Roberts wrote:
> > > >>> On Mon, Feb 7, 2022 at 9:08 AM Paul Moore <[e-mail]> wrote:
> > > >>>>
> > > >>>> On Wed, Feb 2, 2022 at 5:13 AM Demi Marie Obenour <[e-mail]> wrote:
> > > >>>>> On 2/1/22 12:26, Paul Moore wrote:
> > > >>>>>> On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
> > > >>>>>> <[e-mail]> wrote:
> > > >>>>>>> On 1/26/22 17:41, Paul Moore wrote:
> > > >>>>>>>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
> > > >>>>>>>> <[e-mail]> wrote:
> > > >>>>>>>>> On 1/25/22 17:27, Paul Moore wrote:
> > > >>>>>>>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
> > > >>>>>>>>>> <[e-mail]> wrote:
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
> > > >>>>>>>>>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
> > > >>>>>>>>>>> descriptor being leaked to a process that should not have access to it.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
> > > >>>>>>>>>>> ---
> > > >>>>>>>>>>>  security/selinux/hooks.c | 5 +++++
> > > >>>>>>>>>>>  1 file changed, 5 insertions(+)
> > > >>>>>>>>>>
> > > >>>>>>>>>> I'm not convinced that these two ioctls should be exempt from SELinux
> > > >>>>>>>>>> policy control, can you explain why allowing these ioctls with the
> > > >>>>>>>>>> file:ioctl permission is not sufficient for your use case?  Is it a
> > > >>>>>>>>>> matter of granularity?
> > > >>>>>>>>>
> > > >>>>>>>>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
> > > >>>>>>>>> files.  If I want to allow them with SELinux policy, I have to grant
> > > >>>>>>>>> *:ioctl to all processes and use xperm rules to determine what ioctls
> > > >>>>>>>>> are actually allowed.  That is incompatible with existing policies and
> > > >>>>>>>>> needs frequent maintenance when new ioctls are added.
> > > >>>>>>>>>
> > > >>>>>>>>> Furthermore, these ioctls do not allow one to do anything that cannot
> > > >>>>>>>>> already be done by fcntl(F_SETFD), and (unless I have missed something)
> > > >>>>>>>>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
> > > >>>>>>>>> does not improve security, but does risk breaking userspace programs.
> > > >>>>>>>>> The risk is especially great because in the absence of SELinux, I
> > > >>>>>>>>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
> > > >>>>>>>>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
> > > >>>>>>>>> a file descriptor can be leaked to a child process that should not have
> > > >>>>>>>>> access to it, but which SELinux allows access to.  Userspace
> > > >>>>>>>>> SELinux-naive sandboxes are one way this could happen.  Therefore,
> > > >>>>>>>>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
> > > >>>>>>>>
> > > >>>>>>>> I can see you are frustrated with my initial take on this, but please
> > > >>>>>>>> understand that excluding an operation from the security policy is not
> > > >>>>>>>> something to take lightly and needs discussion.  I've added the
> > > >>>>>>>> SELinux refpolicy list to this thread as I believe their input would
> > > >>>>>>>> be helpful here.
> > > >>>>>>>
> > > >>>>>>> Absolutely it is not something that should be taken lightly, though I
> > > >>>>>>> strongly believe it is correct in this case.  Is one of my assumptions
> > > >>>>>>> mistaken?
> > > >>>>>>
> > > >>>>>> My concern is that there is a distro/admin somewhere which is relying
> > > >>>>>> on their SELinux policy enforcing access controls on these ioctls and
> > > >>>>>> removing these controls would cause them a regression.
> > > >>>>>
> > > >>>>> I obviously do not have visibility into all systems, but I suspect that
> > > >>>>> nobody is actually relying on this.  Setting and clearing CLOEXEC via
> > > >>>>> fcntl is not subject to SELinux restrictions, so blocking FIOCLEX
> > > >>>>> and FIONCLEX can be trivially bypassed unless fcntl(F_SETFD) is
> > > >>>>> blocked by seccomp or another LSM.  Clearing close-on-exec can also be
> > > >>>>> implemented with dup2(), and setting it can be implemented with dup3()
> > > >>>>> and F_DUPFD_CLOEXEC (which SELinux also allows).  In short, I believe
> > > >>>>> that unconditionally allowing FIOCLEX and FIONCLEX may fix real-world
> > > >>>>> problems, and that it is highly unlikely that anyone is relying on the
> > > >>>>> current behavior.
> > > >>>>
> > > >>>> I understand your point, but I remain concerned about making a kernel
> > > >>>> change for something that can be addressed via policy.  I'm also
> > > >>>> concerned that in the nine days this thread has been on both the mail
> > > >>>> SELinux developers and refpolicy lists no one other than you and I
> > > >>>> have commented on this patch.  In order to consider this patch
> > > >>>> further, I'm going to need to see comments from others, preferably
> > > >>>> those with a background in supporting SELinux policy.
> > > >>>>
> > > >>>
> > > >>> AFAIK/AFAICT Android makes no reference to F_SETFD, and tracing the code
> > > >>> does seem to be ignored, and the code for FIOCLEX FIONCLEX calls into
> > > >>> the same kernel routine set_close_on_exec().
> > > >>> Considering that Android's bionic contains support for "e" flag to
> > > >>> fopen, and it's
> > > >>> used in a lot of places, makes me more sure the check is skipped for F_SETFD
> > > >>>
> > > >>> However, Android does make reference to FIOCLEX FIONCLEX and every
> > > >>> domain has it enabled:
> > > >>> domain.te:allowxperm domain { file_type fs_type domain dev_type }:{
> > > >>> dir notdevfile_class_set blk_file } ioctl { FIOCLEX FIONCLEX };
> > > >>> domain.te:allowxperm domain tun_device:chr_file ioctl { FIOCLEX FIONCLEX };
> > > >>>
> > > >>> Refpolicy doesn't use xperm AFAICT.
> > > >>>
> > > >>> I stayed quiet, I wouldn't ack on this myself, but the premise seems
> > > >>> correct and we
> > > >>> can safely drop this. Note that I didn't review the code. But we need
> > > >>> to ensure we handle
> > > >>> policy correctly and not break anything. I'm not sure what the
> > > >>> compilers are doing
> > > >>> for validation of policy macro values, but we would probably want to
> > > >>> mark it deprecated,
> > > >>> but still allow loading of old compiled policies.
> > > >>
> > > >> Loading of policies is not impacted.  My patch simply skips the
> > > >> checks for FIOCLEX and FIONCLEX, instead unconditionally allowing the
> > > >> operation.  This is actually *more* selective than anything that can
> > > >> be done via policy, as my patch checks the entire ioctl number whereas
> > > >> policy can only check the low 16 bits.  As such, it is safer than using
> > > >> policy to allow FIOCLEX and FIONCLEX system-wide: if my patch causes an
> > > >> ioctl to be allowed, it is guaranteed that that ioctl will change the
> > > >> close-on-exec flag and have no other effect.
> > > >>
> > > >
> > > > What I meant by my comment is that patching the kernel is only 1/2 the
> > > > problem. We
> > > > still need to coordinate with existing policies to deprecate that out,
> > > > but since it's just
> > > > Android (AFIAK), that's pretty simple to do. I just want to make sure
> > > > we don't leave
> > > > confusing cruft floating around. I looked more at how they do xperms
> > > > in Android, and it's just
> > > > an m4 macro to a number. So we would want to coordinate a patch into the kernel
> > > > with a patch that drops that from Android policy.
> > >
> > > The kernel patch needs to come first, but there is no urgency at all
> > > for the Android policy patch.  The existing Android policy will work
> > > fine with a patched kernel.
> >
> > Yes it will work, no one said it wouldn't.
> > **If we make the change in the kernel, we should also do the cleanup
> > in policies.**
> > No cruft left behind, no dead rules.
> > We shouldn't take a patch here without ensuring that AOSP has a clean
> > path forward.
> > and putting a patch through for review gets us buy in and lets them
> > know about the
> > kernel change. This isn't about "technically it works". It's about community and
> > notice to AOSP. We give them a policy patch + link to the kernel patch, it keeps
> > everyone happy. But that's my opinion, let's just ask them (CC'd).
> >
>
> Jeff?
>
> > Dan/Nick do you guys care about these dead ioctl rules after this patch? How
> > would you like to proceed? Do you have any concerns we're not aware of?
> >
> > >  Removing the allowxperms for FIOCLEX and
> > > FIONCLEX will require ensuring that doing so does not make some domains
> > > not subject to xperm rules, and therefore allow ioctls that would
> > > previously have been forbidden.
> >
> > Yes, but this is very solvable. The set of xperms shouldn't change for
> > an affected
> > domain with the exception of FIOCLEX and FIONCLEX. sesearch will give
> > you that, I don't
> > know if sediff ever got updated for xperms.
> >
> > > --
> > > Sincerely,
> > > Demi Marie Obenour (she/her/hers)

William Roberts Feb. 8, 2022, 2:05 p.m. UTC | #18

On Mon, Feb 7, 2022 at 6:02 PM Paul Moore <[e-mail]> wrote:
>
> On Mon, Feb 7, 2022 at 4:51 PM William Roberts <[e-mail]> wrote:
> >
> > On Mon, Feb 7, 2022 at 3:42 PM William Roberts <[e-mail]> wrote:
> > >
> > > + NNK and Dan
> > - nnk and Dan.
> > + Jeff
> > Let me try again, looks like Nick left, not sure about Dan.
> > Jeff, can you look this over?
>
> FWIW, I'm still not convinced merging this kernel patch is something
> we want to do, so please don't assume that it's a done deal on the
> kernel side.

I agree and I don't think we can hit the merge button until it gets buy-in from
in-use policy holders.

>
> > > On Mon, Feb 7, 2022 at 3:12 PM Demi Marie Obenour <[e-mail]> wrote:
> > > >
> > > > On 2/7/22 13:35, William Roberts wrote:
> > > > > On Mon, Feb 7, 2022 at 11:09 AM Demi Marie Obenour
> > > > > <[e-mail]> wrote:
> > > > >>
> > > > >> On 2/7/22 12:00, William Roberts wrote:
> > > > >>> On Mon, Feb 7, 2022 at 9:08 AM Paul Moore <[e-mail]> wrote:
> > > > >>>>
> > > > >>>> On Wed, Feb 2, 2022 at 5:13 AM Demi Marie Obenour <[e-mail]> wrote:
> > > > >>>>> On 2/1/22 12:26, Paul Moore wrote:
> > > > >>>>>> On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
> > > > >>>>>> <[e-mail]> wrote:
> > > > >>>>>>> On 1/26/22 17:41, Paul Moore wrote:
> > > > >>>>>>>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
> > > > >>>>>>>> <[e-mail]> wrote:
> > > > >>>>>>>>> On 1/25/22 17:27, Paul Moore wrote:
> > > > >>>>>>>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
> > > > >>>>>>>>>> <[e-mail]> wrote:
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
> > > > >>>>>>>>>>> always allows too.  Furthermore, a failed FIOCLEX could result in a file
> > > > >>>>>>>>>>> descriptor being leaked to a process that should not have access to it.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> Signed-off-by: Demi Marie Obenour <[e-mail]>
> > > > >>>>>>>>>>> ---
> > > > >>>>>>>>>>>  security/selinux/hooks.c | 5 +++++
> > > > >>>>>>>>>>>  1 file changed, 5 insertions(+)
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> I'm not convinced that these two ioctls should be exempt from SELinux
> > > > >>>>>>>>>> policy control, can you explain why allowing these ioctls with the
> > > > >>>>>>>>>> file:ioctl permission is not sufficient for your use case?  Is it a
> > > > >>>>>>>>>> matter of granularity?
> > > > >>>>>>>>>
> > > > >>>>>>>>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
> > > > >>>>>>>>> files.  If I want to allow them with SELinux policy, I have to grant
> > > > >>>>>>>>> *:ioctl to all processes and use xperm rules to determine what ioctls
> > > > >>>>>>>>> are actually allowed.  That is incompatible with existing policies and
> > > > >>>>>>>>> needs frequent maintenance when new ioctls are added.
> > > > >>>>>>>>>
> > > > >>>>>>>>> Furthermore, these ioctls do not allow one to do anything that cannot
> > > > >>>>>>>>> already be done by fcntl(F_SETFD), and (unless I have missed something)
> > > > >>>>>>>>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
> > > > >>>>>>>>> does not improve security, but does risk breaking userspace programs.
> > > > >>>>>>>>> The risk is especially great because in the absence of SELinux, I
> > > > >>>>>>>>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
> > > > >>>>>>>>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
> > > > >>>>>>>>> a file descriptor can be leaked to a child process that should not have
> > > > >>>>>>>>> access to it, but which SELinux allows access to.  Userspace
> > > > >>>>>>>>> SELinux-naive sandboxes are one way this could happen.  Therefore,
> > > > >>>>>>>>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
> > > > >>>>>>>>
> > > > >>>>>>>> I can see you are frustrated with my initial take on this, but please
> > > > >>>>>>>> understand that excluding an operation from the security policy is not
> > > > >>>>>>>> something to take lightly and needs discussion.  I've added the
> > > > >>>>>>>> SELinux refpolicy list to this thread as I believe their input would
> > > > >>>>>>>> be helpful here.
> > > > >>>>>>>
> > > > >>>>>>> Absolutely it is not something that should be taken lightly, though I
> > > > >>>>>>> strongly believe it is correct in this case.  Is one of my assumptions
> > > > >>>>>>> mistaken?
> > > > >>>>>>
> > > > >>>>>> My concern is that there is a distro/admin somewhere which is relying
> > > > >>>>>> on their SELinux policy enforcing access controls on these ioctls and
> > > > >>>>>> removing these controls would cause them a regression.
> > > > >>>>>
> > > > >>>>> I obviously do not have visibility into all systems, but I suspect that
> > > > >>>>> nobody is actually relying on this.  Setting and clearing CLOEXEC via
> > > > >>>>> fcntl is not subject to SELinux restrictions, so blocking FIOCLEX
> > > > >>>>> and FIONCLEX can be trivially bypassed unless fcntl(F_SETFD) is
> > > > >>>>> blocked by seccomp or another LSM.  Clearing close-on-exec can also be
> > > > >>>>> implemented with dup2(), and setting it can be implemented with dup3()
> > > > >>>>> and F_DUPFD_CLOEXEC (which SELinux also allows).  In short, I believe
> > > > >>>>> that unconditionally allowing FIOCLEX and FIONCLEX may fix real-world
> > > > >>>>> problems, and that it is highly unlikely that anyone is relying on the
> > > > >>>>> current behavior.
> > > > >>>>
> > > > >>>> I understand your point, but I remain concerned about making a kernel
> > > > >>>> change for something that can be addressed via policy.  I'm also
> > > > >>>> concerned that in the nine days this thread has been on both the mail
> > > > >>>> SELinux developers and refpolicy lists no one other than you and I
> > > > >>>> have commented on this patch.  In order to consider this patch
> > > > >>>> further, I'm going to need to see comments from others, preferably
> > > > >>>> those with a background in supporting SELinux policy.
> > > > >>>>
> > > > >>>
> > > > >>> AFAIK/AFAICT Android makes no reference to F_SETFD, and tracing the code
> > > > >>> does seem to be ignored, and the code for FIOCLEX FIONCLEX calls into
> > > > >>> the same kernel routine set_close_on_exec().
> > > > >>> Considering that Android's bionic contains support for "e" flag to
> > > > >>> fopen, and it's
> > > > >>> used in a lot of places, makes me more sure the check is skipped for F_SETFD
> > > > >>>
> > > > >>> However, Android does make reference to FIOCLEX FIONCLEX and every
> > > > >>> domain has it enabled:
> > > > >>> domain.te:allowxperm domain { file_type fs_type domain dev_type }:{
> > > > >>> dir notdevfile_class_set blk_file } ioctl { FIOCLEX FIONCLEX };
> > > > >>> domain.te:allowxperm domain tun_device:chr_file ioctl { FIOCLEX FIONCLEX };
> > > > >>>
> > > > >>> Refpolicy doesn't use xperm AFAICT.
> > > > >>>
> > > > >>> I stayed quiet, I wouldn't ack on this myself, but the premise seems
> > > > >>> correct and we
> > > > >>> can safely drop this. Note that I didn't review the code. But we need
> > > > >>> to ensure we handle
> > > > >>> policy correctly and not break anything. I'm not sure what the
> > > > >>> compilers are doing
> > > > >>> for validation of policy macro values, but we would probably want to
> > > > >>> mark it deprecated,
> > > > >>> but still allow loading of old compiled policies.
> > > > >>
> > > > >> Loading of policies is not impacted.  My patch simply skips the
> > > > >> checks for FIOCLEX and FIONCLEX, instead unconditionally allowing the
> > > > >> operation.  This is actually *more* selective than anything that can
> > > > >> be done via policy, as my patch checks the entire ioctl number whereas
> > > > >> policy can only check the low 16 bits.  As such, it is safer than using
> > > > >> policy to allow FIOCLEX and FIONCLEX system-wide: if my patch causes an
> > > > >> ioctl to be allowed, it is guaranteed that that ioctl will change the
> > > > >> close-on-exec flag and have no other effect.
> > > > >>
> > > > >
> > > > > What I meant by my comment is that patching the kernel is only 1/2 the
> > > > > problem. We
> > > > > still need to coordinate with existing policies to deprecate that out,
> > > > > but since it's just
> > > > > Android (AFIAK), that's pretty simple to do. I just want to make sure
> > > > > we don't leave
> > > > > confusing cruft floating around. I looked more at how they do xperms
> > > > > in Android, and it's just
> > > > > an m4 macro to a number. So we would want to coordinate a patch into the kernel
> > > > > with a patch that drops that from Android policy.
> > > >
> > > > The kernel patch needs to come first, but there is no urgency at all
> > > > for the Android policy patch.  The existing Android policy will work
> > > > fine with a patched kernel.
> > >
> > > Yes it will work, no one said it wouldn't.
> > > **If we make the change in the kernel, we should also do the cleanup
> > > in policies.**
> > > No cruft left behind, no dead rules.
> > > We shouldn't take a patch here without ensuring that AOSP has a clean
> > > path forward.
> > > and putting a patch through for review gets us buy in and lets them
> > > know about the
> > > kernel change. This isn't about "technically it works". It's about community and
> > > notice to AOSP. We give them a policy patch + link to the kernel patch, it keeps
> > > everyone happy. But that's my opinion, let's just ask them (CC'd).
> > >
> >
> > Jeff?
> >
> > > Dan/Nick do you guys care about these dead ioctl rules after this patch? How
> > > would you like to proceed? Do you have any concerns we're not aware of?
> > >
> > > >  Removing the allowxperms for FIOCLEX and
> > > > FIONCLEX will require ensuring that doing so does not make some domains
> > > > not subject to xperm rules, and therefore allow ioctls that would
> > > > previously have been forbidden.
> > >
> > > Yes, but this is very solvable. The set of xperms shouldn't change for
> > > an affected
> > > domain with the exception of FIOCLEX and FIONCLEX. sesearch will give
> > > you that, I don't
> > > know if sediff ever got updated for xperms.
> > >
> > > > --
> > > > Sincerely,
> > > > Demi Marie Obenour (she/her/hers)
>
> --
> paul-moore.com

William Roberts Feb. 8, 2022, 2:17 p.m. UTC | #19

<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.

Chris PeBenito Feb. 8, 2022, 3:47 p.m. UTC | #20

On 2/8/2022 09:17, William Roberts 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.

Just to clarify (Demi Marie also mentioned this earlier in the thread), 
what I originally meant was how to emulate this patch by using policy 
rules means you need a rule that allows the two ioctls on all domains 
for all objects.  That results in xperms checks enabled everywhere.


>> 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.

If you can bypass FIONCLEX and FIOCLEX checks by F_SETFD and FD_CLOEXEC, 
then I agree that the two FIO checks don't have value and can be skipped 
as F_SETFD is.

> 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.

Reference policy does not have any xperm rules at this time.  I looked 
at the Fedora policy, and that doesn't have any.

Paul Moore Feb. 8, 2022, 4:26 p.m. UTC | #21

On Tue, Feb 8, 2022 at 9:05 AM William Roberts <[e-mail]> wrote:
> On Mon, Feb 7, 2022 at 6:02 PM Paul Moore <[e-mail]> wrote:
> > On Mon, Feb 7, 2022 at 4:51 PM William Roberts <[e-mail]> wrote:
> > > On Mon, Feb 7, 2022 at 3:42 PM William Roberts <[e-mail]> wrote:
> > > >
> > > > + NNK and Dan
> > > - nnk and Dan.
> > > + Jeff
> > > Let me try again, looks like Nick left, not sure about Dan.
> > > Jeff, can you look this over?
> >
> > FWIW, I'm still not convinced merging this kernel patch is something
> > we want to do, so please don't assume that it's a done deal on the
> > kernel side.
>
> I agree and I don't think we can hit the merge button until it gets buy-in from
> in-use policy holders.

One thing I should have mentioned yesterday, my current thinking is
that if we do make the kernel change it needs to be wrapped with a
policy capability just as we do with other kernel changes which have a
potential impact on policy.

Dominick Grift Feb. 8, 2022, 4:47 p.m. UTC | #22

Chris PeBenito <[e-mail]> writes:

> On 2/8/2022 09:17, William Roberts 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.
>
> Just to clarify (Demi Marie also mentioned this earlier in the
> thread), what I originally meant was how to emulate this patch by
> using policy rules means you need a rule that allows the two ioctls on
> all domains for all objects.  That results in xperms checks enabled
> everywhere.

Thanks. That is clear now. I also learned that is pretty much what
Android's sepolicy is doing. That is probably not something I would do
(enable xperms globally). I would probably leverage it only for "devnode"
chr and maybe blk files and only where they actually are accessed.

I would not mind removing these two checks, but i am not a big user of
xperms (i only filter TIOSCTI on terminal chr files and only for the
entities that write or append them).

>
>
>>> 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.
>
> If you can bypass FIONCLEX and FIOCLEX checks by F_SETFD and
> FD_CLOEXEC, then I agree that the two FIO checks don't have value and
> can be skipped as F_SETFD is.
>
>> 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.
>
> Reference policy does not have any xperm rules at this time.  I looked
> at the Fedora policy, and that doesn't have any.

David Laight Feb. 8, 2022, 11:44 p.m. UTC | #23

From: Dominick Grift
> Sent: 08 February 2022 16:47
...
> I would not mind removing these two checks, but i am not a big user of
> xperms (i only filter TIOSCTI on terminal chr files and only for the
> entities that write or append them).

TIOSCTI isn't your only problem.
Much 'fun' can be had with terminals that support a settable
answerback message.
Possibly that is limited to physical serial terminals, but some
emulators might be 'good enough' to support the relevant escape
sequences.

Even the default answerback message can be very confusing.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Jeffrey Vander Stoep Feb. 14, 2022, 7:11 a.m. UTC | #24

On Tue, Feb 8, 2022 at 3:18 PM William Roberts <[e-mail]> 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.

Paul Moore Feb. 15, 2022, 8:34 p.m. UTC | #25

On Mon, Feb 14, 2022 at 2:11 AM Jeffrey Vander Stoep <[e-mail]> wrote:
> On Tue, Feb 8, 2022 at 3:18 PM William Roberts <[e-mail]> 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?

Christian Göttsche Feb. 17, 2022, 3:04 p.m. UTC | #26

On Tue, 15 Feb 2022 at 21:35, Paul Moore <[e-mail]> wrote:
>
> On Mon, Feb 14, 2022 at 2:11 AM Jeffrey Vander Stoep <[e-mail]> wrote:
> > On Tue, Feb 8, 2022 at 3:18 PM William Roberts <[e-mail]> 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?

May I ask why?
To my understanding policy capabilities exist to retain backwards
compatibility for older
policies, e.g. if a new check is introduced or a new essential class
or permission, which
would break systems running an updated kernel with a non updated policy.
In this case no check or class/permission is added, the xperm checks
against FIO(N)CLEX
are just dropped.  Old policies still defining related allow rules
continue to work.  Existing
polices explicitly not allowing them and relying on SELinux to block changes on
the close-on-exec flag are already broken due to the bypasses via
fnctl(2) and dup(2).

>
> Demi, are you able to respin this patch with policy capability changes?
>
> --
> paul-moore.com

Paul Moore Feb. 17, 2022, 10:25 p.m. UTC | #27

On Thu, Feb 17, 2022 at 10:05 AM Christian Göttsche
<[e-mail]> wrote:
> On Tue, 15 Feb 2022 at 21:35, Paul Moore <[e-mail]> wrote:
> > On Mon, Feb 14, 2022 at 2:11 AM Jeffrey Vander Stoep <[e-mail]> wrote:
> > > On Tue, Feb 8, 2022 at 3:18 PM William Roberts <[e-mail]> 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?
>
> May I ask why?
> To my understanding policy capabilities exist to retain backwards
> compatibility for older
> policies, e.g. if a new check is introduced or a new essential class
> or permission, which
> would break systems running an updated kernel with a non updated policy.
> In this case no check or class/permission is added, the xperm checks
> against FIO(N)CLEX
> are just dropped.  Old policies still defining related allow rules
> continue to work.  Existing
> polices explicitly not allowing them and relying on SELinux to block changes on
> the close-on-exec flag are already broken due to the bypasses via
> fnctl(2) and dup(2).

Policy capabilities are a general tool that we can use when we make a
change in the kernel that could potentially have an effect on the
policy; it allows the policy to (typically) "opt-in" to the change.

In this particular case we are talking about removing access controls,
which is a Very Serious Thing, and protecting this behavior with an
opt-in policy capability seems like a good way to not surprise anyone
with the change.  You are correct in that old policy would continue to
load and work regardless, but I believe it is safer to create a new
policy capability for this.

Demi Marie Obenour Feb. 17, 2022, 11:55 p.m. UTC | #28

On 2/15/22 15:34, Paul Moore wrote:
> On Mon, Feb 14, 2022 at 2:11 AM Jeffrey Vander Stoep <[e-mail]> wrote:
>> On Tue, Feb 8, 2022 at 3:18 PM William Roberts <[e-mail]> 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.

Richard Haines Feb. 18, 2022, 3:06 p.m. UTC | #29

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
> > <[e-mail]> wrote:
> > > On Tue, Feb 8, 2022 at 3:18 PM William Roberts
> > > <[e-mail]> 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;
```

Richard Haines Feb. 18, 2022, 3:39 p.m. UTC | #30

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
> > <[e-mail]> wrote:
> > > On Tue, Feb 8, 2022 at 3:18 PM William Roberts
> > > <[e-mail]> 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;
```

Demi Marie Obenour Feb. 20, 2022, 1:15 a.m. UTC | #31

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
>>> <[e-mail]> wrote:
>>>> On Tue, Feb 8, 2022 at 3:18 PM William Roberts
>>>> <[e-mail]> 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.

Patch

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5b6895e4fc29..8f3b2f15c1f3 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3728,6 +3728,11 @@  static int selinux_file_ioctl(struct file *file, unsigned int cmd,
 		error = file_has_perm(cred, file, FILE__GETATTR);
 		break;
 
+	/* must always succeed */
+	case FIOCLEX:
+	case FIONCLEX:
+		break;
+
 	case FS_IOC_SETFLAGS:
 	case FS_IOC_SETVERSION:
 		error = file_has_perm(cred, file, FILE__SETATTR);