[v2,5/5] selftests/ima: loading kernel modules
diff mbox series

Message ID 1551223620-11586-6-git-send-email-zohar@linux.ibm.com
State New
Headers show
Series
  • selftests/ima: add kexec and kernel module tests
Related show

Commit Message

Mimi Zohar Feb. 26, 2019, 11:27 p.m. UTC
While the appended kernel module signature can be verified, when loading
a kernel module via either the init_module or the finit_module syscall,
verifying the IMA signature requires access to the file descriptor,
which is only available via the finit_module syscall.  As "modprobe"
does not provide a flag allowing the syscall - init_module or
finit_module - to be specified, this patch does not load a kernel
module.

This test simply verifies that on secure boot enabled systems with
"CONFIG_IMA_ARCH_POLICY" configured, that at least an appended kernel
module signature or an IMA signature is required based on the Kconfig
and the runtime IMA policy.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 tools/testing/selftests/ima/Makefile              |  2 +-
 tools/testing/selftests/ima/test_kernel_module.sh | 96 +++++++++++++++++++++++
 2 files changed, 97 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/ima/test_kernel_module.sh

Comments

shuah Feb. 27, 2019, 1:59 a.m. UTC | #1
On 2/26/19 4:27 PM, Mimi Zohar wrote:
> While the appended kernel module signature can be verified, when loading
> a kernel module via either the init_module or the finit_module syscall,
> verifying the IMA signature requires access to the file descriptor,
> which is only available via the finit_module syscall.  As "modprobe"
> does not provide a flag allowing the syscall - init_module or
> finit_module - to be specified, this patch does not load a kernel
> module.
> 
> This test simply verifies that on secure boot enabled systems with
> "CONFIG_IMA_ARCH_POLICY" configured, that at least an appended kernel
> module signature or an IMA signature is required based on the Kconfig
> and the runtime IMA policy.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   tools/testing/selftests/ima/Makefile              |  2 +-
>   tools/testing/selftests/ima/test_kernel_module.sh | 96 +++++++++++++++++++++++
>   2 files changed, 97 insertions(+), 1 deletion(-)
>   create mode 100755 tools/testing/selftests/ima/test_kernel_module.sh
> 
> diff --git a/tools/testing/selftests/ima/Makefile b/tools/testing/selftests/ima/Makefile
> index 049c83c9426c..ef5201ff0bea 100644
> --- a/tools/testing/selftests/ima/Makefile
> +++ b/tools/testing/selftests/ima/Makefile
> @@ -4,7 +4,7 @@ uname_M := $(shell uname -m 2>/dev/null || echo not)
>   ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/)
>   
>   ifeq ($(ARCH),x86)
> -TEST_PROGS := test_kexec_load.sh test_kexec_file_load.sh
> +TEST_PROGS := test_kexec_load.sh test_kexec_file_load.sh test_kernel_module.sh
>   TEST_FILES := common_lib.sh
>   
>   include ../lib.mk
> diff --git a/tools/testing/selftests/ima/test_kernel_module.sh b/tools/testing/selftests/ima/test_kernel_module.sh
> new file mode 100755
> index 000000000000..4009e1b60b03
> --- /dev/null
> +++ b/tools/testing/selftests/ima/test_kernel_module.sh
> @@ -0,0 +1,96 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later

Same here

# SPDX-License-Identifier: GPL-2.0

> +#
> +# On secure boot enabled systems with "CONFIG_IMA_ARCH_POLICY" configured,
> +# this test verifies that at least an appended kernel module signature or
> +# an IMA signature is required.  It does not attempt to load a kernel module.
> +
> +TEST="KERNEL_MODULE"
> +. ./common_lib.sh
> +
> +trap "{ rm -f $IKCONFIG ; }" EXIT
> +
> +# Some of the IMA builtin policies may require the kernel modules to
> +# be signed, but these policy rules may be replaced with a custom
> +# policy.  Only CONFIG_IMA_APPRAISE_REQUIRE_MODULE_SIGS persists after
> +# loading a custom policy.  Check if it is enabled, before reading the
> +# IMA runtime sysfs policy file.
> +# Return 1 for IMA signature required and 0 for not required.
> +is_ima_sig_required()
> +{
> +	local ret=0
> +
> +	kconfig_enabled "CONFIG_IMA_APPRAISE_REQUIRE_MODULE_SIGS=y" \
> +		"IMA kernel module signature required"
> +	if [ $? -eq 1 ]; then
> +		log_info "IMA kernel module signature required"
> +		return 1
> +	fi
> +
> +	# The architecture specific or a custom policy may require the
> +	# kernel module to be signed.  Policy rules are walked sequentially.
> +	# As a result, a policy rule may be defined, but might not necessarily
> +	# be used.  This test assumes if a policy rule is specified, that is
> +	# the intent.
> +	if [ $ima_read_policy -eq 1 ]; then
> +		check_ima_policy "appraise" "func=MODULE_CHECK" \
> +			"appraise_type=imasig"
> +		ret=$?
> +		[ $ret -eq 1 ] && log_info "IMA signature required";
> +	fi
> +	return $ret
> +}
> +
> +# loading kernel modules requires root privileges
> +if [ $(id -ru) -ne 0 ]; then
> +	log_skip "requires root privileges"
> +fi
> +
> +# Are appended signatures required?
> +if [ -e /sys/module/module/parameters/sig_enforce ]; then
> +	sig_enforce=$(cat /sys/module/module/parameters/sig_enforce)
> +	if [ $sig_enforce = "Y" ]; then
> +		log_pass "appended kernel module signature required"
> +	fi
> +fi
> +
> +get_secureboot_mode
> +if [ $? -eq 0 ]; then
> +	log_skip "secure boot not enabled"
> +fi
> +
> +# get the kernel config
> +get_kconfig
> +

get_kconfig() will be good candidate as a kselftest common
function. Is that possible?

> +# Determine which kernel config options are enabled
> +kconfig_enabled "CONFIG_IMA_ARCH_POLICY=y" \
> +	"architecture specific policy enabled"
> +arch_policy=$?
> +
> +kconfig_enabled "CONFIG_MODULE_SIG=y" \
> +	"appended kernel modules signature enabled"
> +appended_sig_enabled=$?
> +
> +kconfig_enabled "CONFIG_IMA_READ_POLICY=y" "reading IMA policy permitted"
> +ima_read_policy=$?
> +
> +is_ima_sig_required
> +ima_sig_required=$?
> +
> +if [ $arch_policy -eq 0 ]; then
> +	log_skip "architecture specific policy not enabled"
> +fi
> +
> +if [ $appended_sig_enabled -eq 1 ]; then
> +	log_fail "appended kernel module signature enabled, but not required"
> +fi
> +
> +if [ $ima_sig_required -eq 1 ]; then
> +	log_pass "IMA kernel module signature required"
> +fi
> +
> +if [ $ima_read_policy -eq 1 ]; then
> +	log_fail "IMA kernel module signature not required"
> +else
> +	log_skip "reading IMA policy not permitted"
> +fi
> 

thanks,
-- Shuah
Mimi Zohar Feb. 27, 2019, 2:14 p.m. UTC | #2
Hi Shuah,

> > diff --git a/tools/testing/selftests/ima/test_kernel_module.sh b/tools/testing/selftests/ima/test_kernel_module.sh
> > new file mode 100755
> > index 000000000000..4009e1b60b03
> > --- /dev/null
> > +++ b/tools/testing/selftests/ima/test_kernel_module.sh
> > @@ -0,0 +1,96 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> 
> Same here
> 
> # SPDX-License-Identifier: GPL-2.0

Sure, I'll make the change here and in the other places.


> > +get_secureboot_mode
> > +if [ $? -eq 0 ]; then
> > +	log_skip "secure boot not enabled"
> > +fi
> > +
> > +# get the kernel config
> > +get_kconfig
> > +
> 
> get_kconfig() will be good candidate as a kselftest common
> function. Is that possible?

Sure, where would it go?  get_kconfig calls log_skip.  I didn't see
any common logging functions.  Petr suggested defining a set of common
logging functions.  Did you want to only make "log_skip" a common
function or the other logging functions log_pass, log_fail, log_info
as well?

Thanks,

Mimi
shuah Feb. 27, 2019, 3:33 p.m. UTC | #3
Hi Mimi,

On 2/27/19 7:14 AM, Mimi Zohar wrote:
> Hi Shuah,
> 
>>> diff --git a/tools/testing/selftests/ima/test_kernel_module.sh b/tools/testing/selftests/ima/test_kernel_module.sh
>>> new file mode 100755
>>> index 000000000000..4009e1b60b03
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/ima/test_kernel_module.sh
>>> @@ -0,0 +1,96 @@
>>> +#!/bin/sh
>>> +# SPDX-License-Identifier: GPL-2.0-or-later
>>
>> Same here
>>
>> # SPDX-License-Identifier: GPL-2.0
> 
> Sure, I'll make the change here and in the other places.
> 
> 

Thanks.

>>> +get_secureboot_mode
>>> +if [ $? -eq 0 ]; then
>>> +	log_skip "secure boot not enabled"
>>> +fi
>>> +
>>> +# get the kernel config
>>> +get_kconfig
>>> +
>>
>> get_kconfig() will be good candidate as a kselftest common
>> function. Is that possible?
> 
> Sure, where would it go?  get_kconfig calls log_skip.  I didn't see
> any common logging functions.  Petr suggested defining a set of common
> logging functions.  Did you want to only make "log_skip" a common
> function or the other logging functions log_pass, log_fail, log_info
> as well?
> 

We can do this as a separate effort in the interest of getting these
in the interest of getting these in.

We have common functions in ksefltest.h for c and we don't have them
for tests scripts. We might be able to collect common routines such
as get_kconfig into a common .sh and include in tests. If you have time
to do this, that will be great. It can be done as a separate effort.

thanks,
-- Shuah
Mimi Zohar Feb. 27, 2019, 6:37 p.m. UTC | #4
> >>> +get_secureboot_mode
> >>> +if [ $? -eq 0 ]; then
> >>> +	log_skip "secure boot not enabled"
> >>> +fi
> >>> +
> >>> +# get the kernel config
> >>> +get_kconfig
> >>> +
> >>
> >> get_kconfig() will be good candidate as a kselftest common
> >> function. Is that possible?
> > 
> > Sure, where would it go?  get_kconfig calls log_skip.  I didn't see
> > any common logging functions.  Petr suggested defining a set of common
> > logging functions.  Did you want to only make "log_skip" a common
> > function or the other logging functions log_pass, log_fail, log_info
> > as well?
> > 
> 
> We can do this as a separate effort in the interest of getting these
> in the interest of getting these in.
> 
> We have common functions in ksefltest.h for c and we don't have them
> for tests scripts. We might be able to collect common routines such
> as get_kconfig into a common .sh and include in tests. If you have time
> to do this, that will be great. It can be done as a separate effort.

Ok.  For now, I'm waiting for some review/ack's.

Mimi
Petr Vorel Feb. 28, 2019, 10:32 p.m. UTC | #5
Hi Mimi,

> While the appended kernel module signature can be verified, when loading
> a kernel module via either the init_module or the finit_module syscall,
> verifying the IMA signature requires access to the file descriptor,
> which is only available via the finit_module syscall.  As "modprobe"
> does not provide a flag allowing the syscall - init_module or
> finit_module - to be specified, this patch does not load a kernel
> module.

> This test simply verifies that on secure boot enabled systems with
> "CONFIG_IMA_ARCH_POLICY" configured, that at least an appended kernel
> module signature or an IMA signature is required based on the Kconfig
> and the runtime IMA policy.

> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>

...
> diff --git a/tools/testing/selftests/ima/test_kernel_module.sh b/tools/testing/selftests/ima/test_kernel_module.sh
...
> +# Are appended signatures required?
> +if [ -e /sys/module/module/parameters/sig_enforce ]; then
> +	sig_enforce=$(cat /sys/module/module/parameters/sig_enforce)
> +	if [ $sig_enforce = "Y" ]; then
> +		log_pass "appended kernel module signature required"
> +	fi
> +fi
Another possible helper [1]:
is_enabled() # or sysfs_enabled
{
	[ -f "$1" ] && [ "$(cat $1)" = "Y" -o "$(cat $1)" = "1" ]
}

is_enabled /sys/module/module/parameters/sig_enforce && \
	log_pass "appended kernel module signature required"

...


Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/blob/master/ver_linux#L30
Petr Vorel Feb. 28, 2019, 11:14 p.m. UTC | #6
Hi Shuah,

> We can do this as a separate effort in the interest of getting these
> in the interest of getting these in.

> We have common functions in ksefltest.h for c and we don't have them
> for tests scripts. We might be able to collect common routines such
> as get_kconfig into a common .sh and include in tests. If you have time
> to do this, that will be great. It can be done as a separate effort.
Some inspiration, what sort of helpers can be for shell see LTP shell API [1].
These helpers tests help tests to be short, readable and with unified output.
(I like library variables [2] which are self describing and making test even
shorter), setup and cleanup functions called automatically, ...
The same applies for C API [3] (e.g. SAFE_*() helpers for handling errors, ...).

I wonder if there is even any interest of having any sort of helpers or even
framework for shell, when so far there was nothing.

> thanks,
> -- Shuah


Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-writing-a-testcase-in-shell
[2] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#232-library-variables
[3] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c
Mimi Zohar March 10, 2019, 5:48 p.m. UTC | #7
On Thu, 2019-02-28 at 23:32 +0100, Petr Vorel wrote:
> Hi Mimi,
> 
> > While the appended kernel module signature can be verified, when loading
> > a kernel module via either the init_module or the finit_module syscall,
> > verifying the IMA signature requires access to the file descriptor,
> > which is only available via the finit_module syscall.  As "modprobe"
> > does not provide a flag allowing the syscall - init_module or
> > finit_module - to be specified, this patch does not load a kernel
> > module.
> 
> > This test simply verifies that on secure boot enabled systems with
> > "CONFIG_IMA_ARCH_POLICY" configured, that at least an appended kernel
> > module signature or an IMA signature is required based on the Kconfig
> > and the runtime IMA policy.
> 
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> ...
> > diff --git a/tools/testing/selftests/ima/test_kernel_module.sh b/tools/testing/selftests/ima/test_kernel_module.sh
> ...
> > +# Are appended signatures required?
> > +if [ -e /sys/module/module/parameters/sig_enforce ]; then
> > +	sig_enforce=$(cat /sys/module/module/parameters/sig_enforce)
> > +	if [ $sig_enforce = "Y" ]; then
> > +		log_pass "appended kernel module signature required"
> > +	fi
> > +fi
> Another possible helper [1]:
> is_enabled() # or sysfs_enabled
> {
> 	[ -f "$1" ] && [ "$(cat $1)" = "Y" -o "$(cat $1)" = "1" ]
> }
> 
> is_enabled /sys/module/module/parameters/sig_enforce && 
> 	log_pass "appended kernel module signature required"
> 

Agreed.  As this is being used only here in the IMA selftests,
deferring making this change until there is a generic common library.

Mimi

Patch
diff mbox series

diff --git a/tools/testing/selftests/ima/Makefile b/tools/testing/selftests/ima/Makefile
index 049c83c9426c..ef5201ff0bea 100644
--- a/tools/testing/selftests/ima/Makefile
+++ b/tools/testing/selftests/ima/Makefile
@@ -4,7 +4,7 @@  uname_M := $(shell uname -m 2>/dev/null || echo not)
 ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/)
 
 ifeq ($(ARCH),x86)
-TEST_PROGS := test_kexec_load.sh test_kexec_file_load.sh
+TEST_PROGS := test_kexec_load.sh test_kexec_file_load.sh test_kernel_module.sh
 TEST_FILES := common_lib.sh
 
 include ../lib.mk
diff --git a/tools/testing/selftests/ima/test_kernel_module.sh b/tools/testing/selftests/ima/test_kernel_module.sh
new file mode 100755
index 000000000000..4009e1b60b03
--- /dev/null
+++ b/tools/testing/selftests/ima/test_kernel_module.sh
@@ -0,0 +1,96 @@ 
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# On secure boot enabled systems with "CONFIG_IMA_ARCH_POLICY" configured,
+# this test verifies that at least an appended kernel module signature or
+# an IMA signature is required.  It does not attempt to load a kernel module.
+
+TEST="KERNEL_MODULE"
+. ./common_lib.sh
+
+trap "{ rm -f $IKCONFIG ; }" EXIT
+
+# Some of the IMA builtin policies may require the kernel modules to
+# be signed, but these policy rules may be replaced with a custom
+# policy.  Only CONFIG_IMA_APPRAISE_REQUIRE_MODULE_SIGS persists after
+# loading a custom policy.  Check if it is enabled, before reading the
+# IMA runtime sysfs policy file.
+# Return 1 for IMA signature required and 0 for not required.
+is_ima_sig_required()
+{
+	local ret=0
+
+	kconfig_enabled "CONFIG_IMA_APPRAISE_REQUIRE_MODULE_SIGS=y" \
+		"IMA kernel module signature required"
+	if [ $? -eq 1 ]; then
+		log_info "IMA kernel module signature required"
+		return 1
+	fi
+
+	# The architecture specific or a custom policy may require the
+	# kernel module to be signed.  Policy rules are walked sequentially.
+	# As a result, a policy rule may be defined, but might not necessarily
+	# be used.  This test assumes if a policy rule is specified, that is
+	# the intent.
+	if [ $ima_read_policy -eq 1 ]; then
+		check_ima_policy "appraise" "func=MODULE_CHECK" \
+			"appraise_type=imasig"
+		ret=$?
+		[ $ret -eq 1 ] && log_info "IMA signature required";
+	fi
+	return $ret
+}
+
+# loading kernel modules requires root privileges
+if [ $(id -ru) -ne 0 ]; then
+	log_skip "requires root privileges"
+fi
+
+# Are appended signatures required?
+if [ -e /sys/module/module/parameters/sig_enforce ]; then
+	sig_enforce=$(cat /sys/module/module/parameters/sig_enforce)
+	if [ $sig_enforce = "Y" ]; then
+		log_pass "appended kernel module signature required"
+	fi
+fi
+
+get_secureboot_mode
+if [ $? -eq 0 ]; then
+	log_skip "secure boot not enabled"
+fi
+
+# get the kernel config
+get_kconfig
+
+# Determine which kernel config options are enabled
+kconfig_enabled "CONFIG_IMA_ARCH_POLICY=y" \
+	"architecture specific policy enabled"
+arch_policy=$?
+
+kconfig_enabled "CONFIG_MODULE_SIG=y" \
+	"appended kernel modules signature enabled"
+appended_sig_enabled=$?
+
+kconfig_enabled "CONFIG_IMA_READ_POLICY=y" "reading IMA policy permitted"
+ima_read_policy=$?
+
+is_ima_sig_required
+ima_sig_required=$?
+
+if [ $arch_policy -eq 0 ]; then
+	log_skip "architecture specific policy not enabled"
+fi
+
+if [ $appended_sig_enabled -eq 1 ]; then
+	log_fail "appended kernel module signature enabled, but not required"
+fi
+
+if [ $ima_sig_required -eq 1 ]; then
+	log_pass "IMA kernel module signature required"
+fi
+
+if [ $ima_read_policy -eq 1 ]; then
+	log_fail "IMA kernel module signature not required"
+else
+	log_skip "reading IMA policy not permitted"
+fi