Message-ID: <c0ba2244832ea4c95e936674c4e818b26b511d61.camel@huaweicloud.com>
Subject: Re: [PATCH ima-evm-utils v5] Add tests for MMAP_CHECK and
 MMAP_CHECK_REQPROT hooks
From:   Roberto Sassu <roberto.sassu@huaweicloud.com>
To:     Mimi Zohar <zohar@linux.ibm.com>, dmitry.kasatkin@gmail.com,
        jmorris@namei.org, serge@hallyn.com
Cc:     linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org, stefanb@linux.ibm.com,
        viro@zeniv.linux.org.uk, pvorel@suse.cz,
        Roberto Sassu <roberto.sassu@huawei.com>
Date:   Tue, 07 Feb 2023 17:57:31 +0100
In-Reply-To: <d6efb292273eee6caff9afc8b64e42984a3ae517.camel@huaweicloud.com>
References: <20230203125637.2673781-1-roberto.sassu@huaweicloud.com>
         <cd21f0d2a65f9673a0abe7f0a7219d5f1fe55911.camel@linux.ibm.com>
         <d6efb292273eee6caff9afc8b64e42984a3ae517.camel@huaweicloud.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 2023-02-07 at 11:16 +0100, Roberto Sassu wrote:
> On Mon, 2023-02-06 at 08:20 -0500, Mimi Zohar wrote:
> > On Fri, 2023-02-03 at 13:56 +0100, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > 
> > > Add tests to ensure that, after applying the kernel patch 'ima: Align
> > > ima_file_mmap() parameters with mmap_file LSM hook', the MMAP_CHECK hook
> > > checks the protections applied by the kernel and not those requested by the
> > > application.
> > > 
> > > Also ensure that after applying 'ima: Introduce MMAP_CHECK_REQPROT hook',
> > > the MMAP_CHECK_REQPROT hook checks the protections requested by the
> > > application.
> > > 
> > > Test both with the test_mmap application that by default requests the
> > > PROT_READ protection flag. Its syntax is:
> > > 
> > > test_mmap <file> <mode>
> > > 
> > > where mode can be:
> > > - exec: adds the PROT_EXEC protection flag to mmap()
> > > - read_implies_exec: calls the personality() system call with
> > >                      READ_IMPLIES_EXEC as the first argument before mmap()
> > > - mprotect: adds the PROT_EXEC protection flag to a memory area in addition
> > >             to PROT_READ
> > > - exec_on_writable: calls mmap() with PROT_EXEC on a file which has a
> > >                     writable mapping
> > > 
> > > Check the different combinations of hooks/modes and ensure that a
> > > measurement entry is found in the IMA measurement list only when it is
> > > expected. No measurement entry should be found when only the PROT_READ
> > > protection flag is requested or the matching policy rule has the
> > > MMAP_CHECK_REQPROT hook and the personality() system call was called with
> > > READ_IMPLIES_EXEC.
> > > 
> > > mprotect() with PROT_EXEC on an existing memory area protected with
> > > PROT_READ should be denied (with an appraisal rule), regardless of the MMAP
> > > hook specified in the policy. The same applies for mmap() with PROT_EXEC on
> > > a file with a writable mapping.
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Thanks, Roberto.  Other than the one comment below, it looks good.
> > 
> > > +
> > > +if ! awk '$0 ~ /^(measure|appraise)/ && $0 !~ /fsuuid=/ && $0 !~ /fowner=/ { exit 1 }' < /sys/kernel/security/ima/policy; then
> > > +	echo "${CYAN}IMA policy rules without fsuuid= and fowner=, cannot continue due to possible interference with the tests${NORM}"
> > > +	exit "$SKIP"
> > > +fi
> > 
> > The test should be limited to just MMAP_CHECK and MMAP_CHECK_REQPROT
> > policy rules.
> > 
> > +if ! awk '$0 ~ /^(measure|appraise)/ && $0 ~ /func=MMAP_CHECK/ && $0 !~ /fsuuid=/ && ...
> 
> Oh, yes. Better.

It seems more complicated than that.

If we consider only MMAP_CHECK and MMAP_CHECK_REQPROT rules, we might
miss rules without func= that can potentially overlap.

Overlap of measure and appraise rules per se should not be a problem,
unless additional options are specified in the rule. In that case, the
options of the first matching rule are taken and the other options from
other rules might not be processed (IMA stops checking the policy when
it has encountered rules with the possible actions, determined when the
policy is loaded).

Also, dont_measure and dont_appraise rules are a possible concern, as
they could be matched before ours and could change the expected
outcome.

A proposal could be to ignore existing rules, regardless of the action,
if they provide a different value for at least one of the policy
keywords (in 'base' and 'lsm') present in the rule being added.

For the rules that we didn't ignore, we can accept them if they have
the same action and no/the same policy options.

Roberto