| Message ID | 1591808483-22040-1-git-send-email-t-josne@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] IMA: Add a test to verify measurment of keys | expand |
Hi Lachlan, thank you for updating LTP! I have few comments below. Mostly just tiny details, which I could fix before merge. > Add a testcase that verifies that the IMA subsystem has correctly > measured keys added to keyrings specified in the IMA policy file. > Additionally, add support for handling a new IMA template descriptor, > namely ima-buf[1], in the IMA measurement tests. Great, thanks! > [1]: https://www.kernel.org/doc/html/latest/security/IMA-templates.html#use ... > +++ b/runtest/ima > @@ -4,3 +4,4 @@ ima_policy ima_policy.sh > ima_tpm ima_tpm.sh > ima_violations ima_violations.sh > evm_overlay evm_overlay.sh > +ima_keys ima_keys.sh Please move evm_overlay after ima_keys. EVM tests require specific configuration, that's why I put it on the end. Or before ima_policy.sh (see comment below). > diff --git a/testcases/kernel/security/integrity/ima/tests/compute_digest.sh b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh > new file mode 100644 Please use 755 (shell libraries which are sources can be 644, but this is meant to be executed). > index 000000000..85f6bf3da > --- /dev/null > +++ b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh Could you please move this to ima_setup.sh? That's the file we keep helper functions. (BTW in LTP shell based tests it's usually foo_lib.sh, I named this ima_setup.sh instead of ima_lib.sh, because it was first meant as tmpfs related setup and still it does, although I later I added other helper functions there. ... > +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh > @@ -0,0 +1,72 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0-or-later > +# Copyright (c) 2020 Microsoft Corporation > +# Author: Lachlan Sneff <t-josne@linux.microsoft.com> > +# > +# Verify that keys are measured correctly based on policy. > + > +TST_NEEDS_CMDS="awk cut" You missed xxd, which is IMHO less common than awk and cut, therefore it should be specified. > +TST_SETUP="setup" > +TST_CNT=1 > +TST_NEEDS_DEVICE=1 > + > +. ima_setup.sh > +. compute_digest.sh > + > +setup() > +{ > + TEST_FILE="$PWD/test.txt" This suggest that file tested in test2 (second commit) requires to be the same. Is it true? If not, I'd set just local variable in each test function. > +} > + > +# Based on https://lkml.org/lkml/2019/12/13/564. Maybe also mention the commit? 450d0fd51564 ("IMA: Call workqueue functions to measure queued keys") > +test1() > +{ > + local keyrings keycheck_line templates > + > + tst_res TINFO "verifying key measurement for keyrings and \ > +templates specified in ima policy file" Could you please keep string on single line? We prefer it over to 80 chars per line rule (although I admit that super long string in check_policy_writable is awfull). > + > + IMA_POLICY="$IMA_DIR/policy" Could you please move IMA_POLICY to ima_setup.sh and remove the same definition from ima_policy.sh? > + [ -f $IMA_POLICY ] || tst_brk TCONF "missing $IMA_POLICY" BTW to read IMA policy requires CONFIG_IMA_READ_POLICY=y. Maybe there could be just some hint, because grep: /sys/kernel/security/ima/policy: Permission denied ima_keys 1 TCONF: ima policy does not specify "func=KEY_CHECK" Could we have here something like cat $IMA_POLICY > /dev/null || tst_res TCONF "cannot read IMA policy (CONFIG_IMA_READ_POLICY=y required)" > + > + keycheck_line=$(grep "func=KEY_CHECK" $IMA_POLICY) > + if [ -z "$keycheck_line" ]; then > + tst_brk TCONF "ima policy does not specify \"func=KEY_CHECK\"" > + fi Could we prepare policy example in testcases/kernel/security/integrity/ima/datafiles/keycheck.policy? I'm trying IMA tests to prepare themselves, otherwise most of the tests would be TCONF. And the idea is that test has everything prepared when run as part of runtest file or also on it's own. But here is getting complicated. Unless there is CONFIG_IMA_WRITE_POLICY=y, we cannot load it in the test. And if we require loading it before, then ima_policy.sh TCONF. Not sure if we should have 2 versions of correct IMA policy. in test2 in ima_policy.sh try to load first keycheck.policy and if it fails then the old measure.policy. That way we would also avoid testing kernel version (this functionality can be backported). We could also print support for new feature. And try load keycheck.policy in ima_keys.sh, in case it's run on it's own (not part of other tests in runtest file). I understand if you don't want to play with this, I can add this after merging your test. But providing keycheck.policy would help. > + > + if echo "$keycheck_line" | grep -q "*keyrings*"; then > + tst_brk TCONF "ima policy does not specify a keyrings to check" > + fi > + > + keyrings=$(echo "$keycheck_line" | tr " " "\n" | grep "keyrings" | \ > + sed "s/\./\\\./g" | cut -d'=' -f2) > + if [ -z "$keyrings" ]; then > + tst_brk TCONF "ima policy has a keyring key-value specifier, \ > +but no specified keyrings" + also here put on single line. > + fi > + > + templates=$(echo "$keycheck_line" | tr " " "\n" | grep "template" | \ > + cut -d'=' -f2) > + > + grep -E "($templates)*($keyrings)" $ASCII_MEASUREMENTS | while read line > + do > + local digest expected_digest algorithm > + > + digest=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f2) > + algorithm=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f1) > + > + echo "$line" | cut -d' ' -f6 | xxd -r -p > $TEST_FILE > + > + expected_digest="$(compute_digest $algorithm $TEST_FILE)" || \ > + tst_brk TCONF "cannot compute digest for $algorithm" > + > + if [ "$digest" != "$expected_digest" ]; then > + tst_res TFAIL "incorrect digest was found for the \ > +$(echo "$line" | cut -d' ' -f5) keyring" Here as well. Maybe add $keyring and use it as variable. > + fi > + done > + > + tst_res TPASS "specified keyrings were measured correctly" > +} ... Kind regards, Petr
Thank you for your review! On 6/11/20 11:30 AM, Petr Vorel wrote: > Hi Lachlan, > > thank you for updating LTP! > > I have few comments below. > Mostly just tiny details, which I could fix before merge. > >> Add a testcase that verifies that the IMA subsystem has correctly >> measured keys added to keyrings specified in the IMA policy file. >> Additionally, add support for handling a new IMA template descriptor, >> namely ima-buf[1], in the IMA measurement tests. > Great, thanks! :) >> [1]: https://www.kernel.org/doc/html/latest/security/IMA-templates.html#use > ... >> +++ b/runtest/ima >> @@ -4,3 +4,4 @@ ima_policy ima_policy.sh >> ima_tpm ima_tpm.sh >> ima_violations ima_violations.sh >> evm_overlay evm_overlay.sh >> +ima_keys ima_keys.sh > Please move evm_overlay after ima_keys. > EVM tests require specific configuration, that's why I put it on the end. > Or before ima_policy.sh (see comment below). Okay >> diff --git a/testcases/kernel/security/integrity/ima/tests/compute_digest.sh b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh >> new file mode 100644 > Please use 755 (shell libraries which are sources can be 644, but this is > meant to be executed). This makes sense, sorry! >> index 000000000..85f6bf3da >> --- /dev/null >> +++ b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh > Could you please move this to ima_setup.sh? That's the file we keep helper > functions. (BTW in LTP shell based tests it's usually foo_lib.sh, I named this > ima_setup.sh instead of ima_lib.sh, because it was first meant as tmpfs related > setup and still it does, although I later I added other helper functions there. I wasn't sure what to do with compute_digest. I didn't want to duplicate it, so I added a a new file for it. Adding it to ima_setup.sh is a solid idea. > ... >> +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh >> @@ -0,0 +1,72 @@ >> +#!/bin/sh >> +# SPDX-License-Identifier: GPL-2.0-or-later >> +# Copyright (c) 2020 Microsoft Corporation >> +# Author: Lachlan Sneff <t-josne@linux.microsoft.com> >> +# >> +# Verify that keys are measured correctly based on policy. >> + >> +TST_NEEDS_CMDS="awk cut" > You missed xxd, which is IMHO less common than awk and cut, therefore it should > be specified. Good call. >> +TST_SETUP="setup" >> +TST_CNT=1 >> +TST_NEEDS_DEVICE=1 >> + >> +. ima_setup.sh >> +. compute_digest.sh >> + >> +setup() >> +{ >> + TEST_FILE="$PWD/test.txt" > This suggest that file tested in test2 (second commit) requires to be the > same. Is it true? If not, I'd set just local variable in each test function. They don't need to be the same file. I'll just create a new one in each test. >> +} >> + >> +# Based on https://lkml.org/lkml/2019/12/13/564. > Maybe also mention the commit? > 450d0fd51564 ("IMA: Call workqueue functions to measure queued keys") Good call. I'll expand this comment. >> +test1() >> +{ >> + local keyrings keycheck_line templates >> + >> + tst_res TINFO "verifying key measurement for keyrings and \ >> +templates specified in ima policy file" > Could you please keep string on single line? We prefer it over to 80 chars per > line rule (although I admit that super long string in check_policy_writable is awfull). Sounds good. I wasn't sure what to do with the long strings. >> + >> + IMA_POLICY="$IMA_DIR/policy" > Could you please move IMA_POLICY to ima_setup.sh and remove the same definition > from ima_policy.sh? Yep. >> + [ -f $IMA_POLICY ] || tst_brk TCONF "missing $IMA_POLICY" > BTW to read IMA policy requires CONFIG_IMA_READ_POLICY=y. Maybe there could be > just some hint, because > > grep: /sys/kernel/security/ima/policy: Permission denied > ima_keys 1 TCONF: ima policy does not specify "func=KEY_CHECK" > > Could we have here something like > cat $IMA_POLICY > /dev/null || tst_res TCONF "cannot read IMA policy (CONFIG_IMA_READ_POLICY=y required)" > Didn't realize it wasn't necessarily readable. Will add that check. >> + >> + keycheck_line=$(grep "func=KEY_CHECK" $IMA_POLICY) >> + if [ -z "$keycheck_line" ]; then >> + tst_brk TCONF "ima policy does not specify \"func=KEY_CHECK\"" >> + fi > Could we prepare policy example in > testcases/kernel/security/integrity/ima/datafiles/keycheck.policy? > > I'm trying IMA tests to prepare themselves, otherwise most of the tests would be > TCONF. And the idea is that test has everything prepared when run as part of > runtest file or also on it's own. But here is getting complicated. Unless there is > CONFIG_IMA_WRITE_POLICY=y, we cannot load it in the test. And if we require > loading it before, then ima_policy.sh TCONF. > > Not sure if we should have 2 versions of correct IMA policy. > in test2 in ima_policy.sh try to load first keycheck.policy and if it fails then > the old measure.policy. That way we would also avoid testing kernel version > (this functionality can be backported). We could also print support for new > feature. And try load keycheck.policy in ima_keys.sh, in case it's run on it's > own (not part of other tests in runtest file). > > I understand if you don't want to play with this, I can add this after merging > your test. But providing keycheck.policy would help. I'll supply a keycheck.policy in my updated patch, but I need to think more about what you've written here. Is the idea to have an example policy that the test can load if the kernel was built with CONFIG_IMA_WRITE_POLICY=y? What should the test do in that case if the kernel wasn't build with a writable IMA policy? >> + >> + if echo "$keycheck_line" | grep -q "*keyrings*"; then >> + tst_brk TCONF "ima policy does not specify a keyrings to check" >> + fi >> + >> + keyrings=$(echo "$keycheck_line" | tr " " "\n" | grep "keyrings" | \ >> + sed "s/\./\\\./g" | cut -d'=' -f2) >> + if [ -z "$keyrings" ]; then >> + tst_brk TCONF "ima policy has a keyring key-value specifier, \ >> +but no specified keyrings" > + also here put on single line. Yep >> + fi >> + >> + templates=$(echo "$keycheck_line" | tr " " "\n" | grep "template" | \ >> + cut -d'=' -f2) >> + >> + grep -E "($templates)*($keyrings)" $ASCII_MEASUREMENTS | while read line >> + do >> + local digest expected_digest algorithm >> + >> + digest=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f2) >> + algorithm=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f1) >> + >> + echo "$line" | cut -d' ' -f6 | xxd -r -p > $TEST_FILE >> + >> + expected_digest="$(compute_digest $algorithm $TEST_FILE)" || \ >> + tst_brk TCONF "cannot compute digest for $algorithm" >> + >> + if [ "$digest" != "$expected_digest" ]; then >> + tst_res TFAIL "incorrect digest was found for the \ >> +$(echo "$line" | cut -d' ' -f5) keyring" > Here as well. Maybe add $keyring and use it as variable. Okay >> + fi >> + done >> + >> + tst_res TPASS "specified keyrings were measured correctly" >> +} > ... > > Kind regards, > Petr Thanks, Lachlan
diff --git a/runtest/ima b/runtest/ima index f3ea88cf0..939fb40f0 100644 --- a/runtest/ima +++ b/runtest/ima @@ -4,3 +4,4 @@ ima_policy ima_policy.sh ima_tpm ima_tpm.sh ima_violations ima_violations.sh evm_overlay evm_overlay.sh +ima_keys ima_keys.sh diff --git a/testcases/kernel/security/integrity/ima/tests/compute_digest.sh b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh new file mode 100644 index 000000000..85f6bf3da --- /dev/null +++ b/testcases/kernel/security/integrity/ima/tests/compute_digest.sh @@ -0,0 +1,38 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +# Copyright (c) 2009 IBM Corporation +# Copyright (c) 2018-2020 Petr Vorel <pvorel@suse.cz> +# Author: Mimi Zohar <zohar@linux.ibm.com> + +# TODO: find support for rmd128 rmd256 rmd320 wp256 wp384 tgr128 tgr160 +compute_digest() +{ + local algorithm="$1" + local file="$2" + local digest + + digest="$(${algorithm}sum $file 2>/dev/null | cut -f1 -d ' ')" + if [ -n "$digest" ]; then + echo "$digest" + return 0 + fi + + digest="$(openssl $algorithm $file 2>/dev/null | cut -f2 -d ' ')" + if [ -n "$digest" ]; then + echo "$digest" + return 0 + fi + + # uncommon ciphers + local arg="$algorithm" + case "$algorithm" in + tgr192) arg="tiger" ;; + wp512) arg="whirlpool" ;; + esac + + digest="$(rdigest --$arg $file 2>/dev/null | cut -f1 -d ' ')" + if [ -n "$digest" ]; then + echo "$digest" + return 0 + fi + return 1 +} diff --git a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh new file mode 100644 index 000000000..1b0dd0aed --- /dev/null +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh @@ -0,0 +1,72 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0-or-later +# Copyright (c) 2020 Microsoft Corporation +# Author: Lachlan Sneff <t-josne@linux.microsoft.com> +# +# Verify that keys are measured correctly based on policy. + +TST_NEEDS_CMDS="awk cut" +TST_SETUP="setup" +TST_CNT=1 +TST_NEEDS_DEVICE=1 + +. ima_setup.sh +. compute_digest.sh + +setup() +{ + TEST_FILE="$PWD/test.txt" +} + +# Based on https://lkml.org/lkml/2019/12/13/564. +test1() +{ + local keyrings keycheck_line templates + + tst_res TINFO "verifying key measurement for keyrings and \ +templates specified in ima policy file" + + IMA_POLICY="$IMA_DIR/policy" + [ -f $IMA_POLICY ] || tst_brk TCONF "missing $IMA_POLICY" + + keycheck_line=$(grep "func=KEY_CHECK" $IMA_POLICY) + if [ -z "$keycheck_line" ]; then + tst_brk TCONF "ima policy does not specify \"func=KEY_CHECK\"" + fi + + if echo "$keycheck_line" | grep -q "*keyrings*"; then + tst_brk TCONF "ima policy does not specify a keyrings to check" + fi + + keyrings=$(echo "$keycheck_line" | tr " " "\n" | grep "keyrings" | \ + sed "s/\./\\\./g" | cut -d'=' -f2) + if [ -z "$keyrings" ]; then + tst_brk TCONF "ima policy has a keyring key-value specifier, \ +but no specified keyrings" + fi + + templates=$(echo "$keycheck_line" | tr " " "\n" | grep "template" | \ + cut -d'=' -f2) + + grep -E "($templates)*($keyrings)" $ASCII_MEASUREMENTS | while read line + do + local digest expected_digest algorithm + + digest=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f2) + algorithm=$(echo "$line" | cut -d' ' -f4 | cut -d':' -f1) + + echo "$line" | cut -d' ' -f6 | xxd -r -p > $TEST_FILE + + expected_digest="$(compute_digest $algorithm $TEST_FILE)" || \ + tst_brk TCONF "cannot compute digest for $algorithm" + + if [ "$digest" != "$expected_digest" ]; then + tst_res TFAIL "incorrect digest was found for the \ +$(echo "$line" | cut -d' ' -f5) keyring" + fi + done + + tst_res TPASS "specified keyrings were measured correctly" +} + +tst_run diff --git a/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh b/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh index 54237d688..0a58d021d 100755 --- a/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh +++ b/testcases/kernel/security/integrity/ima/tests/ima_measurements.sh @@ -12,6 +12,7 @@ TST_CNT=3 TST_NEEDS_DEVICE=1 . ima_setup.sh +. compute_digest.sh setup() { @@ -28,7 +29,7 @@ setup() # parse digest index # https://www.kernel.org/doc/html/latest/security/IMA-templates.html#use case "$template" in - ima|ima-ng|ima-sig) DIGEST_INDEX=4 ;; + ima|ima-ng|ima-sig|ima-buf) DIGEST_INDEX=4 ;; *) # using ima_template_fmt kernel parameter local IFS="|" @@ -46,40 +47,6 @@ setup() "Cannot find digest index (template: '$template')" } -# TODO: find support for rmd128 rmd256 rmd320 wp256 wp384 tgr128 tgr160 -compute_digest() -{ - local algorithm="$1" - local file="$2" - local digest - - digest="$(${algorithm}sum $file 2>/dev/null | cut -f1 -d ' ')" - if [ -n "$digest" ]; then - echo "$digest" - return 0 - fi - - digest="$(openssl $algorithm $file 2>/dev/null | cut -f2 -d ' ')" - if [ -n "$digest" ]; then - echo "$digest" - return 0 - fi - - # uncommon ciphers - local arg="$algorithm" - case "$algorithm" in - tgr192) arg="tiger" ;; - wp512) arg="whirlpool" ;; - esac - - digest="$(rdigest --$arg $file 2>/dev/null | cut -f1 -d ' ')" - if [ -n "$digest" ]; then - echo "$digest" - return 0 - fi - return 1 -} - ima_check() { local delimiter=':'
Add a testcase that verifies that the IMA subsystem has correctly measured keys added to keyrings specified in the IMA policy file. Additionally, add support for handling a new IMA template descriptor, namely ima-buf[1], in the IMA measurement tests. [1]: https://www.kernel.org/doc/html/latest/security/IMA-templates.html#use Signed-off-by: Lachlan Sneff <t-josne@linux.microsoft.com> --- runtest/ima | 1 + .../integrity/ima/tests/compute_digest.sh | 38 ++++++++++ .../security/integrity/ima/tests/ima_keys.sh | 72 +++++++++++++++++++ .../integrity/ima/tests/ima_measurements.sh | 37 +--------- 4 files changed, 113 insertions(+), 35 deletions(-) create mode 100644 testcases/kernel/security/integrity/ima/tests/compute_digest.sh create mode 100644 testcases/kernel/security/integrity/ima/tests/ima_keys.sh