Re: [PATCH ima-evm-utils v5] Add tests for MMAP_CHECK and MMAP_CHECK_REQPROT hooks

On Tue, 2023-02-07 at 11:16 +0100, Roberto Sassu wrote:

On Mon, 2023-02-06 at 08:20 -0500, Mimi Zohar wrote:

quoted

On Fri, 2023-02-03 at 13:56 +0100, Roberto Sassu wrote:

quoted

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.

quoted

+
+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