diff mbox series

[ima-evm-utils,v2] Add tests for MMAP_CHECK and MMAP_CHECK_REQPROT hooks

Message ID 20230131174245.2343342-3-roberto.sassu@huaweicloud.com (mailing list archive)
State Handled Elsewhere
Headers show
Series [ima-evm-utils,v2] Add tests for MMAP_CHECK and MMAP_CHECK_REQPROT hooks | expand

Commit Message

Roberto Sassu Jan. 31, 2023, 5:42 p.m. UTC
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>
---
Changelog

v1:
- Declare PATCHES and uses new expect_pass_if() and expect_fail_if()
  (suggested by Stefan)
- Replace $FAIL with $HARDFAIL when the error occurs in the setup phase of
  the test (to not make an expect_fail test successful for other reasons
  than the focus of the test)
- Declare local variables in one line (suggested by Stefan)
- Print the test being executed at the beginning, so that the message
  always appears also if an error occurs
- Print the result of the tests in a separate line and separate the output
  of each test

 tests/Makefile.am     |   4 +-
 tests/mmap_check.test | 284 ++++++++++++++++++++++++++++++++++++++++++
 tests/test_mmap.c     |  75 +++++++++++
 3 files changed, 362 insertions(+), 1 deletion(-)
 create mode 100755 tests/mmap_check.test
 create mode 100644 tests/test_mmap.c

Comments

Stefan Berger Jan. 31, 2023, 10:39 p.m. UTC | #1
On 1/31/23 12:42, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 


> +check_mmap() {
> +	local hook="$1"
> +	local arg="$2"
> +	local test_file fowner rule result test_file_entry
> +
> +	echo -e "\nTest: ${FUNCNAME[0]} (hook=\"$hook\", test_mmap arg: \"$arg\")"
> +
> +	if ! test_file=$(mktemp -p "$PWD"); then
> +		echo "${RED}Cannot write $test_file${NORM}"
> +		return "$HARDFAIL"
> +	fi
> +
> +	fowner="$MMAP_CHECK_FOWNER"
> +	rule="$MEASURE_MMAP_CHECK_RULE"
> +
> +	if [ "$hook" = "MMAP_CHECK_REQPROT" ]; then
> +		fowner="$MMAP_CHECK_REQPROT_FOWNER"
> +		rule="$MEASURE_MMAP_CHECK_REQPROT_RULE"
> +	fi
> +
> +	if ! chown "$fowner" "$test_file"; then
> +		echo "${RED}Cannot change owner of $test_file${NORM}"
> +		return "$HARDFAIL"
> +	fi
> +
> +	check_load_ima_rule "$rule"
> +	result=$?
> +	if [ $result -ne "$OK" ]; then
> +		return $result
> +	fi
> +
> +	test_mmap "$test_file" "$arg"

In this case it should succeed or fail depending on the $rule?  I am just wondering whether to check $? here as well for expected outcome...

> +
> +	if [ "$TFAIL" != "yes" ]; then
> +		echo -n "Result (expect found): "
> +	else
> +		echo -n "Result (expect not found): "
> +	fi
> +
> +	test_file_entry=$(awk '$5 == "'"$test_file"'"' < /sys/kernel/security/ima/ascii_runtime_measurements)
> +	if [ -z "$test_file_entry" ]; then
> +		echo "not found"
> +		return "$FAIL"
> +	fi
> +
> +	echo "found"
> +	return "$OK"
> +}

> +if [ -n "$TST_KEY_PATH" ]; then
> +	if [ "${TST_KEY_PATH:0:1}" != "/" ]; then
> +		echo "${RED}Absolute path required for the signing key${NORM}"
> +		exit "$FAIL"
> +	fi
> +
> +	if [ ! -f "$TST_KEY_PATH" ]; then
> +		echo "${RED}Kernel signing key not found in $TST_KEY_PATH${NORM}"
> +		exit "$FAIL"
> +	fi
> +
> +	key_path="$TST_KEY_PATH"

g_key_path ? or pass as parameter to check_deny (better IMO)

> +elif [ -f "$PWD/../signing_key.pem" ]; then
> +	key_path="$PWD/../signing_key.pem"
> +elif [ -f "/lib/modules/$(uname -r)/source/certs/signing_key.pem" ]; then
> +	key_path="/lib/modules/$(uname -r)/source/certs/signing_key.pem"
> +elif [ -f "/lib/modules/$(uname -r)/build/certs/signing_key.pem" ]; then
> +	key_path="/lib/modules/$(uname -r)/build/certs/signing_key.pem"
> +else
> +	echo "${CYAN}Kernel signing key not found${NORM}"
> +	exit "$SKIP"
> +fi
> +
> +key_path_der=$(mktemp)

g_key_path_der for consistency

> +++ b/tests/test_mmap.c
> @@ -0,0 +1,75 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Huawei Technologies Duesseldorf GmbH
> + *
> + * Tool to test IMA MMAP_CHECK and MMAP_CHECK_REQPROT hooks.
> + */
> +#include <stdio.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/stat.h>
> +#include <sys/mman.h>
> +#include <sys/personality.h>
> +
> +int main(int argc, char *argv[])
> +{
> +	struct stat st;
> +	void *ptr, *ptr_write = NULL;
> +	int ret, fd, fd_write, prot = PROT_READ;
> +
> +	if (!argv[1])
> +		return -ENOENT;
> +
> +	if (argv[2] && !strcmp(argv[2], "read_implies_exec")) {
> +		ret = personality(READ_IMPLIES_EXEC);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (stat(argv[1], &st) == -1)
> +		return -errno;
> +
> +	if (argv[2] && !strcmp(argv[2], "exec_on_writable")) {
> +		fd_write = open(argv[1], O_RDWR);
> +		if (fd_write == -1)
> +			return -errno;
> +
> +		ptr_write = mmap(0, st.st_size, PROT_WRITE, MAP_SHARED,
> +				 fd_write, 0);
> +		close(fd_write);
> +
> +		if (ptr_write == (void *)-1)
> +			return -errno;
> +	}
> +
> +	fd = open(argv[1], O_RDONLY);
> +	if (fd == -1) {
> +		if (ptr_write)
> +			munmap(ptr_write, st.st_size);
> +
> +		return -errno;
> +	}
> +
> +	if (argv[2] && !strncmp(argv[2], "exec", 4))
> +		prot |= PROT_EXEC;
> +
> +	ptr = mmap(0, st.st_size, prot, MAP_PRIVATE, fd, 0);
> +
> +	close(fd);
> +
> +	if (ptr_write)
> +		munmap(ptr_write, st.st_size);
> +
> +	if (ptr == (void *)-1)
> +		return -errno;
> +
> +	ret = 0;
> +
> +	if (argv[2] && !strcmp(argv[2], "mprotect"))
> +		ret = mprotect(ptr, st.st_size, PROT_EXEC);
> +
> +	munmap(ptr, st.st_size);
> +	return ret;
> +}

Are there any unexpected failure cases here where it should report an error to the user?

    Stefan
Mimi Zohar Feb. 1, 2023, midnight UTC | #2
On Tue, 2023-01-31 at 18:42 +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>

Nice!   Including some comments, or at least the test assumption, would
help simplify reviewing the code.

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Mimi Zohar Feb. 1, 2023, 3:02 a.m. UTC | #3
Hi Roberto,

> +check_mmap() {
> +	local hook="$1"
> +	local arg="$2"
> +	local test_file fowner rule result test_file_entry
> +
> +	echo -e "\nTest: ${FUNCNAME[0]} (hook=\"$hook\", test_mmap arg: \"$arg\")"
> +
> +	if ! test_file=$(mktemp -p "$PWD"); then
> +		echo "${RED}Cannot write $test_file${NORM}"
> +		return "$HARDFAIL"
> +	fi
> +
> +	fowner="$MMAP_CHECK_FOWNER"
> +	rule="$MEASURE_MMAP_CHECK_RULE"
> +
> +	if [ "$hook" = "MMAP_CHECK_REQPROT" ]; then
> +		fowner="$MMAP_CHECK_REQPROT_FOWNER"
> +		rule="$MEASURE_MMAP_CHECK_REQPROT_RULE"
> +	fi
> +
> +	if ! chown "$fowner" "$test_file"; then
> +		echo "${RED}Cannot change owner of $test_file${NORM}"
> +		return "$HARDFAIL"
> +	fi
> +
> +	check_load_ima_rule "$rule"
> +	result=$?
> +	if [ $result -ne "$OK" ]; then
> +		return $result
> +	fi
> +
> +	test_mmap "$test_file" "$arg"
> +
> +	if [ "$TFAIL" != "yes" ]; then
> +		echo -n "Result (expect found): "
> +	else
> +		echo -n "Result (expect not found): "
> +	fi
> +
> +	test_file_entry=$(awk '$5 == "'"$test_file"'"' < /sys/kernel/security/ima/ascii_runtime_measurements)
> +	if [ -z "$test_file_entry" ]; then
> +		echo "not found"
> +		return "$FAIL"
> +	fi

Using temporary files should prevent having multiple records.  Having a
verbose option to show the actual matching measurement list record
would be nice.

> +
> +	echo "found"
> +	return "$OK"
> +}
> +



> +
> +# Run in the new environment if TST_ENV is set.
> +_run_env "$TST_KERNEL" "$PWD/$(basename "$0")" "TST_ENV=$TST_ENV TST_KERNEL=$TST_KERNEL PATH=$PATH LD_LIBRARY_PATH=$LD_LIBRARY_PATH VERBOSE=$VERBOSE"
> +
> +# Exit from the creator of the new environment.
> +_exit_env "$TST_KERNEL"
> +
> +# Mount filesystems in the new environment.
> +_init_env
> +
> +if [ "$(whoami)" != "root" ]; then
> +	echo "${CYAN}This script must be executed as root${NORM}"
> +	exit "$SKIP"
> +fi
> +
> +if [ ! -f /sys/kernel/security/ima/policy ]; then
> +	echo "${CYAN}IMA policy file not found${NORM}"
> +	exit "$SKIP"
> +fi
> +
> +if ! cat /sys/kernel/security/ima/policy &> /dev/null; then
> +	echo "${CYAN}IMA policy file is not readable${NORM}"
> +	exit "$SKIP"
> +fi

An existing policy with an mmap rule would affect this test.  Check to
see if one already exists.

> +
> +if [ -n "$TST_KEY_PATH" ]; then
> +	if [ "${TST_KEY_PATH:0:1}" != "/" ]; then
> +		echo "${RED}Absolute path required for the signing key${NORM}"
> +		exit "$FAIL"
> +	fi
>
Mimi Zohar Feb. 1, 2023, 1:48 p.m. UTC | #4
On Tue, 2023-01-31 at 19:00 -0500, Mimi Zohar wrote:
> On Tue, 2023-01-31 at 18:42 +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>
> 
> Nice!   Including some comments, or at least the test assumption, would
> help simplify reviewing the code.

An example of a test assumption, or background information, for the
"mprotect" test is described in the ima_file_mprotect() function
comment:

 * Files can be mmap'ed read/write and later changed to execute to circumvent
 * IMA's mmap appraisal policy rules.  Due to locking issues (mmap semaphore
 * would be taken before i_mutex), files can not be measured or appraised at
 * this point.  Eliminate this integrity gap by denying the mprotect
 * PROT_EXECUTE change, if an mmap appraise policy rule exists.
Roberto Sassu Feb. 1, 2023, 4:05 p.m. UTC | #5
On Tue, 2023-01-31 at 17:39 -0500, Stefan Berger wrote:
> 
> On 1/31/23 12:42, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > +check_mmap() {
> > +	local hook="$1"
> > +	local arg="$2"
> > +	local test_file fowner rule result test_file_entry
> > +
> > +	echo -e "\nTest: ${FUNCNAME[0]} (hook=\"$hook\", test_mmap arg: \"$arg\")"
> > +
> > +	if ! test_file=$(mktemp -p "$PWD"); then
> > +		echo "${RED}Cannot write $test_file${NORM}"
> > +		return "$HARDFAIL"
> > +	fi
> > +
> > +	fowner="$MMAP_CHECK_FOWNER"
> > +	rule="$MEASURE_MMAP_CHECK_RULE"
> > +
> > +	if [ "$hook" = "MMAP_CHECK_REQPROT" ]; then
> > +		fowner="$MMAP_CHECK_REQPROT_FOWNER"
> > +		rule="$MEASURE_MMAP_CHECK_REQPROT_RULE"
> > +	fi
> > +
> > +	if ! chown "$fowner" "$test_file"; then
> > +		echo "${RED}Cannot change owner of $test_file${NORM}"
> > +		return "$HARDFAIL"
> > +	fi
> > +
> > +	check_load_ima_rule "$rule"
> > +	result=$?
> > +	if [ $result -ne "$OK" ]; then
> > +		return $result
> > +	fi
> > +
> > +	test_mmap "$test_file" "$arg"
> 
> In this case it should succeed or fail depending on the $rule?  I am just wondering whether to check $? here as well for expected outcome...

I agree. For the check_mmap() test, test_mmap is always expected to
succeed.

> > +
> > +	if [ "$TFAIL" != "yes" ]; then
> > +		echo -n "Result (expect found): "
> > +	else
> > +		echo -n "Result (expect not found): "
> > +	fi
> > +
> > +	test_file_entry=$(awk '$5 == "'"$test_file"'"' < /sys/kernel/security/ima/ascii_runtime_measurements)
> > +	if [ -z "$test_file_entry" ]; then
> > +		echo "not found"
> > +		return "$FAIL"
> > +	fi
> > +
> > +	echo "found"
> > +	return "$OK"
> > +}
> > +if [ -n "$TST_KEY_PATH" ]; then
> > +	if [ "${TST_KEY_PATH:0:1}" != "/" ]; then
> > +		echo "${RED}Absolute path required for the signing key${NORM}"
> > +		exit "$FAIL"
> > +	fi
> > +
> > +	if [ ! -f "$TST_KEY_PATH" ]; then
> > +		echo "${RED}Kernel signing key not found in $TST_KEY_PATH${NORM}"
> > +		exit "$FAIL"
> > +	fi
> > +
> > +	key_path="$TST_KEY_PATH"
> 
> g_key_path ? or pass as parameter to check_deny (better IMO)

There are other global variables. Also the expect_ lines now are clean
and say more or less what the test is about. Maybe better g_key_path,
will think about for new tests.

> > +elif [ -f "$PWD/../signing_key.pem" ]; then
> > +	key_path="$PWD/../signing_key.pem"
> > +elif [ -f "/lib/modules/$(uname -r)/source/certs/signing_key.pem" ]; then
> > +	key_path="/lib/modules/$(uname -r)/source/certs/signing_key.pem"
> > +elif [ -f "/lib/modules/$(uname -r)/build/certs/signing_key.pem" ]; then
> > +	key_path="/lib/modules/$(uname -r)/build/certs/signing_key.pem"
> > +else
> > +	echo "${CYAN}Kernel signing key not found${NORM}"
> > +	exit "$SKIP"
> > +fi
> > +
> > +key_path_der=$(mktemp)
> 
> g_key_path_der for consistency

Ok.

> > +++ b/tests/test_mmap.c
> > @@ -0,0 +1,75 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2023 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Tool to test IMA MMAP_CHECK and MMAP_CHECK_REQPROT hooks.
> > + */
> > +#include <stdio.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +#include <sys/stat.h>
> > +#include <sys/mman.h>
> > +#include <sys/personality.h>
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +	struct stat st;
> > +	void *ptr, *ptr_write = NULL;
> > +	int ret, fd, fd_write, prot = PROT_READ;
> > +
> > +	if (!argv[1])
> > +		return -ENOENT;
> > +
> > +	if (argv[2] && !strcmp(argv[2], "read_implies_exec")) {
> > +		ret = personality(READ_IMPLIES_EXEC);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	if (stat(argv[1], &st) == -1)
> > +		return -errno;
> > +
> > +	if (argv[2] && !strcmp(argv[2], "exec_on_writable")) {
> > +		fd_write = open(argv[1], O_RDWR);
> > +		if (fd_write == -1)
> > +			return -errno;
> > +
> > +		ptr_write = mmap(0, st.st_size, PROT_WRITE, MAP_SHARED,
> > +				 fd_write, 0);
> > +		close(fd_write);
> > +
> > +		if (ptr_write == (void *)-1)
> > +			return -errno;
> > +	}
> > +
> > +	fd = open(argv[1], O_RDONLY);
> > +	if (fd == -1) {
> > +		if (ptr_write)
> > +			munmap(ptr_write, st.st_size);
> > +
> > +		return -errno;
> > +	}
> > +
> > +	if (argv[2] && !strncmp(argv[2], "exec", 4))
> > +		prot |= PROT_EXEC;
> > +
> > +	ptr = mmap(0, st.st_size, prot, MAP_PRIVATE, fd, 0);
> > +
> > +	close(fd);
> > +
> > +	if (ptr_write)
> > +		munmap(ptr_write, st.st_size);
> > +
> > +	if (ptr == (void *)-1)
> > +		return -errno;
> > +
> > +	ret = 0;
> > +
> > +	if (argv[2] && !strcmp(argv[2], "mprotect"))
> > +		ret = mprotect(ptr, st.st_size, PROT_EXEC);
> > +
> > +	munmap(ptr, st.st_size);
> > +	return ret;
> > +}
> 
> Are there any unexpected failure cases here where it should report an error to the user?

Uhm, ok. I differentiated when an error could occur from when it should
not.

Roberto
Roberto Sassu Feb. 1, 2023, 4:06 p.m. UTC | #6
On Wed, 2023-02-01 at 08:48 -0500, Mimi Zohar wrote:
> On Tue, 2023-01-31 at 19:00 -0500, Mimi Zohar wrote:
> > On Tue, 2023-01-31 at 18:42 +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>
> > 
> > Nice!   Including some comments, or at least the test assumption, would
> > help simplify reviewing the code.
> 
> An example of a test assumption, or background information, for the
> "mprotect" test is described in the ima_file_mprotect() function
> comment:
> 
>  * Files can be mmap'ed read/write and later changed to execute to circumvent
>  * IMA's mmap appraisal policy rules.  Due to locking issues (mmap semaphore
>  * would be taken before i_mutex), files can not be measured or appraised at
>  * this point.  Eliminate this integrity gap by denying the mprotect
>  * PROT_EXECUTE change, if an mmap appraise policy rule exists.

Ok, I added the description to each test. Will send the new patch
shortly.

Roberto
Roberto Sassu Feb. 1, 2023, 5:29 p.m. UTC | #7
On Tue, 2023-01-31 at 22:02 -0500, Mimi Zohar wrote:
> Hi Roberto,
> 
> > +check_mmap() {
> > +	local hook="$1"
> > +	local arg="$2"
> > +	local test_file fowner rule result test_file_entry
> > +
> > +	echo -e "\nTest: ${FUNCNAME[0]} (hook=\"$hook\", test_mmap arg: \"$arg\")"
> > +
> > +	if ! test_file=$(mktemp -p "$PWD"); then
> > +		echo "${RED}Cannot write $test_file${NORM}"
> > +		return "$HARDFAIL"
> > +	fi
> > +
> > +	fowner="$MMAP_CHECK_FOWNER"
> > +	rule="$MEASURE_MMAP_CHECK_RULE"
> > +
> > +	if [ "$hook" = "MMAP_CHECK_REQPROT" ]; then
> > +		fowner="$MMAP_CHECK_REQPROT_FOWNER"
> > +		rule="$MEASURE_MMAP_CHECK_REQPROT_RULE"
> > +	fi
> > +
> > +	if ! chown "$fowner" "$test_file"; then
> > +		echo "${RED}Cannot change owner of $test_file${NORM}"
> > +		return "$HARDFAIL"
> > +	fi
> > +
> > +	check_load_ima_rule "$rule"
> > +	result=$?
> > +	if [ $result -ne "$OK" ]; then
> > +		return $result
> > +	fi
> > +
> > +	test_mmap "$test_file" "$arg"
> > +
> > +	if [ "$TFAIL" != "yes" ]; then
> > +		echo -n "Result (expect found): "
> > +	else
> > +		echo -n "Result (expect not found): "
> > +	fi
> > +
> > +	test_file_entry=$(awk '$5 == "'"$test_file"'"' < /sys/kernel/security/ima/ascii_runtime_measurements)
> > +	if [ -z "$test_file_entry" ]; then
> > +		echo "not found"
> > +		return "$FAIL"
> > +	fi
> 
> Using temporary files should prevent having multiple records.  Having a
> verbose option to show the actual matching measurement list record
> would be nice.
> 
> > +
> > +	echo "found"
> > +	return "$OK"
> > +}
> > +
> 
> 
> > +
> > +# Run in the new environment if TST_ENV is set.
> > +_run_env "$TST_KERNEL" "$PWD/$(basename "$0")" "TST_ENV=$TST_ENV TST_KERNEL=$TST_KERNEL PATH=$PATH LD_LIBRARY_PATH=$LD_LIBRARY_PATH VERBOSE=$VERBOSE"
> > +
> > +# Exit from the creator of the new environment.
> > +_exit_env "$TST_KERNEL"
> > +
> > +# Mount filesystems in the new environment.
> > +_init_env
> > +
> > +if [ "$(whoami)" != "root" ]; then
> > +	echo "${CYAN}This script must be executed as root${NORM}"
> > +	exit "$SKIP"
> > +fi
> > +
> > +if [ ! -f /sys/kernel/security/ima/policy ]; then
> > +	echo "${CYAN}IMA policy file not found${NORM}"
> > +	exit "$SKIP"
> > +fi
> > +
> > +if ! cat /sys/kernel/security/ima/policy &> /dev/null; then
> > +	echo "${CYAN}IMA policy file is not readable${NORM}"
> > +	exit "$SKIP"
> > +fi
> 
> An existing policy with an mmap rule would affect this test.  Check to
> see if one already exists.

Not sure about this. We are specifying the filesystem UUID and the file
owner. Should be enough to avoid interferences. Also
check_load_ima_rule() avoids duplicated rules.

Roberto

> > +
> > +if [ -n "$TST_KEY_PATH" ]; then
> > +	if [ "${TST_KEY_PATH:0:1}" != "/" ]; then
> > +		echo "${RED}Absolute path required for the signing key${NORM}"
> > +		exit "$FAIL"
> > +	fi
> >
Mimi Zohar Feb. 1, 2023, 5:54 p.m. UTC | #8
On Wed, 2023-02-01 at 18:29 +0100, Roberto Sassu wrote:
> On Tue, 2023-01-31 at 22:02 -0500, Mimi Zohar wrote:
> > Hi Roberto,
> > 
> > > +check_mmap() {
> > > +	local hook="$1"
> > > +	local arg="$2"
> > > +	local test_file fowner rule result test_file_entry
> > > +
> > > +	echo -e "\nTest: ${FUNCNAME[0]} (hook=\"$hook\", test_mmap arg: \"$arg\")"
> > > +
> > > +	if ! test_file=$(mktemp -p "$PWD"); then
> > > +		echo "${RED}Cannot write $test_file${NORM}"
> > > +		return "$HARDFAIL"
> > > +	fi
> > > +
> > > +	fowner="$MMAP_CHECK_FOWNER"
> > > +	rule="$MEASURE_MMAP_CHECK_RULE"
> > > +
> > > +	if [ "$hook" = "MMAP_CHECK_REQPROT" ]; then
> > > +		fowner="$MMAP_CHECK_REQPROT_FOWNER"
> > > +		rule="$MEASURE_MMAP_CHECK_REQPROT_RULE"
> > > +	fi
> > > +
> > > +	if ! chown "$fowner" "$test_file"; then
> > > +		echo "${RED}Cannot change owner of $test_file${NORM}"
> > > +		return "$HARDFAIL"
> > > +	fi
> > > +
> > > +	check_load_ima_rule "$rule"
> > > +	result=$?
> > > +	if [ $result -ne "$OK" ]; then
> > > +		return $result
> > > +	fi
> > > +
> > > +	test_mmap "$test_file" "$arg"
> > > +
> > > +	if [ "$TFAIL" != "yes" ]; then
> > > +		echo -n "Result (expect found): "
> > > +	else
> > > +		echo -n "Result (expect not found): "
> > > +	fi
> > > +
> > > +	test_file_entry=$(awk '$5 == "'"$test_file"'"' < /sys/kernel/security/ima/ascii_runtime_measurements)
> > > +	if [ -z "$test_file_entry" ]; then
> > > +		echo "not found"
> > > +		return "$FAIL"
> > > +	fi
> > 
> > Using temporary files should prevent having multiple records.  Having a
> > verbose option to show the actual matching measurement list record
> > would be nice.
> > 
> > > +
> > > +	echo "found"
> > > +	return "$OK"
> > > +}
> > > +
> > 
> > 
> > > +
> > > +# Run in the new environment if TST_ENV is set.
> > > +_run_env "$TST_KERNEL" "$PWD/$(basename "$0")" "TST_ENV=$TST_ENV TST_KERNEL=$TST_KERNEL PATH=$PATH LD_LIBRARY_PATH=$LD_LIBRARY_PATH VERBOSE=$VERBOSE"
> > > +
> > > +# Exit from the creator of the new environment.
> > > +_exit_env "$TST_KERNEL"
> > > +
> > > +# Mount filesystems in the new environment.
> > > +_init_env
> > > +
> > > +if [ "$(whoami)" != "root" ]; then
> > > +	echo "${CYAN}This script must be executed as root${NORM}"
> > > +	exit "$SKIP"
> > > +fi
> > > +
> > > +if [ ! -f /sys/kernel/security/ima/policy ]; then
> > > +	echo "${CYAN}IMA policy file not found${NORM}"
> > > +	exit "$SKIP"
> > > +fi
> > > +
> > > +if ! cat /sys/kernel/security/ima/policy &> /dev/null; then
> > > +	echo "${CYAN}IMA policy file is not readable${NORM}"
> > > +	exit "$SKIP"
> > > +fi
> > 
> > An existing policy with an mmap rule would affect this test.  Check to
> > see if one already exists.
> 
> Not sure about this. We are specifying the filesystem UUID and the file
> owner. Should be enough to avoid interferences. Also
> check_load_ima_rule() avoids duplicated rules.

Since the policy rules are walked sequentially, existing mmap rules
without the uuid or owner qualifiers will interfere with the test
policy rules.   To see the problem load a custom policy containing an
mmap rule, without any qualifiers.  Then run the mmap_check test.

> 
> > > +
> > > +if [ -n "$TST_KEY_PATH" ]; then
> > > +	if [ "${TST_KEY_PATH:0:1}" != "/" ]; then
> > > +		echo "${RED}Absolute path required for the signing key${NORM}"
> > > +		exit "$FAIL"
> > > +	fi
> > > 
>
diff mbox series

Patch

diff --git a/tests/Makefile.am b/tests/Makefile.am
index a0463b7b5b5d..ca9c4ca18380 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -2,7 +2,9 @@  check_SCRIPTS =
 TESTS = $(check_SCRIPTS)
 
 check_SCRIPTS += ima_hash.test sign_verify.test boot_aggregate.test \
-		 fsverity.test portable_signatures.test
+		 fsverity.test portable_signatures.test mmap_check.test
+
+check_PROGRAMS := test_mmap
 
 .PHONY: check_logs
 check_logs:
diff --git a/tests/mmap_check.test b/tests/mmap_check.test
new file mode 100755
index 000000000000..a045ee867437
--- /dev/null
+++ b/tests/mmap_check.test
@@ -0,0 +1,284 @@ 
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (C) 2022-2023 Roberto Sassu <roberto.sassu@huawei.com>
+#
+# Check the behavior of MMAP_CHECK and MMAP_CHECK_REQPROT
+
+trap '_report_exit_and_cleanup _cleanup_env cleanup' SIGINT SIGTERM SIGSEGV EXIT
+
+PATCHES=(
+'ima: Align ima_file_mmap() parameters with mmap_file LSM hook'
+'ima: Introduce MMAP_CHECK_REQPROT hook'
+)
+
+# Base VERBOSE on the environment variable, if set.
+VERBOSE="${VERBOSE:-0}"
+
+cd "$(dirname "$0")" || exit "$FAIL"
+export PATH=$PWD/../src:$PWD:$PATH
+export LD_LIBRARY_PATH=$LD_LIBRARY_PATH
+. ./functions.sh
+_require evmctl
+
+cleanup() {
+	if [ "$g_loop_mounted" = "1" ]; then
+		popd > /dev/null || exit "$FAIL"
+		umount "$g_mountpoint"
+	fi
+
+	if [ -n "$g_dev" ]; then
+		losetup -d "$g_dev"
+	fi
+
+	if [ -n "$g_image" ]; then
+		rm -f "$g_image"
+	fi
+
+	if [ -n "$g_mountpoint" ]; then
+		rm -Rf "$g_mountpoint"
+	fi
+
+	if [ -n "$key_path_der" ]; then
+		rm -f "$key_path_der"
+	fi
+}
+
+# Use the fsuuid= IMA policy keyword to select only files created/used by the
+# tests below. Also use fowner= to differentiate between files created/used by
+# individual tests.
+IMA_UUID="28b23254-9467-44c0-b6ba-34b12e85a26e"
+MMAP_CHECK_FOWNER=2000
+MMAP_CHECK_REQPROT_FOWNER=2001
+MEASURE_MMAP_CHECK_RULE="measure func=MMAP_CHECK fsuuid=$IMA_UUID fowner=$MMAP_CHECK_FOWNER"
+MEASURE_MMAP_CHECK_REQPROT_RULE="measure func=MMAP_CHECK_REQPROT fsuuid=$IMA_UUID fowner=$MMAP_CHECK_REQPROT_FOWNER"
+APPRAISE_MMAP_CHECK_RULE="appraise func=MMAP_CHECK fsuuid=$IMA_UUID fowner=$MMAP_CHECK_FOWNER"
+APPRAISE_MMAP_CHECK_REQPROT_RULE="appraise func=MMAP_CHECK_REQPROT fsuuid=$IMA_UUID fowner=$MMAP_CHECK_REQPROT_FOWNER"
+
+check_load_ima_rule() {
+	local rule_loaded result new_policy
+
+	rule_loaded=$(grep "$1" /sys/kernel/security/ima/policy)
+	if [ -z "$rule_loaded" ]; then
+		new_policy=$(mktemp -p "$g_mountpoint")
+		echo "$1" > "$new_policy"
+		echo "$new_policy" > /sys/kernel/security/ima/policy
+		result=$?
+		rm -f "$new_policy"
+
+		if [ "$result" -ne 0 ]; then
+			echo "${RED}Failed to set IMA policy${NORM}"
+			return "$HARDFAIL"
+		fi
+	fi
+
+	return "$OK"
+}
+
+check_mmap() {
+	local hook="$1"
+	local arg="$2"
+	local test_file fowner rule result test_file_entry
+
+	echo -e "\nTest: ${FUNCNAME[0]} (hook=\"$hook\", test_mmap arg: \"$arg\")"
+
+	if ! test_file=$(mktemp -p "$PWD"); then
+		echo "${RED}Cannot write $test_file${NORM}"
+		return "$HARDFAIL"
+	fi
+
+	fowner="$MMAP_CHECK_FOWNER"
+	rule="$MEASURE_MMAP_CHECK_RULE"
+
+	if [ "$hook" = "MMAP_CHECK_REQPROT" ]; then
+		fowner="$MMAP_CHECK_REQPROT_FOWNER"
+		rule="$MEASURE_MMAP_CHECK_REQPROT_RULE"
+	fi
+
+	if ! chown "$fowner" "$test_file"; then
+		echo "${RED}Cannot change owner of $test_file${NORM}"
+		return "$HARDFAIL"
+	fi
+
+	check_load_ima_rule "$rule"
+	result=$?
+	if [ $result -ne "$OK" ]; then
+		return $result
+	fi
+
+	test_mmap "$test_file" "$arg"
+
+	if [ "$TFAIL" != "yes" ]; then
+		echo -n "Result (expect found): "
+	else
+		echo -n "Result (expect not found): "
+	fi
+
+	test_file_entry=$(awk '$5 == "'"$test_file"'"' < /sys/kernel/security/ima/ascii_runtime_measurements)
+	if [ -z "$test_file_entry" ]; then
+		echo "not found"
+		return "$FAIL"
+	fi
+
+	echo "found"
+	return "$OK"
+}
+
+check_deny() {
+	local hook="$1"
+	local arg="$2"
+	local test_file fowner rule result
+
+	echo -e "\nTest: ${FUNCNAME[0]} (hook=\"$hook\", test_mmap arg: \"$arg\")"
+
+	if ! test_file=$(mktemp -p "$PWD"); then
+		echo "${RED}Cannot write $test_file${NORM}"
+		return "$HARDFAIL"
+	fi
+
+	echo "test" > "$test_file"
+
+	if ! evmctl ima_sign -a sha256 --key "$key_path" "$test_file" &> /dev/null; then
+		echo "${RED}Cannot sign $test_file${NORM}"
+		return "$HARDFAIL"
+	fi
+
+	fowner="$MMAP_CHECK_FOWNER"
+	rule="$APPRAISE_MMAP_CHECK_RULE"
+
+	if [ "$hook" = "MMAP_CHECK_REQPROT" ]; then
+		fowner="$MMAP_CHECK_REQPROT_FOWNER"
+		rule="$APPRAISE_MMAP_CHECK_REQPROT_RULE"
+	fi
+
+	if ! chown "$fowner" "$test_file"; then
+		echo "${RED}Cannot change owner of $test_file${NORM}"
+		return "$HARDFAIL"
+	fi
+
+	check_load_ima_rule "$rule"
+	result=$?
+	if [ $result -ne "$OK" ]; then
+		return $result
+	fi
+
+	if ! test_mmap "$test_file" exec > /dev/null; then
+		echo "${RED}Cannot read $test_file${NORM}"
+		return "$HARDFAIL"
+	fi
+
+	if [ "$TFAIL" != "yes" ]; then
+		echo -n "Result (expect denied): "
+	else
+		echo -n "Result (expect allowed): "
+	fi
+
+	if test_mmap "$test_file" "$arg"; then
+		echo "allowed"
+		return "$FAIL"
+	fi
+
+	echo "denied"
+	return "$OK"
+}
+
+# Run in the new environment if TST_ENV is set.
+_run_env "$TST_KERNEL" "$PWD/$(basename "$0")" "TST_ENV=$TST_ENV TST_KERNEL=$TST_KERNEL PATH=$PATH LD_LIBRARY_PATH=$LD_LIBRARY_PATH VERBOSE=$VERBOSE"
+
+# Exit from the creator of the new environment.
+_exit_env "$TST_KERNEL"
+
+# Mount filesystems in the new environment.
+_init_env
+
+if [ "$(whoami)" != "root" ]; then
+	echo "${CYAN}This script must be executed as root${NORM}"
+	exit "$SKIP"
+fi
+
+if [ ! -f /sys/kernel/security/ima/policy ]; then
+	echo "${CYAN}IMA policy file not found${NORM}"
+	exit "$SKIP"
+fi
+
+if ! cat /sys/kernel/security/ima/policy &> /dev/null; then
+	echo "${CYAN}IMA policy file is not readable${NORM}"
+	exit "$SKIP"
+fi
+
+if [ -n "$TST_KEY_PATH" ]; then
+	if [ "${TST_KEY_PATH:0:1}" != "/" ]; then
+		echo "${RED}Absolute path required for the signing key${NORM}"
+		exit "$FAIL"
+	fi
+
+	if [ ! -f "$TST_KEY_PATH" ]; then
+		echo "${RED}Kernel signing key not found in $TST_KEY_PATH${NORM}"
+		exit "$FAIL"
+	fi
+
+	key_path="$TST_KEY_PATH"
+elif [ -f "$PWD/../signing_key.pem" ]; then
+	key_path="$PWD/../signing_key.pem"
+elif [ -f "/lib/modules/$(uname -r)/source/certs/signing_key.pem" ]; then
+	key_path="/lib/modules/$(uname -r)/source/certs/signing_key.pem"
+elif [ -f "/lib/modules/$(uname -r)/build/certs/signing_key.pem" ]; then
+	key_path="/lib/modules/$(uname -r)/build/certs/signing_key.pem"
+else
+	echo "${CYAN}Kernel signing key not found${NORM}"
+	exit "$SKIP"
+fi
+
+key_path_der=$(mktemp)
+
+openssl x509 -in "$key_path" -out "$key_path_der" -outform der
+if ! keyctl padd asymmetric pubkey %keyring:.ima < "$key_path_der" &> /dev/null; then
+	echo "${RED}Public key cannot be added to the IMA keyring${NORM}"
+	exit "$FAIL"
+fi
+
+g_mountpoint=$(mktemp -d)
+g_image=$(mktemp)
+
+if [ -z "$g_mountpoint" ]; then
+	echo "${RED}Mountpoint directory not created${NORM}"
+	exit "$FAIL"
+fi
+
+if ! dd if=/dev/zero of="$g_image" bs=1M count=20 &> /dev/null; then
+	echo "${RED}Cannot create test image${NORM}"
+	exit "$FAIL"
+fi
+
+g_dev=$(losetup -f "$g_image" --show)
+if [ -z "$g_dev" ]; then
+	echo "${RED}Cannot create loop device${NORM}"
+	exit "$FAIL"
+fi
+
+if ! mkfs.ext4 -U "$IMA_UUID" -b 4096 "$g_dev" &> /dev/null; then
+	echo "${RED}Cannot format $g_dev${NORM}"
+	exit "$FAIL"
+fi
+
+if ! mount -o i_version "$g_dev" "$g_mountpoint"; then
+	echo "${RED}Cannot mount loop device${NORM}"
+	exit "$FAIL"
+fi
+
+g_loop_mounted=1
+pushd "$g_mountpoint" > /dev/null || exit "$FAIL"
+
+expect_fail check_mmap "MMAP_CHECK" ""
+expect_pass check_mmap "MMAP_CHECK" "exec"
+expect_pass_if '0' check_mmap "MMAP_CHECK" "read_implies_exec"
+
+expect_fail_if '1' check_mmap "MMAP_CHECK_REQPROT" ""
+expect_pass_if '1' check_mmap "MMAP_CHECK_REQPROT" "exec"
+expect_fail_if '1' check_mmap "MMAP_CHECK_REQPROT" "read_implies_exec"
+
+expect_pass check_deny "MMAP_CHECK" "mprotect"
+expect_pass_if '1' check_deny "MMAP_CHECK_REQPROT" "mprotect"
+
+expect_pass check_deny "MMAP_CHECK" "exec_on_writable"
+expect_pass_if '1' check_deny "MMAP_CHECK_REQPROT" "exec_on_writable"
diff --git a/tests/test_mmap.c b/tests/test_mmap.c
new file mode 100644
index 000000000000..6d1beb3bb6a2
--- /dev/null
+++ b/tests/test_mmap.c
@@ -0,0 +1,75 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 Huawei Technologies Duesseldorf GmbH
+ *
+ * Tool to test IMA MMAP_CHECK and MMAP_CHECK_REQPROT hooks.
+ */
+#include <stdio.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/mman.h>
+#include <sys/personality.h>
+
+int main(int argc, char *argv[])
+{
+	struct stat st;
+	void *ptr, *ptr_write = NULL;
+	int ret, fd, fd_write, prot = PROT_READ;
+
+	if (!argv[1])
+		return -ENOENT;
+
+	if (argv[2] && !strcmp(argv[2], "read_implies_exec")) {
+		ret = personality(READ_IMPLIES_EXEC);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (stat(argv[1], &st) == -1)
+		return -errno;
+
+	if (argv[2] && !strcmp(argv[2], "exec_on_writable")) {
+		fd_write = open(argv[1], O_RDWR);
+		if (fd_write == -1)
+			return -errno;
+
+		ptr_write = mmap(0, st.st_size, PROT_WRITE, MAP_SHARED,
+				 fd_write, 0);
+		close(fd_write);
+
+		if (ptr_write == (void *)-1)
+			return -errno;
+	}
+
+	fd = open(argv[1], O_RDONLY);
+	if (fd == -1) {
+		if (ptr_write)
+			munmap(ptr_write, st.st_size);
+
+		return -errno;
+	}
+
+	if (argv[2] && !strncmp(argv[2], "exec", 4))
+		prot |= PROT_EXEC;
+
+	ptr = mmap(0, st.st_size, prot, MAP_PRIVATE, fd, 0);
+
+	close(fd);
+
+	if (ptr_write)
+		munmap(ptr_write, st.st_size);
+
+	if (ptr == (void *)-1)
+		return -errno;
+
+	ret = 0;
+
+	if (argv[2] && !strcmp(argv[2], "mprotect"))
+		ret = mprotect(ptr, st.st_size, PROT_EXEC);
+
+	munmap(ptr, st.st_size);
+	return ret;
+}