[PATCH testsuite] policy/test_overlayfs.te: allow mounter to create whiteouts

[PATCH testsuite] policy/test_overlayfs.te: allow mounter to create whiteouts

[Stephen Smalley]

This is required to pass the tests for kernels that include
commit a3c751a50fe6 ("vfs: allow unprivileged whiteout creation"),
which changed vfs_mknod() to permit whiteout creation without
requiring CAP_MKNOD and then switched vfs_whiteout() to use vfs_mknod()
rather than calling i_op->mknod() directly, which was originally done
to avoid such checking.  However, vfs_mknod() still calls the LSM hook
and therefore applies SELinux checks on whiteout creation.  Since
vfs_whiteout() now calls vfs_mknod(), SELinux :chr_file create permission
is now required for such whiteout creation by overlayfs.  Skipping the LSM
hook call or SELinux check entirely seems unsafe since we otherwise would
never check whether the process was allowed to create a file in that label;
even though the whiteout device cannot be read/written, it can be used as
a channel wrt its existence and attributes.

See the discussion in:
https://lore.kernel.org/linux-fsdevel/20200409212859.GH28467@miu.piliscsaba.redhat.com/

Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
 policy/test_overlayfs.te | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/policy/test_overlayfs.te b/policy/test_overlayfs.te
index b29621e..c844b82 100644
--- a/policy/test_overlayfs.te
+++ b/policy/test_overlayfs.te
@@ -88,7 +88,7 @@ manage_dirs_pattern(test_overlay_mounter_t, test_overlay_mounter_files_t, test_o
 #
 # Needed to remove a transition file
 #
-allow test_overlay_mounter_t test_overlay_mounter_files_t:chr_file { getattr rename unlink };
+allow test_overlay_mounter_t test_overlay_mounter_files_t:chr_file { create getattr rename unlink };
 allow test_overlay_mounter_t test_overlay_files_rwx_t:chr_file { manage_chr_file_perms rename unlink };
 
 #
-- 
2.23.3

Re: [PATCH testsuite] policy/test_overlayfs.te: allow mounter to create whiteouts

[Ondrej Mosnacek]

On Tue, Jun 2, 2020 at 7:42 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> This is required to pass the tests for kernels that include
> commit a3c751a50fe6 ("vfs: allow unprivileged whiteout creation"),
> which changed vfs_mknod() to permit whiteout creation without
> requiring CAP_MKNOD and then switched vfs_whiteout() to use vfs_mknod()
> rather than calling i_op->mknod() directly, which was originally done
> to avoid such checking.  However, vfs_mknod() still calls the LSM hook
> and therefore applies SELinux checks on whiteout creation.  Since
> vfs_whiteout() now calls vfs_mknod(), SELinux :chr_file create permission
> is now required for such whiteout creation by overlayfs.  Skipping the LSM
> hook call or SELinux check entirely seems unsafe since we otherwise would
> never check whether the process was allowed to create a file in that label;
> even though the whiteout device cannot be read/written, it can be used as
> a channel wrt its existence and attributes.
>
> See the discussion in:
> https://lore.kernel.org/linux-fsdevel/20200409212859.GH28467@miu.piliscsaba.redhat.com/
>
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
>  policy/test_overlayfs.te | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, this looks good and fixes the failure for me.

>
> diff --git a/policy/test_overlayfs.te b/policy/test_overlayfs.te
> index b29621e..c844b82 100644
> --- a/policy/test_overlayfs.te
> +++ b/policy/test_overlayfs.te
> @@ -88,7 +88,7 @@ manage_dirs_pattern(test_overlay_mounter_t, test_overlay_mounter_files_t, test_o
>  #
>  # Needed to remove a transition file

Just please also update this comment ("...to create and remove..." or similar).

>  #
> -allow test_overlay_mounter_t test_overlay_mounter_files_t:chr_file { getattr rename unlink };
> +allow test_overlay_mounter_t test_overlay_mounter_files_t:chr_file { create getattr rename unlink };
>  allow test_overlay_mounter_t test_overlay_files_rwx_t:chr_file { manage_chr_file_perms rename unlink };
>
>  #
> --
> 2.23.3
>

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH testsuite] policy/test_overlayfs.te: allow mounter to create whiteouts

[Stephen Smalley]

On Wed, Jun 3, 2020 at 4:23 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Tue, Jun 2, 2020 at 7:42 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> > This is required to pass the tests for kernels that include
> > commit a3c751a50fe6 ("vfs: allow unprivileged whiteout creation"),
> > which changed vfs_mknod() to permit whiteout creation without
> > requiring CAP_MKNOD and then switched vfs_whiteout() to use vfs_mknod()
> > rather than calling i_op->mknod() directly, which was originally done
> > to avoid such checking.  However, vfs_mknod() still calls the LSM hook
> > and therefore applies SELinux checks on whiteout creation.  Since
> > vfs_whiteout() now calls vfs_mknod(), SELinux :chr_file create permission
> > is now required for such whiteout creation by overlayfs.  Skipping the LSM
> > hook call or SELinux check entirely seems unsafe since we otherwise would
> > never check whether the process was allowed to create a file in that label;
> > even though the whiteout device cannot be read/written, it can be used as
> > a channel wrt its existence and attributes.
> >
> > See the discussion in:
> > https://lore.kernel.org/linux-fsdevel/20200409212859.GH28467@miu.piliscsaba.redhat.com/
> >
> > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > ---
> >  policy/test_overlayfs.te | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Thanks, this looks good and fixes the failure for me.
>
> >
> > diff --git a/policy/test_overlayfs.te b/policy/test_overlayfs.te
> > index b29621e..c844b82 100644
> > --- a/policy/test_overlayfs.te
> > +++ b/policy/test_overlayfs.te
> > @@ -88,7 +88,7 @@ manage_dirs_pattern(test_overlay_mounter_t, test_overlay_mounter_files_t, test_o
> >  #
> >  # Needed to remove a transition file
>
> Just please also update this comment ("...to create and remove..." or similar).

That wouldn't be correct.  The additional :chr_file create permission
is needed to remove the transition file because removing a file from
overlayfs creates a whiteout device file.  It isn't to create the
transition file.

Re: [PATCH testsuite] policy/test_overlayfs.te: allow mounter to create whiteouts

[Ondrej Mosnacek]

On Wed, Jun 3, 2020 at 3:12 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Wed, Jun 3, 2020 at 4:23 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > On Tue, Jun 2, 2020 at 7:42 PM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > > This is required to pass the tests for kernels that include
> > > commit a3c751a50fe6 ("vfs: allow unprivileged whiteout creation"),
> > > which changed vfs_mknod() to permit whiteout creation without
> > > requiring CAP_MKNOD and then switched vfs_whiteout() to use vfs_mknod()
> > > rather than calling i_op->mknod() directly, which was originally done
> > > to avoid such checking.  However, vfs_mknod() still calls the LSM hook
> > > and therefore applies SELinux checks on whiteout creation.  Since
> > > vfs_whiteout() now calls vfs_mknod(), SELinux :chr_file create permission
> > > is now required for such whiteout creation by overlayfs.  Skipping the LSM
> > > hook call or SELinux check entirely seems unsafe since we otherwise would
> > > never check whether the process was allowed to create a file in that label;
> > > even though the whiteout device cannot be read/written, it can be used as
> > > a channel wrt its existence and attributes.
> > >
> > > See the discussion in:
> > > https://lore.kernel.org/linux-fsdevel/20200409212859.GH28467@miu.piliscsaba.redhat.com/
> > >
> > > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > > ---
> > >  policy/test_overlayfs.te | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Thanks, this looks good and fixes the failure for me.
> >
> > >
> > > diff --git a/policy/test_overlayfs.te b/policy/test_overlayfs.te
> > > index b29621e..c844b82 100644
> > > --- a/policy/test_overlayfs.te
> > > +++ b/policy/test_overlayfs.te
> > > @@ -88,7 +88,7 @@ manage_dirs_pattern(test_overlay_mounter_t, test_overlay_mounter_files_t, test_o
> > >  #
> > >  # Needed to remove a transition file
> >
> > Just please also update this comment ("...to create and remove..." or similar).
>
> That wouldn't be correct.  The additional :chr_file create permission
> is needed to remove the transition file because removing a file from
> overlayfs creates a whiteout device file.  It isn't to create the
> transition file.

Ah, I see... In that case the patch is fine as-is, so:

Acked-by: Ondrej Mosnacek <omosnace@redhat.com>

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH testsuite] policy/test_overlayfs.te: allow mounter to create whiteouts

[Ondrej Mosnacek]

On Wed, Jun 3, 2020 at 3:20 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Jun 3, 2020 at 3:12 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> > On Wed, Jun 3, 2020 at 4:23 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > On Tue, Jun 2, 2020 at 7:42 PM Stephen Smalley
> > > <stephen.smalley.work@gmail.com> wrote:
> > > > This is required to pass the tests for kernels that include
> > > > commit a3c751a50fe6 ("vfs: allow unprivileged whiteout creation"),
> > > > which changed vfs_mknod() to permit whiteout creation without
> > > > requiring CAP_MKNOD and then switched vfs_whiteout() to use vfs_mknod()
> > > > rather than calling i_op->mknod() directly, which was originally done
> > > > to avoid such checking.  However, vfs_mknod() still calls the LSM hook
> > > > and therefore applies SELinux checks on whiteout creation.  Since
> > > > vfs_whiteout() now calls vfs_mknod(), SELinux :chr_file create permission
> > > > is now required for such whiteout creation by overlayfs.  Skipping the LSM
> > > > hook call or SELinux check entirely seems unsafe since we otherwise would
> > > > never check whether the process was allowed to create a file in that label;
> > > > even though the whiteout device cannot be read/written, it can be used as
> > > > a channel wrt its existence and attributes.
> > > >
> > > > See the discussion in:
> > > > https://lore.kernel.org/linux-fsdevel/20200409212859.GH28467@miu.piliscsaba.redhat.com/
> > > >
> > > > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > > > ---
> > > >  policy/test_overlayfs.te | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > Thanks, this looks good and fixes the failure for me.
> > >
> > > >
> > > > diff --git a/policy/test_overlayfs.te b/policy/test_overlayfs.te
> > > > index b29621e..c844b82 100644
> > > > --- a/policy/test_overlayfs.te
> > > > +++ b/policy/test_overlayfs.te
> > > > @@ -88,7 +88,7 @@ manage_dirs_pattern(test_overlay_mounter_t, test_overlay_mounter_files_t, test_o
> > > >  #
> > > >  # Needed to remove a transition file
> > >
> > > Just please also update this comment ("...to create and remove..." or similar).
> >
> > That wouldn't be correct.  The additional :chr_file create permission
> > is needed to remove the transition file because removing a file from
> > overlayfs creates a whiteout device file.  It isn't to create the
> > transition file.
>
> Ah, I see... In that case the patch is fine as-is, so:
>
> Acked-by: Ondrej Mosnacek <omosnace@redhat.com>

This patch is now applied.

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.