Re: [PATCH V2 1/1] selinux-testsuite: Add filesystem tests

[Richard Haines <richard_c_haines@btinternet.com>]

On Mon, 2020-01-13 at 09:07 -0500, Stephen Smalley wrote:
> On 1/13/20 8:52 AM, Stephen Smalley wrote:
> > On 1/12/20 11:04 AM, Richard Haines wrote:
> > > On Fri, 2020-01-10 at 13:18 -0500, Stephen Smalley wrote:
> > > > On 1/10/20 1:09 PM, Richard Haines wrote:
> > > > > On Thu, 2020-01-09 at 12:19 -0500, Stephen Smalley wrote:
> > > > > > On 1/9/20 10:07 AM, Richard Haines wrote:
> > > > > > > Test filesystem permissions and setfscreatecon(3).
> > > > > > > 
> > > > > > >    From kernels 5.5 filesystem { watch } is also tested.
> > > > > > > 
> > > > > > > Signed-off-by: Richard Haines <
> > > > > > > richard_c_haines@btinternet.com>
> > > > > > > ---
> > > > > > > diff --git a/policy/test_filesystem.te
> > > > > > > b/policy/test_filesystem.te
> > > > > > > new file mode 100644
> > > > > > > index 0000000..2eee1fc
> > > > > > > --- /dev/null
> > > > > > > +++ b/policy/test_filesystem.te
> > > > > > > @@ -0,0 +1,324 @@
> > > > > > > +#
> > > > > > > +######### Test filesystem permissions policy module
> > > > > > > ##########
> > > > > > > +#
> > > > > > > +attribute filesystemdomain;
> > > > > > > +
> > > > > > > +#################### Create a test file context
> > > > > > > ######################
> > > > > > > +type test_filesystem_filecon_t;
> > > > > > > +unconfined_runs_test(test_filesystem_filecon_t)
> > > > > > > +
> > > > > > > +################# Test all functions
> > > > > > > ##########################
> > > > > > > +type test_filesystem_t;
> > > > > > > +domain_type(test_filesystem_t)
> > > > > > > +unconfined_runs_test(test_filesystem_t)
> > > > > > > +typeattribute test_filesystem_t testdomain;
> > > > > > > +typeattribute test_filesystem_t filesystemdomain;
> > > > > > > +
> > > > > > > +allow test_filesystem_t self:capability { sys_admin };
> > > > > > > +allow test_filesystem_t self:filesystem { mount remount
> > > > > > > quotamod
> > > > > > > relabelfrom relabelto unmount quotaget };
> > > > > > > +allow test_filesystem_t self:dir { mounton add_name
> > > > > > > write };
> > > > > > > +allow test_filesystem_t test_file_t:dir { mounton write
> > > > > > > remove_name rmdir };
> > > > > > > +# Create test file
> > > > > > > +allow test_filesystem_t self:dir { add_name write };
> > > > > > > +allow test_filesystem_t self:file { create relabelfrom
> > > > > > > relabelto
> > > > > > > };
> > > > > > > +
> > > > > > > +fs_mount_all_fs(test_filesystem_t)
> > > > > > > +fs_remount_all_fs(test_filesystem_t)
> > > > > > > +fs_unmount_all_fs(test_filesystem_t)
> > > > > > > +fs_relabelfrom_all_fs(test_filesystem_t)
> > > > > > > +fs_get_xattr_fs_quotas(test_filesystem_t)
> > > > > > > +files_search_all(test_filesystem_t)
> > > > > > > +# Required for mount opts
> > > > > > > "rootcontext=system_u:object_r:test_filesystem_t:s0";
> > > > > > > +fs_associate(test_filesystem_t)
> > > > > > > +fs_getattr_xattr_fs(test_filesystem_t)
> > > > > > > +
> > > > > > > +# For running quotacheck(8)
> > > > > > > +files_type(test_filesystem_t)
> > > > > > > +# Update quotas
> > > > > > > +fs_set_all_quotas(test_filesystem_t)
> > > > > > > +allow test_filesystem_t self:file { quotaon };
> > > > > > > +# Create test file and change context:
> > > > > > > +fs_associate(test_filesystem_filecon_t)
> > > > > > > +allow test_filesystem_t test_filesystem_filecon_t:file {
> > > > > > > open
> > > > > > > read
> > > > > > > getattr relabelto write };
> > > > > > > +dontaudit test_filesystem_t kernel_t:process { setsched
> > > > > > > };
> > > > > > 
> > > > > > Why do you need these dontaudit statements?  It seems like
> > > > > > a
> > > > > > kernel
> > > > > > bug
> > > > > > if something is triggering a setsched permission check on
> > > > > > the
> > > > > > kernel_t
> > > > > > domain?  Something the kernel module is doing during
> > > > > > initialization?
> > > > > > 
> > > > > 
> > > > > I've tracked this down to them all being called from
> > > > > block/ioprio.c
> > > > > with: security_task_setioprio(task, ioprio) ->
> > > > > selinux_task_setioprio
> > > > > 
> > > > > Why the SECCLASS_PROCESS, PROCESS__SETSCHED I've no idea. The
> > > > > following
> > > > > also use SET/GETSCHED permission:
> > > > > 
> > > > > selinux_task_getioprio, selinux_task_setnice,
> > > > > selinux_task_movememory
> > > > 
> > > > The confusing bit is that it is between test_filesystem_t and
> > > > kernel_t.
> > > > If the process was setting its own ioprio, then I'd expect to
> > > > see
> > > > the
> > > > denial between test_filesystem_t and test_filesystem_t aka
> > > > self.  If
> > > > the
> > > > process inserted a kernel module and the module initializer
> > > > spawned
> > > > a
> > > > kernel thread that set its ioprio, I would expect it to be
> > > > kernel_t
> > > > to
> > > > kernel_t.
> > > 
> > > Some more info on who calls set_task_ioprio:
> > > 
> > > fs/ext4/super.c calls 'set_task_ioprio' in two places using:
> > >      set_task_ioprio(sbi->s_journal->j_task, journal_ioprio);
> > > The return codes are not checked. This code was added 11 years
> > > ago.
> > > 
> > > fs/btrfs/reada.c also calls 'set_task_ioprio' in two places
> > > using:
> > >      set_task_ioprio(current, BTRFS_IOPRIO_READA);
> > > The return codes are not checked.
> > > 
> > > As can be seen the ext4 module does not use 'current'. I have
> > > patched
> > > kernel 5.5-rc5 to use 'current' and it now works as you expected.
> > > Also
> > > the kernel_t:process { setsched } rules can be removed.
> > > As the problem will exist for some time, I've added to the test
> > > policy:
> > >      kernel_dontaudit_setsched(filesystemdomain)
> > > 
> > > It appears that most of the refpolicy modules do the same.
> > 
> > This seems like a kernel bug to me. I assume that these
> > filesystems 
> > expect the I/O priority to be always set in these cases
> > irrespective of 
> > the permissions of the current process.  Either they should be
> > using 
> > some internal helper function ala a new set_task_ioprio_noperm()
> > that 
> > skips permission checking or they should be temporarily overriding
> > their 
> > cred to the init cred before doing this.  Probably a topic for 
> > linux-fsdevel and/or the respective per-filesystem mailing lists.
> 
> Also, looks like kernel_dontaudit_setsched() isn't defined by
> upstream 
> refpolicy so you'll need the usual ifdefery in test_policy.if to
> allow 
> this to build against refpolicy to appease travis-ci.
> 

I'll change this to domain_setpriority_all_domains() in V4 as that is
in both Fedora and Ref Policy.

>