[LTP] [PATCH] security/ima: limit the scope of the LTP policy rules based on the UUID

[Mimi Zohar]

Hi Petr,

On Thu, 2022-10-06 at 23:02 +0200, Petr Vorel wrote:
> Hi Mimi,
> 
> > The LTP policy rules either replace or extend the global IMA policy. As a
> > result, the ordering of the LTP IMA tests is important and affects the
> > ability of re-running the tests.  For example, ima_conditionals.sh
> > defines a rule to measure user files, while ima_measuremnets.sh verifies
> > not measuring user files.  Not limiting the LTP IMA policy scope could
> > also affect the running system.
> 
> > To allow the LTP tests to be re-run without rebooting the system, limit the
> > scope of the LTP policy rules to the loopback mounted filesystem based on
> > the UUID.
> Thanks a lot for this, that'll be a great simplification for IMA testing.

By limiting the scope of the IMA policy rules, not everything would
have to be signed on the file system, which brings us one step closer
to defining LTP appraise policy rules.

> I'll have a deeper look tomorrow, but what we need is to ima_setup.sh is to
> always have loopback device. ATM it's just only if TMPDIR is tmpfs.
> See patch below (untested, I'll test it tomorrow).

Agreed.   Seems to be working.  :)

> 
> Also is the kernel code path very different to use UUID from the current code?

The code path is the same - either the policy rule matches or it
doesn't.  Previously, however, the test files being measured could have
been located on any filesystem.  With this change, the test files now
have to be on the UUID filesystem.

> If yes, we might want also to keep the old behavior enabled with some environment
> variable (the default would be to use UUID). Or not worth of keeping it?

Instead of keeping the old behavior, how about defining additional file
tests that do not match the new UUID policy rule?   These files will
not be measured.

> I'd also wish to have simple C implementation instead requesting blkid
> (although util-linux is very common, it's an extra dependency).
> I might write simple C code which finds which UUID in /dev/disk/by-uuid/ is for
> loop device should be pretty simple code. But for now it's ok to use blkid,
> although it should be added into TST_NEEDS_CMDS.

Sure.  I posted this patch more as a proof of concept than anything
else.

> ...
> > diff --git a/testcases/kernel/security/integrity/ima/tests/ima_conditionals.sh b/testcases/kernel/security/integrity/ima/tests/ima_conditionals.sh
> > index 0d50db906..d5c5f3ebe 100755
> > --- a/testcases/kernel/security/integrity/ima/tests/ima_conditionals.sh
> > +++ b/testcases/kernel/security/integrity/ima/tests/ima_conditionals.sh
> > @@ -28,7 +28,7 @@ verify_measurement()
> >  	ROD rm -f $test_file
> 
> >  	tst_res TINFO "verify measuring user files when requested via $request"
> > -	ROD echo "measure $request=$value" \> $IMA_POLICY
> > +	ROD echo "measure $FSUUID $request=$value" \> $IMA_POLICY
> >  	ROD echo "$(cat /proc/uptime) $request test" \> $test_file
> 
> >  	case "$request" in
> > diff --git a/testcases/kernel/security/integrity/ima/tests/ima_policy.sh b/testcases/kernel/security/integrity/ima/tests/ima_policy.sh
> > index af1fb0028..95e7331a4 100755
> > --- a/testcases/kernel/security/integrity/ima/tests/ima_policy.sh
> > +++ b/testcases/kernel/security/integrity/ima/tests/ima_policy.sh
> > @@ -27,7 +27,12 @@ load_policy()
> >  	exec 2>/dev/null 4>$IMA_POLICY
> >  	[ $? -eq 0 ] || exit 1
> 
> > -	cat $1 >&4 2> /dev/null
> > +	if [ -n "$FSUUID" ]; then
> Interesting, would it be correct if there is no UUID with my changes below (i.e.
> always use the loop device)? Actually, do we also want to have way to disable
> loop device (obviously only on TMPDIR not being tmpfs).

If/when using a non loopback device, there should at least be a major
warning that the global policy has been modified.

> > +		sed "s/measure /measure $FSUUID /" $1 >&4 2> /dev/null
> > +	else
> > +		cat $1 >&4 2> /dev/null
> > +	fi
> > +
> >  	ret=$?
> >  	exec 4>&-
> 
> > diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> > index df3fc5603..016a68cb2 100644
> > --- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> > +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> > @@ -178,6 +178,10 @@ ima_setup()
> >  	if [ "$TST_MOUNT_DEVICE" = 1 ]; then
> >  		tst_res TINFO "\$TMPDIR is on tmpfs => run on loop device"
> >  		cd "$TST_MNTPOINT"
> > +
> > +		loopdev=$(mount | grep $TST_MNTPOINT | cut -f1 -d' ')
> We have $TST_DEVICE for this.
> 
> > +		FSUUID="fsuuid=$(blkid | grep $loopdev | cut -f2 -d'"')"
> > +		tst_res TINFO "LTP IMA policy rules based on $FSUUID"
> >  	fi
> 
> >  	[ -n "$TST_SETUP_CALLER" ] && $TST_SETUP_CALLER
> 
> Proposed (not yet tested) changes.
> 

Thanks, the proposed changes seem to be working.

thanks,

Mimi

> 
> diff --git testcases/kernel/security/integrity/ima/tests/ima_setup.sh testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> index 016a68cb2..dd88fbc71 100644
> --- testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> +++ testcases/kernel/security/integrity/ima/tests/ima_setup.sh
> @@ -11,9 +11,7 @@ TST_CLEANUP_CALLER="$TST_CLEANUP"
>  TST_CLEANUP="ima_cleanup"
>  TST_NEEDS_ROOT=1
>  TST_MOUNT_DEVICE=1
> -
> -# TST_MOUNT_DEVICE can be unset, therefore specify explicitly
> -TST_NEEDS_TMPDIR=1
> +TST_NEEDS_CMDS="$TST_NEEDS_CMDS blkid"
>  
>  SYSFS="/sys"
>  UMOUNT=
> @@ -179,8 +177,7 @@ ima_setup()
>  		tst_res TINFO "\$TMPDIR is on tmpfs => run on loop device"
>  		cd "$TST_MNTPOINT"
>  
> -		loopdev=$(mount | grep $TST_MNTPOINT | cut -f1 -d' ')
> -		FSUUID="fsuuid=$(blkid | grep $loopdev | cut -f2 -d'"')"
> +		FSUUID="fsuuid=$(blkid | grep $TST_DEVICE | cut -f2 -d'"')"
>  		tst_res TINFO "LTP IMA policy rules based on $FSUUID"
>  	fi
>  
> @@ -339,10 +336,4 @@ require_evmctl()
>  	fi
>  }
>  
> -# loop device is needed to use only for tmpfs
> -TMPDIR="${TMPDIR:-/tmp}"
> -if tst_supported_fs -d $TMPDIR -s "tmpfs"; then
> -	unset TST_MOUNT_DEVICE
> -fi
> -
>  . tst_test.sh