| Message ID | 20210910164448.28302-1-alexh@vpitech.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] IMA: Add tests for uid, gid, fowner, and fgroup options | expand |
Hi Alex, > Requires "ima: add gid support". > v2: > - Add sudo to list of required commands > - Check policy writability > - Check kernel version > - Use `sudo sg` to test the gid option > - Don't try to restore the original policy after testing ... > - check_policy_writable > + require_policy_writable Good point to fix function name. But could you please do the rename and move to ima_setup.sh in separate commit? Also, why do you extending test3()? Wouldn't be more readable to add test4()? See notes below. ... > +++ b/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh ... > -TST_NEEDS_CMDS="awk cut sed" > +TST_NEEDS_CMDS="awk cut sed sg sudo" I'm sorry, I was wrong. sudo is needed just in the last test, thus original check "tst_check_cmds sudo || return" is enough. Having it TST_NEEDS_CMDS it requires it also for old kernels, which is necessary. chgrp and sg you newly introduced, should be also tested by tst_check_cmds, after checking kernel version. > TST_SETUP="setup" > TST_CNT=3 > TST_NEEDS_DEVICE=1 > @@ -20,6 +20,8 @@ setup() > TEST_FILE="$PWD/test.txt" > POLICY="$IMA_DIR/policy" > [ -f "$POLICY" ] || tst_res TINFO "not using default policy" > + > + require_policy_writable This changes test to require CONFIG_IMA_WRITE_POLICY=y. Most distributions does not have it, thus you'd disable testing for most distros. Not having policy readable and writeable everywhere greatly complicates IMA testing. ... > } > @@ -103,7 +105,7 @@ test3() > local file="$dir/test.txt" > # Default policy does not measure user files > - tst_res TINFO "verify not measuring user files" > + tst_res TINFO "verify not measuring user files by default" > tst_check_cmds sudo || return > if ! id $user >/dev/null 2>/dev/null; then > @@ -116,9 +118,38 @@ test3() > cd $dir > # need to read file to get updated $ASCII_MEASUREMENTS > sudo -n -u $user sh -c "echo $(date) user file > $file; cat $file > /dev/null" > + EXPECT_FAIL "grep $file $ASCII_MEASUREMENTS" > cd .. > - EXPECT_FAIL "grep $file $ASCII_MEASUREMENTS" > + tst_res TINFO "verify measuring user files when requested via uid" > + ROD echo "measure uid=$(id -u $user)" \> $IMA_POLICY This is the reason for require_policy_writable. Previously it was possible to run it without: ima_measurements 1 TPASS: correct digest found ima_measurements 2 TINFO: verify updating record in the IMA measurement list ima_measurements 2 TINFO: computing digest for sha256 algorithm ima_measurements 2 TPASS: correct digest found ima_measurements 3 TINFO: verify not measuring user files ima_measurements 3 TPASS: grep /tmp/LTP_ima_measurements.6nhS7ScgBn/user/test.txt /sys/kernel/security/ima/ascii_runtime_measurements failed as expected I'd keep the old EXPECT_FAIL variant (suppose it's still valid and don't require writable policy) and definitely separate new tests. Remember, test should run the same on older kernels (we don't want to drop coverage on older distros / enterprise distros). > + ROD echo "$(date) uid test" \> $TEST_FILE > + sudo -n -u $user sh -c "cat $TEST_FILE > /dev/null" > + ima_check > + > + tst_res TINFO "verify measuring user files when requested via fowner" > + ROD echo "measure fowner=$(id -u $user)" \> $IMA_POLICY > + ROD echo "$(date) fowner test" \> $TEST_FILE > + chown $user $TEST_FILE > + cat $TEST_FILE > /dev/null > + ima_check > + > + if tst_kvcmp -lt 5.15; then > + tst_brk TCONF "gid and fgroup options require kernel 5.15 or newer" > + fi > + > + tst_res TINFO "verify measuring user files when requested via gid" > + ROD echo "measure gid=$(id -g $user)" \> $IMA_POLICY > + ROD echo "$(date) gid test" \> $TEST_FILE > + sudo sg $user "sh -c 'cat $TEST_FILE > /dev/null'" > + ima_check > + > + tst_res TINFO "verify measuring user files when requested via fgroup" > + ROD echo "measure fgroup=$(id -g $user)" \> $IMA_POLICY > + ROD echo "$(date) fgroup test" \> $TEST_FILE > + chgrp $user $TEST_FILE > + cat $TEST_FILE > /dev/null > + ima_check > } ... Kind regards, Petr
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh b/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh index 1927e937c..d685fc161 100755 --- a/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh +++ b/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh @@ -6,7 +6,7 @@ # # Verify that measurements are added to the measurement list based on policy. -TST_NEEDS_CMDS="awk cut sed" +TST_NEEDS_CMDS="awk cut sed sg sudo" TST_SETUP="setup" TST_CNT=3 TST_NEEDS_DEVICE=1 @@ -20,6 +20,8 @@ setup() TEST_FILE="$PWD/test.txt" POLICY="$IMA_DIR/policy" [ -f "$POLICY" ] || tst_res TINFO "not using default policy" + + require_policy_writable } ima_check() @@ -103,7 +105,7 @@ test3() local file="$dir/test.txt" # Default policy does not measure user files - tst_res TINFO "verify not measuring user files" + tst_res TINFO "verify not measuring user files by default" tst_check_cmds sudo || return if ! id $user >/dev/null 2>/dev/null; then @@ -116,9 +118,38 @@ test3() cd $dir # need to read file to get updated $ASCII_MEASUREMENTS sudo -n -u $user sh -c "echo $(date) user file > $file; cat $file > /dev/null" + EXPECT_FAIL "grep $file $ASCII_MEASUREMENTS" cd .. - EXPECT_FAIL "grep $file $ASCII_MEASUREMENTS" + tst_res TINFO "verify measuring user files when requested via uid" + ROD echo "measure uid=$(id -u $user)" \> $IMA_POLICY + ROD echo "$(date) uid test" \> $TEST_FILE + sudo -n -u $user sh -c "cat $TEST_FILE > /dev/null" + ima_check + + tst_res TINFO "verify measuring user files when requested via fowner" + ROD echo "measure fowner=$(id -u $user)" \> $IMA_POLICY + ROD echo "$(date) fowner test" \> $TEST_FILE + chown $user $TEST_FILE + cat $TEST_FILE > /dev/null + ima_check + + if tst_kvcmp -lt 5.15; then + tst_brk TCONF "gid and fgroup options require kernel 5.15 or newer" + fi + + tst_res TINFO "verify measuring user files when requested via gid" + ROD echo "measure gid=$(id -g $user)" \> $IMA_POLICY + ROD echo "$(date) gid test" \> $TEST_FILE + sudo sg $user "sh -c 'cat $TEST_FILE > /dev/null'" + ima_check + + tst_res TINFO "verify measuring user files when requested via fgroup" + ROD echo "measure fgroup=$(id -g $user)" \> $IMA_POLICY + ROD echo "$(date) fgroup test" \> $TEST_FILE + chgrp $user $TEST_FILE + cat $TEST_FILE > /dev/null + ima_check } tst_run diff --git a/testcases/kernel/security/integrity/ima/tests/ima_policy.sh b/testcases/kernel/security/integrity/ima/tests/ima_policy.sh index 244cf081d..f1d3b6074 100755 --- a/testcases/kernel/security/integrity/ima/tests/ima_policy.sh +++ b/testcases/kernel/security/integrity/ima/tests/ima_policy.sh @@ -11,19 +11,9 @@ TST_CNT=2 . ima_setup.sh -check_policy_writable() -{ - local err="IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y)" - - [ -f $IMA_POLICY ] || tst_brk TCONF "$err" - # CONFIG_IMA_READ_POLICY - echo "" 2> log > $IMA_POLICY - grep -q "Device or resource busy" log && tst_brk TCONF "$err" -} - setup() { - check_policy_writable + require_policy_writable VALID_POLICY="$TST_DATAROOT/measure.policy" [ -f $VALID_POLICY ] || tst_brk TCONF "missing $VALID_POLICY" @@ -55,7 +45,6 @@ test1() local p1 - check_policy_writable load_policy $INVALID_POLICY & p1=$! wait "$p1" if [ $? -ne 0 ]; then @@ -71,7 +60,6 @@ test2() local p1 p2 rc1 rc2 - check_policy_writable load_policy $VALID_POLICY & p1=$! load_policy $VALID_POLICY & p2=$! wait "$p1"; rc1=$? diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh index 565f0bc3e..9c25d634d 100644 --- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh @@ -73,6 +73,16 @@ require_policy_readable() fi } +require_policy_writable() +{ + local err="IMA policy already loaded and kernel not configured to enable multiple writes to it (need CONFIG_IMA_WRITE_POLICY=y)" + + [ -f $IMA_POLICY ] || tst_brk TCONF "$err" + # CONFIG_IMA_READ_POLICY + echo "" 2> log > $IMA_POLICY + grep -q "Device or resource busy" log && tst_brk TCONF "$err" +} + check_ima_policy_content() { local pattern="$1"
Requires "ima: add gid support". Signed-off-by: Alex Henrie <alexh@vpitech.com> --- v2: - Add sudo to list of required commands - Check policy writability - Check kernel version - Use `sudo sg` to test the gid option - Don't try to restore the original policy after testing --- .../integrity/ima/tests/ima_measurements.sh | 37 +++++++++++++++++-- .../integrity/ima/tests/ima_policy.sh | 14 +------ .../security/integrity/ima/tests/ima_setup.sh | 10 +++++ 3 files changed, 45 insertions(+), 16 deletions(-)