From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B602EC433DB for ; Mon, 21 Dec 2020 23:24:26 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 261AB22B2B for ; Mon, 21 Dec 2020 23:24:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 261AB22B2B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: mail.kernel.org; spf=tempfail smtp.mailfrom=dm-devel-bounces@redhat.com Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-39-CQwe25FMPDakqTk6nG3N-g-1; Mon, 21 Dec 2020 18:24:22 -0500 X-MC-Unique: CQwe25FMPDakqTk6nG3N-g-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A1C46107ACE3; Mon, 21 Dec 2020 23:24:17 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3C5616064B; Mon, 21 Dec 2020 23:24:14 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id 3FC791809C9F; Mon, 21 Dec 2020 23:24:10 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id 0BLNNj6j023788 for ; Mon, 21 Dec 2020 18:23:46 -0500 Received: by smtp.corp.redhat.com (Postfix) id CA8305F25E; Mon, 21 Dec 2020 23:23:45 +0000 (UTC) Received: from mimecast-mx02.redhat.com (mimecast04.extmail.prod.ext.rdu2.redhat.com [10.11.55.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C55DB621BF for ; Mon, 21 Dec 2020 23:23:42 +0000 (UTC) Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 4D627101A560 for ; Mon, 21 Dec 2020 23:23:42 +0000 (UTC) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-280-gVbbufvNOXGGphFt2DhSTA-1; Mon, 21 Dec 2020 18:23:38 -0500 X-MC-Unique: gVbbufvNOXGGphFt2DhSTA-1 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 8FAD9AC93; Mon, 21 Dec 2020 23:05:33 +0000 (UTC) Date: Tue, 22 Dec 2020 00:05:31 +0100 From: Petr Vorel To: Tushar Sugandhi Message-ID: <20201221230531.GD4453@pevik> References: <20200928035605.22701-1-tusharsu@linux.microsoft.com> <20200928035605.22701-2-tusharsu@linux.microsoft.com> MIME-Version: 1.0 In-Reply-To: <20200928035605.22701-2-tusharsu@linux.microsoft.com> X-Mimecast-Impersonation-Protect: Policy=CLT - Impersonation Protection Definition; Similar Internal Domain=false; Similar Monitored External Domain=false; Custom External Domain=false; Mimecast External Domain=false; Newly Observed Domain=false; Internal User Name=false; Custom Display Name List=false; Reply-to Address Mismatch=false; Targeted Threat Dictionary=false; Mimecast Threat Dictionary=false; Custom Threat Dictionary=false X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-loop: dm-devel@redhat.com Cc: snitzer@redhat.com, zohar@linux.ibm.com, nramas@linux.microsoft.com, dm-devel@redhat.com, ltp@lists.linux.it, linux-integrity@vger.kernel.org, gmazyland@gmail.com, agk@redhat.com Subject: Re: [dm-devel] [PATCH v2 1/2] IMA: generalize key measurement tests X-BeenThere: dm-devel@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk Reply-To: Petr Vorel List-Id: device-mapper development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dm-devel-bounces@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Hi Tushar, I'm very sorry about the delay. I'll finish this review in January, here just some quick thoughts (minor style nits, I'll fix it before merge). Generally LGTM, thanks for your work. Reviewed-by: Petr Vorel > New functionality is being added in IMA to measure data provided by > kernel components. Tests have to be added in LTP to validate this new > feature. The functionality in ima_keys.sh can be reused to test this new > feature if it is made generic. > Refactor check_keys_policy() and test1() implemented in ima_keys.sh to > make it generic, and move the functionality to ima_setup.sh as new > functions - check_policy_pattern() and check_ima_ascii_log_for_policy(). > Signed-off-by: Tushar Sugandhi > --- > .../security/integrity/ima/tests/ima_keys.sh | 62 +++------------ > .../security/integrity/ima/tests/ima_setup.sh | 79 +++++++++++++++++++ > 2 files changed, 92 insertions(+), 49 deletions(-) > diff --git a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh > index c9eef4b68..c2120358a 100755 > --- a/testcases/kernel/security/integrity/ima/tests/ima_keys.sh > +++ b/testcases/kernel/security/integrity/ima/tests/ima_keys.sh > @@ -6,7 +6,7 @@ > # Verify that keys are measured correctly based on policy. > -TST_NEEDS_CMDS="cmp cut grep sed xxd" > +TST_NEEDS_CMDS="cmp cut grep xxd" It still requires sed, it's just hidden in check_ima_ascii_log_for_policy Maybe just put at the top of check_ima_ascii_log_for_policy(): tst_require_cmds cut grep sed xxd And here still keep TST_NEEDS_CMDS="cmp cut grep tail xxd" This leads to duplicity in check, but it will not lead to hidden "command not found". > TST_CNT=2 > TST_NEEDS_DEVICE=1 > TST_SETUP=setup > @@ -28,64 +28,28 @@ cleanup() > tst_is_num $KEYRING_ID && keyctl clear $KEYRING_ID > } > -check_keys_policy() > -{ > - local pattern="$1" > - > - if ! grep -E "$pattern" $TST_TMPDIR/policy.txt; then > - tst_res TCONF "IMA policy must specify $pattern, $FUNC_KEYCHECK, $TEMPLATE_BUF" > - return 1 > - fi > - return 0 > -} > - > # Based on https://lkml.org/lkml/2019/12/13/564. > # (450d0fd51564 - "IMA: Call workqueue functions to measure queued keys") OK, it has been merged in v5.6-rc1. Any more relevant commits, changes since then? > test1() > { > local keycheck_lines i keyrings templates > local pattern='keyrings=[^[:space:]]+' > - local test_file="file.txt" tmp_file="file2.txt" > + local policy="keyrings" > + local tmp_file="$TST_TMPDIR/keycheck_tmp_file.txt" > + local res Will be unused, see below. > tst_res TINFO "verify key measurement for keyrings and templates specified in IMA policy" > - check_keys_policy "$pattern" > $tmp_file || return > - keycheck_lines=$(cat $tmp_file) > - keyrings=$(for i in $keycheck_lines; do echo "$i" | grep "keyrings" | \ > - sed "s/\./\\\./g" | cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g') > - if [ -z "$keyrings" ]; then > - tst_res TCONF "IMA policy has a keyring key-value specifier, but no specified keyrings" > - return > - fi > - > - templates=$(for i in $keycheck_lines; do echo "$i" | grep "template" | \ > - cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g') > - > - tst_res TINFO "keyrings: '$keyrings'" > - tst_res TINFO "templates: '$templates'" > - > - 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) > - keyring=$(echo "$line" | cut -d' ' -f5) > + check_policy_pattern "$pattern" $FUNC_KEYCHECK $TEMPLATE_BUF > $tmp_file || return > - echo "$line" | cut -d' ' -f6 | xxd -r -p > $test_file > + res="$(check_ima_ascii_log_for_policy $policy $tmp_file)" > - if ! expected_digest="$(compute_digest $algorithm $test_file)"; then > - tst_res TCONF "cannot compute digest for $algorithm" > - return > - fi > - > - if [ "$digest" != "$expected_digest" ]; then > - tst_res TFAIL "incorrect digest was found for $keyring keyring" > - return > - fi > - done > + if [ "$res" = "0" ]; then > + tst_res TPASS "specified keyrings were measured correctly" > + else > + tst_res TFAIL "failed to measure specified keyrings" > + fi Instead of: res="$(check_ima_ascii_log_for_policy $policy $tmp_file)" if [ "$res" = "0" ]; then I'd prefer to have it as: check_ima_ascii_log_for_policy $policy $tmp_file if [ $? -eq 0 ]; then > - tst_res TPASS "specified keyrings were measured correctly" > } > # Create a new keyring, import a certificate into it, and verify > @@ -97,11 +61,11 @@ test2() > local cert_file="$TST_DATAROOT/x509_ima.der" > local keyring_name="key_import_test" > local pattern="keyrings=[^[:space:]]*$keyring_name" > - local temp_file="file.txt" > + local temp_file="$TST_TMPDIR/key_import_test_file.txt" > tst_res TINFO "verify measurement of certificate imported into a keyring" > - check_keys_policy "$pattern" >/dev/null || return > + check_policy_pattern "$pattern" $FUNC_KEYCHECK $TEMPLATE_BUF >/dev/null || return > KEYRING_ID=$(keyctl newring $keyring_name @s) || \ > tst_brk TBROK "unable to create a new keyring" > diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh > index 1f17aa707..2841d7df5 100644 > --- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh > +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh > @@ -54,6 +54,85 @@ compute_digest() > return 1 > } > +check_policy_pattern() > +{ > + local pattern="$1" > + local func="$2" > + local template="$3" > + > + if ! grep -E "$pattern" $TST_TMPDIR/policy.txt; then > + tst_res TCONF "IMA policy must specify $pattern, $func, $template" > + return 1 > + fi > + return 0 > +} Probably ok for now (yes, it removes the duplicity with function used in two tests, it's very policy specific). > + > +check_ima_ascii_log_for_policy() > +{ > + local test_file="$TST_TMPDIR/ascii_log_test_file.txt" > + local grep_file="$TST_TMPDIR/ascii_log_grep_file.txt" nit: Since the real description is in variable, I'd just use: local test_file="$TST_TMPDIR/test.txt" local grep_file="$TST_TMPDIR/grep.txt" > + local func_lines sources templates i src > + local input_digest_res=1 > + local policy_option="$1" > + local input_digest="$3" tst_require_cmds cut grep sed xxd > + > + func_lines=$(cat $2) > + > + sources=$(for i in $func_lines; do echo "$i" | grep "$policy_option" | \ > + sed "s/\./\\\./g" | cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g') > + if [ -z "$sources" ]; then > + tst_res TCONF "IMA policy $policy_option is a key-value specifier, but no values specified" > + echo "1" > + return > + fi > + > + templates=$(for i in $func_lines; do echo "$i" | grep "template" | \ > + cut -d'=' -f2; done | sed ':a;N;$!ba;s/\n/|/g') > + > + tst_res TINFO "policy sources: '$sources'" > + tst_res TINFO "templates: '$templates'" > + > + grep -E "($templates).*($sources)" $ASCII_MEASUREMENTS > $grep_file > + > + 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) > + src_line=$(echo "$line" | cut -d' ' -f5) > + > + echo "$line" | cut -d' ' -f6 | xxd -r -p > $test_file > + > + if ! expected_digest="$(compute_digest $algorithm $test_file)"; then > + tst_res TCONF "cannot compute digest for $algorithm" > + echo "1" > + return > + fi > + > + if [ "$digest" != "$expected_digest" ]; then > + tst_res TINFO "incorrect digest was found for $src_line $policy_option" > + echo "1" > + return > + fi > + > + if [ "$input_digest" ]; then > + if [ "$digest" = "$input_digest" ]; then > + input_digest_res=0 > + fi > + fi I'd prefer it as single if: if [ -n "$input_digest" -a "$digest" = "$input_digest" ]; then input_digest_res=0 fi > + > + done < $grep_file > + > + if [ "$input_digest" ]; then > + echo "$input_digest_res" > + return this return is redundant. > + else > + echo "0" > + return Also this one. > + fi And actually, instead of whole if/else block wouldn't be just this enough? echo "$input_digest_res" Isn't it the zero value set in the loop at: if [ -n "$input_digest" -a "$digest" = "$input_digest" ]; then input_digest_res=0 fi Kind regards, Petr -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel