| Message ID | 20200602174220.10210-1-stephen.smalley.work@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Ondrej Mosnáček |
| Headers | show |
| Series | [testsuite] policy/test_overlayfs.te: allow mounter to create whiteouts | expand |
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 >
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.
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>
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.
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 }; #
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(-)